Fix Update of Users with Alias during Login

docs/UAA-Alias-Entities.md

@@ -115,9 +115,17 @@ Please note that disabling the flag does not lead to existing entities with alia
 In addition to enabling the alias feature, one must ensure that no groups can be created that would give users inside a 
 custom zone any authorizations in other zones (e.g., `zones.<zone ID>.admin`).
 This can be achieved by using the allow list for groups (`userConfig.allowedGroups`) in the configuration of the 
-identity zone. 
+identity zone.
 
+## User Logon
 
+During logon, the information of the matching shadow user is updated with the information from the identity provider 
+(e.g., the ID token in the OpenID Connect flow).
+
+If this shadow user has an alias, ...
+- *alias entities enabled:* the updated properties are propagated to the alias.
+- *alias entities disabled:* only the user itself is updated, the alias user is left unchanged.
+  - the alias properties are not changed - original and alias user still reference each other
 
 
 

server/src/main/java/org/cloudfoundry/identity/uaa/alias/AliasPropertiesInvalidException.java

@@ -0,0 +1,7 @@
+package org.cloudfoundry.identity.uaa.alias;
+
+public class AliasPropertiesInvalidException extends RuntimeException {
+    public AliasPropertiesInvalidException() {
+        super("The fields 'aliasId' and/or 'aliasZid' are invalid.");
+    }
+}

server/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUserAliasHandler.java

@@ -23,7 +23,7 @@ public class ScimUserAliasHandler extends EntityAliasHandler<ScimUser> {
     private final IdentityProviderProvisioning identityProviderProvisioning;
     private final IdentityZoneManager identityZoneManager;
 
-    protected ScimUserAliasHandler(
+    public ScimUserAliasHandler(
             @Qualifier("identityZoneProvisioning") final IdentityZoneProvisioning identityZoneProvisioning,
             final ScimUserProvisioning scimUserProvisioning,
             final IdentityProviderProvisioning identityProviderProvisioning,

server/src/main/java/org/cloudfoundry/identity/uaa/scim/bootstrap/ScimUserBootstrap.java

@@ -12,11 +12,13 @@
 import org.cloudfoundry.identity.uaa.scim.exception.MemberAlreadyExistsException;
 import org.cloudfoundry.identity.uaa.scim.exception.MemberNotFoundException;
 import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceNotFoundException;
+import org.cloudfoundry.identity.uaa.scim.services.ScimUserService;
 import org.cloudfoundry.identity.uaa.user.UaaUser;
 import org.cloudfoundry.identity.uaa.zone.IdentityZoneHolder;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.InitializingBean;
+import org.springframework.beans.factory.annotation.Qualifier;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.context.ApplicationEvent;
 import org.springframework.context.ApplicationEventPublisher;
@@ -49,30 +51,38 @@ public class ScimUserBootstrap implements
     private static final Logger logger = LoggerFactory.getLogger(ScimUserBootstrap.class);
 
     private final ScimUserProvisioning scimUserProvisioning;
+    private final ScimUserService scimUserService;
     private final ScimGroupProvisioning scimGroupProvisioning;
     private final ScimGroupMembershipManager membershipManager;
     private final Collection<UaaUser> users;
     private final boolean override;
     private final List<String> usersToDelete;
+    private final boolean aliasEntitiesEnabled;
     private ApplicationEventPublisher publisher;
 
     /**
      *
      * @param users Users to create
      * @param override Flag to indicate that user accounts can be updated as well as created
      */
-    public ScimUserBootstrap(final ScimUserProvisioning scimUserProvisioning,
-                             final ScimGroupProvisioning scimGroupProvisioning,
-                             final ScimGroupMembershipManager membershipManager,
-                             final Collection<UaaUser> users,
-                             @Value("${scim.user.override:false}") final boolean override,
-                             @Value("${delete.users:#{null}}") final List<String> usersToDelete) {
+    public ScimUserBootstrap(
+            final ScimUserProvisioning scimUserProvisioning,
+            final ScimUserService scimUserService,
+            final ScimGroupProvisioning scimGroupProvisioning,
+            final ScimGroupMembershipManager membershipManager,
+            final Collection<UaaUser> users,
+            @Value("${scim.user.override:false}") final boolean override,
+            @Value("${delete.users:#{null}}") final List<String> usersToDelete,
+            @Qualifier("aliasEntitiesEnabled") final boolean aliasEntitiesEnabled
+    ) {
         this.scimUserProvisioning = scimUserProvisioning;
+        this.scimUserService = scimUserService;
         this.scimGroupProvisioning = scimGroupProvisioning;
         this.membershipManager = membershipManager;
         this.users = Collections.unmodifiableCollection(users);
         this.override = override;
         this.usersToDelete = usersToDelete;
+        this.aliasEntitiesEnabled = aliasEntitiesEnabled;
     }
 
     @Override
@@ -160,7 +170,23 @@ private void updateUser(ScimUser existingUser, UaaUser updatedUser, boolean upda
 
         final ScimUser newScimUser = convertToScimUser(updatedUser);
         newScimUser.setVersion(existingUser.getVersion());
-        scimUserProvisioning.update(id, newScimUser, IdentityZoneHolder.get().getId());
+        newScimUser.setZoneId(existingUser.getZoneId());
+
+        /* the user in the event won't have the alias properties set, we must therefore propagate them from the existing
+         * user, if present */
+        if (hasText(existingUser.getAliasId()) && hasText(existingUser.getAliasZid())) {
+            newScimUser.setAliasId(existingUser.getAliasId());
+            newScimUser.setAliasZid(existingUser.getAliasZid());
+        }
+
+        if (aliasEntitiesEnabled) {
+            // update the user and propagate the changes to the alias, if present
+            scimUserService.updateUser(id, newScimUser);
+        } else {
+            // update only the original user, even if it has an alias (the alias properties remain unchanged)
+            scimUserProvisioning.update(id, newScimUser, IdentityZoneHolder.get().getId());
+        }
+
         if (OriginKeys.UAA.equals(newScimUser.getOrigin()) && hasText(updatedUser.getPassword())) { //password is not relevant for non UAA users
             scimUserProvisioning.changePassword(id, null, updatedUser.getPassword(), IdentityZoneHolder.get().getId());
         }

server/src/main/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpoints.java

@@ -3,6 +3,7 @@
 import com.jayway.jsonpath.JsonPathException;
 import org.cloudfoundry.identity.uaa.account.UserAccountStatus;
 import org.cloudfoundry.identity.uaa.account.event.UserAccountUnlockedEvent;
+import org.cloudfoundry.identity.uaa.alias.AliasPropertiesInvalidException;
 import org.cloudfoundry.identity.uaa.alias.EntityAliasFailedException;
 import org.cloudfoundry.identity.uaa.approval.Approval;
 import org.cloudfoundry.identity.uaa.approval.ApprovalStore;
@@ -33,6 +34,7 @@
 import org.cloudfoundry.identity.uaa.scim.exception.ScimException;
 import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceConflictException;
 import org.cloudfoundry.identity.uaa.scim.exception.UserAlreadyVerifiedException;
+import org.cloudfoundry.identity.uaa.scim.services.ScimUserService;
 import org.cloudfoundry.identity.uaa.scim.util.ScimUtils;
 import org.cloudfoundry.identity.uaa.scim.validate.PasswordValidator;
 import org.cloudfoundry.identity.uaa.security.IsSelfCheck;
@@ -142,6 +144,7 @@ public class ScimUserEndpoints implements InitializingBean, ApplicationEventPubl
      */
     private final AtomicInteger scimDeletes;
     private final Map<String, AtomicInteger> errorCounts;
+    private final ScimUserService scimUserService;
     private final ScimUserAliasHandler aliasHandler;
     private final TransactionTemplate transactionTemplate;
 
@@ -161,6 +164,7 @@ public ScimUserEndpoints(
             final ExpiringCodeStore codeStore,
             final ApprovalStore approvalStore,
             final ScimGroupMembershipManager membershipManager,
+            final ScimUserService scimUserService,
             final ScimUserAliasHandler aliasHandler,
             final TransactionTemplate transactionTemplate,
             final @Qualifier("aliasEntitiesEnabled") boolean aliasEntitiesEnabled,
@@ -187,6 +191,7 @@ public ScimUserEndpoints(
         this.messageConverters = new HttpMessageConverter[] {
                 new ExceptionReportHttpMessageConverter()
         };
+        this.scimUserService = scimUserService;
         this.aliasHandler = aliasHandler;
         this.transactionTemplate = transactionTemplate;
         scimUpdates = new AtomicInteger();
@@ -319,23 +324,11 @@ public ScimUser updateUser(@RequestBody ScimUser user, @PathVariable String user
 
         user.setZoneId(identityZoneManager.getCurrentIdentityZoneId());
 
-        final ScimUser existingScimUser = scimUserProvisioning.retrieve(
-                userId,
-                identityZoneManager.getCurrentIdentityZoneId()
-        );
-        if (!aliasHandler.aliasPropertiesAreValid(user, existingScimUser)) {
-            throw new ScimException("The fields 'aliasId' and/or 'aliasZid' are invalid.", HttpStatus.BAD_REQUEST);
-        }
-
         final ScimUser scimUser;
         try {
-            if (aliasEntitiesEnabled) {
-                // update user and create/update alias, if necessary
-                scimUser = updateUserWithAliasHandling(userId, user, existingScimUser);
-            } else {
-                // update user without alias handling
-                scimUser = scimUserProvisioning.update(userId, user, identityZoneManager.getCurrentIdentityZoneId());
-            }
+            scimUser = scimUserService.updateUser(userId, user);
+        } catch (final AliasPropertiesInvalidException e) {
+            throw new ScimException("The fields 'aliasId' and/or 'aliasZid' are invalid.", HttpStatus.BAD_REQUEST);
         } catch (final OptimisticLockingFailureException e) {
             throw new ScimResourceConflictException(e.getMessage());
         } catch (final EntityAliasFailedException e) {
@@ -348,20 +341,6 @@ public ScimUser updateUser(@RequestBody ScimUser user, @PathVariable String user
         return scimUserWithApprovalsAndGroups;
     }
 
-    private ScimUser updateUserWithAliasHandling(final String userId, final ScimUser user, final ScimUser existingUser) {
-        return transactionTemplate.execute(txStatus -> {
-            final ScimUser updatedOriginalUser = scimUserProvisioning.update(
-                    userId,
-                    user,
-                    identityZoneManager.getCurrentIdentityZoneId()
-            );
-            return aliasHandler.ensureConsistencyOfAliasEntity(
-                    updatedOriginalUser,
-                    existingUser
-            );
-        });
-    }
-
     @RequestMapping(value = "/Users/{userId}", method = RequestMethod.PATCH)
     @ResponseBody
     public ScimUser patchUser(@RequestBody ScimUser patch, @PathVariable String userId,

server/src/main/java/org/cloudfoundry/identity/uaa/scim/services/ScimUserService.java

@@ -0,0 +1,71 @@
+package org.cloudfoundry.identity.uaa.scim.services;
+
+import org.cloudfoundry.identity.uaa.alias.AliasPropertiesInvalidException;
+import org.cloudfoundry.identity.uaa.alias.EntityAliasFailedException;
+import org.cloudfoundry.identity.uaa.scim.ScimUser;
+import org.cloudfoundry.identity.uaa.scim.ScimUserAliasHandler;
+import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
+import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
+import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.dao.OptimisticLockingFailureException;
+import org.springframework.stereotype.Component;
+import org.springframework.transaction.support.TransactionTemplate;
+
+@Component
+public class ScimUserService {
+
+    private final ScimUserAliasHandler aliasHandler;
+    private final ScimUserProvisioning scimUserProvisioning;
+    private final IdentityZoneManager identityZoneManager;
+    private final TransactionTemplate transactionTemplate;
+    private final boolean aliasEntitiesEnabled;
+
+    public ScimUserService(
+            final ScimUserAliasHandler aliasHandler,
+            final ScimUserProvisioning scimUserProvisioning,
+            final IdentityZoneManager identityZoneManager,
+            final TransactionTemplate transactionTemplate,
+            @Qualifier("aliasEntitiesEnabled") final boolean aliasEntitiesEnabled
+    ) {
+        this.aliasHandler = aliasHandler;
+        this.scimUserProvisioning = scimUserProvisioning;
+        this.identityZoneManager = identityZoneManager;
+        this.transactionTemplate = transactionTemplate;
+        this.aliasEntitiesEnabled = aliasEntitiesEnabled;
+    }
+
+    public ScimUser updateUser(final String userId, final ScimUser user)
+            throws AliasPropertiesInvalidException, OptimisticLockingFailureException, EntityAliasFailedException {
+        final ScimUser existingScimUser = scimUserProvisioning.retrieve(
+                userId,
+                identityZoneManager.getCurrentIdentityZoneId()
+        );
+        if (!aliasHandler.aliasPropertiesAreValid(user, existingScimUser)) {
+            throw new AliasPropertiesInvalidException();
+        }
+
+        if (!aliasEntitiesEnabled) {
+            // update user without alias handling
+            return scimUserProvisioning.update(userId, user, identityZoneManager.getCurrentIdentityZoneId());
+        }
+
+        // update user and create/update alias, if necessary
+        return updateUserWithAliasHandling(userId, user, existingScimUser);
+    }
+
+    private ScimUser updateUserWithAliasHandling(
+            final String userId,
+            final ScimUser user,
+            final ScimUser existingUser
+    ) {
+        return transactionTemplate.execute(txStatus -> {
+            final ScimUser updatedOriginalUser = scimUserProvisioning.update(
+                    userId,
+                    user,
+                    identityZoneManager.getCurrentIdentityZoneId()
+            );
+            return aliasHandler.ensureConsistencyOfAliasEntity(updatedOriginalUser, existingUser);
+        });
+    }
+
+}

server/src/test/java/org/cloudfoundry/identity/uaa/provider/saml/LoginSamlAuthenticationProviderTests.java

@@ -16,12 +16,14 @@
 import org.cloudfoundry.identity.uaa.scim.ScimGroup;
 import org.cloudfoundry.identity.uaa.scim.ScimGroupProvisioning;
 import org.cloudfoundry.identity.uaa.scim.ScimUser;
+import org.cloudfoundry.identity.uaa.scim.ScimUserAliasHandler;
 import org.cloudfoundry.identity.uaa.scim.ScimUserProvisioning;
 import org.cloudfoundry.identity.uaa.scim.bootstrap.ScimUserBootstrap;
 import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimGroupExternalMembershipManager;
 import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimGroupMembershipManager;
 import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimGroupProvisioning;
 import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimUserProvisioning;
+import org.cloudfoundry.identity.uaa.scim.services.ScimUserService;
 import org.cloudfoundry.identity.uaa.test.TestUtils;
 import org.cloudfoundry.identity.uaa.user.JdbcUaaUserDatabase;
 import org.cloudfoundry.identity.uaa.user.UaaAuthority;
@@ -200,7 +202,26 @@ void configureProvider() throws SAMLException, SecurityException, DecryptionExce
         JdbcScimGroupMembershipManager membershipManager = new JdbcScimGroupMembershipManager(
                 jdbcTemplate, new TimeServiceImpl(), userProvisioning, null, dbUtils);
         membershipManager.setScimGroupProvisioning(groupProvisioning);
-        ScimUserBootstrap bootstrap = new ScimUserBootstrap(userProvisioning, groupProvisioning, membershipManager, Collections.emptyList(), false, Collections.emptyList());
+
+        final ScimUserAliasHandler aliasHandler = mock(ScimUserAliasHandler.class);
+        when(aliasHandler.aliasPropertiesAreValid(any(), any())).thenReturn(true);
+
+        ScimUserBootstrap bootstrap = new ScimUserBootstrap(
+                userProvisioning,
+                new ScimUserService(
+                        aliasHandler,
+                        userProvisioning,
+                        identityZoneManager,
+                        null, // not required since alias is disabled
+                        false
+                ),
+                groupProvisioning,
+                membershipManager,
+                Collections.emptyList(),
+                false,
+                Collections.emptyList(),
+                false
+        );
 
         externalManager = new JdbcScimGroupExternalMembershipManager(jdbcTemplate, dbUtils);
         externalManager.setScimGroupProvisioning(groupProvisioning);