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

Fix /refresh endpoint to be V1 instead of V3 #1289

Closed
wants to merge 1 commit into from
Closed

Fix /refresh endpoint to be V1 instead of V3 #1289

wants to merge 1 commit into from

Conversation

TobiasFella
Copy link

@TobiasFella TobiasFella commented Oct 14, 2022 •
edited by github-actions bot
Loading

The implementations in synapse and matrix-js-sdk use this as /v1/refresh, which, according to @KitsuneRal, is correct

Signed-off-by: Tobias Fella [email protected]

Preview: https://pr1289--matrix-spec-previews.netlify.app

@TobiasFella TobiasFella requested a review from a team as a code owner October 14, 2022 15:01
turt2live
turt2live requested changes Oct 14, 2022
View reviewed changes
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.

As an r0 endpoint, it got elevated to v3 in MSC2844

The implementations would be incorrect.

@KitsuneRal
Copy link
Member

Does that effectively mean that refresh tokens do not exist in the wild despite being specced for half a year?

@turt2live
Copy link
Member

Seemingly so, but that wasn't really the requirement for implementation on that MSC.

@clokep
Copy link
Member

clokep commented Oct 14, 2022

As an r0 endpoint, it got elevated to v3 in MSC2844

I don't think this was an r0 endpoint. It looks like it landed at the same time as MSC2844 in Matrix 1.3.

(Note the MSC does use r0 since it was written before MSC2844, MSC2918 merged after it though FWIW.)

@KitsuneRal
Copy link
Member

Wait, I actually don't see it being an r0 endpoint in scope for MSC2844.

@turt2live
Copy link
Member

All endpoints in the spec at the time of MSC2844 landing were in scope - that includes MSC2918 which was specified as an r0 endpoint and landed in the same spec version as MSC2844, ergo became /v3 automatically. MSCs which landed after the v1.1 spec release (the first release with MSC2844 endpoints) but specified /r0 also got elevated to v3, though I don't have specific examples on hand.

@turt2live
Copy link
Member

err, sorry: point of clarification on the refresh tokens bits: since it got merged as r0, it triggered the clauses of MSC2844 and got bumped to /v3.

@turt2live
Copy link
Member

turt2live commented Oct 14, 2022 •
edited by richvdh
Loading

for even more clarity on the timeline:

  1. MSC2844 (replacing existing /r0 with /v3) was accepted into the spec January 2021
  2. MSC2918 (proposing the new /r0 endpoint) was accepted into the spec October 2021
  3. MSC2844 was formally specified a few days later in October 2021
  4. Matrix 1.1 was released November 2021, codifying MSC2844
  5. Synapse added support for /v1/refresh in December 2021
  6. MSC2918 was formally specified in June 2022 (8 months later than MSC2844)
  7. Matrix 1.3 was released June 2022, codifying MSC2918

With this timeline in mind, MSC2918 was in fact r0-stable at the time of Matrix 1.1 and thus subject to MSC2844's changes.

@tulir tulir mentioned this pull request Nov 4, 2022
Merged
4 tasks
tulir added a commit to tulir/synapse-old that referenced this pull request Nov 4, 2022
@tulir
Fix /refresh endpoint version
b507c3d 
As per matrix-org/matrix-spec#1289, the refresh
token MSC landed before the new endpoint versioning scheme, which means
the /refresh endpoint should be available as /r0 and /v3 instead of /v1

Signed-off-by: Tulir Asokan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turt2live turt2live turt2live requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TobiasFella @KitsuneRal @turt2live @clokep