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
Namespaced Roles #318
base: main
Are you sure you want to change the base?
Namespaced Roles #318
Changes from 2 commits
c5ee38d
3ac777a
cccb141
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.
Is particle a common term in this context?
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.
Does this mean we've reserved
/kc
and/ud
, but folks can define anything else if they want like/myorg/
or do you think there's always a "short" special start to the path?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.
Wouldn't this be
role_v1:/ud/groups/{group-name}/roleName
?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 a little bit concerned with making the role namespace proposal mix-up with multi-tenant proposal. Also, a little bit unsure if "tenant" is an optional part in your head. Will need to discuss this with you to get some more clarification.
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.
One thought is to leave the multi-tenant part a bit out of this design, perhaps just including it at the end as a separate section on how we could leverage role namespaces for multi-tenancy.
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.
Not sure I understand why there's a
*
hereThere 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.
Wonder if in this case you'd just use a free-form role namespace instead.
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 was thinking that clients, groups, tenants, etc, would instead of having a fixed path to their namespace would allow configuring the namespace that is associated with them.
So, a client could have it's default namespace be
/kc/clients/{client-id}
, but could be configured to use a different namespace like/my-roles/my-group-of-apps
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.
We have roles in the master realm that is used to control both the master realm and other realms when authenticating to the master console, but there are also the roles within each realm that can be used to grant users within a given realm access to only that realms admin endpoints.
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.
There's also a client created within the new realm
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 more fine-grained than what we have today. Right now, we have just manage-clients, not ability to manage an individual client. Wonder if this level is too fine-grained for roles, and rather we should look at fine-grained admin permission to manage individual clients (and also could look at integrating with external authz services in the future).
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.
Would be good to explicitly mention how role namespaces can clean-up the roles used "internally" by Keycloak's admin APIs. Getting rid of the strange clients, and having a separate/dedicated namespace for the roles so they don't conflict with "custom roles". I had something like the following in mind:
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.
Would like to see some alternatives on how roles can be represented in a token. For example options to flatten them if that makes sense. For example a client may not understand the concept of role namespaces, but would only want the roles for a specific namespace flattened.
Also, would be good to touch on how the roles can be represented in the current way they are being done in tokens with regards to realm and client 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.
Wouldn't this be incredibly complex to resolve? Also, we'd want to use client-id to specify a client rather than client-name, and often these are just generated (and really should be), so is there a meaningful regex 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.
Hmmm... I'm struggling to understand this one ;)
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.
Versioning is important, but may be complicated to incorporate at least in a first pass of role namespaces. Versioning has implications on versioning of other entity types, Rest APIs, UI, Java APIs, etc. and should probably be done as an overall concept rather than something "bespoke" for role namespace.