[#2020] Reject unknown properties in Credentials Management endpoint

Added explicit error handling in case of failure to process requests. Improved integration tests. Improved credentials and secret data objects to implicitly check constraints on property values during de-serialization/instantiation. Signed-off-by: Kai Hudalla <[email protected]>
eclipse-hono · Jun 24, 2020 · c267680 · c267680
1 parent c110578
commit c267680
Show file tree

Hide file tree

Showing 18 changed files with 624 additions and 543 deletions.
diff --git a/core/src/main/java/org/eclipse/hono/util/CredentialsConstants.java b/core/src/main/java/org/eclipse/hono/util/CredentialsConstants.java
@@ -14,6 +14,7 @@
 
 import java.util.Objects;
 import java.util.Optional;
+import java.util.regex.Pattern;
 
 import io.vertx.core.json.JsonObject;
 
@@ -119,11 +120,11 @@ public final class CredentialsConstants extends RequestResponseApiConstants {
     /**
      * The regular expression to validate that the auth-id field supplied in credentials is legal.
      */
-    public static final String AUTH_ID_VALUE_REGEX = "^[a-zA-Z0-9-=.]+$";
+    public static final Pattern PATTERN_AUTH_ID_VALUE = Pattern.compile("^[a-zA-Z0-9-=.]+$");
     /**
      * The regular expression to validate that the type field supplied in credentials is legal.
      */
-    public static final String TYPE_VALUE_REGEX = "^[a-z0-9-]+$";
+    public static final Pattern PATTERN_TYPE_VALUE = Pattern.compile("^[a-z0-9-]+$");
 
     /**
      * Request actions that belong to the Credentials API.

diff --git a/service-base/src/main/java/org/eclipse/hono/service/http/AbstractHttpEndpoint.java b/service-base/src/main/java/org/eclipse/hono/service/http/AbstractHttpEndpoint.java
@@ -19,7 +19,6 @@
 import java.util.Set;
 import java.util.function.Function;
 import java.util.function.Predicate;
-import java.util.regex.Pattern;
 
 import org.eclipse.hono.client.ClientErrorException;
 import org.eclipse.hono.client.ServiceInvocationException;
@@ -39,7 +38,6 @@
 import io.vertx.core.http.HttpHeaders;
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.json.DecodeException;
-import io.vertx.core.json.JsonObject;
 import io.vertx.ext.web.MIMEHeader;
 import io.vertx.ext.web.RoutingContext;
 import io.vertx.ext.web.handler.CorsHandler;
@@ -70,8 +68,6 @@ public abstract class AbstractHttpEndpoint<T extends ServiceConfigProperties> ex
      */
     protected static final String KEY_RESOURCE_VERSION = "KEY_RESOURCE_VERSION";
 
-    private static final Pattern PATTERN_ANY_STRING = Pattern.compile(".*");
-
     /**
      * The configuration properties for this endpoint.
      */
@@ -152,9 +148,14 @@ protected final void extractRequiredJsonPayload(final RoutingContext ctx) {
     }
 
     /**
-     * Check if the payload is not null and call \
-     * {@link #extractRequiredJson(RoutingContext, Function)}} to extract it accordingly.
+     * Extracts JSON payload from a request body, if not empty.
+     * <p>
+     * This method tries to de-serialize the content of the request body
+     * into a {@code JsonObject} and put it to the request context using key
+     * {@value #KEY_REQUEST_BODY}.
      * <p>
+     * The request is failed with a 400 status code if de-serialization fails, for example
+     * because the content is not a valid JSON string.
      *
      * @param ctx The routing context to retrieve the JSON request body from.
      */
@@ -163,7 +164,6 @@ protected final void extractOptionalJsonPayload(final RoutingContext ctx) {
         if (ctx.getBody().length() != 0) {
             extractRequiredJson(ctx, RoutingContext::getBodyAsJson);
         } else {
-            ctx.put(KEY_REQUEST_BODY, new JsonObject());
             ctx.next();
         }
     }
@@ -256,94 +256,6 @@ protected final Future<String> getRequestParameter(
         return result.future();
     }
 
-    /**
-     * Get request parameter value and check if it has been set. If it's not set, fail the request.
-     *
-     * @param paramName The name of the parameter to get.
-     * @param ctx The routing context of the request.
-     * @param span The active OpenTracing span for this operation. In case of the missing mandatory parameter, the error is logged and the span is finished.
-     *             Otherwise, the parameter is set as a span tag.
-     * @return The value of the parameter if it's set or {@code null} otherwise.
-     * @throws NullPointerException If ctx or paramName are {@code null}.
-     * @deprecated Use {@link #getRequestIdParam(String, RoutingContext, Span, boolean)} instead.
-     */
-    @Deprecated
-    protected final String getMandatoryIdRequestParam(final String paramName, final RoutingContext ctx, final Span span) {
-        return getRequestIdParam(paramName, ctx, span, false);
-    }
-
-    /**
-     * Get request parameter value. Optionally, if parameter has not been set, fail the request.
-     *
-     * @param paramName The name of the parameter to get.
-     * @param ctx The routing context of the request.
-     * @param span The active OpenTracing span for this operation. In case of the missing mandatory parameter, the error is logged and the span is finished.
-     *             Otherwise, the parameter is set as a span tag.
-     * @param optional Whether to check if parameter has been set or not.
-     * @return The value of the parameter if it's set or {@code null} otherwise.
-     * @throws NullPointerException If ctx or paramName are {@code null}.
-     * @deprecated Use {@link #getRequestIdParam(String, RoutingContext, Span, boolean)} instead.
-     */
-    @Deprecated
-    protected final String getRequestIdParam(final String paramName, final RoutingContext ctx, final Span span, final boolean optional) {
-
-        final String value = ctx.request().getParam(paramName);
-
-        if (value != null) {
-
-            final Pattern pattern;
-            final boolean matches;
-            switch (paramName) {
-            case PARAM_TENANT_ID:
-                pattern = config.getTenantIdPattern();
-                matches = pattern.matcher(value).matches();
-                break;
-            case PARAM_DEVICE_ID:
-                pattern = config.getDeviceIdPattern();
-                matches = pattern.matcher(value).matches();
-                break;
-            default:
-                pattern = PATTERN_ANY_STRING;
-                matches = true;
-            }
-
-            if (matches) {
-                span.setTag(paramName, value);
-                return value;
-            } else {
-                final String msg = String.format("parameter [name: %s, value: %s] does not match allowed pattern: %s",
-                        paramName, value, pattern.pattern());
-                HttpUtils.badRequest(ctx, msg);
-                finishSpanWithError(span, HttpURLConnection.HTTP_BAD_REQUEST, msg);
-                return null;
-            }
-
-        } else if (!optional) {
-            final String msg = String.format("Missing request parameter: %s", paramName);
-            HttpUtils.badRequest(ctx, msg);
-            finishSpanWithError(span, HttpURLConnection.HTTP_BAD_REQUEST, msg);
-            return null;
-        } else {
-            span.setTag(paramName, value);
-            return value;
-        }
-    }
-
-    /**
-     * Finish the given span with the error code and logs the error message.
-     *
-     * @param span The nspan to finish.
-     * @param httpErrorCode The HTTP Error code to use.
-     * @param errorMessage A string containing a message describing the error.
-     * @deprecated Use {@link #failRequest(RoutingContext, Throwable, Span)} instead.
-     */
-    @Deprecated
-    protected final void finishSpanWithError(final Span span, final int httpErrorCode, final String errorMessage) {
-        TracingHelper.logError(span, errorMessage);
-        Tags.HTTP_STATUS.set(span, httpErrorCode);
-        span.finish();
-    }
-
     /**
      * Fails a request with a given error.
      * <p>

diff --git a/...ase-jdbc/src/test/java/org/eclipse/hono/service/base/jdbc/store/device/JsonStoreTest.java b/...ase-jdbc/src/test/java/org/eclipse/hono/service/base/jdbc/store/device/JsonStoreTest.java
@@ -36,8 +36,7 @@ private static PskCredential newPskCredential(final String authId) {
         final PskSecret psk1 = new PskSecret();
         psk1.setKey("foo".getBytes(StandardCharsets.UTF_8));
 
-        final PskCredential result = new PskCredential();
-        result.setAuthId(authId);
+        final PskCredential result = new PskCredential(authId);
         result.setSecrets(Arrays.asList(psk1));
 
         return result;

diff --git a/...main/java/org/eclipse/hono/service/management/AbstractDelegatingRegistryHttpEndpoint.java b/...main/java/org/eclipse/hono/service/management/AbstractDelegatingRegistryHttpEndpoint.java
@@ -14,7 +14,6 @@
 
 package org.eclipse.hono.service.management;
 
-import java.net.HttpURLConnection;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.BiConsumer;
@@ -28,12 +27,12 @@
 
 import io.opentracing.Span;
 import io.opentracing.tag.Tags;
-import io.vertx.core.Handler;
 import io.vertx.core.MultiMap;
 import io.vertx.core.Vertx;
+import io.vertx.core.buffer.Buffer;
 import io.vertx.core.http.HttpHeaders;
 import io.vertx.core.http.HttpServerResponse;
-import io.vertx.core.json.JsonObject;
+import io.vertx.core.json.Json;
 import io.vertx.ext.web.RoutingContext;
 
 
@@ -134,74 +133,17 @@ protected final void writeResponse(final RoutingContext ctx, final Result<?> res
             customHandler.accept(response.headers(), status);
         }
         // once the body has been written, the response headers can no longer be changed
-        HttpUtils.setResponseBody(response, asJson(result.getPayload()));
+        HttpUtils.setResponseBody(response, asJson(result.getPayload()), HttpUtils.CONTENT_TYPE_JSON_UTF8);
         Tags.HTTP_STATUS.set(span, status);
         response.end();
     }
 
-    private JsonObject asJson(final Object obj) {
+    private Buffer asJson(final Object obj) {
         try {
-            return JsonObject.mapFrom(obj);
+            return Json.encodeToBuffer(obj);
         } catch (final IllegalArgumentException e) {
             logger.debug("error serializing result object [type: {}] to JSON", obj.getClass().getName(), e);
             return null;
         }
     }
-
-    /**
-     * Writes a response based on generic result.
-     * <p>
-     * The behavior is as follows:
-     * <ol>
-     * <li>Set the status code on the response.</li>
-     * <li>If the status code represents an error condition (i.e. the code is &gt;= 400),
-     * then the JSON object passed in the result is written to the response body.</li>
-     * <li>If the result is created (the code is = 201), the JSON object is written to the response body and the given custom handler is
-     * invoked (if not {@code null}).</li>
-     * <li>Sets the status of the tracing span and finishes it.</li>
-     * <li>Ends a response.</li>
-     * </ol>
-     *
-     * @param ctx The routing context of the request.
-     * @param result The generic result of the operation.
-     * @param customHandler An (optional) handler for post processing successful HTTP response, e.g. to set any additional HTTP
-     *                      headers. The handler <em>must not</em> write to response body. May be {@code null}.
-     * @param span The active OpenTracing span for this operation. The status of the response is logged and span is finished.
-     * @deprecated Use {@link #writeResponse(RoutingContext, Result, BiConsumer, Span)} instead.
-     */
-    @Deprecated
-    protected final void writeResponse(final RoutingContext ctx, final Result<?> result, final Handler<HttpServerResponse> customHandler, final Span span) {
-        final int status = result.getStatus();
-        final HttpServerResponse response = ctx.response();
-        response.setStatusCode(status);
-        if (status >= 400) {
-            HttpUtils.setResponseBody(response, JsonObject.mapFrom(result.getPayload()));
-        } else if (status == HttpURLConnection.HTTP_CREATED) {
-            if (customHandler != null) {
-                customHandler.handle(response);
-            }
-            HttpUtils.setResponseBody(response, JsonObject.mapFrom(result.getPayload()));
-        }
-        Tags.HTTP_STATUS.set(span, status);
-        span.finish();
-        response.end();
-    }
-
-    /**
-     * Writes a response based on operation result (including the resource version).
-     * <p>
-     * Sets ETAG header and then calls {@link #writeResponse}
-     *
-     * @param ctx The routing context of the request.
-     * @param result The operation result of the operation.
-     * @param customHandler An (optional) handler for post processing successful HTTP response, e.g. to set any additional HTTP
-     *                      headers. The handler <em>must not</em> write to response body. May be {@code null}.
-     * @param span The active OpenTracing span for this operation. The status of the response is logged and span is finished.
-     * @deprecated Use {@link #writeResponse(RoutingContext, Result, BiConsumer, Span)} instead.
-     */
-    @Deprecated
-    protected final void writeOperationResponse(final RoutingContext ctx, final OperationResult<?> result, final Handler<HttpServerResponse> customHandler, final Span span) {
-        result.getResourceVersion().ifPresent(v -> ctx.response().putHeader(HttpHeaders.ETAG, v));
-        writeResponse(ctx, result, customHandler, span);
-    }
 }
diff --git a/...-base/src/main/java/org/eclipse/hono/service/management/credentials/CommonCredential.java b/...-base/src/main/java/org/eclipse/hono/service/management/credentials/CommonCredential.java
@@ -17,11 +17,13 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.function.Predicate;
+import java.util.regex.Matcher;
 
+import org.eclipse.hono.util.CredentialsConstants;
 import org.eclipse.hono.util.RegistryManagementConstants;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonTypeInfo;
@@ -34,10 +36,23 @@
 @JsonInclude(value = JsonInclude.Include.NON_NULL)
 @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "type", visible = true)
 @JsonTypeIdResolver(CredentialTypeResolver.class)
-@JsonIgnoreProperties(ignoreUnknown = true)
 public abstract class CommonCredential {
 
-    @JsonProperty(RegistryManagementConstants.FIELD_AUTH_ID)
+    /**
+     * A predicate for matching authentication identifiers against the
+     * {@linkplain CredentialsConstants#PATTERN_AUTH_ID_VALUE default pattern}.
+     */
+    protected static final Predicate<String> AUTH_ID_VALIDATOR_DEFAULT = authId -> {
+        final Matcher matcher = CredentialsConstants.PATTERN_AUTH_ID_VALUE.matcher(authId);
+        if (matcher.matches()) {
+            return true;
+        } else {
+            throw new IllegalArgumentException("authentication identifier must match pattern "
+                    + CredentialsConstants.PATTERN_AUTH_ID_VALUE.pattern());
+        }
+    };
+
+    @JsonProperty(value = RegistryManagementConstants.FIELD_AUTH_ID)
     private String authId;
     @JsonProperty(RegistryManagementConstants.FIELD_ENABLED)
     private Boolean enabled;
@@ -48,6 +63,22 @@ public abstract class CommonCredential {
     @JsonInclude(value = JsonInclude.Include.NON_EMPTY)
     private Map<String, Object> extensions = new HashMap<>();
 
+    /**
+     * Creates a new credentials object for an authentication identifier.
+     *
+     * @param authId The authentication identifier.
+     * @throws NullPointerException if the auth ID is {@code null}.
+     * @throws IllegalArgumentException if auth ID does not match {@link CredentialsConstants#PATTERN_AUTH_ID_VALUE}.
+     */
+    protected CommonCredential(final String authId) {
+        Objects.requireNonNull(authId);
+        if (getAuthIdValidator().test(authId)) {
+            this.authId = authId;
+        } else {
+            throw new IllegalArgumentException("validation of authentication identifier failed");
+        }
+    }
+
     /**
      * Get a list of secrets for this credential.
      *
@@ -62,21 +93,43 @@ public abstract class CommonCredential {
      */
     public abstract String getType();
 
-    public String getAuthId() {
-        return authId;
+    /**
+     * Gets a predicate to use for validating authentication identifiers.
+     * <p>
+     * The predicate is invoked by the constructor before setting the authId property.
+     * This default implementation returns a predicate that matches the identifier
+     * against the {@linkplain CredentialsConstants#PATTERN_AUTH_ID_VALUE default pattern}.
+     * <p>
+     * The constructor will re-throw an {@code IllegalArgumentException}y thrown by the
+     * predicate's <em>test</em> method. This might be used to convey additional information
+     * about the failed validation.
+     * <p>
+     * Subclasses should override this method in order to use other means of validation.
+     *
+     * @return The predicate.
+     */
+    protected Predicate<String> getAuthIdValidator() {
+        return AUTH_ID_VALIDATOR_DEFAULT;
     }
 
     /**
-     * Sets the authentication identifier that the device uses for authenticating to protocol adapters.
+     * Sets the authentication identifier used by the device.
      *
-     * @param authId The authentication identifier use for authentication.
+     * @param authId The identifier.
      * @return A reference to this for fluent use.
+     * @throws NullPointerException if ID is {@code null}.
      */
-    public CommonCredential setAuthId(final String authId) {
+    @JsonIgnore
+    protected final CommonCredential setAuthId(final String authId) {
+        Objects.requireNonNull(authId);
         this.authId = authId;
         return this;
     }
 
+    public final String getAuthId() {
+        return authId;
+    }
+
     @JsonIgnore
     public boolean isEnabled() {
         return Optional.ofNullable(enabled).orElse(true);