Conversation
rien
left a comment
There was a problem hiding this comment.
I have experimented with conventional comments in my inline comments to indicate my intention with each comment. Let me know if it is unclear or if you don't like them.
I do think this is a nice improvement and it looks thoroughly implemented. My suggestions are mostly naming-related.
Nice job 👍
| <div class="subtitle"> | ||
| Roles are given to users, which allows for more fine-grained client-level permissions. <br> | ||
| If a client requests the user info after login, it can give this user additional permissions based on the included roles. <br> | ||
| Global roles are always returned; client-specific roles are only returned for that client. <br> |
There was a problem hiding this comment.
thought: The difference between global and client-specific roles are not clear in the code.
Roles are now implicitly global when their client_id is None. I am wondering how we could make this more explicit.
There was a problem hiding this comment.
The only time there really needs to be made some distinction, is when the roles are returned. So I honestly see no better way, without adding additional unnecessary redundancy.
I realise now, I never showed it in the frontend 😅.
|
praise: @rien for the in depth feedback 👍 I find the convential comments helpful, so I quite like them 😄 |
rien
left a comment
There was a problem hiding this comment.
👍
I only have one non-blocking UI-related suggestion left about how roles are presented. Because this is only a minor details and this feature is mostly admin-facing, I leave it to you whether you want to implement it or not.
This is unfortunately not in the OAUTH2 spec, but the implementation is loosely based on how Keycloak does it to guarantee interoperability.
Roles are given to users, which allows for more fine-grained client-level permissions. If a client requests the user info after login,
roles: [...]is included and a client can give additional permissions based on this list.There are two type of roles:
The OAuth scope must include 'roles' for the roles to be included in the ID token or user info.
For example the
bestuurrole can be given to the Zeus WPI board. Applications like Gitmate can then automatically give the correct permissions. Or Tabdmin only allows users with that role, but Ieben can be given thetabdmin_admintabdmin role to still gain access.An additional tab
Rolesis added in the Zauth navbar.TODO