Skip to content

Commit

Permalink
Merge pull request #2712 from cloudfoundry/2671-inconsistent-update-b…
Browse files Browse the repository at this point in the history 
…ehavior-for-scim-usersuserid

Inconsistent Update Behavior for SCIM "/Users/{userId}"
  • Loading branch information
@adrianhoelzl-sap
adrianhoelzl-sap authored Feb 8, 2024
2 parents d0b75bb + 4752fb4 commit c011141
Show file tree
Hide file tree
Showing 8 changed files with 237 additions and 122 deletions.
10 changes: 10 additions & 0 deletions metrics-data/src/main/java/org/cloudfoundry/identity/uaa/util/JsonUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package org.cloudfoundry.identity.uaa.util;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand Down Expand Up @@ -55,6 +56,15 @@ public static <T> T readValue(String s, Class<T> clazz) throws JsonUtilException
}
}

public static Map<String, Object> readValueAsMap(final String input) {
try {
final JsonNode rootNode = objectMapper.readTree(input);
return getNodeAsMap(rootNode);
} catch (final JsonProcessingException e) {
throw new JsonUtilException(e);
}
}

public static <T> T readValue(byte[] data, Class<T> clazz) throws JsonUtilException {
try {
if (data!=null && data.length>0) {
Expand Down
25 changes: 25 additions & 0 deletions metrics-data/src/test/java/org/cloudfoundry/identity/uaa/util/JsonUtilsTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package org.cloudfoundry.identity.uaa.util;

import com.fasterxml.jackson.core.type.TypeReference;

import org.assertj.core.api.Assertions;
import org.cloudfoundry.identity.uaa.metrics.UrlGroup;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import java.util.List;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -42,6 +47,26 @@ void testReadValueByteClass() {
assertNull(JsonUtils.readValue((byte[]) null, UrlGroup.class));
}

@Test
void testReadValueAsMap() {
final String jsonInput = "{\"prop1\":\"abc\",\"prop2\":{\"prop2a\":\"def\",\"prop2b\":\"ghi\"},\"prop3\":[\"jkl\",\"mno\"]}";
final Map<String, Object> map = JsonUtils.readValueAsMap(jsonInput);
Assertions.assertThat(map).isNotNull();
Assertions.assertThat(map.get("prop1")).isNotNull().isEqualTo("abc");
Assertions.assertThat(map.get("prop2")).isNotNull().isInstanceOf(Map.class);
Assertions.assertThat(((Map<String, Object>) map.get("prop2")).get("prop2a")).isNotNull().isEqualTo("def");
Assertions.assertThat(((Map<String, Object>) map.get("prop2")).get("prop2b")).isNotNull().isEqualTo("ghi");
Assertions.assertThat(map.get("prop3")).isNotNull().isInstanceOf(List.class);
Assertions.assertThat((List<String>) map.get("prop3")).containsExactly("jkl", "mno");
}

@ParameterizedTest
@ValueSource(strings = {"{", "}", "{\"prop1\":\"abc\","})
void testReadValueAsMap_Invalid(final String input) {
Assertions.assertThatExceptionOfType(JsonUtils.JsonUtilException.class)
.isThrownBy(() -> JsonUtils.readValueAsMap(input));
}

@Test
void testReadValueBytes() {
assertNotNull(JsonUtils.readValue(jsonTestObjectString.getBytes(), new TypeReference<Map<String, Object>>() {}));
Expand Down
29 changes: 20 additions & 9 deletions model/src/main/java/org/cloudfoundry/identity/uaa/scim/ScimUser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,27 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.scim;

import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import static java.util.Optional.ofNullable;
import static org.springframework.util.StringUtils.hasText;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Objects;
import java.util.Set;

import org.cloudfoundry.identity.uaa.approval.Approval;
import org.cloudfoundry.identity.uaa.impl.JsonDateSerializer;
import org.cloudfoundry.identity.uaa.scim.impl.ScimUserJsonDeserializer;
import org.springframework.util.Assert;

import java.util.*;

import static java.util.Optional.ofNullable;
import static org.springframework.util.StringUtils.hasText;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;

/**
* Object to hold SCIM data for Jackson to map to and from JSON
Expand Down Expand Up @@ -772,6 +779,10 @@ public void patch(ScimUser patch) {
}
}

if (hasText(patch.getOrigin()) && !patch.getOrigin().equals(getOrigin())) {
throw new IllegalArgumentException("Cannot change origin in patch of user.");
}

//Merge simple Attributes, that are stored
ofNullable(patch.getUserName()).ifPresent(this::setUserName);

Expand Down
20 changes: 12 additions & 8 deletions server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,17 +273,23 @@ public String extractPhoneNumber(final ScimUser user) {
}

@Override
public ScimUser update(final String id, final ScimUser user, String zoneId) throws InvalidScimResourceException {
public ScimUser update(final String id, final ScimUser user, final String zoneId) throws InvalidScimResourceException {
logger.debug("Updating user " + user.getUserName());
final String origin = hasText(user.getOrigin()) ? user.getOrigin() : OriginKeys.UAA;
user.setOrigin(origin);
if (isCheckOriginEnabled(zoneId)) {
checkOrigin(origin, zoneId);

// check if the origin was changed
final ScimUser existingUser = retrieve(id, zoneId);
if (!origin.equals(existingUser.getOrigin())) {
throw new InvalidScimResourceException("Cannot change user's origin in update operation.");
}

ScimUtils.validate(user);
int updated = jdbcTemplate.update(UPDATE_USER_SQL, ps -> {
int pos = 1;
Timestamp t = new Timestamp(new Date().getTime());

// placeholders in UPDATE
ps.setInt(pos++, user.getVersion() + 1);
ps.setTimestamp(pos++, t);
ps.setString(pos++, user.getUserName());
Expand All @@ -296,9 +302,11 @@ public ScimUser update(final String id, final ScimUser user, String zoneId) thro
ps.setString(pos++, origin);
ps.setString(pos++, hasText(user.getExternalId())?user.getExternalId():null);
ps.setString(pos++, user.getSalt());

// placeholders in WHERE
ps.setString(pos++, id);
ps.setInt(pos++, user.getVersion());
ps.setString(pos++, zoneId);
ps.setString(pos, zoneId);
});
ScimUser result = retrieve(id, zoneId);
if (updated == 0) {
Expand Down Expand Up @@ -574,10 +582,6 @@ private void validateUserLimit(String zoneId, UserConfig userConfig) {
}
}

private boolean isCheckOriginEnabled(String zoneId) {
return isCheckOriginEnabled(getUserConfig(zoneId));
}

private boolean isCheckOriginEnabled(UserConfig userConfig) {
return userConfig != null && userConfig.isCheckOriginEnabled();
}
Expand Down
8 changes: 8 additions & 0 deletions server/src/test/java/org/cloudfoundry/identity/uaa/scim/ScimUserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*******************************************************************************/
package org.cloudfoundry.identity.uaa.scim;

import org.assertj.core.api.Assertions;
import org.cloudfoundry.identity.uaa.approval.Approval;
import org.cloudfoundry.identity.uaa.scim.ScimUser.Group;
import org.cloudfoundry.identity.uaa.util.JsonUtils;
Expand Down Expand Up @@ -583,6 +584,13 @@ public void testPatchUserDropNameSubAttributes() {
assertNull(user.getName().getFamilyName());
}

@Test
public void testPatchUserRejectChangingOrigin() {
patch.setOrigin("some-new-origin");
Assertions.assertThatIllegalArgumentException().isThrownBy(() -> user.patch(patch))
.withMessage("Cannot change origin in patch of user.");
}

@Test
public void testPatchUserDropNonUsedAttributes() {
int pos = 0;
Expand Down
108 changes: 63 additions & 45 deletions .../src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,31 +398,29 @@ void countUsersAcrossAllZones() {
}

@Test
void validateOriginAndExternalIDDuringCreateAndUpdate() {
String origin = "test-"+randomString();
void validateExternalIdDuringCreateAndUpdate() {
final String origin = "test-"+randomString();
addIdentityProvider(jdbcTemplate, IdentityZone.getUaaZoneId(), origin);
String externalId = "testId";
ScimUser user = new ScimUser(null, "jo@foo.com", "Jo", "User");
final String externalId = "testId";
final ScimUser user = new ScimUser(null, "jo@foo.com", "Jo", "User");
user.setOrigin(origin);
user.setExternalId(externalId);
user.addEmail("jo@blah.com");
ScimUser created = jdbcScimUserProvisioning.createUser(user, "j7hyqpassX", currentIdentityZoneId);
final ScimUser created = jdbcScimUserProvisioning.createUser(user, "j7hyqpassX", currentIdentityZoneId);
assertEquals("jo@foo.com", created.getUserName());
assertNotNull(created.getId());
assertNotSame(user.getId(), created.getId());
Map<String, Object> map = jdbcTemplate.queryForMap("select * from users where id=?", created.getId());
final Map<String, Object> map = jdbcTemplate.queryForMap("select * from users where id=?", created.getId());
assertEquals(user.getUserName(), map.get("userName"));
assertEquals(user.getUserType(), map.get(UaaAuthority.UAA_USER.getUserType()));
assertNull(created.getGroups());
assertEquals(origin, created.getOrigin());
assertEquals(externalId, created.getExternalId());
String origin2 = "test2-"+randomString();
addIdentityProvider(jdbcTemplate, IdentityZone.getUaaZoneId(), origin2);
String externalId2 = "testId2";
created.setOrigin(origin2);

// update external ID
final String externalId2 = "testId2";
created.setExternalId(externalId2);
ScimUser updated = jdbcScimUserProvisioning.update(created.getId(), created, currentIdentityZoneId);
assertEquals(origin2, updated.getOrigin());
final ScimUser updated = jdbcScimUserProvisioning.update(created.getId(), created, currentIdentityZoneId);
assertEquals(externalId2, updated.getExternalId());
}

Expand Down Expand Up @@ -539,6 +537,31 @@ void updateCannotModifyGroups() {
assertNull(joe.getGroups());
}

@Test
void updateCannotModifyOrigin() {
final String userId = UUID.randomUUID().toString();

final ScimUser userToCreate = new ScimUser(userId, "john.doe", "John", "Doe");
userToCreate.setPassword("some-password");
userToCreate.setOrigin("origin1");
userToCreate.setZoneId(currentIdentityZoneId);
userToCreate.setPhoneNumbers(Collections.singletonList(new PhoneNumber("12345")));
userToCreate.setPrimaryEmail("john.doe@example.com");
addUser(jdbcTemplate, userToCreate);

final ScimUser scimUser = jdbcScimUserProvisioning.retrieve(userId, currentIdentityZoneId);

// change origin
scimUser.setOrigin("origin2");

final InvalidScimResourceException exception = assertThrows(InvalidScimResourceException.class, () ->
jdbcScimUserProvisioning.update(userId, scimUser, currentIdentityZoneId)
);

assertEquals(HttpStatus.BAD_REQUEST, exception.getStatus());
assertEquals("Cannot change user's origin in update operation.", exception.getMessage());
}

@Test
void updateWithWrongVersionIsError() {
ScimUser jo = new ScimUser(null, "josephine", "Jo", "NewUser");
Expand All @@ -556,12 +579,19 @@ void updateWithBadUsernameIsError() {

@Test
void updateWithBadUsernameIsOk_For_Non_UAA() {
ScimUser jo = new ScimUser(null, "jo$ephine", "Jo", "NewUser");
jo.setOrigin(OriginKeys.LDAP);
jo.addEmail("jo@blah.com");
ScimUser joe = jdbcScimUserProvisioning.update(joeId, jo, currentIdentityZoneId);
assertEquals("jo$ephine", joe.getUserName());
assertEquals(OriginKeys.LDAP, joe.getOrigin());
final String id = UUID.randomUUID().toString();
final ScimUser user = new ScimUser(id, "josephine", "Jo", "NewUser");
user.setOrigin(OriginKeys.LDAP);
user.setZoneId(currentIdentityZoneId);
user.addEmail("jo@blah.com");
user.setPhoneNumbers(Collections.singletonList(new PhoneNumber("12345")));
addUser(jdbcTemplate, user);

final ScimUser updatePayload = jdbcScimUserProvisioning.retrieve(id, currentIdentityZoneId);
updatePayload.setUserName("jo$ephine");
final ScimUser userAfterUpdate = jdbcScimUserProvisioning.update(id, updatePayload, currentIdentityZoneId);
assertEquals("jo$ephine", userAfterUpdate.getUserName());
assertEquals(OriginKeys.LDAP, userAfterUpdate.getOrigin());
}

@Test
Expand Down Expand Up @@ -1115,33 +1145,6 @@ void cannotCreateUserWithInvalidOrigin() {
idzManager.getCurrentIdentityZone().getConfig().getUserConfig().setCheckOriginEnabled(false);
}

@Test
void cannotUpdateUserWithInvalidOrigin() {
String validOrigin = "validOrigin-"+randomString();
String invalidOrigin = "invalidOrigin-"+randomString();
addIdentityProvider(jdbcTemplate, currentIdentityZoneId, validOrigin);
String userId = "user-"+randomString();
ScimUser scimUser = new ScimUser(userId, "user@example.com", "User", "Example");
ScimUser.Email email = new ScimUser.Email();
email.setValue(userId+"@example.com");
scimUser.setEmails(Collections.singletonList(email));
scimUser.setPassword(randomString());
scimUser.setOrigin(validOrigin);
try {
jdbcScimUserProvisioning.create(scimUser, currentIdentityZoneId);
} catch (Exception e) {
fail("Can't create test user");
}
scimUser.setOrigin(invalidOrigin);
idzManager.getCurrentIdentityZone().getConfig().getUserConfig().setCheckOriginEnabled(true);
assertThrowsWithMessageThat(
InvalidScimResourceException.class,
() -> jdbcScimUserProvisioning.update(userId, scimUser, currentIdentityZoneId),
containsString("Invalid origin")
);
idzManager.getCurrentIdentityZone().getConfig().getUserConfig().setCheckOriginEnabled(false);
}

private static String createUserForDelete(final JdbcTemplate jdbcTemplate, String zoneId) {
String randomUserId = UUID.randomUUID().toString();
addUser(jdbcTemplate, randomUserId, randomUserId, "password", randomUserId + "@delete.com", "ToDelete", "User", "+1-234-5678910", zoneId);
Expand Down Expand Up @@ -1170,6 +1173,21 @@ private static void addUser(final JdbcTemplate jdbcTemplate,
jdbcTemplate.execute(addUserSql);
}

private static void addUser(final JdbcTemplate jdbcTemplate, final ScimUser scimUser) {
String addUserSql = String.format(
"insert into users (id, username, password, email, givenName, familyName, phoneNumber, identity_zone_id, origin) values ('%s','%s','%s','%s','%s','%s','%s','%s', '%s')",
scimUser.getId(),
scimUser.getUserName(),
scimUser.getPassword(),
scimUser.getPrimaryEmail(),
scimUser.getName().getGivenName(),
scimUser.getName().getFamilyName(),
scimUser.getPhoneNumbers().get(0),
scimUser.getZoneId(),
scimUser.getOrigin());
jdbcTemplate.execute(addUserSql);
}

private static void createRandomUserInZone(
final JdbcTemplate jdbcTemplate,
final RandomValueStringGenerator generator,
Expand Down
4 changes: 2 additions & 2 deletions uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointDocs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ class ScimUserEndpointDocs extends EndpointDocs {
fieldWithPath("approvals").ignored().type(ARRAY).description("Approvals are not created at this time"),
fieldWithPath("active").optional(true).type(BOOLEAN).description(userActiveDescription),
fieldWithPath("verified").optional(true).type(BOOLEAN).description(userVerifiedDescription),
fieldWithPath("origin").optional(OriginKeys.UAA).type(STRING).description(userOriginDescription),
fieldWithPath("origin").optional(OriginKeys.UAA).type(STRING).description(userOriginDescription + " The `origin` value cannot be changed in an update operation."),
fieldWithPath("zoneId").ignored().type(STRING).description(userZoneIdDescription),
fieldWithPath("passwordLastModified").ignored().type(STRING).description(passwordLastModifiedDescription),
fieldWithPath("externalId").optional(null).type(STRING).description(externalIdDescription),
Expand Down Expand Up @@ -271,7 +271,7 @@ class ScimUserEndpointDocs extends EndpointDocs {
fieldWithPath("approvals").ignored().type(ARRAY).description("Approvals are not created at this time"),
fieldWithPath("active").optional(true).type(BOOLEAN).description(userActiveDescription),
fieldWithPath("verified").optional(true).type(BOOLEAN).description(userVerifiedDescription),
fieldWithPath("origin").optional(OriginKeys.UAA).type(STRING).description(userOriginDescription),
fieldWithPath("origin").optional(OriginKeys.UAA).type(STRING).description(userOriginDescription + " The `origin` value cannot be changed in a patch operation."),
fieldWithPath("zoneId").ignored().type(STRING).description(userZoneIdDescription),
fieldWithPath("passwordLastModified").ignored().type(STRING).description(passwordLastModifiedDescription),
fieldWithPath("externalId").optional(null).type(STRING).description(externalIdDescription),
Expand Down
Loading

0 comments on commit c011141

Please sign in to comment.