Alias Handler for SCIM Users #2769
Conversation
…into feature/alias-handler-for-scim-users
|
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. |
adrianhoelzl-sap
added
DO NOT MERGE
…s-if-alias-feature-disabled' into feature/alias-handler-for-scim-users
…s-if-alias-feature-disabled' into feature/alias-handler-for-scim-users
…estamps from original user to alias user
adrianhoelzl-sap
removed
the
DO NOT MERGE
| final ScimUserProvisioning scimUserProvisioning, | ||
| final IdentityProviderProvisioning identityProviderProvisioning, | ||
| final IdentityZoneManager identityZoneManager, | ||
| @Value("${login.aliasEntitiesEnabled:false}") final boolean aliasEntitiesEnabled |
There was a problem hiding this comment.
the 2nd place for aliasEntitiesEnabled , maybe create an extra bean or component and use here a ref
There was a problem hiding this comment.
How should this bean look like? Just a wrapper for the boolean value?
| 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.", |
There was a problem hiding this comment.
did not found a test which cover this part
There was a problem hiding this comment.
unit test added in commit e78e1fd
| aliasUser.setVerified(originalEntity.isVerified()); | ||
|
|
||
| // idzId and alias properties will be set later | ||
| aliasUser.setZoneId(null); |
There was a problem hiding this comment.
you dont need to set objects to null, why these many calls with null
There was a problem hiding this comment.
I wanted to make it explicit in this method which fields are set and which are null.
| * - 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); |
There was a problem hiding this comment.
empty is not what I would expect. is this a required field?
There was a problem hiding this comment.
In the create user endpoint, we also set the password explicitly to an empty string for all users with a non-uaa origin:
| * - update: with values from existing alias entity */ | ||
| aliasUser.setPasswordLastModified(null); | ||
| aliasUser.setLastLogonTime(null); | ||
| aliasUser.setPreviousLogonTime(null); |
There was a problem hiding this comment.
We do in other places Json object copy, e.g.
ScimUser scimCopy = JsonUtils.convertValue(originalEntity, ScimUser.class);
There was a problem hiding this comment.
see the comment above
…ied in the alias zone
|
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). |
see issue #2505
Preparation for alias feature for SCIM users; adds subclass of
EntityAliasHandlerthat handles alias entities of SCIM users.Prerequisites: