23404 improve client role listing #24012

sschu · 2023-10-16T10:53:08Z

First draft.

sschu · 2023-10-16T11:39:23Z

First iteration on improving the situation here. I would rather use an iterative approach here rather than having a big bang rewrite also changing the admin API. The PR also tries to improve listing effective roles for a user as this also times out with many clients and it is essentially the same code. In the first iteration:

I improved the RoleMapper.convertToModel() method to be O(1) instead of O(#clients) as it essentially made all listing operations O(#clients^2).
I improved the listing the effective role mapping to not be O(#clients) but O(#role-mappings).
I refactored the code to make it more readable and explicit, also because code sharing between available and effective role mappings won't be possible in the future (the former definitely has to go to the database). But this can be debated.

What is missing is an actual improvement to listing the available role mappings, this is as bad as before. This definitely has to look for roles that can be assigned so it has to somehow iterate over all roles. To make this efficient, this (and pagination) has to happen on the database level. This would be my next step. Other open questions:

The fact that only client roles are listed is due to the fact that the same for realm roles is taken from the admin api. For effective role mappings, this could easily include realm roles as well meaning the UI would only have to make one call (currently assigned realm roles are filtered out). This would also be possible for available roles.
listCompositeRealmRoleMappings doesn't do what it says it just lists subroles of the default role instead of the role provided as parameter. I don't know if this is intended but it looks super odd to me.
In the first iteration, I would probably leave everything in admin-ui-extwith the option of using it to improve the admin API later.

sschu · 2023-10-16T15:30:06Z

The challenge with putting pagination in the database for available roles is that it also needs to contain the fine-grained permission check for assignability by the user. I'll first try to come up with something without fine-grained permissions and check if I can extend on that later.

sschu · 2023-10-17T15:23:52Z

With fine-grained permissions, the most efficient way would be to:

collect all roles the user can manage via fine-grained permissions directly
collect all roles the user can manage indirectly via fine-grained permissions on a client
subtract all roles the user has directly assigned.

Ideally, this would all happen in the database so the database can directly do the pagination. Due to the nature of the user filtering by policies, I fear this won't be possible. So I would probably go for a similar approach to how fine-grained permissions are used when searching for users: add a method getRolesWithMapPermissions() to RolePermissions.java and use its output as input for a DB query with pagination.

To filter out roles that are already assigned, I am thinking about changing the RoleProvider interface to also a query for roles with the exception of certain IDs.

danielFesenmeyer · 2023-10-24T09:11:54Z

I think this is fine as a starting point.
I'd suggest the following minor improvements before merging:

move the clientRole-search-condition to a re-usable method (AvailableRoleMappingResource, clientRole -> clientRole.getClient().contains(search) || clientRole.getRole().contains(search)), to make sure the implementation stays consistents between endpoints
remove no longer needed variables and imports

One more point with regard to shifting query logic to the DB level: I think it would be great to make sure that no unnessary entities (as clients) are loaded into the cache. Besides query performance, that seems to be another weakness of the current implementation.

thomasdarimont

Looks good to me, just a few minor remarks.

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/RoleMappingResource.java

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/model/RoleMapper.java

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AvailableRoleMappingResource.java

sschu · 2023-10-25T09:06:26Z

@danielFesenmeyer @thomasdarimont Thanks for your suggestions! I'll pick them up next week when I am back from vacation.

vramik

Thank you @sschu for this PR! It looks good to me, just few minor comments.

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/model/RoleMapper.java

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/RoleMappingResource.java

server-spi/src/main/java/org/keycloak/models/RoleProvider.java

model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java

model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java

sschu · 2023-11-01T13:56:05Z

Thanks for the feedback @vramik !
I am currently wondering if getRolesStream is actually the best name for the newly introduced method. In other places, methods taking a search string parameter are typically called searchXyz. It might even make sense to move the methods to the RoleLookupProvider interface (or even introduce a RoleQueryProvider? If I look at how it's done for Users, "lookup" typically seems to refer to methods for retrieving individual elements based on various parameters, not for searching for a set of elements).
Also, I am thinking of switching to the Criteria API to not bloat the RoleEntity.java with named functions and have some reuse. The search semantics is also not complete as searching for client roles should also include the client IDs, not just the role names.

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/EffectiveRoleMappingResource.java

vramik · 2023-11-02T09:15:33Z

Thank you @sschu!

I am currently wondering if getRolesStream is actually the best name for the newly introduced method. In other places, methods taking a search string parameter are typically called searchXyz. It might even make sense to move the methods to the RoleLookupProvider interface (or even introduce a RoleQueryProvider? If I look at how it's done for Users, "lookup" typically seems to refer to methods for retrieving individual elements based on various parameters, not for searching for a set of elements).

Good point, I think it makes sense to rename the method to searchXzy and move it to RoleLookupProvider. I wouldn't create new interface RoleQueryProvider for this.

Also, I am thinking of switching to the Criteria API to not bloat the RoleEntity.java with named functions and have some reuse.

Currently the jpa storage layer is not consistent in using queries. Whether it's named queries, criteria API or even native queries at some places. If you find it convenient, e.g. to have some reuse there, then go for it. I wouldn't mind the current approach with named queries as well.

The search semantics is also not complete as searching for client roles should also include the client IDs, not just the role names.

Would be the searching by client IDs required by / useful for by admin UI? Or do you mention it in sake of completeness?

sschu · 2023-11-02T12:22:36Z

It's just the existing behaviour to search in client id and role name when searching for client roles, see

keycloak/rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/RoleMappingResource.java

Line 35 in 6736f31

    
           return mapping(predicate).filter(clientRole -> clientRole.getClient().contains(search) || clientRole.getRole().contains(search))

.

sschu · 2023-11-03T15:37:16Z

I updated the implementation so listing effective role mappings sorts correctly and available role mappings are listed using the new function from the RoleLookupProvider for the case that fine-grained permissions are disabled. Next step is to implement listing available role mappings with fine-grained permissions enabled.
Some open questions for me:

I added the new search method as a default method so alternative RoleLookupProviders still compile. However, they will encounter a runtime exception if they don't implement the method so this might not be ideal.
Searching for available client roles in the RoleLookupProvider searches for matches in role name and client name because that's what the current API is doing to list available client roles. However, it is not consistent with what the other search functions are doing, they search in role name and role description.
Listing effective role mappings doesn't offer pagination (it was like that before) but the admin console actually sends a request with pagination, at least when listing roles for a user.
Listing effective role mappings could probably be more efficient by going to the DB directly similar to available role mappings. But it might not be that important considering that the amount of assigned roles should not be that big.
The way this is currently going I see a lot of code duplication searching for realm/client/all roles. I am wondering if it would be better to just have an RoleQueryType enum with values CLIENT_ROLES,REALM_ROLES,ALL since inside the code differences are minimal.

WDYT? @vramik @danielFesenmeyer @thomasdarimont

cypress · 2023-11-15T17:29:36Z

1 flaky test on run #9835 ↗︎

0	537	49	0	1

Details:

Merge 64010ca1d9fd552ee105e780020521e62ee63135 into `07a3def`...
Project: Keycloak Admin UI	Commit: `2e8e2edef0 ℹ️`
Status: Passed	Duration: 07:25 💡
Started: Nov 15, 2023 5:22 PM	Ended: Nov 15, 2023 5:29 PM

cypress/e2e/clients_test.spec.ts • 1 flaky test • chrome

View
Output

Test		Artifacts
Clients test > Keys tab test > Generate new keys		`Test Replay` `Screenshots`

Review all test suite changes for PR #24012 ↗︎

ghost

Unreported flaky test detected, please review

ghost · 2023-11-20T16:27:09Z

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:313)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

vramik

Thank you @sschu! The PR looks great. I've several comments.

I added the new search method as a default method so alternative RoleLookupProviders still compile. However, they will encounter a runtime exception if they don't implement the method so this might not be ideal.

I agree this is not ideal and I believe we could leave it without default implementation. @ahus1 wdyt? If it turns out we have to have a default implementation, I'd advise to return either null or Stream.empty(), but from my POV the best would be to not have default implementation there.

Searching for available client roles in the RoleLookupProvider searches for matches in role name and client name because that's what the current API is doing to list available client roles. However, it is not consistent with what the other search functions are doing, they search in role name and role description.

True, @edewit is this desired behavior from UI point of view?

Listing effective role mappings doesn't offer pagination (it was like that before) but the admin console actually sends a request with pagination, at least when listing roles for a user.

Interesting, could you please link here a place in code from where the admin console sends the request? I'd say this is something we should look at, probably within follow up issue, could you please create one?

Listing effective role mappings could probably be more efficient by going to the DB directly similar to available role mappings. But it might not be that important considering that the amount of assigned roles should not be that big.

I agree, if you'd consider to make this more efficient, please do it within follow-up issue and PR.

The way this is currently going I see a lot of code duplication searching for realm/client/all roles. I am wondering if it would be better to just have an RoleQueryType enum with values CLIENT_ROLES,REALM_ROLES,ALL since inside the code differences are minimal.

You're right, we may want to refactor / improve the code in future, would you like to create enhancement issue for it?

server-spi/src/main/java/org/keycloak/storage/role/RoleLookupProvider.java

vramik · 2023-11-20T10:21:59Z

server-spi/src/main/java/org/keycloak/storage/role/RoleLookupProvider.java

+    default Stream<RoleModel> searchForClientRolesStream(RealmModel realm, String search, Integer first, Integer max, Stream<String> excludedIds) {
+        throw new UnsupportedOperationException("Not supported.");
+    }


I'd suggest to reorder parameters in the method signature as first and max are always at the end in other providers. I've noticed that this method differs from the other one only be the order of params, should we consider a rename?

I moved the excludedIds parameter behind the search. I wouldn't want to rename the method as it does exactly the same thing as the others, just based on different parameters.

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/RoleMappingResource.java

server-spi/src/main/java/org/keycloak/storage/role/RoleLookupProvider.java

model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/AvailableRoleMappingResource.java

ahus1 · 2023-11-21T11:43:12Z

from my POV the best would be to not have default implementation there.

I agree that a default implementation only makes sense if it returns a meaningful default, and returning an exception is no meaningful default IMHO. If there is no meaningful default implementation, don't provide one as @vramik suggested.

…ppings Signed-off-by: Sebastian Schuster <sebastian.schuster@bosch.io> Co-authored-by: Vlasta Ramik <vramik@users.noreply.github.com>

sschu · 2023-11-21T14:50:00Z

@vramik @ahus1 Thanks for the feedback! I included all remarks so from my perspective the PR is good to go if CI is green.

sschu · 2023-11-21T14:51:57Z

@vramik With regards to the admin UI using pagination when listing effective role assignments: I assume this is the code but not sure, frontend isn´t my area:

keycloak/js/apps/admin-ui/src/components/role-mapping/resource.ts

Lines 30 to 42 in 153d1a9

    
           const fetchEndpoint = async ({ 
        
             id, 
        
             type, 
        
             first, 
        
             max, 
        
             search, 
        
             endpoint, 
        
           }: Query): Promise<any> => 
        
             fetchAdminUI(`/ui-ext/${endpoint}/${type}/${id}`, { 
        
               first: (first || 0).toString(), 
        
               max: (max || 10).toString(), 
        
               search: search || "", 
        
             });

edewit · 2023-11-21T15:20:04Z

@sschu yep that is correct

sschu · 2023-11-21T17:01:23Z

@vramik I don't think the test failure is related to my changes. I also created the follow-up issues you asked for.

vramik

Thank you @sschu!

ahus1

Approving as of @vramik's review. Thank you to everyone involved in this PR to make this performance improvement happen!

ahus1 · 2023-11-22T08:52:40Z

@edewit - could you please re-approve this PR for the UI team, as your last review is now stale after the lasted rework from the author. For the one GHA that failed due to flakiness I triggered a rerun.

Thanks!

sschu · 2023-11-22T14:17:18Z

Cool, thanks a lot everybody!

sschu requested a review from a team as a code owner October 16, 2023 10:53

ghost added the team/ui label Oct 16, 2023

sschu marked this pull request as draft October 16, 2023 10:54

sschu force-pushed the 23404_improve_client_role_listing branch 2 times, most recently from fd84df3 to 7080772 Compare October 16, 2023 11:43

ghost added the flaky-test label Oct 16, 2023

jonkoops requested a review from edewit October 23, 2023 22:11

thomasdarimont suggested changes Oct 24, 2023

View reviewed changes

vramik self-requested a review October 25, 2023 18:16

This was referenced Nov 1, 2023

Flaky test: org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createGroup #24337

Closed

Flaky test: org.keycloak.testsuite.model.infinispan.CacheExpirationTest#testCacheExpiration #23769

Open

vramik reviewed Nov 1, 2023

View reviewed changes

ghost mentioned this pull request Nov 1, 2023

Flaky test: org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#testAllConcurrently #23056

Closed

sschu commented Nov 1, 2023

View reviewed changes

rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/EffectiveRoleMappingResource.java Show resolved Hide resolved

vramik added the team/store label Nov 2, 2023

sschu force-pushed the 23404_improve_client_role_listing branch from 3b8ec86 to 68fe375 Compare November 3, 2023 15:41

sschu force-pushed the 23404_improve_client_role_listing branch from 64010ca to 6fe63d3 Compare November 16, 2023 10:16

ghost reviewed Nov 20, 2023

View reviewed changes

vramik suggested changes Nov 21, 2023

View reviewed changes

sschu dismissed edewit’s stale review via 7decfac November 21, 2023 12:19

sschu force-pushed the 23404_improve_client_role_listing branch from 0ca012f to a2c7993 Compare November 21, 2023 13:02

23404 more efficient listing of assigned and available client role ma…

d41a7da

…ppings Signed-off-by: Sebastian Schuster <sebastian.schuster@bosch.io> Co-authored-by: Vlasta Ramik <vramik@users.noreply.github.com>

sschu force-pushed the 23404_improve_client_role_listing branch from a2c7993 to d41a7da Compare November 21, 2023 14:42

sschu requested review from a team as code owners November 21, 2023 14:42

ghost added team/core team/cloud-native labels Nov 21, 2023

This was referenced Nov 21, 2023

Refactor RoleProvider and RoleLookupProvider #24887

Open

Add pagination to listing of effective role mappings #24891

Open

Improve efficiency of listing effective role mappings #24892

Open

vramik approved these changes Nov 22, 2023

View reviewed changes

ahus1 enabled auto-merge (squash) November 22, 2023 08:49

ahus1 approved these changes Nov 22, 2023

View reviewed changes

ahus1 requested a review from edewit November 22, 2023 08:51

stianst removed this from the 23.0.0 milestone Nov 22, 2023

edewit approved these changes Nov 22, 2023

View reviewed changes

ahus1 merged commit 030f42e into keycloak:main Nov 22, 2023
69 checks passed

sschu mentioned this pull request Mar 1, 2024

Performance problem with big number of roles #12885

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

23404 improve client role listing #24012

23404 improve client role listing #24012

sschu commented Oct 16, 2023

sschu commented Oct 16, 2023

sschu commented Oct 16, 2023

sschu commented Oct 17, 2023 •

edited

Loading

danielFesenmeyer commented Oct 24, 2023 •

edited

Loading

thomasdarimont left a comment

sschu commented Oct 25, 2023

vramik left a comment

sschu commented Nov 1, 2023

vramik commented Nov 2, 2023

sschu commented Nov 2, 2023

sschu commented Nov 3, 2023 •

edited

Loading

cypress bot commented Nov 15, 2023

ghost left a comment

ghost commented Nov 20, 2023

vramik left a comment •

edited

Loading

vramik Nov 20, 2023

sschu Nov 21, 2023

ahus1 commented Nov 21, 2023

sschu commented Nov 21, 2023

sschu commented Nov 21, 2023 •

edited

Loading

edewit commented Nov 21, 2023

sschu commented Nov 21, 2023 •

edited

Loading

vramik left a comment

ahus1 left a comment

ahus1 commented Nov 22, 2023

sschu commented Nov 22, 2023

23404 improve client role listing #24012

23404 improve client role listing #24012

Conversation

sschu commented Oct 16, 2023

sschu commented Oct 16, 2023

sschu commented Oct 16, 2023

sschu commented Oct 17, 2023 • edited Loading

danielFesenmeyer commented Oct 24, 2023 • edited Loading

thomasdarimont left a comment

Choose a reason for hiding this comment

sschu commented Oct 25, 2023

vramik left a comment

Choose a reason for hiding this comment

sschu commented Nov 1, 2023

vramik commented Nov 2, 2023

sschu commented Nov 2, 2023

sschu commented Nov 3, 2023 • edited Loading

cypress bot commented Nov 15, 2023

1 flaky test on run #9835 ↗︎

Review all test suite changes for PR #24012 ↗︎

ghost left a comment

Choose a reason for hiding this comment

ghost commented Nov 20, 2023

Unreported flaky test detected

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginSuccessWithCRLSignedWithIntermediateCA3FromTruststore

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromFile

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

vramik left a comment • edited Loading

Choose a reason for hiding this comment

vramik Nov 20, 2023

Choose a reason for hiding this comment

sschu Nov 21, 2023

Choose a reason for hiding this comment

ahus1 commented Nov 21, 2023

sschu commented Nov 21, 2023

sschu commented Nov 21, 2023 • edited Loading

edewit commented Nov 21, 2023

sschu commented Nov 21, 2023 • edited Loading

vramik left a comment

Choose a reason for hiding this comment

ahus1 left a comment

Choose a reason for hiding this comment

ahus1 commented Nov 22, 2023

sschu commented Nov 22, 2023

sschu commented Oct 17, 2023 •

edited

Loading

danielFesenmeyer commented Oct 24, 2023 •

edited

Loading

sschu commented Nov 3, 2023 •

edited

Loading

vramik left a comment •

edited

Loading

sschu commented Nov 21, 2023 •

edited

Loading

sschu commented Nov 21, 2023 •

edited

Loading