Allow restricting profiles & profile_options based on JupyterHub groups #3883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Merging this PR will trigger the following deployment actions. Support and Staging deployments
Production deployments
|
This was referenced Mar 30, 2024
|
I'm going to mark this as draft, as I don't believe we should use our engineering capacity to review and merge this right now. |
yuvipanda
force-pushed
the
earthscope-generic
branch
2 times, most recently
from
8f74c80 to
9b92edd
Compare
yuvipanda
force-pushed
the
earthscope-generic
branch
from
d911502 to
ed17f94
Compare
yuvipanda
force-pushed
the
earthscope-generic
branch
from
ed17f94 to
c84ac83
Compare
yuvipanda
changed the title
8 tasks
yuvipanda
changed the title
yuvipanda
force-pushed
the
earthscope-generic
branch
from
31e6f19 to
beaa424
Compare
|
This is missing additional feature docs on enabling |
|
Added and cleaned up docs! |
GeorgianaElena
approved these changes
May 7, 2024
There was a problem hiding this comment.
Thank you for working on this @yuvipanda , it's looking great! <3 I left a few small comments, but none is blocking so please feel free to merge this PR when you think it's ready 🚀
Comment on lines
+124
to
+123
| # Convert 'scope' from the OAuth2 response into JupyterHub groups | ||
| manage_groups: true |
There was a problem hiding this comment.
I think this can go, as manage_groups is already set to true at line 100 through
c.GenericOAuthenticator.manage_groups = True
There was a problem hiding this comment.
I removed the line 100!
Comment on lines
+203
to
+212
| ## Enabling this feature for other Authenticators | ||
|
|
||
| Currently, the EarthScope hub has this feature enabled via custom overrides. Once | ||
| [this PR](https://github.com/jupyterhub/oauthenticator/pull/735) is merged and | ||
| deployed, we can enable this feature for hubs using other Authenticators more generally. |
There was a problem hiding this comment.
I'd add this to the beginning of the page under a {warning} directive so that it's more clear that is not yet available outside of the CustomGenericOAuthenticator
helm-charts/basehub/values.yaml
Outdated
Comment on lines
1071
to
1072
| # casefold teams so we can do case insensitive comparisons, as github itself is case insensitive (but preserving) | ||
| # for orgs and teams |
There was a problem hiding this comment.
I think this ended up here by accident and should instead be before line 1035 maybe?
Suggested change
| # casefold teams so we can do case insensitive comparisons, as github itself is case insensitive (but preserving) | |
| # for orgs and teams |
yuvipanda
force-pushed
the
earthscope-generic
branch
2 times, most recently
from
bf9f382 to
0b77600
Compare
The Auth0 authenticator made the EarthScope authentication mechanism look more custom than it really was - it's really just standard auth0, and if you look at the [auth0 authenticator](https://github.com/jupyterhub/oauthenticator/blob/main/oauthenticator/auth0.py) code it's fairly minimal - just a couple of convenience functions. This PR switches that (+ our auth0 documentation) to simply use GenericOAuthenticator. This has the following advantages: 1. The Auth0 documentation we have can be easily ported to just support any Generic OAuth provider if needed in the future. 2. The GenericOAuthenticator has features the Auth0 one does not - particularly around groups management that we do want to use. While eventually I think this should be made available to all authenticators (and will work with upstream in doing so), moving to GenericOAuthenticator unblocks planning & scheduling engineering work here as soon as 2i2c-org#3818 is merged. 3. It signals that the EarthScope hub is not *that* special, just the first as a way for us to develop and offer new features. We should work on structuring how we do this, and signal when features are available in what hubs. But in the meantime, this reduces the overall apparent complexity to match actual complexity 4. Removes the custom logout_url work, and instead just mentions you need to set the `client_id` in the logout_url, and adds that to our auth0 documentation. This removes more custom code we have for EarthScope. Once jupyterhub/oauthenticator#719 is merged, this helps us remove all custom code here from earthscope. This part fixes 2i2c-org#3715 This was triggered as cleanup by https://2i2c.freshdesk.com/a/tickets/1453. I'll create appropriate issues for next steps, and will prioritize any future work accordingly with our engineering processes.
for more information, see https://pre-commit.ci
Otherwise, there were issues with closure capture in earthscope - somehow, original_profile_list was actually still being magically set to the function, even though it works fine in gh-teams set in basehub/values.yaml! I suspect some traitlets magic. Instead of debugging it through, let's partial our way out. Also more cleanly set the profile_list override in gh-teams, so the overriding function is not set unless necessary.
- `allowed_groups` functionality is put in basehub, and hence available to everyone! Individual authenticators still need to figure out how to enable groups, but that's separated out from `profile_list` filtering functionality. - Pending jupyterhub/oauthenticator#735, we explicitly also treat GitHub teams from auth_state as 'groups'. This allows us to bring all our existing users along, without issue. - Get rid of the code duplication in earthscope - Rename all `allowed_teams` to `allowed_groups`.
Co-authored-by: Georgiana <[email protected]>
yuvipanda
force-pushed
the
earthscope-generic
branch
from
0b77600 to
b8a8e49
Compare
|
🎉🎉🎉🎉 Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/8990616478 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does a bunch of things that would be difficult to do separately, and hence it's slightly big. Not too big!
1. Move from
Auth0OAuthenticatortoGenericOAuthenticatorfor EarthScopeThe Auth0 authenticator made the EarthScope authentication mechanism look more custom than it really was - it's really just standard auth0, and if you look at the auth0 authenticator code it's fairly minimal - just a couple of convenience functions. This PR switches that (+ our auth0 documentation) to simply use GenericOAuthenticator. This has the following advantages:
client_idin the logout_url, and adds that to our auth0 documentation. This removes more custom code we have for EarthScope. Once Addallowed_scopesto all authenticators to allow some users based on granted scopes jupyterhub/oauthenticator#719 is merged, this helps us remove all custom code here from earthscope. This part fixes [Support] Finalize earthscope domain migration, a documentation thing remains #3715There is still a custom override for the authenticator, but most of that can be removed once jupyterhub/oauthenticator#719 is in a released version of OAuthenticator.
2. Provision JupyterHub groups for
scopesAuth0 allows us to use granted scopes to differentiate what rights various users on the JupyterHub should have. In this PR, we use our custom auth override to provision JupyterHub groups based on the scopes granted. We can not use the current
claim_groups_key, as scopes are not a part of the user claim. jupyterhub/oauthenticator#735 fixes this, but until that lands, we can rely on the custom auth override to provision JupyterHub groups.3.
allowed_groupsfunctionalityWe currently have
allowed_teamsfunctionality that restricts which profiles are available to which users based on GitHub team membership. However, this is specific to GitHubOAuthenticator only. This PR expands it intoallowed_groupsfunctionality, which works for all authenticators. Until jupyterhub/oauthenticator#735 lands, we also directly treat GitHub teams as 'groups'. All existing references toallowed_teamsis renamed toallowed_groupswith no loss of functionality for us. This also allows us to restrict profiles on EarthScope based on JupyterHub group membership.4.
allowed_groupssupport forprofile_optionCurrently,
allowed_groups(andallowed_teams) only supported restricting whole profiles (selected via radio buttons), but not profile_options (the dropdowns) orunlisted_choice(the textbox). This lead to some unclean duplication in places, and causes long term issues in places where the 'profile' is to select the environment and the dropdown is to select resource size. This PR addsallowed_groupssupport for bothprofile_option.choicesandprofile_option.unlisted_choice, so this awkwardness goes away.Upstreaming strategy for this work is described in #4021