Skip to content

Rename threepidCreds to threepid_creds and get rid of array #3471

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

Conversation

deepbluev7
Copy link
Contributor

@deepbluev7 deepbluev7 commented Nov 3, 2021

This fixes the behaviour to match what synapse implements in practice.
If you use threepidCreds, you will just get an error about a missing
threepid_creds field. Synapse also treats this as an object. All clients
also seem to send threepid_creds, if they work on Synapse. Since
matrix.org requires an email currently for registration, most clients
that implement registration, will hit this issue.

https://github.com/matrix-org/synapse/blob/a0f48ee89d88fd7b6da8023dbba607a69073152e/synapse/handlers/ui_auth/checkers.py#L145

fixes #3156
fixes #2189

Signed-off-by: Nicolas Werner nicolas.werner@hotmail.de

Note that there is a lot of debate about this in the different issues. I really don't care about how this is resolved, but after 4 years, can please someone either fix this in synapse or matrix-doc? I just wasted half a day on this, the 3pid stuff in UIA is complicated enough. Alternatively I can add a note in the spec, that this is still up for debate and clients should send multiple versions of the auth dict, until one of them works or something, that at least suggests, that the spec does not match reality.

Current reality is that clients use an object called threepid_creds, Synapse uses it. It is probably easier to change the spec at this point. But I haven't checked what other servers do, I am still doing that.

Implementation threepid_creds object threepidCreds object threepidCreds array threepid_creds array
Spec
Element/Web
Element/Android
Element/iOS
Synapse
Conduit
Ruma
Dendrite
FluffyChat
Nheko

(fluffy actually seems to send both spelling variations as an array... Ruma added that variant now...)
(Nheko doesn't care, I'll implement whatever we decide on, but I just fixed it to work on Synapse today.)
(Couldn't find anything in libQuotient)

Preview: https://pr3471--matrix-org-previews.netlify.app

This fixes the behaviour to match what synapse implements in practice.
If you use threepidCreds, you will just get an error about a missing
threepid_creds field. Synapse also treats this as an object. All clients
also seem to send threepid_creds, if they work on Synapse. Since
matrix.org requires an email currently for registration, most clients
that implement registration, will hit this issue.

https://github.com/matrix-org/synapse/blob/a0f48ee89d88fd7b6da8023dbba607a69073152e/synapse/handlers/ui_auth/checkers.py#L145

fixes matrix-org#3156
fixes matrix-org#2189

Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@deepbluev7 deepbluev7 marked this pull request as ready for review November 3, 2021 17:31
deepbluev7 added a commit to Nheko-Reborn/nheko that referenced this pull request Nov 3, 2021
This was a bit of a journey:
matrix-org/matrix-spec-proposals#3471
But it should work now and we now use the UIAHandler everywhere.

fixes #670
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
Signed-off-by: Nicolas Werner <nicolas.werner@hotmail.de>
@turt2live turt2live requested a review from a team November 3, 2021 18:02
@deepbluev7
Copy link
Contributor Author

I couldn't find anything about this in the openapi spec, so I am assuming there are no changes needed there.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg yespls

@richvdh
Copy link
Member

richvdh commented Nov 4, 2021

(sorry ruma and fluffy, but this is the right decision for the reasons set out at #2189 (comment))

@deepbluev7
Copy link
Contributor Author

Ruma seems to be happy to fix this, from when I talked to them. Fluffy I'll fix myself, so this is fine, I think. ;-)

@richvdh richvdh merged commit 938354b into matrix-org:main Nov 4, 2021
huangred pushed a commit to huangred/matrix_api_lite that referenced this pull request Jan 10, 2022
For context, see matrix-org/matrix-spec-proposals#3471

Basically the spec was wrong and didn't match what clients and servers
were doing. Might fix registration on matrix.org.

BREAKING CHANGE: Any client that implements the email portion will fail
to build now.
td-famedly pushed a commit to famedly/dart_matrix_api_lite that referenced this pull request Mar 22, 2024
For context, see matrix-org/matrix-spec-proposals#3471

Basically the spec was wrong and didn't match what clients and servers
were doing. Might fix registration on matrix.org.

BREAKING CHANGE: Any client that implements the email portion will fail
to build now.
td-famedly pushed a commit to famedly/dart_matrix_api_lite that referenced this pull request Mar 22, 2024
For context, see matrix-org/matrix-spec-proposals#3471

Basically the spec was wrong and didn't match what clients and servers
were doing. Might fix registration on matrix.org.

BREAKING CHANGE: Any client that implements the email portion will fail
to build now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

m.login.email.identity example has threepidCreds defined as an array, not an object Docs list wrong key threepidCreds for email/id based user auth
2 participants