keycloak / keycloak

Open Source Identity and Access Management For Modern Applications and Services
https://www.keycloak.org
Apache License 2.0
23.42k stars 6.77k forks source link

Federated users cannot delete groups they are a member of with LDAP_ONLY mode #30938

Open Salvoxia opened 4 months ago

Salvoxia commented 4 months ago

Before reporting an issue

Area

ldap

Describe the bug

If a federated user has the roles to delete groups in a realm and they try to delete a group they are a member of while using an ldap-group mapper with mode LDAP_ONLY, the admin console shows an error Network response was not OK. Press here to refresh and continue.

Version

25.0.1

Regression

Expected behavior

Keycloak should delete the group and propagate the deletion to LDAP (related to #29099), effectively removing the user from the no-longer existing group in LDAP and Keycloak.

Actual behavior

Admin console shows an error message image When following the link the group has not been deleted. A workaround is for the user trying to delete the group to leave the group first.

The Keycloak logs show a unique key constraint violation:

2024-06-28 18:23:16,671 DEBUG [org.hibernate.orm.jdbc.batch] (executor-thread-38) PreparedStatementDetails did not contain PreparedStatement on #releaseStatements : delete from public.KEYCLOAK_GROUP where ID=?                 
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)                                                                                                                                                                 
    at org.keycloak.connections.jpa.PersistenceExceptionConverter.invoke(PersistenceExceptionConverter.java:66)                                                                                                                   
    ... 45 more                                                                                                                                                                                                                   
Caused by: org.postgresql.util.PSQLException: ERROR: duplicate key value violates unique constraint "sibling_names"                                                                                                               
  Detail: Key (realm_id, parent_group, name)=(5b350cda-0cd5-4745-af22-7ccc3bcfe746,  , Test3) already exists.                                                                                                                     
    at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2725)                                                                                                                                 
    at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2412)                                                                                                                                       
    ... 65 more                                                                                                                                                                                                                   

2024-06-28 18:23:17,079 DEBUG [org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl] (executor-thread-39) HHH000420: Closing un-released batch                                                                                  
2024-06-28 18:23:17,079 DEBUG [org.hibernate.orm.jdbc.batch] (executor-thread-39) PreparedStatementDetails did not contain PreparedStatement on #releaseStatements : insert into public.KEYCLOAK_GROUP (NAME,PARENT_GROUP,REALM_ID
2024-06-28 18:23:17,079 DEBUG [org.hibernate.orm.jdbc.batch] (executor-thread-39) PreparedStatementDetails did not contain PreparedStatement on #releaseStatements : insert into public.KEYCLOAK_GROUP (NAME,PARENT_GROUP,REALM_ID
2024-06-28 18:23:17,079 DEBUG [org.keycloak.services.error.KeycloakErrorHandler] (executor-thread-38) Error response 409: org.keycloak.models.ModelDuplicateException: Duplicate resource error                                   
    at org.keycloak.connections.jpa.PersistenceExceptionConverter.convert(PersistenceExceptionConverter.java:103)                                                                                                                 
    at org.keycloak.connections.jpa.PersistenceExceptionConverter.invoke(PersistenceExceptionConverter.java:68)                                                                                                                   
    at jdk.proxy2/jdk.proxy2.$Proxy66.flush(Unknown Source)                                                                                                                                                                       
    at org.keycloak.models.jpa.JpaRealmProvider.createGroup(JpaRealmProvider.java:717)                                                                                                                                            
    at org.keycloak.storage.GroupStorageManager.createGroup(GroupStorageManager.java:123)                                                                                                                                         
    at org.keycloak.models.cache.infinispan.RealmCacheSession.createGroup(RealmCacheSession.java:1109)                                                                                                                            
    at org.keycloak.models.cache.infinispan.RealmAdapter.createGroup(RealmAdapter.java:1489)                                                                                                                                      
    at org.keycloak.models.RealmModel.createGroup(RealmModel.java:643)                                                                                                                                                            
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper.createKcGroup(GroupLDAPStorageMapper.java:805)                                                                                                   
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper.updateKeycloakGroupTreeEntry(GroupLDAPStorageMapper.java:320)                                                                                    
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper.updateKeycloakGroupTree(GroupLDAPStorageMapper.java:299)                                                                                         
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper.syncDataFromFederationProviderToKeycloak(GroupLDAPStorageMapper.java:184)                                                                        
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper.findKcGroupOrSyncFromLDAP(GroupLDAPStorageMapper.java:388)                                                                                       
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper$LDAPGroupMappingsUserDelegate.lambda$getLDAPGroupMappingsConverted$0(GroupLDAPStorageMapper.java:770)                                            
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)                                                                                                                                        
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1708)                                                                                                                                   
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)                                                                                                                                            
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)                                                                                                                                     
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)                                                                                                                                       
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)                                                                                                                                            
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)                                                                                                                                           
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper$LDAPGroupMappingsUserDelegate.getLDAPGroupMappingsConverted(GroupLDAPStorageMapper.java:772)                                                     
    at org.keycloak.storage.ldap.mappers.membership.group.GroupLDAPStorageMapper$LDAPGroupMappingsUserDelegate.getGroupsStream(GroupLDAPStorageMapper.java:703)                                                                   
    at org.keycloak.models.cache.infinispan.entities.CachedUser.lambda$new$3(CachedUser.java:74)                                                                                                                                  
    at org.keycloak.models.cache.infinispan.DefaultLazyLoader.get(DefaultLazyLoader.java:43)                                                                                                                                      
    at org.keycloak.models.cache.infinispan.entities.CachedUser.getGroups(CachedUser.java:130)                                                                                                                                    
    at org.keycloak.models.cache.infinispan.UserAdapter.getGroupsStream(UserAdapter.java:400)                                                                                                                                     
    at org.keycloak.models.cache.infinispan.UserAdapter.hasRole(UserAdapter.java:364)                                                                                                                                             
    at org.keycloak.authorization.common.UserModelIdentity.hasOneClientRole(UserModelIdentity.java:65)                                                                                                                            
    at org.keycloak.services.resources.admin.permissions.MgmtPermissions.hasOneAdminRole(MgmtPermissions.java:177)                                                                                                                
    at org.keycloak.services.resources.admin.permissions.MgmtPermissions.hasOneAdminRole(MgmtPermissions.java:164)                                                                                                                
    at org.keycloak.services.resources.admin.permissions.UserPermissions.canViewDefault(UserPermissions.java:597)                                                                                                                 
    at org.keycloak.services.resources.admin.permissions.GroupPermissions.canView(GroupPermissions.java:287)                                                                                                                      
    at org.keycloak.services.resources.admin.permissions.GroupPermissions.canList(GroupPermissions.java:150)                                                                                                                      
    at org.keycloak.services.resources.admin.permissions.GroupPermissions.requireList(GroupPermissions.java:155)                                                                                                                  
    at org.keycloak.services.resources.admin.GroupsResource.getGroups(GroupsResource.java:96)                                                                                                                                     
    at org.keycloak.services.resources.admin.GroupsResource$quarkusrestinvoker$getGroups_7b020fd0a44403122f36a3a47bc8b7638a5dc3e8.invoke(Unknown Source)                                                                          
    at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)                                                                                                                            
    at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)                                                                            
    at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)                                                                                                      
    at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:582)                                                                                                                                     
    at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)                                                                                                                                          
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)                                                                                                                                    
    at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
    at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)                                                                                                                                   
    at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)                                                                                                                                      
    at java.base/java.lang.Thread.run(Thread.java:1583)                                                                                                                                                                           
Caused by: java.sql.BatchUpdateException: Batch entry 0 insert into public.KEYCLOAK_GROUP (NAME,PARENT_GROUP,REALM_ID,ID) values (('Test3'),(' '),('5b350cda-0cd5-4745-af22-7ccc3bcfe746'),('51284f2a-bc6c-4993-8696-c47946805f06'
  Detail: Key (realm_id, parent_group, name)=(5b350cda-0cd5-4745-af22-7ccc3bcfe746,  , Test3) already exists.  Call getNextException to see other errors in the batch.                                                            
    at org.postgresql.jdbc.BatchResultHandler.handleError(BatchResultHandler.java:165)                                                                                                                                            
    at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2413)                                                                                                                                       
    at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:579)                                                                                                                                               
    at org.postgresql.jdbc.PgStatement.internalExecuteBatch(PgStatement.java:912)                                                                                                                                                 
    at org.postgresql.jdbc.PgStatement.executeBatch(PgStatement.java:936)                                                                                                                                                         
    at org.postgresql.jdbc.PgPreparedStatement.executeBatch(PgPreparedStatement.java:1733)                                                                                                                                        
    at io.agroal.pool.wrapper.StatementWrapper.executeBatch(StatementWrapper.java:340)                                                                                                                                            
    at org.hibernate.engine.jdbc.batch.internal.BatchImpl.lambda$performExecution$2(BatchImpl.java:279)                                                                                                                           
    at org.hibernate.engine.jdbc.mutation.internal.PreparedStatementGroupSingleTable.forEachStatement(PreparedStatementGroupSingleTable.java:59)                                                                                  
    at org.hibernate.engine.jdbc.batch.internal.BatchImpl.performExecution(BatchImpl.java:264)                                                                                                                                    
    at org.hibernate.engine.jdbc.batch.internal.BatchImpl.execute(BatchImpl.java:242)                                                                                                                                             
    at org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl.executeBatch(JdbcCoordinatorImpl.java:188)                                                                                                                          
    at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:662)                                                                                                                                                  
    at org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:499)                                                                                                                                                  
    at org.hibernate.event.internal.AbstractFlushingEventListener.performExecutions(AbstractFlushingEventListener.java:363)                                                                                                       
    at org.hibernate.event.internal.DefaultFlushEventListener.onFlush(DefaultFlushEventListener.java:41)                                                                                                                          
    at org.hibernate.event.service.internal.EventListenerGroupImpl.fireEventOnEachListener(EventListenerGroupImpl.java:127)                                                                                                       
    at org.hibernate.internal.SessionImpl.doFlush(SessionImpl.java:1407)                                                                                                                                                          
    at org.hibernate.internal.SessionImpl.flush(SessionImpl.java:1393)                                                                                                                                                            
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)                                                                                                                      
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)                                                                                                                                                                 
    at org.keycloak.connections.jpa.PersistenceExceptionConverter.invoke(PersistenceExceptionConverter.java:66)                                                                                                                   
    ... 45 more                                                                                                                                                                                                                   
Caused by: org.postgresql.util.PSQLException: ERROR: duplicate key value violates unique constraint "sibling_names"                                                                                                               
  Detail: Key (realm_id, parent_group, name)=(5b350cda-0cd5-4745-af22-7ccc3bcfe746,  , Test3) already exists.                                                                                                                     
    at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2725)                                                                                                                                 
    at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2412)                                                                                                                                       
    ... 65 more

How to Reproduce?

  1. Set up user federation with LDAP and add an ldap-group mapper for syncing LDAP groups to Keycloak
  2. Create a group in Keycloak and add a federated user to it
  3. Assign the federated user the proper roles to delete groups in Admin Console (I used the admin role of the master realm for testing)
  4. Log in to the Admin Console with that federated user
  5. Delete the group they are a member of

Anything else?

No response

pedroigor commented 4 months ago

Tried in both 25.0.1 the same steps and I wasn't able to reproduce the problem. The user is a member of both parent and a sub-group and deleting one by one, starting from the sub-group, works as expected.

Salvoxia commented 4 months ago

Thank you for investing your time trying to reproduce. I checked again with a clean installation and indeed could not reproduce it the first time around, but I think I found the missing piece: The way the federated user is assigned the admin role matters. If the role is assigned to the user directly, everything works fine. However, if the user gets the role via a group role assignment, the error is reliably reproducible.

So the new steps to reproduce are as follows:

  1. Set up user federation with LDAP and add an ldap-group mapper for syncing LDAP groups to Keycloak, use LDAP_ONLY mode
  2. Create a user TestUser that gets propagated to LDAP
  3. Create a group AdminGroup in Keycloak, add TestUser as a member and assign the admin role to the group
  4. Create a group TestGroup in Keycloak and add TestUser to that group as well
  5. Log in to the Admin Console with TestUser
  6. Delete the group TestGroup
sguilhen commented 4 months ago

I was able to reproduce the issue following the steps described in the last comment.

The issue seems to happen because the admin console sends two requests to the groups endpoint almost simultaneously after deleting the group:

DELETE | http://localhost:8080/admin/realms/master/groups/f96c7a63-51c3-4b3b-82b8-608eece1fa5e  (200 OK)
GET | http://localhost:8080/admin/realms/master/groups?first=0&max=21&exact=false&global=false     (409 error)
GET | http://localhost:8080/admin/realms/master/groups?max=11  (200 OK)

In the GroupsResource endpoint, we check if the user has permissions to list groups. This leads to the stack trace seen above where we inspect the groups for roles assigned, which triggers the GroupLdapStorageMapper to synch the groups. Ultimately, this leads to the creation of any groups that exist in LDAP but not in Keycloak (our Test Group in this example). One of the two GET requests successfully recreates the group, and the second fails as it tries to create the same group again. It looks like a race condition due to the simultaneous requests made to the endpoint, but curiously it is always the first request that fails for me.

The whole thing is related to the fact that groups are synched automatically to LDAP when created, but not when deleted. If the groups deletion was properly synched, the GET requests wouldn't try to load it again from LDAP because the deleted group would be gone by then.

@pedroigor I think we need to accept this one. One way to fix this would be to investigate why the admin console issues those two separate requests at the same time when loading groups. Another way to fix this is to synch group deletion (I think there's a PR opened for https://github.com/keycloak/keycloak/issues/29099. It has been requested quite a few times already.

pedroigor commented 3 months ago

@Salvoxia @sguilhen The second request is because of how the UI is rendered. One to render the tree and the other the group on the right side. I also noticed this when working with the scalability of groups.

Glad to know there is a contribution. Let's review the PR then.

I've talked with @edewit about it and we discussed changing the UI to only render the right side of the page (group details) after clicking on a group on the left side. But that won't solve the original problem.

pedroigor commented 3 months ago

@sguilhen But https://github.com/keycloak/keycloak/issues/29099 seems to be actually adding support to propagate group deletion when removing local groups, isn't it?

The problem here should still happen because we are not really fixing the race condition?

sguilhen commented 3 months ago

@pedroigor IMO if the group is properly deleted on LDAP, then it won't be re-synced to Keycloak in the next request

pedroigor commented 3 months ago

@Salvoxia @sguilhen Please, see https://github.com/keycloak/keycloak/pull/31090#issuecomment-2248932845. We can not solve this issue by automatically removing groups in LDAP.

As an alternative, we could avoid the error you reported by making sure synchronization will not happen twice so that we make sure there won't be errors when trying to duplicate groups in the database when running multiple synchronization in parallel. But still, the root problem is not solved.

The root problem here is that we don't have a clear policy on when synchronization should happen. The root cause of this issue is that we are synchronizing groups when evaluating the permissions to access the API. As you are using a user who is a member of an LDAP group, that will force synchronization, and the error will happen. If you try to delete a group where the user is not a member, even if there are other members there, then you will be able to successfully delete the group.

I'll move this issue to our backlog because we need to sort out not only this one but other issues related to LDAP and specifically with the group mapper.

keycloak-github-bot[bot] commented 3 months ago

Due to the amount of issues reported by the community we are not able to prioritise resolving this issue at the moment.

If you are affected by this issue, upvote it by adding a :thumbsup: to the description. We would also welcome a contribution to fix the issue.