From e3b2eec1ba43f731e3b218877ae70fcb3d3b5ba8 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 13 Nov 2023 13:29:22 +0100 Subject: [PATCH] Make user profile validation success if the attribute was already wrong and read-only in the context Closes https://github.com/keycloak/keycloak/issues/24697 Signed-off-by: rmartinc --- .../userprofile/DefaultAttributes.java | 13 ++++- .../user/profile/UserProfileTest.java | 53 +++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java index 699f6ca993..878b367cad 100644 --- a/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java +++ b/server-spi-private/src/main/java/org/keycloak/userprofile/DefaultAttributes.java @@ -29,7 +29,8 @@ import java.util.Optional; import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; - +import org.jboss.logging.Logger; +import org.keycloak.common.util.CollectionUtil; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; @@ -51,6 +52,8 @@ import org.keycloak.validate.ValidationError; */ public class DefaultAttributes extends HashMap> implements Attributes { + private static Logger logger = Logger.getLogger(DefaultAttributes.class); + /** * To reference dynamic attributes that can be configured as read-only when setting up the provider. * We should probably remove that once we remove the legacy provider, because this will come from the configuration. @@ -142,6 +145,14 @@ public class DefaultAttributes extends HashMap> implements continue; } + if (user != null && metadata.isReadOnly(attributeContext) + && CollectionUtil.collectionEquals(user.getAttributeStream(name).collect(Collectors.toList()), attribute.getValue())) { + // allow update if the value was already wrong in the user and is read-only in this context + logger.warnf("User '%s' attribute '%s' has previous validation errors %s but is read-only in context %s.", + user.getUsername(), name, vc.getErrors(), attributeContext.getContext()); + continue; + } + if (result == null) { result = false; } 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 72fc0a656d..0e30fe7fb7 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 @@ -30,6 +30,7 @@ import static org.junit.Assert.fail; import static org.keycloak.userprofile.config.UPConfigUtils.ROLE_ADMIN; import static org.keycloak.userprofile.config.UPConfigUtils.ROLE_USER; +import jakarta.ws.rs.core.Response; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -44,6 +45,7 @@ import java.util.function.Consumer; import org.junit.Assert; import org.junit.Test; +import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.component.ComponentModel; import org.keycloak.component.ComponentValidationException; import org.keycloak.models.Constants; @@ -53,6 +55,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RealmRepresentation; +import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.services.messages.Messages; import org.keycloak.testsuite.runonserver.RunOnServer; import org.keycloak.userprofile.AttributeGroupMetadata; @@ -61,6 +64,7 @@ import org.keycloak.representations.userprofile.config.UPAttributePermissions; import org.keycloak.representations.userprofile.config.UPAttributeRequired; import org.keycloak.representations.userprofile.config.UPAttributeSelector; import org.keycloak.representations.userprofile.config.UPConfig; +import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.util.ClientScopeBuilder; import org.keycloak.testsuite.util.KeycloakModelUtils; import org.keycloak.userprofile.Attributes; @@ -98,6 +102,55 @@ public class UserProfileTest extends AbstractUserProfileTest { getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) UserProfileTest::testIdempotentProfile); } + @Test + public void testReadOnlyAllowed() throws Exception { + // create a user with attribute foo value 123 allowed by the profile now but disallowed later + UPConfig config = JsonSerialization.readValue("{\"attributes\": [{\"name\": \"foo\", \"permissions\": {\"edit\": [\"admin\"]}}]}", UPConfig.class); + RealmResource realmRes = testRealm(); + realmRes.users().userProfile().update(config); + + UserRepresentation userRep = new UserRepresentation(); + userRep.setUsername("profiled-user-foo-ro"); + userRep.setFirstName("John"); + userRep.setLastName("Doe"); + userRep.setEmail(org.keycloak.models.utils.KeycloakModelUtils.generateId() + "@keycloak.org"); + userRep.setAttributes(Map.of("foo", Collections.singletonList("123"))); + Response response = realmRes.users().create(userRep); + final String userId = ApiUtil.getCreatedId(response); + userRep = realmRes.users().get(userId).toRepresentation(); + Assert.assertEquals(Collections.singletonList("123"), userRep.getAttributes().get("foo")); + + // it should work if foo is read-only in the context + getTestingClient().server(TEST_REALM_NAME).run(session -> { + RealmModel realm = session.getContext().getRealm(); + UserModel user = session.users().getUserById(realm, userId); + UserProfileProvider provider = getUserProfileProvider(session); + provider.setConfiguration("{\"attributes\": [{\"name\": \"foo\", \"validations\": {\"length\": {\"min\": \"5\", \"max\": \"15\"}}, \"permissions\": {\"edit\": [\"admin\"]}}]}"); + + Map> attributes = new HashMap<>(user.getAttributes()); + UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes, user); + profile.validate(); + }); + + // it should fail if foo can be modified + getTestingClient().server(TEST_REALM_NAME).run(session -> { + RealmModel realm = session.getContext().getRealm(); + UserModel user = session.users().getUserById(realm, userId); + UserProfileProvider provider = getUserProfileProvider(session); + provider.setConfiguration("{\"attributes\": [{\"name\": \"foo\", \"validations\": {\"length\": {\"min\": \"5\", \"max\": \"15\"}}, \"permissions\": {\"edit\": [\"admin\", \"user\"]}}]}"); + + Map> attributes = new HashMap<>(user.getAttributes()); + UserProfile profile = provider.create(UserProfileContext.UPDATE_PROFILE, attributes, user); + try { + profile.validate(); + Assert.fail("Should fail validation on foo minimum length"); + } catch (ValidationException ve) { + assertTrue(ve.isAttributeOnError("foo")); + assertTrue(ve.hasError(LengthValidator.MESSAGE_INVALID_LENGTH)); + } + }); + } + private static void testIdempotentProfile(KeycloakSession session) { Map attributes = new HashMap<>(); UserProfileProvider provider = session.getProvider(UserProfileProvider.class);