search

keycloak / keycloak

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

Realm update fails when realm has many Identity Providers configured and saves rep. with Admin Events #14851

Closed jsorah closed 1 year ago

jsorah commented 1 year ago

Describe the bug

Realm update fails when a realm has many Identity Providers configured and also is saving Admin Events with the representation of the request.

Concretely, our Keycloak instance has approximately 34 IDPs configured - when I attempt to update another, unrelated configuration item (such as the Content-Security-Policy) - the request fails. When I disable saving the representation as part of the Admin Event, the request succeeds.

The error we see in our logs indicates it is unable to fit the representation (what appears to be the raw JSON sent by the front end to the backend) - inspecting the request payload to the server, I see it is 70988 characters when I copied from Chrome devtools.

Interestingly, across database vendors the size of the REPRESENTATION field can vary widely

It appears for some reason the front-end always includes the IDP metadata with its payload, even if you are changing unrelated configuration settings, which means even if you resolve the backend storage issue, the payload would always grow with each new IDP added. So I would think any change to ADMIN_EVENT_TABLE would just be deferring this other unbounded growth issue.

Long Stack Trace Warning

[2022-10-06 14:32:12,884+0000] ERROR [io.undertow.request] (default task-8510) UT005023: Exception handling request to /auth/admin/realms/REDACTED: org.keycloak.models.ModelException: org.hibernate.exception.SQLGrammarException: could not execute statement
    at org.keycloak.keycloak-model-jpa@18.0.0.redhat-00001//org.keycloak.connections.jpa.PersistenceExceptionConverter.convert(PersistenceExceptionConverter.java:84)
    at org.keycloak.keycloak-model-jpa@18.0.0.redhat-00001//org.keycloak.connections.jpa.JpaExceptionConverter.convert(JpaExceptionConverter.java:31)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.transaction.JtaTransactionWrapper.lambda$handleException$0(JtaTransactionWrapper.java:65)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195)
    at java.base/java.util.HashMap$ValueSpliterator.forEachRemaining(HashMap.java:1693)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
    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.forEach(ReferencePipeline.java:497)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.transaction.JtaTransactionWrapper.handleException(JtaTransactionWrapper.java:67)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.transaction.JtaTransactionWrapper.commit(JtaTransactionWrapper.java:92)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.services.DefaultKeycloakTransactionManager.commit(DefaultKeycloakTransactionManager.java:136)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.services.filters.AbstractRequestFilter.close(AbstractRequestFilter.java:64)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.services.filters.AbstractRequestFilter.filter(AbstractRequestFilter.java:49)
    at org.keycloak.keycloak-wildfly-extensions@18.0.0.redhat-00001//org.keycloak.provider.wildfly.WildFlyRequestFilter.doFilter(WildFlyRequestFilter.java:39)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ManagedFilter.doFilter(ManagedFilter.java:61)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:131)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.FilterHandler.handleRequest(FilterHandler.java:84)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletSecurityRoleHandler.handleRequest(ServletSecurityRoleHandler.java:62)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletChain$1.handleRequest(ServletChain.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletDispatchingHandler.handleRequest(ServletDispatchingHandler.java:36)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.SecurityContextAssociationHandler.handleRequest(SecurityContextAssociationHandler.java:78)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.RedirectDirHandler.handleRequest(RedirectDirHandler.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.SSLInformationAssociationHandler.handleRequest(SSLInformationAssociationHandler.java:117)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletAuthenticationCallHandler.handleRequest(ServletAuthenticationCallHandler.java:57)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AbstractConfidentialityHandler.handleRequest(AbstractConfidentialityHandler.java:46)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletConfidentialityConstraintHandler.handleRequest(ServletConfidentialityConstraintHandler.java:64)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AuthenticationMechanismsHandler.handleRequest(AuthenticationMechanismsHandler.java:60)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.CachedAuthenticatedSessionHandler.handleRequest(CachedAuthenticatedSessionHandler.java:77)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.NotificationReceiverHandler.handleRequest(NotificationReceiverHandler.java:50)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AbstractSecurityContextAssociationHandler.handleRequest(AbstractSecurityContextAssociationHandler.java:43)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.jacc.JACCContextIdHandler.handleRequest(JACCContextIdHandler.java:61)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.GlobalRequestControllerHandler.handleRequest(GlobalRequestControllerHandler.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.SendErrorPageHandler.handleRequest(SendErrorPageHandler.java:52)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.handleFirstRequest(ServletInitialHandler.java:275)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.access$100(ServletInitialHandler.java:79)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:134)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:131)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ServletRequestContextThreadSetupAction$1.call(ServletRequestContextThreadSetupAction.java:48)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ContextClassLoaderSetupAction$1.call(ContextClassLoaderSetupAction.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.SecurityContextThreadSetupAction.lambda$create$0(SecurityContextThreadSetupAction.java:105)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.dispatchRequest(ServletInitialHandler.java:255)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.access$000(ServletInitialHandler.java:79)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$1.handleRequest(ServletInitialHandler.java:100)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.Connectors.executeRootHandler(Connectors.java:387)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.HttpServerExchange$1.run(HttpServerExchange.java:852)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1990)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
    at org.jboss.xnio@3.8.6.Final-redhat-00001//org.xnio.XnioWorker$WorkerThreadFactory$1$1.run(XnioWorker.java:1280)
    at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: org.hibernate.exception.SQLGrammarException: could not execute statement
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.exception.internal.SQLExceptionTypeDelegate.convert(SQLExceptionTypeDelegate.java:63)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:42)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:113)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:99)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:178)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3193)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:3707)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.action.internal.EntityInsertAction.execute(EntityInsertAction.java:90)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:604)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.spi.ActionQueue.executeActions(ActionQueue.java:478)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.event.internal.AbstractFlushingEventListener.performExecutions(AbstractFlushingEventListener.java:356)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.event.internal.DefaultFlushEventListener.onFlush(DefaultFlushEventListener.java:39)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.internal.SessionImpl.doFlush(SessionImpl.java:1472)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.internal.SessionImpl.managedFlush(SessionImpl.java:512)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.internal.SessionImpl.flushBeforeTransactionCompletion(SessionImpl.java:3310)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.internal.SessionImpl.beforeTransactionCompletion(SessionImpl.java:2506)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.jdbc.internal.JdbcCoordinatorImpl.beforeTransactionCompletion(JdbcCoordinatorImpl.java:447)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.resource.transaction.backend.jta.internal.JtaTransactionCoordinatorImpl.beforeCompletion(JtaTransactionCoordinatorImpl.java:352)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.resource.transaction.backend.jta.internal.synchronization.SynchronizationCallbackCoordinatorNonTrackingImpl.beforeCompletion(SynchronizationCallbackCoordinatorNonTrackingImpl.java:47)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.resource.transaction.backend.jta.internal.synchronization.RegisteredSynchronization.beforeCompletion(RegisteredSynchronization.java:37)
    at org.wildfly.transaction.client@1.1.14.Final-redhat-00001//org.wildfly.transaction.client.AbstractTransaction.performConsumer(AbstractTransaction.java:236)
    at org.wildfly.transaction.client@1.1.14.Final-redhat-00001//org.wildfly.transaction.client.AbstractTransaction.performConsumer(AbstractTransaction.java:247)
    at org.wildfly.transaction.client@1.1.14.Final-redhat-00001//org.wildfly.transaction.client.AbstractTransaction$AssociatingSynchronization.beforeCompletion(AbstractTransaction.java:292)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.internal.jta.resources.arjunacore.SynchronizationImple.beforeCompletion(SynchronizationImple.java:76)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.arjuna.coordinator.TwoPhaseCoordinator.beforeCompletion(TwoPhaseCoordinator.java:360)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.arjuna.coordinator.TwoPhaseCoordinator.end(TwoPhaseCoordinator.java:91)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.arjuna.AtomicAction.commit(AtomicAction.java:162)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.internal.jta.transaction.arjunacore.TransactionImple.commitAndDisassociate(TransactionImple.java:1295)
    at org.jboss.jts@5.11.4.Final-redhat-00001//com.arjuna.ats.internal.jta.transaction.arjunacore.BaseTransaction.commit(BaseTransaction.java:128)
    at org.jboss.jts.integration@5.11.4.Final-redhat-00001//com.arjuna.ats.jbossatx.BaseTransactionManagerDelegate.commit(BaseTransactionManagerDelegate.java:94)
    at org.wildfly.transaction.client@1.1.14.Final-redhat-00001//org.wildfly.transaction.client.LocalTransaction.commitAndDissociate(LocalTransaction.java:78)
    at org.wildfly.transaction.client@1.1.14.Final-redhat-00001//org.wildfly.transaction.client.ContextTransactionManager.commit(ContextTransactionManager.java:71)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.transaction.JtaTransactionWrapper.commit(JtaTransactionWrapper.java:90)
    ... 50 more
Caused by: java.sql.SQLSyntaxErrorException: (conn=147696) Data too long for column 'REPRESENTATION' at row 1
    at org.mariadb//org.mariadb.jdbc.internal.util.exceptions.ExceptionFactory.createException(ExceptionFactory.java:62)
    at org.mariadb//org.mariadb.jdbc.internal.util.exceptions.ExceptionFactory.create(ExceptionFactory.java:155)
    at org.mariadb//org.mariadb.jdbc.MariaDbStatement.executeExceptionEpilogue(MariaDbStatement.java:274)
    at org.mariadb//org.mariadb.jdbc.ClientSidePreparedStatement.executeInternal(ClientSidePreparedStatement.java:229)
    at org.mariadb//org.mariadb.jdbc.ClientSidePreparedStatement.execute(ClientSidePreparedStatement.java:149)
    at org.mariadb//org.mariadb.jdbc.ClientSidePreparedStatement.executeUpdate(ClientSidePreparedStatement.java:181)
    at org.jboss.ironjacamar.jdbcadapters@1.5.3.Final-redhat-00001//org.jboss.jca.adapters.jdbc.WrappedPreparedStatement.executeUpdate(WrappedPreparedStatement.java:537)
    at org.hibernate@5.3.25.Final-redhat-00002//org.hibernate.engine.jdbc.internal.ResultSetReturnImpl.executeUpdate(ResultSetReturnImpl.java:175)
    ... 78 more
Caused by: org.mariadb.jdbc.internal.util.exceptions.MariaDbSqlException: Data too long for column 'REPRESENTATION' at row 1
    at org.mariadb//org.mariadb.jdbc.internal.util.exceptions.MariaDbSqlException.of(MariaDbSqlException.java:34)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.exceptionWithQuery(AbstractQueryProtocol.java:194)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.exceptionWithQuery(AbstractQueryProtocol.java:177)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.executeQuery(AbstractQueryProtocol.java:321)
    at jdk.internal.reflect.GeneratedMethodAccessor185.invoke(Unknown Source)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:566)
    at org.mariadb//org.mariadb.jdbc.internal.failover.AbstractMastersListener.invoke(AbstractMastersListener.java:408)
    at org.mariadb//org.mariadb.jdbc.internal.failover.FailoverProxy.executeInvoc2//org.keycloak.services.filters.AbstractRequestFilter.close(AbstractRequestFilter.java:64)
    at org.keycloak.keycloak-services@18.0.0.redhat-00002//org.keycloak.services.filters.AbstractRequestFilter.filter(AbstractRequestFilter.java:49)
    at org.keycloak.keycloak-wildfly-extensions@18.0.0.redhat-00001//org.keycloak.provider.wildfly.WildFlyRequestFilter.doFilter(WildFlyRequestFilter.java:39)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ManagedFilter.doFilter(ManagedFilter.java:61)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:131)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.FilterHandler.handleRequest(FilterHandler.java:84)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletSecurityRoleHandler.handleRequest(ServletSecurityRoleHandler.java:62)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletChain$1.handleRequest(ServletChain.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletDispatchingHandler.handleRequest(ServletDispatchingHandler.java:36)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.SecurityContextAssociationHandler.handleRequest(SecurityContextAssociationHandler.java:78)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.RedirectDirHandler.handleRequest(RedirectDirHandler.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.SSLInformationAssociationHandler.handleRequest(SSLInformationAssociationHandler.java:117)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletAuthenticationCallHandler.handleRequest(ServletAuthenticationCallHandler.java:57)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AbstractConfidentialityHandler.handleRequest(AbstractConfidentialityHandler.java:46)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.ServletConfidentialityConstraintHandler.handleRequest(ServletConfidentialityConstraintHandler.java:64)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AuthenticationMechanismsHandler.handleRequest(AuthenticationMechanismsHandler.java:60)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.security.CachedAuthenticatedSessionHandler.handleRequest(CachedAuthenticatedSessionHandler.java:77)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.NotificationReceiverHandler.handleRequest(NotificationReceiverHandler.java:50)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.security.handlers.AbstractSecurityContextAssociationHandler.handleRequest(AbstractSecurityContextAssociationHandler.java:43)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.jacc.JACCContextIdHandler.handleRequest(JACCContextIdHandler.java:61)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.GlobalRequestControllerHandler.handleRequest(GlobalRequestControllerHandler.java:68)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.SendErrorPageHandler.handleRequest(SendErrorPageHandler.java:52)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.handlers.PredicateHandler.handleRequest(PredicateHandler.java:43)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.handleFirstRequest(ServletInitialHandler.java:275)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.access$100(ServletInitialHandler.java:79)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:134)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$2.call(ServletInitialHandler.java:131)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ServletRequestContextThreadSetupAction$1.call(ServletRequestContextThreadSetupAction.java:48)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.core.ContextClassLoaderSetupAction$1.call(ContextClassLoaderSetupAction.java:43)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.security.SecurityContextThreadSetupAction.lambda$create$0(SecurityContextThreadSetupAction.java:105)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at org.wildfly.extension.undertow@7.4.4.GA-redhat-00011//org.wildfly.extension.undertow.deployment.UndertowDeploymentInfoService$UndertowThreadSetupAction.lambda$create$0(UndertowDeploymentInfoService.java:1551)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.dispatchRequest(ServletInitialHandler.java:255)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler.access$000(ServletInitialHandler.java:79)
    at io.undertow.servlet@2.2.16.Final-redhat-00001//io.undertow.servlet.handlers.ServletInitialHandler$1.handleRequest(ServletInitialHandler.java:100)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.Connectors.executeRootHandler(Connectors.java:387)
    at io.undertow.core@2.2.16.Final-redhat-00001//io.undertow.server.HttpServerExchange$1.run(HttpServerExchange.java:852)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1990)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
    at org.jboss.threads@2.4.0.Final-redhat-00001//org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1377)
    at org.jboss.xnio@3.8.6.Final-redhat-00001//org.xniation(FailoverProxy.java:301)
    at org.mariadb//org.mariadb.jdbc.internal.failover.FailoverProxy.invoke(FailoverProxy.java:294)
    at org.mariadb//com.sun.proxy.$Proxy32.executeQuery(Unknown Source)
    at org.mariadb//org.mariadb.jdbc.ClientSidePreparedStatement.executeInternal(ClientSidePreparedStatement.java:220)
    ... 82 more
Caused by: java.sql.SQLException: Data too long for column 'REPRESENTATION' at row 1
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.readErrorPacket(AbstractQueryProtocol.java:1694)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.readPacket(AbstractQueryProtocol.java:1556)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.getResult(AbstractQueryProtocol.java:1519)
    at org.mariadb//org.mariadb.jdbc.internal.protocol.AbstractQueryProtocol.executeQuery(AbstractQueryProtocol.java:318)
    ... 90 more

Version

18

Expected behavior

Realm is able to be saved with Admin Events having representation enabled.

Actual behavior

Realm is not able to be saved with Admin Events having representation enabled.

How to Reproduce?

Our Keycloak Setup:

Authenticate to the admin console once this set up is configured, and attempt to change a security configuration for the realm such as the Content-Security-Policy

Anything else?

No response

alechenninger commented 1 year ago

To generalize this a bit, this is a problem for multi-tenant realms–where the same clients are used across many tenants, and each tenant may configure its own IdP. We might expect up to several thousand IdPs for example.

To generalize even further, what else is included in a realm representation in the admin console or in admin events? If there are any other unbounded arrays like IdPs, it becomes a soft limit on scaling those objects within a realm. I see many other high cardinality lists (e.g. groups, users) in the realm representation, but of course as users don't cause this problem, it's not clear why some are populated (IdPs) and not others (users) in the representation.

mposolda commented 1 year ago

@jsorah @alechenninger I suppose that when you are updating realm, you are using admin console. Is it correct?

It seems that what is going is:

Options for fixing this

1) Handle this at storage level. This can work, but I am not sure about the effort needed for this and it looks to me more as a workaround TBH

2) Make sure that identity providers and IDentity provider mappers won't be sent in the PUT request to update realm. This can be achieved by: a) Not return IDP and IDP mappers in the very first GET request for retrieve the realm. This will be fine and it will be more proper solution as there are separate admin REST API endpoints for CRUD IDPs and IDP Mappers. However in theory it breaks backwards compatibility (in case that someone relied before on the fact that IDP and IDP mappers are returned from the GET request, he will need to change his admin REST API calls to use the endpoints for CRUD list of IDPs). b) Still return IDP from GET request, but stop sending them in the PUT request. This can be fixed at the admin console level to make sure that realm JSON is stripped from IDP and IDP mappers before the PUT request is sent to the server

My vote is (2.a) as this is the easiest option and IMO most proper. We may need to document this in the upgrading guide that realm GET endpoint won't return IDP and IDP mappers anymore. If we cannot break admin REST API (I am not 100% sure what the general contract is for admin REST API), then we can go 2.b.

BTV. If we have versioning support for admin REST API, then this will be easy as we can go 2.a and increase version without any side-effect.

Workaround option

I think the easiest workaround is to handle this at admin console level. You can create your own theme (and/or possibly directly update "themes" folder in the distribution, I am not 100% sure if this works, so you can try first...) and update some file (I think it is in realm.js or services.js ) and strip IDP and IDP mappers before PUT request is sent. Maybe you need to update more places as realm PUT request is probably sent from various pages in the admin console.

Another workaround is to use admin REST API directly and make sure that you don't sent IDP and IDP mappers in the realm PUT request

mposolda commented 1 year ago

To generalize this a bit, this is a problem for multi-tenant realms–where the same clients are used across many tenants, and each tenant may configure its own IdP. We might expect up to several thousand IdPs for example.

@alechenninger One question to this: Is this few thousand IdPs per realm or per whole deployment? If it is per whole deployment, then how many IdPs do you expect per realm?

I think Keycloak actually does not expect that realm will contain thousands (or even tens) of IdPs and it is possible there are more similar issues. For example there is no yet pagination for IdPs (at admin console, REST API and model layer). I suggest to try test with some bigger number of IdPs in your stage/test environment to avoid some unexpected surprises as more bugs can happen once you start increasing count of IdPs in production :-/

alechenninger commented 1 year ago

Thank you @mposolda . Very helpful!

I agree with your suggestion (2a), but 2b seems also like a reasonable compatible fix. Might need @stianst to weigh in on what the policy is around admin API compatibility from Keycloak to Keycloak release, but based on some internal discussion I think we want to avoid breaking changes. Let's continue that discussion on options internally for now.

@alechenninger One question to this: Is this few thousand IdPs per realm or per whole deployment? If it is per whole deployment, then how many IdPs do you expect per realm?

I think Keycloak actually does not expect that realm will contain thousands (or even tens) of IdPs and it is possible there are more similar issues.

It is per realm. We are just starting to look at the scaling issues here. Right now we administratively still control how many we have configured, so we can stop adding them if there are problems. We are investigating what issues we might see with self-service which is what we expect will take us into the 100s or 1000s. We will work on discovering what else may come up as we increase this number well before prod of course :).

hmlnarik commented 1 year ago

I believe this is a more general problem - events cannot store detail, including representation, longer than what the underlying engine supports. Even if it was fixed for one REST endpoint, same issue could occur for any other endpoint that updates an object with representation longer than that limit. The only way out IMO is to enable limit for the event detail, i.e. representation in this case.

There is already a similar functionality for login events implemented in #6576, and I'd argue the same limit (set via max-detail-length of eventStore SPI) should be applied to the representation, WDYT?

Technically speaking, it means applying trimToMaxLength method to this line: https://github.com/keycloak/keycloak/blob/f80a8fbed0940f28e8a23ebcdc5469c7299c0457/model/jpa/src/main/java/org/keycloak/events/jpa/JpaEventStoreProvider.java#L233

Or perhaps better, introducing a new parameter which would set the maximum field length

alechenninger commented 1 year ago

I agree but I think both things are true–the API is asymmetric: it "GETs" what it does not equivalently "PUT." This is surprising. Additionally, the "GET" returns a potentially high cardinality array of objects which you can otherwise query separately. So even ignoring event storage, that is a problem.

I agree with your point also, @hmlnarik , about the event storage generally.

jsorah commented 1 year ago

Would it make sense to break this issue up into these separate concerns, so the conversations don't get too confused, as they are both valid?

hmlnarik commented 1 year ago

Would it make sense to break this issue up into these separate concerns, so the conversations don't get too confused, as they are both valid?

  • Admin Event Representation should be truncated / limited in some fashion (maybe this is handled here)
  • Admin API changes - don't include high cardinality properties in GET response

Absolutely, these are two issues. The first one is a no-brainer for me; I'm very reluctant to implementing the second as proposed though since this is a breaking change. I'd rather define a new REST API (v2) and migration path off the current one.

mposolda commented 1 year ago

I agree that it will be ideal to address both issues (trimming the events as well as stop sending IDP in GET-realm request). I wasn't aware of trimToMaxLength (or maybe I just forgot :-) ), so I am sorry to not propose it earlier.

The trimToMaxLength solution is fine as it means avoiding the DB errors, on the other hand, it has some other limitations. For example if there are some important attributes in the realm JSON after the identity-providers attribute, then those attributes won't be shown in the saved admin event (due the fact that long identity-providers attribute will be trimmed in the middle and everything in the JSON after that won't be logged). Also the saved events in the DB as well as REST API requests/responses will be still unnecessary big.

One way to handle REST API in backwards compatible way is to introduce some flag like include-idps in the GET-realm request. This can be true by default (due the backwards compatibility), but can be switched to false in the admin console (as admin console doesn't need list of IDPs in the GET-realm response). The flag in the REST API is not ideal, but I count that at some point, we will have new admin REST API (based on OpenAPI etc), which will be cool from the beginning.

At the same time, there will be likely much more things to address if we want to support thousands of IDP per realm? Looks like IDP should be handled same way like the other objects, which are supported for long count (like clients, groups, roles etc). So looks like we may need pagination, better searching support, not sure if also something like IdentityProviderStorageSpi ?

So are we in agreement, that as "milestone 1", we will just trim the admin events, which will solve this particular issue? Then as "milestone 2+", we can handle the other things related to IDP scaling (which can be much more work...).

stianst commented 1 year ago

From what I can infer there are actually two issues here:

1) When updating a realm the realm representation in the event is to large to store in the database 2) When fetching the realm the result may be to large if there are thousands of IdPs configured, which would case both the admin console and endpoints to be slow

One open question: how are realms updated when seeing this issue? My guess is that it is only through the admin console?

In terms of what team on the Keycloak side should handle this it seems it should be the core features team, as from what we've discussed so far no one has suggested increasing the support for the database to store larger representations in the events.

Interestingly enough we have a relevant PR from the community that introduces pagination for IdPs in the old admin console: https://github.com/keycloak/keycloak/pull/7960

We could pick up this PR and likely also include an option in the get realm endpoint (&excludeIdps=true or something like that) to make this a non-breaking change. The admin console would always use this, and would need to use a paginated endpoint to fetch the list of IdPs.

Another tweak we could make is to clear out the IdPs list before storing the admin event. It doesn't make much sense to include the list of IdPs in the admin event as this list is ignored while updating the realm anyways.

Long term for sure we need a new version of admin endpoints that handle these (and many other cases) more consistently. However, that is not something that is going to happen anytime soon, so we should patch up the current approach as cleanly as we can.

hmlnarik commented 1 year ago

So are we in agreement, that as "milestone 1", we will just trim the admin events, which will solve this particular issue? Then as "milestone 2+", we can handle the other things related to IDP scaling (which can be much more work...).

I'd agree, but I would go as far as considering backwards-compatible REST API change you suggested in milestone 1.

One way to handle REST API in backwards compatible way is to introduce some flag like include-idps in the GET-realm request. This can be true by default (due the backwards compatibility), but can be switched to false in the admin console (as admin console doesn't need list of IDPs in the GET-realm response). The flag in the REST API is not ideal, but I count that at some point, we will have new admin REST API (based on OpenAPI etc), which will be cool from the beginning.

I agree that this could be a short-term workaround and thus think this could be a separate issue for the first milestone. I rely on your statement that admin console doesn't need list of IDPs in the GET-realm response, I have not checked that. @ssilvert could perhaps confirm that is the case.

So in total, I can see 3 issues:

At the same time, there will be likely much more things to address if we want to support thousands of IDP per realm? Looks like IDP should be handled same way like the other objects, which are supported for long count (like clients, groups, roles etc). So looks like we may need pagination, better searching support, not sure if also something like IdentityProviderStorageSpi ?

I would think this is not exactly necessary, and API-wise the potentially needed changes can still fit the RealmModel. Let's discuss this in a separate issue once we understand the use cases better.

stianst commented 1 year ago

I wouldn't truncate admin event rep, but rather do the simple thing here which is to remove the list of IdPs, as these are not considered when updating the realm.

For the admin console for this to scale we would need the ability to paginate these, and have just checked that the new admin console is not fetching any separate list of IdPs, so I presume it is currently just using the list from the realm rep.

hmlnarik commented 1 year ago

I wouldn't truncate admin event rep, but rather do the simple thing here which is to remove the list of IdPs, as these are not considered when updating the realm.

Not truncating admin event rep would cause the whole transaction to roll back, so the realm would not be updatable, just as stated in the bug report. Is this desired? Trimming is not done automatically but has to be set by the administrator to adhere to the underlying storage limitation.

stianst commented 1 year ago

I wouldn't truncate admin event rep, but rather do the simple thing here which is to remove the list of IdPs, as these are not considered when updating the realm.

Not truncating admin event rep would cause the whole transaction to roll back, so the realm would not be updatable, just as stated in the bug report. Is this desired? Trimming is not done automatically but has to be set by the administrator to adhere to the underlying storage limitation.

As mentioned we don't need to include the list of IdPs in the admin realm update event as this field is ignored when updating a realm. Once that is been removed the size of the event shouldn't be an issue, and there's no need for a separate truncating option.

hmlnarik commented 1 year ago

Do we know that there is no other place suffering from this issue? https://github.com/keycloak/keycloak/issues/14851#issuecomment-1278023506

stianst commented 1 year ago

Looks like the admin console fetches the realm with briefRepresentation=true most of the time, except when doing something like editing realm settings, and listing IdPs.

Most other things are using the component model, which is not included in either of the realm representations, so I don't think this is an issue beyond IdPs.

hmlnarik commented 1 year ago

It is possible to have the same issue with too long realm attributes, too many components etc. Is there any harm to have trimming there? It does not prevent other optimizations to happen

stianst commented 1 year ago

What I'm suggesting is a simple fix for this particular case, that doesn't require any configuration on the realm admin side, and doesn't have the side-effect of potentially missing important information from the event.

For sure there could be other cases that can cause this (like too long realm attributes). However, I think what you are suggesting is better served as a follow-up in a separate issue. I do have some concerns with the "randomness" of it though, as it would basically just be luck that decides if the relevant information is included in the event or not.

mposolda commented 1 year ago

My vote is to include both - make sure that GET-realm doesn't return IdPs and Idp mappers in the admin console (controlled by boolean flag at the admin REST API like exclude-identity-providers) and also add the trimming events.

This can be handled in separate PRs or the same PR whatever you prefer.

The trimming events seems more like a workaround and can trim important data from the logged event. On the other hand, it is already done for "regular events" and it is better option than requirement of DB rollback. At least, if there is similar issue in the future, administrator can enable trimming without being completely stuck (unless he entirely disable admin events, which is worse option than trimming IMO).

stianst commented 1 year ago

Let's split this into 3 issues:

  1. Keep this issue to solve this specific problem - filter out IdPs from stored events, and also from admin console when it doesn't need them with an extra query param
  2. Pagination support for IdPs in the new admin console
  3. Trimming for admin events
hmlnarik commented 1 year ago

What I'm suggesting is a simple fix for this particular case, that doesn't require any configuration on the realm admin side, and doesn't have the side-effect of potentially missing important information from the event.

For sure there could be other cases that can cause this (like too long realm attributes). However, I think what you are suggesting is better served as a follow-up in a separate issue. I do have some concerns with the "randomness" of it though, as it would basically just be luck that decides if the relevant information is included in the event or not.

Could you elaborate why there would be any randomness? Unless the administrator sets the config option to trim, the behaviour remains as of now. This is the same as the behaviour of detail trimming which is already present in Keycloak.

My vote is to include both - make sure that GET-realm doesn't return IdPs and Idp mappers in the admin console (controlled by boolean flag at the admin REST API like exclude-identity-providers) and also add the trimming events.

This can be handled in separate PRs or the same PR whatever you prefer.

Two issues and separate PRs please. Storage team does not own the REST part.

Let's split this into 3 issues:

  1. Keep this issue to solve this specific problem - filter out IdPs from stored events, and also from admin console when it doesn't need them with an extra query param
  2. Pagination support for IdPs in the new admin console
  3. Trimming for admin events

That one is already filed (#14888), and has a PR.

mposolda commented 1 year ago

Keep this issue to solve this specific problem - filter out IdPs from stored events, and also from admin console when it doesn't need them with an extra query param

Ok, so we can fix this issue (1) in this GH issue. Additional question: I suppose we need to handle both new and old admin console (old one due the fact that we need to backport to z-stream). Is it correct? So we will need PR to the new admin console as well. Old admin console can be addressed directly in this PR.

stianst commented 1 year ago

Thinking about this more i don't think the trimming fields is a good approach at all.

It will potentially hide the relevant fields that have been updated, making it useless from an audit perspective, and one may as well recommend disabling reps in the event. In addition when trimming it as a string in results in invalid JSON.

mposolda commented 1 year ago

@stianst I agree with your points for trimming. On the other hand, when there is similar issue, admin will have 3 options: 1) Being stucked and not being able to update realm due the DB transaction is rolled-back just because of admin event 2) Disable admin events store or at least disable "Include representation" flag 3) Enable trimming

Looks that (3) is smallest evil from those options :-) At least to me. With (2), the guy don't have any representations at all when with (3), he has at least reps from the admin events, which did not needed to be trimmed. But I am not strong on that as all options are quite bad...

stianst commented 1 year ago

I think I have a better option for not including the idps in the event. We should create a realm representation version without the idp listed, and ignore the idp lists in that version. That would also make the api docs more accurate as it won't mention the ignores fields.

stianst commented 1 year ago

I disagree that trimming is a good option. As mentioned before it results in invalid JSON, and removes potentially vital information needed from an audit perspective. I think it's a hacky and poor solution, that smells of a bad work around nothing else. Additionally it requires an admin to tune the length depending on what the dB can store, as well as finding the option in the first place, which is poor UX.

Honestly don't know why we're even discussing this option. We have a proper solution for the reported issue here, and can rather come up with a better way to solve it properly for other cases later. For example why can't we increase the size/type of the column in the dB so it can actually store it properly without trimming it? Or perhaps we could store a diff, which shows what was actually changed rather than the whole things.

stianst commented 1 year ago

With regards to pagination that is less urgent in the particular case that is reported here as there will be some time until that becomes a real issue, so we won't need to backport that to the old admin console or rh-sso. Regardless we should pick up the pr from the community and get that merged upstream.

stianst commented 1 year ago

With regard to the trimming of the json within the event. I'm okay with including that, but I wouldn't consider it the solution to this issue, and also I wouldn't consider it a solved problem with it.

Just think about the user experience around the trimming:

  1. User enables admin events with representations
  2. Everything is working fine
  3. Suddenly the user can't update the realm anymore
  4. The user scratches his head for a while thinking about what it is he's trying to do to the realm that is not valid. Eventually (maybe after a day) the user figures out it's the event not saving, not the realm update itself
  5. The user thinks hard about what to do now (and I'm pretty sure reading the docs to find this config option is going to be the first thing)
  6. The user may open a bug report against Keycloak, and wait for a while for an answer there
  7. The user may try to increase the column size in the DB
  8. The user may actually read the docs and find the config option, but then is unsure what to set it to. The user has to then figure out the size of the column, and what the max size of the fields should be

Eventually the user goes with the trimming option, and through some trial/error figures out what to set it to. However, then a few days later someone has changed the realm somehow, and they need to figure out who/what. First problem now is they can't parse the JSON as it's no longer valid, so they fix that, but then they figure out that the realm presentation in the updated event is equal to the previous update, and not sure what happened, but turns out the updated fields where something towards the end of the JSON representation, and what was updated is now lost.

All in all not a great user experience, and a lot of time wasted for anyone running into this problem.

mposolda commented 1 year ago

I think I have a better option for not including the idps in the event. We should create a realm representation version without the idp listed, and ignore the idp lists in that version. That would also make the api docs more accurate as it won't mention the ignores fields.

@stianst Do you mean to use this instead of introducing the boolean flag exclude-idp to the admin REST API? Yes, I agree this will work for fixing this issue and will be easy solution as it doesn't require any updates to the admin console. On the other hand, there would be still all IDPs present inside GET-realm JSON response and also in the update-realm JSON request right? So I wonder if it would be sufficient solution in general for being able to scale to many IDPs?

mposolda commented 1 year ago

@stianst I agree with your points regarding trimming. I also don't consider this as a solution for this particular issue. I consider it more as backdoor for some similar issues in the future without people being stucked, but agree the usability around this is not great...

From the other options you mentioned, I like the "diff" approach. I believe this will be best regarding UX . However it may be also the most work to make sure that "diff" is accurate for all the various endpoints, which we have as there will be some logic needed to figure what fields were changed. Also some guys may already count with full JSON (in their tools for processing events etc). So maybe full JSON may be still supported, but by default, we can use the "diff" option though as this can be the preferred way for new deployments?

Regarding increasing the size of the DB column: Here is the question what should be the ideal length and also the limit can be still reached at some point? For example if we increase the length of the DB columnt, we will be able to handle realm update even if it contains 34 IDPs. However we may still reach the limit with for example 60 Idps though. In general, increasing the limit might be just "postpone" the problem.

hmlnarik commented 1 year ago

Regarding increasing the size of the DB column: Here is the question what should be the ideal length and also the limit can be still reached at some point? For example if we increase the length of the DB columnt, we will be able to handle realm update even if it contains 34 IDPs. However we may still reach the limit with for example 60 Idps though. In general, increasing the limit might be just "postpone" the problem.

I fully agree with that.

The problem is there since Keycloak 1.3.0 (#1168). Including full representation in admin event is just a flaw unless there is the space enough for the representation to fit.

Changing the column to fit more data may be an option as long as potential downtime induced by translating the MySQL TEXT column into LONGTEXT is not a concern.

If that is intended, we can do that. @stianst, please provide your opinion.

stianst commented 1 year ago

I think I have a better option for not including the idps in the event. We should create a realm representation version without the idp listed, and ignore the idp lists in that version. That would also make the api docs more accurate as it won't mention the ignores fields.

@stianst Do you mean to use this instead of introducing the boolean flag exclude-idp to the admin REST API? Yes, I agree this will work for fixing this issue and will be easy solution as it doesn't require any updates to the admin console. On the other hand, there would be still all IDPs present inside GET-realm JSON response and also in the update-realm JSON request right? So I wonder if it would be sufficient solution in general for being able to scale to many IDPs?

My thinking here is that we solve this in two phases:

  1. Remove the IdP list from the representation in the persisted event in the cleanest way we can - could be just removing it from the rep before saving it, or with the alternative representation file that doesn't include it. I think the latter is cleanest as it also fixes the API including bits and pieces that are not used. For that we can do a follow-up on other endpoints later where we have similar issues (clients, users, etc.)
  2. Fix the admin console scale issue with pagination, where we'd need to either use briefRepresentation always in the admin console, or include another query param that excludes the IdP list from the rep

With regard to RH-SSO I would only back-port 1 for now. Including pagination in the admin console is a bigger change that IMO is not safe to back-port and shouldn't be added to the old admin console either.

For the other cases where the admin event could overflow I'd do that as a follow-up and not do it in this round of changes.

stianst commented 1 year ago

For the diff, trimming, increasing column size, etc. as mentioned in the previous comment I would do those as a follow-up as needed. If we remove the list of IdPs from the persisted representation those enhancements wouldn't be needed for this particular case.

hmlnarik commented 1 year ago

For the diff, trimming, increasing column size, etc. as mentioned in the previous comment I would do those as a follow-up as needed. If we remove the list of IdPs from the persisted representation those enhancements wouldn't be needed for this particular case.

This does not solve the issue for any other long fields, e.g. components or attributes which could overflow the limit, and does not solve it for any other type of objects than realms. This might cause the same issue e.g. in the case of clients where the size of attributes can overflow even more.

stianst commented 1 year ago

For the diff, trimming, increasing column size, etc. as mentioned in the previous comment I would do those as a follow-up as needed. If we remove the list of IdPs from the persisted representation those enhancements wouldn't be needed for this particular case.

This does not solve the issue for any other long fields, e.g. components or attributes which could overflow the limit, and does not solve it for any other type of objects than realms. This might cause the same issue e.g. in the case of clients where the size of attributes can overflow even more.

What does not solve it for any other long fields? If it's removing list of IdPs from the rep, it doesn't have to solve it for other fields. If it's for the follow-up, I suggest we discuss that as a follow-up and get this issue out of the way first.

hmlnarik commented 1 year ago

This does not solve the issue for any other long fields, e.g. components or attributes which could overflow the limit, and does not solve it for any other type of objects than realms. This might cause the same issue e.g. in the case of clients where the size of attributes can overflow even more.

What does not solve it for any other long fields? If it's removing list of IdPs from the rep, it doesn't have to solve it for other fields. If it's for the follow-up, I suggest we discuss that as a follow-up and get this issue out of the way first.

There are more callers of AdminEventBuilder.representation than realm update. Each of these calls can get into the very same exception if the respective representation is long enough - clients (namely attributes), roles (namely composites) etc.

If there is storing representation enabled, then the any of those operations would be effectively blocked until it is fixed by a fix of yet another keycloak bug.

I struggle to see why there is such an objection against trimming, especially given that we already have the trimming of details available to the admin. Do we prefer having the admins being effectively blocked on MySQL / MariaDB until next Keycloak version is released?

hmlnarik commented 1 year ago

Based on today's meeting between myself, @mposolda and @stianst, the trimming has been agreed on to be included, to be used by admin for emergencies. This does not mean it should be used by default but as a stopgap solution until the cause is properly resolved

hmlnarik commented 1 year ago

Removing storage area label (trimming implemented as #14891) from this issue since it now only relates admin API and UI.

ssilvert commented 1 year ago

@mposolda @hmlnarik @stianst Who is implementing the API part?

The UI team just needs to be notified when the API is ready. If you transfer the issue to the UI repo and add the triage label back then I will see it. Or just ping me directly.

alice-wondered commented 1 year ago

We currently have an in-progress API implementation here https://github.com/agandhew/keycloak/tree/fix-realm-api

It is not completely ready for review or merge but it is being worked on

@agandhew tagging you for visibility

pedroigor commented 1 year ago

@mposolda Is this a blocker for 22?

mposolda commented 1 year ago

@pedroigor It's from sso.rh team and hence I consider it as 22 for now.

alice-wondered commented 1 year ago

I've created a PR to add the brief representation to the GET path for the realm resource, however I want to make sure I'm addressing all concerns

Reading through this entire thread it seems that there's also talk about a couple of other solutions:

It seems that the second of these bullets has been dropped in favor of the brief representation param, but I'm wondering about the first.

Reading through the code, JpaEventStoreProvider does indeed implement a max detail length field that can be configured, so I assume for at least that solution that this is resolved. However, MapEventStoreProvider seems to not include any of these same features. Does this need to be addressed?

pedroigor commented 1 year ago

@mposolda @Redhat-Alice Isn't this issue now solved by https://github.com/keycloak/keycloak/pull/14891?

What I understand from the discussions is that trimming the details should be the solution.

@Redhat-Alice The PR you sent does not seem to help here because the admin console also needs to use the briefRepresentation parameter. It is also not helping with other 1:N fields such as attributes, client policies, and so forth.

FTR, I would also say this issue is related to supporting PATCH operations in our REST APIs.

The administration console relies on the full realm representation to manage settings from different tabs. Although it makes things easier, it has some drawbacks. At the same time, I'm not sure if we should be so strict and provide endpoints for the different aspects you can manage from a realm.

The last points are a different discussion. Just wanted to expose them here as I think it is related to the core issue we are trying to solve.

alice-wondered commented 1 year ago

This issue thread was incredibly long with a lot of ideas on how to solve the issue that went in different directions. I made my PR based on these points above

https://github.com/keycloak/keycloak/issues/14851#issuecomment-1282402901 https://github.com/keycloak/keycloak/issues/14851#issuecomment-1278843749

With the issue splitting in mind, my focus was on providing a non-breaking way of electing to ignore the identity providers when calling this endpoint which I believe also allows for the admin-console to call this endpoint and get the brief representation and thus never log it in the admin event.

If this is a flawed approach to that then I can adjust

pedroigor commented 1 year ago

@Redhat-Alice Not saying it is flawed but to properly leverage your change we also need updates to the admin console. Otherwise, it is still fetching/sending the full representation.

Do you think that the already existing support for trimming the representation when persisting the event is enough for sso.redhat.com? That is basically what I'm trying to figure out.

alice-wondered commented 1 year ago

I believe that this does technically solve the initial issue, though I do agree with others that throwing away data when given another option is not ideal.

I think #14891 will be fine in the short term but in the long term subverting this issue without potentially sacrificing important data would be ideal!

alice-wondered commented 1 year ago

@jsorah do you want to weigh in since you were the original issue reporter?