-
Notifications
You must be signed in to change notification settings - Fork 67
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
callysto: fix auth logic for use with oauthenticator v16 #3236
callysto: fix auth logic for use with oauthenticator v16 #3236
Conversation
Merging this PR will trigger the following deployment actions. Support and Staging deployments
Production deployments
|
allowed_domains = List( | ||
Unicode, | ||
default=None, | ||
# This must always be set! | ||
allow_none=False, | ||
help=""" | ||
Allow access to users with verified emails belonging to these domains. | ||
""" | ||
Custom override of CILogonOAuthenticator to allow access control via | ||
one property (email) while the username itself is a different | ||
property (oidc), and to allow `allowed_domains` to involve wildcards | ||
like *. | ||
|
||
This allows us to restrict access based on the *email* of the | ||
authenticated user without having to store the actual email in our | ||
servers or use it as a username. | ||
""" | ||
|
||
Allows glob patterns but not regexes. So you can allow access to | ||
*.edu, for example. See https://docs.python.org/3/library/fnmatch.html for | ||
list of allowed patterns. | ||
""", | ||
config=True | ||
) |
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.
Instead of defining a new trait, we rely on the existing on provided under the individual allowed_idps
entries.
async def authenticate(self, *args, **kwargs): | ||
authenticated_user = await super().authenticate(*args, **kwargs) |
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.
Instead of overriding authenticate
we have to override check_allowed
based on how it works now in oauthenticator v16.
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.
Thanks for fixing this @consideRatio!
I am not sure if you are aware of jupyterhub/oauthenticator#547, that I opened right after Yuvi implemented this special case authenticator for Callysto.
What do you think about upstreaming this change to authenticator directly?
I still believe this is something useful to have by default in oauthenticator
.
Also, we're still not enforcing to define allowed_domains
only when the claim is email
. So people can still define an allowed_idp.allowed_domains
even if their usernama_claim
is not email
, which will make check_allowed
to fail, with a potentially confusing error message.
What do you think?
Thanks for linking that and opening it in the first place! I wasn't aware and had it on my todo list to write such issue :)
Yes, this confusion makes me consider it almost a bugfix to make the breaking change of checking against
I'd like to take this step first separately because callysto's deployment is broken currently, and I don't want to make us feel forced to push through something upstream because we have put us in a situation needing that. |
Thank you for reviewing this @GeorgianaElena!!!! |
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'd like to take this step first separately because callysto's deployment is broken currently, and I don't want to make us feel forced to push through something upstream because we have put us in a situation needing that.
Agree! Then let's merge this and remove it once we've upstreamed it 🚀 🚀
Actually, @consideRatio, can you please also add a FIXME comment with a link to the upstream issue before merging, so we know where this is being tracked?
f084e34
to
46d9e30
Compare
@GeorgianaElena 100% agree about the comment. I linked to two issues, one about wildcards to handle |
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6460792898 |
Currently deployed to staging and prod as I figured it would help us avoid stressing getting this in.