diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java index 71820f67a85..935f2d38809 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java @@ -26,12 +26,10 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import org.keycloak.Config; import org.keycloak.client.clienttype.ClientTypeManager; import org.keycloak.cluster.ClusterProvider; import org.keycloak.common.Profile; import org.keycloak.component.ComponentModel; -import org.keycloak.models.AdminRoles; import org.keycloak.models.ClientInitialAccessModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientProvider; @@ -483,12 +481,6 @@ public class RealmCacheSession implements CacheRealmProvider { return adapter; } - /** - * Cache that we've triggered an admin role invalidation in this session already. - * This avoids loading the admin role again from the database - */ - private boolean adminRoleInvalidated; - private RealmAdapter prepareCachedRealm(String id, KeycloakSession session) { CachedRealm cached = cache.get(id, CachedRealm.class); RealmAdapter adapter; @@ -499,18 +491,6 @@ public class RealmCacheSession implements CacheRealmProvider { return null; } - // To accommodate imports of new realms in a separate process, always evict the admin role. - // As an alternative, one could iterate through all composite roles of the Admin role, but that would - // load about 20 roles per realm to the cache which is also inefficient. - if (!adminRoleInvalidated && !model.getName().equals(Config.getAdminRealm())) { - RealmModel adminRealm = session.realms().getRealmByName(Config.getAdminRealm()); - if (adminRealm != null) { - RoleModel adminRole = adminRealm.getRole(AdminRoles.ADMIN); - invalidateRole(adminRole.getId()); - adminRoleInvalidated = true; - } - } - cached = new CachedRealm(loaded, model); cache.addRevisioned(cached, startupRevision); adapter = new RealmAdapter(session, cached, this); @@ -559,11 +539,9 @@ public class RealmCacheSession implements CacheRealmProvider { } query = new RealmListQuery(loaded, cacheKey, model.getId()); cache.addRevisioned(query, startupRevision); - return model; - } else { - String realmId = query.getRealms().iterator().next(); - return getRealm(realmId); } + String realmId = query.getRealms().iterator().next(); + return getRealm(realmId); } static String getRealmByNameCacheKey(String name) { @@ -757,7 +735,6 @@ public class RealmCacheSession implements CacheRealmProvider { query = new RoleListQuery(loaded, cacheKey, realm, ids); logger.tracev("adding realm roles cache miss: realm {0} key {1}", realm.getName(), cacheKey); cache.addRevisioned(query, startupRevision); - return model.stream(); } Set list = new HashSet<>(); for (String id : query.getRoles()) { @@ -792,7 +769,6 @@ public class RealmCacheSession implements CacheRealmProvider { query = new RoleListQuery(loaded, cacheKey, client.getRealm(), ids, client.getClientId()); logger.tracev("adding client roles cache miss: client {0} key {1}", client.getClientId(), cacheKey); cache.addRevisioned(query, startupRevision); - return model.stream(); } Set list = new HashSet<>(); for (String id : query.getRoles()) { @@ -877,7 +853,6 @@ public class RealmCacheSession implements CacheRealmProvider { } logger.tracev("adding realm role cache miss: client {0} key {1}", realm.getName(), cacheKey); cache.addRevisioned(query, startupRevision); - return model; } String roleId = query.getRole(); if (roleId == null) { @@ -915,7 +890,6 @@ public class RealmCacheSession implements CacheRealmProvider { } logger.tracev("adding client role cache miss: client {0} key {1}", client.getClientId(), cacheKey); cache.addRevisioned(query, startupRevision); - return model; } String roleId = query.getRole(); if (roleId == null) { @@ -952,6 +926,22 @@ public class RealmCacheSession implements CacheRealmProvider { @Override public RoleModel getRoleById(RealmModel realm, String id) { + if (invalidations.contains(id)) { + return getRoleDelegate().getRoleById(realm, id); + } else if (managedRoles.containsKey(id)) { + return managedRoles.get(id); + } + + CachedRole cached = getCachedRole(realm, id); + if (cached == null) { + return null; + } + RoleAdapter adapter = new RoleAdapter(cached,this, realm); + managedRoles.put(id, adapter); + return adapter; + } + + protected CachedRole getCachedRole(RealmModel realm, String id) { CachedRole cached = cache.get(id, CachedRole.class); if (cached != null && !cached.getRealm().equals(realm.getId())) { cached = null; @@ -961,22 +951,14 @@ public class RealmCacheSession implements CacheRealmProvider { long loaded = cache.getCurrentRevision(id); RoleModel model = getRoleDelegate().getRoleById(realm, id); if (model == null) return null; - if (invalidations.contains(id)) return model; if (model.isClientRole()) { cached = new CachedClientRole(loaded, model.getContainerId(), model, realm); } else { cached = new CachedRealmRole(loaded, model, realm); } cache.addRevisioned(cached, startupRevision); - - } else if (invalidations.contains(id)) { - return getRoleDelegate().getRoleById(realm, id); - } else if (managedRoles.containsKey(id)) { - return managedRoles.get(id); } - RoleAdapter adapter = new RoleAdapter(cached,this, realm); - managedRoles.put(id, adapter); - return adapter; + return cached; } @Override @@ -1343,9 +1325,6 @@ public class RealmCacheSession implements CacheRealmProvider { query = new ClientListQuery(loaded, cacheKey, realm, id); logger.tracev("adding client by name cache miss: {0}", clientId); cache.addRevisioned(query, startupRevision); - if (invalidations.contains(model.getId())) { - return model; - } } else { id = query.getClients().iterator().next(); } @@ -1607,4 +1586,38 @@ public class RealmCacheSession implements CacheRealmProvider { listInvalidations.add(realm.getId()); getGroupDelegate().preRemove(realm); } + + @Override + public boolean refreshMasterAdminRole(RoleModel masterAdminRole, String clientId) { + if (!(masterAdminRole instanceof RoleAdapter)) { + return false; + } + RoleAdapter role = (RoleAdapter)masterAdminRole; + if (role.isUpdated() || role.cached.getCachedComposites().clientContainerIds().contains(clientId)) { + return false; + } + CachedRole inCache = getCachedRole(role.realm, role.getId()); + if (inCache == null) { + return false; + } + if (!inCache.getComposites(session, role::getRoleModel).clientContainerIds().contains(clientId)) { + // we suspect that the cache entry is out-of-date, lock to prevent multiple invalidations + synchronized (inCache) { + inCache = getCachedRole(role.realm, role.getId()); + if (inCache == null) { + return false; + } + if (!inCache.getComposites(session, role::getRoleModel).clientContainerIds().contains(clientId)) { + cache.invalidateObject(role.getId()); + inCache = getCachedRole(role.realm, role.getId()); + } + } + } + if (inCache != null) { + role.composites = null; + role.cached = inCache; + return true; + } + return false; + } } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java index f92ab8e6cea..bf2e9604d62 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java @@ -142,7 +142,7 @@ public class RoleAdapter implements RoleModel { if (composites == null) { composites = new HashSet<>(); - for (String id : cached.getComposites(session, modelSupplier)) { + for (String id : cached.getComposites(session, modelSupplier).ids()) { RoleModel role = realm.getRoleById(id); if (role == null) { // chance that composite role was removed, so invalidate this entry and fallback to delegate @@ -160,7 +160,7 @@ public class RoleAdapter implements RoleModel { public Stream getCompositesStream(String search, Integer first, Integer max) { if (isUpdated()) return updated.getCompositesStream(search, first, max); - return cacheSession.getRoleDelegate().getRolesStream(realm, cached.getComposites(session, modelSupplier).stream(), search, first, max); + return cacheSession.getRoleDelegate().getRolesStream(realm, cached.getComposites(session, modelSupplier).ids().stream(), search, first, max); } @Override @@ -243,7 +243,7 @@ public class RoleAdapter implements RoleModel { return cached.getAttributes(session, modelSupplier); } - private RoleModel getRoleModel() { + protected RoleModel getRoleModel() { return cacheSession.getRoleDelegate().getRoleById(realm, cached.getId()); } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java index 2173cdf5491..b741ab13c78 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java @@ -17,10 +17,10 @@ package org.keycloak.models.cache.infinispan.entities; +import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.function.Supplier; -import java.util.stream.Collectors; import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.models.KeycloakSession; @@ -35,15 +35,17 @@ import org.keycloak.models.cache.infinispan.LazyLoader; */ public class CachedRole extends AbstractRevisioned implements InRealm { + public record CompositeRolesRecord (Set clientContainerIds, Set ids) {} + final protected String name; final protected String realm; final protected String description; - final protected LazyLoader> composites; + final protected LazyLoader composites; /** * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this * items should be evicted. */ - private Set cachedComposites = new HashSet<>(); + private volatile CompositeRolesRecord cachedComposites = new CompositeRolesRecord(Set.of(), Set.of()); private final LazyLoader> attributes; public CachedRole(long revision, RoleModel model, RealmModel realm) { @@ -51,7 +53,17 @@ public class CachedRole extends AbstractRevisioned implements InRealm { description = model.getDescription(); name = model.getName(); this.realm = realm.getId(); - composites = new DefaultLazyLoader<>(roleModel -> roleModel.getCompositesStream().map(RoleModel::getId).collect(Collectors.toSet()), HashSet::new); + composites = new DefaultLazyLoader<>(roleModel -> { + Set ids = new HashSet<>(); + Set clientContainerIds = new HashSet<>(); + roleModel.getCompositesStream().forEach(r -> { + ids.add(r.getId()); + if (r.isClientRole()) { + clientContainerIds.add(r.getContainerId()); + } + }); + return new CompositeRolesRecord(Collections.unmodifiableSet(clientContainerIds), Collections.unmodifiableSet(ids)); + }, null); attributes = new DefaultLazyLoader<>(roleModel -> new MultivaluedHashMap<>(roleModel.getAttributes()), MultivaluedHashMap::new); } @@ -59,6 +71,7 @@ public class CachedRole extends AbstractRevisioned implements InRealm { return name; } + @Override public String getRealm() { return realm; } @@ -68,10 +81,10 @@ public class CachedRole extends AbstractRevisioned implements InRealm { } public boolean isComposite(KeycloakSession session, Supplier roleModel) { - return !getComposites(session, roleModel).isEmpty(); + return !getComposites(session, roleModel).ids().isEmpty(); } - public Set getComposites(KeycloakSession session, Supplier roleModel) { + public CompositeRolesRecord getComposites(KeycloakSession session, Supplier roleModel) { cachedComposites = composites.get(session, roleModel); return cachedComposites; } @@ -80,7 +93,7 @@ public class CachedRole extends AbstractRevisioned implements InRealm { * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this * items should be evicted. Will return an empty list if it hasn't been cached yet (and then no invalidation is necessary) */ - public Set getCachedComposites() { + public CompositeRolesRecord getCachedComposites() { return cachedComposites; } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java index 96846394ec2..59ce4c36d65 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java @@ -43,7 +43,7 @@ public class HasRolePredicate implements Predicate @Override public boolean test(Map.Entry entry) { Object value = entry.getValue(); - return (value instanceof CachedRole cachedRole && cachedRole.getCachedComposites().contains(role)) || + return (value instanceof CachedRole cachedRole && cachedRole.getCachedComposites().ids().contains(role)) || (value instanceof CachedGroup cachedGroup && cachedGroup.getCachedRoleMappings().contains(role)) || (value instanceof RoleQuery roleQuery && roleQuery.getRoles().contains(role)) || (value instanceof CachedClient cachedClient && cachedClient.getScope().contains(role)) || diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java index b5f2843e941..2da1221e329 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java @@ -116,7 +116,8 @@ public class RoleAdapter implements RoleModel, JpaModel { @Override public Stream getCompositesStream() { - Stream composites = getChildRoles().map(c -> new RoleAdapter(session, realm, em, c)); + // look up the roles via the session to allow returning cached entries + Stream composites = getChildRoles().map(c -> session.roles().getRoleById(realm, c.getId())); return composites.filter(Objects::nonNull); } diff --git a/model/storage-private/src/main/java/org/keycloak/models/cache/CacheRealmProvider.java b/model/storage-private/src/main/java/org/keycloak/models/cache/CacheRealmProvider.java index c45257898f1..69c92fda89c 100755 --- a/model/storage-private/src/main/java/org/keycloak/models/cache/CacheRealmProvider.java +++ b/model/storage-private/src/main/java/org/keycloak/models/cache/CacheRealmProvider.java @@ -21,6 +21,7 @@ import org.keycloak.models.ClientProvider; import org.keycloak.models.ClientScopeProvider; import org.keycloak.models.GroupProvider; import org.keycloak.models.RealmProvider; +import org.keycloak.models.RoleModel; import org.keycloak.models.RoleProvider; /** @@ -40,4 +41,7 @@ public interface CacheRealmProvider extends RealmProvider, ClientProvider, Clien void registerGroupInvalidation(String id); void registerInvalidation(String id); + default boolean refreshMasterAdminRole(RoleModel masterAdminRole, String clientId) { + return false; + } } diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ImportDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ImportDistTest.java index b59fe274315..91e69a3a27e 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ImportDistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ImportDistTest.java @@ -35,6 +35,7 @@ import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestMethodOrder; +import static org.junit.Assert.assertFalse; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -78,6 +79,16 @@ public class ImportDistTest { @Test void testImportNewRealm(KeycloakDistribution dist) throws IOException { + dist.setEnvVar("MY_SECRET", "admin123"); + + RawKeycloakDistribution rawDist = dist.unwrap(RawKeycloakDistribution.class); + CLIResult result = rawDist.run("bootstrap-admin", "service", "--db=dev-file", "--client-id=admin", "--client-secret:env=MY_SECRET"); + + assertTrue(result.getErrorOutput().isEmpty(), result.getErrorOutput()); + + rawDist.setManualStop(true); + result = rawDist.run("start-dev", "--db-url-properties=;AUTO_SERVER=TRUE;DB_CLOSE_ON_EXIT=TRUE"); + File file = new File("target/realm.json"); RealmRepresentation newRealm=new RealmRepresentation(); @@ -90,20 +101,14 @@ public class ImportDistTest { mapper.writeValue(fos, newRealm); } - var cliResult = dist.run("import", "--file=" + file.getAbsolutePath()); + CLIResult adminResult = rawDist.kcadm("get", "realms", "--server", "http://localhost:8080", "--realm", "master", "--client", "admin", "--secret", "admin123"); + assertEquals(0, adminResult.exitCode()); + assertFalse(adminResult.getOutput().contains("anotherRealm")); + + var cliResult = rawDist.kc("import", "--db-url-properties=;AUTO_SERVER=TRUE;DB_CLOSE_ON_EXIT=TRUE", "--file=" + file.getAbsolutePath()); cliResult.assertMessage("Realm 'anotherRealm' imported"); - dist.setEnvVar("MY_SECRET", "admin123"); - - RawKeycloakDistribution rawDist = dist.unwrap(RawKeycloakDistribution.class); - CLIResult result = rawDist.run("bootstrap-admin", "service", "--db=dev-file", "--client-id=admin", "--client-secret:env=MY_SECRET"); - - assertTrue(result.getErrorOutput().isEmpty(), result.getErrorOutput()); - - rawDist.setManualStop(true); - rawDist.run("start-dev"); - - CLIResult adminResult = rawDist.kcadm("get", "realms", "--server", "http://localhost:8080", "--realm", "master", "--client", "admin", "--secret", "admin123"); + adminResult = rawDist.kcadm("get", "realms", "--server", "http://localhost:8080", "--realm", "master", "--client", "admin", "--secret", "admin123"); assertEquals(0, adminResult.exitCode()); assertTrue(adminResult.getOutput().contains("anotherRealm")); diff --git a/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java b/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java index 59c95df501c..c46e2d6cd69 100644 --- a/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java +++ b/quarkus/tests/junit5/src/main/java/org/keycloak/it/utils/RawKeycloakDistribution.java @@ -134,14 +134,26 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { return this; } + public CLIResult kc(String... arguments) throws IOException { + return kc(Arrays.asList(arguments)); + } + + public CLIResult kc(List arguments) throws IOException { + return invoke(SCRIPT_CMD, arguments); + } + public CLIResult kcadm(String... arguments) throws IOException { return kcadm(Arrays.asList(arguments)); } public CLIResult kcadm(List arguments) throws IOException { + return invoke(SCRIPT_KCADM_CMD, arguments); + } + + private CLIResult invoke(String script, List arguments) throws IOException { List allArgs = new ArrayList<>(); - invoke(allArgs, SCRIPT_KCADM_CMD); + invoke(allArgs, script); if (this.isDebug()) { allArgs.add("-x"); @@ -159,12 +171,12 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { builder.environment().putAll(envVars); - Process kcadm = builder.start(); + Process proc = builder.start(); DefaultOutputConsumer outputConsumer = new DefaultOutputConsumer(); - readOutput(kcadm, outputConsumer); + readOutput(proc, outputConsumer); - int exitValue = kcadm.exitValue(); + int exitValue = proc.exitValue(); return CLIResult.create(outputConsumer.getStdOut(), outputConsumer.getErrOut(), exitValue); } @@ -257,7 +269,7 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { if (descendants.isEmpty()) { return; } - + LOG.debugf("Found %d descendant processes to terminate", descendants.size()); CompletableFuture allProcesses = CompletableFuture.completedFuture(null); @@ -498,7 +510,7 @@ public final class RawKeycloakDistribution implements KeycloakDistribution { } private Path inDistZipDirectory(File distFile) throws Exception{ - + try (ZipFile zipFile = new ZipFile(distFile)) { Optional e = zipFile.stream().filter(ZipEntry::isDirectory).findFirst(); if (e.isPresent()) { diff --git a/services/src/main/java/org/keycloak/services/resources/admin/fgap/MgmtPermissions.java b/services/src/main/java/org/keycloak/services/resources/admin/fgap/MgmtPermissions.java index 001676ded27..8215209c0e8 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/fgap/MgmtPermissions.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/fgap/MgmtPermissions.java @@ -45,6 +45,7 @@ import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; +import org.keycloak.models.cache.CacheRealmProvider; import org.keycloak.protocol.oidc.mappers.AbstractOIDCProtocolMapper; import org.keycloak.representations.AccessToken; import org.keycloak.representations.idm.authorization.Permission; @@ -192,11 +193,13 @@ class MgmtPermissions implements AdminPermissionEvaluator, AdminPermissionManage if (!admin.hasRole(masterAdminRole)) { return false; } + CacheRealmProvider cache = session.getProvider(CacheRealmProvider.class); + if (cache == null || !cache.refreshMasterAdminRole(masterAdminRole, clientId)) { + return false; + } Set roleNames = Set.of(adminRoles); - ClientModel clientModel = masterRealm.getClientByClientId(clientId); - return clientModel != null && masterAdminRole.getCompositesStream() - .anyMatch(r -> (r.isClientRole() && r.getContainerId().equals(clientModel.getId()) - && roleNames.contains(r.getName()))); + return masterAdminRole.getCompositesStream().anyMatch(r -> (r.isClientRole() + && r.getContainerId().equals(clientId) && roleNames.contains(r.getName()))); } public boolean isAdminSameRealm() {