Refactor shared group mounting using RBAC

[viniciusdc]

Reference Issues or PRs

closes #2431

What does this implement/fix?

This adds the ability to manage group shared directory creation based on the availability of the shared-directory component/scope in any associated jupyterhub client role bonded to a group.

All default groups analyst, developer, admin, for backward compatibility, will now have the following role assigned to them:

allow-group-directory-creation-role:
   component: shared-directory
   scopes: write:shared

which is then grabed along the other roles inside the jupyterhub authenticator, and later parsed by the render_profiles function to determine which group will be mounted into the user spawned profile.

• Add a new jupyterhub client role, named allow-group-directory-creation-role, and assign it to default groups
• add new attribute to the KeyCloakOAuthenticator class to help the spawner object to access the keycloak group/roles information during runtime;
• Extended default roles testing and included a new test to make sure component/scope naming has been followed

Put a x in the boxes that apply

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds a feature)
• Breaking change (fix or feature that would cause existing features not to work as expected)
• Documentation Update
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no API changes)
• Build related changes
• Other (please describe):

Testing

• Did you test the pull request locally?
• Did you add new tests?

Any other comments?

[dcmcand]

A couple of minor stylistic comments for you @viniciusdc , I will leave the functional review to @aktech though since he is most familiar with the issue.

[dcmcand]

Is there any reason we need to loop through groups to create a list, then loop through that list to create a list of volume mounts and a list of commands and a list of init containers??

Couldn't we just generate the list of volume mounts, commands and init containers in the original loop and just use them later? That would be more readable and reduce the loops from 4 to 1

It may even be worth extracting that loop into a seperate function that takes a list of groups and group roles in and returns the 3 lists that you need further in this function.

[viniciusdc]

I complelty agree; though most of this was already here.If possible, I would prefer to include enhancements in a separate PR. No strong opinions, if you also consider this would suit this PR I can change that as well 😄

[aktech]

@viniciusdc have you tested it on local deployment?

[viniciusdc]

@viniciusdc have you tested it on local deployment?

I need to fix the tests, but yes it was working locally -- the changes are the same as the previous PR, only that I needed to reopen as something was amiss with the previous branch (I think one merge conflict commit might have been incomplete)

[aktech]

Did you deploy with jhub-apps enabled?

[viniciusdc]

Did you deploy with jhub-apps enabled?

good point, I haven't. Will do a test with that on and follow back

[viniciusdc]

@dcmcand I did a small refactor to collapse the volumeMounts into a single loop. I think if I try to collapse the command as well might get too condensed.

[viniciusdc]

@aktech I also tested within Jhub-apps, and all are working as expected. Is there anything you want me to check with you on the dashboard?

Created example-user and created group Test initially without role access:

• You can see it has the role already in due to the default "analyst" and "developer" groups I gave to it

• All roles on fresh deploy

After giving Test group the new role

[viniciusdc]

[aktech]

Created example-user and created group Test initially without role access:

Can you share your Nebari config, with which you deployed this?

[aktech]

This is going to cause problem in jhub-apps: https://github.com/nebari-dev/jhub-apps/blob/657f7c871003e5f678d865b2b4aad1845e3b5bf7/jhub_apps/service/utils.py#L53

Since that's a downstream problem, it's not blocking this but I would need to think of an alternative approach in jhub-apps to get the spawner with authenticator.

[viniciusdc]

I can add the extra information to the auth_state object (probably), this way you don't need to customize the above mock spawner, and we will not need to rely on spawner.authenticator here (which I think its messy).

Though, the reason I didn't go for it was that I was skeptical of changing it directly as

• changes might not fully apply
• It looked like that object was moved across different location in the jupyter code during the spawn process

[viniciusdc]

I will have a try on that, and check how it goes

[aktech]

I'll check if there is a less hacky way to do this on the jhub-apps.

[aktech]

Actually there isn't a non-hacky way to do this in jhub-apps. In native JupyterHub the spawner object is magically available due to the huge initialisation done at the startup.

Ref:

• https://github.com/jupyterhub/jupyterhub/blob/f4fa229645e007f3c3ea9d2ed440855f7b19595f/jupyterhub/app.py#L3189 (this initialisation, makes the later possible)
• https://github.com/jupyterhub/jupyterhub/blob/f4fa229645e007f3c3ea9d2ed440855f7b19595f/jupyterhub/handlers/pages.py#L155

We do need to set the groups via auth_state and in-fact I think that's even cleaner as all the roles and scopes parsing happens in the authenticator and other components like spawner just consumes the output via auth_state.

[viniciusdc]

Created example-user and created group Test initially without role access:

Can you share your Nebari config, with which you deployed this?

provider: local
namespace: dev
nebari_version: 2024.6.2
project_name: vini-sharerbac
domain: sharerbac.nebari.dev
ci_cd:
  type: none
terraform_state:
  type: remote
security:
  keycloak:
    initial_root_password: bu6czbofm9sbjr6632mf8mfw1mp0vs4y
  authentication:
    type: password
theme:
  jupyterhub:
    hub_title: Nebari - vini-sharerbac
    welcome:
      Welcome! Learn about Nebari's features and configurations in <a href="https://www.nebari.dev/docs/welcome">the
      documentation</a>. If you have any questions or feedback, reach the team on
      <a href="https://www.nebari.dev/docs/community#getting-support">Nebari's support
      forums</a>.
    hub_subtitle: Your open source data science platform, hosted
certificate:
  type: lets-encrypt
  acme_email: vcerutti@quansight.com
local:
  kube_context:
  node_selectors:
    general:
      key: kubernetes.io/os
      value: linux
    user:
      key: kubernetes.io/os
      value: linux
    worker:
      key: kubernetes.io/os
      value: linux
monitoring:
  enabled: false
jhub_apps:
  enabled: true

[aktech]

You can simplify this by moving the parsing in authenticator as that should own the responsibility of evaluating permissions. See #2593 (comment)

This will also, avoid this problem: #2593 (comment)

Also extra calls to keycloak is not required as done in _fetch_and_map_roles, as that information is already available in user_roles_rich (unless I am missing something).

You can simply parse the roles to find out the groups that has permission to be able to mount, by something like this (in authenticator):

    async def get_groups_with_mount_permissions(self, client_roles_rich):
        groups_with_permission_to_mount = set()
        for role in client_roles_rich:
            role_component = role["attributes"].get("component", [None])[0]
            role_scopes = role["attributes"].get("scopes", [None])[0]
            role_groups = role["groups"]
            if (role_component == "shared-directory") and (role_scopes == "write:shared-mount"):
                groups_with_permission_to_mount |= set(role["groups"])
        return list(groups_with_permission_to_mount)

Then the output of this function can be set in auth_state via (in update_auth_model function)

        auth_model["auth_state"]["groups_with_permission_to_mount"] = await self.get_groups_with_mount_permissions(
            user_roles_rich
        )

This list of groups can then be used in spawner to mount groups based on the intersection of this list with the groups the user is part of.

[viniciusdc]

Thanks, @aktech; I also agree this should be passed down to the authenticator, the reason I went with the logic in the profiles side of things was mainly due to all the mounting logic already in there. But moving will be cleaner, so I gladly accept the suggestion.

Now, regarding this part

Also extra calls to keycloak is not required as done in _fetch_and_map_roles, as that information is already available in user_roles_rich (unless I am missing something).

AKAIK, user_roles_rich does not have the groups information, but I will double check. If it does, than I completely agree that changes will be much simple

[aktech]

AKAIK, user_roles_rich does not have the groups information, but I will double check. If it does, than I completely agree that changes will be much simple

I just took a quick look, it does have users and groups, you can double check by adding an assert in there:

assert user_roles_rich == self.group_roles_map

This is what the user_roles_rich look like:

[
  {
    "id": "a301e3d4-1783-47ce-8b65-fc4a43143bce",
    "name": "jupyterhub_admin",
    "description": "jupyterhub_admin",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {},
    "groups": [
      "/admin"
    ],
    "users": []
  },
  {
    "id": "da26800f-1d07-471e-bcac-f4f72fabe020",
    "name": "dask_gateway_admin",
    "description": "dask_gateway_admin",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {},
    "groups": [
      "/admin"
    ],
    "users": []
  },
  {
    "id": "0d1b2685-d842-4bf7-bc0b-6230c0487d65",
    "name": "allow-read-access-to-services-role",
    "description": "Allow read access to services, such that they are visible on the home page e.g. conda-store",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {
      "component": [
        "jupyterhub"
      ],
      "scopes": [
        "read:services"
      ]
    },
    "groups": [
      "/analyst"
    ],
    "users": []
  },
  {
    "id": "7ff2ce27-cb08-4254-add0-d6211120445d",
    "name": "dask_gateway_developer",
    "description": "dask_gateway_developer",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {},
    "groups": [
      "/developer"
    ],
    "users": []
  },
  {
    "id": "959ba8a5-d52e-4d3f-916a-92b1f6399b39",
    "name": "allow-group-directory-creation-role",
    "description": "Grants a group the ability to manage the creation of its corresponding mounted directory.",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {
      "component": [
        "shared-directory"
      ],
      "scopes": [
        "write:shared-mount"
      ]
    },
    "groups": [
      "/admin",
      "/analyst",
      "/developer",
      "/foobar"
    ],
    "users": []
  },
  {
    "id": "db15331a-1c32-40de-9e0f-e37b751d23b7",
    "name": "jupyterhub_developer",
    "description": "jupyterhub_developer",
    "composite": false,
    "clientRole": true,
    "containerId": "37e44629-1fc3-4ec2-ada5-9985a9da9432",
    "attributes": {},
    "groups": [
      "/analyst",
      "/developer"
    ],
    "users": []
  }
]

[viniciusdc]

Nice, that's great, I was spinning up a cluster to check that, so this helped a lot!! -- I will refactor the code to use the auth_state model instead of relying on the spawner.authenticator

[aktech]

Overall it looks in good shape, I have a few comments, after which can get this in.

[aktech]

Do a set comparison instead of list as the ordered might not be guaranteed, that's why the test is failing.

[viniciusdc]

the funny thing was that I remember addressing this already but It seems I never committed it

[aktech]

After this is in, we should create a documentation PR. Here is the relevant issue for the same: nebari-dev/nebari-docs#509

[aktech]

Awesome work @viniciusdc 🎉 and thanks for review @dcmcand

This is good to go as soon as the CI is green.

[viniciusdc]

@aktech, CI will resume once #2645 and #2644 are merged. I'll also perform another local deployment to minimize delays.