-
Notifications
You must be signed in to change notification settings - Fork 47
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
#5444 ConfiguredIDPOAuth shows authentication popup and save JWT to storage #5795
#5444 ConfiguredIDPOAuth shows authentication popup and save JWT to storage #5795
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
…oauth-window-+-save-jwt-to-storage
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.
Some comments for code clarity, just a quick review
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.
Other than the redirect page in progress, my comments were addressed 👍
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.
Looks good for me 👍 Just need to fix redirect part mentioned by Tom.
Probably we can use launchWebAuthFlow method, which enables auth flows for non-Google IdP. There is also getRedirectURL method for generating correct redirect URL.
I previously decided to use our own oauth solution instead of launchwebAuthFlow because of below reason.
|
The biggest problem with launchWebAuthflow is we have limited control for UI testing. |
…oauth-window-+-save-jwt-to-storage
Maybe it can be added without any additional confirmation popup, like
Probably it'll work the same as our current mock implementation - it'll open mock authorization endpoint and will wait until it receives auth token. Also a couple years passed since your initial comment about poor documentation, there should be more guides about it's implementation. I think it's worth to check these methods for our auth process, and if it's still broken - then try to use |
@tomholub and @sosnovsky, I suggest updating our OAuth mechanism. Currently, we inject Let me know your thoughts! PS: We cannot rely on a content script solution for handling OAuth redirects. When OAuth redirects to our Chrome extension page (or to https://{extensionId}.chromiumapp.org), Chrome prevents the content from loading due to security concerns. As a result, the content script does not load, making it unusable for capturing the authorization code in this scenario. |
I agree.
|
chrome.identity.launchWebAuthFlow({
'url': authConf.oauth.authCodeUrl + '?client_id=' + clientId + '&response_type=token',
'interactive': true
}, function(redirectUri) {
// Extract token from redirectUri.
}); |
The problem from what I remember was that the identity mechanism only allowed to authenticate with the account that is connected to your browser. So no multi user to my knowledge I wouldn't start working on replacing that unless we run into a roadblock with our existing mechanisms. They work well imo |
Maybe we could only use launchWebAuthFlow for custom IDP authentication and keep using our customized OAuth solution for google IDP. |
I would like to, but last I tried it didn't work. I think we are using web application type of oauth credentials in oauth settings on google, which doesn't allow setting this type of redirect url meant due extensions. While when trying to use the extension type, it had some other limitations making it unusable for our purposes (I think no pffline_mode or maybe something else). I wouldn't try to change anything unless we absolutely have to. If the current gmail mechanism works, I don't think we have to. Of course, if I'm wrong and it's just a matter of adding this redirect url and a small code change, then sure. But from my experience, you'll run into a lot of dead ends. |
If it seems easy to do and simplifies things then sure. Does it allow offline_mode? Also, does it work the same on firefox? |
But how about we leave existing google mechanism untouched in this pr if it works. Try out the new identity mechanism on 3rd party logins. That way no risk. We gain some experience with it in a low impact scenario. Then we can consider changing google login as well in a separate PR in future version. |
Yeah, it should be safe to start using new identity flow for 3-rd party IdPs, and if no issues found - migrate Google IdP to it later. |
Sounds good!. Thank you |
Yeah, it works same on Firefox
|
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 looks good on my end, didn't test it
@ioanmo226 do you have active test instance of auth0? I tried to use credentials which you previously shared with me, but got error |
Yeah. I have active test instance. How did you test? With which client_id, client_secret did you test. And also did you test with local FES? |
Ohh. I just remember I rotated previous client secret because previous one was in git history. |
FYI, redirect_url in |
I just implicitly set authentication configuration in |
Probably also true on android and ios, but it could be useful if we ever do authentication on the web. Once we figure this out, we'll update the docs. |
I haven't added bnjglocicdkmhmoohhfkfkbbkejdhdgc redirect_url before. Just added to my auth0 config. Please check again. |
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.
All good now, works great, thanks!
This PR implemented feature to let ConfiguredIDPOAuth shows authentication popup and save JWT to storage
close #5444// if this PR closes an issue
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):