-
Notifications
You must be signed in to change notification settings - Fork 270
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
FFI: Expose support for registration through OIDC in the login details. #4169
Conversation
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.
Too much logic at the FFI layer, please create a method in the SDK with tests :)
Haha, a response faster than Clippy! Ok will do next week 🙂 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4169 +/- ##
=======================================
Coverage 84.77% 84.77%
=======================================
Files 269 269
Lines 28963 28963
=======================================
+ Hits 24552 24553 +1
+ Misses 4411 4410 -1 ☔ View full report in Codecov by Sentry. |
if let Ok(issuer) = oidc.fetch_authentication_issuer().await { | ||
if let Ok(metadata) = &oidc.given_provider_metadata(&issuer).await { | ||
metadata | ||
.prompt_values_supported |
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.
Wouldn't be more efficient / generic to expose the prompt_values_supported
list as a list of OidcPrompt
?
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.
Sure, I wasn't sure if the concept of OidcPrompt
s was a bit too much info but if we're happy with that I'll update and at which point, there wouldn't be any custom FFI logic so I would leave all this in the FFI.
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.
Hmmm, the amount of code didn't really change that much, it's basically just the use of Into
instead of contains
. WDYT @bnjbvr, I wouldn't really consider there to be any custom FFI logic here now as it is just mapping SDK values to FFI values.
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.
If that's sufficient for your needs, that's fine with me. (Please make sure to log errors that are silently swallowed here, for both if let Ok(...)
branches.)
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.
Done in 4829f48
dfbcfc1
to
4024ee0
Compare
4024ee0
to
4829f48
Compare
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.
lgtm, thanks!
This allows clients to decide whether or not passing
Create
tourl_for_oidc
is allowed.