jupyterhub / oauthenticator

OAuth + JupyterHub Authenticator = OAuthenticator
https://oauthenticator.readthedocs.io
BSD 3-Clause "New" or "Revised" License
413 stars 365 forks source link

Allow authentication via nested groups for Google OAuthenticator #762

Closed jrdnbradford closed 1 month ago

jrdnbradford commented 1 month ago

Proposed change

Allow authentication via nested groups for Google OAuthenticator. Google Workspace now allows nested Google Groups, but the Google OAuthenticator has no option for allowing auth this way.

Who would use this feature?

This would be helpful for those who manage the IAM for many hubs.

Suggest a solution

I have a fork that implements this feature. The particular issues that need to be discussed are:

Most of this can be solved by required explicit opt-in with:

c.GoogleOAuthenticator.allow_nested_groups = True

If there's interest I can spruce up what I have and put in a PR for further discussion.

consideRatio commented 1 month ago

Makes sense!

I'm opinionated that we should let it default to False, making the more permissive choice be opt-in rather than opt-out.

Opening a PR makes sense in my mind.

An open question remains, what about google_admin_groups? I think it should also be influenced, but maybe not? Whats clear is that it should be considered and documented.

Also, if you can make the requests become async, its a separate improvement of relevance - PR welcome for that as well, but preferably separately from the other. I'll promise to get sync->async PR review done fast if you end up working that as well

consideRatio commented 1 month ago

I think the config name is great as well, i considered include_nested_groups as well but i lean towards your suggestion

jrdnbradford commented 1 month ago

I agree on it affecting admin groups as well, as long as it's obvious in the docs and requires explicit opt-in.

And actually I think I prefer your include_nested_groups more so than my allow_nested_groups 😆. allow in this context might indicate something that can happen but doesn't necessarily happen, whereas include makes it obvious that the nested structure matters. I'll put in a PR with my allow_nested_groups since you lean towards it, but I can update it if wanted.