Add support for group membership replace ops (#61)

Author: papertigers

core/src/in_memory_provider_store.rs

@@ -1347,12 +1347,12 @@ mod test {
             .await
             .unwrap();
 
-        let patched_group2: StoredParts<Group> =
+        let patched_group5: StoredParts<Group> =
             result_as_resource(result).await.unwrap();
-        group_is_durably_stored(&ctx, &patched_group2.resource).await;
+        group_is_durably_stored(&ctx, &patched_group5.resource).await;
 
         let members =
-            patched_group2.resource.members.expect("group has members");
+            patched_group5.resource.members.expect("group has members");
         assert_eq!(members.len(), 1);
         let membership_ids: Vec<_> = members
             .into_iter()
@@ -1364,5 +1364,58 @@ mod test {
             membership_ids
                 .contains(&(jim.id.clone(), ResourceType::User.to_string()))
         );
+
+        // Replace group members via replace op
+
+        let body = json!({
+          "schemas": [
+            "urn:ietf:params:scim:api:messages:2.0:PatchOp"
+          ],
+          "Operations": [
+            {
+              "op": "replace",
+              "path": "members",
+              "value": [
+                {
+                  "value": dwight.id,
+                  "display": dwight.name
+                },
+                {
+                  "value": dwight.id,
+                  "display": dwight.name
+                },
+              ]
+            }
+          ]
+        });
+
+        let result = ctx
+            .client
+            .patch(format!(
+                "{}/Groups/{}",
+                ctx.base_url, patched_group.resource.id
+            ))
+            .json(&body)
+            .send()
+            .await
+            .unwrap();
+
+        let patched_group6: StoredParts<Group> =
+            result_as_resource(result).await.unwrap();
+        group_is_durably_stored(&ctx, &patched_group6.resource).await;
+
+        let members =
+            patched_group6.resource.members.expect("group has members");
+        assert_eq!(members.len(), 1);
+        let membership_ids: Vec<_> = members
+            .into_iter()
+            .map(|member| {
+                (member.value.unwrap(), member.resource_type.unwrap())
+            })
+            .collect();
+        assert!(
+            membership_ids
+                .contains(&(dwight.id.clone(), ResourceType::User.to_string()))
+        );
     }
 }

core/src/patch.rs

@@ -19,8 +19,7 @@ pub enum PatchRequestError {
 #[serde(tag = "op")]
 pub enum PatchOp {
     #[serde(rename = "replace")]
-    // NB: replace ops have optional paths but we don't support it yet
-    Replace { value: serde_json::Value },
+    Replace { path: Option<String>, value: serde_json::Value },
     #[serde(rename = "add")]
     Add { path: Option<String>, value: serde_json::Value },
     #[serde(rename = "remove")]
@@ -58,7 +57,7 @@ impl PatchRequest {
         let mut updated_user = stored_user.clone();
 
         for patch_op in &self.operations {
-            let PatchOp::Replace { value } = patch_op else {
+            let PatchOp::Replace { path: _, value } = patch_op else {
                 return Err(PatchRequestError::Unsupported(
                     "only the replace op is supported for users".to_string(),
                 ));
@@ -94,9 +93,11 @@ impl PatchRequest {
 
         for patch_op in &self.operations {
             match patch_op {
-                PatchOp::Replace { value } => {
-                    apply_group_replace_op(value, &mut updated_group)?
-                }
+                PatchOp::Replace { path, value } => apply_group_replace_op(
+                    path.as_ref(),
+                    value,
+                    &mut updated_group,
+                )?,
                 PatchOp::Add { path, value } => apply_group_add_op(
                     path.as_deref(),
                     value,
@@ -113,31 +114,80 @@ impl PatchRequest {
 }
 
 fn apply_group_replace_op(
+    path: Option<&String>,
     value: &serde_json::Value,
     group: &mut StoredParts<Group>,
 ) -> Result<(), PatchRequestError> {
-    #[derive(Debug, Deserialize)]
-    #[serde(rename_all = "camelCase")]
-    struct DisplayName {
-        id: String,
-        display_name: String,
-    }
+    match path {
+        // Validate that we are replacing a groups members
+        Some(path) => {
+            #[derive(Debug, Deserialize)]
+            #[serde(rename_all = "camelCase")]
+            struct Member {
+                value: String,
+            }
 
-    let Ok(change) = serde_json::from_value::<DisplayName>(value.clone())
-    else {
-        return Err(PatchRequestError::Unsupported(
-            "only replacing a groups displayName is supported".to_string(),
-        ));
-    };
+            if !path.eq_ignore_ascii_case("members") {
+                return Err(PatchRequestError::Unsupported(
+                    "only replacing a groups members path is supported"
+                        .to_string(),
+                ));
+            }
 
-    if !group.resource.id.eq_ignore_ascii_case(&change.id) {
-        return Err(PatchRequestError::Invalid(format!(
-            "unexpected group id {}",
-            change.id
-        )));
-    }
+            let Ok(patch_members) =
+                serde_json::from_value::<Vec<Member>>(value.clone())
+            else {
+                return Err(PatchRequestError::Invalid(
+                    "members being replaced in a group require a value field"
+                        .to_string(),
+                ));
+            };
 
-    group.resource.display_name = change.display_name;
+            let new_members: IdOrdMap<GroupMember> = patch_members
+                .into_iter()
+                .map(|member| GroupMember {
+                    resource_type: Some(ResourceType::User.to_string()),
+                    value: Some(member.value),
+                })
+                .collect();
+            group.resource.members =
+                (!new_members.is_empty()).then_some(new_members);
+        }
+
+        // RFC 7664 § 3.5.2.3:
+        //
+        // If the "path" parameter is omitted, the target is assumed to be the
+        // resource itself. In this case, the "value" attribute SHALL contain a
+        // list of one or more attributes that are to be replaced.
+        None => {
+            #[derive(Debug, Deserialize)]
+            #[serde(rename_all = "camelCase")]
+            struct DisplayName {
+                id: String,
+                display_name: String,
+            }
+
+            // We currently only support changing a display name
+            let Ok(change) =
+                serde_json::from_value::<DisplayName>(value.clone())
+            else {
+                return Err(PatchRequestError::Unsupported(
+                    "only replacing a displayName is supported when not
+                     including a path"
+                        .to_string(),
+                ));
+            };
+
+            if !group.resource.id.eq_ignore_ascii_case(&change.id) {
+                return Err(PatchRequestError::Invalid(format!(
+                    "unexpected group id {}",
+                    change.id
+                )));
+            }
+
+            group.resource.display_name = change.display_name;
+        }
+    }
 
     Ok(())
 }
@@ -285,6 +335,29 @@ mod test {
         serde_json::from_value::<PatchRequest>(json).unwrap();
     }
 
+    #[test]
+    fn test_parse_group_members_replace_op() {
+        let json = json!({
+          "schemas": [
+            "urn:ietf:params:scim:api:messages:2.0:PatchOp"
+          ],
+          "Operations": [
+            {
+              "op": "replace",
+              "path": "members",
+              "value": [
+                {
+                  "value": "abf4dd94-a4c0-4f67-89c9-76b03340cb9b",
+                  "display": "dakota@example.com"
+                }
+              ]
+            }
+          ]
+        });
+
+        serde_json::from_value::<PatchRequest>(json).unwrap();
+    }
+
     #[test]
     fn test_parse_group_membership_ops() {
         let json = json!({