Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove account_threepid_delegates option #5881

Open
anoadragon453 opened this issue Aug 19, 2019 · 21 comments
Open

Remove account_threepid_delegates option #5881

anoadragon453 opened this issue Aug 19, 2019 · 21 comments
Assignees
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases z-privacy-sprint (Deprecated Label)

Comments

@anoadragon453
Copy link
Member

With #5835, we're allowing homeservers to be able to send their own registration emails. Earlier, we allowed homeservers to be able to send password reset emails themselves. Both of these options were just considered temporary to migrate people over from relying on vector.im or another identity server to handle SMTP, to handling SMTP themselves if they required the functionality.

Once a sufficient amount of time has passed, we should remove these options and yell from the mountains that people need to manage SMTP from their Synapse install or disable it altogether.

@richvdh richvdh removed the phase:2 label Oct 1, 2020
@richvdh richvdh added the Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases label Apr 7, 2021
@erikjohnston
Copy link
Member

Removing trust_identity_server_for_password_resets seems fine, but I'm less sure about account_threepid_delegate. We don't seem to mention in the sample config that using it is delegated (its just in the UPGRADE notes), and is also currently the only way of support validating phone numbers?

@richvdh
Copy link
Member

richvdh commented Jul 15, 2021

sigh. That's annoying, as it makes #9677 harder.

@erikjohnston
Copy link
Member

erikjohnston commented Jul 15, 2021

We should figure out if MSISDN is still a thing that is a) used and b) we want to continue to support. It would be nice to strip all this out if its not being used. We should pull the numbers out for how often msisdn is used in Sydent.

Otherwise, we need to port the code to use the new v2 sydent API (or make this work with v2 API)

@anoadragon453
Copy link
Member Author

anoadragon453 commented Jul 15, 2021

We've identified one Element customer that still makes use of this option, though has not had a msisdn registration on their identity server since March 2021. So this may no longer be a concern for any known entity.

However, the context for why some wish to make use of this feature is to improve UX when creating mappings on an identity server for every user is a necessity - rather than something that can happen lazily and at the user's request.

Essentially, if you need 3PID<->MXID mappings to appear on your identity server during user registration (without asking the user to validate their 3PID twice - once for the hs and another time for the is), this feature - plus some Sydent modifications - is one way of going about it.

DINUM were using account_threepid_delegates (for delegation of email verification), but are no longer. Their need was to have the identity server be aware of user's emails - but this has been sidestepped by a little hack to instead use Sydent's internal bind API to register 3PID mappings. They also do not validate msisdns.

Of course it's also nice to allow users to add their phone numbers to their homeserver account. And in Element's UI today, you can't add a phone number to an identity server without first adding it to your homeserver. TravisR has also helpfully noted that Element Web does still support msisdn verification, but matrix.org does not present the UIAA stage as an option to trigger it.

Removing trust_identity_server_for_password_resets seems fine

I'm not aware of anyone else using trust_identity_server_for_password_resets either.

@erikjohnston erikjohnston added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Jul 26, 2021
@richvdh richvdh changed the title Remove trust_identity_server_for_password_resets and account_threepid_delegate options Remove trust_identity_server_for_password_resets and account_threepid_delegates options Aug 5, 2021
@richvdh richvdh changed the title Remove trust_identity_server_for_password_resets and account_threepid_delegates options Remove account_threepid_delegates option Aug 5, 2021
@richvdh
Copy link
Member

richvdh commented Aug 5, 2021

I've created a separate issue (#10545) to handle removing trust_identity_server_for_password_reset, since that's rather easier.

@richvdh
Copy link
Member

richvdh commented Aug 6, 2021

However, the context for why some wish to make use of this feature is to improve UX when creating mappings on an identity server for every user is a necessity - rather than something that can happen lazily and at the user's request.

Essentially, if you need 3PID<->MXID mappings to appear on your identity server during user registration (without asking the user to validate their 3PID twice - once for the hs and another time for the is), this feature - plus some Sydent modifications - is one way of going about it.

Having investigated a bit, I'm reasonably sure that this is a red herring. In particular, even if you enable account_threepid_delegates, the IS still isn't told about the MXID for the user that is registering, so can't store a 3PID<->MXID mapping.

In situations where a single organisation controls both the HS and the IS, and wants registration on one to automatically cause a bind on the other, I think the correct approach is Sydent's internal bind API. We could look into implementing support for this in Synapse; alternatively I think it's pretty easily done with a module.

@richvdh
Copy link
Member

richvdh commented Aug 6, 2021

Let me try to summarize where we stand.

Background

account_threepid_delegates controls whether Synapse performs a threepid validation itself, or delegates that operation to an identity server. In the case of email, Synapse has supported doing its own validation since v1.4.0, but it still has no support for doing phone number validation itself.

Synapse performs threepid validation in the following scenarios:

  • When adding a threepid to an existing homeserver account (eg via the "account settings" page). It's worth noting that element-web and matrix.org allow adding either email addresses or phone numbers via this mechanism.
  • When adding a threepid to a new homeserver account during registration. In this case, element-web supports it if it's enabled by the HS, but matrix.org has disable_msisdn_registration: true which inhibits its use.
  • When performing a password reset, via a threepid that you have previously added to your account. In this case, only email works: despite the presence of POST /_matrix/client/r0/account/password/msisdn/requestToken in the spec, Synapse does not implement that endpoint, and requires an email validation.

Sidebar: what are the reasons that a user might want to add an email address or phone number to their homeserver account?

  1. Element-web makes it a prerequisite for adding those addresses to an identity server, which is required for that user to be discoverable via the email address or phone number.
  2. It allows an alternative mechanism for logging in (you can provide an email address or msisdn instead of username.)
  3. In the case of email, but /not/ phone number, it provides a password recovery mechanism.
  4. In the case of email, but /not/ phone number, it means that you can receive notifications of missed messages, etc, via email.

We announced that we would turn off support for threepid validation delegation in Sydent in December 2019, so we're 18 months overdue. Unfortunately, we can't do that right now, because there remains a valid usecase for it:

Adding phone numbers to HS accounts

As above, Synapse does not support doing its own validation of phone numbers, so currently whenever you add a phone number to your HS Account, it calls out to an identity server to vaidate it.

I think first we need to decide if supporting MSISDNs on HS accounts is actually a useful feature - how many people actually use it for logging in? (Or rather, how many new users would miss the ability to do so, since we're not talking about breaking existing MSISDN<->user associations).

Likewise, how many people use MSISDN as a way to look up other users?

I think we have four options here:

  1. Completely drop support for adding new MSISDNs to homeserver accounts and Identity Servers.
  2. Support adding MSISDNs to Identity Servers, for user discovery, but not Homeservers (for login, presumably). This would require development on the Element "account settings" page but is otherwise supported by the protocol and Synapse today.
  3. Full support as today - two suboptions:
    3a. Add support for MSISDN validation (via an SMS aggregator) to Synapse.
    3b. Roll back on our stated intention to turn off support for threepid validation delegation and reintroduce the API endpoints that it requires back into the spec (or design and use new ones).

@babolivier
Copy link
Contributor

Likewise, how many people use MSISDN as a way to look up other users?

I think mobile clients (at least Element) use this to try and discover Matrix users from your phone's contact? Which sounds like something we probably want to keep supporting.

@callahad callahad assigned callahad and unassigned richvdh Aug 12, 2021
@callahad callahad added this to the Revisit: Monthly milestone Sep 15, 2021
@callahad
Copy link
Contributor

callahad commented Oct 7, 2021

@callahad to discuss this with Element product folks to make sure we're not missing anything

@DMRobertson
Copy link
Contributor

@callahad to discuss this with Element product folks to make sure we're not missing anything

Bump. Any news?

@callahad
Copy link
Contributor

Let's have @neilisfragile + @nadonomy + @richvdh chat about this one and find a conclusion...

@callahad callahad assigned neilisfragile and unassigned callahad Apr 21, 2022
@callahad
Copy link
Contributor

Neil to own scheduling that chat... 🔥 🥔 💨

@richvdh
Copy link
Member

richvdh commented Jul 12, 2022

#13192 drops support for delegation of email validation, which means we now only need to worry about phone number validation.

#5881 (comment) sets out the situation there, but to summarize, we need to do one of:

  1. Completely drop support for adding new MSISDNs to homeserver accounts and Identity Servers.
  2. Support adding MSISDNs to Identity Servers, for user discovery, but not Homeservers (for login, presumably). This would require development on the Element "account settings" page but is otherwise supported by the protocol and Synapse today.
  3. Full support as today - two suboptions:
    3a. Add support for MSISDN validation (via an SMS aggregator) to Synapse.
    3b. Roll back on our stated intention to turn off support for threepid validation delegation and reintroduce the API endpoints that it requires back into the spec (or design and use new ones).

It is worth noting that this overlaps with the OIDC work to some extent, in that in future it will be up to the OIDC authentication server to manage login. Anyway, this is all covered by element-hq/element-meta#478.

@reivilibre
Copy link
Contributor

It appears that ma1sd might support SMS validation; I don't know if it uses the affected APIs :/ https://github.com/ma1uta/ma1sd

@richvdh
Copy link
Member

richvdh commented Oct 13, 2022

It appears that ma1sd might support SMS validation; I don't know if it uses the affected APIs :/ ma1uta/ma1sd

I think this is ma1uta/ma1sd#114, but also somewhat orthogonal to whether Synapse uses the deprecated APIs (noting that this is a Synapse issue)

@reivilibre
Copy link
Contributor

I meant more along the lines of: ma1sd might provide the affected APIs and expect Synapse to use them in order to provide its behaviour. If you've already opened an issue then that's fine with me

@DMRobertson
Copy link
Contributor

Coming back over this during triage and not sure what the next steps are. Looks like we need to decide which of Rich's options here we take?

@richvdh
Copy link
Member

richvdh commented Nov 4, 2022

Yes. Still.

Given that matrix.org's/vector.im's sydents haven't supported MSISDN validation for several months (and afaik nobody else supports it because it requires a commercial agreement with openmarket), I think there's a strong argument that we could now rip out synapse's support for delegation with no loss of functionality.

@richvdh richvdh mentioned this issue Apr 27, 2023
@MadLittleMods
Copy link
Contributor

MadLittleMods commented Jun 8, 2023

Trying to summarize some notes from the triage meeting.

  • We've been told to hang onto this because we may want to use phone numbers (MSISDN) in the future (@erikjohnston has the context around discussions). But in the future, product will probably want X from phone numbers which isn't what we're doing today so why hang onto this implementation?
  • As @richvdh mentioned, this feature requires an OpenMarket contract which is a very specific distinction. Are people really likely to get a contract just to do this?
  • Most people have work emails, not work phone numbers.
  • It's been disabled on matrix.org forever (unclear with vector.im, I'm getting mixed signals from the triage meeting vs the comments in the issue) https://github.com/matrix-org/matrix-ansible-private/pull/6930 seems to indicate disabled on vector.im as well.
  • This isn't a simple case of removing X because we would also want to remove the v1 API's and that's a lot of work.
  • This use case is probably best covered by OIDC anyway so maybe we don't have to do anything here and just wait it out.

@richvdh
Copy link
Member

richvdh commented Jun 8, 2023

This isn't a simple case of removing X because we would also want to remove the v1 API's and that's a lot of work.

I'm not sure what X refers to, but presumably "v1 APIs" refers to sydent's support for the V1 ID server api (matrix-org/sydent#338), of which this synapse feature is one, but not the only, blocker.

@richvdh
Copy link
Member

richvdh commented Jun 8, 2023

  • As @richvdh mentioned, this feature requires an OpenMarket contract which is a very specific distinction. Are people really likely to get a contract just to do this?

In theory it's possible someone could implement another service which implements the V1 IS API and verifies phone numbers a different way, and hence use this synapse feature with it. I find it very unlikely, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases z-privacy-sprint (Deprecated Label)
Projects
None yet
Development

No branches or pull requests

9 participants