From daaa0d0a151246e663f3aae6005e3a43ba261e4b Mon Sep 17 00:00:00 2001 From: wilmerdooley Date: Fri, 26 Jun 2026 20:18:03 +0000 Subject: [PATCH] fix: avoid duplicate role creation when importing realm roles Signed-off-by: wilmerdooley --- .../datastore/DefaultExportImportManager.java | 2 +- .../models/utils/RepresentationToModel.java | 50 ++++++-- .../utils/RepresentationToModelTest.java | 121 ++++++++++++++++++ ...lmImportSharedUndefinedClientRoleTest.java | 87 +++++++++++++ 4 files changed, 250 insertions(+), 10 deletions(-) create mode 100644 server-spi-private/src/test/java/org/keycloak/models/utils/RepresentationToModelTest.java create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/exportimport/RealmImportSharedUndefinedClientRoleTest.java diff --git a/model/storage-private/src/main/java/org/keycloak/storage/datastore/DefaultExportImportManager.java b/model/storage-private/src/main/java/org/keycloak/storage/datastore/DefaultExportImportManager.java index 519d0c8095b1..265fa59e6953 100644 --- a/model/storage-private/src/main/java/org/keycloak/storage/datastore/DefaultExportImportManager.java +++ b/model/storage-private/src/main/java/org/keycloak/storage/datastore/DefaultExportImportManager.java @@ -1014,7 +1014,7 @@ public UserModel createUser(RealmModel newRealm, UserRepresentation userRep) { } createCredentials(userRep, session, newRealm, user, false); createFederatedIdentities(userRep, session, newRealm, user); - createRoleMappings(userRep, user, newRealm); + createRoleMappings(session, userRep, user, newRealm); if (userRep.getClientConsents() != null) { for (UserConsentRepresentation consentRep : userRep.getClientConsents()) { UserConsentModel consentModel = RepresentationToModel.toModel(newRealm, consentRep, session); diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java index d9d56bb41665..e6e3e6982451 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java @@ -67,6 +67,7 @@ import org.keycloak.common.util.ObjectUtil; import org.keycloak.common.util.UriUtils; import org.keycloak.component.ComponentModel; +import org.keycloak.connections.jpa.support.EntityManagers; import org.keycloak.credential.CredentialModel; import org.keycloak.deployment.DeployedConfigurationsManager; import org.keycloak.migration.migrators.MigrationUtils; @@ -163,7 +164,7 @@ public static void importRoles(RolesRepresentation realmRoles, RealmModel realm) if (realmRoles.getRealm() != null) { // realm roles for (RoleRepresentation roleRep : realmRoles.getRealm()) { if (! realm.getDefaultRole().getName().equals(roleRep.getName())) { // default role was already imported - createRole(realm, roleRep); + importRealmRole(realm, roleRep); } } } @@ -175,11 +176,7 @@ public static void importRoles(RolesRepresentation realmRoles, RealmModel realm) } for (RoleRepresentation roleRep : entry.getValue()) { // Application role may already exists (for example if it is defaultRole) - RoleModel role = roleRep.getId() != null ? client.addRole(roleRep.getId(), roleRep.getName()) : client.addRole(roleRep.getName()); - role.setDescription(roleRep.getDescription()); - if (roleRep.getAttributes() != null) { - roleRep.getAttributes().forEach((key, value) -> role.setAttribute(key, value)); - } + importClientRole(client, roleRep); } } } @@ -204,6 +201,31 @@ public static void importRoles(RolesRepresentation realmRoles, RealmModel realm) } } + private static RoleModel importRealmRole(RealmModel realm, RoleRepresentation roleRep) { + RoleModel role = realm.getRole(roleRep.getName()); + if (role == null) { + role = roleRep.getId() != null ? realm.addRole(roleRep.getId(), roleRep.getName()) : realm.addRole(roleRep.getName()); + } + updateRole(role, roleRep); + return role; + } + + private static RoleModel importClientRole(ClientModel client, RoleRepresentation roleRep) { + RoleModel role = client.getRole(roleRep.getName()); + if (role == null) { + role = roleRep.getId() != null ? client.addRole(roleRep.getId(), roleRep.getName()) : client.addRole(roleRep.getName()); + } + updateRole(role, roleRep); + return role; + } + + private static void updateRole(RoleModel role, RoleRepresentation roleRep) { + role.setDescription(roleRep.getDescription()); + if (roleRep.getAttributes() != null) { + roleRep.getAttributes().forEach(role::setAttribute); + } + } + public static void importGroup(RealmModel realm, GroupModel parent, GroupRepresentation group) { GroupModel newGroup = realm.createGroup(group.getId(), group.getName(), parent); @@ -849,12 +871,17 @@ public static CredentialModel toModel(CredentialRepresentation cred) { // Role mappings - public static void createRoleMappings(UserRepresentation userRep, UserModel user, RealmModel realm) { + public static void createRoleMappings(KeycloakSession session, UserRepresentation userRep, UserModel user, RealmModel realm) { if (userRep.getRealmRoles() != null) { for (String roleString : userRep.getRealmRoles()) { RoleModel role = realm.getRole(roleString.trim()); if (role == null) { role = realm.addRole(roleString.trim()); + // when running in batch mode queries cannot see non-flushed changes, so flush the newly + // created role to avoid creating it twice when another user references the same role + if (EntityManagers.isBatchMode()) { + EntityManagers.flush(session, false); + } } user.grantRole(role); } @@ -865,12 +892,12 @@ public static void createRoleMappings(UserRepresentation userRep, UserModel user if (client == null) { throw new RuntimeException("Unable to find client role mappings for client: " + entry.getKey()); } - createClientRoleMappings(client, user, entry.getValue()); + createClientRoleMappings(session, client, user, entry.getValue()); } } } - private static void createClientRoleMappings(ClientModel clientModel, UserModel user, List roleNames) { + private static void createClientRoleMappings(KeycloakSession session, ClientModel clientModel, UserModel user, List roleNames) { if (user == null) { throw new RuntimeException("User not found"); } @@ -879,6 +906,11 @@ private static void createClientRoleMappings(ClientModel clientModel, UserModel RoleModel role = clientModel.getRole(roleName.trim()); if (role == null) { role = clientModel.addRole(roleName.trim()); + // when running in batch mode queries cannot see non-flushed changes, so flush the newly + // created role to avoid creating it twice when another user references the same role + if (EntityManagers.isBatchMode()) { + EntityManagers.flush(session, false); + } } user.grantRole(role); diff --git a/server-spi-private/src/test/java/org/keycloak/models/utils/RepresentationToModelTest.java b/server-spi-private/src/test/java/org/keycloak/models/utils/RepresentationToModelTest.java new file mode 100644 index 000000000000..b42410fdfefa --- /dev/null +++ b/server-spi-private/src/test/java/org/keycloak/models/utils/RepresentationToModelTest.java @@ -0,0 +1,121 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.utils; + +import java.lang.reflect.Proxy; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; + +import org.keycloak.models.ClientModel; +import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleModel; +import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.representations.idm.RolesRepresentation; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class RepresentationToModelTest { + + @Test + public void importRolesReusesExistingClientRole() { + Map clientRoles = new HashMap<>(); + AtomicInteger addedRoles = new AtomicInteger(); + + RoleModel existingRole = role("CALL_ACCESS"); + clientRoles.put("CALL_ACCESS", existingRole); + + ClientModel client = client(clientRoles, addedRoles); + RealmModel realm = realm("account", client); + + RolesRepresentation roles = new RolesRepresentation(); + roles.setClient(Collections.singletonMap("account", + Collections.singletonList(new RoleRepresentation("CALL_ACCESS", "imported", false)))); + + RepresentationToModel.importRoles(roles, realm); + + assertEquals(0, addedRoles.get()); + assertEquals("imported", existingRole.getDescription()); + } + + private static RealmModel realm(String clientId, ClientModel client) { + return (RealmModel) Proxy.newProxyInstance(RepresentationToModelTest.class.getClassLoader(), new Class[]{RealmModel.class}, + (proxy, method, args) -> { + if ("getClientByClientId".equals(method.getName()) && clientId.equals(args[0])) { + return client; + } + return defaultValue(method.getReturnType()); + }); + } + + private static ClientModel client(Map roles, AtomicInteger addedRoles) { + return (ClientModel) Proxy.newProxyInstance(RepresentationToModelTest.class.getClassLoader(), new Class[]{ClientModel.class}, + (proxy, method, args) -> { + if ("getRole".equals(method.getName())) { + return roles.get(args[0]); + } + if ("addRole".equals(method.getName())) { + addedRoles.incrementAndGet(); + RoleModel role = role(args.length == 1 ? (String) args[0] : (String) args[1]); + roles.put(role.getName(), role); + return role; + } + return defaultValue(method.getReturnType()); + }); + } + + private static RoleModel role(String name) { + return (RoleModel) Proxy.newProxyInstance(RepresentationToModelTest.class.getClassLoader(), new Class[]{RoleModel.class}, + new Object() { + private String description; + + public Object invoke(Object proxy, java.lang.reflect.Method method, Object[] args) { + if ("getName".equals(method.getName())) { + return name; + } + if ("getDescription".equals(method.getName())) { + return description; + } + if ("setDescription".equals(method.getName())) { + description = (String) args[0]; + return null; + } + return defaultValue(method.getReturnType()); + } + }::invoke); + } + + private static Object defaultValue(Class returnType) { + if (void.class.equals(returnType)) { + return null; + } + if (!returnType.isPrimitive()) { + return null; + } + if (boolean.class.equals(returnType)) { + return false; + } + if (char.class.equals(returnType)) { + return '\0'; + } + return 0; + } +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/exportimport/RealmImportSharedUndefinedClientRoleTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/exportimport/RealmImportSharedUndefinedClientRoleTest.java new file mode 100644 index 000000000000..c26417f00ab1 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/exportimport/RealmImportSharedUndefinedClientRoleTest.java @@ -0,0 +1,87 @@ +/* + * Copyright 2026 Red Hat Inc. and/or its affiliates and other contributors + * as indicated by the @author tags. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package org.keycloak.testsuite.exportimport; + +import java.util.List; + +import org.junit.Test; +import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.testframework.realm.ClientBuilder; +import org.keycloak.testframework.realm.RealmBuilder; +import org.keycloak.testframework.realm.UserBuilder; +import org.keycloak.testsuite.AbstractKeycloakTest; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Reproduces the duplicate KEYCLOAK_ROLE primary key violation reported in issue #49731: when several users + * reference the same client role that is not declared in {@code roles.client}, the role must be auto-created + * only once during the batch-mode user import instead of being created for every user that references it. + */ +public class RealmImportSharedUndefinedClientRoleTest extends AbstractKeycloakTest { + + private static final String REALM_NAME = "shared-undefined-client-role"; + private static final String CLIENT_ID = "shared-role-client"; + private static final String ROLE_NAME = "CALL_ACCESS"; + private static final String USER_1 = "user-one"; + private static final String USER_2 = "user-two"; + + @Override + public void addTestRealms(List testRealms) { + // The realm under test is imported and removed within the test itself. + } + + @Test + public void importRealmWithSharedUndefinedClientRole() { + // The client declares no roles, yet both users reference the same client role. The import must + // auto-create the role exactly once instead of failing with a duplicate primary key violation. + RealmRepresentation realmRep = RealmBuilder.create() + .name(REALM_NAME) + .clients(ClientBuilder.create().clientId(CLIENT_ID).enabled(true)) + .users( + UserBuilder.create().username(USER_1).enabled(true).clientRoles(CLIENT_ID, ROLE_NAME), + UserBuilder.create().username(USER_2).enabled(true).clientRoles(CLIENT_ID, ROLE_NAME)) + .build(); + + try { + // (a) The import must succeed without a duplicate-key / PK violation. + adminClient.realms().create(realmRep); + + RealmResource realm = adminClient.realm(REALM_NAME); + String clientUuid = realm.clients().findByClientId(CLIENT_ID).get(0).getId(); + + // (b) Exactly one such client role exists on the client after import. + long matching = realm.clients().get(clientUuid).roles().list().stream() + .filter(role -> ROLE_NAME.equals(role.getName())) + .count(); + assertEquals("The shared client role must be created exactly once", 1, matching); + + // (c) Both users end up with the shared client-role mapping. + for (String username : List.of(USER_1, USER_2)) { + String userId = realm.users().search(username).get(0).getId(); + List mappings = realm.users().get(userId).roles().clientLevel(clientUuid).listAll(); + assertTrue("User " + username + " must have the shared client role mapped", + mappings.stream().anyMatch(role -> ROLE_NAME.equals(role.getName()))); + } + } finally { + removeRealm(REALM_NAME); + } + } +}