Grant access for 2i2c members only via admin_users #3233

consideRatio · 2023-10-05T13:35:16Z

With oauthenticator 16, being listed in admin_users makes 2i2c members have access even if they are now listed in allowed_domains or allowed_organizations. Due to this, we can avoid the need to declare 2i2c.org and 2i2c-org there and rely on being listed in admin_users.

I've provided a few comments, one per kind of change made.

-          # Allow 2i2c staff to login with Google
-          http://google.com/accounts/o8/id:
-            username_derivation:
-              username_claim: "email"
-            allowed_domains:
-              - "2i2c.org"
          # Allow MTU to login via Shibboleth
          https://sso.mtu.edu/idp/shibboleth:
            username_derivation:
              username_claim: "email"
            allowed_domains:
              - "mtu.edu"
+          # Allow 2i2c staff to login with Google accounts
+          http://google.com/accounts/o8/id:
+            username_derivation:
+              username_claim: "email"


This was a re-ordering of config entries. I want us to put the user-facing idp's first as a way to prepare for letting that lead to presenting that option by default in favor of presenting the option only relevant to us admin first.

For more details about this, see jupyterhub/oauthenticator#690

consideRatio · 2023-10-05T16:28:37Z

config/clusters/callysto/common.values.yaml

+      # add_staff_user_ids_to_admin_users is disabled because the usernames
+      # aren't github id or email based, individual 2i2c members have added
+      # their user to admin_users manually instead.
      add_staff_user_ids_to_admin_users: false
      # add_staff_user_ids_of_type: "google"


In this hub where we aren't added to admin_users, it remains important to not remove allowing us via allowed_domains.

config/clusters/meom-ige/common.values.yaml

consideRatio · 2023-10-05T16:32:04Z

config/clusters/smithsonian/common.values.yaml

@@ -76,7 +75,6 @@ basehub:
          description: &profile_list_description "Start a container with at least a chosen share of capacity on a node of this type"
          slug: small
          default: true
-          allowed_teams: *allowed_github_orgs


Its better if we just allow all users by default instead, because otherwise we need to make a few extra API calls to check for membership etc for no real reason.

Since #3234 (comment), removing allowed_teams makes all users logged in to the hub be able to see this like we want.

GeorgianaElena

I think this is good to go so it doesn't get into conflict with future work.

Note, that I believe documenting the oauthenticator > 16 new behaviour in the context of 2i2c config is really important, as I often find myself confused by the new defaults, even though I stayed pretty up to date with the work that into into that release.

So, I believe it is really important to have the docs up to date with the new version, so we can all relate to it when uncertain.

We should prioritize getting #3247 ready soon, as atm, after we merge this PR, the examples in the docs are not complete and might cause confusion.

Until #3247 gets implemented, since we are removing the need to explicitly allow the 2i2c org or the 2i2c domain from the examples in the docs in this PR, what do you think about adding a really quick warning message in the docs pages of both authentication options that says something like:

This examples assume that `add_staff_user_ids_to_admin_user` and `add_staff_user_ids_of_type` were configured for this hub to allow 2i2c staff access as admins.

If this is not the case, consider making the appropriate changes to allow such access.

GeorgianaElena · 2023-10-10T09:53:49Z

config/clusters/2i2c-aws-us/dask-staging.values.yaml

@@ -37,7 +37,6 @@ basehub:
          authenticator_class: "github"
        GitHubOAuthenticator:
          oauth_callback_url: "https://dask-staging.aws.2i2c.cloud/hub/oauth_callback"
-          allowed_organizations:
-            - 2i2c-org
+          allowed_organizations: []


@consideRatio, not setting allowed_organizations at all has the same effect as setting it to be an empty list?

Yepp, this could also be removed entirely. I'll do so!

Resolved via 2bd0faf

GeorgianaElena · 2023-10-10T12:03:03Z

docs/hub-deployment-guide/configure-auth/github-orgs.md

@@ -97,7 +97,6 @@ You can remove yourself from the org once you have confirmed that login is worki
          GitHubOAuthenticator:
            oauth_callback_url: https://{{ HUB_DOMAIN }}/hub/oauth_callback
            allowed_organizations:
-              - 2i2c-org:hub-access-for-2i2c-staff


Since we're updating the GitHub authentication example to not include the 2i2c org, let's also update the example in https://infrastructure.2i2c.org/hub-deployment-guide/configure-auth/cilogon/#configure-the-authenticator to not include 2i2c in the allowed_domains.

Resolved via 53c8e49

With oauthenticator 16, being listed in `admin_users` makes 2i2c members have access even if they are now listed in `allowed_domains` or `allowed_organizations`. Due to this, we can avoid the need to declare `2i2c.org` and `2i2c-org` there and rely on being listed in `admin_users`.

github-actions · 2023-10-10T14:20:46Z

🎉🎉🎉🎉

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

consideRatio · 2023-10-10T14:21:33Z

Thank you for reviewing this @GeorgianaElena!!! I agree on the importance of following up in #3247, and I've done some steps towards it in 53c8e49 as a start.

consideRatio requested a review from a team as a code owner October 5, 2023 13:35

github-actions bot assigned consideRatio Oct 5, 2023

This comment was marked as resolved.

Sign in to view

consideRatio force-pushed the pr/tighten-auth-config-for-2i2c-members branch 2 times, most recently from 969e459 to 2844cc2 Compare October 5, 2023 16:22

consideRatio commented Oct 5, 2023

View reviewed changes

GeorgianaElena approved these changes Oct 10, 2023

View reviewed changes

consideRatio added 4 commits October 10, 2023 14:58

Revert typo

a9a5824

Remove empty allowed_organizations

2bd0faf

Include add_2i2c_staff_user_ids... config in auth examples

53c8e49

consideRatio force-pushed the pr/tighten-auth-config-for-2i2c-members branch from 2403389 to 53c8e49 Compare October 10, 2023 12:58

consideRatio merged commit 307bbcf into 2i2c-org:master Oct 10, 2023
31 checks passed

consideRatio mentioned this pull request Oct 10, 2023

Document add_staff_user_ids... config #3247

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grant access for 2i2c members only via admin_users #3233

Grant access for 2i2c members only via admin_users #3233

consideRatio commented Oct 5, 2023 •

edited

Loading

This comment was marked as resolved.

consideRatio Oct 5, 2023

consideRatio Oct 5, 2023

consideRatio Oct 5, 2023

consideRatio Oct 5, 2023

consideRatio Oct 5, 2023

GeorgianaElena left a comment

GeorgianaElena Oct 10, 2023

consideRatio Oct 10, 2023

consideRatio Oct 10, 2023

GeorgianaElena Oct 10, 2023

consideRatio Oct 10, 2023

github-actions bot commented Oct 10, 2023

consideRatio commented Oct 10, 2023

Grant access for 2i2c members only via admin_users #3233

Grant access for 2i2c members only via admin_users #3233

Conversation

consideRatio commented Oct 5, 2023 • edited Loading

Related

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GeorgianaElena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2023

consideRatio commented Oct 10, 2023

consideRatio commented Oct 5, 2023 •

edited

Loading