diff --git a/docs/_layouts/api-endpoint.html b/docs/_layouts/api-endpoint.html
index bf493ad10..6a4ebfad3 100644
--- a/docs/_layouts/api-endpoint.html
+++ b/docs/_layouts/api-endpoint.html
@@ -44,7 +44,7 @@ Example Request
Example Curl
{%- 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 %}
diff --git a/docs/content/_endpoints/post-query-batch.md b/docs/content/_endpoints/post-query-batch.md
index bb05c4ac2..76d3a8eda 100644
--- a/docs/content/_endpoints/post-query-batch.md
+++ b/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.
+
+Note that the x-client-id: my_app_name
+header must be supplied since anonymous requests are not permitted.
\ No newline at end of file
diff --git a/docs/content/_endpoints/post-query-metrics.md b/docs/content/_endpoints/post-query-metrics.md
index 4c183892a..e247aa889 100644
--- a/docs/content/_endpoints/post-query-metrics.md
+++ b/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.'
---
+Note that the x-client-id: my_app_name
+header must be supplied since anonymous requests are not permitted.
\ No newline at end of file
diff --git a/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java b/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
index d77c76aff..42f540eb6 100644
--- a/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
+++ b/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
@@ -128,7 +128,7 @@ private List 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");
diff --git a/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java b/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
index ca8149eb6..eff38c2a9 100644
--- a/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
+++ b/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.
+ * *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,15 +65,15 @@ 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;
}
/**
@@ -78,11 +81,10 @@ public void doFilter(
* @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 */ }
};
diff --git a/heroic-core/src/main/java/com/spotify/heroic/servlet/ShutdownFilter.java b/heroic-core/src/main/java/com/spotify/heroic/servlet/ShutdownFilter.java
index dec650a61..4102b77ea 100644
--- a/heroic-core/src/main/java/com/spotify/heroic/servlet/ShutdownFilter.java
+++ b/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 stopping;
- private final ObjectMapper mapper;
public ShutdownFilter(final Supplier 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;
}
};
diff --git a/heroic-core/src/main/java/com/spotify/heroic/servlet/SimpleFilter.java b/heroic-core/src/main/java/com/spotify/heroic/servlet/SimpleFilter.java
new file mode 100644
index 000000000..bec7970f9
--- /dev/null
+++ b/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());
+ }
+ }
+}
diff --git a/heroic-core/src/main/java/com/spotify/heroic/ws/MandatoryClientIdErrorMessage.java b/heroic-core/src/main/java/com/spotify/heroic/ws/MandatoryClientIdErrorMessage.java
new file mode 100644
index 000000000..1cf0a0681
--- /dev/null
+++ b/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";
+ }
+}
diff --git a/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java b/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java
index c02c0dd9b..ad82afc2c 100644
--- a/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java
+++ b/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
diff --git a/system-tests/test_heroic.py b/system-tests/test_heroic.py
index f4755d413..4dff82d38 100644
--- a/system-tests/test_heroic.py
+++ b/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