Responding with 404 when updating an immutable attribute of a non-exi… (

#235) …sting resource Currently when doing a Patch of an immutable attribute of a non-existing resource the status code is 400. But it should be a 404, because the validation of the body should happen after the validation of the path. Summary: PATCH /Groups/nonexisting ``` { "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp" ], "Operations": [ { "op": "remove", "path": "displayName", "value": "aName" } ] } ``` Current response: ``` { "status": "400", "scimType": "invalidValue", "schemas": [ "urn:ietf:params:scim:api:messages:2.0:Error" ], "detail": "Attribute with name urn:ietf:params:scim:schemas:core:2.0:Group:displayName is required and cannot not be removed" } ``` After patch applied: ``` { "status": "404", "schemas": [ "urn:ietf:params:scim:api:messages:2.0:Error" ], "detail": "HTTP 404 Not Found" } ```
SAP · Aug 6, 2024 · 0e625d2 · 0e625d2
1 parent bab180d
commit 0e625d2
Show file tree

Hide file tree

Showing 4 changed files with 165 additions and 6 deletions.
diff --git a/scimono-server/src/main/java/com/sap/scimono/api/Groups.java b/scimono-server/src/main/java/com/sap/scimono/api/Groups.java
@@ -16,7 +16,6 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 import java.util.UUID;
 
 import javax.validation.Valid;
@@ -191,6 +190,13 @@ public Response patchGroup(@PathParam("id") final String groupId, final PatchBod
     if (patchBody == null) {
       throw new InvalidInputException(NOT_VALID_INPUTS);
     }
+
+    Group groupFromDb = groupAPI.getGroup(groupId);
+
+    if (groupFromDb == null) {
+      throw new ResourceNotFoundException(RESOURCE_TYPE_GROUP, groupId);
+    }
+
     PatchValidationFramework validationFramework = PatchValidationFramework.groupsFramework(schemaAPI, resourceTypesAPI, groupAPI);
     validationFramework.validate(patchBody);
 

diff --git a/scimono-server/src/main/java/com/sap/scimono/api/Users.java b/scimono-server/src/main/java/com/sap/scimono/api/Users.java
@@ -16,7 +16,6 @@
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Set;
 import java.util.UUID;
 
 import javax.validation.Valid;
@@ -222,6 +221,12 @@ public Response patchUser(@PathParam("id") final String userId, final PatchBody
     if (patchBody == null) {
       throw new InvalidInputException(NOT_VALID_INPUTS);
     }
+
+    User userFromDb = usersAPI.getUser(userId);
+    if (userFromDb == null) {
+      throw new ResourceNotFoundException(RESOURCE_TYPE_USER, userId);
+    }
+
     PatchValidationFramework validationFramework = PatchValidationFramework.usersFramework(schemaAPI, resourceTypesAPI, usersAPI);
     validationFramework.validate(patchBody);
 

diff --git a/scimono-server/src/test/java/com/sap/scimono/api/GroupsTest.java b/scimono-server/src/test/java/com/sap/scimono/api/GroupsTest.java
@@ -1,34 +1,123 @@
 package com.sap.scimono.api;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.sap.scimono.SCIMApplication;
+import com.sap.scimono.callback.groups.GroupsCallback;
+import com.sap.scimono.callback.schemas.SchemasCallback;
+import com.sap.scimono.entity.Group;
+import com.sap.scimono.entity.patch.PatchBody;
+import com.sap.scimono.entity.patch.PatchOperation;
+import com.sap.scimono.entity.schema.Attribute;
 import com.sap.scimono.exception.InvalidInputException;
+import com.sap.scimono.exception.ResourceNotFoundException;
+
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.jupiter.api.DisplayName;
+import org.mockito.Mockito;
 
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.Set;
 import java.util.UUID;
 
 public class GroupsTest {
 
   private Groups groups;
 
+  private ObjectMapper mapper;
+  private SchemasCallback schemasCallbackMock = Mockito.mock(SchemasCallback.class, Mockito.CALLS_REAL_METHODS);
+  private GroupsCallback groupsCallback = Mockito.mock(GroupsCallback.class, Mockito.CALLS_REAL_METHODS);
+
+  private final String PATCH_OP_SCHEMA = "urn:ietf:params:scim:api:messages:2.0:PatchOp";
+
   @Before
   public void setup() {
+    mapper = new ObjectMapper();
     SCIMApplication scimApplication = new SCIMApplication() {
 
+      @Override
+      public SchemasCallback getSchemasCallback() {
+        return schemasCallbackMock;
+      }
+
+      @Override
+      public GroupsCallback getGroupsCallback() {
+        return groupsCallback;
+      }
+
     };
     groups = new Groups(scimApplication, null);
   }
 
   @Test(expected = InvalidInputException.class)
   public void testUpdateGroupWithEmptyBody() {
-    String userId = String.valueOf(UUID.randomUUID());
-    groups.updateGroup(userId, null);
+    String groupId = String.valueOf(UUID.randomUUID());
+    groups.updateGroup(groupId, null);
   }
 
   @Test(expected = InvalidInputException.class)
   public void testPatchGroupWithEmptyBody() {
-    String userId = String.valueOf(UUID.randomUUID());
-    groups.patchGroup(userId, null);
+    String groupId = String.valueOf(UUID.randomUUID());
+    groups.patchGroup(groupId, null);
+  }
+
+  @Test(expected = ResourceNotFoundException.class)
+  @DisplayName("Test patch group with non existing resource and remove operation on a not removable attribute. The existence of the resource given in the path should be validated first. Expected ResourceNotFoundException (404).")
+  public void testPatchGroupNonExistingResource() throws JsonProcessingException {
+    Mockito.doNothing().when(groupsCallback).patchGroup(Mockito.any(), Mockito.any(), Mockito.any());
+    Mockito.doReturn(new Attribute.Builder().required(true).build()).when(schemasCallbackMock).getAttribute(Mockito.any());
+    Mockito.doReturn(new ArrayList<>()).when(schemasCallbackMock).getCustomSchemas();
+    String groupId = String.valueOf(UUID.randomUUID());
+    Set<String> schemas = new HashSet<>();
+    schemas.add(PATCH_OP_SCHEMA);
+
+
+    JsonNode valueDisplayName = getValueDisplayName();
+    PatchOperation patchOperation1 = new PatchOperation.Builder()
+        .setOp(PatchOperation.Type.REMOVE)
+        .setPath("displayName")
+        .setValue(valueDisplayName)
+        .build();
+    PatchBody patchBody = new PatchBody.Builder()
+        .addOperation(patchOperation1)
+        .setSchemas(schemas)
+        .build();
+    groups.patchGroup(groupId, patchBody);
+  }
+
+  @Test(expected = InvalidInputException.class)
+  @DisplayName("Test patch group with existing resource and remove operation on a not removable attribute. Expected InvalidInputException (400) since this is not allowed.")
+  public void testPatchGroupExistingResource() throws JsonProcessingException {
+    Mockito.doNothing().when(groupsCallback).patchGroup(Mockito.any(), Mockito.any(), Mockito.any());
+    Mockito.doReturn(new Attribute.Builder().required(true).build()).when(schemasCallbackMock).getAttribute(Mockito.any());
+    Mockito.doReturn(new Group.Builder().build()).when(groupsCallback).getGroup(Mockito.any());
+    Mockito.doReturn(new ArrayList<>()).when(schemasCallbackMock).getCustomSchemas();
+    String groupId = String.valueOf(UUID.randomUUID());
+    Set<String> schemas = new HashSet<>();
+    schemas.add(PATCH_OP_SCHEMA);
+
+
+    JsonNode valueDisplayName = getValueDisplayName();
+    PatchOperation patchOperation1 = new PatchOperation.Builder()
+        .setOp(PatchOperation.Type.REMOVE)
+        .setPath("displayName")
+        .setValue(valueDisplayName)
+        .build();
+    PatchBody patchBody = new PatchBody.Builder()
+        .addOperation(patchOperation1)
+        .setSchemas(schemas)
+        .build();
+    groups.patchGroup(groupId, patchBody);
+  }
+
+
+
+  private JsonNode getValueDisplayName() throws JsonProcessingException {
+    JsonNode stringValue = mapper.readTree("\"displayName\"");
+    return stringValue;
   }
 
 }
diff --git a/scimono-server/src/test/java/com/sap/scimono/api/UsersTest.java b/scimono-server/src/test/java/com/sap/scimono/api/UsersTest.java
@@ -8,6 +8,7 @@
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.jupiter.api.DisplayName;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
@@ -17,9 +18,12 @@
 import com.sap.scimono.SCIMApplication;
 import com.sap.scimono.callback.schemas.SchemasCallback;
 import com.sap.scimono.callback.users.UsersCallback;
+import com.sap.scimono.entity.User;
 import com.sap.scimono.entity.patch.PatchBody;
 import com.sap.scimono.entity.patch.PatchOperation;
+import com.sap.scimono.entity.schema.Attribute;
 import com.sap.scimono.exception.InvalidInputException;
+import com.sap.scimono.exception.ResourceNotFoundException;
 
 public class UsersTest {
 
@@ -67,6 +71,7 @@ public void testPatchUserWithEmptyBody() {
   public void testPatchUserActivate() throws JsonProcessingException {
     Mockito.doNothing().when(usersCallbackMock).patchUser(Mockito.any(), Mockito.any(), Mockito.any());
     Mockito.doReturn(new ArrayList<>()).when(schemasCallbackMock).getCustomSchemas();
+    Mockito.doReturn(new User.Builder().build()).when(usersCallbackMock).getUser(Mockito.any());
     String userId = String.valueOf(UUID.randomUUID());
     Set<String> schemas = new HashSet<>();
     schemas.add(PATCH_OP_SCHEMA);
@@ -89,9 +94,63 @@ public void testPatchUserActivate() throws JsonProcessingException {
     Assert.assertEquals(patchBody, patchBodyCaptor.getValue());
   }
 
+  @Test(expected = ResourceNotFoundException.class)
+  @DisplayName("Test patch user with non existing resource and remove operation on a not removable attribute. The existence of the resource given in the path should be validated first. Expected ResourceNotFoundException (404).")
+  public void testPatchUserNonExistingResource() throws JsonProcessingException {
+    Mockito.doNothing().when(usersCallbackMock).patchUser(Mockito.any(), Mockito.any(), Mockito.any());
+    Mockito.doReturn(new Attribute.Builder().required(true).build()).when(schemasCallbackMock).getAttribute(Mockito.any());
+    Mockito.doReturn(new ArrayList<>()).when(schemasCallbackMock).getCustomSchemas();
+    String userId = String.valueOf(UUID.randomUUID());
+    Set<String> schemas = new HashSet<>();
+    schemas.add(PATCH_OP_SCHEMA);
+
+
+    JsonNode valueDisplayName = getValueDisplayName();
+    PatchOperation patchOperation1 = new PatchOperation.Builder()
+        .setOp(PatchOperation.Type.REMOVE)
+        .setPath("displayName")
+        .setValue(valueDisplayName)
+        .build();
+    PatchBody patchBody = new PatchBody.Builder()
+        .addOperation(patchOperation1)
+        .setSchemas(schemas)
+        .build();
+    users.patchUser(userId, patchBody);
+  }
+
+  @Test(expected = InvalidInputException.class)
+  @DisplayName("Test patch user with existing resource and remove operation on a not removable attribute. Expected InvalidInputException (400) since this is not allowed.")
+  public void testPatchUserExistingResource() throws JsonProcessingException {
+    Mockito.doNothing().when(usersCallbackMock).patchUser(Mockito.any(), Mockito.any(), Mockito.any());
+    Mockito.doReturn(new Attribute.Builder().required(true).build()).when(schemasCallbackMock).getAttribute(Mockito.any());
+    Mockito.doReturn(new User.Builder().build()).when(usersCallbackMock).getUser(Mockito.any());
+    Mockito.doReturn(new ArrayList<>()).when(schemasCallbackMock).getCustomSchemas();
+    String userId = String.valueOf(UUID.randomUUID());
+    Set<String> schemas = new HashSet<>();
+    schemas.add(PATCH_OP_SCHEMA);
+
+
+    JsonNode valueDisplayName = getValueDisplayName();
+    PatchOperation patchOperation1 = new PatchOperation.Builder()
+        .setOp(PatchOperation.Type.REMOVE)
+        .setPath("displayName")
+        .setValue(valueDisplayName)
+        .build();
+    PatchBody patchBody = new PatchBody.Builder()
+        .addOperation(patchOperation1)
+        .setSchemas(schemas)
+        .build();
+    users.patchUser(userId, patchBody);
+  }
+
   private JsonNode getValueTrue() throws JsonProcessingException {
     JsonNode boolValue = mapper.readTree("true");
     return boolValue;
   }
 
+  private JsonNode getValueDisplayName() throws JsonProcessingException {
+    JsonNode stringValue = mapper.readTree("\"displayName\"");
+    return stringValue;
+  }
+
 }