Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alias Handler for SCIM Users #2769

Merged
merged 31 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
b60ec98
Merge branch 'feature/add-alias-id-and-alias-zid-fields-to-scimuser' …
adrianhoelzl-sap Mar 7, 2024
3b14d18
Add alias properties to JSON deserialization of ScimUser class
adrianhoelzl-sap Mar 7, 2024
2b3ba5e
Add alias handler for SCIM users
adrianhoelzl-sap Mar 7, 2024
36d441e
Merge branch 'develop' into feature/alias-handler-for-scim-users
adrianhoelzl-sap Apr 3, 2024
75bbacb
Merge branch 'feature/prevent-update-and-delete-of-entities-with-alia…
adrianhoelzl-sap Apr 3, 2024
aa22118
Refactor
adrianhoelzl-sap Apr 3, 2024
4009ed3
Merge branch 'feature/prevent-update-and-delete-of-entities-with-alia…
adrianhoelzl-sap Apr 4, 2024
800433e
Add @NonNull annotations to EntityAliasHandler.isValidAliasPair param…
adrianhoelzl-sap Apr 4, 2024
ced3026
Add null check to IdPs in ScimUserAliasHandler.additionalValidationCh…
adrianhoelzl-sap Apr 4, 2024
688a513
Add tests for validation logic for ScimUsers
adrianhoelzl-sap Apr 4, 2024
bd6a79a
Merge branch 'develop' into feature/alias-handler-for-scim-users
adrianhoelzl-sap Apr 9, 2024
1a3668d
Adjust ScimUser properties set in ScimUserAliasHandler.cloneEntity
adrianhoelzl-sap Apr 10, 2024
28e206a
Revert adding ScimUserProvisioning.retrievePasswordForUser method
adrianhoelzl-sap Apr 10, 2024
091fbf2
Revert adding alias properties to ScimUserJsonDeserializer.deserialize
adrianhoelzl-sap Apr 10, 2024
79cd7c7
Introduce superclass for tests for EntityAliasHandler.ensureConsisten…
adrianhoelzl-sap Apr 10, 2024
ef5f4dc
Refactor
adrianhoelzl-sap Apr 10, 2024
37eedad
Refactor
adrianhoelzl-sap Apr 10, 2024
ff6079a
Move IdpWithAliasMatcher to superclass and make it generic
adrianhoelzl-sap Apr 10, 2024
b4fb274
Refactor
adrianhoelzl-sap Apr 10, 2024
463202f
Add method 'entitiesAreEqual' to later on support equality checks for…
adrianhoelzl-sap Apr 10, 2024
9ed1580
Add ScimUserAliasHandlerEnsureConsistencyTest
adrianhoelzl-sap Apr 10, 2024
96af351
Change update behavior for SCIM user aliases: no longer propagate tim…
adrianhoelzl-sap Apr 11, 2024
968a88e
Merge branch 'develop' into feature/alias-handler-for-scim-users
adrianhoelzl-sap Apr 17, 2024
e78e1fd
Add unit test: creation of alias user fails as username already occup…
adrianhoelzl-sap May 8, 2024
5699f43
Remove setting properties of new alias explicitly to null in ScimAlia…
adrianhoelzl-sap Jun 3, 2024
0cf1ba7
Refactor
adrianhoelzl-sap Jun 3, 2024
97d13b3
Add unit test for SCIM alias validation: no existing alias, alias fea…
adrianhoelzl-sap Jun 3, 2024
4321693
Add unit test for SCIM alias validation: no existing alias, alias fea…
adrianhoelzl-sap Jun 3, 2024
31ebf13
Add missing empty line
adrianhoelzl-sap Jun 3, 2024
823fc2f
Introduce bean for aliasEntitiesEnabled configuration property
adrianhoelzl-sap Jun 3, 2024
f451e8e
Remove unused imports
adrianhoelzl-sap Jun 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.cloudfoundry.identity.uaa.constants.OriginKeys.UAA;
import static org.springframework.util.StringUtils.hasText;

import java.util.Objects;
import java.util.Optional;

import org.cloudfoundry.identity.uaa.EntityWithAlias;
Expand Down Expand Up @@ -154,6 +155,7 @@ public final T ensureConsistencyOfAliasEntity(
// update the existing alias entity
if (existingAliasEntity != null) {
setId(aliasEntity, existingAliasEntity.getId());
setPropertiesFromExistingAliasEntity(aliasEntity, existingAliasEntity);
updateEntity(aliasEntity, originalEntity.getAliasZid());
return originalEntity;
}
Expand All @@ -177,6 +179,12 @@ public final T ensureConsistencyOfAliasEntity(
return updateEntity(originalEntity, originalEntity.getZoneId());
}

/**
* Set properties from the existing alias entity in the new alias entity before it is updated. Can be used if
* certain properties should differ between the original and the alias entity.
*/
protected abstract void setPropertiesFromExistingAliasEntity(final T newAliasEntity, final T existingAliasEntity);

private T buildAliasEntity(final T originalEntity) {
final T aliasEntity = cloneEntity(originalEntity);
aliasEntity.setAliasId(originalEntity.getId());
Expand Down Expand Up @@ -208,4 +216,23 @@ public final Optional<T> retrieveAliasEntity(final T originalEntity) {
protected abstract T updateEntity(final T entity, final String zoneId);

protected abstract T createEntity(final T entity, final String zoneId) throws EntityAliasFailedException;

protected static <T extends EntityWithAlias> boolean isValidAliasPair(
@NonNull final T entity1,
@NonNull final T entity2
) {
// check if both entities have an alias
final boolean entity1HasAlias = hasText(entity1.getAliasId()) && hasText(entity1.getAliasZid());
final boolean entity2HasAlias = hasText(entity2.getAliasId()) && hasText(entity2.getAliasZid());
if (!entity1HasAlias || !entity2HasAlias) {
return false;
strehle marked this conversation as resolved.
Show resolved Hide resolved
}

// check if they reference each other
final boolean entity1ReferencesEntity2 = Objects.equals(entity1.getAliasId(), entity2.getId()) &&
Objects.equals(entity1.getAliasZid(), entity2.getZoneId());
final boolean entity2ReferencesEntity1 = Objects.equals(entity2.getAliasId(), entity1.getId()) &&
Objects.equals(entity2.getAliasZid(), entity1.getZoneId());
return entity1ReferencesEntity2 && entity2ReferencesEntity1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ protected boolean additionalValidationChecksForNewAlias(@NonNull final IdentityP
return IDP_TYPES_ALIAS_SUPPORTED.contains(requestBody.getType());
}

@Override
protected void setPropertiesFromExistingAliasEntity(
final IdentityProvider<?> newAliasEntity,
final IdentityProvider<?> existingAliasEntity
) {
// not required for identity providers
}

@Override
protected void setId(final IdentityProvider<?> entity, final String id) {
entity.setId(id);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
package org.cloudfoundry.identity.uaa.scim;

import static org.cloudfoundry.identity.uaa.util.UaaStringUtils.EMPTY_STRING;

import java.util.Optional;

import org.cloudfoundry.identity.uaa.alias.EntityAliasFailedException;
import org.cloudfoundry.identity.uaa.alias.EntityAliasHandler;
import org.cloudfoundry.identity.uaa.provider.IdentityProvider;
import org.cloudfoundry.identity.uaa.provider.IdentityProviderProvisioning;
import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceAlreadyExistsException;
import org.cloudfoundry.identity.uaa.scim.exception.ScimResourceNotFoundException;
import org.cloudfoundry.identity.uaa.zone.IdentityZoneProvisioning;
import org.cloudfoundry.identity.uaa.zone.beans.IdentityZoneManager;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Component;

@Component
public class ScimUserAliasHandler extends EntityAliasHandler<ScimUser> {
private final ScimUserProvisioning scimUserProvisioning;
private final IdentityProviderProvisioning identityProviderProvisioning;
private final IdentityZoneManager identityZoneManager;

protected ScimUserAliasHandler(
@Qualifier("identityZoneProvisioning") final IdentityZoneProvisioning identityZoneProvisioning,
final ScimUserProvisioning scimUserProvisioning,
final IdentityProviderProvisioning identityProviderProvisioning,
final IdentityZoneManager identityZoneManager,
@Value("${login.aliasEntitiesEnabled:false}") final boolean aliasEntitiesEnabled
strehle marked this conversation as resolved.
Show resolved Hide resolved
) {
super(identityZoneProvisioning, aliasEntitiesEnabled);
this.scimUserProvisioning = scimUserProvisioning;
this.identityProviderProvisioning = identityProviderProvisioning;
this.identityZoneManager = identityZoneManager;
}

@Override
protected boolean additionalValidationChecksForNewAlias(final ScimUser requestBody) {
/* check if an IdP with the user's origin exists in both the current and the alias zone and that they are
* aliases of each other */
final String origin = requestBody.getOrigin();
final Optional<IdentityProvider<?>> idpInAliasZone = retrieveIdpByOrigin(origin, requestBody.getAliasZid());
if (idpInAliasZone.isEmpty()) {
return false;
}
final Optional<IdentityProvider<?>> idpInCurrentZone = retrieveIdpByOrigin(origin, identityZoneManager.getCurrentIdentityZoneId());
if (idpInCurrentZone.isEmpty()) {
return false;
strehle marked this conversation as resolved.
Show resolved Hide resolved
}
return EntityAliasHandler.isValidAliasPair(idpInCurrentZone.get(), idpInAliasZone.get());
}

@Override
protected void setPropertiesFromExistingAliasEntity(
final ScimUser newAliasEntity,
final ScimUser existingAliasEntity
) {
// these three timestamps should not be overwritten by the timestamps of the original user
newAliasEntity.setPasswordLastModified(existingAliasEntity.getPasswordLastModified());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be setPasswordLastModifiedTime to align to the naming of the two following Time setters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setters are already named consistently to the underlying fields in the class as well as its JSON representation (passwordLastModified does not end with "time", lastLogonTime and previousLogonTime do), see for example here:

} else if ("passwordLastModified".equalsIgnoreCase(fieldName)) {

I would therefore suggest to leave them as they are. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the naming inconsistency is more widespread, maybe this would be a good clean up following this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the naming inconsistency is more widespread, maybe this would be a good clean up following this PR.

newAliasEntity.setLastLogonTime(existingAliasEntity.getLastLogonTime());
newAliasEntity.setPreviousLogonTime(existingAliasEntity.getPreviousLogonTime());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these setting the Time to the existingAliasEntity instead of "now" or even null for the 2 logon times - a new Alias should have it's own timestamps. Setting them to the existingAlias seems it would be confusing when it was actually Modified or logged into.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used for managing the fields that should differ between the "original" user and its alias, i.e., the properties of an alias that should be independent from the original user.

During updates, we build a copy of the original user and leave these three timestamps empty. After that, we call this method to overwrite the timestamps with the values from the version of the alias prior to the update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one is it,

"the properties of an alias that should be independent from the original user."

which I agree with
But this seems contrary to keeping them independent:

"After that, we call this method to overwrite the timestamps with the values from the version of the alias prior to the update."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion in my explanation, I'll try again:

Let's say there is a user U in the "uaa" zone, which has an alias user A in the zone "custom". Whenever we perform an update on U, we persist the changes, which leads to a newer version U' with the changed properties.

Then, to propagate the changes also to the alias, we build a new clone of U', i.e., A'. This is done here:

protected ScimUser cloneEntity(final ScimUser originalEntity) {

However, as you correctly addressed, A' would now have the same timestamp values (last logon, password last modified and previous logon) as U', which is incorrect. Therefore, before persisting A', we look up the version of A' before the update, i.e., A, and overwrite the timestamp values of A' with the timestamp values of A.

This is done in the method of this GitHub conversation. The parameter newAliasEntity corresponds to A', while existingAliasEntity corresponds to A (and not to U or U', as you might have thought when asking the question).


private Optional<IdentityProvider<?>> retrieveIdpByOrigin(final String originKey, final String zoneId) {
final IdentityProvider<?> idpInAliasZone;
try {
idpInAliasZone = identityProviderProvisioning.retrieveByOrigin(originKey, zoneId);
} catch (final EmptyResultDataAccessException e) {
return Optional.empty();
}
return Optional.ofNullable(idpInAliasZone);
}

@Override
protected void setId(final ScimUser entity, final String id) {
entity.setId(id);
}

@Override
protected void setZoneId(final ScimUser entity, final String zoneId) {
entity.setZoneId(zoneId);
}

@Override
protected ScimUser cloneEntity(final ScimUser originalEntity) {
final ScimUser aliasUser = new ScimUser();

aliasUser.setId(null);
aliasUser.setExternalId(originalEntity.getExternalId());

/* we only allow alias users to be created if their origin IdP has an alias to the same zone, therefore, an IdP
* with the same origin key will exist in the alias zone */
aliasUser.setOrigin(originalEntity.getOrigin());

aliasUser.setUserName(originalEntity.getUserName());
aliasUser.setName(new ScimUser.Name(originalEntity.getGivenName(), originalEntity.getFamilyName()));

aliasUser.setEmails(originalEntity.getEmails());
aliasUser.setPhoneNumbers(originalEntity.getPhoneNumbers());

aliasUser.setActive(originalEntity.isActive());
aliasUser.setVerified(originalEntity.isVerified());

// idzId and alias properties will be set later
aliasUser.setZoneId(null);
strehle marked this conversation as resolved.
Show resolved Hide resolved
aliasUser.setAliasId(null);
aliasUser.setAliasZid(null);

/* these timestamps will be overwritten:
* - creation: with current timestamp during persistence (JdbcScimUserProvisioning)
* - update: with values from existing alias entity */
aliasUser.setPasswordLastModified(null);
aliasUser.setLastLogonTime(null);
aliasUser.setPreviousLogonTime(null);
strehle marked this conversation as resolved.
Show resolved Hide resolved

/* password: empty string
* - alias users are only allowed for IdPs that also have an alias
* - IdPs can only have an alias if they are of type SAML, OIDC or OAuth 2.0
* - users with such IdPs as their origin always have an empty password
*/
aliasUser.setPassword(EMPTY_STRING);
strehle marked this conversation as resolved.
Show resolved Hide resolved
aliasUser.setSalt(null);

return aliasUser;
}

@Override
protected Optional<ScimUser> retrieveEntity(final String id, final String zoneId) {
final ScimUser user;
try {
user = scimUserProvisioning.retrieve(id, zoneId);
} catch (final ScimResourceNotFoundException e) {
return Optional.empty();
}
return Optional.ofNullable(user);
}

@Override
protected ScimUser updateEntity(final ScimUser entity, final String zoneId) {
return scimUserProvisioning.update(entity.getId(), entity, zoneId);
}

@Override
protected ScimUser createEntity(final ScimUser entity, final String zoneId) {
try {
return scimUserProvisioning.createUser(entity, entity.getPassword(), zoneId);
} catch (final ScimResourceAlreadyExistsException e) {
final String errorMessage = String.format(
"Could not create %s. A user with the same username already exists in the alias zone.",
strehle marked this conversation as resolved.
Show resolved Hide resolved
entity.getAliasDescription()
);
throw new EntityAliasFailedException(errorMessage, HttpStatus.CONFLICT.value(), e);
}
}
}
Loading
Loading