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

MSC4183: Additional Error Codes for submitToken endpoint #4183

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 28, 2024

@dbkr dbkr added the proposal A matrix spec change proposal label Aug 28, 2024
@turt2live turt2live added client-server Client-Server API 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. identity service and removed client-server Client-Server API labels Aug 28, 2024
@turt2live
Copy link
Member

I don't feel as though implementation is required for this MSC.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Aug 28, 2024

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

Concerns:

  • lack of clarity about changes to requestToken

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.

@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 28, 2024
@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 needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Aug 28, 2024
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Aug 28, 2024
@dbkr dbkr changed the title MSC4180: Additional Error Codes for submitToken endpoint MSC4183: Additional Error Codes for submitToken endpoint Aug 29, 2024
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Nov 5, 2024

@mscbot concern lack of clarity about changes to requestToken

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Nov 5, 2024
@turt2live turt2live requested a review from richvdh November 26, 2024 01:15
Comment on lines 37 to 39
Also change the corresponding submitYoken-like API given in the C/S API's definition of [`POST
/_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken)
to specify that both request and response parameters / error codes are the same as the I/S API version, rather than just parameters, apart from needing an I/S API token.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Also change the corresponding submitYoken-like API given in the C/S API's definition of [`POST
/_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken)
to specify that both request and response parameters / error codes are the same as the I/S API version, rather than just parameters, apart from needing an I/S API token.
Also change the `submit_url` field in the response to [`POST
/_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken),
to specify that response parameters and error codes are the same as the I/S API version, as well as request parameters.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this, by the way? AFAICT synapse currently returns a bit of HTML here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the sample impl (element-hq/synapse#17625) actually changes the email submit-token endpoint.

Copy link
Member Author

@dbkr dbkr Nov 27, 2024

Choose a reason for hiding this comment

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

So the deal here is that the POST endpoint isn't proxied by Synapse: it doesn't need to be as Sydent's email can just link straight to its own version of the GET endpoint. I've tried to clarify this in 63dcbe0 (it's different for phone numbers because Sydent sends a code for the user to enter rather than a link).

dbkr and others added 2 commits November 26, 2024 17:59
Co-authored-by: Richard van der Hoff <[email protected]>
and how it isn't really a thing in practice
endpoint in the C/S API also specifies a `submit_url` response parameter, defining its parameters to be the same as the
Identity API's `submitToken` endpoints. Everything this MSC specifies applies to this endpoint in the same way.

Note that the email `submitToken` endpoint ([`POST
Copy link
Member

Choose a reason for hiding this comment

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

it's a bit confusing that we jump from talking about an endpoint in the C-S API to one in the IS API without mentioning that.

Also, if you're talking specifically about the POST variant, you better say that, rather than saying that the entire endpoint is unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - sort of, I don't think there is a POST variant for email on the C/S API. Hopefully clearer in af1dc86.

Copy link
Member

Choose a reason for hiding this comment

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

This is still confusing: we still jump from talking about an endpoint in the C-S API to one in the IS API without mentioning that.

Comment on lines 42 to 44
Also change the `submit_url` field in the response to [`POST
/_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken),
to specify that response parameters and error codes are the same as the I/S API version, as well as request parameters.
Copy link
Member

Choose a reason for hiding this comment

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

why does this only apply to POST /_matrix/client/v3/account/3pid/email/requestToken, and not also:

  • POST /_matrix/client/v3/account/password/email/requestToken
  • POST /_matrix/client/v3/account/password/msisdn/requestToken
  • POST /_matrix/client/v3/register/email/requestToken
  • POST /_matrix/client/v3/register/msisdn/requestToken
  • POST /_matrix/client/v3/account/3pid/msisdn/requestToken

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, yeah - I'd done it as only editing what I needed to, but you're right, they should be consistent. Changed in 0f43a51.

dbkr added 2 commits November 29, 2024 09:34
...from the other requestToken enpoints

Also try to further clarify the note about the largely unused POST email submitToken
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved

## Proposal

Add the following specific error code as a code that can be returned by the two endpoints given above:
Copy link
Member

Choose a reason for hiding this comment

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

not least because there are now at least three different endpoints given above, I think you should restate which endpoints are changing here.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +46 to +51
* [`POST /_matrix/client/v3/register/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3registeremailrequesttoken)
* [`POST /_matrix/client/v3/register/msisdn/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3registerrequesttoken)
* [`POST /_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken)
* [`POST /_matrix/client/v3/account/3pid/msisdn/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidmsisdnrequesttoken)
* [`POST /_matrix/client/v3/account/password/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3accountpasswordemailrequesttoken)
* [`POST /_matrix/client/v3/account/password/msisdn/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3accountpasswordmsisdnrequesttoken)
Copy link
Member

Choose a reason for hiding this comment

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

haven't checked this morning, but I'm pretty sure your synapse implementation PR doesn't change all of these.

(I don't think synapse returns a submit_url from the email/requestToken endpoints, but it does from the msisdn/requestToken endpoints, and you haven't updated all the implementations of those submit_url endpoints.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It actually does (so it's just as well we included the other endpoints): the various different rest classes use that function in the handler to do the request to the ID server and then the error code is in the exception that it throws.

proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

re-asserting approval as a read marker

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge identity service kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. unresolved-concerns This proposal has at least one outstanding concern
Projects
Status: Ready for FCP ticks
Development

Successfully merging this pull request may close these issues.

6 participants