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

MSC4183: Additional Error Codes for submitToken endpoint #4183

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

MSC4183: Additional Error Codes for submitToken endpoint #4183

wants to merge 15 commits into from
+65 −0

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Aug 28, 2024 •
edited by turt2live
Loading

@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 •
edited
Loading

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 and others added 3 commits August 29, 2024 11:08
@dbkr @Johennes
Right MSC number
7fb14aa 
Co-authored-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@dbkr
Rename
725f2ec
@dbkr
Add http status code
d383285
@dbkr dbkr changed the title MSC4180: Additional Error Codes for submitToken endpoint MSC4183: Additional Error Codes for submitToken endpoint Aug 29, 2024
anoadragon453
anoadragon453 reviewed Oct 1, 2024
View reviewed changes
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
although that MSC is for `requestToken` on the C/S API only.

The [`POST /_matrix/client/v3/account/3pid/email/requestToken`](https://spec.matrix.org/v1.11/client-server-api/#post_matrixclientv3account3pidemailrequesttoken) 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. This MSC also affects this.
Copy link
Member

Choose a reason for hiding this comment

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

This MSC also affects this.

Could you briefly expand upon how it does so? The proposal section doesn't mention this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

proposals/4183-submitToken-error-codes.md Outdated
Comment on lines 30 to 31
Also change 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 the entire API is the same, including response / error codes, rather than just parameters.
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 understand what you mean by "specify that the entire API is the same".

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully clearer?

proposals/4183-submitToken-error-codes.md Outdated
Comment on lines 48 to 50
`M_NO_VALID_SESSION` if the code is incorrect. Once an identity server (or homeserver) switches to
use the new error code, they may not recognise the error condition correctly until updated to support
the new code. We say that this is acceptable in favour of avoiding the complexity of negotiating error
Copy link
Member

Choose a reason for hiding this comment

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

Once an identity server (or homeserver) switches to use the new error code, they may not recognise the error condition correctly until updated to support the new code.

I'm failing to understand what this means. Is "they" the homeserver?

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, that wasn't super clear. Is that better?

proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
proposals/4183-submitToken-error-codes.md
## Proposal

Add the following specific error code as a code that can be returned by the two endpoints given above:
* `M_TOKEN_INCORRECT`: Indicates that the token that the user entered to validate the session is incorrect.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth re-using/redefining M_UNKNOWN_TOKEN from the C-S API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yes, probably

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thoughts... that's talking about an access token whereas this is the user entering a code that's been sent to them, so they're not really the same thing. I think maybe it's clearer if it's a separate code? I can update the MSC to call this out if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

I think maybe it's clearer if it's a separate code?

(I agree)

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted this in c026da7

proposals/4183-submitToken-error-codes.md Outdated Show resolved Hide resolved
dbkr and others added 5 commits October 2, 2024 11:29
@dbkr @anoadragon453
Apply suggestions from code review
7809ced 
Spelling / grammar fixes

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
@dbkr
Word wrap
be435a8
@dbkr
Clearer wording
007d364
@dbkr
clarify
43638f1
@dbkr
Clarify
5b8b163
richvdh
richvdh requested changes Nov 5, 2024
View reviewed changes
proposals/4183-submitToken-error-codes.md Outdated
Comment on lines 34 to 36
Also change 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 the endpoint is the same as the I/S API version in all ways, including response / error codes, rather than just parameters.
Copy link
Member

Choose a reason for hiding this comment

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

wait, what? this came out of left field. Why are we suddenly changing requestToken in an MSC which purports to be about submitToken?

There doesn't seem to be any justification for this in the proposal, or discussion of it in general.

What exactly would need to change to bring the current definition into line with the I/S API?

Copy link
Member

Choose a reason for hiding this comment

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

ohhh by "the endpoint" do you mean "the endpoint referenced by the submit_url response field"? Could you say so if so?

Still, presumably you don't mean "in all ways" -- for example, it doesn't need an IS access token. Would be good to tighten this up a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, indeed, that wasn't very clear, hopefully better now.

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richvdh richvdh richvdh requested changes

@anoadragon453 anoadragon453 anoadragon453 left review comments

@Johennes Johennes Johennes left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
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
Spec Core Team Backlog
Status: Ready for FCP ticks
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dbkr @turt2live @mscbot @richvdh @Johennes @anoadragon453