From 170e05cb33bda43e7eade83312f100eff3ea3017 Mon Sep 17 00:00:00 2001 From: jasonfagerberg-toast Date: Thu, 5 Dec 2024 11:36:34 -0500 Subject: [PATCH 1/7] failing test attempting to replace entire enterprise schema --- .../scim/core/repository/PatchHandlerTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index 2df84326f..b68278b72 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -19,6 +19,7 @@ package org.apache.directory.scim.core.repository; +import java.util.HashMap; import lombok.SneakyThrows; import org.apache.directory.scim.core.schema.SchemaRegistry; import org.apache.directory.scim.spec.extension.EnterpriseExtension; @@ -56,6 +57,22 @@ public PatchHandlerTest() { this.patchHandler = new DefaultPatchHandler(schemaRegistry); } + @Test + public void applyReplaceEntireEnterpriseExtension() { + String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; + Map enterpriseExtensionValue = new HashMap<>(); + enterpriseExtensionValue.put("costCenter", "New Cost Center"); + enterpriseExtensionValue.put("department", "New Department"); + PatchOperation op = patchOperation(REPLACE, enterpriseExtensionUrn, enterpriseExtensionValue); + // This throws an NPE because DefaultPatchHandler is treating the path as an exact path to a simple attribute and not a path to a complex attribute + ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); + EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); + assertThat(actual).isNotNull(); + assertThat(actual.getCostCenter()).isEqualTo("New Cost Center"); + assertThat(actual.getDepartment()).isEqualTo("new Department"); + } + + @Test public void applyReplaceUserName() { String newUserName = "testUser_updated@test.com"; From 5ba2c4e8c1347cb9d99b14f9a67825e9e8ae6bd8 Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Thu, 5 Dec 2024 21:24:51 -0500 Subject: [PATCH 2/7] Correct new PatchHandlerTest assertion --- .../apache/directory/scim/core/repository/PatchHandlerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index b68278b72..c87ff155d 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -69,7 +69,7 @@ public void applyReplaceEntireEnterpriseExtension() { EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); assertThat(actual).isNotNull(); assertThat(actual.getCostCenter()).isEqualTo("New Cost Center"); - assertThat(actual.getDepartment()).isEqualTo("new Department"); + assertThat(actual.getDepartment()).isEqualTo("New Department"); } From 5e6af09f01b454599c08eeaa1faf38a7d7d968a1 Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Thu, 5 Dec 2024 21:27:14 -0500 Subject: [PATCH 3/7] Add ability to accept patches for entire root extension Fixes: #695 --- .../core/repository/DefaultPatchHandler.java | 46 +++++++++++++++++-- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java index aa15875b1..a2f1f7079 100644 --- a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java +++ b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java @@ -132,10 +132,50 @@ private void apply(T source, Map source // if the attribute has a URN, assume it's an extension that URN does not match the baseUrn if (attributeReference.hasUrn() && !attributeReference.getUrn().equals(source.getBaseUrn())) { Schema schema = this.schemaRegistry.getSchema(attributeReference.getUrn()); - Attribute attribute = schema.getAttribute(attributeReference.getAttributeName()); - checkMutability(schema.getAttributeFromPath(attributeReference.getFullAttributeName())); - patchOperationHandler.applyExtensionValue(source, sourceAsMap, schema, attribute, valuePathExpression, attributeReference.getUrn(), patchOperation.getValue()); + if (schema != null) { + Attribute attribute = schema.getAttribute(attributeReference.getAttributeName()); + checkMutability(schema.getAttributeFromPath(attributeReference.getFullAttributeName())); + + patchOperationHandler.applyExtensionValue(source, sourceAsMap, schema, attribute, valuePathExpression, attributeReference.getUrn(), patchOperation.getValue()); + } else { + // If schema is null, it's either the root of an extension, or an invalid patch path + // It's not possible from the antlr parser to tell the diff between 'this:is:extension:urn' and 'this:is:extension:urn:attribute' + // Check if the fully qualified attribute is a valid schema + schema = this.schemaRegistry.getSchema(attributeReference.getFullyQualifiedAttributeName()); + if (schema == null) { + throw new IllegalArgumentException("Invalid attribute path found in patch request: " + attributeReference); + } + + // root extension object found, per RFC, the value of the patch must be a Map + if (!(patchOperation.getValue() instanceof Map)) { + throw new IllegalArgumentException("Invalid patch value for root of extension, expected map"); + } + + // loop through each attribute and update them + Map mapValue = (Map) patchOperation.getValue(); + for (Map.Entry entry : mapValue.entrySet()) { + String attributeName = entry.getKey(); + Attribute attribute = schema.getAttribute(attributeName); + checkMutability(attribute); + + // recreate the valuePathExpression using each child attribute + ValuePathExpression extensionValuePathExpression = new ValuePathExpression(new AttributeReference(schema.getUrn(), attributeName)); + + // ensure the sourceAsMap contains the extensions object, create one if needed. + PatchOperation.Type op = patchOperation.getOperation(); + if (op == PatchOperation.Type.ADD || op == PatchOperation.Type.REPLACE) { + sourceAsMap.computeIfAbsent(schema.getUrn(), k -> new HashMap<>()); + List schemas = (List) sourceAsMap.get("schemas"); + if (!schemas.contains(attributeName)) { + schemas.add(attributeName); + } + } + + // now update the sourceAsMap + patchOperationHandler.applyExtensionValue(source, sourceAsMap, schema, attribute, extensionValuePathExpression, schema.getUrn(), entry.getValue()); + } + } } else { Schema schema = this.schemaRegistry.getSchema(source.getBaseUrn()); Attribute attribute = schema.getAttribute(attributeReference.getAttributeName()); From 894bdd41263fb54d559dacb1dbfd8a5bf11f319a Mon Sep 17 00:00:00 2001 From: Brian Demers Date: Thu, 5 Dec 2024 22:13:39 -0500 Subject: [PATCH 4/7] [fix] Clean up flow of code - Removes special checks for operation types, moves logic to specfic PatchOperationHandlers - Adds test case for removal of full extension --- .../core/repository/DefaultPatchHandler.java | 82 ++++++++++++------- .../core/repository/PatchHandlerTest.java | 8 +- 2 files changed, 61 insertions(+), 29 deletions(-) diff --git a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java index a2f1f7079..b69e07ea4 100644 --- a/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java +++ b/scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java @@ -147,34 +147,7 @@ private void apply(T source, Map source throw new IllegalArgumentException("Invalid attribute path found in patch request: " + attributeReference); } - // root extension object found, per RFC, the value of the patch must be a Map - if (!(patchOperation.getValue() instanceof Map)) { - throw new IllegalArgumentException("Invalid patch value for root of extension, expected map"); - } - - // loop through each attribute and update them - Map mapValue = (Map) patchOperation.getValue(); - for (Map.Entry entry : mapValue.entrySet()) { - String attributeName = entry.getKey(); - Attribute attribute = schema.getAttribute(attributeName); - checkMutability(attribute); - - // recreate the valuePathExpression using each child attribute - ValuePathExpression extensionValuePathExpression = new ValuePathExpression(new AttributeReference(schema.getUrn(), attributeName)); - - // ensure the sourceAsMap contains the extensions object, create one if needed. - PatchOperation.Type op = patchOperation.getOperation(); - if (op == PatchOperation.Type.ADD || op == PatchOperation.Type.REPLACE) { - sourceAsMap.computeIfAbsent(schema.getUrn(), k -> new HashMap<>()); - List schemas = (List) sourceAsMap.get("schemas"); - if (!schemas.contains(attributeName)) { - schemas.add(attributeName); - } - } - - // now update the sourceAsMap - patchOperationHandler.applyExtensionValue(source, sourceAsMap, schema, attribute, extensionValuePathExpression, schema.getUrn(), entry.getValue()); - } + patchOperationHandler.applyRootExtensionValue(source, sourceAsMap, schema, patchOperation.getValue()); } } else { Schema schema = this.schemaRegistry.getSchema(source.getBaseUrn()); @@ -269,6 +242,28 @@ default void applyExtensionValue(final T source, Map void applyRootExtensionValue(final T source, Map sourceAsMap, Schema schema, Object value) { + + // root extension object found, per RFC, the value of the patch must be a Map + if (!(value instanceof Map)) { + throw new IllegalArgumentException("Invalid patch value for root of extension, expected map"); + } + + // loop through each attribute and update them + Map mapValue = (Map) value; + for (Map.Entry entry : mapValue.entrySet()) { + String attributeName = entry.getKey(); + Attribute attribute = schema.getAttribute(attributeName); + checkMutability(attribute); + + // recreate the valuePathExpression using each child attribute + ValuePathExpression extensionValuePathExpression = new ValuePathExpression(new AttributeReference(schema.getUrn(), attributeName)); + + // now update the sourceAsMap + applyExtensionValue(source, sourceAsMap, schema, attribute, extensionValuePathExpression, schema.getUrn(), entry.getValue()); + } + } + void applySingleValue(Map sourceAsMap, Attribute attribute, AttributeReference attributeReference, Object value); void applyMultiValue(final T source, Map sourceAsMap, Schema schema, Attribute attribute, AttributeReference attributeReference, Object value); void applyMultiValue(final T source, Map sourceAsMap, Schema schema, Attribute attribute, ValuePathExpression valuePathExpression, Object value); @@ -369,6 +364,21 @@ public void applyMultiValue(T source, Map void applyExtensionValue(T source, Map sourceAsMap, Schema schema, Attribute attribute, ValuePathExpression valuePathExpression, String urn, Object value) { + + // add the extension URN + Collection schemas = (Collection) sourceAsMap.get("schemas"); + schemas.add(urn); + + // if the extension object does not yet exist, create it + if (!sourceAsMap.containsKey(urn)) { + sourceAsMap.put(urn, new HashMap<>()); + } + + PatchOperationHandler.super.applyExtensionValue(source, sourceAsMap, schema, attribute, valuePathExpression, urn, value); + } + @Override public void applySingleValue(Map sourceAsMap, Attribute attribute, AttributeReference attributeReference, Object value) { if (attributeReference.hasSubAttribute()) { @@ -439,6 +449,22 @@ public void applyValue(final T source, Map void applyRootExtensionValue(T source, Map sourceAsMap, Schema schema, Object value) { + + // remove the whole extension if value is null + if (value == null) { + // remove the schema definition + Collection schemas = (Collection) sourceAsMap.get("schemas"); + schemas.remove(schema.getUrn()); + + // remove the root extension object + sourceAsMap.remove(schema.getUrn()); + } else { + throw new IllegalArgumentException("Unsupported remove operation, expected patch value to be null when removing full extension object."); + } + } + @Override public void applySingleValue(Map sourceAsMap, Attribute attribute, AttributeReference attributeReference, Object value) { if (attributeReference.hasSubAttribute()) { diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index c87ff155d..b80d9edcb 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -64,7 +64,6 @@ public void applyReplaceEntireEnterpriseExtension() { enterpriseExtensionValue.put("costCenter", "New Cost Center"); enterpriseExtensionValue.put("department", "New Department"); PatchOperation op = patchOperation(REPLACE, enterpriseExtensionUrn, enterpriseExtensionValue); - // This throws an NPE because DefaultPatchHandler is treating the path as an exact path to a simple attribute and not a path to a complex attribute ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); assertThat(actual).isNotNull(); @@ -72,6 +71,13 @@ public void applyReplaceEntireEnterpriseExtension() { assertThat(actual.getDepartment()).isEqualTo("New Department"); } + @Test + public void applyRemoveEntireEnterpriseExtension() { + String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; + PatchOperation op = patchOperation(REMOVE, enterpriseExtensionUrn, null); + ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); + assertThat(updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User")).isNull(); + } @Test public void applyReplaceUserName() { From 2f5c09732fb535d93753f0989cc1cfe3bc7ba81d Mon Sep 17 00:00:00 2001 From: jasonfagerberg-toast Date: Fri, 6 Dec 2024 08:04:51 -0500 Subject: [PATCH 5/7] Added unit test for if path is null but extension is updated in value --- .../scim/core/repository/PatchHandlerTest.java | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index b80d9edcb..c0a7b6ae7 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -71,6 +71,21 @@ public void applyReplaceEntireEnterpriseExtension() { assertThat(actual.getDepartment()).isEqualTo("New Department"); } + @Test + public void applyReplaceEntireEnterpriseExtensionWithNullPath() { + String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; + Map enterpriseExtensionValue = Map.of( + "costCenter", "New Cost Center", + "department", "New Department" + ); + PatchOperation op = patchOperation(REPLACE, null, Map.of(enterpriseExtensionUrn, enterpriseExtensionValue)); + ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); + EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); + assertThat(actual).isNotNull(); + assertThat(actual.getCostCenter()).isEqualTo("New Cost Center"); + assertThat(actual.getDepartment()).isEqualTo("New Department"); + } + @Test public void applyRemoveEntireEnterpriseExtension() { String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; From e3b8adcebb5af669bc676a09aaf6b3d447b3240d Mon Sep 17 00:00:00 2001 From: jasonfagerberg-toast Date: Fri, 6 Dec 2024 08:18:50 -0500 Subject: [PATCH 6/7] minor test cleanup --- .../core/repository/PatchHandlerTest.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index c0a7b6ae7..08af47aed 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -19,7 +19,16 @@ package org.apache.directory.scim.core.repository; -import java.util.HashMap; +import static java.util.Map.entry; +import static org.apache.directory.scim.spec.patch.PatchOperation.Type.ADD; +import static org.apache.directory.scim.spec.patch.PatchOperation.Type.REMOVE; +import static org.apache.directory.scim.spec.patch.PatchOperation.Type.REPLACE; +import static org.apache.directory.scim.test.assertj.ScimpleAssertions.scimAssertThat; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import java.util.Map; +import java.util.Optional; import lombok.SneakyThrows; import org.apache.directory.scim.core.schema.SchemaRegistry; import org.apache.directory.scim.spec.extension.EnterpriseExtension; @@ -37,15 +46,6 @@ import org.apache.directory.scim.spec.resources.ScimUser; import org.junit.jupiter.api.Test; -import static org.apache.directory.scim.spec.patch.PatchOperation.Type.*; -import static java.util.Map.entry; -import static org.apache.directory.scim.test.assertj.ScimpleAssertions.scimAssertThat; -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.List; -import java.util.Map; -import java.util.Optional; - public class PatchHandlerTest { DefaultPatchHandler patchHandler; @@ -60,12 +60,13 @@ public PatchHandlerTest() { @Test public void applyReplaceEntireEnterpriseExtension() { String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; - Map enterpriseExtensionValue = new HashMap<>(); - enterpriseExtensionValue.put("costCenter", "New Cost Center"); - enterpriseExtensionValue.put("department", "New Department"); + Map enterpriseExtensionValue = Map.ofEntries( + entry("costCenter", "New Cost Center"), + entry("department", "New Department") + ); PatchOperation op = patchOperation(REPLACE, enterpriseExtensionUrn, enterpriseExtensionValue); ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); - EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); + EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension(enterpriseExtensionUrn); assertThat(actual).isNotNull(); assertThat(actual.getCostCenter()).isEqualTo("New Cost Center"); assertThat(actual.getDepartment()).isEqualTo("New Department"); @@ -74,13 +75,13 @@ public void applyReplaceEntireEnterpriseExtension() { @Test public void applyReplaceEntireEnterpriseExtensionWithNullPath() { String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; - Map enterpriseExtensionValue = Map.of( - "costCenter", "New Cost Center", - "department", "New Department" + Map enterpriseExtensionValue = Map.ofEntries( + entry("costCenter", "New Cost Center"), + entry("department", "New Department") ); PatchOperation op = patchOperation(REPLACE, null, Map.of(enterpriseExtensionUrn, enterpriseExtensionValue)); ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); - EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"); + EnterpriseExtension actual = (EnterpriseExtension) updatedUser.getExtension(enterpriseExtensionUrn); assertThat(actual).isNotNull(); assertThat(actual.getCostCenter()).isEqualTo("New Cost Center"); assertThat(actual.getDepartment()).isEqualTo("New Department"); @@ -91,7 +92,7 @@ public void applyRemoveEntireEnterpriseExtension() { String enterpriseExtensionUrn = "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"; PatchOperation op = patchOperation(REMOVE, enterpriseExtensionUrn, null); ScimUser updatedUser = patchHandler.apply(user(), List.of(op)); - assertThat(updatedUser.getExtension("urn:ietf:params:scim:schemas:extension:enterprise:2.0:User")).isNull(); + assertThat(updatedUser.getExtension(enterpriseExtensionUrn)).isNull(); } @Test From 58ac25bf5f2494675c972769d0062edcbb498415 Mon Sep 17 00:00:00 2001 From: jasonfagerberg-toast Date: Fri, 6 Dec 2024 10:10:46 -0500 Subject: [PATCH 7/7] undo import change --- .../core/repository/PatchHandlerTest.java | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java index 08af47aed..fd5a3fe14 100644 --- a/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java +++ b/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java @@ -19,16 +19,6 @@ package org.apache.directory.scim.core.repository; -import static java.util.Map.entry; -import static org.apache.directory.scim.spec.patch.PatchOperation.Type.ADD; -import static org.apache.directory.scim.spec.patch.PatchOperation.Type.REMOVE; -import static org.apache.directory.scim.spec.patch.PatchOperation.Type.REPLACE; -import static org.apache.directory.scim.test.assertj.ScimpleAssertions.scimAssertThat; -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.List; -import java.util.Map; -import java.util.Optional; import lombok.SneakyThrows; import org.apache.directory.scim.core.schema.SchemaRegistry; import org.apache.directory.scim.spec.extension.EnterpriseExtension; @@ -46,6 +36,15 @@ import org.apache.directory.scim.spec.resources.ScimUser; import org.junit.jupiter.api.Test; +import static org.apache.directory.scim.spec.patch.PatchOperation.Type.*; +import static java.util.Map.entry; +import static org.apache.directory.scim.test.assertj.ScimpleAssertions.scimAssertThat; +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + public class PatchHandlerTest { DefaultPatchHandler patchHandler;