[autorules] Rules POST handler enhancements (#423)

* Use URL-safe Base64 encoder/decoder (#419)

* Use URL-safe Base64 encoder/decoder

Use encoder which produces, and decoder which expects, the URL-safe Base64 alphabet variant. The decoder is also tolerant of missing padding characters at the end, which the client will not send due to their illegality in the WebSocket Subprotocol header value

* Update web-client to include base64url client-side changes

* Update web-client to fix build (#424)

* Add Quarkus JVM sample app (#418)

* Throw exception if added rule collides by name

* Throw exception if integer attrs negative

Add unit tests for rules post handler

* Refactor to extract string constants to enum

Add tests for non-integer params

* Correct spotbugs findings

* Add some Rule attribute assertions and tests

* Do all attribute validation in Rule

* Correct exception message formatting

* Catch IllegalArgumentException and rethrow as ApiException 400

* Minor refactor

* Remove no-op CHANGED event handling

* Include exception reason in API response message

src/main/java/io/cryostat/net/web/http/api/v2/RuleDeleteHandler.java

@@ -49,6 +49,7 @@
 import io.cryostat.net.AuthManager;
 import io.cryostat.net.web.http.HttpMimeType;
 import io.cryostat.net.web.http.api.ApiVersion;
+import io.cryostat.rules.Rule;
 import io.cryostat.rules.RuleRegistry;
 
 import com.google.gson.Gson;
@@ -95,7 +96,7 @@ public HttpMimeType mimeType() {
 
     @Override
     public IntermediateResponse<Void> handle(RequestParameters params) throws ApiException {
-        String name = params.getPathParams().get("ruleName");
+        String name = params.getPathParams().get(Rule.Attribute.NAME.getSerialKey());
         try {
             ruleRegistry.deleteRule(name);
         } catch (IOException e) {

src/main/java/io/cryostat/net/web/http/api/v2/RuleGetHandler.java

@@ -55,7 +55,7 @@
 
 class RuleGetHandler extends AbstractV2RequestHandler<Rule> {
 
-    static final String PATH = RulesPostHandler.PATH + "/:ruleName";
+    static final String PATH = RulesPostHandler.PATH + "/:name";
 
     private final RuleRegistry ruleRegistry;
     private final Logger logger;
@@ -94,7 +94,7 @@ public HttpMimeType mimeType() {
 
     @Override
     public IntermediateResponse<Rule> handle(RequestParameters params) throws ApiException {
-        String name = params.getPathParams().get("ruleName");
+        String name = params.getPathParams().get(Rule.Attribute.NAME.getSerialKey());
         return new IntermediateResponse<Rule>()
                 .body(this.ruleRegistry.getRule(name).orElseThrow(() -> new ApiException(404)));
     }

src/main/java/io/cryostat/net/web/http/api/v2/RulesPostHandler.java

@@ -110,8 +110,12 @@ public boolean isOrdered() {
     @Override
     public IntermediateResponse<String> handle(RequestParameters params) throws ApiException {
         Rule rule;
-        String rawMime = params.getHeaders().get(HttpHeaders.CONTENT_TYPE).split(";")[0];
-        HttpMimeType mime = HttpMimeType.fromString(rawMime);
+        String rawMime = params.getHeaders().get(HttpHeaders.CONTENT_TYPE);
+        if (rawMime == null) {
+            throw new ApiException(415, "Bad content type: null");
+        }
+        String firstMime = rawMime.split(";")[0];
+        HttpMimeType mime = HttpMimeType.fromString(firstMime);
         if (mime == null) {
             throw new ApiException(415, "Bad content type: " + rawMime);
         }
@@ -120,17 +124,31 @@ public IntermediateResponse<String> handle(RequestParameters params) throws ApiE
             case URLENCODED_FORM:
                 Rule.Builder builder =
                         new Rule.Builder()
-                                .name(params.getFormAttributes().get("name"))
-                                .targetAlias(params.getFormAttributes().get("targetAlias"))
-                                .description(params.getFormAttributes().get("description"))
-                                .eventSpecifier(params.getFormAttributes().get("eventSpecifier"));
-
-                builder = setOptionalInt(builder, "archivalPeriodSeconds", params);
-                builder = setOptionalInt(builder, "preservedArchives", params);
-                builder = setOptionalInt(builder, "maxAgeSeconds", params);
-                builder = setOptionalInt(builder, "maxSizeBytes", params);
-
-                rule = builder.build();
+                                .name(
+                                        params.getFormAttributes()
+                                                .get(Rule.Attribute.NAME.getSerialKey()))
+                                .targetAlias(
+                                        params.getFormAttributes()
+                                                .get(Rule.Attribute.TARGET_ALIAS.getSerialKey()))
+                                .description(
+                                        params.getFormAttributes()
+                                                .get(Rule.Attribute.DESCRIPTION.getSerialKey()))
+                                .eventSpecifier(
+                                        params.getFormAttributes()
+                                                .get(
+                                                        Rule.Attribute.EVENT_SPECIFIER
+                                                                .getSerialKey()));
+
+                builder = setOptionalInt(builder, Rule.Attribute.ARCHIVAL_PERIOD_SECONDS, params);
+                builder = setOptionalInt(builder, Rule.Attribute.PRESERVED_ARCHIVES, params);
+                builder = setOptionalInt(builder, Rule.Attribute.MAX_AGE_SECONDS, params);
+                builder = setOptionalInt(builder, Rule.Attribute.MAX_SIZE_BYTES, params);
+
+                try {
+                    rule = builder.build();
+                } catch (IllegalArgumentException iae) {
+                    throw new ApiException(400, iae);
+                }
                 break;
             case JSON:
                 rule = gson.fromJson(params.getBody(), Rule.class);
@@ -142,7 +160,10 @@ public IntermediateResponse<String> handle(RequestParameters params) throws ApiE
         try {
             rule = this.ruleRegistry.addRule(rule);
         } catch (IOException e) {
-            throw new ApiException(500, "IOException occurred while writing rule definition", e);
+            throw new ApiException(
+                    500,
+                    "IOException occurred while writing rule definition: " + e.getMessage(),
+                    e);
         }
 
         return new IntermediateResponse<String>()
@@ -151,29 +172,41 @@ public IntermediateResponse<String> handle(RequestParameters params) throws ApiE
                 .body(rule.getName());
     }
 
-    private Rule.Builder setOptionalInt(Rule.Builder builder, String key, RequestParameters params)
+    private Rule.Builder setOptionalInt(
+            Rule.Builder builder, Rule.Attribute key, RequestParameters params)
             throws IllegalArgumentException {
         MultiMap attrs = params.getFormAttributes();
-        if (!attrs.contains(key)) {
+        if (!attrs.contains(key.getSerialKey())) {
             return builder;
         }
         Function<Integer, Rule.Builder> fn;
         switch (key) {
-            case "archivalPeriodSeconds":
+            case ARCHIVAL_PERIOD_SECONDS:
                 fn = builder::archivalPeriodSeconds;
                 break;
-            case "preservedArchives":
+            case PRESERVED_ARCHIVES:
                 fn = builder::preservedArchives;
                 break;
-            case "maxAgeSeconds":
+            case MAX_AGE_SECONDS:
                 fn = builder::maxAgeSeconds;
                 break;
-            case "maxSizeBytes":
+            case MAX_SIZE_BYTES:
                 fn = builder::maxSizeBytes;
                 break;
             default:
                 throw new IllegalArgumentException("Unknown key " + key);
         }
-        return fn.apply(Integer.valueOf(attrs.get(key)));
+        int value;
+        try {
+            value = Integer.parseInt(attrs.get(key.getSerialKey()));
+        } catch (NumberFormatException nfe) {
+            throw new ApiException(
+                    400,
+                    String.format(
+                            "\"%s\" is an invalid (non-integer) value for \"%s\"",
+                            attrs.get(key.getSerialKey()), key),
+                    nfe);
+        }
+        return fn.apply(value);
     }
 }

src/main/java/io/cryostat/rules/Rule.java

@@ -41,8 +41,6 @@
  */
 package io.cryostat.rules;
 
-import java.util.Objects;
-
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
@@ -62,14 +60,17 @@ public class Rule {
     private final int maxSizeBytes;
 
     Rule(Builder builder) {
-        this.name = sanitizeRuleName(requireNonBlank(builder.name, "ruleName"));
-        this.description = builder.description;
-        this.targetAlias = requireNonBlank(builder.targetAlias, "targetAlias");
-        this.eventSpecifier = requireNonBlank(builder.eventSpecifier, "eventSpecifier");
-        this.archivalPeriodSeconds = builder.archivalPeriodSeconds;
-        this.preservedArchives = builder.preservedArchives;
+        this.name = sanitizeRuleName(requireNonBlank(builder.name, Attribute.NAME));
+        this.description = builder.description == null ? "" : builder.description;
+        this.targetAlias = requireNonBlank(builder.targetAlias, Attribute.TARGET_ALIAS);
+        this.eventSpecifier = requireNonBlank(builder.eventSpecifier, Attribute.EVENT_SPECIFIER);
+        this.archivalPeriodSeconds =
+                requireNonNegative(
+                        builder.archivalPeriodSeconds, Attribute.ARCHIVAL_PERIOD_SECONDS);
+        this.preservedArchives =
+                requireNonNegative(builder.preservedArchives, Attribute.PRESERVED_ARCHIVES);
         this.maxAgeSeconds =
-                builder.maxAgeSeconds > 0 ? builder.maxAgeSeconds : archivalPeriodSeconds;
+                builder.maxAgeSeconds > 0 ? builder.maxAgeSeconds : this.archivalPeriodSeconds;
         this.maxSizeBytes = builder.maxSizeBytes;
     }
 
@@ -115,14 +116,22 @@ static String sanitizeRuleName(String name) {
         return name.replaceAll("\\s", "_");
     }
 
-    private static String requireNonBlank(String s, String name) {
-        if (StringUtils.isBlank(Objects.requireNonNull(s))) {
+    private static String requireNonBlank(String s, Attribute attr) {
+        if (StringUtils.isBlank(s)) {
             throw new IllegalArgumentException(
-                    String.format("%s cannot be blank, was %s", name, s));
+                    String.format("\"%s\" cannot be blank, was \"%s\"", attr, s));
         }
         return s;
     }
 
+    private static int requireNonNegative(int i, Attribute attr) {
+        if (i < 0) {
+            throw new IllegalArgumentException(
+                    String.format("\"%s\" cannot be negative, was \"%d\"", attr, i));
+        }
+        return i;
+    }
+
     @Override
     public boolean equals(Object o) {
         return EqualsBuilder.reflectionEquals(this, o);
@@ -140,7 +149,7 @@ public static class Builder {
         private String eventSpecifier;
         private int archivalPeriodSeconds = 30;
         private int preservedArchives = 1;
-        private int maxAgeSeconds;
+        private int maxAgeSeconds = -1;
         private int maxSizeBytes = -1;
 
         public Builder name(String name) {
@@ -187,4 +196,31 @@ public Rule build() {
             return new Rule(this);
         }
     }
+
+    public enum Attribute {
+        NAME("name"),
+        DESCRIPTION("description"),
+        TARGET_ALIAS("targetAlias"),
+        EVENT_SPECIFIER("eventSpecifier"),
+        ARCHIVAL_PERIOD_SECONDS("archivalPeriodSeconds"),
+        PRESERVED_ARCHIVES("preservedArchives"),
+        MAX_AGE_SECONDS("maxAgeSeconds"),
+        MAX_SIZE_BYTES("maxSizeBytes"),
+        ;
+
+        private final String serialKey;
+
+        Attribute(String serialKey) {
+            this.serialKey = serialKey;
+        }
+
+        public String getSerialKey() {
+            return serialKey;
+        }
+
+        @Override
+        public String toString() {
+            return getSerialKey();
+        }
+    }
 }

src/main/java/io/cryostat/rules/RuleException.java

@@ -0,0 +1,50 @@
+/*-
+ * #%L
+ * Container JFR
+ * %%
+ * Copyright (C) 2020 Red Hat, Inc.
+ * %%
+ * The Universal Permissive License (UPL), Version 1.0
+ *
+ * Subject to the condition set forth below, permission is hereby granted to any
+ * person obtaining a copy of this software, associated documentation and/or data
+ * (collectively the "Software"), free of charge and under any and all copyright
+ * rights in the Software, and any and all patent rights owned or freely
+ * licensable by each licensor hereunder covering either (i) the unmodified
+ * Software as contributed to or provided by such licensor, or (ii) the Larger
+ * Works (as defined below), to deal in both
+ *
+ * (a) the Software, and
+ * (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
+ * one is included with the Software (each a "Larger Work" to which the Software
+ * is contributed by such licensors),
+ *
+ * without restriction, including without limitation the rights to copy, create
+ * derivative works of, display, perform, and distribute the Software and make,
+ * use, sell, offer for sale, import, export, have made, and have sold the
+ * Software and the Larger Work(s), and to sublicense the foregoing rights on
+ * either these or other terms.
+ *
+ * This license is subject to the following condition:
+ * The above copyright notice and either this complete permission notice or at
+ * a minimum a reference to the UPL must be included in all copies or
+ * substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ * #L%
+ */
+package com.redhat.rhjmc.containerjfr.rules;
+
+import java.io.IOException;
+
+public class RuleException extends IOException {
+    RuleException(String reason) {
+        super(reason);
+    }
+}

src/main/java/io/cryostat/rules/RuleRegistry.java

@@ -91,7 +91,12 @@ public void loadRules() throws IOException {
     }
 
     public Rule addRule(Rule rule) throws IOException {
-        this.deleteRule(rule);
+        if (hasRuleByName(rule.getName())) {
+            throw new RuleException(
+                    String.format(
+                            "Rule with name \"%s\" already exists; refusing to overwrite",
+                            rule.getName()));
+        }
         Path destination = rulesDir.resolve(rule.getName() + ".json");
         this.fs.writeString(
                 destination,
@@ -103,6 +108,10 @@ public Rule addRule(Rule rule) throws IOException {
         return rule;
     }
 
+    public boolean hasRuleByName(String name) {
+        return getRule(name).isPresent();
+    }
+
     public Optional<Rule> getRule(String name) {
         return this.rules.stream().filter(r -> Objects.equals(r.getName(), name)).findFirst();
     }

src/test/java/io/cryostat/net/BasicAuthManagerTest.java

@@ -366,6 +366,56 @@ void shouldPassKnownCredentialsAndStrippedPadding() throws Exception {
                             .get());
         }
 
+        @Test
+        void shouldPassKnownCredentialsWithPadding() throws Exception {
+            Path mockPath = Mockito.mock(Path.class);
+            Mockito.when(
+                            fs.pathOf(
+                                    System.getProperty("user.home"),
+                                    BasicAuthManager.USER_PROPERTIES_FILENAME))
+                    .thenReturn(mockPath);
+            Mockito.when(fs.exists(mockPath)).thenReturn(true);
+            Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true);
+            Mockito.when(fs.isReadable(mockPath)).thenReturn(true);
+            // credentials: "user:pass1234"
+            BufferedReader props =
+                    new BufferedReader(
+                            new StringReader(
+                                    "user:bd94dcda26fccb4e68d6a31f9b5aac0b571ae266d822620e901ef7ebe3a11d4f"));
+            Mockito.when(fs.readFile(mockPath)).thenReturn(props);
+            Assertions.assertTrue(
+                    mgr.validateWebSocketSubProtocol(
+                                    () -> "basic.authorization.containerjfr.dXNlcjpwYXNzMTIzNA==")
+                            .get());
+        }
+
+        @Test
+        void shouldPassKnownCredentialsAndStrippedPadding() throws Exception {
+            Path mockPath = Mockito.mock(Path.class);
+            Mockito.when(
+                            fs.pathOf(
+                                    System.getProperty("user.home"),
+                                    BasicAuthManager.USER_PROPERTIES_FILENAME))
+                    .thenReturn(mockPath);
+            Mockito.when(fs.exists(mockPath)).thenReturn(true);
+            Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true);
+            Mockito.when(fs.isReadable(mockPath)).thenReturn(true);
+            // credentials: "user:pass1234"
+            BufferedReader props =
+                    new BufferedReader(
+                            new StringReader(
+                                    "user:bd94dcda26fccb4e68d6a31f9b5aac0b571ae266d822620e901ef7ebe3a11d4f"));
+            Mockito.when(fs.readFile(mockPath)).thenReturn(props);
+            // the subprotocol token part here should be "dXNlcjpwYXNzMTIzNA==", but the '='s are
+            // padding and stripped out. The decoder should treat these as optional, and the client
+            // is likely not to send them since they are not permitted by the WebSocket
+            // specification for the Sec-WebSocket-Protocol header
+            Assertions.assertTrue(
+                    mgr.validateWebSocketSubProtocol(
+                                    () -> "basic.authorization.containerjfr.dXNlcjpwYXNzMTIzNA")
+                            .get());
+        }
+
         @Test
         void shouldFailUnknownCredentials() throws Exception {
             Path mockPath = Mockito.mock(Path.class);