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

Adding oauth2 generic RFC authentication #60

Merged
merged 3 commits into from
Jan 2, 2017
Merged

Conversation

dmvieira
Copy link
Contributor

@dmvieira dmvieira commented Dec 9, 2016

Copy link
Contributor Author

@dmvieira dmvieira left a comment

Choose a reason for hiding this comment

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

Resolves #59


class OAuth2EnvMixin(OAuth2Mixin):
_OAUTH_ACCESS_TOKEN_URL = os.environ.get('OAUTH2_TOKEN_URL', '')
_OAUTH_AUTHORIZE_URL = os.environ.get('OAUTH2_AUTHORIZE_URL', '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could I get these attributes using trailets?

I really don't know a better solution than env vars

Copy link
Member

Choose a reason for hiding this comment

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

Env vars are the only thing really available at this level, so this is fine.

@dmvieira dmvieira changed the title Adding oauth2 default RFC authentication Adding oauth2 generic RFC authentication Dec 12, 2016
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

Great, thanks! I made some minor comments about the organization, but this seems super useful.

@@ -1,104 +1,135 @@
"""
Base classes for Custom Authenticator to use GitHub OAuth with JupyterHub
Copy link
Member

Choose a reason for hiding this comment

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

To avoid confusion / compatibility, can you put the new class in oauthenticator/generic.py, rather than changing what oauthenticator.oauth2 provides and means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did

)

token_url = Unicode(
os.environ.get('OAUTH2_USERDATA_METHOD', 'GET'),
Copy link
Member

Choose a reason for hiding this comment

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

copy/paste USERDATA_METHOD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

pass


class OAuth2OAuthenticator(OAuthenticator):
Copy link
Member

Choose a reason for hiding this comment

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

Can you use Generic as the name, too? GenericOAuthenticator, etc.?

@@ -1,6 +1,7 @@
# include github, bitbucket, google here for backward-compatibility
# don't add new oauthenticators here.
from .oauth2 import *
from .generic import *
Copy link
Member

Choose a reason for hiding this comment

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

See the comment above for 'don't add new things here'. Authenticators should be imported directly from their submodule, so that we don't import all the unused Authenticators as this package grows.


login_service = "OAuth2"

client_id_env = 'OAUTH2_CLIENT_ID'
Copy link
Member

Choose a reason for hiding this comment

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

I think this can leave the defaults from the parent class, of OAUTH_...

@dmvieira
Copy link
Contributor Author

I think it's now ready ;) Really thx

@dmvieira
Copy link
Contributor Author

I'm using here and it's working ;)

@minrk minrk merged commit 1a1cc36 into jupyterhub:master Jan 2, 2017
@minrk
Copy link
Member

minrk commented Jan 2, 2017

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.

2 participants