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

refactor(oidc): Remove method to authorize arbitrary scope #4664

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

zecakeh
Copy link
Collaborator

@zecakeh zecakeh commented Feb 13, 2025

Only the scopes necessary during login are specified in MSC2967 now.

@zecakeh zecakeh requested a review from a team as a code owner February 13, 2025 10:12
@zecakeh zecakeh requested review from stefanceriu and removed request for a team February 13, 2025 10:12
Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.81%. Comparing base (f33d104) to head (cd307c1).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4664      +/-   ##
==========================================
- Coverage   85.82%   85.81%   -0.01%     
==========================================
  Files         292      292              
  Lines       33646    33644       -2     
==========================================
- Hits        28875    28873       -2     
  Misses       4771     4771              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poljar
Copy link
Contributor

poljar commented Feb 13, 2025

Hmm, I don't really understand where it says that scopes can't be authorized after one has logged in. To be fair I don't remember what the MSC looked like previously.

Any pointers and more info where I can see that this indeed isn't supported anymore?

@poljar poljar requested review from poljar and removed request for stefanceriu February 13, 2025 15:11
@zecakeh
Copy link
Collaborator Author

zecakeh commented Feb 13, 2025

The MSC was changed in matrix-org/matrix-spec-proposals@f65aef3.

There used to be a section called "Insufficient privilege response" that used to say that we could have an error response about an "insufficient scope", and that we needed to request another authorization with the given scope, which is why that method was necessary.

Given that this section was removed, the only use case for the authorization endpoint right now is for logging in, so this method has no use case anymore.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification, makes sense.

Could you just get rid of the merge conflict?

Only the scopes necessary during login are specified in MSC2967 now.

Signed-off-by: Kévin Commaille <[email protected]>
@zecakeh
Copy link
Collaborator Author

zecakeh commented Feb 15, 2025

Done

@bnjbvr bnjbvr merged commit 1c11497 into matrix-org:main Feb 17, 2025
42 checks passed
@zecakeh zecakeh deleted the oidc-no-authorize branch February 17, 2025 11:14
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.

3 participants