-
Notifications
You must be signed in to change notification settings - Fork 976
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
feat: support OAuth2 and OpenID Connect in API-based flows #2346
Conversation
Bruh are you for real? Awesome! Could you please provide a high-level overview of how this feature worked? Preferably with an example for e.g. the Facebook SDK? |
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.
Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)
Just submit login flow with
or
in the body. |
@splaunov feel free to re-request a review any time you're ready :) |
60d0a1e
to
cc02004
Compare
I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕
But the good news is, giving access is easy! If the repository belongs to an organization, please add me for the project as a collaborator! |
Thank you, this looks great! The CI is failing because some files are formatted incorrectly. To format them, run:
Thank you! |
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.
Thank you for these great changes, I think we are now on track to get this shipped. I haven't had a lot of time to really look into the details of the implementation yet (currently on vacation) but from a high level it looks very good.
One question I have is how we deal with validation errors? In the browser flow, if a validation error occurrs (e.g. some required field is missing), we show the registration form again and ask the user to fill out their details. How does that work for API flows?
Another question is, how do we deal with detecting whether we need to execute login or registration? How is that information relayed to the application? In the browser flow, we just redirect to the appropriate login (user exists) or registration (user does not exist yet) endpoint.
Another thing we will need to add to this PR is an e2e test for this new feature. We already have e2e tests for browser OIDC flows which you can find here:
- https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/oidc/registration
- https://github.com/ory/kratos/tree/master/test/e2e/cypress/integration/profiles/oidc/login
- https://github.com/ory/kratos/tree/master/test/e2e/cypress/integration/profiles/oidc/settings
The profile for mobile apps (API flows) you can find here: https://github.com/ory/kratos/tree/master/test/e2e/cypress/integration/profiles/mobile
It will probably not be trivial to add OIDC tests to the mobile app as the flow works differently (we need a valid ID token) but it is a requirement from our end to get this merged into master, as it is the only way to verify that everything works as expected from end-to-end. The tests will need to cover:
- Login with an ID Token and a user which exists
- Login with an ID Token and a user which does not exist -> we end up registering a new user
- Registration with an ID Token and a user which exists -> we end up signed in
- Registration with an ID Token and a user which does not exist, and validation errors (e.g. website missing)
- Successful registration with an ID Token
- Linking and unlinking OIDC providers in the settings flow
- Error cases for each flow (login, registration, settings)
- Audience mismatch
- Signature key mismatch
- ID Token generally invalid
- Issuer mismatch
Codecov Report
@@ Coverage Diff @@
## master #2346 +/- ##
==========================================
+ Coverage 75.89% 76.43% +0.54%
==========================================
Files 311 318 +7
Lines 19185 17414 -1771
==========================================
- Hits 14560 13310 -1250
+ Misses 3486 3158 -328
+ Partials 1139 946 -193
... and 336 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
As for now, there is no registration API flow for oidc. New users are silently provisioned in login flow. You are right in that we need to support registration flow as well. |
This assumes extending the React Native app. It should get ID Token from Hydra. |
Thank you! I will for sure be able to help when I'm back from vacation in May. I think we need to iron out some of the big questions (registration vs login) before we can finalize it and merge it to upstream. But your work so far has been great! If you want to work on this in the meanwhile please do let me know if there's anything I can help with (e.g. pointers) :)) |
Hi everyone, is there any planned work to come back and finish this PR? This is a critically important feature that is blocking my project. I'm not well-versed in Golang, but could try to help with documentation or testing if that helps move this forward. |
Hey there, I think this is a great PR and feature, and it's on our roadmap and backlog! For us unfortauntely it has not as high of a priority as a couple of other very urgent topics. |
@aeneasr Are there any alternative solutions for mobile based OAuth flows while this PR is still in progress? |
Would be very nice if this could be prioritized :) |
Have added support for apps using WebView for oidc logins |
@splaunov thank you for pushing this feature forward 🙏 I have a question though. I am not sure how the proposed WebView flow would work for native apps. AFAIK the final redirect often relies on a custom URL scheme (at least on iOS) which passes the control to the application and the only information available to the native app is the URL so any extra details (like credentials) should be passed via query params. Usually mobile apps don't have access to the WebView context (for security and privacy reasons) especially if something like Apple's Authentication Services framework is used There is an open discussion on that topic here: #2434 It could be that I am missing something here. If so, my apologies in advance. |
Hi all 👋 Two questions: 1/ About this:
As @aeneasr stated, in the current social browser flow, the decision between login and register is made when the callback is called whether the user (or more precisely the OIDC credentials) exist or not. So that we can avoid having the mobile app starting a registration flow when a new user connects through a social provider. 2/ About the method used. |
Here is what missed to my understanding:
|
@dmakarenko yes, you're right. I have added the token to query params of the final redirect. |
Can we merge this, as we really need this functionality in our app? |
Since I'm interested in having this feature in native apps as well I did some checking on the code changes that are made. Most of them look OK but I have to put some question marks at the "webview" flow, the reason mainly being that it would be all to easy for the dev to not open it in an actual webview but to delegate it to the system browser and then redirecting back using a "custom url scheme". This on it's own would not be a problem if those custom schemes would actually require some form of registration at a central provider but unfortunately (or luckily depending on how you look at it) they don't and thus it'd be possible for any app to hijack the redirect and have access to the session and/or session_token. I would suggest making the flow similar to the OAuth2 with PKCE flow where the app needs to generate a challenge + verifier, first it sends the verifier together with the flow request, opens the browser, get's back a code and then exchanges the code together with the challenge for the actual session and session_token. This makes it more flexible, secure and actually easier (since we don't have to decode the redirect url query string or document body) at the cost of a single extra request. FYI: I'm not a Go dev but I've been doing web and app development for over a decade and been coding for even longer and in that time I've written a lot of code in a lot of different (programming) languages (and read even more in even more (programming) languages) so although I don't know the specifics of Go, I do have a quite broad "general programming" skill-set and can read it well enough to understand what's happening. |
Hi @chancezeus! |
I'm not saying we should remove it (especially since we'd need it for some flows where a native SDK wouldn't be available or provide an ID token (like Facebook and/or potentially some generic provider)), I'm just suggesting we should use a code exchange like flow with PKCE similar to OAuth2 to make it more secure so that a malicious party is less likely to hijack the session token. |
Note for previous remark: you wouldn't even need to put anything in the query, since we could use the flows id as the "code" and the challenge/verifiers are generated (and thus known) and send by the frontend anyway, so you could simply say:
=> You could/should even generalize this together with the regular browser flow so it wouldn't matter if you're using browser OR api, but that would be a (major) breaking change... |
Hi! |
Does this PR enable me to use Apple Authentication and Google Sign In libraries to get sign in tokens from the mobile device directly and then pass that token to Kratos over API in exchange for a Kratos auth token? Without webview? I think I'm understanding that the webview implementation is incomplete (missing PKCE), but the support for API login from a mobile device Apple/Google login token is here? Ultimately, some form of Apple/Google social login from React Native is critical for our switch from Firebase to Ory/Kratos. |
styles: format code feat: add registration api flow chore: mostly renames styles: format code test: audience check feat: webview support feat: webview support - added session_token to redirect URL query params feat: webview support - errors handling fix(oidc api webview): check flow messages are set
9b0ccbd
to
3f5b41c
Compare
To answer my own question, yes it does, and it works great! And thanks @splaunov for the recent update from master. (I had done my own master -> feature/oidc-api update a few weeks ago and was going to offer it up this week, but you beat me to it 😄 ) @aeneasr @ory-team This PR passes all unit tests, builds, and functions well for us. We're running our development stacks from this build. Mobile API social logins are a requirement for us to go live with our switch to Ory, which we have operational with this PR. Are there any remaining barriers to getting this merged to master? We were aiming to make the switch to Ory/Kratos end of January. Would be preferable to use official images instead of our own Kratos build pipeline. |
@@ -27,7 +27,7 @@ require ( | |||
github.com/bwmarrin/discordgo v0.23.0 | |||
github.com/bxcodec/faker/v3 v3.3.1 | |||
github.com/cenkalti/backoff v2.2.1+incompatible | |||
github.com/coreos/go-oidc v2.2.1+incompatible | |||
github.com/coreos/go-oidc/v3 v3.1.0 |
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.
BTW, looks like there's a newer version now:
github.com/coreos/go-oidc/v3 v3.1.0 | |
github.com/coreos/go-oidc/v3 v3.5.0 |
We are definitely highly interested to make this part of Kratos out of the box - we're just time constrained on our end. We'll give this a review soon! |
Hi! We're using the code from this PR on our backend, and I'm trying to do a Facebook login on our React Native app. The issue I have is that the Facebook SDK doesn't give me an ID Token, but an Access Token and I can't get the API to accept it (sending it as Is there a way to do API Facebook login that's known to work, by any chance? |
Just for info, we implemented @MortalKastor's feature with the following patch. I thought we should share it with you all :)
|
Hi everybody! First of all a big thank you to @splaunov for this PR. Based on this work, we revisited the flow to get OIDC working on mobile and ultimately settled on an approach implemented here: #3216. The main difference between that PR and this is that we continue to let Kratos fetch the identity from the OIDC provider, which also means that we can continue using the more secure authorization code grant. The app gets the session token as follows:
If you are interested, please take a look at the implementation. I welcome any comments :). And thanks for being part of the Ory Community ❤️ |
Hey @David-Wobrock, i am working on a same design (google sign in on native app), have you had any progress with this ? |
Not really @aptgetgit, sorry. But happy to see this feature to be merged into Kratos 👍 Thank you everyone involved ❤️ |
Related issue(s)
#707
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments