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

allow user-defined custom scopes #3713

Merged
merged 3 commits into from Mar 16, 2022
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Dec 13, 2021

defined with

c.JupyterHub.custom_scopes = {
    'custom:scope': {'description': "text shown on oauth confirm"}
}

Allows injecting custom scopes to roles,
allowing extension of granular permissions to service-defined custom scopes (e.g. jupyter-server/jupyter_server#165).

Custom scopes:

  • MUST start with custom:
  • MUST only contain ascii lowercase, numbers, colon, hyphen, asterisk, underscore
  • MUST define a description
  • MAY also define subscopes list(s), each of which must also be explicitly defined

Because scopes are always retrieved from the oauth /api/user endpoint, HubAuth can be used to retrieve and check for custom scopes to authorize requests.

TODO:

  • implement custom scopes support
  • tests
  • example doc that covers:
    1. defining a custom scope
    2. associating scope in custom role
    3. using custom scope with HubAuth to authorize a request

@minrk minrk marked this pull request as draft December 13, 2021 12:00
@minrk
Copy link
Member Author

minrk commented Dec 13, 2021

Can be combined with jupyter-server/jupyter_server#165 to extend granular permissions (e.g. read-only support) to single-user servers.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this @minrk!

I wonder about subscopes defining subscopes. I'm thinking perhaps we should that we validate they don't if they aren't allowed to do so, or we should test for it if they are allowed to do so.

jupyterhub/scopes.py Outdated Show resolved Hide resolved
jupyterhub/scopes.py Outdated Show resolved Hide resolved
jupyterhub/scopes.py Outdated Show resolved Hide resolved
@minrk
Copy link
Member Author

minrk commented Jan 31, 2022

I wonder about subscopes defining subscopes...we should test for it if they are allowed to do so.

They definitely are. I'll make sure that:

  1. they are allowed, and
  2. they are only allowed to subscope other custom scopes (i.e. you can't bundle jupyterhub scopes in a custom scope - that's what roles are for).

@minrk
Copy link
Member Author

minrk commented Feb 7, 2022

Working on an example for this has revealed something of an issue. The following aspects of current behavior are causing a problem:

  1. a service specifies requested permissions (a list of roles) for its tokens with its oauth_roles config
  2. when issuing a token, if any scopes are outside the owner's own permissions, the token will not be issued

Because in my test example, I want some users to have write access to the service and others to have read:

  • custom:myservice:read - some users have this
  • custom:myservice:write - other users have this

and I currently have two choices:

  • request write permissions, in which case users who don't have write permissions can't access the service
  • request read permissions, in which case users with write permissions won't have those permissions assigned to the token (effectively, they won't be able to use their write permissions)

So far, I think the only solution is to allow for services to request more permissions than they will receive. That means either:

  1. requesting multiple roles (already exists), but granting only the roles which provede a subset of the user's permissions, instead of resolving a single yes/no on the whole set of requested scopes
  2. allow assigning permissions to a token that the user does not have, and rely on request-time scope intersection (already exists, but currently as safety measure for user permissions being reduced after tokens are issued, not meant as something that should happen under typical operation)

I feel more secure in implementing the first, but as I sketch example config, it seems extremely tedious to have to write out a role for every possible combination of user permissions. Plus, if roles change, we are forced to rely on the runtime scope intersectin anyway. The second should work today just by removing the check preventing issuing tokens with more permissions than their owners, because we already reduce token scopes to subsets of owner scopes at request time. The main thing it changes is moving the state of a token having more permissions than its owner to a 'normal' situation, rather than an exceptional one.

@manics
Copy link
Member

manics commented Feb 13, 2022

Your rationale for option two makes sense, and in some ways it's similar to the permissions model for e.g. phone apps. An app can request a wide range of permissions but a user can refuse individual permissions, and it's up to the app to handle that gracefully. This means it's important to document this behaviour for developers of services.

@minrk
Copy link
Member Author

minrk commented Feb 14, 2022

@manics yup, that's the conclusion we came to after a chat with @consideRatio. Working on the implementation now, and will write up some notes and make sure the docs are clear.

The gist of the updates I want to do here:

  1. remove the permission check when assigning roles to a token
  2. update logs surrounding the "restricting-token-permission" logic to no longer indicate something's awry
  3. update the "authorization" page to note when it's asking for permissions you don't have, and that authorizing the application:
    1. will not grant the application the permission if you don't have it
    2. will grant the application permission if/when you do get it in the future (this is the part I don't like)

The mitigation of the step I didn't like (that if the user's permissions are escalated, the token's permissions may be, too) that I plan to explore is revoking tokens whose scopes have expanded. i.e. during startup:

for each user whose permissions increased:
    for each token:
        if token permissions increased as a result:
            revoke

This require's the ability to easily evaluate before/after scopes, which I'm not sure is that doable.

Ultimately, one conclusion I came to was that role assignments for tokens was perhaps not the right choice - tokens should probably have been assigned static lists of scopes directly, and reserve roles for users/groups/services. That's a tough change to make at this point, though. Several things would be easier if we did fully resolve final permissions when tokens are issued, rather than relying on roles.

@minrk
Copy link
Member Author

minrk commented Feb 28, 2022

After looking into some implementations, I'm increasingly convinced that tokens should resolve to fixed scopes immediately upon issue, rather than have lazily-evaluated permissions (roles) on tokens themselves, which can change over time. That makes things hairier.

My plan:

  1. resolve roles to (non-expanded) scopes when issuing tokens (add api_tokens.scopes column)
  2. grant only intersection of requested scopes and owner scopes when issuing tokens
  3. Perform the same mapping in db upgrade, so we don't have to revoke tokens
  4. deprecate assigning roles on tokens
  5. figure out changes to OAuth API (we currently request role names in the oauth scopes field. This should switch to actual scopes while being backward-compatible)
  6. add some convenience utilities for e.g. "owner has scope" that work for both expanded and raw scopes.

@minrk minrk mentioned this pull request Mar 4, 2022
@minrk
Copy link
Member Author

minrk commented Mar 11, 2022

To avoid this PR getting too big, I think I want to do it in 3 PRs, for easier review:

  1. (this PR) implement support for custom scopes. This works already, and is relatively straightforward. Without the later PRs, it is less useful than I want, due to the issues above. But the change is still self-contained and fairly clear, and make sense.
  2. transition from roles to scopes on tokens. This is mostly a db upgrade, and some implementation details around resolving entity->scopes.
  3. implement the ultimate goal of granular permissions based on the user (mostly has the form of more granular utilities for checking presence of scopes, and applying subsets for checking)

@minrk minrk marked this pull request as ready for review March 11, 2022 10:06
defined with

    c.JupyterHub.custom_scopes = {
        'custom:scope': {'description': "text shown on oauth confirm"}
    }

Allows injecting custom scopes to roles,
allowing extension of granular permissions to service-defined custom scopes.

Custom scopes:

- MUST start with `custom:`
- MUST only contain ascii lowercase, numbers, colon, hyphen, asterisk, underscore
- MUST define a `description`
- MAY also define `subscopes` list(s), each of which must also be explicitly defined

HubAuth can be used to retrieve and check for custom scopes to authorize requests.
@minrk
Copy link
Member Author

minrk commented Mar 11, 2022

I believe this is ready to go. There are two changes here:

  1. Custom scopes may be defined and used by external services
  2. oauth is allowed to proceed if a subset of requested roles are held by the user (allows service to request roles A, B, C and oauth will complete with A, B if user has roles A, B but not C.

This is enough to make a 'grader' service useful, which I've included in the examples.

It isn't as far as we want to go for e.g. read-only access to just one single-user server (that needs subsetting at the token level, and specific scopes on tokens), but I think it's enough for this PR to be complete, and I'll move on to separate PRs for the two later steps.

- oauth clients can request a list of roles
- authorization will proceed with the _subset_ of those roles held by the user
- in the future, this subsetting will be refined to the scope level
jupyterhub/app.py Outdated Show resolved Hide resolved
Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minrk amazing work, especially on tests and examples!

I've only added comments about documentation and such, this otherwise LGTM!

@minrk
Copy link
Member Author

minrk commented Mar 16, 2022

@consideRatio thanks for the feedback! I think it's addressed now.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, was about to merge just as tests passed, but another push arrived :p

@consideRatio consideRatio merged commit 454e356 into jupyterhub:main Mar 16, 2022
@minrk minrk deleted the custom-scopes branch March 16, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants