Add allowed_scopes to all authenticators to allow some users based on granted scopes

[yuvipanda]

We can already control what scopes we ask for by setting 'scope'. However, OAuth2
states that not all the scopes requested may be granted, for whatever reason. This could be because the user choose not to give us the grant, or because the user themselves doesn't have access to grant us this scope (This is how Auth0 uses it for example -
https://auth0.com/docs/get-started/authentication-and-authorization-flow/authorization-code-flow/call-your-api-using-the-authorization-code-flow).

This PR adds an allowed_scopes property, which allows granting access to a subset of users based on the presence of a particular scope in the list of scopes granted. This can be used in addition to other authorization mechanisms to allow access to the hub.

[yuvipanda]

I've a local version of this deployed in 2i2c-org/infrastructure#3618, and it works well for use in auth0.

[consideRatio]

This seems reasonable and clear for future maintenance! This looks almost ready to merge to me!

• Could you update the title to include the new config name?
• Could you clarify if "scope" is expected to be returned with the access token request response and/or the userinfo response from the oauth spec, and that we get it from where its specced to be returned?
• Could you provide in code (either config help or inline code comment) reference links to the oauth2 docs of relevance?

[yuvipanda]

Thanks for the review, @consideRatio! I've done the three things :)

[consideRatio]

I've considered if this should be in an earlier stage than check_allowed. Its nice to have it here if seen as authorization logic, but it could be seen as a technical requirement - for example to get relevant info for the username. Then, it would be more suitable that not meeting this criteria is seen ss an error rather than 403.

Hmmmm... Consider if we request email scope and username_claim email and declare it as email scope required but don't get it - should that be another kind of error rather than denied authorization 403?

If we think so, this could be more like an assertion early in update_auth_model instead raising an error maybe? Wdyt?

[consideRatio]

I'm not sure what i think, but it wouldn't be breaking to relocate this check so I'm open with going onwards either way

[yuvipanda]

Good question. There are two ways to think about this:

1. It's part of authentication logic, as if we aren't granted a specific claim from a scope, we can't even log the user in (for username_claim, for example). So if the scope isn't granted, we can't actually validate who this user is. So it gets relocated elsewhere.
2. It is part of authorization logic, where scopes are used as a way to 'gate' access after we could validate who the user is. So we know who the user is, but we don't want to give them access. In this case it stays where it is.

So how do we best determine if this is part of authentication or authorization?

I went to look at possible available scopes for a bunch of authenticators:

• GitHub scopes list
• Mediawiki rights
• Standard OpenID connect scopes (as supported by Auth0)

username_claim feels clearly part of authentication to me, and right now, if we don't get a username_claim back for any reason, it's already a 403. But the 403 is a little bit unclear on why exactly the permission was denied and can be frustrating for the user. However, the scopes that can be related to username_claim are few in number. In GitHub for example, it really is just user - its unlikely you would want the username to be derived from anything else (for the most part). I think this is generally true for most auth providers, particularly given the prevalance & design decisions of openid connect. This means that when configuring, if the username_claim is not present due to a missing scope, it most likely is because the hub admin did not ask for the scope, and it's going to be clear to them in initial testing.

However, you may want to deny authorization for some subsets - for example, someone may want to setup a specific ssh key in github for a JupyterHub, and hence reject authorization if they don't grant write:public_key, even though it has nothing to do with authentication itself - we know who the user is, just don't want them in here.

I'm a big fan of @GeorgianaElena's work in #594, and think that separation of authentication and authorization here makes things easier to reason about. I think the most likely use of this feature is authorization and not authentication. username_claim being set to something that isn't present if a specific scope is not granted is IMO a different problem, and we can tackle that differently if you desire - I'd say the primary extra bit there is to find a way to provide useful error information.

All this to say, I think it's ok for this check to remain here :)

[yuvipanda]

I opened #721 to make the missing username_claim situation slightly easier.

[consideRatio]

Thanks for reasoning about this - in great depth as well @yuvipanda!! I think this makes a good case for considering this to be part of authorization (authz) logic rather than authentication (authn) logic!

[minrk]

Makes great sense. Approved by me, pending @consideRatio's more detailed feedback.

[minrk]

It is often useful to log which scopes are held and not granted, rather than just log what wasn't granted.

I suspect that the most common case for this branch will be that scope is entirely missing due to some misconfiguration or something, and it's helpful for that to appear distinct from a completely successful authentication with insufficient permissions. Another example would be a typo in a scope name. Showing the held scopes would make that much easier to identify and resolve.

[consideRatio]

If the scopes listed in this config are not granted, the user will not be allowed to log in.

I think this isn't correctly implemented currently.

• If allow_all is true, and the required_scopes isn't met, this will not deny authorization.
• Further, if a subclass like Generic's check_allowed is called, it will simply return True if the base class returns true, but not return false if the base class return false.

In general terms, check_allowed as implemented in Authenticator, replaced in OAuthenticator, and augmented in GenericOAuthenticator etc, needs to handle three situations.

1. The authenticated user gets disallowed
2. The authenticated user gets allowed
3. The authenticated user doesn't get disallowed or allowed, which will in the end lead to a default of being disallowed.

This could be implemented if check_allowed makes use of True/None/False, or if check_allowed makes use of True/Raise error/False. I'm confident we can safely take the raise error path to handle this, but I'm not confident we can adopt a True/None/False approach without breaking interactions with subclasses or JupyterHub itself.

With this considered, I think the path forward is to:

• relocate the required_scopes check to the first thing happening in check_allowed after the mentioned workaround for jupyterhub<5
• make the required_scopes check do nothing or raise an error

[yuvipanda]

@consideRatio I spent some more time thinking through this, and agree that as is worded it doesn't actually do what it says in the documentation. So I've made the following changes:

1. If this check fails, we raise a 403 (similar to what happens in other places in the CILogonAuthenticator) with a useful error message (incorporating the suggestions from @minrk)
2. I've moved the check up, but still after allow_all - as I think allow_all should basically sidestep everything. However, since this can be a bad footgun, I've also added validation that makes sure that allow_all and required_scopes are mutually exclusive - you can't have both on. I think this makes sense, as by definition if you require some scopes for authorization, you aren't actually 'allowing all'.
3. I added the fact that you can raise a 403 to the method documentation
4. I also added validation to make sure that you aren't requiring scopes you aren't requesting (thanks @minrk).

How does it look to you now?

[yuvipanda]

And if we decide throwing a 403 here is ok, we should document it in JupyterHub too - I've done so at jupyterhub/jupyterhub#4682

[consideRatio]

To give allow_all special power in allowing, making it more allowing than allowed_users for example, is not intuitive at all in my mind. It would also mess with the use case of allowing all users as long as they show up with the required issued scope - isn't this a key use case?

required_scopes and allow_all makes sense together imo - a user must be allowed and have the required scopes to be authorized - either the user is allowed via allow_all or allowed_users etc.

[manics]

The original idea behind allow_all is to allow any user without having to define allowed users/groups. For example, admins were inadvertently allowing any GitHub user to login to their installations, which is why we made a breaking change to add this, with a default of False
#625

If required_scopes is mutually exclusive with allow_all then you must also also set allowed users/groups, and also means you can't allow all. Therefore I think the most intuitive behaviour is to say allow_all is the last check after all other prerequisites. Probably worth updating the docstring for allow_all to reduce confusion in future!

[yuvipanda]

Ok so the core issue was that this was an authorization check that was kinda being implemented like an authentication check. Since 16, OAuthenticator has many different allowed_X properties that gate which users are allowed to access the hub - these are all authorization properties. required_scopes was a bit like that, but not entirely, and that led down my rabbit hole of confusion.

I've now changed the PR to make this much clearer. The traitlet is now allowed_scopes, and if all of them are present, it grants you access to the hub automatically. This now works very much like all the other allowed_X properties, and comes after allow_all as well. So we don't actually have to modify the docstring of allow_all to talk about this exception.

This is different from allowed_organizations in that all the scopes in this list must be present, while allowed_organizations is fine with any of the orgs present. I think this could also be changed, and I'll work through that.

But regardless, I think this fits in much nicer than required_scopes. I know I went down a blind alley for a while, but I'm very happy with where I've come out of my confusion with. Thanks for the review @consideRatio and @manics

[minrk]

I think the latest behavior as it is makes sense and is consistent with other allow methods, which can only grant access, and cannot be used to prevent access granted via other allow mechanisms.

If someone does want "block unless this scope is held" behavior, the method to implement is check_blocked_users, which is the negative equivalent to check_allowed (don't ask my why it's not called check_blocked for symmetry, I don't remember), and has priority over allow checks.

[consideRatio]

The traitlet is now allowed_scopes, and if all of them are present, it grants you access to the hub automatically. This now works very much like all the other allowed_X properties, and comes after allow_all as well. So we don't actually have to modify the docstring of allow_all to talk about this exception.

I see allow_all as one one of several properties allowing users. That other allow properties only allow subsets of the users allowed by allow_all isn't weird or a proplem in my mind.

I think it should be allowed to configure allow_all=True without erroring even if other allow properties are declared. I also reason that it doesn't matter if you return True from point A or B in the check_allowed function as implemented in oauthenticator as we never return False before checking all paths towards return True.

[consideRatio]

@yuvipanda I think the pivot to providing allowed_scopes instead of required_scopes is reasonable and aligns more with other config. At the same time, its fundamentally different, where there can become a need for required_scopes alongside this config, which is fine.

Naming

I'd like to propose the naming allowing_scopes, users issued all scopes in one set of scopes will be allowed access.

I propose another name than allowed_scopes as that sounds like its about scopes being allowed (which is hard to make sense of) rather than users issued those scopes are allowed. Overall allowed_scopes also sounds like it could be a requirement to meet, but its just one possible way to get allowed access as a user.

All scopes, or any of the scopes?

If we start with allowing users issued all scopes in a set of scopes via this config, we can then easily either now or later let the config accept a set of "set of scopes", where having any set of scopes fully issued would allow the user.

Example configurations

# allow users for whom both a and b are issued
# this represents the PR in its current form - a single set of scopes is provided
allowing_scope = {"a", "b"}

# allow users for whom either a are issued or b1 and b2 are issued
allowing_scopes = {
  {"a"},
  {"b1", "b2"},
}

[minrk]

I think allowed_scopes is preferable, since it shares a namespace with other allowed_ config. I think there is a benefit to all such names sharing a prefix and having consistent behavior. It's not clear to me without further explanation what allowing_scopes would mean, which is not a natural phrase, to me.

I think it should be allowed to configure allow_all=True without erroring even if other allow properties are declared.

A warning could provide some experience improvement to make it clearer that some config will have no effect, but I agree an error is too far.

[manics]

allowed_scopes is going to be inconsistent anyway, since all scopes are required, whereas with the other properties you only have to be a member of one.

If we wanted to be consistent we could go with the suggestion in #719 (comment) of makeing allowed_scopes a list of lists?

[consideRatio]

Considering naming, here is an overview of the config that allows users:

• admin_users
• allowed_users
• (oauth) allow_all
• (oauth) allow_existing_users
• (bitbucket) allowed_teams
• (cilogon) allowed_domains
• (generic, openshift) allowed_groups
• (github) allowed_organizations
• (gitlab) allowed_gitlab_groups
• (globus) allowed_globus_groups
• (google) allowed_google_groups
• (google) admin_google_groups

We have the prefixes admin, allow, and allowed so far, so adding allowing would be another.

Looking at the terminology in https://datatracker.ietf.org/doc/html/rfc6749#section-3.3, I think the terminology is "requested scope" and "granted scope" (the access token is "issued" and the scope the token has is "granted").

Do you consider allow_granted_scope or allowed_granted_scope to be an improvement to allowed_scope @yuvipanda @minrk ? I think it would help convey the config function better from its name than allowed_scope.

[manics]

allowed_scopes or similar is going to be inconsistent anyway, since all scopes are required, whereas with the other properties only one is required.

[consideRatio]

allowed_scopes or similar is going to be inconsistent anyway, since all scopes are required, whereas with the other properties only one is required.

If we support the sets of sets / lists of lists version, its like allowed_scope_sets, where each set of scopes are granted, making it consistent with being individual parts allowing a user access, but each part is a "scope set" instead of individual requested scope entries.

Function when providing a single set could be made to mean "allow when any of scope entries are granted" (OR) or "allow when all of scope entries are granted" (AND), do you think it should be the OR version if just passing the config a single set/list of scopes @manics?

[manics]

I don't know..... given this is a new property we could go for the list-of-lists only? It's more complicated for admins but at least it's unambiguous?

[yuvipanda]

Thanks for the comments everyone, I'm very swamped this week but will come back to this next week.

[yuvipanda]

Sigh, momentum seems so important to me getting stuff done. There are merge conflicts here already :(

[yuvipanda]

ok, so my understanding is that there are two questions at play:

1. Naming
2. Should this be a list of lists from the start?

For naming, I would like to propose we keep this as is, as I agree with @minrk's comments in #719 (comment). I also think allowing would make sense if any of the scopes granted access, but in this case, you need all to be granted access.

For list of lists, the question for me is if we can add that in the future in a way that doesn't break backwards compatibility. Can we meaningfully differentiate between a 'list of strings' vs 'list of lists' scenario? The way I approached this is by trying to figure out what documentation for such a property in the future would look like. If we add this feature in the future, here's what the docstring may be like

Allow users to login based on the scopes they have been granted.

If this is a list of strings, then users must be granted *all* these scopes
to be able to log in.

If this is a list of list of strings, then the user must be granted all the
scopes in at least one of the lists to be allowed to log in.

This reads natural enough to me. You can either have a list of list of strings, or for the most common case, a list of strings. It's unambiguous. We don't even know if admins would want to have a list of list of strings. We do know that they would want to have a list of strings. Given that we can evolve this later on in a backwards compatible way, I think it's safe to try this one as is.

So to summarize:

1. I think allowed_scopes is a good enough name, and consistent with the rest of our config.
2. We can add list of list of strings in the future if that sees an actual use case.

I would love to see this be merged! I think it's ready to go as is, now that I've fixed the merge conflicts.

[minrk]

I like all of @yuvipanda's explanation and reasoning. 👍 to merge as-is from me.

[yuvipanda]

anyone wanna hit merge? :)

[yuvipanda]

yay, thank you @minrk