p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
Other
367 stars 65 forks source link

Members of an organization can't be displayed if a user part of that organization has been deleted while still posessing organization roles #65

Closed cato447 closed 1 year ago

cato447 commented 1 year ago

I encountered a bug that prevented me from seeing which members are part of an organization in the admin-ui. It displayed a JSON Parsing issue. I sadly dont have the error message from the admin-ui at hand.

After some investigation I discovered that one of the user_ids in the organization_member table was the ID of a user i deleted. The user had some roles in the organization at the point of deletion.

Deleting the row containing the deleted user_id from the organization_member table resolved the bug.

I deleted the user through the admin ui for users provided by Keycloak.

I tried to reproduce this. It handled it correctly this time (deleting the entries refrencing this user_id from all relevant tables).

2023-04-20 18:03:45,980 ERROR [org.keycloak.services.error.KeycloakErrorHandler] (executor-thread-3) Uncaught server error: com.fasterxml.jackson.databind.JsonMappingException: Cannot invoke "org.keycloak.models.UserModel.getId()" because "user" is null
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._wrapAsIOE(DefaultSerializerProvider.java:509)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:482)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:400)
        at com.fasterxml.jackson.databind.ObjectWriter$Prefetch.serialize(ObjectWriter.java:1514)
        at com.fasterxml.jackson.databind.ObjectWriter.writeValue(ObjectWriter.java:1007)
        at org.jboss.resteasy.plugins.providers.jackson.ResteasyJackson2Provider.writeTo(ResteasyJackson2Provider.java:345)
        at org.jboss.resteasy.core.interception.jaxrs.ServerWriterInterceptorContext.lambda$writeTo$1(ServerWriterInterceptorContext.java:79)
        at io.quarkus.resteasy.runtime.standalone.VertxHttpRequest$VertxExecutionContext.executeBlockingIo(VertxHttpRequest.java:251)
        at org.jboss.resteasy.core.interception.jaxrs.ServerWriterInterceptorContext.writeTo(ServerWriterInterceptorContext.java:79)
        at org.jboss.resteasy.core.interception.jaxrs.AbstractWriterInterceptorContext.syncProceed(AbstractWriterInterceptorContext.java:245)
        at org.jboss.resteasy.core.interception.jaxrs.AbstractWriterInterceptorContext.proceed(AbstractWriterInterceptorContext.java:224)
        at org.keycloak.quarkus.runtime.integration.jaxrs.TransactionalResponseInterceptor.aroundWriteTo(TransactionalResponseInterceptor.java:41)
        at org.jboss.resteasy.core.interception.jaxrs.AbstractWriterInterceptorContext.syncProceed(AbstractWriterInterceptorContext.java:254)
        at org.jboss.resteasy.core.interception.jaxrs.AbstractWriterInterceptorContext.getStarted(AbstractWriterInterceptorContext.java:170)
        at org.jboss.resteasy.core.interception.jaxrs.ServerWriterInterceptorContext.lambda$getStarted$0(ServerWriterInterceptorContext.java:73)
        at org.jboss.resteasy.core.interception.jaxrs.ServerWriterInterceptorContext.aroundWriteTo(ServerWriterInterceptorContext.java:93)
        at org.jboss.resteasy.core.interception.jaxrs.ServerWriterInterceptorContext.getStarted(ServerWriterInterceptorContext.java:73)
        at org.jboss.resteasy.core.ServerResponseWriter.lambda$writeNomapResponse$3(ServerResponseWriter.java:163)
        at org.jboss.resteasy.core.interception.jaxrs.ContainerResponseContextImpl.filter(ContainerResponseContextImpl.java:410)
        at org.jboss.resteasy.core.ServerResponseWriter.executeFilters(ServerResponseWriter.java:252)
        at org.jboss.resteasy.core.ServerResponseWriter.writeNomapResponse(ServerResponseWriter.java:101)
        at org.jboss.resteasy.core.ServerResponseWriter.writeNomapResponse(ServerResponseWriter.java:74)
        at org.jboss.resteasy.core.SynchronousDispatcher.writeResponse(SynchronousDispatcher.java:594)
        at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:524)
        at org.jboss.resteasy.core.SynchronousDispatcher.lambda$invoke$4(SynchronousDispatcher.java:261)
        at org.jboss.resteasy.core.SynchronousDispatcher.lambda$preprocess$0(SynchronousDispatcher.java:161)
        at org.jboss.resteasy.core.interception.jaxrs.PreMatchContainerRequestContext.filter(PreMatchContainerRequestContext.java:364)
        at org.jboss.resteasy.core.SynchronousDispatcher.preprocess(SynchronousDispatcher.java:164)
        at org.jboss.resteasy.core.SynchronousDispatcher.invoke(SynchronousDispatcher.java:247)
        at io.quarkus.resteasy.runtime.standalone.RequestDispatcher.service(RequestDispatcher.java:73)
        at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.dispatch(VertxRequestHandler.java:151)
        at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.handle(VertxRequestHandler.java:82)
        at io.quarkus.resteasy.runtime.standalone.VertxRequestHandler.handle(VertxRequestHandler.java:42)
        at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284)
        at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173)
        at io.vertx.ext.web.impl.RoutingContextWrapper.next(RoutingContextWrapper.java:200)
        at io.quarkus.vertx.http.runtime.StaticResourcesRecorder$2.handle(StaticResourcesRecorder.java:84)
        at io.quarkus.vertx.http.runtime.StaticResourcesRecorder$2.handle(StaticResourcesRecorder.java:71)
        at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284)
        at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173)
        at io.vertx.ext.web.impl.RoutingContextWrapper.next(RoutingContextWrapper.java:200)
        at io.quarkus.vertx.http.runtime.VertxHttpRecorder$6.handle(VertxHttpRecorder.java:430)
        at io.quarkus.vertx.http.runtime.VertxHttpRecorder$6.handle(VertxHttpRecorder.java:408)
        at io.vertx.ext.web.impl.RouteState.handleContext(RouteState.java:1284)
        at io.vertx.ext.web.impl.RoutingContextImplBase.iterateNext(RoutingContextImplBase.java:173)
        at io.vertx.ext.web.impl.RoutingContextWrapper.next(RoutingContextWrapper.java:200)
        at org.keycloak.quarkus.runtime.integration.web.QuarkusRequestFilter.lambda$createBlockingHandler$0(QuarkusRequestFilter.java:82)
        at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:576)
        at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2449)
        at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1478)
        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:833)
Caused by: java.lang.NullPointerException: Cannot invoke "org.keycloak.models.UserModel.getId()" because "user" is null
        at org.keycloak.models.utils.ModelToRepresentation.toRepresentation(ModelToRepresentation.java:253)
        at io.phasetwo.service.resource.MembersResource.lambda$getMembers$0(MembersResource.java:41)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
        at java.base/java.util.stream.SliceOps$1$1.accept(SliceOps.java:200)
        at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
        at java.base/java.util.Spliterators$IteratorSpliterator.tryAdvance(Spliterators.java:1856)
        at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
        at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
        at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
        at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.forEachOrdered(ReferencePipeline.java:601)
        at com.fasterxml.jackson.datatype.jdk8.StreamSerializer.serialize(StreamSerializer.java:71)
        at com.fasterxml.jackson.datatype.jdk8.StreamSerializer.serialize(StreamSerializer.java:15)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:480)
        ... 52 more
xgp commented 1 year ago

@cato447 Thanks for the detailed bug report. I think we may have a bug in how we handle UserModel.UserRemovedEvent here: https://github.com/p2-inc/keycloak-orgs/blob/main/src/main/java/io/phasetwo/service/resource/OrganizationResourceProviderFactory.java#L157

The logic looks right, but we need to add a test to verify it's working as expected.

xgp commented 1 year ago

@cato447 I wrote a test for the issue you described, and it wouldn't reproduce the issue. Can you take a look and make sure it is doing the same thing? https://github.com/p2-inc/keycloak-orgs/blob/issue-65/src/test/java/io/phasetwo/service/resource/OrganizationResourceTest.java#L263

Are you somehow removing the user in a way that doesn't trigger a UserModel.UserRemovedEvent provider event?

cato447 commented 1 year ago

Seems good just two things:

Line 273 checks against a member count of 11 which is probably a typo https://github.com/p2-inc/keycloak-orgs/blob/issue-65/src/test/java/io/phasetwo/service/resource/OrganizationResourceTest.java#L273

I would not even revoke the role of the user explicitly. Deleting the user should automatically trigger the deletion of the role mapping in the database either programmatically on event (UserModel.UserRemovedEvent) or even better in my opinion through the schema defenition of user_organization_role_mapping and all other tables dependent on the user_id. This would even cover the deletion of a user without triggering the UserModel.UserRemovedEvent.

I am using the intended way to remove userers through the admin-ui of Keycloak. I don't know if this triggers the Event UserModel.UserRemovedEvent

xgp commented 1 year ago

It's a 1l ("one lima") because that method requires a long.

I am using the intended way to remove userers through the admin-ui of Keycloak

Thanks. I'll do a manual test this way and see if I can reproduce.

I agree that both the programmatic way and the database cascade are a good way to enforce the relationship. I just need to figure out how/where it is failing.

cato447 commented 1 year ago

I am playing around with importing and exporting realms maybe this could be an issue?

xgp commented 1 year ago

Shouldn't have an impact. However, any reproducible test would be helpful. It's late here, and I'll investigate more tomorrow.

xgp commented 1 year ago

Closing due to lack of reproducible test. Reopen if there is more information.