Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
allowed_scopes
to all authenticators to allow some users based on granted scopes #719Add
allowed_scopes
to all authenticators to allow some users based on granted scopes #719Changes from 3 commits
8071830
e801059
2175af8
b1496d5
7a211d4
a130d77
ae734ef
8b65b4f
e3b9d96
e5ea350
ea2972c
0480dc6
3bb1c2e
c7fc637
00360fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. There are two ways to think about this:
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:
username_claim
feels clearly part of authentication to me, and right now, if we don't get ausername_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 tousername_claim
are few in number. In GitHub for example, it really is justuser
- 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 theusername_claim
is not present due to a missingscope
, 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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #721 to make the missing username_claim situation slightly easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.