From 557d7e87b2936dc5b1102ffe5376b283af6bab5f Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Tue, 3 Sep 2024 09:35:09 -0300 Subject: [PATCH] Avoid iterating through all mappers when running the config event listeners Closes #32233 Signed-off-by: Stefan Guilhen --- ...nispanIdentityProviderStorageProvider.java | 4 +- .../JpaIdentityProviderStorageProvider.java | 48 ++++++++++++++-- .../AbstractConfigPropertySynchronizer.java | 38 ++----------- .../mappersync/ConfigSyncEventListener.java | 14 +---- .../mappersync/ConfigSynchronizer.java | 8 +-- ...GroupConfigPropertyByPathSynchronizer.java | 55 ++++++++----------- ...eConfigPropertyByClientIdSynchronizer.java | 47 +++++----------- ...eConfigPropertyByRoleNameSynchronizer.java | 38 +++++-------- .../IdentityProviderStorageProvider.java | 29 ++++++++-- .../tests/base/testsuites/database-suite | 2 + 10 files changed, 131 insertions(+), 152 deletions(-) diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java index 05491fb83a..4b0a5c7419 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/idp/InfinispanIdentityProviderStorageProvider.java @@ -239,8 +239,8 @@ public class InfinispanIdentityProviderStorageProvider implements IdentityProvid } @Override - public Stream getMappersStream() { - return idpDelegate.getMappersStream(); + public Stream getMappersStream(Map options, Integer first, Integer max) { + return idpDelegate.getMappersStream(options, first, max); } @Override diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java index d242586d02..10a1c80ecb 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaIdentityProviderStorageProvider.java @@ -238,7 +238,9 @@ public class JpaIdentityProviderStorageProvider implements IdentityProviderStora } break; } + case ALIAS: case FIRST_BROKER_LOGIN_FLOW_ID: + case POST_BROKER_LOGIN_FLOW_ID: case ORGANIZATION_ID: { if (StringUtil.isBlank(value)) { predicates.add(builder.isNull(idp.get(key))); @@ -388,16 +390,52 @@ public class JpaIdentityProviderStorageProvider implements IdentityProviderStora } @Override - public Stream getMappersStream() { + public Stream getMappersStream(Map options, Integer first, Integer max) { CriteriaBuilder builder = em.getCriteriaBuilder(); CriteriaQuery query = builder.createQuery(IdentityProviderMapperEntity.class); - Root mapper = query.from(IdentityProviderMapperEntity.class); + Root idp = query.from(IdentityProviderMapperEntity.class); - Predicate predicate = builder.equal(mapper.get("realmId"), getRealm().getId()); + List predicates = new ArrayList<>(); + predicates.add(builder.equal(idp.get("realmId"), getRealm().getId())); - TypedQuery typedQuery = em.createQuery(query.select(mapper).where(predicate)); + if (options != null) { + for (Map.Entry entry : options.entrySet()) { + String key = entry.getKey(); + String value = entry.getValue(); + if (StringUtil.isBlank(key)) { + continue; + } + String dbProductName = em.unwrap(Session.class).doReturningWork(connection -> connection.getMetaData().getDatabaseProductName()); + MapJoin configJoin = idp.joinMap("config"); + Predicate configNamePredicate = builder.equal(configJoin.key(), key); - return closing(typedQuery.getResultStream().map(this::toModel)); + if (dbProductName.equals("Oracle")) { + // Oracle is not able to compare a CLOB with a VARCHAR unless it being converted with TO_CHAR + // But for this all values in the table need to be smaller than 4K, otherwise the cast will fail with + // "ORA-22835: Buffer too small for CLOB to CHAR" (even if it is in another row). + // This leaves DBMS_LOB.COMPARE and DBMS_LOB.INSTR as the options to compare the CLOB with the value. + if (value.endsWith("*")) { + // prefix search - use DBMS_LOB.INSTR + value = value.substring(0, value.length() - 1); + Predicate configValuePredicate = builder.equal(builder.function("DBMS_LOB.INSTR", Integer.class, configJoin.value(), builder.literal(value)), 1); + predicates.add(builder.and(configNamePredicate, configValuePredicate)); + } else { + Predicate configValuePredicate = builder.equal(builder.function("DBMS_LOB.COMPARE", Integer.class, configJoin.value(), builder.literal(value)), 0); + predicates.add(builder.and(configNamePredicate, configValuePredicate)); + } + } else { + if (value.endsWith("*")) { + value = value.replace("%", "\\%").replace("_", "\\_").replace("*", "%"); + predicates.add(builder.and(configNamePredicate, builder.like(configJoin.value(), value))); + } else { + predicates.add(builder.and(configNamePredicate, builder.equal(configJoin.value(), value))); + } + } + } + } + + TypedQuery typedQuery = em.createQuery(query.select(idp).where(predicates.toArray(Predicate[]::new))); + return closing(paginateQuery(typedQuery, first, max).getResultStream()).map(this::toModel); } @Override diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/AbstractConfigPropertySynchronizer.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/AbstractConfigPropertySynchronizer.java index 181f169fff..df79cc9b97 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/AbstractConfigPropertySynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/AbstractConfigPropertySynchronizer.java @@ -32,40 +32,14 @@ import java.util.function.Consumer; * @author Daniel Fesenmeyer */ public abstract class AbstractConfigPropertySynchronizer implements ConfigSynchronizer { + private static final Logger LOG = Logger.getLogger(AbstractConfigPropertySynchronizer.class); - protected abstract String getConfigPropertyName(); + protected void logEventProcessed(String configPropertyName, String previousValue, String newValue, String realmName, + String mapperName, String idpAlias) { + LOG.infof( + "Reference of type '%s' changed from '%s' to '%s' in realm '%s'. Adjusting the reference from mapper '%s' of IDP '%s'.", + configPropertyName, previousValue, newValue, realmName, mapperName, idpAlias); - protected abstract void updateConfigPropertyIfNecessary(T event, String currentPropertyValue, - Consumer propertyUpdater); - - @Override - public final void handleEvent(T event, IdentityProviderMapperModel idpMapper) { - Map config = idpMapper.getConfig(); - if (config == null) { - return; - } - - String configPropertyName = getConfigPropertyName(); - String configuredValue = config.get(configPropertyName); - if (StringUtil.isBlank(configuredValue)) { - return; - } - - Consumer propertyUpdater = value -> config.put(configPropertyName, value); - - updateConfigPropertyIfNecessary(event, configuredValue, propertyUpdater); - - final String newConfiguredValue = config.get(configPropertyName); - if (!configuredValue.equals(newConfiguredValue)) { - RealmModel realm = extractRealm(event); - - LOG.infof( - "Reference of type '%s' changed from '%s' to '%s' in realm '%s'. Adjusting the reference from mapper '%s' of IDP '%s'.", - configPropertyName, configuredValue, newConfiguredValue, realm.getName(), idpMapper.getName(), - idpMapper.getIdentityProviderAlias()); - getKeycloakSession(event).identityProviders().updateMapper(idpMapper); - } } - } diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSyncEventListener.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSyncEventListener.java index 19a27f9018..96be427a66 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSyncEventListener.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSyncEventListener.java @@ -42,24 +42,12 @@ public final class ConfigSyncEventListener implements ProviderEventListener { @Override public void onEvent(ProviderEvent event) { - List realmMappers = null; - for (ConfigSynchronizer s : SYNCHRONIZERS) { ConfigSynchronizer configSynchronizer = (ConfigSynchronizer) s; if (eventMatchesSynchronizer(event, configSynchronizer)) { LOG.debugf("Synchronizer %s matches event: %s", configSynchronizer, event); - - if (realmMappers == null) { - realmMappers = configSynchronizer.getKeycloakSession(event).identityProviders().getMappersStream() - .collect(Collectors.toList()); - } - - realmMappers.forEach(idpMapper -> { - LOG.debugf("Apply synchronizer %s to event %s and mapper with name %s", configSynchronizer, event, - idpMapper.getName()); - configSynchronizer.handleEvent(event, idpMapper); - }); + configSynchronizer.handleEvent(event); } else { LOG.debugf("Synchronizer %s does not match event: %s", configSynchronizer, event); } diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSynchronizer.java index 4ae572336c..7f13b9afc5 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/ConfigSynchronizer.java @@ -30,9 +30,5 @@ import org.keycloak.provider.ProviderEvent; public interface ConfigSynchronizer { Class getEventClass(); - RealmModel extractRealm(T event); - - KeycloakSession getKeycloakSession(T event); - - void handleEvent(T event, IdentityProviderMapperModel idpMapper); -} \ No newline at end of file + void handleEvent(T event); +} diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/GroupConfigPropertyByPathSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/GroupConfigPropertyByPathSynchronizer.java index 53485f3d92..c81f45a5e0 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/GroupConfigPropertyByPathSynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/GroupConfigPropertyByPathSynchronizer.java @@ -23,6 +23,7 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.KeycloakModelUtils; +import java.util.Map; import java.util.function.Consumer; import static org.keycloak.models.utils.KeycloakModelUtils.GROUP_PATH_SEPARATOR; @@ -46,37 +47,27 @@ public class GroupConfigPropertyByPathSynchronizer extends AbstractConfigPropert } @Override - public RealmModel extractRealm(GroupModel.GroupPathChangeEvent event) { - return event.getRealm(); + public void handleEvent(GroupModel.GroupPathChangeEvent event) { + + // first find all mappers that have a group config property that maps exactly to the changed path. + event.getKeycloakSession().identityProviders().getMappersStream(Map.of(ConfigConstants.GROUP, event.getPreviousPath()), null, null) + .forEach(idpMapper -> { + idpMapper.getConfig().put(ConfigConstants.GROUP, event.getNewPath()); + super.logEventProcessed(ConfigConstants.GROUP, event.getPreviousPath(), event.getNewPath(), event.getRealm().getName(), + idpMapper.getName(), idpMapper.getIdentityProviderAlias()); + event.getKeycloakSession().identityProviders().updateMapper(idpMapper); + }); + + // then find all mappers that have a group config that maps to a sub-path of the changed path. + event.getKeycloakSession().identityProviders().getMappersStream( + Map.of(ConfigConstants.GROUP, event.getPreviousPath() + GROUP_PATH_SEPARATOR + "*"), null, null) + .forEach(idpMapper -> { + String currentGroupPath = idpMapper.getConfig().get(ConfigConstants.GROUP); + String newGroupPath = event.getNewPath() + currentGroupPath.substring(event.getPreviousPath().length()); + idpMapper.getConfig().put(ConfigConstants.GROUP, newGroupPath); + super.logEventProcessed(ConfigConstants.GROUP, currentGroupPath, newGroupPath, event.getRealm().getName(), + idpMapper.getName(), idpMapper.getIdentityProviderAlias()); + event.getKeycloakSession().identityProviders().updateMapper(idpMapper); + }); } - - @Override - public KeycloakSession getKeycloakSession(GroupModel.GroupPathChangeEvent event) { - return event.getKeycloakSession(); - } - - @Override - public String getConfigPropertyName() { - return ConfigConstants.GROUP; - } - - @Override - protected void updateConfigPropertyIfNecessary(GroupModel.GroupPathChangeEvent event, - String currentPropertyValue, Consumer propertyUpdater) { - String configuredGroupPath = KeycloakModelUtils.normalizeGroupPath(currentPropertyValue); - - String previousGroupPath = event.getPreviousPath(); - if (configuredGroupPath.equals(previousGroupPath)) { - String newPath = event.getNewPath(); - propertyUpdater.accept(newPath); - } else if (isSubGroupOf(configuredGroupPath, previousGroupPath)) { - String newPath = event.getNewPath() + configuredGroupPath.substring(previousGroupPath.length()); - propertyUpdater.accept(newPath); - } - } - - private static boolean isSubGroupOf(String groupPath, String potentialParentGroupPath) { - return groupPath.startsWith(potentialParentGroupPath + GROUP_PATH_SEPARATOR); - } - } diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByClientIdSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByClientIdSynchronizer.java index 7bf2eb91d3..6a622b3245 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByClientIdSynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByClientIdSynchronizer.java @@ -23,6 +23,7 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.utils.KeycloakModelUtils; +import java.util.Map; import java.util.function.Consumer; /** @@ -30,8 +31,7 @@ import java.util.function.Consumer; * * @author Daniel Fesenmeyer */ -public class RoleConfigPropertyByClientIdSynchronizer - extends AbstractConfigPropertySynchronizer { +public class RoleConfigPropertyByClientIdSynchronizer extends AbstractConfigPropertySynchronizer { public static final RoleConfigPropertyByClientIdSynchronizer INSTANCE = new RoleConfigPropertyByClientIdSynchronizer(); @@ -46,36 +46,17 @@ public class RoleConfigPropertyByClientIdSynchronizer } @Override - public RealmModel extractRealm(ClientModel.ClientIdChangeEvent event) { - return event.getUpdatedClient().getRealm(); + public void handleEvent(ClientModel.ClientIdChangeEvent event) { + // find all mappers that have a role config property that maps to a role associated with the changed client. + event.getKeycloakSession().identityProviders().getMappersStream(Map.of(ConfigConstants.ROLE, event.getPreviousClientId() + ".*"), null, null) + .forEach(idpMapper -> { + String currentRoleValue = idpMapper.getConfig().get(ConfigConstants.ROLE); + String configuredRoleName = KeycloakModelUtils.parseRole(currentRoleValue)[1]; + String newRoleValue = KeycloakModelUtils.buildRoleQualifier(event.getNewClientId(), configuredRoleName); + idpMapper.getConfig().put(ConfigConstants.ROLE, newRoleValue); + super.logEventProcessed(ConfigConstants.ROLE, currentRoleValue, newRoleValue, event.getUpdatedClient().getRealm().getName(), + idpMapper.getName(), idpMapper.getIdentityProviderAlias()); + event.getKeycloakSession().identityProviders().updateMapper(idpMapper); + }); } - - @Override - public KeycloakSession getKeycloakSession(ClientModel.ClientIdChangeEvent event) { - return event.getKeycloakSession(); - } - - @Override - public String getConfigPropertyName() { - return ConfigConstants.ROLE; - } - - @Override - protected void updateConfigPropertyIfNecessary(ClientModel.ClientIdChangeEvent event, String currentPropertyValue, - Consumer propertyUpdater) { - String[] parsedConfiguredRoleQualifier = KeycloakModelUtils.parseRole(currentPropertyValue); - String configuredClientId = parsedConfiguredRoleQualifier[0]; - if (configuredClientId == null) { - // a realm role is configured for the mapper, event is not relevant - return; - } - - String configuredRoleName = parsedConfiguredRoleQualifier[1]; - - if (configuredClientId.equals(event.getPreviousClientId())) { - String newRoleQualifier = KeycloakModelUtils.buildRoleQualifier(event.getNewClientId(), configuredRoleName); - propertyUpdater.accept(newRoleQualifier); - } - } - } diff --git a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByRoleNameSynchronizer.java b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByRoleNameSynchronizer.java index ed2b5d75f2..67ce7325d3 100644 --- a/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByRoleNameSynchronizer.java +++ b/server-spi-private/src/main/java/org/keycloak/broker/provider/mappersync/RoleConfigPropertyByRoleNameSynchronizer.java @@ -23,6 +23,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.utils.KeycloakModelUtils; +import java.util.Map; import java.util.function.Consumer; /** @@ -46,30 +47,17 @@ public class RoleConfigPropertyByRoleNameSynchronizer } @Override - public RealmModel extractRealm(RoleModel.RoleNameChangeEvent event) { - return event.getRealm(); + public void handleEvent(RoleModel.RoleNameChangeEvent event) { + // first find all mappers that have a role config property that maps exactly to the changed path. + String currentRoleValue = KeycloakModelUtils.buildRoleQualifier(event.getClientId(), event.getPreviousName()); + event.getKeycloakSession().identityProviders().getMappersStream(Map.of(ConfigConstants.ROLE, currentRoleValue), null, null) + .forEach(idpMapper -> { + String newRoleValue = KeycloakModelUtils.buildRoleQualifier(event.getClientId(), event.getNewName()); + idpMapper.getConfig().put(ConfigConstants.ROLE, newRoleValue); + super.logEventProcessed(ConfigConstants.ROLE, currentRoleValue, newRoleValue, event.getRealm().getName(), + idpMapper.getName(), idpMapper.getIdentityProviderAlias()); + event.getKeycloakSession().identityProviders().updateMapper(idpMapper); + }); + } - - @Override - public KeycloakSession getKeycloakSession(RoleModel.RoleNameChangeEvent event) { - return event.getKeycloakSession(); - } - - @Override - public String getConfigPropertyName() { - return ConfigConstants.ROLE; - } - - @Override - protected void updateConfigPropertyIfNecessary(RoleModel.RoleNameChangeEvent event, String currentPropertyValue, - Consumer propertyUpdater) { - - String previousRoleQualifier = - KeycloakModelUtils.buildRoleQualifier(event.getClientId(), event.getPreviousName()); - if (previousRoleQualifier.equals(currentPropertyValue)) { - String newRoleQualifier = KeycloakModelUtils.buildRoleQualifier(event.getClientId(), event.getNewName()); - propertyUpdater.accept(newRoleQualifier); - } - } - } diff --git a/server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java b/server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java index 0189f4357f..38e9b5388f 100644 --- a/server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java +++ b/server-spi/src/main/java/org/keycloak/models/IdentityProviderStorageProvider.java @@ -102,14 +102,23 @@ public interface IdentityProviderStorageProvider extends Provider { } /** - * Returns all identity providers in the realm filtered according to the specified parameters. + * Returns all identity providers in the realm filtered according to the specified search options. The options include: + *
    + *
  • Regular fields found in {@link IdentityProviderModel}, such as {@code ALIAS}, {@code ENABLED}, {@code HIDE_ON_LOGIN}, etc;
    • + *
  • Special search keys also present in {@link IdentityProviderModel}. Those include {@code SEARCH}, used to perform + * exact, prefix, and infix searches by alias, and {@code ALIAS_NOT_IN}, used to perform searches for identity providers + * whose alias doesn't match any of the specified aliases (separated by comma);
    • + *
  • Any attribute found in the identity provider's config. If the option key doesn't match any of the previous + * cases, the implementations must search the providers whose config contains a pair that matches the specified search + * option
    • + *
* - * @param attrs a {@code Map} containig identity provider config attributes that must be matched. + * @param options a {@link Map} containing identity provider search options that must be matched. * @param first the position of the first result to be processed (pagination offset). Ignored if negative or {@code null}. * @param max the maximum number of results to be returned. Ignored if negative or {@code null}. * @return a non-null stream of {@link IdentityProviderModel}s that match the search criteria. */ - Stream getAllStream(Map attrs, Integer first, Integer max); + Stream getAllStream(Map options, Integer first, Integer max); /** * Returns all identity providers associated with the organization with the provided id. @@ -295,7 +304,19 @@ public interface IdentityProviderStorageProvider extends Provider { * Returns all identity provider mappers as a stream. * @return Stream of {@link IdentityProviderMapperModel}. Never returns {@code null}. */ - Stream getMappersStream(); + default Stream getMappersStream() { + return this.getMappersStream(Map.of(), null, null); + } + + /** + * Returns all identity provider mappers in the realm filtered according to the specified search options. + * + * @param options a {@link Map} containing identity provider search options that must be matched. + * @param first the position of the first result to be processed (pagination offset). Ignored if negative or {@code null}. + * @param max the maximum number of results to be returned. Ignored if negative or {@code null}. + * @return a non-null stream of {@link IdentityProviderModel}s that match the search criteria. + */ + Stream getMappersStream(Map options, Integer first, Integer max); /** * Returns identity provider mappers by the provided alias as a stream. diff --git a/testsuite/integration-arquillian/tests/base/testsuites/database-suite b/testsuite/integration-arquillian/tests/base/testsuites/database-suite index 827343d353..92d3b8b248 100644 --- a/testsuite/integration-arquillian/tests/base/testsuites/database-suite +++ b/testsuite/integration-arquillian/tests/base/testsuites/database-suite @@ -15,6 +15,8 @@ SSOTest SamlClientTest TransactionsTest UserProfileTest +OidcAdvancedClaimToGroupMapperTest +OidcAdvancedClaimToRoleMapperTest org.keycloak.testsuite.admin.** org.keycloak.testsuite.authz.**ManagementTest org.keycloak.testsuite.organization.admin.**