Make explicit and verify what context should be extracted for each path

3 files changed, 76 insertions, 19 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java
index fa9822da943..756703ca085 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/PathGroup.java
@@ -4,8 +4,10 @@ package com.yahoo.vespa.hosted.controller.role;
 import com.yahoo.restapi.Path;
 
 import java.util.EnumSet;
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
+import java.util.regex.Pattern;
 
 /**
  * This declares and groups all known REST API paths in the controller.
@@ -30,31 +32,40 @@ public enum PathGroup {
     onboardingUser("/application/v4/user"),
 
     /** Paths used for creating tenants with access control */
-    onboardingTenant("/application/v4/tenant/{tenant}"),
+    onboardingTenant("/application/v4/tenant/{ignored}"),
 
     /** Read-only paths used when onboarding tenants */
     onboardingTenantInformation("/athenz/v1/",
                                 "/athenz/v1/domains"),
 
     /** Paths used for user management */
-    userManagement("/user/v1/{*}"),
+    userManagement("/user/v1/{*}"), // TODO probably add tenant and application levels.
 
-    /** Paths used by tenant/application administrators */
-    tenant("/application/v4/",
-           "/application/v4/property/",
-           "/application/v4/tenant/",
-           "/application/v4/tenant-pipeline/",
+    /** Paths used by tenant administrators */
+    tenantInfo("/application/v4/",
+               "/application/v4/property/",
+               "/application/v4/tenant/",
+               "/application/v4/tenant-pipeline/"),
+
+    /** Paths used by tenant administrators */
+    tenant(Matcher.tenant,
            "/application/v4/tenant/{tenant}",
-           "/application/v4/tenant/{tenant}/application/",
-           "/application/v4/tenant/{tenant}/application/{application}",
-           "/application/v4/tenant/{tenant}/application/{application}/deploying/{*}",
-           "/application/v4/tenant/{tenant}/application/{application}/instance/{*}",
-           "/application/v4/tenant/{tenant}/application/{application}/environment/dev/{*}",
-           "/application/v4/tenant/{tenant}/application/{application}/environment/perf/{*}",
-           "/application/v4/tenant/{tenant}/application/{application}/environment/prod/region/{region}/instance/{instance}/global-rotation/override"),
+           "/application/v4/tenant/{tenant}/application/"),
+
+    /** Paths used by application administrators */
+    application(Matcher.tenant,
+                Matcher.application,
+                "/application/v4/tenant/{tenant}/application/{application}",
+                "/application/v4/tenant/{tenant}/application/{application}/deploying/{*}",
+                "/application/v4/tenant/{tenant}/application/{application}/instance/{*}",
+                "/application/v4/tenant/{tenant}/application/{application}/environment/dev/{*}",
+                "/application/v4/tenant/{tenant}/application/{application}/environment/perf/{*}",
+                "/application/v4/tenant/{tenant}/application/{application}/environment/prod/region/{region}/instance/{instance}/global-rotation/override"),
 
     /** Paths used for deployments by build service(s) */
-    buildService("/application/v4/tenant/{tenant}/application/{application}/jobreport",
+    buildService(Matcher.tenant,
+                 Matcher.application,
+                 "/application/v4/tenant/{tenant}/application/{application}/jobreport",
                  "/application/v4/tenant/{tenant}/application/{application}/submit",
                  "/application/v4/tenant/{tenant}/application/{application}/promote",
                  "/application/v4/tenant/{tenant}/application/{application}/environment/prod/{*}",
@@ -71,10 +82,25 @@ public enum PathGroup {
               "/d/{*}",
               "/statuspage/v1/{*}");
 
-    final Set<String> pathSpecs;
+    final List<String> pathSpecs;
+    final List<Matcher> matchers;
 
     PathGroup(String... pathSpecs) {
-        this.pathSpecs = Set.of(pathSpecs);
+        this(List.of(), List.of(pathSpecs));
+    }
+
+    PathGroup(Matcher first, String... pathSpecs) {
+        this(List.of(first), List.of(pathSpecs));
+    }
+
+    PathGroup(Matcher first, Matcher second, String... pathSpecs) {
+        this(List.of(first, second), List.of(pathSpecs));
+    }
+
+    /** Creates a new path group, if the given context matchers are each present exactly once in each of the given specs. */
+    PathGroup(List<Matcher> matchers, List<String> pathSpecs) {
+        this.matchers = matchers;
+        this.pathSpecs = pathSpecs;
     }
 
     /** Returns path if it matches any spec in this group, with match groups set by the match. */
@@ -106,4 +132,17 @@ public enum PathGroup {
         }).orElse(false);
     }
 
+
+    /** Fragments used to match parts of a path to create a context. */
+    enum Matcher {
+
+        tenant("{tenant}"),
+        application("{application}");
+
+        final String pattern;
+
+        Matcher(String pattern) { this.pattern = pattern; }
+
+    }
+
 }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Policy.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Policy.java
index 12e5a36a3ae..62d87355387 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Policy.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Policy.java
@@ -22,7 +22,7 @@ public enum Policy {
      * selected systems
      */
     tenant(Privilege.grant(Action.all())
-                    .on(PathGroup.tenant)
+                    .on(PathGroup.tenantInfo, PathGroup.tenant, PathGroup.application)
                     .in(SystemName.all())),
 
     /** Build service policy only allows access relevant for build service(s) */
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/PathGroupTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/PathGroupTest.java
index d110ff4c2fe..7ec97434966 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/PathGroupTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/PathGroupTest.java
@@ -4,6 +4,7 @@ import org.junit.Test;
 
 import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.regex.Pattern;
 
 import static org.junit.Assert.fail;
 
@@ -20,7 +21,7 @@ public class PathGroupTest {
                 if (pg == pg2) continue;
                 Set<String> overlapping = new LinkedHashSet<>(pg.pathSpecs);
                 overlapping.retainAll(pg2.pathSpecs);
-                if (!overlapping.isEmpty()) {
+                if ( ! overlapping.isEmpty()) {
                     fail("The following path specs overlap in " + pg + " and " + pg2 + ": " + overlapping);
                 }
             }
@@ -55,4 +56,21 @@ public class PathGroupTest {
                 }
     }
 
+    @Test
+    public void contextMatches() {
+        for (PathGroup group : PathGroup.values())
+            for (String spec : group.pathSpecs) {
+                for (PathGroup.Matcher matcher : PathGroup.Matcher.values()) {
+                    if (group.matchers.contains(matcher)) {
+                        if ( ! spec.contains(matcher.pattern))
+                            fail("Spec '" + spec + "' in '" + group.name() + "' should contain matcher '" + matcher.pattern + "'.");
+                        if (spec.replaceFirst(Pattern.quote(matcher.pattern), "").contains(matcher.pattern))
+                            fail("Spec '" + spec + "' in '" + group.name() + "' contains more than one instance of '" + matcher.pattern + "'.");
+                    }
+                    else if (spec.contains(matcher.pattern))
+                        fail("Spec '" + spec + "' in '" + group.name() + "' should not contain matcher '" + matcher.pattern + "'.");
+                }
+            }
+    }
+
 }