-
Notifications
You must be signed in to change notification settings - Fork 93
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
Fetch JupyterHub roles from Keycloak #2447
Conversation
move `reset_managed_roles_on_startup` to implementation class. This reduces the diff substantially.
) | ||
role["users"] = [user["username"] for user in users] | ||
|
||
return list(roles.values()) |
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.
🤔 this still does not include all the roles that keycloak returns in oauth for users.
Currently it gives us: uma_authorization
, offline_access
, default-roles-nebari
; jupyterhub_admin
, dask_gateway_developer
, dask_gateway_admin
, and jupyterhub_developer
, however the oauth response also includes the following (for non-admin user):
manage-account
, argo-developer
, dask_gateway_developer
, grafana_viewer
, conda_store_developer
, argo-viewer
, grafana_developer
, manage-account-links
, view-profile
.
The difference appears to be largely explained by the oauth response including roles for all clients, whereas the logic above only loads the jupyterhub client roles. I think this is ok.
verify=False, | ||
) | ||
user = response.json() | ||
assert set(user["roles"]) == { |
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.
This is passing locally but failing on CI. I suspect this is because the CI is using docker images with older JupyterHub versions - if I am right, merging nebari-dev/nebari-docker-images#140 should make the tests green.
There are some conflicts and now we can merge the latest main and test on the CI. |
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.
This looks good to me. Verified with a local deployment.
The load_roles returned are consistent with JupyterHub's recommended format:
Below is a sample for posterity:
[
{
"name": "default-roles-nebari",
"description": "${role_default-roles}",
"groups": [],
"users": [
"service-account-jupyterhub",
"aktech"
]
},
{
"name": "offline_access",
"description": "${role_offline-access}",
"groups": [],
"users": []
},
{
"name": "uma_authorization",
"description": "${role_uma_authorization}",
"groups": [],
"users": []
},
{
"name": "dask_gateway_admin",
"description": "dask_gateway_admin",
"groups": [
"/admin"
],
"users": []
},
{
"name": "jupyterhub_admin",
"description": "jupyterhub_admin",
"groups": [
"/admin"
],
"users": []
},
{
"name": "dask_gateway_developer",
"description": "dask_gateway_developer",
"groups": [
"/developer"
],
"users": []
},
{
"name": "jupyterhub_developer",
"description": "jupyterhub_developer",
"groups": [
"/analyst",
"/developer"
],
"users": []
}
]
I have verified the tests works locally, the failure in cypress tests is due to this: #2463 That is irrelevant to this PR, so I am going to merge this one. |
Reference Issues or PRs
Closes #2308
It is useful to think about this PR as having two parts:
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?
This PR includes #2427 for now, but ideally #2427 would be merged first.
As of this PR the roles have no scopes. There are a few ways to associate the roles with a scope, one is having a key-value map, another is encoding the scope specification in the role name or description; this is tracked in #2432.