Repository navigation
KEYCLOAK-10603 adjust user-role and group-role assignments - #7970
Conversation
|
@mposolda @pedroigor can one of you take a look at this please? |
There was a problem hiding this comment.
@danielFesenmeyer The fix makes and sense.
1a81f86 to
9979bbc
Compare
|
Hi @pedroigor, I have integrated your suggestions into the PR:
Currently the PR only considers user-role and group-role assignments, but the same issue applies to client- and client-scope role-mappings. I would also adjust this within this PR - or should I create a separate one? |
9979bbc to
a3c2d70
Compare
|
Pushed a small update with a bugfix for failing integration test LDAPRoleMappingsNoImportTest. |
There was a problem hiding this comment.
Don't you think that realm-child here is a bit confusing and susceptible to mistakes? Admins now see that role as available, but it was granted by the composite. Although indirectly.
I'm not 100% if we should deal with composite roles differently in this case. So that they are not listed as available.
There was a problem hiding this comment.
@pedroigor I agree that this might be a bit confusing, at least for users who are used to the old behaviour. But otherwise it would not be possible to assign "realm-child" via the UI (at least without bigger changes to the UI).
Furthermore, there are other parts of the UI, where it is possible to assign indirectly assigned roles which are not directly assigned. There, similar mistakes might happen:
- assigning default roles to a realm
- assigning a child to a composite role
|
@danielFesenmeyer The PR is smaller now, so perhaps we can include scope mapping too. From a reviewer's perspective, the pattern should be the same. |
a3c2d70 to
f3f0739
Compare
|
@pedroigor, @mposolda: I've added the corresponding adjustments to scope mapping to this PR. Currently some model tests are failing, but these are also failing on master. I can rebase when master is fixed. |
|
@danielFesenmeyer Thanks for the PR! Sorry for the late review. The changes look good to me and I don't have any more comments to the code itself. Just one thing: Can you please rebase to latest master? This will allow to run admin console tests in our internal testing pipeline as in theory there can be some regression in them. If you have a chance to doublecheck admin console tests in your environment, it will be beneficial as well. Thanks! |
…assignments, client-scope and client "scope mappings"): allow assignments of roles which are already indirectly assigned (e.g. by composite role) - extend RoleMapperModel with method hasDirectRole(RoleModel), which only checks for direct assignment in contrast to the existing method hasRole(RoleModel) - extend ScopeContainerModel with method hasDirectScope(RoleModel), which only checks for direct scope mapping in contrast to the existing method hasScope(RoleModel) - use the new hasDirectRole and hasDirectScope methods to check whether a role is in the "available" list and whether it can be assigned (previously, the hasRole method was used for this purpose) - add hint to UI that available roles contain effectively assigned roles which are not directly assigned - adjust and extend tests
|
@mposolda Thanks for the review. I'll rebase to master and also doublecheck admin console tests - will let you know when finished. |
f3f0739 to
448bc77
Compare
Thanks! |
|
@mposolda: Rebased to a commit from yesterday. Cross-DC tests have failures, but these are the same as on master ("org.jboss.arquillian.container.spi.client.container.LifecycleException: Infinispan server startup timed out"). |
|
@danielFesenmeyer Thanks, trying pipeline build in our CI environment and will merge once it pass. |
There was a problem hiding this comment.
@danielFesenmeyer Thanks! I've checked in our pipeline that cross-dc, cluster and admin console tests are fine. Just for the reference:
https://keycloak-jenkins.com/job/universal-test-pipeline-server/788/
https://keycloak-jenkins.com/job/universal-test-pipeline-server/791/
|
Thanks to you maintainers for reviewing and merging :) |
|
hi @mposolda @pedroigor @stianst Thanks for the review on this PR, which is really helpful in our use case. Just want to ask if there's an estimated time of the Thanks for the hard work on Keycloak :) |
This PR adjusts user-role and group-role assignments as suggested in issue KEYCLOAK-10603: It allows assignments of roles which are already indirectly assigned.
When you are comfortable with the changes, I would adjust client- and client-scope role-mappings accordingly. I think it makes sense to have a consistent behavior for all sorts of role-mappings.
Basically, I made the following changes:
Can someone please review my suggested changes and provide feedback? Thanks in advance :)