From 9c19a8972bf4139c89bf94ec54978f9b0f647a09 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 11 Oct 2023 11:42:35 -0300 Subject: [PATCH] Removing the default cache metadata Closes #23910 --- .../DeclarativeUserProfileProvider.java | 47 ++++++++++++--- .../profile/CustomUserProfileProvider.java | 8 +-- .../account/AccountRestServiceTest.java | 10 +++- ...AccountRestServiceWithUserProfileTest.java | 1 + .../admin/UserTestWithUserProfile.java | 22 ++++++- .../user/profile/UserProfileTest.java | 57 ++++++++++++++++--- 6 files changed, 123 insertions(+), 22 deletions(-) diff --git a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java index 3d1da0bfe87..0ba11a5eb4d 100644 --- a/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java +++ b/services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java @@ -105,16 +105,17 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< return getRequestedClientScopes(requestedScopesString, client).map((csm) -> csm.getName()).anyMatch(configuredScopes::contains); } - private String defaultRawConfig; - private static final Map DEFAULT_METADATA = Collections.synchronizedMap(new HashMap<>()); + protected String defaultRawConfig; + protected UPConfig parsedDefaultRawConfig; public DeclarativeUserProfileProvider() { - defaultRawConfig = UPConfigUtils.readDefaultConfig(); + // factory create } - public DeclarativeUserProfileProvider(KeycloakSession session, Map metadataRegistry, String defaultRawConfig) { + public DeclarativeUserProfileProvider(KeycloakSession session, Map metadataRegistry, String defaultRawConfig, UPConfig parsedDefaultRawConfig) { super(session, metadataRegistry); this.defaultRawConfig = defaultRawConfig; + this.parsedDefaultRawConfig = parsedDefaultRawConfig; } @Override @@ -124,7 +125,7 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< @Override protected UserProfileProvider create(KeycloakSession session, Map metadataRegistry) { - return new DeclarativeUserProfileProvider(session, metadataRegistry, defaultRawConfig); + return new DeclarativeUserProfileProvider(session, metadataRegistry, defaultRawConfig, parsedDefaultRawConfig); } @Override @@ -141,6 +142,16 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< return new LegacyAttributes(context, attributes, user, metadata, session); } + @Override + protected UserProfileMetadata configureUserProfile(UserProfileMetadata metadata) { + if (isDeclarativeConfigurationEnabled) { + // default metadata for each context is based on the default realm configuration + return decorateUserProfileForCache(metadata, parsedDefaultRawConfig); + } + + return metadata; + } + @Override protected UserProfileMetadata configureUserProfile(UserProfileMetadata metadata, KeycloakSession session) { UserProfileContext context = metadata.getContext(); @@ -161,7 +172,7 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< ComponentModel component = getComponentModel().orElse(null); if (component == null) { - return DEFAULT_METADATA.computeIfAbsent(context, (c) -> decorateUserProfileForCache(decoratedMetadata, getParsedConfig(defaultRawConfig))); + return decoratedMetadata; } Map metadataMap = component.getNote(PARSED_CONFIG_COMPONENT_KEY); @@ -267,8 +278,14 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< @Override public void init(Config.Scope config) { - super.init(config); isDeclarativeConfigurationEnabled = Profile.isFeatureEnabled(Profile.Feature.DECLARATIVE_USER_PROFILE); + defaultRawConfig = UPConfigUtils.readDefaultConfig(); + try { + parsedDefaultRawConfig = parseConfig(defaultRawConfig); + } catch (IOException cause) { + throw new RuntimeException("Failed to parse default user profile configuration", cause); + } + super.init(config); } @Override @@ -535,6 +552,22 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider< throw new RuntimeException("UserProfile configuration for realm '" + session.getContext().getRealm().getName() + "' is invalid: " + errors.toString()); } + for (AttributeMetadata metadata : decoratedMetadata.getAttributes()) { + String attributeName = metadata.getName(); + + if (isBuiltInAttribute(attributeName)) { + UPAttribute upAttribute = parsedDefaultRawConfig.getAttribute(attributeName); + Map> validations = Optional.ofNullable(upAttribute.getValidations()).orElse(Collections.emptyMap()); + + for (String id : validations.keySet()) { + List validators = metadata.getValidators(); + // do not include the default validators for built-in attributes into the base metadata + // user-defined configuration will add its own validators + validators.removeIf(m -> m.getValidatorId().equals(id)); + } + } + } + return decorateUserProfileForCache(decoratedMetadata, parsedConfig); }; } diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/user/profile/CustomUserProfileProvider.java b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/user/profile/CustomUserProfileProvider.java index c8ac96e4449..fd4be419fee 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/user/profile/CustomUserProfileProvider.java +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/java/org/keycloak/testsuite/user/profile/CustomUserProfileProvider.java @@ -7,7 +7,7 @@ import org.keycloak.userprofile.UserProfile; import org.keycloak.userprofile.UserProfileContext; import org.keycloak.userprofile.UserProfileMetadata; import org.keycloak.userprofile.UserProfileProvider; -import org.keycloak.userprofile.config.UPConfigUtils; +import org.keycloak.userprofile.config.UPConfig; import java.util.Map; @@ -20,14 +20,14 @@ public class CustomUserProfileProvider extends DeclarativeUserProfileProvider { } public CustomUserProfileProvider(KeycloakSession session, - Map metadataRegistry, String defaultRawConfig) { - super(session, metadataRegistry, defaultRawConfig); + Map metadataRegistry, String defaultRawConfig, UPConfig parsedDefaultRawConfig) { + super(session, metadataRegistry, defaultRawConfig, parsedDefaultRawConfig); } @Override protected UserProfileProvider create(KeycloakSession session, Map metadataRegistry) { - return new CustomUserProfileProvider(session, metadataRegistry, UPConfigUtils.readDefaultConfig()); + return new CustomUserProfileProvider(session, metadataRegistry, defaultRawConfig, parsedDefaultRawConfig); } @Override diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java index 0d6032b9609..20c80b43d6a 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java @@ -630,7 +630,15 @@ public class AccountRestServiceTest extends AbstractRestServiceTest { protected void updateError(UserRepresentation user, int expectedStatus, String expectedMessage) throws IOException { SimpleHttp.Response response = SimpleHttp.doPost(getAccountUrl(null), httpClient).auth(tokenUtil.getToken()).json(user).asResponse(); assertEquals(expectedStatus, response.getStatus()); - assertEquals(expectedMessage, response.asJson(ErrorRepresentation.class).getErrorMessage()); + ErrorRepresentation errorRep = response.asJson(ErrorRepresentation.class); + List errors = errorRep.getErrors(); + + if (errors == null) { + assertEquals(expectedMessage, errorRep.getErrorMessage()); + } else { + assertThat(errors.stream().map(ErrorRepresentation::getErrorMessage) + .filter(expectedMessage::equals).collect(Collectors.toList()), containsInAnyOrder(expectedMessage)); + } } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java index 7d6813f5d14..f5c0a063f74 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceWithUserProfileTest.java @@ -365,6 +365,7 @@ public class AccountRestServiceWithUserProfileTest extends AccountRestServiceTes @Override public void testUpdateSingleField() throws IOException { setUserProfileConfiguration("{\"attributes\": [" + + "{\"name\": \"email\"," + PERMISSIONS_ALL + "}," + "{\"name\": \"firstName\"," + PERMISSIONS_ALL + "}," + "{\"name\": \"lastName\"," + PERMISSIONS_ALL + ", \"required\": {}}" + "]}"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTestWithUserProfile.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTestWithUserProfile.java index 7341cfd2b77..692a973bf37 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTestWithUserProfile.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTestWithUserProfile.java @@ -20,15 +20,19 @@ package org.keycloak.testsuite.admin; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import java.io.IOException; +import java.util.List; import java.util.Set; -import org.jetbrains.annotations.Nullable; +import jakarta.ws.rs.WebApplicationException; import org.junit.Before; import org.junit.Test; import org.keycloak.common.Profile.Feature; import org.keycloak.models.UserModel; +import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.UserProfileAttributeMetadata; import org.keycloak.representations.idm.UserProfileMetadata; import org.keycloak.representations.idm.RealmRepresentation; @@ -119,6 +123,22 @@ public class UserTestWithUserProfile extends UserTest { return attribute; } + @Test + public void testDefaultCharacterValidationOnUsername() { + List invalidNames = List.of("1user\\\\", "2user\\\\%", "3user\\\\*", "4user\\\\_"); + + for (String invalidName : invalidNames) { + try { + createUser(invalidName, "test@invalid.org"); + fail("Should fail because the username contains invalid characters"); + } catch (WebApplicationException bre) { + assertEquals(400, bre.getResponse().getStatus()); + ErrorRepresentation error = bre.getResponse().readEntity(ErrorRepresentation.class); + assertEquals("username contains invalid character.", error.getErrorMessage()); + } + } + } + @Override protected boolean isDeclarativeUserProfile() { return true; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java index 3abb76dfc33..650df8857ad 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java @@ -121,6 +121,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "profiled-user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); UserProfileProvider provider = getUserProfileProvider(session); @@ -136,8 +139,7 @@ public class UserProfileTest extends AbstractUserProfileTest { assertTrue(ve.isAttributeOnError("address")); } - assertThat(profile.getAttributes().nameSet(), - containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, "address")); + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME, "address"); attributes.put("address", "myaddress"); @@ -157,6 +159,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "profiled-user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); UserProfileProvider provider = getUserProfileProvider(session); @@ -311,6 +316,9 @@ public class UserProfileTest extends AbstractUserProfileTest { assertTrue(ve.isAttributeOnError("address")); } + user.setSingleAttribute(UserModel.FIRST_NAME, "john"); + user.setSingleAttribute(UserModel.LAST_NAME, "doe"); + user.setSingleAttribute(UserModel.EMAIL, "jd@keycloak.org"); user.setAttribute("address", Arrays.asList("fixed-address")); profile = provider.create(UserProfileContext.ACCOUNT, user); @@ -326,6 +334,9 @@ public class UserProfileTest extends AbstractUserProfileTest { private static void testGetProfileAttributes(KeycloakSession session) { RealmModel realm = session.getContext().getRealm(); UserModel user = session.users().addUser(realm, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + user.setFirstName("John"); + user.setLastName("John"); + user.setEmail(org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); UserProfileProvider provider = getUserProfileProvider(session); provider.setConfiguration("{\"attributes\": [{\"name\": \"address\", \"required\": {}, \"permissions\": {\"edit\": [\"user\"]}}]}"); @@ -345,9 +356,9 @@ public class UserProfileTest extends AbstractUserProfileTest { } assertNotNull(attributes.getFirstValue(UserModel.USERNAME)); - assertNull(attributes.getFirstValue(UserModel.EMAIL)); - assertNull(attributes.getFirstValue(UserModel.FIRST_NAME)); - assertNull(attributes.getFirstValue(UserModel.LAST_NAME)); + assertNotNull(attributes.getFirstValue(UserModel.EMAIL)); + assertNotNull(attributes.getFirstValue(UserModel.FIRST_NAME)); + assertNotNull(attributes.getFirstValue(UserModel.LAST_NAME)); assertNull(attributes.getFirstValue("address")); user.setAttribute("address", Arrays.asList("fixed-address")); @@ -508,6 +519,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); attributes.put("address", Arrays.asList("fixed-address")); attributes.put("department", Arrays.asList("sales")); @@ -519,7 +533,7 @@ public class UserProfileTest extends AbstractUserProfileTest { UserModel user = profile.create(); assertThat(profile.getAttributes().nameSet(), - containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.LOCALE, "department")); + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.LOCALE, "department")); assertNull(user.getFirstAttribute("department")); @@ -558,6 +572,8 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); attributes.put(UserModel.EMAIL, "readonly@foo.bar"); UserProfileProvider provider = getUserProfileProvider(session); @@ -569,7 +585,7 @@ public class UserProfileTest extends AbstractUserProfileTest { UserModel user = profile.create(); assertThat(profile.getAttributes().nameSet(), - containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.LOCALE)); + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.LOCALE)); profile = provider.create(UserProfileContext.USER_API, attributes, user); @@ -600,6 +616,8 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); attributes.put(UserModel.EMAIL, "canchange@foo.bar"); UserProfileProvider provider = getUserProfileProvider(session); @@ -611,7 +629,7 @@ public class UserProfileTest extends AbstractUserProfileTest { UserModel user = profile.create(); assertThat(profile.getAttributes().nameSet(), - containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.LOCALE)); + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.LOCALE)); profile = provider.create(UserProfileContext.USER_API, attributes, user); @@ -637,6 +655,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId()); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); attributes.put("address", Arrays.asList("fixed-address")); attributes.put("department", Arrays.asList("sales")); attributes.put("phone", Arrays.asList("fixed-phone")); @@ -649,7 +670,7 @@ public class UserProfileTest extends AbstractUserProfileTest { UserProfile profile = provider.create(UserProfileContext.ACCOUNT, attributes); UserModel user = profile.create(); assertThat(profile.getAttributes().nameSet(), - containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.LOCALE, "address", "department", "phone")); + containsInAnyOrder(UserModel.USERNAME, UserModel.EMAIL, UserModel.FIRST_NAME, UserModel.LAST_NAME, UserModel.LOCALE, "address", "department", "phone")); attributes.put("address", Arrays.asList("change-address")); attributes.put("department", Arrays.asList("changed-sales")); @@ -837,6 +858,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "us"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -934,6 +958,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); // not present attributes are OK UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -997,6 +1024,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -1049,6 +1079,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); // null is OK as attribute is optional UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -1168,6 +1201,9 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); // NO fail on common contexts UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -1218,6 +1254,7 @@ public class UserProfileTest extends AbstractUserProfileTest { attributes.put(UserModel.USERNAME, "user"); attributes.put(UserModel.FIRST_NAME, "user"); attributes.put(UserModel.LAST_NAME, "user"); + attributes.put(UserModel.EMAIL, org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); // NO fail on USER contexts UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes); @@ -1400,6 +1437,8 @@ public class UserProfileTest extends AbstractUserProfileTest { Map attributes = new HashMap<>(); attributes.put(UserModel.USERNAME, "user"); + attributes.put(UserModel.FIRST_NAME, "John"); + attributes.put(UserModel.LAST_NAME, "Doe"); attributes.put(UserModel.EMAIL, "user@email.test"); // client with default scopes for which is attribute NOT configured as required