-
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(oidc): Only support public clients #4634
Conversation
Public clients are clients with only a client ID, and no secret or other authentication method. It should be the most common case and allows to simplify several APIs. Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
This simplifies the registration flow, and matches what higher level methods are doing. 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 #4634 +/- ##
=======================================
Coverage 85.70% 85.70%
=======================================
Files 292 292
Lines 33590 33586 -4
=======================================
- Hits 28788 28786 -2
+ Misses 4802 4800 -2 ☔ View full report in Codecov by Sentry. |
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.
The changes LGTM in general, but I'd rather have someone else with more experience in this part of the SDK review it too. I'm not sure I fully understand what this change would mean for the clients.
@matrix-org/rust ?
.client_id() | ||
.to_owned(); | ||
let client_id = | ||
api.client_id().context("OIDC client credentials are missing.")?.0.clone(); |
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.
This message is probably not accurate now.
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.
Fixed
cc @pixlwave do you have a feedback about this change? |
Looks reasonable to me. We've only ever extracted the client ID out of the credentials/created credentials from a client ID anyway so this shouldn't have any effect to us. |
Signed-off-by: Kévin Commaille <[email protected]>
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.
Yup, makes sense.
Could you please resolve the conflict?
Signed-off-by: Kévin Commaille <[email protected]>
Done |
This should be the most common case, and is already the only case supported by the higher level APIs like
url_for_oidc
andlogin_with_qr_code
. It simplifies the API because we can callrestore_registered_client
directly fromregister_client
, which was a TODO.