Merge pull request apache#502 from markusmueller/fix-location-resourc…

…e-id-duplication Fixing duplication of resource id in location header
acryldata · Jan 17, 2024 · e02f29e · e02f29e
2 parents 69a975a + f7978ac
commit e02f29e
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 26 deletions.
diff --git a/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/tests/GroupsIT.java b/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/tests/GroupsIT.java
@@ -19,6 +19,7 @@
 
 package org.apache.directory.scim.compliance.tests;
 
+import io.restassured.response.ValidatableResponse;
 import org.apache.directory.scim.compliance.junit.EmbeddedServerExtension;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Order;
@@ -76,6 +77,7 @@ public void createGroup() {
     // retrieve the group by id
     get("/Groups/" + id)
       .statusCode(200)
+      .header("Location", matchesRegex(".*/Groups/" + id))
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:Group"),
         "id", not(emptyString()),
@@ -160,6 +162,7 @@ public void updateGroup() {
 
     // update Group,
     put("/Groups/" + id, updatedBody)
+      .header("Location", matchesRegex(".*/Groups/" + id))
       .statusCode(200)
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:Group"),
@@ -188,20 +191,22 @@ public void updateGroupWithPatch() {
         "\"path\": \"members[value eq \\\"" + userId + "\\\"]\"" +
       "}]}";
 
-    String id = post("/Groups", body)
+    ValidatableResponse response = post("/Groups", body)
       .statusCode(201)
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:Group"),
         "id", not(emptyString()),
         "displayName", is(groupName),
         "members[0].value", is(userId),
         "members[0].display", is(email)
-      )
-      .extract().jsonPath().get("id");
+      );
+    String id = response.extract().jsonPath().get("id");
+    response.header("Location", matchesRegex(".*/Groups/" + id));
 
     // update Group,
     patch("/Groups/" + id, patchBody)
       .statusCode(200)
+      .header("Location", matchesRegex(".*/Groups/" + id))
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:Group"),
         "members", empty()

diff --git a/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/tests/UsersIT.java b/scim-compliance-tests/src/main/java/org/apache/directory/scim/compliance/tests/UsersIT.java
@@ -19,6 +19,7 @@
 
 package org.apache.directory.scim.compliance.tests;
 
+import io.restassured.response.ValidatableResponse;
 import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.directory.scim.compliance.junit.EmbeddedServerExtension;
 import org.junit.jupiter.api.DisplayName;
@@ -103,7 +104,7 @@ public void createUser() {
         "\"active\":true" +
         "}";
 
-    String id = post("/Users", body)
+    ValidatableResponse response = post("/Users", body)
       .statusCode(201)
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:User"),
@@ -112,12 +113,15 @@ public void createUser() {
         "name.givenName", is(givenName),
         "name.familyName", is(familyName),
         "userName", equalToIgnoringCase(email)
-      )
-      .extract().jsonPath().get("id");
+      );
+
+    String id = response.extract().jsonPath().get("id");
+    response.header("Location", matchesRegex(".*/Users/" + id));
 
     // retrieve the user by id
     get("/Users/" + id)
       .statusCode(200)
+      .header("Location", matchesRegex(".*/Users/" + id))
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:User"),
         "active", is(true),
@@ -168,6 +172,7 @@ public void updateUser() {
 
     put("/Users/" + id, updatedBody)
       .statusCode(200)
+      .header("Location", matchesRegex(".*/Users/" + id))
       .body(
         "schemas", contains("urn:ietf:params:scim:schemas:core:2.0:User"),
         "active", is(true),
@@ -238,6 +243,7 @@ public void deactivateWithPatch() {
 
     patch("/Users/" + id, patchBody)
       .statusCode(200)
+      .header("Location", matchesRegex(".*/Users/" + id))
       .body(
         "active", is(false)
       );

diff --git a/...ver/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java b/...ver/src/main/java/org/apache/directory/scim/server/rest/BaseResourceTypeResourceImpl.java
@@ -6,7 +6,7 @@
 * to you under the Apache License, Version 2.0 (the
 * "License"); you may not use this file except in compliance
 * with the License.  You may obtain a copy of the License at
- 
+
 * http://www.apache.org/licenses/LICENSE-2.0
 
 * Unless required by applicable law or agreed to in writing,
@@ -24,6 +24,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
+import java.util.Objects;
 import java.util.Set;
 
 import jakarta.enterprise.inject.spi.CDI;
@@ -141,7 +142,7 @@ public Response getById(String id, AttributeReferenceListWrapper attributes, Att
     resource = attributesForDisplayThrowOnError(resource, attributeReferences, excludedAttributeReferences);
     return Response.ok()
                    .entity(resource)
-                   .location(buildLocationTag(resource))
+                   .location(uriInfo.getAbsolutePath())
                    .tag(etag)
                    .build();
   }
@@ -158,7 +159,7 @@ public Response query(AttributeReferenceListWrapper attributes, AttributeReferen
     else {
       searchRequest.setFilter(null);
     }
-    
+
     searchRequest.setSortBy(sortBy);
     searchRequest.setSortOrder(sortOrder);
     searchRequest.setStartIndex(startIndex);
@@ -188,11 +189,16 @@ public Response create(T resource, AttributeReferenceListWrapper attributes, Att
         log.debug("Exception thrown while processing attributes", e);
     }
 
+    Objects.requireNonNull(created.getId(), "Repository must supply an id for a resource");
+    URI location = uriInfo.getAbsolutePathBuilder()
+      .path(created.getId())
+      .build();
+
     return Response.status(Status.CREATED)
-                   .location(buildLocationTag(created))
-                   .tag(etag)
-                   .entity(created)
-                   .build();
+      .location(location)
+      .tag(etag)
+      .entity(created)
+      .build();
   }
 
   @Override
@@ -267,7 +273,6 @@ public Response delete(String id) throws ScimException, ResourceException {
   }
 
   private Response update(AttributeReferenceListWrapper attributes, AttributeReferenceListWrapper excludedAttributes, UpdateFunction<T> updateFunction) throws ScimException, ResourceException {
-
     Repository<T> repository = getRepositoryInternal();
 
     Set<AttributeReference> attributeReferences = AttributeReferenceListWrapper.getAttributeReferences(attributes);
@@ -283,7 +288,7 @@ private Response update(AttributeReferenceListWrapper attributes, AttributeRefer
 
     EntityTag etag = fromVersion(updated);
     return Response.ok(updated)
-      .location(buildLocationTag(updated))
+      .location(uriInfo.getAbsolutePath())
       .tag(etag)
       .build();
   }
@@ -313,17 +318,6 @@ private T processFilterAttributeExtensions(Repository<T> repository, T resource,
     return resource;
   }
 
-  private URI buildLocationTag(T resource) {
-    String id = resource.getId();
-    if (id == null) {
-      LOG.warn("Repository must supply an id for a resource");
-      id = "unknown";
-    }
-    return uriInfo.getAbsolutePathBuilder()
-                  .path(id)
-                  .build();
-  }
-
   private <T extends ScimResource> T attributesForDisplay(T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws AttributeException {
     if (!excludedAttributes.isEmpty()) {
       resource = attributeUtil.setExcludedAttributesForDisplay(resource, excludedAttributes);