-
Notifications
You must be signed in to change notification settings - Fork 362
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
[All] Added extra_authorize_params to pass extra params in the initial request to the identity provider #338
Conversation
3482283
to
19e5dc0
Compare
Personally, I'd like for BTW, tests pass
|
I'm just making my example work, I'm pretty sure there are better ways to do it, I'm not smart enough to figure these out yet. |
The current changes here expose the extra params for any Oauthenticator using the default LoginHandler (Or a derivative). For example, any OAuthenticator would be able to configure custom params through the Jupyterhub Config file without overriding LoginHandler:
Or for CILogon
Or for Globus
It would be nice to have these params generally configurable for any OAuthentictor. |
@NickolausDS I see that, what I see in my approach that I don't see in yours is that in my approach users could just update to a new version and won't need to change anything for their current setup to work (I guess CILogon only), and if they decide to add more params they can just do that too. |
For CiLogon, your changes would be a nice way to keep the explicit
|
@NickolausDS I went ahead and did it oauthenticator/oauthenticator/cilogon.py Lines 38 to 49 in 10030d6
oauthenticator/oauthenticator/oauth2.py Lines 236 to 242 in 10030d6
tests passed I guess we should still wait to hear back from the maintainers on what approach they'd prefer |
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.
Two comments on the code.
I haven't done any functional testing but the change makes sense to me.
@manics I applied both your suggestions. Thank you for reviewing! Also thanks to @missingcharacter for suggestions and testing. I retested with Globus to make sure I could supply params in the config file. Let me know if you see anything else. |
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.
I've tested this with https://developers.google.com/identity/protocols/oauth2/web-server#creatingclient and it works as expected. Thanks!
Changes for #244. These changes expose a generic config option for passing in arbitrary options alongside the
state
parameter. One example is requesting refresh tokens, as in #211, which should look like this:This may also solve #298 in order to select arbitrary accounts for Google. I created an app and tested the following:
Which generated the following URL:
This appeared to work, although I'm not familiar enough with Google to know if that worked exactly as intended. I tried requesting Google refresh tokens as well, but it didn't seem to want to give me any. I also tested with Globus, and was able to successfully request refresh tokens simply by setting
{'access_type': 'offline'}
.