Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Synapse to send registration emails + choose Synapse or an external server to handle 3pid validation #5987

Merged
merged 11 commits into from Sep 6, 2019

Conversation

@anoadragon453
Copy link
Member

commented Sep 5, 2019

This is a combination of a few different PRs, finally all being merged into develop:

  • #5875
  • #5876
  • #5868 (This one added the /versions flag but the flag itself was actually backed out in #5969. What's left is just giving /versions access to the config file, which could be useful in the future)
  • #5835
  • #5969
  • #5940

Clients should not actually use the new registration functionality until #5972 is merged.

UPGRADE.rst, changelog entries and config file changes should all be reviewed closely before this PR is merged.

Part of #5835

Removes the concept of a trusted identity server. The original concept of having the homeserver keep a list of trusted identity servers was to mitigate the danger of having a malicious IS handling password reset or registration emails. Since #5835 gives the homeserver the ability to do both of these things itself, as well as the requirement for it to choose an external, trusted identity server if it so chooses, the homeserver no longer needs to constrain which identity servers are chosen (which was traditionally a choice given to the client).

Thus, we can safely the functionality of `trusted_third_party_id_servers`. It does need to stay in the config file for the foreseeable though, as it is currently used by a background job for old 3PIDs, which were bound before Synapse tracked which IS a 3PID was bound to. The identity servers in `trusted_third_party_id_servers` are likely candidates to be where a user registered their 3PID, so this is used during the background update.

This background job was added in v0.99.4, so we're catering for those still updating from before v0.99.4.
…id_delegate (#5876)

Replaces the trust_identity_server_for_password_resets boolean option with a account_threepid_delegate str option that defines which identity server to use to handle password resets and registration if the homeserver does not want to or is unable to handle these tasks itself.

Having this option being something other than null or "" gives the same indication as True for trust_identity_server_for_password_resets.

The domain of the identity server is actually used in #5835
#5868)

When a client is registering an account, they need to know whether they should supply an `id_server` param (the contents being the domain of an identity server) to the server in order to specify which `id_server` to send their email from.

Beginning from the branch this PR is getting merged into, the homeserver has the capability to send registration emails, as well as instead specify which identity server should send them. There's no longer any need for the client to specify an identity server here.

This flag will also be used in other cases, such as password reset and binding 3PIDs, so it's preferable to not just put it in the registration parameters.

We will remove this flag in the future when the spec drops support from needing `id_server` in `/register` and other endpoints, and Synapse drops support for older spec versions.
…il (#5835)

~~Fixes #5833 Moved out to ~~#5863
Part of fixing #5751

Decouples the activity of sending registration emails and binding them to an identity server.

This PR simply sends the registration email, but clicking it does not approve the user for registration. That will come in PR #2.

Some of this makes use of existing stuff for sending password reset emails from Synapse. Some work was done to make that stuff even more generic.

Upgrade notes:
* There is a new top-level config option, `account_threepid_delegate` which defines the address of an identity server that you would like to send registration/password reset emails on your behalf.

  The option `email.trust_identity_server_for_password_resets` has been replaced by this. If you set `email.trust_identity_server_for_password_resets` in your config to `true`, please remove it and configure `account_threepid_delegate` instead. The [sample config](https://github.com/matrix-org/synapse/blob/anoa/reg_email_sending_email/docs/sample_config.yaml) has information on how to configure it.

Note: This PR does not allow homeservers to send emails when simply adding an email to your account. That will come after this and will be blocked on a new MSC.

Part [2/2] will be successfully completing the registration step when `/submit_token` is hit with the correct details, and clearing out the `password_servlet` flag stuff, which is no longer needed. Will be a much smaller PR than this one.

~~Requires #5863 has been merged into the base branch.
~~Requires #5876 has been merged into the base branch.
`account_threepid_delegate` was an option added as part of this privacy sprint for the homeserver admin to declare which identity server (or any server handling third-party verification requests) they'd like to use to send email/sms messages on their behalf for the purposes of user registration and password resets.

We realized however, that while admins would want to set this option to `""` (allow Synapse to handle email sending itself), some homeservers have users with bound phone numbers, and setting `account_threepid_delegate` to `""` would prevent them from having any phone number verification, since Synapse does not at this time support sending SMS messages.

So, seeing as a common use case would be to have Synapse handle email verification, but an external server handle MSISDN verification, we split `account_threepid_delegate` into a dictionary, and called it `account_threepid_delegates` instead. This contains two keys as of present, `email` and `msisdn`. You can then set either to an external server of your choice, or `""` for Synapse to attempt to handle it.
…erification (#5940)

Fixes: #5751
Fixes: #5928

#5835 allowed Synapse to send registration emails to the user. Now we need to accept them and have it succeed the `m.login.email.identity` registration flow step.

`account_threepid_handler` will also be switched from a `str` in the config file to a dictionary which contains entries for `msisdn` and `email`, each with their own `str`. This will let people use an external server to handle `msisdn` registration and password reset requests, while using Synapse for email-based things.

And the `password_servlet` hack that was introduced in https://github.com/matrix-org/synapse/pull/5377/files#diff-b8464485d36f6f87caee3f4d82524213R189 to distinguish a registration call from a password reset call will be removed.
@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Sep 5, 2019
@anoadragon453 anoadragon453 marked this pull request as ready for review Sep 5, 2019
@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Sep 5, 2019
@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Actually, we may want to add some text to INSTALL.md as well.

@richvdh

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@anoadragon453 what's actually new in this PR that hasn't already been reviewed?

@richvdh richvdh removed the request for review from matrix-org/synapse-core Sep 6, 2019
@anoadragon453 anoadragon453 merged commit 0c0b82b into develop Sep 6, 2019
20 checks passed
20 checks passed
buildkite/synapse Build #4087 passed (25 minutes, 8 seconds)
Details
buildkite/synapse/check-sample-config Passed (1 minute, 20 seconds)
Details
buildkite/synapse/check-style Passed (1 minute, 17 seconds)
Details
buildkite/synapse/isort Passed (33 seconds)
Details
buildkite/synapse/mypy Passed (1 minute, 20 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (17 seconds)
Details
buildkite/synapse/packaging Passed (32 seconds)
Details
buildkite/synapse/pipeline Passed (9 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Passed (11 minutes, 39 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Passed (9 minutes, 5 seconds)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite-slash-old-deps Passed (22 minutes, 59 seconds)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Passed (9 minutes, 31 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Passed (11 minutes, 17 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Passed (11 minutes, 47 seconds)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Passed (9 minutes, 24 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-monolith Passed (6 minutes, 8 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-postgres-9-dot-6-slash-workers Passed (7 minutes, 24 seconds)
Details
buildkite/synapse/sytest-python-3-dot-5-slash-sqlite-slash-monolith Passed (5 minutes, 19 seconds)
Details
buildkite/synapse/trigger-webhook Passed (7 seconds)
Details
coverage/coveralls Coverage decreased (-0.08%) to 64.59%
Details
Homeserver Task Board automation moved this from In progress to Done Sep 6, 2019
@anoadragon453 anoadragon453 deleted the anoa/reg_email branch Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.