Send password reset from HS: Sending the email #5345
Conversation
This changes the default behaviour of Synapse to send password reset emails itself rather than through an identity server. The reasoning behind the change is to prevent a malicious identity server from being able to initiate a password reset attempt and then answering it, successfully resetting their password, all without the user's knowledge. This also aides in decentralisation by putting less trust on the identity server itself, which traditionally is quite centralised. If users wish to continue with the old behaviour of proxying password reset requests through the user's configured identity server, they can do so by setting email.enable_password_reset_from_is to True in Synapse's config. Users should be able that with that option disabled (the default), password resets will now no longer work unless email sending has been enabled and set up correctly.
Codecov Report
@@ Coverage Diff @@
## anoa/feature_hs_password_resets #5345 +/- ##
====================================================================
- Coverage 63.04% 31.48% -31.57%
====================================================================
Files 341 341
Lines 35637 35767 +130
Branches 5835 5862 +27
====================================================================
- Hits 22468 11260 -11208
- Misses 11598 24138 +12540
+ Partials 1571 369 -1202 |
Codecov Report
@@ Coverage Diff @@
## anoa/feature_hs_password_resets #5345 +/- ##
==================================================================
Coverage ? 22.51%
==================================================================
Files ? 276
Lines ? 31709
Branches ? 5090
==================================================================
Hits ? 7138
Misses ? 24531
Partials ? 40 |
…reset_sending_email
This reverts commit 6d2d3c9.
| @@ -77,7 +77,7 @@ | |||
| ] | |||
|
|
|||
| CONDITIONAL_REQUIREMENTS = { | |||
| "email.enable_notifs": ["Jinja2>=2.9", "bleach>=1.4.2"], | |||
| "email": ["Jinja2>=2.9", "bleach>=1.4.2"], | |||
There was a problem hiding this comment.
This was changed as multiple options (all relating to email) now require these dependencies. If some documentation lists the different pip [...] options, we'll need to update them.
Codecov Report
@@ Coverage Diff @@
## anoa/feature_hs_password_resets #5345 +/- ##
==================================================================
Coverage ? 62.03%
==================================================================
Files ? 341
Lines ? 35772
Branches ? 5863
==================================================================
Hits ? 22192
Misses ? 11976
Partials ? 1604 |
synapse/config/emailconfig.py
Outdated
| ) | ||
| self.enable_password_resets = ( | ||
| self.email_enable_password_reset_from_is | ||
| or (not self.email_enable_password_reset_from_is and email_config != {}) |
There was a problem hiding this comment.
Isn't this just self.enable_password_resets = self.email_enable_password_reset_from_is or email_config != {}?
synapse/config/emailconfig.py
Outdated
| self.email_enable_password_reset_from_is | ||
| or (not self.email_enable_password_reset_from_is and email_config != {}) | ||
| ) | ||
| if email_config == {} and not self.email_enable_password_reset_from_is: |
There was a problem hiding this comment.
| if email_config == {} and not self.email_enable_password_reset_from_is: | |
| if not self.enable_password_resets: |
synapse/config/emailconfig.py
Outdated
| ) | ||
|
|
||
| self.email_validation_token_lifetime = email_config.get( | ||
| "validation_token_lifetime", 15 * 60, |
There was a problem hiding this comment.
Use self.parse_duration so that you can say 15m in the config. We probably want to keep this valid for, like, 1h at least, since email can be quite slow at times.
There was a problem hiding this comment.
Also, should be in milliseconds for consistency
synapse/config/emailconfig.py
Outdated
| self.email_enable_notifs | ||
| or account_validity_renewal_enabled | ||
| or (self.enable_password_resets | ||
| and self.email_enable_password_reset_from_is) |
There was a problem hiding this comment.
| and self.email_enable_password_reset_from_is) | |
| and not self.email_enable_password_reset_from_is) |
No?
synapse/config/emailconfig.py
Outdated
| # make sure we can import the required deps | ||
| import jinja2 | ||
| import bleach | ||
| # prevent unused warnings | ||
| jinja2 | ||
| bleach | ||
|
|
||
| if self.enable_password_resets and not self.email_enable_password_reset_from_is: |
There was a problem hiding this comment.
I think at this point we change enable_password_resets to ``enable_local_passwords_resets`, as all this boolean logic between the two is confusing me
| raise SynapseError(400, "Missing 'id_server' param in body") | ||
|
|
||
| # Have the identity server handle the password reset flow | ||
| ret = yield self.identity_handler.requestEmailToken(**body) |
There was a problem hiding this comment.
I'd prefer if we explicitly pulled stuff out of the dict that we need, it makes it easier to validate and see what's going on
| "email", client_secret, address=email, validated=False, | ||
| ) | ||
|
|
||
| logger.info("Ret is %s", ret) |
| """ | ||
| # Check that this email/client_secret/send_attempt combo is new or | ||
| # greater than what we've seen previously | ||
| ret = yield self.datastore.get_threepid_validation_session( |
| ) | ||
|
|
||
| token_expires = (self.hs.clock.time_msec() + | ||
| self.config.email_validation_token_lifetime * 1000) |
There was a problem hiding this comment.
Can we please keep all synapse times in milliseconds please
| # Save the session_id and send_attempt to the database | ||
| yield self.datastore.upsert_threepid_validation_session( | ||
| "email", email, client_secret, send_attempt, session_id, | ||
| ) |
There was a problem hiding this comment.
Can we not combine both of these DB calls into one txn?
| # every minute | ||
| hs.get_clock().looping_call( | ||
| self.datastore.cull_expired_threepid_validation_tokens, 60 * 1000, | ||
| ) |
There was a problem hiding this comment.
This should probably go in the datastore. Also, lets do this only every 30mins or so.
…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
Based on #5308
This changes the default behaviour of Synapse to send password reset emails itself rather than through an identity server. The reasoning behind the change is to prevent a malicious identity server from being able to initiate a password reset attempt and then answering it, successfully resetting their password, all without the user's knowledge. This also aides in decentralisation by putting less trust on the identity server itself, which traditionally is quite centralised.
If users wish to continue with the old behaviour of proxying password reset requests through the user's configured identity server, they can do so by setting email.enable_password_reset_from_is to True in Synapse's config.
Users should be able that with that option disabled (the default), password resets will now no longer work unless email sending has been enabled and set up correctly.