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

callysto: fix auth logic for use with oauthenticator v16 #3236

Merged
merged 3 commits into from
Oct 9, 2023

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Oct 5, 2023

Currently deployed to staging and prod as I figured it would help us avoid stressing getting this in.

@consideRatio consideRatio requested a review from a team as a code owner October 5, 2023 18:04
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

Merging this PR will trigger the following deployment actions.

Support and Staging deployments

Cloud Provider Cluster Name Upgrade Support? Reason for Support Redeploy Upgrade Staging? Reason for Staging Redeploy
gcp callysto No Yes Following helm chart values files were modified: common.values.yaml

Production deployments

Cloud Provider Cluster Name Hub Name Reason for Redeploy
gcp callysto prod Following helm chart values files were modified: common.values.yaml

Comment on lines -77 to -90
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
)
Copy link
Contributor Author

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.

Comment on lines -92 to -93
async def authenticate(self, *args, **kwargs):
authenticated_user = await super().authenticate(*args, **kwargs)
Copy link
Contributor Author

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.

Copy link
Member

@GeorgianaElena GeorgianaElena left a 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?

config/clusters/callysto/common.values.yaml Outdated Show resolved Hide resolved
@consideRatio
Copy link
Contributor Author

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.

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 :)

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.

Yes, this confusion makes me consider it almost a bugfix to make the breaking change of checking against email claim instead of username. I think it may not end up being a breaking change, but I'm not sure... Let's follow up such discussion in the linked issue.

What do you think about upstreaming this change to authenticator directly?

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.

@consideRatio
Copy link
Contributor Author

Thank you for reviewing this @GeorgianaElena!!!!

Copy link
Member

@GeorgianaElena GeorgianaElena left a 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?

@consideRatio
Copy link
Contributor Author

@GeorgianaElena 100% agree about the comment. I linked to two issues, one about wildcards to handle *.edu and one about checking against email claim and not the chosen username.

@consideRatio consideRatio merged commit 672128d into 2i2c-org:master Oct 9, 2023
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6460792898

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

Fix Callysto's custom authenticator
2 participants