Merge pull request #5694 from vespa-engine/bjorncs/json-security-filter-base

Bjorncs/json security filter base

10 files changed, 192 insertions, 52 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java
index e3df55a9c85..5166f53c6d2 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/AthenzPrincipalFilter.java
@@ -58,7 +58,7 @@ public class AthenzPrincipalFilter extends CorsRequestFilterBase {
     }
 
     @Override
-    public Optional<ErrorResponse> filter(DiscFilterRequest request) {
+    public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) {
         try {
             Optional<AthenzPrincipal> certificatePrincipal = getClientCertificate(request)
                     .map(AthenzIdentities::from)
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java
index 909051dcefc..910cf05b156 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/athenz/filter/UserAuthWithAthenzPrincipalFilter.java
@@ -48,7 +48,7 @@ public class UserAuthWithAthenzPrincipalFilter extends AthenzPrincipalFilter {
     }
 
     @Override
-    public Optional<ErrorResponse> filter(DiscFilterRequest request) {
+    public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) {
         if (request.getMethod().equals("OPTIONS")) return Optional.empty(); // Skip authentication on OPTIONS - required for Javascript CORS
 
         try {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
index 41b4091f836..0b1b88c4389 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
@@ -78,7 +78,7 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase {
     // NOTE: Be aware of the ordering of the path pattern matching. Semantics may change if the patterns are evaluated
     //       in different order.
     @Override
-    public Optional<ErrorResponse> filter(DiscFilterRequest request) {
+    public Optional<ErrorResponse> filterRequest(DiscFilterRequest request) {
         Method method = getMethod(request);
         if (isWhiteListedMethod(method)) return Optional.empty();
 
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 22d487628e7..c7a3cf76085 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -746,7 +746,7 @@ public class ApplicationApiTest extends ControllerContainerTest {
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", POST)
                                       .userIdentity(unauthorizedUser)
                                       .nToken(N_TOKEN),
-                              "{\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
+                              "{\n  \"code\" : 403,\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
                               403);
 
         // (Create it with the right tenant id)
@@ -761,13 +761,13 @@ public class ApplicationApiTest extends ControllerContainerTest {
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1/environment/prod/region/us-west-1/instance/default/deploy", POST)
                                       .data(entity)
                                       .userIdentity(USER_ID),
-                              "{\n  \"message\" : \"'user.myuser' is not a Screwdriver identity. Only Screwdriver is allowed to deploy to this environment.\"\n}",
+                              "{\n  \"code\" : 403,\n  \"message\" : \"'user.myuser' is not a Screwdriver identity. Only Screwdriver is allowed to deploy to this environment.\"\n}",
                               403);
 
         // Deleting an application for an Athens domain the user is not admin for is disallowed
         tester.assertResponse(request("/application/v4/tenant/tenant1/application/application1", DELETE)
                                       .userIdentity(unauthorizedUser),
-                              "{\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
+                              "{\n  \"code\" : 403,\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
                               403);
 
         // (Deleting it with the right tenant id)
@@ -781,7 +781,7 @@ public class ApplicationApiTest extends ControllerContainerTest {
         tester.assertResponse(request("/application/v4/tenant/tenant1", PUT)
                                       .data("{\"athensDomain\":\"domain1\", \"property\":\"property1\"}")
                                       .userIdentity(unauthorizedUser),
-                              "{\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
+                              "{\n  \"code\" : 403,\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
                               403);
         
         // Change Athens domain
@@ -796,7 +796,7 @@ public class ApplicationApiTest extends ControllerContainerTest {
         // Deleting a tenant for an Athens domain the user is not admin for is disallowed
         tester.assertResponse(request("/application/v4/tenant/tenant1", DELETE)
                                       .userIdentity(unauthorizedUser),
-                              "{\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
+                              "{\n  \"code\" : 403,\n  \"message\" : \"Tenant admin or Vespa operator role required\"\n}",
                               403);
     }
 
diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java
new file mode 100644
index 00000000000..e2440bc4c5f
--- /dev/null
+++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBase.java
@@ -0,0 +1,86 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.jdisc.http.filter.security.base;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import com.yahoo.jdisc.Response;
+import com.yahoo.jdisc.handler.FastContentWriter;
+import com.yahoo.jdisc.handler.ResponseDispatch;
+import com.yahoo.jdisc.handler.ResponseHandler;
+import com.yahoo.jdisc.http.filter.DiscFilterRequest;
+import com.yahoo.jdisc.http.filter.SecurityRequestFilter;
+
+import java.io.UncheckedIOException;
+import java.util.Optional;
+
+/**
+ * A base class for {@link SecurityRequestFilter} implementations that renders an error response as JSON.
+ *
+ * @author bjorncs
+ */
+public abstract class JsonSecurityRequestFilterBase implements SecurityRequestFilter {
+
+    private static final ObjectMapper mapper = new ObjectMapper();
+
+    @Override
+    public final void filter(DiscFilterRequest request, ResponseHandler handler) {
+        filter(request)
+                .ifPresent(errorResponse -> writeResponse(errorResponse, handler));
+    }
+
+    protected abstract Optional<ErrorResponse> filter(DiscFilterRequest request);
+
+    private void writeResponse(ErrorResponse error, ResponseHandler responseHandler) {
+        ObjectNode errorMessage = mapper.createObjectNode();
+        errorMessage.put("code", error.errorCode);
+        errorMessage.put("message", error.message);
+        error.response.headers().put("Content-Type", "application/json"); // Note: Overwrites header if already exists
+        try (FastContentWriter writer = ResponseDispatch.newInstance(error.response).connectFastWriter(responseHandler)) {
+            writer.write(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(errorMessage));
+        } catch (JsonProcessingException e) {
+            throw new UncheckedIOException(e);
+        }
+    }
+
+    /**
+     * An error response that contains a {@link Response}, error code and message.
+     * The error code and message will be rendered as JSON fields with name 'code' and 'message' respectively.
+     */
+    protected static class ErrorResponse {
+        private final Response response;
+        private final int errorCode;
+        private final String message;
+
+        public ErrorResponse(Response response, int errorCode, String message) {
+            this.response = response;
+            this.errorCode = errorCode;
+            this.message = message;
+        }
+
+        public ErrorResponse(Response response, String message) {
+            this(response, response.getStatus(), message);
+        }
+
+        public ErrorResponse(int httpStatusCode, int errorCode, String message) {
+            this(new Response(httpStatusCode), errorCode, message);
+        }
+
+        public ErrorResponse(int httpStatusCode, String message) {
+            this(new Response(httpStatusCode), message);
+        }
+
+        public Response getResponse() {
+            return response;
+        }
+
+        public int getErrorCode() {
+            return errorCode;
+        }
+
+        public String getMessage() {
+            return message;
+        }
+
+    }
+}
diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java
new file mode 100644
index 00000000000..38f7b126443
--- /dev/null
+++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/base/package-info.java
@@ -0,0 +1,8 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+/**
+ * @author bjorncs
+ */
+@ExportPackage
+package com.yahoo.jdisc.http.filter.security.base;
+
+import com.yahoo.osgi.annotation.ExportPackage;
\ No newline at end of file
diff --git a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java
index 7bdbd7eddf4..eafe17153ad 100644
--- a/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java
+++ b/jdisc-security-filters/src/main/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBase.java
@@ -1,15 +1,9 @@
 // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.jdisc.http.filter.security.cors;
 
-import com.fasterxml.jackson.core.JsonProcessingException;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.databind.node.ObjectNode;
 import com.yahoo.jdisc.Response;
-import com.yahoo.jdisc.handler.FastContentWriter;
-import com.yahoo.jdisc.handler.ResponseDispatch;
-import com.yahoo.jdisc.handler.ResponseHandler;
 import com.yahoo.jdisc.http.filter.DiscFilterRequest;
-import com.yahoo.jdisc.http.filter.SecurityRequestFilter;
+import com.yahoo.jdisc.http.filter.security.base.JsonSecurityRequestFilterBase;
 
 import java.util.HashSet;
 import java.util.Optional;
@@ -23,9 +17,7 @@ import static com.yahoo.jdisc.http.filter.security.cors.CorsLogic.createCorsResp
  *
  * @author bjorncs
  */
-public abstract class CorsRequestFilterBase implements SecurityRequestFilter {
-
-    private static final ObjectMapper mapper = new ObjectMapper();
+public abstract class CorsRequestFilterBase extends JsonSecurityRequestFilterBase {
 
     private final Set<String> allowedUrls;
 
@@ -38,44 +30,17 @@ public abstract class CorsRequestFilterBase implements SecurityRequestFilter {
     }
 
     @Override
-    public final void filter(DiscFilterRequest request, ResponseHandler handler) {
-        filter(request)
-                .ifPresent(errorResponse -> sendErrorResponse(request, errorResponse, handler));
+    public final Optional<ErrorResponse> filter(DiscFilterRequest request) {
+        Optional<ErrorResponse> errorResponse = filterRequest(request);
+        errorResponse.ifPresent(response -> addCorsHeaders(request, response.getResponse()));
+        return errorResponse;
     }
 
-    protected abstract Optional<ErrorResponse> filter(DiscFilterRequest request);
-
-    private void sendErrorResponse(DiscFilterRequest request,
-                                   ErrorResponse errorResponse,
-                                   ResponseHandler responseHandler) {
-        Response response = new Response(errorResponse.statusCode);
-        addHeaders(request, response);
-        writeResponse(errorResponse, responseHandler, response);
-    }
+    protected abstract Optional<ErrorResponse> filterRequest(DiscFilterRequest request);
 
-    private void addHeaders(DiscFilterRequest request, Response response) {
+    private void addCorsHeaders(DiscFilterRequest request, Response response) {
         createCorsResponseHeaders(request.getHeader("Origin"), allowedUrls)
                 .forEach(response.headers()::add);
-        response.headers().add("Content-Type", "application/json");
     }
 
-    private void writeResponse(ErrorResponse errorResponse, ResponseHandler responseHandler, Response response) {
-        ObjectNode errorMessage = mapper.createObjectNode();
-        errorMessage.put("message", errorResponse.message);
-        try (FastContentWriter writer = ResponseDispatch.newInstance(response).connectFastWriter(responseHandler)) {
-            writer.write(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(errorMessage));
-        } catch (JsonProcessingException e) {
-            throw new RuntimeException(e);
-        }
-    }
-
-    protected static class ErrorResponse {
-        final int statusCode;
-        final String message;
-
-        public ErrorResponse(int statusCode, String message) {
-            this.statusCode = statusCode;
-            this.message = message;
-        }
-    }
 }
diff --git a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java
new file mode 100644
index 00000000000..9b0a875a73d
--- /dev/null
+++ b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/base/JsonSecurityRequestFilterBaseTest.java
@@ -0,0 +1,57 @@
+package com.yahoo.jdisc.http.filter.security.base;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.yahoo.container.jdisc.RequestHandlerTestDriver;
+import com.yahoo.jdisc.Response;
+import com.yahoo.jdisc.http.filter.DiscFilterRequest;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.Optional;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.junit.Assert.assertThat;
+import static org.mockito.Mockito.mock;
+
+/**
+ * @author bjorncs
+ */
+public class JsonSecurityRequestFilterBaseTest {
+
+    private final ObjectMapper mapper = new ObjectMapper();
+
+    @Test
+    public void filter_renders_errors_as_json() throws IOException {
+        int statusCode = 403;
+        String message = "Forbidden";
+        DiscFilterRequest request = mock(DiscFilterRequest.class);
+        SimpleSecurityRequestFilter filter =
+                new SimpleSecurityRequestFilter(new JsonSecurityRequestFilterBase.ErrorResponse(statusCode, message));
+        RequestHandlerTestDriver.MockResponseHandler responseHandler = new RequestHandlerTestDriver.MockResponseHandler();
+        filter.filter(request, responseHandler);
+
+        Response response = responseHandler.getResponse();
+        assertThat(response, notNullValue());
+        assertThat(response.getStatus(), equalTo(statusCode));
+
+        JsonNode jsonNode = mapper.readTree(responseHandler.readAll());
+        assertThat(jsonNode.get("message").asText(), equalTo(message));
+        assertThat(jsonNode.get("code").asInt(), equalTo(statusCode));
+    }
+
+    private static class SimpleSecurityRequestFilter extends JsonSecurityRequestFilterBase {
+        private final ErrorResponse errorResponse;
+
+        SimpleSecurityRequestFilter(ErrorResponse errorResponse) {
+            this.errorResponse = errorResponse;
+        }
+
+        @Override
+        protected Optional<ErrorResponse> filter(DiscFilterRequest request) {
+            return Optional.ofNullable(this.errorResponse);
+        }
+    }
+
+}
\ No newline at end of file
diff --git a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java
index 29d28499a28..2cb25bc93cb 100644
--- a/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java
+++ b/jdisc-security-filters/src/test/java/com/yahoo/jdisc/http/filter/security/cors/CorsRequestFilterBaseTest.java
@@ -52,7 +52,7 @@ public class CorsRequestFilterBaseTest {
         }
 
         @Override
-        protected Optional<ErrorResponse> filter(DiscFilterRequest request) {
+        protected Optional<ErrorResponse> filterRequest(DiscFilterRequest request) {
             return Optional.ofNullable(this.errorResponse);
         }
     }
diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java
index cc95945c495..62d161a20df 100644
--- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java
+++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/v2/filter/NodePrincipal.java
@@ -4,6 +4,7 @@ package com.yahoo.vespa.hosted.provision.restapi.v2.filter;
 import java.security.Principal;
 import java.security.cert.X509Certificate;
 import java.util.List;
+import java.util.Objects;
 import java.util.Optional;
 
 /**
@@ -66,4 +67,27 @@ public class NodePrincipal implements Principal {
 
     public enum Type { ATHENZ, LEGACY }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (o == null || getClass() != o.getClass()) return false;
+        NodePrincipal principal = (NodePrincipal) o;
+        return Objects.equals(identityName, principal.identityName) &&
+                Objects.equals(hostname, principal.hostname) &&
+                type == principal.type;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(identityName, hostname, type);
+    }
+
+    @Override
+    public String toString() {
+        return "NodePrincipal{" +
+                "identityName='" + identityName + '\'' +
+                ", hostname='" + hostname + '\'' +
+                ", type=" + type +
+                '}';
+    }
 }