improved Mandatory ID Filter to return info upon rejection (#751)

docs/_layouts/api-endpoint.html

@@ -44,7 +44,7 @@ <h4>Example Request</h4>
       <h4>Example Curl</h4>
       <pre><code class="language-bash">
           {%- capture headers %}
-          {%- unless page.empty %}-H "X-Client-Id: my_app_name" -H "Content-Type: application/json"
+          {%- unless page.empty %}-H "Content-Type: application/json"
         {%
         endunless %}
           {%- endcapture %}

docs/content/_endpoints/post-query-batch.md

@@ -14,3 +14,6 @@ response_fields:
   purpose: Responses to each query run.
 ---
 This accepts a JSON document where all keys are expected to map up to a Query.
+<p></p>
+<em>Note that the <code>x-client-id: my_app_name</code> 
+header must be supplied since anonymous requests are not permitted.</em>

docs/content/_endpoints/post-query-metrics.md

@@ -33,3 +33,5 @@ response_fields:
   type_name: 'Statistics'
   purpose: 'Statistics about the current query. This field should be inspected for errors which will have caused the result to be inconsistent.'
 ---
+<em>Note that the <code>x-client-id: my_app_name</code> 
+header must be supplied since anonymous requests are not permitted.</em>

heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java

@@ -128,7 +128,7 @@ private List<Connector> setupConnectors(final Server server) {
             connectors.stream().map(c -> c.setup(server, address)).iterator());
     }
 
-    private ServerConnector findServerConnector(Server server) {
+    private static ServerConnector findServerConnector(Server server) {
         final Connector[] connectors = server.getConnectors();
 
         if (connectors.length == 0) {
@@ -213,7 +213,7 @@ private HandlerCollection setupHandler() throws Exception {
 
         context.addServlet(jerseyServlet, "/*");
         context.addFilter(new FilterHolder(new ShutdownFilter(stopping, mapper)), "/*", null);
-        context.addFilter(new FilterHolder(new MandatoryClientIdFilter()), "/*", null);
+        context.addFilter(new FilterHolder(new MandatoryClientIdFilter(mapper)), "/query/*", null);
         context.setErrorHandler(new JettyJSONErrorHandler(mapper));
 
         final RequestLogHandler requestLogHandler = new RequestLogHandler();
@@ -230,7 +230,7 @@ private HandlerCollection setupHandler() throws Exception {
         return handlers;
     }
 
-    private void makeRewriteRules(RewriteHandler rewrite) {
+    private static void makeRewriteRules(RewriteHandler rewrite) {
         {
             final RewritePatternRule rule = new RewritePatternRule();
             rule.setPattern("/metrics");

heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java

@@ -21,32 +21,35 @@
 
 package com.spotify.heroic.servlet;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.base.Strings;
+import com.spotify.heroic.ws.ErrorMessage;
+import com.spotify.heroic.ws.MandatoryClientIdErrorMessage;
 import java.io.IOException;
-import javax.servlet.Filter;
 import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
 import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.ws.rs.core.Response.Status;
 
 /**
  * Rejects anonymous requests. That is, requests to any API endpoint that are
- * missing a non-null X-Client-Id HTTP header.
+ * missing a non-null X-Client-Id HTTP header.<p></p>
+ * *Note* that this @SuppressWarnings has to go here even though it's just for the doFilter
+ * method, else the JavaDoc for it doesn't render. Weird.
  */
-// Note that this @Suppress has to go here even though it's just for the doFilter
-// method, else the JavaDoc for it doesn't render. Weird.
 @SuppressWarnings("checkstyle:LineLength")
-public class MandatoryClientIdFilter implements Filter {
+public class MandatoryClientIdFilter extends SimpleFilter {
 
     public static final String X_CLIENT_ID_HEADER_NAME = "X-Client-Id";
+    public static final String ERROR_MESSAGE_TEXT =
+            "This anonymous request has been rejected. Please add a 'x-client-id' " +
+                    "HTTP header to your request.";
 
-    @Override
-    public void init(FilterConfig filterConfig) throws ServletException { /* intentionally empty
-    */ }
+    public MandatoryClientIdFilter(ObjectMapper mapper) {
+        super(mapper);
+    }
 
     /**
      * Reject (with a 400) the request, if the X-Client-Id HTTP header is not present
@@ -62,27 +65,26 @@ public void init(FilterConfig filterConfig) throws ServletException { /* intenti
      * to the client.
      */
     @Override
-    public void doFilter(
-        ServletRequest request, ServletResponse response, FilterChain chain
+    public ErrorMessage doFilterImpl(
+            HttpServletRequest request, HttpServletResponse response, FilterChain chain
     ) throws IOException, ServletException {
-        if (passesFilter(request)) {
-            chain.doFilter(request, response);
-        } else {
-            var httpResponse = HttpServletResponse.class.cast(response);
-            httpResponse.setStatus(Status.BAD_REQUEST.getStatusCode());
-        }
+
+        final var info =
+                new MandatoryClientIdErrorMessage("Anonymous requests are not permitted");
+
+        response.setStatus(Status.BAD_REQUEST.getStatusCode());
+        return info;
     }
 
     /**
      * Returns true if the HTTP header X-Client-Id is present and non-null and not empty.
      * @param request request to pluck X-Client-Id's value from
      * @return see above
      */
-    public static boolean passesFilter(ServletRequest request) {
+    @Override
+    public boolean passesFilter(ServletRequest request) {
         var req = HttpServletRequest.class.cast(request);
         return !Strings.isNullOrEmpty(req.getHeader(X_CLIENT_ID_HEADER_NAME));
     }
 
-    @Override
-    public void destroy() { /* intentionally empty */ }
 };

heroic-core/src/main/java/com/spotify/heroic/servlet/ShutdownFilter.java

@@ -22,64 +22,39 @@
 package com.spotify.heroic.servlet;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.google.common.base.Charsets;
+import com.spotify.heroic.ws.ErrorMessage;
 import com.spotify.heroic.ws.InternalErrorMessage;
-
-import javax.servlet.Filter;
+import java.io.IOException;
+import java.util.function.Supplier;
 import javax.servlet.FilterChain;
-import javax.servlet.FilterConfig;
 import javax.servlet.ServletException;
 import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import javax.ws.rs.core.Response.Status;
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.OutputStreamWriter;
-import java.util.function.Supplier;
-
-public class ShutdownFilter implements Filter {
-    private static final String CONTENT_TYPE = "application/json; charset=UTF-8";
 
+public class ShutdownFilter extends SimpleFilter {
     private final Supplier<Boolean> stopping;
-    private final ObjectMapper mapper;
 
     public ShutdownFilter(final Supplier<Boolean> stopping, final ObjectMapper mapper) {
+        super(mapper);
         this.stopping = stopping;
-        this.mapper = mapper;
     }
 
     @Override
-    public void init(FilterConfig filterConfig) throws ServletException {
+    public boolean passesFilter(ServletRequest request) {
+        return !stopping.get();
     }
 
     @Override
-    public void doFilter(
-        final ServletRequest request, final ServletResponse response, final FilterChain chain
+    public ErrorMessage doFilterImpl(
+            final HttpServletRequest request, final HttpServletResponse response,
+            final FilterChain chain
     ) throws IOException, ServletException {
-        if (!stopping.get()) {
-            chain.doFilter(request, response);
-            return;
-        }
+        final var info =
+                new InternalErrorMessage("Heroic is shutting down", Status.SERVICE_UNAVAILABLE);
 
-        final HttpServletResponse httpResponse = (HttpServletResponse) response;
-
-        final InternalErrorMessage info =
-            new InternalErrorMessage("Heroic is shutting down", Status.SERVICE_UNAVAILABLE);
-
-        httpResponse.setStatus(Status.SERVICE_UNAVAILABLE.getStatusCode());
-        httpResponse.setContentType(CONTENT_TYPE);
-
-        // intercept request
-        try (final ByteArrayOutputStream output = new ByteArrayOutputStream(4096)) {
-            final OutputStreamWriter writer = new OutputStreamWriter(output, Charsets.UTF_8);
-            mapper.writeValue(writer, info);
-            response.setContentLength(output.size());
-            output.writeTo(httpResponse.getOutputStream());
-        }
-    }
-
-    @Override
-    public void destroy() {
+        response.setStatus(Status.SERVICE_UNAVAILABLE.getStatusCode());
+        return info;
     }
 };

heroic-core/src/main/java/com/spotify/heroic/servlet/SimpleFilter.java

@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2015 Spotify AB.
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.spotify.heroic.servlet;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.base.Charsets;
+import com.spotify.heroic.ws.ErrorMessage;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.OutputStreamWriter;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * Class that dictates how simple request filtering operations should be done to
+ * its subclasses, which then just implement #doFilterImpl and #passesFilter.
+ */
+public abstract class SimpleFilter implements Filter {
+    private static final String CONTENT_TYPE = "application/json; charset=UTF-8";
+    private static final int BYTE_ARRAY_SIZE = 4096;
+    private final ObjectMapper mapper;
+
+    public SimpleFilter(ObjectMapper mapper) {
+        this.mapper = mapper;
+    }
+
+    @Override
+    public void doFilter(
+            final ServletRequest request, final ServletResponse response, final FilterChain chain
+    ) throws IOException, ServletException {
+        if (passesFilter(request)) {
+            chain.doFilter(request, response);
+        } else {
+            var httpResponse = HttpServletResponse.class.cast(response);
+
+            var info = doFilterImpl((HttpServletRequest) request, httpResponse, chain);
+
+            writeResponse(response, httpResponse, info);
+        }
+    }
+    @Override
+    public void init(FilterConfig filterConfig) throws ServletException { /* intentionally empty
+     */
+    }
+
+    @Override
+    public void destroy() { /* intentionally empty */ }
+
+    /**
+     * Does this request pass this filter's stipulations i.e. is it a "good"
+     * request?
+     * @param request request to interrogate
+     * @return true if it passes
+     */
+    public abstract boolean passesFilter(ServletRequest request);
+
+    /**
+     * This is an example of the Template Method design pattern where the super
+     * class "runs the show" and the subclasses act out its directions.
+     */
+    protected abstract ErrorMessage doFilterImpl(final HttpServletRequest request,
+                                                 final HttpServletResponse response,
+                                                 final FilterChain chain
+    ) throws IOException, ServletException;
+
+    private void writeResponse(ServletResponse response, HttpServletResponse httpResponse,
+                           ErrorMessage info) throws IOException {
+        httpResponse.setContentType(CONTENT_TYPE);
+
+        try (final ByteArrayOutputStream output = new ByteArrayOutputStream(BYTE_ARRAY_SIZE)) {
+            final OutputStreamWriter writer = new OutputStreamWriter(output, Charsets.UTF_8);
+            mapper.writeValue(writer, info);
+            response.setContentLength(output.size());
+            output.writeTo(httpResponse.getOutputStream());
+        }
+    }
+}

heroic-core/src/main/java/com/spotify/heroic/ws/MandatoryClientIdErrorMessage.java

@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2015 Spotify AB.
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.spotify.heroic.ws;
+
+import javax.ws.rs.core.Response.Status;
+
+public class MandatoryClientIdErrorMessage extends ErrorMessage {
+    public MandatoryClientIdErrorMessage(final String message) {
+        super(message, Status.BAD_REQUEST);
+    }
+
+    public String getType() {
+        return "authentication-error";
+    }
+}

heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java

@@ -5,6 +5,7 @@
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import java.io.IOException;
 import javax.servlet.FilterChain;
 import javax.servlet.ServletException;
@@ -53,7 +54,7 @@ public class MandatoryClientIdFilterTest {
 
     @BeforeClass
     public static void setUpClass() {
-        filter = new MandatoryClientIdFilter();
+        filter = new MandatoryClientIdFilter(new ObjectMapper());
     }
 
     @Before

system-tests/test_heroic.py

@@ -49,7 +49,6 @@ def api_session(session_scoped_container_getter):
 
 
 def test_loading(api_session):
-    api_session.headers.update({'x-client-id': 'Heroic--System-Tests'})
     resp = api_session.get('/status', timeout=10)
 
     assert resp.ok
@@ -61,8 +60,15 @@ def test_loading(api_session):
     assert status['metadataBackend']['ok']
     assert status['cluster']['ok']
 
-def test_loading_no_client_id(api_session):
+# This should return a 400 because it's an anonymous request
+def test_query_no_client_id(api_session):
     api_session.headers.update({'x-client-id': ''})
-    resp = api_session.get('/status', timeout=10)
-    assert resp.status_code == requests.codes['bad_request']
+    resp = api_session.post('/query/metrics', timeout=5)
+    assert requests.codes['bad_request'] == resp.status_code
+
+# This should return a 500 because it's a malformed request
+def test_query_with_client_id(api_session):
+    api_session.headers.update({'x-client-id': 'test-heroic-system-test'})
+    resp = api_session.post('/query/metrics', timeout=5)
+    assert requests.codes['internal_server_error'] == resp.status_code