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

Whitelist #65

Merged
merged 4 commits into from
Sep 6, 2017
Merged

Whitelist #65

merged 4 commits into from
Sep 6, 2017

Conversation

jkuruzovich
Copy link
Contributor

Whitelist is necessary if using GitHub authentication in the wild.
This will prevent all users and only enable authorized users.

Whitelist is necessary if using GitHub authentication in the wild.
This will prevent all users and only enable authorized users.
c.Authenticator.admin_users = get_config('admin.users', [])

if get_config('whitelist.enabled', False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this explicit check needed?

@yuvipanda
Copy link
Collaborator

Thanks for the patch!

Two things:

  1. Can you move the 'whitelist' option inside 'hub'? Keeping 'admin' outside was a mistake we need to fix, and I'd rather not add more top level entries
  2. Can you add a 'null' default in values.yaml?

Thank you for the patch!

Based on feedback:

(1) Moved whitelist from a top level config variable to a hub.
(2) Removed whitelist.enabled check.
(3) Added default value to values.yaml.
@jkuruzovich
Copy link
Contributor Author

Thanks for the feedback! I think I got the changes you suggested. Just let me know if more changes needed.
Jason

@minrk minrk merged commit b2fb7a6 into jupyterhub:master Sep 6, 2017
@jkuruzovich
Copy link
Contributor Author

Thanks!

@choldgraf
Copy link
Member

pinging @arokem as we ran into this problem with neurohackweek I believe...just making sure he sees it

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.

None yet

4 participants