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

[All] Adding overrides to user_is_authorized method #571

Closed

Conversation

npapapietro
Copy link

@npapapietro npapapietro commented Feb 1, 2023

Motivation

The overall goal of this PR is to expose some override functionality to the user_is_authorized method that will allow downstream consumers (most notably the Kubespawner) of the authenticator make further decisions on a smaller subset of group/role type properties. My goal is to create a path for allowing auth logic in KubeSpawner.profile_list.

Changes

In each implementation where the concept of allowed user groups/projects/roles exist, there is a simple override kwarg option passed.

async def user_is_authorized(self, auth_model, **overrides):
  allowed_groups = overrides.pop("allowed_groups", self.allowed_groups)
  # rest of logic

Each override option has been documented in the respective docstring as well as testing added to cover the new changes. Some of the implementations have no concept of allowed groups currently implemented and the contributing guidelines favor moving towards GenericOAuthenticator, so they were ignored (cilogin for example).

Risks

This method is called by the authenticate method.

Mitigations

Testing was created and no changes were made inside the authenticate method.

PS:
I also had to bump isort bugfix version to fix the pre-commit scripts. Also flake8==6.0.0 hook breaks for python==3.7.

@welcome
Copy link

welcome bot commented Feb 1, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

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 a lot for opening this PR @npapapietro!

Can you please help me understand it better, in order to be able to review it?

expose some override functionality to the user_is_authorized method that will allow downstream consumers (most notably the Kubespawner) of the authenticator make further decisions on a smaller subset of group/role type properties

Can you please describe in more detail how this override functionality added to user_is_authorized would be used in a profile_list context?

My goal is to create a path for allowing auth logic in KubeSpawner.profile_list.

I believe @consideRatio created a blog post in https://discourse.jupyter.org/t/tailoring-spawn-options-and-server-configuration-to-certain-users/8449 describing how can the auth_state can be used to influence the profile_list. And this is where auth_state is built before being returned by a successful authenticate call.

@npapapietro
Copy link
Author

npapapietro commented Feb 2, 2023

Absolutely,

In side the Kubespawner._render_options_form method, one can check/filter the rendered profile list by the base class property Spawner.authenticator here. Extending the method on OAuthenticator, allows for this check without exposing too much of the internals. This would require a PR to the kubespawner to expand this method and altering the schema of the profile list. My suggestion would be to add the respective **override kwarg option to the profile list depending on the chosen Authenticator.

For example

c.KubeSpawner.profile_list = [
  {
      'display_name': 'Training Env',
      'slug': 'training-python',
      'default': True,
      # for example
      'authenticator_override': {
         'allowed_groups': ['group1', 'group2'], # generic
         'allowed_organizations': ['org1', 'org2'], # github
      },
      'kubespawner_override': {
        'image': 'datascience/bigGPU:latest',
        'cpu_limit': 48,
        'mem_limit': '96G',
        'extra_resource_guarantees': {"nvidia.com/gpu": "2"},
    }
  ]

From here, one can use the z2jh values to define profile lists depending on Auth without needing to inline python code inside .Values.hub.extraConfig

@GeorgianaElena
Copy link
Member

Thanks a lot for the additional context @npapapietro!

Verify understanding

From what I understand, the overrides passed to the user_is_authorized method are meant to make the method usable after the user was authenticated into the hub and to make it callable multiple times, for different groups/teams etc membership. Is this understanding correct @npapapietro?

Possible resolution of the current implementation

If so, from an oauthenticator perspective and not taking into account the kubespawner desired end result, I don't think it makes sense to add these overrides to the user_is_authorized method, as this will only be used during the authenticate call, and it will always use the object configured traits. I think having this will be confusing for any non-kubespawner setups.

Possible alternate solution

From what I understand what's needed is a generic function for group/role restriction check that's specific to each oauthenticator, right?

Maybe a possible solution to this is to have a generic check_user_restricted method with the following signature:

def check_user_restricted(auth_state, **kwargs)

Then, the user_is_authorized (maybe renamed to user_is_authorized_to_login) function will call this, passing it all the self.<trait> relevant configs and do all the relevant logging, exception handling.

What do you think @npapapietro ?

Kubespawner extended context

I took a peek to jupyterhub/kubespawner#700 to make sure I got the context right. It is my underestanding that this is a complicated topic with additional information about it available at jupyterhub/kubespawner#589.

So, I will defer merging changes to oauthenticator that are not relevant for oauthenticator, given no additional context from other packages like kubespawner don't make sense to be merged.

@consideRatio
Copy link
Member

consideRatio commented Nov 27, 2023

I currently understand this PR's intent to be to adjust the authorization check (now check_allowed) to be usable by other components (like KubeSpawner) to check if an authenticated and authorized user is authorized under different authenticator configuration.

I'd like to steer towards the alternative approach of relying on Authenticator class agnostic information like:

  • the user's auth state
  • the user's JupyterHub RBAC scopes (permissions)
  • the user's JupyterHub groups membership

I'll go for a preliminary close on this PR for now since it hasn't received a response since March, it can easily be re-opened though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants