You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "bdemers (via GitHub)" <gi...@apache.org> on 2023/12/16 06:25:50 UTC
[PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
bdemers opened a new pull request, #454:
URL: https://github.com/apache/directory-scimple/pull/454
- Handle Azure style Group member remove operations
- Fix merge conflicts
- Adds support for Azure non-standard patch REMOVE requests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#discussion_r1428712628
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +469,46 @@ public void deleteItemWithComplexFilter() throws FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ GroupMembership member1 = userRef("1234");
+ GroupMembership member2 = userRef("5678");
+ GroupMembership member3 = new GroupMembership()
Review Comment:
`ResourceReference` was split up and renamed to `GroupMembership` and `UserGroup` (they have slightly different attributes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#issuecomment-1858738189
@kjthorpe18 I rebased your branch on `develop`, but I didn't want to force push to your PR.
If you want you can merge these changes in and we can continue working on your branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#issuecomment-1860838115
> This looks good to me - I'm fine with using my PR (which I rebased and force pushed) or this one to continue. Nice work!
Let's continue working with yours! I'll clean up my changes a little more, and push the rest of the bits to your PR
(I'll leave this one open until I get the bits moved)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#discussion_r1428712393
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -49,6 +52,7 @@ public class PatchHandlerTest {
public PatchHandlerTest() {
SchemaRegistry schemaRegistry = new SchemaRegistry();
schemaRegistry.addSchema(ScimUser.class, List.of(EnterpriseExtension.class));
+ schemaRegistry.addSchema(ScimGroup.class, null);
Review Comment:
The tests don't auto discover all the schema, so they must manually be added (this was causing one of the new tests to fail)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#discussion_r1428713069
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java:
##########
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
}
}
}
+
+ /**
+ * Detects Azure Quirk mode.
+ * Azure uses an out of spec patch operation that does not use an express,
+ * but instead uses a remove with a value. Detect this and convert it to an expression
+ * <pre><code>
+ * {
+ * "op":"remove",
+ * "path":"members",
+ * "value":[{
+ * "value":"<id>"
+ * }]
+ * }
+ * </code></pre>
+ * @param valuePathExpression The valuePathExpression to check if it has a null attribute
+ * @return true, if Azure patch REMOVE detected.
+ */
+ private static boolean isAzureRemoveQuirk(Attribute attribute, ValuePathExpression valuePathExpression, Object value) {
+ return attribute.isMultiValued()
+ && attribute.getAttribute(VALUE_ATTRIBUTE_NAME) != null
+ && valuePathExpression.getAttributeExpression() == null
+ && value instanceof Collection;
+ }
+
+ private static List<String> azureQuirkValuesToRemove(Collection<?> listOfMaps, Attribute attribute) {
+ return listOfMaps.stream()
+ .map(item -> {
+ if (!(item instanceof Map)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but 'value' is not a list of maps");
+ }
+ return (Map<?,?>) item;
+ })
+ .map(item -> {
+ Attribute valueAttribute = attribute.getAttribute(VALUE_ATTRIBUTE_NAME);
+ Object itemValue = item.get(valueAttribute.getName());
+ if (!(itemValue instanceof String)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but item 'value' is not a string");
+ }
+
+ return (String) itemValue;
+ })
+ .collect(toList());
Review Comment:
This is a bit ugly, but I assume we want to fail if we detect a different version of this non-spec request.
Open to thoughts on cleaning this up!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers closed pull request #454: Handle Azure style Group member remove operations (rebased)
URL: https://github.com/apache/directory-scimple/pull/454
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#issuecomment-1862960449
All changes are now part of #448
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "kjthorpe18 (via GitHub)" <gi...@apache.org>.
kjthorpe18 commented on code in PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#discussion_r1428820778
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java:
##########
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
}
}
}
+
+ /**
+ * Detects Azure Quirk mode.
+ * Azure uses an out of spec patch operation that does not use an express,
Review Comment:
```suggestion
* Azure uses an out of spec patch operation that does not use a path expression,
```
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java:
##########
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
}
}
}
+
+ /**
+ * Detects Azure Quirk mode.
+ * Azure uses an out of spec patch operation that does not use an express,
+ * but instead uses a remove with a value. Detect this and convert it to an expression
+ * <pre><code>
+ * {
+ * "op":"remove",
+ * "path":"members",
+ * "value":[{
+ * "value":"<id>"
+ * }]
+ * }
+ * </code></pre>
+ * @param valuePathExpression The valuePathExpression to check if it has a null attribute
+ * @return true, if Azure patch REMOVE detected.
+ */
+ private static boolean isAzureRemoveQuirk(Attribute attribute, ValuePathExpression valuePathExpression, Object value) {
+ return attribute.isMultiValued()
+ && attribute.getAttribute(VALUE_ATTRIBUTE_NAME) != null
+ && valuePathExpression.getAttributeExpression() == null
+ && value instanceof Collection;
+ }
+
+ private static List<String> azureQuirkValuesToRemove(Collection<?> listOfMaps, Attribute attribute) {
+ return listOfMaps.stream()
+ .map(item -> {
+ if (!(item instanceof Map)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but 'value' is not a list of maps");
+ }
+ return (Map<?,?>) item;
+ })
+ .map(item -> {
+ Attribute valueAttribute = attribute.getAttribute(VALUE_ATTRIBUTE_NAME);
+ Object itemValue = item.get(valueAttribute.getName());
+ if (!(itemValue instanceof String)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but item 'value' is not a string");
+ }
+
+ return (String) itemValue;
+ })
+ .collect(toList());
Review Comment:
My mind doesn't immediately jump to using streams, so it took a little longer to understand what's happening, but this is fine!
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java:
##########
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
}
}
}
+
+ /**
+ * Detects Azure Quirk mode.
+ * Azure uses an out of spec patch operation that does not use an express,
+ * but instead uses a remove with a value. Detect this and convert it to an expression
+ * <pre><code>
+ * {
+ * "op":"remove",
+ * "path":"members",
+ * "value":[{
+ * "value":"<id>"
+ * }]
Review Comment:
Note that this is how Azure structures the request, but in theory, it could have other key:value pairs here, e.g.
```
"value":[{
"value":"<id>"
"display":"someDisplayName",
"ref":"https://scim-example.com/api/v2/Users/<id>",
}]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] Handle Azure style Group member remove operations (rebased) [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #454:
URL: https://github.com/apache/directory-scimple/pull/454#discussion_r1428713069
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/DefaultPatchHandler.java:
##########
@@ -426,5 +447,48 @@ public <T extends ScimResource> void applyMultiValue(T source, Map<String, Objec
}
}
}
+
+ /**
+ * Detects Azure Quirk mode.
+ * Azure uses an out of spec patch operation that does not use an express,
+ * but instead uses a remove with a value. Detect this and convert it to an expression
+ * <pre><code>
+ * {
+ * "op":"remove",
+ * "path":"members",
+ * "value":[{
+ * "value":"<id>"
+ * }]
+ * }
+ * </code></pre>
+ * @param valuePathExpression The valuePathExpression to check if it has a null attribute
+ * @return true, if Azure patch REMOVE detected.
+ */
+ private static boolean isAzureRemoveQuirk(Attribute attribute, ValuePathExpression valuePathExpression, Object value) {
+ return attribute.isMultiValued()
+ && attribute.getAttribute(VALUE_ATTRIBUTE_NAME) != null
+ && valuePathExpression.getAttributeExpression() == null
+ && value instanceof Collection;
+ }
+
+ private static List<String> azureQuirkValuesToRemove(Collection<?> listOfMaps, Attribute attribute) {
+ return listOfMaps.stream()
+ .map(item -> {
+ if (!(item instanceof Map)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but 'value' is not a list of maps");
+ }
+ return (Map<?,?>) item;
+ })
+ .map(item -> {
+ Attribute valueAttribute = attribute.getAttribute(VALUE_ATTRIBUTE_NAME);
+ Object itemValue = item.get(valueAttribute.getName());
+ if (!(itemValue instanceof String)) {
+ throw new IllegalArgumentException("Azure Remove Patch request quirk detected, but item 'value' is not a string");
+ }
+
+ return (String) itemValue;
+ })
+ .collect(toList());
Review Comment:
This is a bit ugly, but I assume we want to fail if we detect a different version of this non-spec request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org