Correct extraction of namespace and name from path

scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java

@@ -1,31 +1,27 @@
 package sonia.scm.web.protocol;
 
 import com.google.inject.Inject;
-import com.google.inject.Provider;
 import com.google.inject.Singleton;
 import lombok.extern.slf4j.Slf4j;
-import org.apache.shiro.SecurityUtils;
-import org.apache.shiro.subject.Subject;
+import org.apache.http.HttpStatus;
 import sonia.scm.PushStateDispatcher;
 import sonia.scm.filter.WebElement;
 import sonia.scm.repository.DefaultRepositoryProvider;
 import sonia.scm.repository.NamespaceAndName;
 import sonia.scm.repository.RepositoryNotFoundException;
-import sonia.scm.repository.RepositoryProvider;
 import sonia.scm.repository.api.RepositoryService;
 import sonia.scm.repository.api.RepositoryServiceFactory;
 import sonia.scm.repository.spi.HttpScmProtocol;
-import sonia.scm.util.HttpUtil;
 import sonia.scm.web.UserAgent;
 import sonia.scm.web.UserAgentParser;
 
+import javax.inject.Provider;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
-
-import static java.lang.Math.max;
+import java.util.Optional;
 
 @Singleton
 @WebElement(value = HttpProtocolServlet.PATTERN)
@@ -34,7 +30,6 @@ public class HttpProtocolServlet extends HttpServlet {
 
   public static final String PATTERN = "/repo/*";
 
-  private final RepositoryProvider repositoryProvider;
   private final RepositoryServiceFactory serviceFactory;
 
   private final Provider<HttpServletRequest> requestProvider;
@@ -42,48 +37,43 @@ public class HttpProtocolServlet extends HttpServlet {
   private final PushStateDispatcher dispatcher;
   private final UserAgentParser userAgentParser;
 
+  private final NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor;
+
   @Inject
-  public HttpProtocolServlet(RepositoryProvider repositoryProvider, RepositoryServiceFactory serviceFactory, Provider<HttpServletRequest> requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) {
-    this.repositoryProvider = repositoryProvider;
+  public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, Provider<HttpServletRequest> requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser, NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor) {
     this.serviceFactory = serviceFactory;
     this.requestProvider = requestProvider;
     this.dispatcher = dispatcher;
     this.userAgentParser = userAgentParser;
+    this.namespaceAndNameFromPathExtractor = namespaceAndNameFromPathExtractor;
   }
 
   @Override
-  protected void service(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
-    Subject subject = SecurityUtils.getSubject();
-
-
-    UserAgent userAgent = userAgentParser.parse(req);
+  protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
+    UserAgent userAgent = userAgentParser.parse(request);
     if (userAgent.isBrowser()) {
       log.trace("dispatch browser request for user agent {}", userAgent);
-      dispatcher.dispatch(req, resp, req.getRequestURI());
+      dispatcher.dispatch(request, response, request.getRequestURI());
     } else {
+      String pathInfo = request.getPathInfo();
+      Optional<NamespaceAndName> namespaceAndName = namespaceAndNameFromPathExtractor.fromUri(pathInfo);
+      if (namespaceAndName.isPresent()) {
+        service(request, response, namespaceAndName.get());
+      } else {
+        log.debug("namespace and name not found in request path {}", pathInfo);
+        response.setStatus(HttpStatus.SC_BAD_REQUEST);
+      }
+    }
+  }
 
-
-      String pathInfo = req.getPathInfo();
-      NamespaceAndName namespaceAndName = fromUri(pathInfo);
+  private void service(HttpServletRequest req, HttpServletResponse resp, NamespaceAndName namespaceAndName) throws IOException, ServletException {
     try (RepositoryService repositoryService = serviceFactory.create(namespaceAndName)) {
       requestProvider.get().setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repositoryService.getRepository());
       HttpScmProtocol protocol = repositoryService.getProtocol(HttpScmProtocol.class);
       protocol.serve(req, resp, getServletConfig());
     } catch (RepositoryNotFoundException e) {
-        resp.setStatus(404);
+      log.debug("Repository not found for namespace and name {}", namespaceAndName, e);
+      resp.setStatus(HttpStatus.SC_NOT_FOUND);
     }
   }
 }
-
-  private NamespaceAndName fromUri(String uri) {
-    if (uri.startsWith(HttpUtil.SEPARATOR_PATH)) {
-      uri = uri.substring(1);
-    }
-
-    String namespace = uri.substring(0, uri.indexOf(HttpUtil.SEPARATOR_PATH));
-    int endIndex = uri.indexOf(HttpUtil.SEPARATOR_PATH, uri.indexOf(HttpUtil.SEPARATOR_PATH) + 1);
-    String name = uri.substring(uri.indexOf(HttpUtil.SEPARATOR_PATH) + 1, max(endIndex, uri.length()));
-
-    return new NamespaceAndName(namespace, name);
-  }
-}

scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java

@@ -0,0 +1,33 @@
+package sonia.scm.web.protocol;
+
+import sonia.scm.repository.NamespaceAndName;
+import sonia.scm.util.HttpUtil;
+
+import java.util.Optional;
+
+import static java.util.Optional.empty;
+import static java.util.Optional.of;
+
+class NamespaceAndNameFromPathExtractor {
+  Optional<NamespaceAndName> fromUri(String uri) {
+    if (uri.startsWith(HttpUtil.SEPARATOR_PATH)) {
+      uri = uri.substring(1);
+    }
+
+    int endOfNamespace = uri.indexOf(HttpUtil.SEPARATOR_PATH);
+    if (endOfNamespace < 1) {
+      return empty();
+    }
+
+    String namespace = uri.substring(0, endOfNamespace);
+    int nameSeparatorIndex = uri.indexOf(HttpUtil.SEPARATOR_PATH, endOfNamespace + 1);
+    int nameIndex = nameSeparatorIndex > 0 ? nameSeparatorIndex : uri.length();
+    if (nameIndex == endOfNamespace + 1) {
+      return empty();
+    }
+
+    String name = uri.substring(endOfNamespace + 1, nameIndex);
+
+    return of(new NamespaceAndName(namespace, name));
+  }
+}

scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java

@@ -0,0 +1,124 @@
+package sonia.scm.web.protocol;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import sonia.scm.PushStateDispatcher;
+import sonia.scm.repository.DefaultRepositoryProvider;
+import sonia.scm.repository.NamespaceAndName;
+import sonia.scm.repository.Repository;
+import sonia.scm.repository.RepositoryNotFoundException;
+import sonia.scm.repository.api.RepositoryService;
+import sonia.scm.repository.api.RepositoryServiceFactory;
+import sonia.scm.repository.spi.HttpScmProtocol;
+import sonia.scm.web.UserAgent;
+import sonia.scm.web.UserAgentParser;
+
+import javax.inject.Provider;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Optional;
+
+import static java.util.Optional.of;
+import static org.mockito.AdditionalMatchers.not;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.mockito.MockitoAnnotations.initMocks;
+
+public class HttpProtocolServletTest {
+
+  private static final NamespaceAndName EXISTING_REPO = new NamespaceAndName("space", "repo");
+
+  @Mock
+  private RepositoryServiceFactory serviceFactory;
+  @Mock
+  private HttpServletRequest httpServletRequest;
+  @Mock
+  private PushStateDispatcher dispatcher;
+  @Mock
+  private UserAgentParser userAgentParser;
+  @Mock
+  private NamespaceAndNameFromPathExtractor namespaceAndNameFromPathExtractor;
+  @Mock
+  private Provider<HttpServletRequest> requestProvider;
+
+  @InjectMocks
+  private HttpProtocolServlet servlet;
+
+  @Mock
+  private RepositoryService repositoryService;
+  @Mock
+  private UserAgent userAgent;
+
+  @Mock
+  private HttpServletRequest request;
+  @Mock
+  private HttpServletResponse response;
+  @Mock
+  private HttpScmProtocol protocol;
+
+  @Before
+  public void init() throws RepositoryNotFoundException {
+    initMocks(this);
+    when(userAgentParser.parse(request)).thenReturn(userAgent);
+    when(userAgent.isBrowser()).thenReturn(false);
+    when(serviceFactory.create(not(eq(EXISTING_REPO)))).thenThrow(RepositoryNotFoundException.class);
+    when(serviceFactory.create(EXISTING_REPO)).thenReturn(repositoryService);
+    when(requestProvider.get()).thenReturn(httpServletRequest);
+  }
+
+  @Test
+  public void shouldDispatchBrowserRequests() throws ServletException, IOException {
+    when(userAgent.isBrowser()).thenReturn(true);
+    when(request.getRequestURI()).thenReturn("uri");
+
+    servlet.service(request, response);
+
+    verify(dispatcher).dispatch(request, response, "uri");
+  }
+
+  @Test
+  public void shouldHandleBadPaths() throws IOException, ServletException {
+    when(request.getPathInfo()).thenReturn("/space/name");
+    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(Optional.empty());
+
+    servlet.service(request, response);
+
+    verify(response).setStatus(400);
+  }
+
+  @Test
+  public void shouldHandleNotExistingRepository() throws RepositoryNotFoundException, IOException, ServletException {
+    when(request.getPathInfo()).thenReturn("/space/name");
+    NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name");
+    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(of(namespaceAndName));
+    doThrow(new RepositoryNotFoundException(namespaceAndName)).when(serviceFactory).create(namespaceAndName);
+
+    servlet.service(request, response);
+
+    verify(response).setStatus(404);
+  }
+
+  @Test
+  public void shouldDelegateToProvider() throws RepositoryNotFoundException, IOException, ServletException {
+    when(request.getPathInfo()).thenReturn("/space/name");
+    NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name");
+    when(namespaceAndNameFromPathExtractor.fromUri("/space/name")).thenReturn(of(namespaceAndName));
+    doReturn(repositoryService).when(serviceFactory).create(namespaceAndName);
+    Repository repository = new Repository();
+    when(repositoryService.getRepository()).thenReturn(repository);
+    when(repositoryService.getProtocol(HttpScmProtocol.class)).thenReturn(protocol);
+
+    servlet.service(request, response);
+
+    verify(httpServletRequest).setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repository);
+    verify(protocol).serve(request, response, null);
+    verify(repositoryService).close();
+  }
+}

scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java

@@ -0,0 +1,59 @@
+package sonia.scm.web.protocol;
+
+import org.junit.jupiter.api.DynamicNode;
+import org.junit.jupiter.api.DynamicTest;
+import org.junit.jupiter.api.TestFactory;
+import sonia.scm.repository.NamespaceAndName;
+
+import java.util.Optional;
+import java.util.stream.Stream;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.DynamicTest.dynamicTest;
+
+public class NamespaceAndNameFromPathExtractorTest {
+  @TestFactory
+  Stream<DynamicNode> shouldExtractCorrectNamespaceAndName() {
+    return Stream.of(
+      "/space/repo",
+      "/space/repo/",
+      "/space/repo/here",
+      "/space/repo/here/there",
+      "space/repo",
+      "space/repo/",
+      "space/repo/here/there"
+    ).map(this::createCorrectTest);
+  }
+
+  private DynamicTest createCorrectTest(String path) {
+    return dynamicTest(
+      "should extract correct namespace and name for path " + path,
+      () -> {
+        Optional<NamespaceAndName> namespaceAndName = new NamespaceAndNameFromPathExtractor().fromUri(path);
+
+        assertThat(namespaceAndName.get()).isEqualTo(new NamespaceAndName("space", "repo"));
+      }
+    );
+  }
+
+  @TestFactory
+  Stream<DynamicNode> shouldHandleMissingParts() {
+    return Stream.of(
+      "",
+      "/",
+      "/space",
+      "/space/"
+    ).map(this::createFailureTest);
+  }
+
+  private DynamicTest createFailureTest(String path) {
+    return dynamicTest(
+      "should not fail for wrong path " + path,
+      () -> {
+        Optional<NamespaceAndName> namespaceAndName = new NamespaceAndNameFromPathExtractor().fromUri(path);
+
+        assertThat(namespaceAndName.isPresent()).isFalse();
+      }
+    );
+  }
+}