Alias Handler for SCIM Users #2769

adrianhoelzl-sap · 2024-03-07T12:34:12Z

see issue #2505

Preparation for alias feature for SCIM users; adds subclass of EntityAliasHandler that handles alias entities of SCIM users.

Prerequisites:

…into feature/alias-handler-for-scim-users

cf-gitbot · 2024-03-07T12:34:16Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187191383

The labels on this github issue will be updated when the story is started.

…s-if-alias-feature-disabled' into feature/alias-handler-for-scim-users

…eters

…ecksForNewAlias

…cyOfAliasEntity

… ScimUsers

…estamps from original user to alias user

peterhaochen47 · 2024-05-22T23:25:00Z

Hi, as this PR is a stepping stone toward the overall "alias feature," and as it looks like the changes are limited to the specific code paths related to the feature, I would defer the approval of this PR to @strehle, in terms of judging 1) whether this PR accomplishes its goal, and 2) wether this PR has enough test coverage and user docs (if applicable).

I'll also approve as well as long as our prior understanding the "alias feature" are still true after this PR: That the functionality & performance of all the existing UAA features are not impacted (do not experience breaking changes / regressions) when the "alias feature" is not turned on (and the feature is off by default).

Tallicia

Approved with request to answer questions before merging.

Tallicia · 2024-04-26T14:31:16Z

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

+            final ScimUser existingAliasEntity
+    ) {
+        // these three timestamps should not be overwritten by the timestamps of the original user
+        newAliasEntity.setPasswordLastModified(existingAliasEntity.getPasswordLastModified());


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

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:

uaa/model/src/main/java/org/cloudfoundry/identity/uaa/scim/impl/ScimUserJsonDeserializer.java

Line 92 in 091fbf2

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

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

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

Tallicia · 2024-04-26T14:33:21Z

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

+        newAliasEntity.setPasswordLastModified(existingAliasEntity.getPasswordLastModified());
+        newAliasEntity.setLastLogonTime(existingAliasEntity.getLastLogonTime());
+        newAliasEntity.setPreviousLogonTime(existingAliasEntity.getPreviousLogonTime());
+    }


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.

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.

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."

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:

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

Line 87 in f451e8e

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).

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

server/src/test/java/org/cloudfoundry/identity/uaa/scim/ScimUserAliasHandlerValidationTest.java

strehle

I would like to see 100 coverage on lines at least. Coverage on all conditions might be hard, but we should have tests with a meaning on all code lines.

Tests are like a technical specification, they later help to understand the code

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

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

…sHandler

…ture enabled, no IdP exists for original user

…ture enabled, IdP of original user has empty aliasId

strehle · 2024-06-03T15:16:37Z

@adrianhoelzl-sap please cleanup PR , see
https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=2769&id=cloudfoundry-identity-parent

Rest is ok

Tallicia · 2024-06-04T14:12:23Z

@adrianhoelzl-sap please cleanup PR , see https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=2769&id=cloudfoundry-identity-parent

Rest is ok

@strehle This link is not showing no results for me:

strehle · 2024-06-04T14:24:39Z

@adrianhoelzl-sap please cleanup PR , see https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=2769&id=cloudfoundry-identity-parent
Rest is ok

@strehle This link is not showing no results for me:

there where updates and now smells are solved
See overview https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2769

strehle · 2024-06-04T18:28:34Z

@Tallicia do you have open issues here ?

torsten-sap · 2024-06-05T10:59:01Z

I merge this PR as all questions seem to be addressed and two approvals exist.

adrianhoelzl-sap added 3 commits March 7, 2024 13:17

Merge branch 'feature/add-alias-id-and-alias-zid-fields-to-scimuser' …

b60ec98

…into feature/alias-handler-for-scim-users

Add alias properties to JSON deserialization of ScimUser class

3b14d18

Add alias handler for SCIM users

2b3ba5e

cf-gitbot added the unscheduled label Mar 7, 2024

adrianhoelzl-sap added DO NOT MERGE Internal Test or WIP, please DO NOT MERGE and removed unscheduled labels Mar 7, 2024

adrianhoelzl-sap added 15 commits April 3, 2024 11:31

Merge branch 'develop' into feature/alias-handler-for-scim-users

36d441e

Merge branch 'feature/prevent-update-and-delete-of-entities-with-alia…

75bbacb

…s-if-alias-feature-disabled' into feature/alias-handler-for-scim-users

Refactor

aa22118

Merge branch 'feature/prevent-update-and-delete-of-entities-with-alia…

4009ed3

…s-if-alias-feature-disabled' into feature/alias-handler-for-scim-users

Add @nonnull annotations to EntityAliasHandler.isValidAliasPair param…

800433e

…eters

Add null check to IdPs in ScimUserAliasHandler.additionalValidationCh…

ced3026

…ecksForNewAlias

Add tests for validation logic for ScimUsers

688a513

Merge branch 'develop' into feature/alias-handler-for-scim-users

bd6a79a

Adjust ScimUser properties set in ScimUserAliasHandler.cloneEntity

1a3668d

Revert adding ScimUserProvisioning.retrievePasswordForUser method

28e206a

Revert adding alias properties to ScimUserJsonDeserializer.deserialize

091fbf2

Introduce superclass for tests for EntityAliasHandler.ensureConsisten…

79cd7c7

…cyOfAliasEntity

Refactor

ef5f4dc

Refactor

37eedad

Move IdpWithAliasMatcher to superclass and make it generic

ff6079a

adrianhoelzl-sap mentioned this pull request Apr 10, 2024

Missing user management from custom IdZ to CC #2505

Closed

adrianhoelzl-sap added 3 commits April 10, 2024 16:47

Refactor

b4fb274

Add method 'entitiesAreEqual' to later on support equality checks for…

463202f

… ScimUsers

Add ScimUserAliasHandlerEnsureConsistencyTest

9ed1580

adrianhoelzl-sap mentioned this pull request Apr 11, 2024

Refactor tests for EntityAliasHandler.ensureConsistencyOfAliasEntity #2824

Merged

Change update behavior for SCIM user aliases: no longer propagate tim…

96af351

…estamps from original user to alias user

adrianhoelzl-sap marked this pull request as ready for review April 11, 2024 15:31

adrianhoelzl-sap requested a review from a team April 11, 2024 15:43

strehle requested review from Tallicia and peterhaochen47 May 14, 2024 11:35

Tallicia approved these changes May 29, 2024

View reviewed changes

Tallicia added in progress accepted Accepted the issue in_review The PR is currently in review clarification needed The issue is not accepted but we need clarification DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels May 29, 2024

strehle reviewed Jun 3, 2024

View reviewed changes

adrianhoelzl-sap added 6 commits June 3, 2024 11:14

Remove setting properties of new alias explicitly to null in ScimAlia…

5699f43

…sHandler

Refactor

0cf1ba7

Add unit test for SCIM alias validation: no existing alias, alias fea…

97d13b3

…ture enabled, no IdP exists for original user

Add unit test for SCIM alias validation: no existing alias, alias fea…

4321693

…ture enabled, IdP of original user has empty aliasId

Add missing empty line

31ebf13

Introduce bean for aliasEntitiesEnabled configuration property

823fc2f

Remove unused imports

f451e8e

strehle removed the clarification needed The issue is not accepted but we need clarification label Jun 4, 2024

strehle approved these changes Jun 4, 2024

View reviewed changes

strehle removed in_review The PR is currently in review DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels Jun 4, 2024

torsten-sap merged commit 3164d36 into develop Jun 5, 2024
20 checks passed

torsten-sap deleted the feature/alias-handler-for-scim-users branch June 5, 2024 10:59

cf-gitbot removed in progress accepted Accepted the issue labels Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alias Handler for SCIM Users #2769

Alias Handler for SCIM Users #2769

adrianhoelzl-sap commented Mar 7, 2024 •

edited

Loading

cf-gitbot commented Mar 7, 2024

peterhaochen47 commented May 22, 2024 •

edited

Loading

Tallicia left a comment

Tallicia Apr 26, 2024

adrianhoelzl-sap Jun 3, 2024

Tallicia Jun 4, 2024

Tallicia Jun 4, 2024

Tallicia Apr 26, 2024

adrianhoelzl-sap Jun 3, 2024

Tallicia Jun 4, 2024

adrianhoelzl-sap Jun 4, 2024

strehle left a comment

strehle commented Jun 3, 2024

Tallicia commented Jun 4, 2024

strehle commented Jun 4, 2024

strehle commented Jun 4, 2024

torsten-sap commented Jun 5, 2024

Alias Handler for SCIM Users #2769

Alias Handler for SCIM Users #2769

Conversation

adrianhoelzl-sap commented Mar 7, 2024 • edited Loading

cf-gitbot commented Mar 7, 2024

peterhaochen47 commented May 22, 2024 • edited Loading

Tallicia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strehle left a comment

Choose a reason for hiding this comment

strehle commented Jun 3, 2024

Tallicia commented Jun 4, 2024

strehle commented Jun 4, 2024

strehle commented Jun 4, 2024

torsten-sap commented Jun 5, 2024

adrianhoelzl-sap commented Mar 7, 2024 •

edited

Loading

peterhaochen47 commented May 22, 2024 •

edited

Loading