From b8897b273afeb31eb6c6cdb2c8eb6dd646e0028e Mon Sep 17 00:00:00 2001
From: Sebastian Sdorra <sebastian.sdorra@cloudogu.com>
Date: Mon, 30 Jul 2018 16:13:17 +0200
Subject: [PATCH] do not use IllegalArgumentException for parameter validation

---
 .../resources/AuthenticationRequestDto.java   |  8 +--
 .../v2/resources/AuthenticationResource.java  |  6 +-
 .../resources/AuthenticationResourceTest.java | 59 ++++++++++++++++++-
 3 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationRequestDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationRequestDto.java
index 951863590d..3b9f6e5c18 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationRequestDto.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationRequestDto.java
@@ -1,7 +1,6 @@
 package sonia.scm.api.v2.resources;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 
 import javax.ws.rs.FormParam;
@@ -45,9 +44,8 @@ public class AuthenticationRequestDto {
     return scope;
   }
 
-  public void validate() {
-    Preconditions.checkArgument(!Strings.isNullOrEmpty(grantType), "grant_type parameter is required");
-    Preconditions.checkArgument(!Strings.isNullOrEmpty(username), "username parameter is required");
-    Preconditions.checkArgument(!Strings.isNullOrEmpty(password), "password parameter is required");
+  public boolean isValid() {
+    // password is currently the only valid grant_type
+    return "password".equals(grantType) && !Strings.isNullOrEmpty(username) && !Strings.isNullOrEmpty(password);
   }
 }
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java
index bf6bc5d76b..ac2f8a05d1 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/AuthenticationResource.java
@@ -29,7 +29,7 @@ public class AuthenticationResource {
 
   private static final Logger LOG = LoggerFactory.getLogger(AuthenticationResource.class);
 
-  public static final String PATH = "v2/auth";
+  static final String PATH = "v2/auth";
 
   private final AccessTokenBuilderFactory tokenBuilderFactory;
   private final AccessTokenCookieIssuer cookieIssuer;
@@ -81,7 +81,9 @@ public class AuthenticationResource {
     HttpServletResponse response,
     AuthenticationRequestDto authentication
   ) {
-    authentication.validate();
+    if (!authentication.isValid()) {
+      return Response.status(Response.Status.BAD_REQUEST).build();
+    }
 
     Response res;
     Subject subject = SecurityUtils.getSubject();
diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java
index d84a117a8a..b3f023e78d 100644
--- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java
+++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/AuthenticationResourceTest.java
@@ -80,6 +80,35 @@ public class AuthenticationResourceTest {
     "\t\"password\": \"doesNotMatter\"\n" +
     "}";
 
+  private static final String AUTH_JSON_WITHOUT_USERNAME = String.join("\n",
+    "{",
+    "\"grant_type\": \"password\",",
+    "\"password\": \"tricia123\"",
+    "}"
+  );
+
+  private static final String AUTH_JSON_WITHOUT_PASSWORD = String.join("\n",
+    "{",
+    "\"grant_type\": \"password\",",
+    "\"username\": \"trillian\"",
+    "}"
+  );
+
+  private static final String AUTH_JSON_WITHOUT_GRANT_TYPE = String.join("\n",
+    "{",
+    "\"username\": \"trillian\",",
+    "\"password\": \"tricia123\"",
+    "}"
+  );
+
+  private static final String AUTH_JSON_WITH_INVALID_GRANT_TYPE = String.join("\n",
+    "{",
+    "\"grant_type\": \"el speciale\",",
+    "\"username\": \"trillian\",",
+    "\"password\": \"tricia123\"",
+    "}"
+  );
+
   @Before
   public void prepareEnvironment() {
     AuthenticationResource authenticationResource = new AuthenticationResource(accessTokenBuilderFactory, cookieIssuer);
@@ -122,7 +151,6 @@ public class AuthenticationResourceTest {
 
   @Test
   public void shouldNotAuthNonexistingUser() throws URISyntaxException {
-
     MockHttpRequest request = getMockHttpRequest(AUTH_JSON_NOT_EXISTING_USER);
     MockHttpResponse response = new MockHttpResponse();
 
@@ -131,6 +159,35 @@ public class AuthenticationResourceTest {
     assertEquals(HttpServletResponse.SC_UNAUTHORIZED, response.getStatus());
   }
 
+  @Test
+  public void shouldReturnBadStatusIfPasswordParameterIsMissing() throws URISyntaxException {
+    shouldReturnBadRequest(AUTH_JSON_WITHOUT_USERNAME);
+  }
+
+  @Test
+  public void shouldReturnBadStatusIfUsernameParameterIsMissing() throws URISyntaxException {
+    shouldReturnBadRequest(AUTH_JSON_WITHOUT_PASSWORD);
+  }
+
+  @Test
+  public void shouldReturnBadStatusIfGrantTypeParameterIsMissing() throws URISyntaxException {
+    shouldReturnBadRequest(AUTH_JSON_WITHOUT_GRANT_TYPE);
+  }
+
+  @Test
+  public void shouldReturnBadStatusIfGrantTypeParameterIsInvalid() throws URISyntaxException {
+    shouldReturnBadRequest(AUTH_JSON_WITH_INVALID_GRANT_TYPE);
+  }
+
+  private void shouldReturnBadRequest(String requestBody) throws URISyntaxException {
+    MockHttpRequest request = getMockHttpRequest(requestBody);
+    MockHttpResponse response = new MockHttpResponse();
+
+    dispatcher.invoke(request, response);
+
+    assertEquals(HttpServletResponse.SC_BAD_REQUEST, response.getStatus());
+  }
+
   private MockHttpRequest getMockHttpRequest(String jsonPayload) throws URISyntaxException {
     MockHttpRequest request = MockHttpRequest.post("/" + AuthenticationResource.PATH + "/access_token");