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.

Sign up for GitHub

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

Jump to bottom

MSC4178: Additional Error Codes for requestToken endpoint #4178

Merged
merged 6 commits into from
Sep 3, 2024
Merged

MSC4178: Additional Error Codes for requestToken endpoint #4178

merged 6 commits into from
Sep 3, 2024

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 16, 2024 •
edited by anoadragon453
Loading

@dbkr dbkr added the client-server Client-Server API label Aug 16, 2024
dbkr added a commit to matrix-org/matrix-react-sdk that referenced this pull request Aug 16, 2024
@dbkr
Support error code for email / phone adding unsupported
9efe162 
matrix-org/matrix-spec-proposals#4178
@dbkr dbkr mentioned this pull request Aug 16, 2024
Closed
4 tasks
dbkr added a commit to element-hq/synapse that referenced this pull request Aug 16, 2024
@dbkr
Return specific error code when email / phone not supported
bcc5a7c 
Implements matrix-org/matrix-spec-proposals#4178
@dbkr dbkr mentioned this pull request Aug 16, 2024
Draft
3 tasks
@turt2live turt2live changed the title Add proposal for unsupported 3pid error code MSC4178: Add proposal for unsupported 3pid error code Aug 16, 2024
@turt2live turt2live added proposal A matrix spec change proposal kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. and removed needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Aug 16, 2024
@turt2live turt2live self-requested a review August 19, 2024 16:01
@dbkr dbkr changed the title MSC4178: Add proposal for unsupported 3pid error code MSC4178: Additional Error Codes for requestToken endpoint Aug 19, 2024
@dbkr
Copy link
Member Author

dbkr commented Aug 19, 2024

@ara4n has given an LGTM in the spec office room, so in hope that this can be a swift and easy win...

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 19, 2024 •
edited by turt2live
Loading

Team member @dbkr has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Aug 19, 2024
anoadragon453
anoadragon453 reviewed Aug 19, 2024
View reviewed changes
proposals/4178-threepid-medium-not-supported.md Outdated Show resolved Hide resolved
proposals/4178-threepid-medium-not-supported.md Outdated Show resolved Hide resolved
proposals/4178-threepid-medium-not-supported.md Outdated Show resolved Hide resolved
proposals/4178-threepid-medium-not-supported.md Outdated Show resolved Hide resolved
proposals/4178-threepid-medium-not-supported.md Show resolved Hide resolved
proposals/4178-threepid-medium-not-supported.md Outdated Show resolved Hide resolved
richvdh
richvdh reviewed Aug 27, 2024
View reviewed changes
proposals/4178-threepid-medium-not-supported.md
Comment on lines +14 to +18
Firstly, Add the `M_THREEPID_MEDIUM_NOT_SUPPORTED` code to be returned by both
[`POST /account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken)
and
[`POST /account/3pid/msisdn/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken),
defined to mean that the homeserver does not support adding a third party identifier of that medium.
Copy link
Member

Choose a reason for hiding this comment

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

slightly wondering if we should extend this to POST /account/3pid/<unknown_medium>/requestToken, but perhaps that's a job for another day.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the whole 3pid management spec could be generalised to arbitrary media, arguably. Might make more sense to do if we ever add another medium.

richvdh
richvdh reviewed Aug 27, 2024
View reviewed changes
proposals/4178-threepid-medium-not-supported.md
Comment on lines +14 to +23
Firstly, Add the `M_THREEPID_MEDIUM_NOT_SUPPORTED` code to be returned by both
[`POST /account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken)
and
[`POST /account/3pid/msisdn/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken),
defined to mean that the homeserver does not support adding a third party identifier of that medium.

Secondly, allow these endpoints to also return `M_INVALID_PARAM`, to indicate that the third party address
was not valid for that medium (eg. not a valid phone number).

For both of these codes, HTTP status code 400 should be used.
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered extending this to the identity server endpoints? /_matrix/identity/v2/validate/email/requestToken etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hadn't really, although only because the need is more obvious for homeservers which commonly may or may not support verifying certain types of identifier. Happy to add this for ID servers too if we'd rather keep symmetry and do it all in one go.

Copy link
Member

Choose a reason for hiding this comment

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

Given FCP has now started, it's probably best to leave this for a future MSC.

@mscbot
Copy link
Collaborator

mscbot commented Aug 28, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Aug 28, 2024
@mscbot
Copy link
Collaborator

mscbot commented Sep 2, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Sep 2, 2024
@anoadragon453 anoadragon453 merged commit 41495d2 into main Sep 3, 2024
1 check passed
@anoadragon453 anoadragon453 deleted the dbkr/threepid-medium-not-supported branch September 3, 2024 10:29
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Sep 4, 2024
@dbkr dbkr mentioned this pull request Sep 9, 2024
Open
3 tasks
@turt2live
Copy link
Member

Spec PR: matrix-org/matrix-spec#1944

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KitsuneRal KitsuneRal KitsuneRal left review comments

@richvdh richvdh richvdh left review comments

@anoadragon453 anoadragon453 anoadragon453 left review comments

@clokep clokep clokep left review comments

@turt2live turt2live Awaiting requested review from turt2live

Assignees
No one assigned
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal spec-pr-in-review A proposal which has been PR'd against the spec and is in review
Projects
Spec Core Team Backlog
Status: Requires spec PR review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dbkr @mscbot @turt2live @clokep @anoadragon453 @richvdh @KitsuneRal