Send password reset from HS: database stuff by anoadragon453 · Pull Request #5308 · matrix-org/synapse

anoadragon453 · 2019-05-31T17:16:24Z

Database component of new behaviour of sending password reset emails from Synapse instead of Sydent.

Allows one to store threepid validation sessions along with password reset token attempts and retrieve them again.

Relevant spec bits:

https://matrix.org/docs/spec/client_server/unstable.html#post-matrix-client-r0-account-password-email-requesttoken
https://matrix.org/docs/spec/identity_service/r0.1.0.html#post-matrix-identity-api-v1-validate-email-submittoken

Essentially the flow is:

A request to /requestToken is made with an email address, a client secret and a send_attempt to the homeserver
We create a new session if one doesn't already exist, but if it does check that the send_attempt is higher than the last one in the session. If it is, overwrite it.
We send out an email. In that email is a link containing the client secret, session id and a newly generated token
The user then clicks that link which leads to /submitToken on the homeserver, and we check if those three vars match up with a session and a validation request
If so, the user's threepid session is marked as valid and is shown a happy validation page

codecov · 2019-05-31T17:38:06Z

Codecov Report

Merging #5308 into anoa/feature_hs_password_resets will decrease coverage by 0.28%.
The diff coverage is 21.66%.

@@                         Coverage Diff                         @@
##           anoa/feature_hs_password_resets    #5308      +/-   ##
===================================================================
- Coverage                            63.04%   62.76%   -0.29%     
===================================================================
  Files                                  341      341              
  Lines                                35637    35536     -101     
  Branches                              5835     5816      -19     
===================================================================
- Hits                                 22468    22304     -164     
- Misses                               11598    11662      +64     
+ Partials                              1571     1570       -1

erikjohnston · 2019-06-04T09:49:47Z

New schema change should go in 55/

erikjohnston · 2019-06-04T10:48:35Z

+        """Remove threepid validation tokens with expiry dates that have passed"""
+        def cull_expired_threepid_validation_tokens_txn(txn, ts):
+            sql = ("DELETE FROM threepid_validation_token WHERE "
+                   "expires < ?")


Can you use multiline strings please, it makes it easier to change/c+p etc

erikjohnston · 2019-06-04T10:49:47Z

+    expires BIGINT NOT NULL
+);
+
+CREATE INDEX threepid_validations_session_id ON threepid_validation_session(session_id);


This will create a duplicate index I believe, since PRIMARY KEY will create a unique index.

erikjohnston · 2019-06-04T10:49:53Z

+
+CREATE INDEX threepid_validations_session_id ON threepid_validation_session(session_id);
+
+CREATE INDEX threepid_validation_token_session_id ON threepid_validation_token(session_id);


…Fix some bugs

erikjohnston

Just fix up the sql a bit and then merge into your base branch

erikjohnston · 2019-06-05T09:37:28Z

+        Args:
+            medium (str): The medium of the 3PID
+            address (str): The address of the 3PID
+            sid (str): The ID of the validation session


What does it mean if these are None?

erikjohnston · 2019-06-05T09:38:03Z

+
+        if sid:
+            keyvalues["session_id"] = sid
+        elif address:


elif? If we expect that only one is set, then let's assert that

erikjohnston · 2019-06-05T09:40:11Z

+                "client_secret", "last_send_attempt", "validated_at",
+            ]
+
+            sql = "SELECT %s FROM threepid_validation_session" % ", ".join(cols_to_return)


Just stick these in the path rather than string concatenating them, see below

erikjohnston · 2019-06-05T09:41:06Z

+            # Convert the resulting row to a dictionary
+            ret = {}
+            for i in range(len(cols_to_return)):
+                ret[cols_to_return[i]] = row[i]


We have helper function that uses the returned data structure to do this for us. Instead of txn.fetchone():

rows = self.cursor_to_dict(txn) if not rows: return None return rows[0]

erikjohnston · 2019-06-05T09:41:41Z

+        if not row:
+            raise ThreepidValidationError(
+                400, "Validation token not found or has expired",
+            )


If we're are conditionally doing stuff we should do this in a txn.

…identity server (#5377) Sends password reset emails from the homeserver instead of proxying to the identity server. This is now the default behaviour for security reasons. If you wish to continue proxying password reset requests to the identity server you must now enable the email.trust_identity_server_for_password_resets option. This PR is a culmination of 3 smaller PRs which have each been separately reviewed: * #5308 * #5345 * #5368

anoadragon453 added 5 commits May 31, 2019 18:09

Send password reset from HS: database stuff

16bad34

Add changelog

00c0edd

0 != false in SQL land

4e319de

Add method to delete a validation session

f2c8ec6

session_id should not be NULL

81f42c7

anoadragon453 requested a review from a team June 3, 2019 15:02

erikjohnston suggested changes Jun 4, 2019

View reviewed changes

anoadragon453 added 4 commits June 4, 2019 17:00

Move db changes to new delta 55. Make validated_at a unix timestamp. …

c1dfe8e

…Fix some bugs

Remove duplicate indexes, use multiline str, simplify deferreds

df6ec0a

Define ThreepidValidationError

853523b

Allow filtering by 'validated_at' when querying 3PID validation sessions

99ada1f

anoadragon453 changed the base branch from develop to anoa/feature_hs_password_resets June 4, 2019 17:33

anoadragon453 mentioned this pull request Jun 4, 2019

Send password reset from HS: Sending the email #5345

Merged

anoadragon453 requested a review from erikjohnston June 4, 2019 19:10

erikjohnston approved these changes Jun 5, 2019

View reviewed changes

Address review comments

8f9daa4

anoadragon453 force-pushed the anoa/hs_password_reset_database branch from 1505cd6 to 8f9daa4 Compare June 5, 2019 11:16

anoadragon453 added 2 commits June 5, 2019 12:51

Don't concat strings

62be823

Add comma

f194bf7

anoadragon453 merged commit 24f31df into anoa/feature_hs_password_resets Jun 5, 2019

anoadragon453 deleted the anoa/hs_password_reset_database branch June 5, 2019 12:29

anoadragon453 mentioned this pull request Jun 6, 2019

Add ability to perform password reset via email without trusting the identity server #5377

Merged


		CREATE INDEX threepid_validations_session_id ON threepid_validation_session(session_id);

		CREATE INDEX threepid_validation_token_session_id ON threepid_validation_token(session_id);

Uh oh!

Conversation

anoadragon453 commented May 31, 2019

Uh oh!

codecov Bot commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

erikjohnston commented Jun 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 31, 2019 •

edited

Loading