From 35fe53617058bba833e738c0ecd622c4a4bd4965 Mon Sep 17 00:00:00 2001
From: Sebastian Sdorra <sebastian.sdorra@cloudogu.com>
Date: Wed, 9 Jun 2021 13:09:25 +0200
Subject: [PATCH] Fix options request return internal server error (#1688)

---
 gradle/changelog/options_request.yaml         |   2 +
 .../main/java/sonia/scm/api/ErrorDtos.java    |  45 ++++++
 .../sonia/scm/api/FailureExceptionMapper.java |  84 ++++++++++++
 .../api/WebApplicationExceptionMapper.java    |   8 +-
 .../scm/api/FailureExceptionMapperTest.java   | 128 ++++++++++++++++++
 5 files changed, 260 insertions(+), 7 deletions(-)
 create mode 100644 gradle/changelog/options_request.yaml
 create mode 100644 scm-webapp/src/main/java/sonia/scm/api/ErrorDtos.java
 create mode 100644 scm-webapp/src/main/java/sonia/scm/api/FailureExceptionMapper.java
 create mode 100644 scm-webapp/src/test/java/sonia/scm/api/FailureExceptionMapperTest.java

diff --git a/gradle/changelog/options_request.yaml b/gradle/changelog/options_request.yaml
new file mode 100644
index 0000000000..0baf2bad26
--- /dev/null
+++ b/gradle/changelog/options_request.yaml
@@ -0,0 +1,2 @@
+- type: fixed
+  description: Options requests returning internal server errors ([#1685](https://github.com/scm-manager/scm-manager/issues/1685), [#1688](https://github.com/scm-manager/scm-manager/pull/1688))
diff --git a/scm-webapp/src/main/java/sonia/scm/api/ErrorDtos.java b/scm-webapp/src/main/java/sonia/scm/api/ErrorDtos.java
new file mode 100644
index 0000000000..85833a8061
--- /dev/null
+++ b/scm-webapp/src/main/java/sonia/scm/api/ErrorDtos.java
@@ -0,0 +1,45 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2020-present Cloudogu GmbH and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package sonia.scm.api;
+
+import org.slf4j.MDC;
+import sonia.scm.api.v2.resources.ErrorDto;
+
+import java.util.Collections;
+
+class ErrorDtos {
+
+  private ErrorDtos() {
+  }
+
+  static ErrorDto from(String code, Exception exception) {
+    ErrorDto errorDto = new ErrorDto();
+    errorDto.setMessage(exception.getMessage());
+    errorDto.setContext(Collections.emptyList());
+    errorDto.setErrorCode(code);
+    errorDto.setTransactionId(MDC.get("transaction_id"));
+    return errorDto;
+  }
+}
diff --git a/scm-webapp/src/main/java/sonia/scm/api/FailureExceptionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/FailureExceptionMapper.java
new file mode 100644
index 0000000000..2cd5f8e73a
--- /dev/null
+++ b/scm-webapp/src/main/java/sonia/scm/api/FailureExceptionMapper.java
@@ -0,0 +1,84 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2020-present Cloudogu GmbH and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package sonia.scm.api;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
+import org.jboss.resteasy.spi.Failure;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import sonia.scm.api.v2.resources.ErrorDto;
+import sonia.scm.web.VndMediaType;
+
+import javax.inject.Inject;
+import javax.inject.Provider;
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import java.util.Set;
+
+@javax.ws.rs.ext.Provider
+public class FailureExceptionMapper implements ExceptionMapper<Failure> {
+
+  private static final Logger LOG = LoggerFactory.getLogger(FailureExceptionMapper.class);
+
+  private static final Set<String> METHODS_WITHOUT_BODY = ImmutableSet.of("HEAD", "OPTIONS");
+
+  @VisibleForTesting
+  static final String ERROR_CODE = "2BSZikGOB1";
+
+  private final Provider<HttpServletRequest> requestProvider;
+
+  @Inject
+  public FailureExceptionMapper(Provider<HttpServletRequest> requestProvider) {
+    this.requestProvider = requestProvider;
+  }
+
+  @Override
+  public Response toResponse(Failure exception) {
+    if (exception.isLoggable()) {
+      LOG.warn("handle uncatched failure", exception);
+    }
+    Response.ResponseBuilder builder = builder(exception);
+    if (shouldAppendErrorDto()) {
+      ErrorDto errorDto = ErrorDtos.from(ERROR_CODE, exception);
+      builder.entity(errorDto).type(VndMediaType.ERROR_TYPE);
+    }
+    return builder.build();
+  }
+
+  private Response.ResponseBuilder builder(Failure exception) {
+    Response response = exception.getResponse();
+    if (response != null) {
+      return Response.fromResponse(response);
+    }
+    return Response.status(exception.getErrorCode());
+  }
+
+  private boolean shouldAppendErrorDto() {
+    String method = requestProvider.get().getMethod();
+    return !METHODS_WITHOUT_BODY.contains(method);
+  }
+}
diff --git a/scm-webapp/src/main/java/sonia/scm/api/WebApplicationExceptionMapper.java b/scm-webapp/src/main/java/sonia/scm/api/WebApplicationExceptionMapper.java
index 41ec6787ef..3c091fdb52 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/WebApplicationExceptionMapper.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/WebApplicationExceptionMapper.java
@@ -26,7 +26,6 @@ package sonia.scm.api;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-import org.slf4j.MDC;
 import sonia.scm.api.v2.resources.ErrorDto;
 import sonia.scm.web.VndMediaType;
 
@@ -34,7 +33,6 @@ import javax.ws.rs.WebApplicationException;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.ext.ExceptionMapper;
 import javax.ws.rs.ext.Provider;
-import java.util.Collections;
 
 @Provider
 public class WebApplicationExceptionMapper implements ExceptionMapper<WebApplicationException> {
@@ -47,12 +45,8 @@ public class WebApplicationExceptionMapper implements ExceptionMapper<WebApplica
   public Response toResponse(WebApplicationException exception) {
     LOG.trace("caught web application exception", exception);
 
-    ErrorDto errorDto = new ErrorDto();
-    errorDto.setMessage(exception.getMessage());
-    errorDto.setContext(Collections.emptyList());
-    errorDto.setErrorCode(ERROR_CODE);
-    errorDto.setTransactionId(MDC.get("transaction_id"));
 
+    ErrorDto errorDto = ErrorDtos.from(ERROR_CODE, exception);
     Response originalResponse = exception.getResponse();
 
     return Response.fromResponse(originalResponse)
diff --git a/scm-webapp/src/test/java/sonia/scm/api/FailureExceptionMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/FailureExceptionMapperTest.java
new file mode 100644
index 0000000000..90e295f96d
--- /dev/null
+++ b/scm-webapp/src/test/java/sonia/scm/api/FailureExceptionMapperTest.java
@@ -0,0 +1,128 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2020-present Cloudogu GmbH and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package sonia.scm.api;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.inject.util.Providers;
+import org.jboss.resteasy.mock.MockDispatcherFactory;
+import org.jboss.resteasy.mock.MockHttpRequest;
+import org.jboss.resteasy.mock.MockHttpResponse;
+import org.jboss.resteasy.spi.Dispatcher;
+import org.jboss.resteasy.spi.Failure;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.ws.rs.GET;
+import javax.ws.rs.HEAD;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.core.Response;
+import java.io.IOException;
+import java.net.URISyntaxException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.when;
+
+@ExtendWith(MockitoExtension.class)
+class FailureExceptionMapperTest {
+
+  @Mock
+  private HttpServletRequest request;
+
+  private Dispatcher dispatcher;
+
+  @BeforeEach
+  void setUpDispatcher() {
+    dispatcher = MockDispatcherFactory.createDispatcher();
+    dispatcher.getRegistry().addSingletonResource(new GreetingResource());
+
+    FailureExceptionMapper mapper = new FailureExceptionMapper(Providers.of(request));
+    dispatcher.getProviderFactory().registerProviderInstance(mapper);
+    dispatcher.getProviderFactory().registerProvider(FallbackExceptionMapper.class);
+  }
+
+  @Test
+  void shouldReturn200ForOptionsRequests() throws URISyntaxException {
+    MockHttpRequest request = MockHttpRequest.options("/");
+    MockHttpResponse response = dispatch(request);
+
+    assertThat(response.getStatus()).isEqualTo(200);
+  }
+
+  @Test
+  void shouldKeepStatusCodeFromFailure() throws URISyntaxException {
+    MockHttpRequest request = MockHttpRequest.head("/");
+    MockHttpResponse response = dispatch(request);
+
+    assertThat(response.getStatus()).isEqualTo(418);
+    assertThat(response.getOutput()).isEmpty();
+  }
+
+  @Test
+  void shouldAppendErrorDtoForMethodsWithBody() throws URISyntaxException, IOException {
+    MockHttpRequest request = MockHttpRequest.post("/");
+    MockHttpResponse response = dispatch(request);
+
+    assertThat(response.getStatus()).isEqualTo(418);
+
+    ObjectMapper mapper = new ObjectMapper();
+    JsonNode node = mapper.readTree(response.getOutput());
+    assertThat(node.get("errorCode").asText()).isEqualTo(FailureExceptionMapper.ERROR_CODE);
+  }
+
+  private MockHttpResponse dispatch(MockHttpRequest request) {
+    when(this.request.getMethod()).thenReturn(request.getHttpMethod());
+    MockHttpResponse response = new MockHttpResponse();
+    dispatcher.invoke(request, response);
+    return response;
+  }
+
+
+  @Path("/")
+  public static class GreetingResource {
+    @GET
+    public String greetings() {
+      return "Hello";
+    }
+
+    @HEAD
+    public Response head() {
+      throw new Failure(418);
+    }
+
+    @POST
+    public Response post() {
+      Failure failure = new Failure(418);
+      failure.setLoggable(true);
+      throw failure;
+    }
+  }
+
+}