From c216692eab43fa5fb32bc5a9c26b13e554648018 Mon Sep 17 00:00:00 2001
From: Sebastian Sdorra <sebastian.sdorra@cloudogu.com>
Date: Thu, 8 Feb 2018 22:36:54 +0100
Subject: [PATCH] #965 fixed handling of resources with spaces in its id

---
 .../it/JerseyGroupClientHandlerITCase.java    | 25 ++++++-
 .../sonia/scm/url/RestModelUrlProvider.java   |  4 +-
 .../scm/url/ModelUrlProviderTestBase.java     |  4 +-
 .../scm/url/RestModelUrlProviderTestBase.java | 15 ++++
 .../resources/AbstractManagerResource.java    | 32 ++++-----
 .../AbstractManagerResourceTest.java          | 71 +++++++++++++++++++
 6 files changed, 129 insertions(+), 22 deletions(-)
 create mode 100644 scm-webapp/src/test/java/sonia/scm/api/rest/resources/AbstractManagerResourceTest.java

diff --git a/scm-clients/scm-client-impl/src/test/java/sonia/scm/client/it/JerseyGroupClientHandlerITCase.java b/scm-clients/scm-client-impl/src/test/java/sonia/scm/client/it/JerseyGroupClientHandlerITCase.java
index 0650564741..08899945ad 100644
--- a/scm-clients/scm-client-impl/src/test/java/sonia/scm/client/it/JerseyGroupClientHandlerITCase.java
+++ b/scm-clients/scm-client-impl/src/test/java/sonia/scm/client/it/JerseyGroupClientHandlerITCase.java
@@ -35,11 +35,14 @@ package sonia.scm.client.it;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import org.junit.Test;
 import sonia.scm.client.ClientHandler;
+import sonia.scm.client.GroupClientHandler;
 import sonia.scm.client.JerseyClientSession;
-import sonia.scm.client.it.AbstractClientHandlerTestBase.ModifyTest;
 import sonia.scm.group.Group;
 
+import static sonia.scm.client.it.ClientTestUtil.createAdminSession;
+
 /**
  *
  * @author Sebastian Sdorra
@@ -99,4 +102,24 @@ public class JerseyGroupClientHandlerITCase
   {
     return new Group("xml", "group-" + number);
   }
+
+  /**
+   * Tests crud operations with a group which name contains spaces.
+   *
+   * @see <a href="https://bitbucket.org/sdorra/scm-manager/issues/965/modify-a-group-with-spaces-not-allowed>#965</a>
+   */
+  @Test
+  public void testGroupNamesWithSpaces() {
+    JerseyClientSession session = createAdminSession();
+    GroupClientHandler handler = session.getGroupHandler();
+
+    String name = "SCM Special Group";
+    handler.create(new Group("xml", name));
+
+    Group group = handler.get(name);
+    group.add("Some Member");
+    handler.modify(group);
+
+    handler.delete(name);
+  }
 }
diff --git a/scm-core/src/main/java/sonia/scm/url/RestModelUrlProvider.java b/scm-core/src/main/java/sonia/scm/url/RestModelUrlProvider.java
index 3974135a53..ece9027941 100644
--- a/scm-core/src/main/java/sonia/scm/url/RestModelUrlProvider.java
+++ b/scm-core/src/main/java/sonia/scm/url/RestModelUrlProvider.java
@@ -35,6 +35,7 @@ package sonia.scm.url;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import com.google.common.net.UrlEscapers;
 import sonia.scm.util.HttpUtil;
 
 /**
@@ -84,7 +85,8 @@ public class RestModelUrlProvider implements ModelUrlProvider
   @Override
   public String getDetailUrl(String name)
   {
-    return HttpUtil.append(base, name).concat(extension);
+    String escaped = UrlEscapers.urlPathSegmentEscaper().escape(name);
+    return HttpUtil.append(base, escaped).concat(extension);
   }
 
   //~--- fields ---------------------------------------------------------------
diff --git a/scm-core/src/test/java/sonia/scm/url/ModelUrlProviderTestBase.java b/scm-core/src/test/java/sonia/scm/url/ModelUrlProviderTestBase.java
index 14d38c1928..269615ab92 100644
--- a/scm-core/src/test/java/sonia/scm/url/ModelUrlProviderTestBase.java
+++ b/scm-core/src/test/java/sonia/scm/url/ModelUrlProviderTestBase.java
@@ -58,7 +58,7 @@ public abstract class ModelUrlProviderTestBase extends UrlTestBase
   public static final String MODEL_USERS = "users";
 
   /** Field description */
-  private static final String[] MODELS = new String[] { MODEL_REPOSITORY,
+  protected static final String[] MODELS = new String[] { MODEL_REPOSITORY,
           MODEL_USERS, MODEL_GROUPS };
 
   //~--- methods --------------------------------------------------------------
@@ -162,7 +162,7 @@ public abstract class ModelUrlProviderTestBase extends UrlTestBase
    *
    * @return
    */
-  private ModelUrlProvider createModelUrlProvider(String baseUrl, String model)
+  protected ModelUrlProvider createModelUrlProvider(String baseUrl, String model)
   {
     ModelUrlProvider urlProvider = null;
 
diff --git a/scm-core/src/test/java/sonia/scm/url/RestModelUrlProviderTestBase.java b/scm-core/src/test/java/sonia/scm/url/RestModelUrlProviderTestBase.java
index 2e7f25818f..03c93ff9aa 100644
--- a/scm-core/src/test/java/sonia/scm/url/RestModelUrlProviderTestBase.java
+++ b/scm-core/src/test/java/sonia/scm/url/RestModelUrlProviderTestBase.java
@@ -34,8 +34,11 @@ package sonia.scm.url;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import org.junit.Test;
 import sonia.scm.util.HttpUtil;
 
+import static org.junit.Assert.assertEquals;
+
 /**
  *
  * @author Sebastian Sdorra
@@ -156,4 +159,16 @@ public abstract class RestModelUrlProviderTestBase
     return createRestUrl(baseUrl,
                          model.concat(HttpUtil.SEPARATOR_PATH).concat(item));
   }
+
+  @Test
+  public void testGetDetailUrlWithSpaces()
+  {
+    String item = "Item with Spaces";
+
+    for (String model : MODELS)
+    {
+      assertEquals(getExpectedDetailUrl(BASEURL, model, "Item%20with%20Spaces"),
+        createModelUrlProvider(BASEURL, model).getDetailUrl(item));
+    }
+  }
 }
diff --git a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java
index bb9c626053..44d99a3636 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java
@@ -35,33 +35,27 @@ package sonia.scm.api.rest.resources;
 
 //~--- non-JDK imports --------------------------------------------------------
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.net.UrlEscapers;
 import org.apache.commons.beanutils.BeanComparator;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import sonia.scm.LastModifiedAware;
 import sonia.scm.Manager;
 import sonia.scm.ModelObject;
 import sonia.scm.api.rest.RestExceptionResult;
 import sonia.scm.security.ScmSecurityException;
 import sonia.scm.util.AssertUtil;
-import sonia.scm.util.HttpUtil;
 import sonia.scm.util.Util;
 
-//~--- JDK imports ------------------------------------------------------------
-
+import javax.ws.rs.core.*;
+import javax.ws.rs.core.Response.Status;
+import java.net.URI;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.Date;
 
-import javax.ws.rs.core.CacheControl;
-import javax.ws.rs.core.EntityTag;
-import javax.ws.rs.core.GenericEntity;
-import javax.ws.rs.core.Request;
-import javax.ws.rs.core.Response;
-import javax.ws.rs.core.Response.Status;
-import javax.ws.rs.core.UriInfo;
+//~--- JDK imports ------------------------------------------------------------
 
 /**
  *
@@ -140,18 +134,14 @@ public abstract class AbstractManagerResource<T extends ModelObject,
   {
     preCreate(item);
 
-    Response response = null;
+    Response response;
 
     try
     {
       manager.create(item);
 
       String id = getId(item);
-
-      id = HttpUtil.encode(id);
-      response = Response.created(
-        uriInfo.getAbsolutePath().resolve(
-          getPathPart().concat("/").concat(id))).build();
+      response = Response.created(location(uriInfo, id)).build();
     }
     catch (ScmSecurityException ex)
     {
@@ -167,6 +157,12 @@ public abstract class AbstractManagerResource<T extends ModelObject,
     return response;
   }
 
+  @VisibleForTesting
+  URI location(UriInfo uriInfo, String id) {
+    String escaped = UrlEscapers.urlPathSegmentEscaper().escape(id);
+    return uriInfo.getAbsolutePath().resolve(getPathPart().concat("/").concat(escaped));
+  }
+
   /**
    * Method description
    *
diff --git a/scm-webapp/src/test/java/sonia/scm/api/rest/resources/AbstractManagerResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/rest/resources/AbstractManagerResourceTest.java
new file mode 100644
index 0000000000..3cb7188f22
--- /dev/null
+++ b/scm-webapp/src/test/java/sonia/scm/api/rest/resources/AbstractManagerResourceTest.java
@@ -0,0 +1,71 @@
+package sonia.scm.api.rest.resources;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import sonia.scm.Manager;
+import sonia.scm.group.Group;
+
+import javax.ws.rs.core.GenericEntity;
+import javax.ws.rs.core.UriInfo;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Collection;
+
+import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AbstractManagerResourceTest {
+
+  @Mock
+  private UriInfo uriInfo;
+
+  @Mock
+  private Manager<Group, Exception> manager;
+
+  @Test
+  public void testLocation() throws URISyntaxException {
+    URI base = new URI("https://scm.scm-manager.org/");
+
+    TestManagerResource resource = new TestManagerResource(manager);
+    when(uriInfo.getAbsolutePath()).thenReturn(base);
+
+    URI uri = resource.location(uriInfo, "special-group");
+    assertEquals(new URI("https://scm.scm-manager.org/groups/special-group"), uri);
+  }
+
+  @Test
+  public void testLocationWithSpaces() throws URISyntaxException {
+    URI base = new URI("https://scm.scm-manager.org/");
+
+    TestManagerResource resource = new TestManagerResource(manager);
+    when(uriInfo.getAbsolutePath()).thenReturn(base);
+
+    URI uri = resource.location(uriInfo, "Scm Special Group");
+    assertEquals(new URI("https://scm.scm-manager.org/groups/Scm%20Special%20Group"), uri);
+  }
+
+  private static class TestManagerResource extends AbstractManagerResource<Group, Exception> {
+
+    private TestManagerResource(Manager<Group, Exception> manager) {
+      super(manager);
+    }
+
+    @Override
+    protected GenericEntity<Collection<Group>> createGenericEntity(Collection<Group> items) {
+      return null;
+    }
+
+    @Override
+    protected String getId(Group group) {
+      return group.getId();
+    }
+
+    @Override
+    protected String getPathPart() {
+      return "groups";
+    }
+  }
+}