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

Remove mixins, per-Authenticator LoginHandler classes #323

Merged
merged 19 commits into from
Jan 28, 2020

Conversation

minrk
Copy link
Member

@minrk minrk commented Dec 18, 2019

This PR simplifies oauthenticator quite a bit by reducing a lot of the boilerplate involved in defining/maintaining implementations.

Implementation progress:

  • github
  • gitlab
  • google
  • bitbucket
  • azuread
  • azureadb2c (never released, removed and replaced with azuread config)
  • auth0
  • aws (never released, removed and replaced with generic config)
  • cilogon
  • generic
  • globus
  • mediawiki
  • okpy
  • openshift
  • yandex (never released, removed and replaced with generic config)

In the future, I think we should perhaps promote more aspects of GenericOAuthenticator to the base class, and implement most of our providers via override of more granular methods than authenticate. I'm pretty sure several can be pretty basic already (openshift, okpy, auth0, bitbucket, google).

these can all be configured via GenericOAuthenticator

since they were merged, replace them with informative import errors
GenericOAuthenticator defined it already, let's use that
@minrk minrk changed the title [WIP] Remove requirement for mixins, per-Authenticator LoginHandler classes Remove requirement for mixins, per-Authenticator LoginHandler classes Dec 18, 2019
@minrk minrk changed the title Remove requirement for mixins, per-Authenticator LoginHandler classes Remove mixins, per-Authenticator LoginHandler classes Dec 18, 2019
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Wow great work going through this @minrk! I really would love to see this PR merged as I would greatly appreciate the reduced complexity that I've experienced both as a user and maintainer of this project. Thank you for reviewing this @NickolausDS!

Note that it is claimed that some of the removed authenticators was unreleased, I believe we did release some new authenticators as part of 0.10.0. Still, I think we should simply make a quick removal of them.

oauthenticator/oauth2.py Outdated Show resolved Hide resolved
@NickolausDS
Copy link
Contributor

I went through and tested the latest changes with both GitHub and Globus, and both worked great. It looks good to me!

@consideRatio
Copy link
Member

@minrk this LGTM to the extent i can evaluate it, and i think it is an essential PR.

I'll go ahead and merge, thank you for review and testing @NickolausDS!

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.

Refactor away some mixins Entrypoints causes issues with JupyterHub.
3 participants