Skip to content

Commit

Permalink
Fix Update of Users with Alias during Login (#3014)
Browse files Browse the repository at this point in the history
* Add ScimUserService as central entrypoint for ScimUser update with alias handling

* Move ScimUser update logic from ScimUserEndpoints to ScimUserService

* Add injection of ScimUserService into ScimUserBootstrap

* Reformat

* Add handling of alias users in ScimUserBootstrap

* Fix ScimUserEndpointsAliasMockMvcTests

* Add test for new behavior to ScimUserBootstrapTests

* Add unit test for ScimUserService.update

* Add documentation for alias user handling during logon

* Inject 'aliasEntitiesEnabled into ScimUserBootstrap'

* Handle login of users with alias when alias entities are enabled: only update original user while alias properties remain unchanged

* Update documentation

* Sonar
  • Loading branch information
adrianhoelzl-sap authored Sep 4, 2024
1 parent 0ac2c30 commit 4c05f30
Show file tree
Hide file tree
Showing 12 changed files with 699 additions and 79 deletions.
10 changes: 9 additions & 1 deletion docs/UAA-Alias-Entities.md
Original file line number Diff line number Diff line change
Expand Up @@ -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



Expand Down
Original file line number Diff line number Diff line change
@@ -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.");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand All @@ -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,
Expand All @@ -187,6 +191,7 @@ public ScimUserEndpoints(
this.messageConverters = new HttpMessageConverter[] {
new ExceptionReportHttpMessageConverter()
};
this.scimUserService = scimUserService;
this.aliasHandler = aliasHandler;
this.transactionTemplate = transactionTemplate;
scimUpdates = new AtomicInteger();
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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);
});
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Loading

0 comments on commit 4c05f30

Please sign in to comment.