Skip to content
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

Allow configuration of OAuthenticator.scope #523

Merged
merged 4 commits into from
Feb 26, 2018

Conversation

manics
Copy link
Member

@manics manics commented Feb 22, 2018

Follow-up to #501

This allows OAuthenticator.scope to be set. The default is [read:org] if auth.github.org_whitelist is set, otherwise empty.

Examples with Github.

auth.github.org_whitelist unset, auth.scopes unset:
screen shot 2018-02-22 at 13 28 53

auth.github.org_whitelist set, auth.scopes set to [read:org]:
screen shot 2018-02-22 at 13 28 23

auth.github.org_whitelist set, auth.scopes set to [read:org, read:user, user:email]:
screen shot 2018-02-22 at 13 27 29

In theory this property should also work with other OAuth providers, but I don't have an easy way to test.

@@ -139,7 +140,8 @@
org_whitelist = get_config('auth.github.org_whitelist', [])
if len(org_whitelist) != 0:
c.GitHubOAuthenticator.github_organization_whitelist = org_whitelist
c.GitHubOAuthenticator.scope = ['read:org'] # required for private membership
if not auth_scopes:
c.OAuthenticator.scope = ['read:org'] # required for private membership
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like the wrong place to embed the logic for default scopes. Maybe this should be left unset since it should work with public members of an org, and an example added to the docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense

@manics
Copy link
Member Author

manics commented Feb 22, 2018

I've removed the default GitHub scope. I can update the docs after #515 is merged

@@ -186,6 +185,8 @@
else:
raise ValueError("Unhandled auth type: %r" % auth_type)

c.OAuthenticator.scope = get_config('auth.scopes', [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overrides the default of the chosen OAuthenticator to an empty list if unset in the helm chart.

To avoid this, use a condition:

auth_scopes = get_config('auth.scopes')
if auth_scopes:
    c.OAuthenticator.scope = auth_scopes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@minrk minrk merged commit 9cfcc6e into jupyterhub:master Feb 26, 2018
@minrk
Copy link
Member

minrk commented Feb 26, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants