From 6d659b8ac104740df4dce2a0c773ddabaa6ef307 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Mon, 10 Sep 2018 08:37:31 +0200 Subject: [PATCH] Correct extraction of namespace and name from path --- .../scm/web/protocol/HttpProtocolServlet.java | 60 ++++----- .../NamespaceAndNameFromPathExtractor.java | 33 +++++ .../web/protocol/HttpProtocolServletTest.java | 124 ++++++++++++++++++ ...NamespaceAndNameFromPathExtractorTest.java | 59 +++++++++ 4 files changed, 241 insertions(+), 35 deletions(-) create mode 100644 scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java create mode 100644 scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java create mode 100644 scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java diff --git a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java b/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java index dc25875d5b..d314628438 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java +++ b/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 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 requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) { - this.repositoryProvider = repositoryProvider; + public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, Provider 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 = req.getPathInfo(); - NamespaceAndName namespaceAndName = fromUri(pathInfo); - 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); + String pathInfo = request.getPathInfo(); + Optional 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); } } } - private NamespaceAndName fromUri(String uri) { - if (uri.startsWith(HttpUtil.SEPARATOR_PATH)) { - uri = uri.substring(1); + 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) { + log.debug("Repository not found for namespace and name {}", namespaceAndName, e); + resp.setStatus(HttpStatus.SC_NOT_FOUND); } - - 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); } } diff --git a/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java b/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java new file mode 100644 index 0000000000..b4315bc6ac --- /dev/null +++ b/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 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)); + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java new file mode 100644 index 0000000000..2d4f6b639b --- /dev/null +++ b/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 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(); + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java new file mode 100644 index 0000000000..be26ac89db --- /dev/null +++ b/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 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 = new NamespaceAndNameFromPathExtractor().fromUri(path); + + assertThat(namespaceAndName.get()).isEqualTo(new NamespaceAndName("space", "repo")); + } + ); + } + + @TestFactory + Stream 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 = new NamespaceAndNameFromPathExtractor().fromUri(path); + + assertThat(namespaceAndName.isPresent()).isFalse(); + } + ); + } +}