Profile editorLink the profile editor & improve rule matching

README.md

@@ -2,6 +2,8 @@
 
 A security proxy for [Apache James](https://james.apache.org/) WebAdmin API, adding OIDC authentication and fine-grained per-client access control.
 
+> 💡 **Don't hand-write `allowed.urls`.** The [profile editor](https://github.com/linagora/twake-mail-admin/tree/main/profile-editor) builds a profile by asking what an administrator should be able to do — and audits profiles you already have.
+
 ## Why?
 
 Apache James WebAdmin uses static bearer tokens and exposes some unauthenticated technical endpoints. It cannot be opened to the internet as-is and has no concept of users or per-operation access control.

docs/01-architecture.md

@@ -40,7 +40,7 @@ For every incoming request:
 
 ## Special proxy endpoints
 
-Requests matching `GET /.proxy/allowed/urls` are handled by the proxy itself and never forwarded to James. The proxy authenticates the caller (steps 1–6), then returns the `allowed.urls` list for that client as JSON. This allows frontends to adapt their UI based on the caller's permission level.
+Requests matching `GET /.proxy/allowed/urls` are handled by the proxy itself and never forwarded to James. The proxy authenticates the caller (steps 1–6), then returns the `allowed.urls` list for that client as JSON. This allows frontends to adapt their UI based on the caller's permission level. A frontend that does so re-implements rule matching on its own, and `twake-mail-admin`'s implementation is not equivalent to the proxy's — see [The proxy and the frontend evaluate rules differently](02-configuration.md#the-proxy-and-the-frontend-evaluate-rules-differently).
 
 ## Components
 

src/main/java/com/linagora/webadmin/proxy/AllowedUrl.java

@@ -50,7 +50,7 @@ public AllowedUrl(List<String> verbs, String endpointPattern, boolean denied) {
         List<String> names = new ArrayList<>();
         this.compiledPathPattern = Pattern.compile(toPathRegex(pathPart, names));
         this.pathVariableNames = List.copyOf(names);
-        this.compiledQueryParams = parseQueryPattern(queryPart);
+        this.compiledQueryParams = parseQueryPattern(queryPart, endpointPattern);
     }
 
     public List<String> verbs() {
@@ -118,20 +118,24 @@ public boolean matches(String method, String fullUri) {
         return match(method, fullUri).isPresent();
     }
 
-    private static Map<String, CompiledQueryParam> parseQueryPattern(String queryPart) {
+    private static Map<String, CompiledQueryParam> parseQueryPattern(String queryPart, String endpointPattern) {
         if (queryPart.isEmpty()) {
             return Map.of();
         }
         Map<String, CompiledQueryParam> result = new HashMap<>();
         for (String param : queryPart.split("&")) {
             int eq = param.indexOf('=');
-            if (eq >= 0) {
-                String paramName = param.substring(0, eq);
-                String valuePattern = param.substring(eq + 1);
-                List<String> varNames = new ArrayList<>();
-                Pattern compiled = Pattern.compile(toPathRegex(valuePattern, varNames));
-                result.put(paramName, new CompiledQueryParam(compiled, List.copyOf(varNames)));
+            if (eq < 0) {
+                throw new IllegalArgumentException("Invalid endpoint pattern '" + endpointPattern + "': query parameter '"
+                    + param + "' has no '='. A parameter without a value pattern would impose no constraint at all. "
+                    + "Write '" + param + "=' to require a flag-style parameter (present, empty or valueless), "
+                    + "or '" + param + "=*' to require it with any value.");
             }
+            String paramName = param.substring(0, eq);
+            String valuePattern = param.substring(eq + 1);
+            List<String> varNames = new ArrayList<>();
+            Pattern compiled = Pattern.compile(toPathRegex(valuePattern, varNames));
+            result.put(paramName, new CompiledQueryParam(compiled, List.copyOf(varNames)));
         }
         return Map.copyOf(result);
     }

src/main/java/com/linagora/webadmin/proxy/WebAdminProxyConfiguration.java

@@ -200,25 +200,70 @@ private static List<AllowedUrl> parseAllowedUrls(JsonNode clientNode) {
         return allowedUrls;
     }
 
+    private static final List<String> ENDPOINT_RULE_KEYS = List.of("endpoint", "verb", "verbs", "denied");
+    private static final List<String> INCLUDE_RULE_KEYS = List.of("include");
+
     private static void resolveUrlNode(JsonNode urlNode, List<AllowedUrl> result) {
         JsonNode includeNode = urlNode.get("include");
         if (includeNode != null) {
+            rejectUnknownKeys(urlNode, INCLUDE_RULE_KEYS);
             result.addAll(loadIncludedAllowedUrls(includeNode.asText()));
         } else {
             result.add(parseSingleAllowedUrl(urlNode));
         }
     }
 
     private static AllowedUrl parseSingleAllowedUrl(JsonNode urlNode) {
+        rejectUnknownKeys(urlNode, ENDPOINT_RULE_KEYS);
+        JsonNode endpointNode = urlNode.get("endpoint");
+        if (endpointNode == null) {
+            throw new IllegalArgumentException("Invalid allowed.urls rule " + urlNode + ": missing required field 'endpoint'");
+        }
         List<String> verbs = new ArrayList<>();
-        JsonNode verbsNode = urlNode.get("verb");
+        JsonNode verbsNode = verbNode(urlNode);
         if (verbsNode != null) {
             verbsNode.forEach(v -> verbs.add(v.asText()));
         }
         boolean denied = Optional.ofNullable(urlNode.get("denied"))
             .map(JsonNode::asBoolean)
             .orElse(false);
-        return new AllowedUrl(verbs, urlNode.get("endpoint").asText(), denied);
+        return new AllowedUrl(verbs, endpointNode.asText(), denied);
+    }
+
+    /**
+     * {@code verbs} is a tolerated alias for the canonical {@code verb}. It used to be ignored, which
+     * silently turned such a rule into an all-verbs rule; honouring it makes the rule mean what it reads as.
+     */
+    private static JsonNode verbNode(JsonNode urlNode) {
+        JsonNode verb = urlNode.get("verb");
+        JsonNode verbsAlias = urlNode.get("verbs");
+        if (verb != null && verbsAlias != null) {
+            throw new IllegalArgumentException("Invalid allowed.urls rule " + urlNode
+                + ": 'verb' and its alias 'verbs' cannot both be set. Keep 'verb'.");
+        }
+        if (verbsAlias != null) {
+            LOGGER.warn("allowed.urls rule {} uses the deprecated field 'verbs'; prefer the canonical 'verb'", urlNode);
+            return verbsAlias;
+        }
+        return verb;
+    }
+
+    /**
+     * Any other unrecognized key in a rule is a mistake that widens the rule — an unread key imposes no
+     * constraint. Fail at startup rather than silently granting more than the profile reads as granting.
+     */
+    private static void rejectUnknownKeys(JsonNode urlNode, List<String> knownKeys) {
+        List<String> unknown = new ArrayList<>();
+        urlNode.fieldNames().forEachRemaining(name -> {
+            if (!knownKeys.contains(name)) {
+                unknown.add(name);
+            }
+        });
+        if (!unknown.isEmpty()) {
+            List<String> documentedKeys = knownKeys.stream().filter(key -> !key.equals("verbs")).toList();
+            throw new IllegalArgumentException("Invalid allowed.urls rule " + urlNode + ": unknown field(s) " + unknown
+                + ". Supported fields are " + documentedKeys + ".");
+        }
     }
 
     private static List<AllowedUrl> loadIncludedAllowedUrls(String includeUri) {

src/main/resources/linagora-calendar-support-profile.json

@@ -11,6 +11,6 @@
         {"verb": ["GET"], "endpoint": "/mailingLists"},
         {"verb": ["GET"], "endpoint": "/mailingLists/*"},
         {"verb": ["PUT", "DELETE"], "endpoint": "/mailingLists/*/members/*"},
-        {"verb": ["PUT", "DELETE"], "endpoint": "/mailingLists/*/owner/*"},
+        {"verb": ["PUT", "DELETE"], "endpoint": "/mailingLists/*/owners/*"},
         {"include": "classpath://functional-admin-calendar-baseline.json"}
 ]

src/test/java/com/linagora/webadmin/proxy/AllowedUrlTest.java

@@ -298,6 +298,39 @@ void sameVariableInPathAndQueryWithDifferentValuesShouldNotMatch() {
             AllowedUrl rule = new AllowedUrl(List.of(), "/domains/{domain}/users?domain={domain}");
             assertThat(rule.matches("GET", "/domains/example.com/users?domain=other.com")).isFalse();
         }
+
+        // --- Flag-style parameters ---
+
+        @Test
+        void valuelessQueryParamPatternShouldBeRejected() {
+            assertThatThrownBy(() -> new AllowedUrl(List.of(), "/quota/users?hasSpecificQuota"))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("hasSpecificQuota");
+        }
+
+        @Test
+        void valuelessQueryParamPatternShouldBeRejectedAmongValuedOnes() {
+            assertThatThrownBy(() -> new AllowedUrl(List.of(), "/messages?user={user}&useSavedDate"))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("useSavedDate");
+        }
+
+        @Test
+        void emptyValuePatternShouldRequireFlagParamPresent() {
+            AllowedUrl rule = new AllowedUrl(List.of(), "/quota/users?hasSpecificQuota=");
+            assertThat(rule.matches("GET", "/quota/users?hasSpecificQuota")).isTrue();
+            assertThat(rule.matches("GET", "/quota/users?hasSpecificQuota=")).isTrue();
+            assertThat(rule.matches("GET", "/quota/users")).isFalse();
+            assertThat(rule.matches("GET", "/quota/users?hasSpecificQuota=true")).isFalse();
+        }
+
+        @Test
+        void starValuePatternShouldRequireFlagParamPresentWithAnyValue() {
+            AllowedUrl rule = new AllowedUrl(List.of(), "/quota/users?hasSpecificQuota=*");
+            assertThat(rule.matches("GET", "/quota/users?hasSpecificQuota")).isTrue();
+            assertThat(rule.matches("GET", "/quota/users?hasSpecificQuota=true")).isTrue();
+            assertThat(rule.matches("GET", "/quota/users")).isFalse();
+        }
     }
 
     @Nested

src/test/java/com/linagora/webadmin/proxy/WebAdminProxyConfigurationTest.java

@@ -445,6 +445,85 @@ void shouldParseMultipleAllowedUrls() throws Exception {
             WebAdminProxyConfiguration config = WebAdminProxyConfiguration.from(writeConfig(json));
             assertThat(config.clientsForId("my-client").get(0).allowedUrls()).hasSize(2);
         }
+
+        private String withRule(String rule) {
+            return """
+                {
+                  "port": "8001",
+                  "oidc.userInfo.url": "http://lemonldap/userinfo",
+                  "oidc.introspect.url": "http://lemonldap/introspect",
+                  "oidc.audience": "webadmin-proxy",
+                  "oidc.claim.authenticated.user": "email",
+                  "oidc.token.cache.expiration": "60s",
+                  "clients": [
+                    {
+                      "my-client": {
+                        "webadmin.backend": "http://james:8000",
+                        "webadmin.token": "secret",
+                        "allowed.urls": [
+                          %s
+                        ]
+                      }
+                    }
+                  ]
+                }
+                """.formatted(rule);
+        }
+
+        @Test
+        void verbsAliasShouldRestrictVerbsLikeVerb() throws Exception {
+            WebAdminProxyConfiguration config = WebAdminProxyConfiguration.from(writeConfig(withRule("""
+                { "denied": true, "verbs": ["DELETE"], "endpoint": "/domains/{domain}/aliases" }""")));
+            AllowedUrl rule = config.clientsForId("my-client").get(0).allowedUrls().get(0);
+            assertThat(rule.verbs()).containsExactly("DELETE");
+            assertThat(rule.matches("DELETE", "/domains/example.com/aliases")).isTrue();
+            assertThat(rule.matches("GET", "/domains/example.com/aliases")).isFalse();
+        }
+
+        @Test
+        void verbAndVerbsAliasTogetherShouldBeRejected() throws Exception {
+            File config = writeConfig(withRule("""
+                { "verb": ["GET"], "verbs": ["DELETE"], "endpoint": "/domains/{domain}/aliases" }"""));
+            assertThatThrownBy(() -> WebAdminProxyConfiguration.from(config))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("verbs");
+        }
+
+        @Test
+        void unknownFieldShouldBeRejected() throws Exception {
+            File config = writeConfig(withRule("""
+                { "method": ["DELETE"], "endpoint": "/domains/{domain}/aliases" }"""));
+            assertThatThrownBy(() -> WebAdminProxyConfiguration.from(config))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("method");
+        }
+
+        @Test
+        void unknownFieldOnIncludeShouldBeRejected() throws Exception {
+            File config = writeConfig(withRule("""
+                { "include": "classpath://test-allowed-urls.json", "endpoint": "/users" }"""));
+            assertThatThrownBy(() -> WebAdminProxyConfiguration.from(config))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("endpoint");
+        }
+
+        @Test
+        void ruleWithoutEndpointShouldBeRejected() throws Exception {
+            File config = writeConfig(withRule("""
+                { "verb": ["GET"] }"""));
+            assertThatThrownBy(() -> WebAdminProxyConfiguration.from(config))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("endpoint");
+        }
+
+        @Test
+        void valuelessQueryParameterShouldBeRejected() throws Exception {
+            File config = writeConfig(withRule("""
+                { "verb": ["GET"], "endpoint": "/quota/users?hasSpecificQuota" }"""));
+            assertThatThrownBy(() -> WebAdminProxyConfiguration.from(config))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageContaining("hasSpecificQuota");
+        }
     }
 
     @Nested