-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(auth): Add support for OpenID Connect provider #6574
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Hey @mikehardy started this PR draft as mentioned in #6305 I'm quite new to contributing to OSS, so feel free to give me any pointers. I would really like collaborate on how we could get support for OpenID Connect into RNFirebase. |
Codecov Report
@@ Coverage Diff @@
## main #6574 +/- ##
============================================
- Coverage 54.08% 54.02% -0.05%
+ Complexity 700 690 -10
============================================
Files 218 219 +1
Lines 10800 10809 +9
Branches 1700 1701 +1
============================================
- Hits 5840 5839 -1
- Misses 4644 4677 +33
+ Partials 316 293 -23 |
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.
Very interesting - I like how the change itself is nearly zero lines of code so it's very easy to see how oidc works (in combination with your screenshot). Certainly not a tough change, I could see it going in just like this. It will need documentation for users as part of the PR and my hunch is that will be the largest part of the diff based on just the idea of oidc.suffix and sending it in
This should fix up the objective-c formatting (where "fix" may actually still look ugly but at least uniformly ugly across whole project) react-native-firebase/package.json Line 13 in ab530bc
|
Thanks a lot @mikehardy really appreciate the help and the feedback! I'l write up the docs when we have landed on an implementation that seems good, no problem 🙂 I'm not sure what is the best approach for RNFirebase. But I'l try to outline some of my thoughts on them. When implementing OpenID Connect on the web, we can use Adding OIDC provider (what I'm doing in this PR and explained above) I also want to add Android support here and I'm also going to try to include nonce into making the credential as an optional argument. An implementation that uses Firebase for the OpenID Connect sign-in flow Doing this would give us an implementation that would not require the user to handle the OAuth flow themselves and would be a richer implementation (although I have not tested this myself). The way I see it, we would then need to support making a new instance of the OAuthProvider. Like I said, this would require more work and have a higher complexity compared to what I am suggesting in this PR. If we did this, an example of how to use an implementation like that could look something like this: const provider = new OAuthProvider('oidc.example-provider');
provider.setCustomParameters({
// Target specific email with login hint.
login_hint: 'user@example.com'
});
provider.setScopes(['mail.read', 'calendars.read']);
// The methods we would need to use are named "getCredentialWith" on iOS and "startActivityForSignInWithProvider" on Android
// so "getCredential" made the most sense to me for a unified implementation in RNFirebase
const credential = await provider.getCredential();
await auth().signInWithCredential(credential); Would you be open to adding the OpenID Connect provider first and loop back in a future PR on an implementation like the one I explained above? The way I see it, one is not better then the other - but both have different use cases. I would be motivated to work towards supporting OpenID Connect using only RNFirebase after that, but I would need more help to get there. I think I lack a little experience on the native side to be able to complete this by myself. But I think the benefit of not having to handle the OAuth flow to use OpenID Connect and having the OAuth configuration come from Firebase automatically is quite big. Again, thank you for your feedback. |
I think a plan that involves incremental progress (that is, a PR like this) along with a future target that might get us most of the way to supporting generic OAuth even would be a dream really. Supporting general OAuth has been something I've wanted for a long time but did not have a personal use case important enough to support my time working on it above general maintenance + wrapping whole new modules here |
…vider to OIDCAuthProvider to be more consistent with the naming of other providers.
That sounds like a good plan @mikehardy ! I will focus on this pr for now, and we can revisit the general support for OAuth in the future. I implemented and tested on the Android side of things now. It works in my app now with both iOS and Android. I also added tests similar to the once that exists for the other providers. I decided to return early in ReactNativeFirebaseAuthModule.java because a switch case was used before. I didn't see any better way to implement it. This time I also linted all the code before commiting 🙌 I will try to draft up some docs next and I would love any feedback on that. Do you think it's best compared to the rest of the docs to:
Should I also go through the steps of the Firebase Console or is that generally a bad idea, since it's hard to stay up to date when the Firebase console changes in the future? |
Fantastic - for the docs, skip the console screens but do your best to describe them, recommend react-native-app-auth, and if you have any code snippets that could be really useful. It's normal to have to add a bunch of things to the spellcheck dictionary, for what it's worth, it'll tell you what's missing on a CI run |
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
Hey, sorry for the delay, have been swamped lately. I'l draft up the docs as soon as I have time, hopefully within a week or two 🙂 |
Lifesaver @Babsvik 🏆 |
@paulrostorp Did you try to use it? Appreciate the feedback, happy to help 🙂 |
Hey, is there anything I can do to help move this forward? 🙂 |
Hey there - thanks for your patience - no further external action is needed to move it forward, I just need my kid back in school from all the holidays (this happened yesterday 😄 ) and then to catch back up with all the repos I maintain (in progress now! 🏃 🏃 ) |
I just applied it with patch-package and it seems to work quite well! (tested it with microsoft active directory). Thanks ❤️ |
Good catch @happyfloat! I refactored that to align more with the other auth classes and forgot to update the docs 🙂 Thanks! I did a new commit now and synced the fork with the main branch. Edit: Updated the message because I commented before I was done writing 😅 |
@Babsvik Thanks 🙏 . Maybe off topic, but do you have any recommendation on how to handle the refresh token of the third party authentication or validity/change of the idToken relative to the firebase authentication? Do you check the expiry yourself and then call firebase Normally I just call I hope this question makes sense... |
I am getting closer to "current" on my reviews and such, let's get this merged :-) Thanks for your patience! |
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.
code review looks great - pending merge - thank you!
This seems great! Had a go at implementing it, but keep hitting an issue with the error message when setting Firebase's
The only difference I can see from the example used is that I'm using Expo Auth Session instead of React Native App Auth. Any hints to what I should check? Using the OAuth Flow with Firebase JS with the OIDC provider works perfectly in a web-based prototype. |
Hey @happyfloat ! To make a simple comparison: when the user does a login you expect them to have their password, but they don't have to type their password again next time they open the app? But if they are going to do something like changing their personal information or something that has elevated privileges maybe you want them to type their password again? I think of the token from IdP as the password in this case. I don't know if this is best practice or even considered safe, but for our use-case this is sufficient. Remember that Firebase uses JWT tokens, so they are also stateless. So unless you manually build something on top that denies specific tokens or use the built in tokenExpiry from firebase when verifying the token on the server it won't have any effect to revoke the tokens. So to answer your question more specifically: you can build something yourself that tracks the original token and does Hope this helps 🙂 |
Hey @rolfb This is just a wild guess from seeing a lot of different messages working with SSO over the years, but my guess would be that it has something to do with nonce or state. See this strack overflow answer for more details about what state and nonce is. In the current implementation of OIDC that we just added, we currently don't do anything with the nonce. So if the nonce is present in the token, the authentication will fail because we did not send a raw nonce (but it's in the token). You could try to decrypt the token that is being sent to Firebase and check if it has a nonce in it to verify this. This could be fixed by passing the rawNonce to the Hope you figure it out 🙂 |
Hey @Babsvik, Thank you for getting back to me. I'll look into the link you provided, but I have gotten a bit further since my last post. At the moment I seem to get to a page which looks blank but has a javacript call to Another thing I'm having some issues to understand is how you exchange the code for a token in your example. Do you know of a more custom way to exchange a code for a token with Firebase Authentication with React Native Firebase after getting back from the third party oidc provider? |
No idea about your first paragraph. My understanding is that the secret is not stored in the app. But it is stored internally in the Firebase infrastructure (when you add it in the console) and when you do I can't say for sure, but it seems like you are confusing differences between web/ios/android and some parts of the authentication process. What we are doing in RN Firebase is largely this.
Best of luck to you 🙂 |
@Babsvik thanks for your response :). To clarify my use case: I want to logout/deactivate the user in my app, if he/she left the parent company or the account got deactivated at their company identity provider. One of the main benefits for SSO? |
Thanks! My understanding is something like this:
Seems to stop at 2. when authorization step returns to the Firebase Auth Handler and I'm not sure why. |
Description
👷 I'm new to contributing to OSS, so please be nice 😅
An attempt at implementing support for OpenID Connect as provider.
I tested this using the Web SDK from Firebase first in React Native. After that worked I wanted to see if I could do the same using RNFirebase.
With some modifications that worked, so I decided to try to get that merge in to support Open ID Connect.
This is documented by Firebase for iOS here and Android here.
I also want to support Android also before merging this in, but for now I have only been focused on iOS.
One of the issues compared to other providers is that in order for Firebase to do the token exchange for you, the providerId can't be static. See screenshot below from the Firebase console.
So it starts with
oidc.
but the rest is defined by the user. Since people can also have multiple OpenID Connect IdP's I think it makes sense to do this as a part of creating the credentials.This is an example of how using it looked in my example:
Related issues
Implementation of the idea @mikehardy suggested in #6305
Release Summary
Added support for OpenID Connect provider
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Will do this later
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on TwitterEdit: Updated Android support to be checked (since it has been implemented)