-
Notifications
You must be signed in to change notification settings - Fork 269
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(auth-qrcode): Use oauth2 crate instead of openidconnect #4604
Conversation
The MSCs are now only based on OAuth 2.0, which is simpler than OpenID Connect. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4604 +/- ##
==========================================
- Coverage 85.79% 85.79% -0.01%
==========================================
Files 293 293
Lines 33774 33768 -6
==========================================
- Hits 28976 28970 -6
Misses 4798 4798 ☔ View full report in Codecov by Sentry. |
Is there some context to this change, some Github issue with a rationale maybe? |
There is a bit of rationale in #4593 and #4550. One of the main differences between OAuth 2.0 and OIDC is that the latter requires the use of an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skimmed the changes and they seem pretty simple, thanks a bunch. In terms of requirements before landing, I'd like to make sure this is tested in some of the EX apps before we merge this.
@@ -0,0 +1,133 @@ | |||
// Copyright 2024 The Matrix.org Foundation C.I.C. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2025
type OauthClientInner< | ||
HasAuthUrl = EndpointNotSet, | ||
HasDeviceAuthUrl = EndpointSet, | ||
HasIntrospectionUrl = EndpointNotSet, | ||
HasRevocationUrl = EndpointNotSet, | ||
HasTokenUrl = EndpointSet, | ||
> = BasicClient<HasAuthUrl, HasDeviceAuthUrl, HasIntrospectionUrl, HasRevocationUrl, HasTokenUrl>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be type OauthClientInner = BasicClient<EndpointNotSet, ...>
to avoid the generics? And if OauthClientInner
is used only a single time, we can even inline the type declaration into its one use, likely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, let's inline this, I was hesitating whether to do it or not.
Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
FWIW, I tested this via the |
I tested this with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please resolve the merge conflict, then we can go ahead and merge.
Done |
Actually the changelog is in the wrong place, let me fix that |
Signed-off-by: Kévin Commaille <[email protected]>
And done. |
The "next-gen auth" MSCs are now only based on OAuth 2.0, which is simpler than OpenID Connect.
This sholdn't break anything but I haven't tested it in production.