Do not empty attributes if they are not provided when user profile is enabled

Closes #11096

server-spi-private/src/main/java/org/keycloak/userprofile/AttributeMetadata.java

@@ -175,16 +175,12 @@ public final class AttributeMetadata {
             this.validators = new ArrayList<>();
         }
 
+        this.validators.removeIf(validators::contains);
         this.validators.addAll(validators.stream().filter(Objects::nonNull).collect(Collectors.toList()));
 
         return this;
     }
 
-    public AttributeMetadata addValidator(AttributeValidatorMetadata validator) {
-        addValidator(Arrays.asList(validator));
-        return this;
-    }
-
     public Map<String, Object> getAnnotations() {
         return annotations;
     }

server-spi-private/src/main/java/org/keycloak/userprofile/AttributeValidatorMetadata.java

@@ -20,6 +20,7 @@
 package org.keycloak.userprofile;
 
 import java.util.Map;
+import java.util.Objects;
 
 import org.keycloak.validate.ValidationContext;
 import org.keycloak.validate.Validator;
@@ -79,4 +80,16 @@ public final class AttributeValidatorMetadata {
         return validator.validate(context.getAttribute().getValue(), context.getMetadata().getName(), new UserProfileAttributeValidationContext(context), validatorConfig);
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (o == this) return true;
+        if (! (o instanceof AttributeValidatorMetadata)) return false;
+        AttributeValidatorMetadata other = (AttributeValidatorMetadata) o;
+        return Objects.equals(getValidatorId(), other.getValidatorId());
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(validatorId);
+    }
 }

server-spi-private/src/main/java/org/keycloak/userprofile/DefaultUserProfile.java

@@ -115,6 +115,9 @@ public final class DefaultUserProfile implements UserProfile {
                 List<String> updatedValue = attribute.getValue().stream().filter(Objects::nonNull).collect(Collectors.toList());
 
                 if (!CollectionUtil.collectionEquals(currentValue, updatedValue)) {
+                    if (!removeAttributes && updatedValue.isEmpty()) {
+                        continue;
+                    }
                     user.setAttribute(name, updatedValue);
 
                     if (UserModel.EMAIL.equals(name) && metadata.getContext().isResetEmailVerified()) {

services/src/main/java/org/keycloak/services/resources/admin/UserResource.java

@@ -173,7 +173,17 @@ public class UserResource {
                 wasPermanentlyLockedOut = session.getProvider(BruteForceProtector.class).isPermanentlyLockedOut(session, realm, user);
             }
 
-            UserProfile profile = session.getProvider(UserProfileProvider.class).create(USER_API, rep.toAttributes(), user);
+            Map<String, List<String>> attributes = new HashMap<>(rep.toAttributes());
+
+            if (rep.getAttributes() == null) {
+                // include existing attributes in case no attributes are set so that validation takes into account the existing
+                // attributes associated with the user
+                for (Map.Entry<String, List<String>> entry : user.getAttributes().entrySet()) {
+                    attributes.putIfAbsent(entry.getKey(), entry.getValue());
+                }
+            }
+
+            UserProfile profile = session.getProvider(UserProfileProvider.class).create(USER_API, attributes, user);
 
             Response response = validateUserProfile(profile, user, session);
             if (response != null) {

services/src/main/java/org/keycloak/userprofile/AbstractUserProfileProvider.java

@@ -58,6 +58,7 @@ import org.keycloak.userprofile.validator.UsernameHasValueValidator;
 import org.keycloak.userprofile.validator.UsernameIDNHomographValidator;
 import org.keycloak.userprofile.validator.UsernameMutationValidator;
 import org.keycloak.validate.ValidatorConfig;
+import org.keycloak.validate.validators.EmailValidator;
 
 /**
  * <p>A base class for {@link UserProfileProvider} implementations providing the main hooks for customizations.
@@ -322,7 +323,8 @@ public abstract class AbstractUserProfileProvider<U extends UserProfileProvider>
                 AbstractUserProfileProvider::readEmailCondition,
                 new AttributeValidatorMetadata(BlankAttributeValidator.ID, BlankAttributeValidator.createConfig(Messages.MISSING_EMAIL, false)),
         		new AttributeValidatorMetadata(DuplicateEmailValidator.ID),
-        		new AttributeValidatorMetadata(EmailExistsAsUsernameValidator.ID))
+        		new AttributeValidatorMetadata(EmailExistsAsUsernameValidator.ID),
+                new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build()))
             .setAttributeDisplayName("${email}");
 
         List<AttributeValidatorMetadata> readonlyValidators = new ArrayList<>();

services/src/main/java/org/keycloak/userprofile/DeclarativeUserProfileProvider.java

@@ -139,18 +139,12 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider<
         UserProfileMetadata decoratedMetadata = metadata.clone();
 
         if (!isEnabled(session)) {
-            if(!context.equals(UserProfileContext.USER_API) && !context.equals(UserProfileContext.REGISTRATION_USER_CREATION)) {
+            if(!context.equals(UserProfileContext.USER_API)
+                    && !context.equals(UserProfileContext.REGISTRATION_USER_CREATION)
+                    && !context.equals(UserProfileContext.UPDATE_EMAIL)) {
                 decoratedMetadata.addAttribute(UserModel.FIRST_NAME, 1, new AttributeValidatorMetadata(BlankAttributeValidator.ID, BlankAttributeValidator.createConfig(
                         Messages.MISSING_FIRST_NAME, metadata.getContext() == UserProfileContext.IDP_REVIEW))).setAttributeDisplayName("${firstName}");
                 decoratedMetadata.addAttribute(UserModel.LAST_NAME, 2, new AttributeValidatorMetadata(BlankAttributeValidator.ID, BlankAttributeValidator.createConfig(Messages.MISSING_LAST_NAME, metadata.getContext() == UserProfileContext.IDP_REVIEW))).setAttributeDisplayName("${lastName}");
-                
-                //add email format validator to legacy profile
-                List<AttributeMetadata> em = decoratedMetadata.getAttribute(UserModel.EMAIL);
-                for(AttributeMetadata e: em) {
-                    e.addValidator(new AttributeValidatorMetadata(EmailValidator.ID, ValidatorConfig.builder().config(EmailValidator.IGNORE_EMPTY_VALUE, true).build()));
-                }
-                
-                return decoratedMetadata;
             }
             return decoratedMetadata;
         }
@@ -272,7 +266,11 @@ public class DeclarativeUserProfileProvider extends AbstractUserProfileProvider<
         UPConfig parsedConfig = getParsedConfig(model);
 
         // do not change config for REGISTRATION_USER_CREATION context, everything important is covered thanks to REGISTRATION_PROFILE
-        if (parsedConfig == null || context == UserProfileContext.REGISTRATION_USER_CREATION) {
+        // do not change config for UPDATE_EMAIL context, validations are already set and do not need including anything else from the configuration
+        if (parsedConfig == null
+                || context == UserProfileContext.REGISTRATION_USER_CREATION
+                || context == UserProfileContext.UPDATE_EMAIL
+        ) {
             return decoratedMetadata;
         }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/AppInitiatedActionUpdateEmailTest.java

@@ -45,5 +45,7 @@ public class AppInitiatedActionUpdateEmailTest extends AbstractAppInitiatedActio
 
         UserRepresentation user = ActionUtil.findUserWithAdminClient(adminClient, "test-user@localhost");
         Assert.assertEquals("new@email.com", user.getEmail());
+        Assert.assertEquals("Tom", user.getFirstName());
+        Assert.assertEquals("Brady", user.getLastName());
     }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/AppInitiatedActionUpdateEmailUserProfileTest.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2021 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.testsuite.actions;
+
+import org.keycloak.common.Profile;
+import org.keycloak.representations.idm.RealmRepresentation;
+import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
+import org.keycloak.testsuite.forms.VerifyProfileTest;
+
+@EnableFeature(Profile.Feature.DECLARATIVE_USER_PROFILE)
+public class AppInitiatedActionUpdateEmailUserProfileTest extends AppInitiatedActionUpdateEmailTest {
+
+    @Override
+    public void configureTestRealm(RealmRepresentation testRealm) {
+        super.configureTestRealm(testRealm);
+        VerifyProfileTest.enableDynamicUserProfile(testRealm);
+    }
+}

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateEmailTest.java

@@ -48,6 +48,8 @@ public class RequiredActionUpdateEmailTest extends AbstractRequiredActionUpdateE
         // assert user is really updated in persistent store
         UserRepresentation user = ActionUtil.findUserWithAdminClient(adminClient, "test-user@localhost");
         assertEquals("new-email@localhost", user.getEmail());
+        assertEquals("Tom", user.getFirstName());
+        assertEquals("Brady", user.getLastName());
         assertFalse(user.getRequiredActions().contains(UserModel.RequiredAction.UPDATE_EMAIL.name()));
     }
 }

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionUpdateEmailUserProfileTest.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright 2021 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.testsuite.actions;
+
+import org.keycloak.common.Profile;
+import org.keycloak.representations.idm.RealmRepresentation;
+import org.keycloak.testsuite.arquillian.annotation.EnableFeature;
+import org.keycloak.testsuite.forms.VerifyProfileTest;
+
+@EnableFeature(Profile.Feature.DECLARATIVE_USER_PROFILE)
+public class RequiredActionUpdateEmailUserProfileTest extends RequiredActionUpdateEmailTest {
+
+    @Override
+    public void configureTestRealm(RealmRepresentation testRealm) {
+        super.configureTestRealm(testRealm);
+        VerifyProfileTest.enableDynamicUserProfile(testRealm);
+    }
+}

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/DeclarativeUserTest.java

@@ -3,6 +3,7 @@ package org.keycloak.testsuite.admin;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 import static org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer.REMOTE;
 import static org.keycloak.testsuite.forms.VerifyProfileTest.PERMISSIONS_ALL;
 import static org.keycloak.testsuite.forms.VerifyProfileTest.enableDynamicUserProfile;
@@ -12,6 +13,7 @@ import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 
+import javax.ws.rs.BadRequestException;
 import javax.ws.rs.core.Response;
 
 import java.util.Collections;
@@ -142,6 +144,40 @@ public class DeclarativeUserTest extends AbstractAdminTest {
         userResource.update(user1);
     }
 
+    @Test
+    public void testValidationUsingExistingAttributes() {
+        setUserProfileConfiguration(this.realm, "{\"attributes\": ["
+                + "{\"name\": \"username\", " + PERMISSIONS_ALL + "},"
+                + "{\"name\": \"firstName\", " + PERMISSIONS_ALL + "},"
+                + "{\"name\": \"email\", " + PERMISSIONS_ALL + "},"
+                + "{\"name\": \"lastName\", " + PERMISSIONS_ALL + "},"
+                + "{\"name\": \"attr1\", \"required\": {}, " + PERMISSIONS_ALL + "}]}");
+
+        UserRepresentation user1 = new UserRepresentation();
+        user1.setUsername("user1");
+        // set an attribute to later remove it from the configuration
+        user1.singleAttribute("attr1", "some-value");
+        String user1Id = createUser(user1);
+
+        UserResource userResource = realm.users().get(user1Id);
+        user1 = userResource.toRepresentation();
+        user1.setFirstName("changed");
+        user1.setAttributes(null);
+
+        // do not validate attr1 because the attribute list is not provided and the user has the attribute
+        userResource.update(user1);
+        user1 = userResource.toRepresentation();
+        assertEquals("changed", user1.getFirstName());
+
+        try {
+            user1.setAttributes(Collections.emptyMap());
+            userResource.update(user1);
+            fail("Should fail because the attribute attr1 is required");
+        } catch (BadRequestException ignore) {
+
+        }
+    }
+
     @Test
     public void testDefaultUserProfileProviderIsActive() {
         getTestingClient().server(TEST_REALM_NAME).run(session -> {

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserTest.java

@@ -1430,6 +1430,23 @@ public class UserTest extends AbstractAdminTest {
 
         user1 = realm.users().get(user1Id).toRepresentation();
         assertNull(user1.getAttributes());
+
+        Map<String, List<String>> attributes = new HashMap<>();
+
+        attributes.put("foo", List.of("foo"));
+        attributes.put("bar", List.of("bar"));
+
+        user1.setAttributes(attributes);
+
+        realm.users().get(user1Id).update(user1);
+        user1 = realm.users().get(user1Id).toRepresentation();
+        assertEquals(2, user1.getAttributes().size());
+
+        user1.getAttributes().remove("foo");
+
+        realm.users().get(user1Id).update(user1);
+        user1 = realm.users().get(user1Id).toRepresentation();
+        assertEquals(1, user1.getAttributes().size());
     }
 
     @Test

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/user/profile/UserProfileTest.java

@@ -1418,4 +1418,77 @@ public class UserProfileTest extends AbstractUserProfileTest {
 
         assertEquals("test@keycloak.com", user.getEmail());
     }
+
+    @Test
+    public void testDoNotRemoveAttributes() {
+        getTestingClient().server(TEST_REALM_NAME).run((RunOnServer) UserProfileTest::testDoNotRemoveAttributes);
+    }
+
+    private static void testDoNotRemoveAttributes(KeycloakSession session) {
+        Map<String, Object> attributes = new HashMap<>();
+
+        attributes.put(UserModel.USERNAME, org.keycloak.models.utils.KeycloakModelUtils.generateId());
+        attributes.put(UserModel.EMAIL, Arrays.asList("test@test.com"));
+        attributes.put("test-attribute", Arrays.asList("Test Value"));
+        attributes.put("foo", Arrays.asList("foo"));
+
+        UserProfileProvider provider = getDynamicUserProfileProvider(session);
+
+        provider.setConfiguration("{\"attributes\": ["
+                + "{\"name\": \"test-attribute\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}},"
+                + "{\"name\": \"foo\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}},"
+                + "{\"name\": \"email\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}}]}");
+
+        UserProfile profile = provider.create(UserProfileContext.USER_API, attributes);
+        UserModel user = profile.create();
+
+        attributes.clear();
+        attributes.put(UserModel.EMAIL, Arrays.asList("new-email@test.com"));
+        attributes.put("foo", "changed");
+        profile = provider.create(UserProfileContext.USER_API, attributes, user);
+        profile.update(false);
+        profile = provider.create(UserProfileContext.USER_API, user);
+        Attributes userAttributes = profile.getAttributes();
+
+        assertEquals("new-email@test.com", userAttributes.getFirstValue(UserModel.EMAIL));
+        assertEquals("Test Value", userAttributes.getFirstValue("test-attribute"));
+        assertEquals("changed", userAttributes.getFirstValue("foo"));
+
+        attributes.remove("foo");
+        attributes.put("test-attribute", userAttributes.getFirstValue("test-attribute"));
+        profile = provider.create(UserProfileContext.USER_API, attributes, user);
+        profile.update(true);
+        profile = provider.create(UserProfileContext.USER_API, user);
+        userAttributes = profile.getAttributes();
+        // remove attribute if not set
+        assertEquals("new-email@test.com", userAttributes.getFirstValue(UserModel.EMAIL));
+        assertEquals("Test Value", userAttributes.getFirstValue("test-attribute"));
+        assertNull(userAttributes.getFirstValue("foo"));
+
+        provider.setConfiguration("{\"attributes\": ["
+                + "{\"name\": \"test-attribute\", \"permissions\": {\"edit\": [\"user\"]}},"
+                + "{\"name\": \"foo\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}},"
+                + "{\"name\": \"email\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}}]}");
+        attributes.remove("test-attribute");
+        profile = provider.create(UserProfileContext.USER_API, attributes, user);
+        profile.update(true);
+        profile = provider.create(UserProfileContext.USER_API, user);
+        userAttributes = profile.getAttributes();
+        // do not remove test-attribute because admin does not have write permissions
+        assertEquals("new-email@test.com", userAttributes.getFirstValue(UserModel.EMAIL));
+        assertEquals("Test Value", userAttributes.getFirstValue("test-attribute"));
+
+        provider.setConfiguration("{\"attributes\": ["
+                + "{\"name\": \"test-attribute\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}},"
+                + "{\"name\": \"foo\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}},"
+                + "{\"name\": \"email\", \"permissions\": {\"edit\": [\"admin\", \"user\"]}}]}");
+        attributes.remove("test-attribute");
+        profile = provider.create(UserProfileContext.USER_API, attributes, user);
+        profile.update(true);
+        profile = provider.create(UserProfileContext.USER_API, user);
+        userAttributes = profile.getAttributes();
+        // removes the test-attribute attribute because now admin has write permission
+        assertEquals("new-email@test.com", userAttributes.getFirstValue(UserModel.EMAIL));
+        assertNull(userAttributes.getFirstValue("test-attribute"));
+    }
 }