Use system from enforcer, not context, when enforcing policies

5 files changed, 53 insertions, 25 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Enforcer.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Enforcer.java
new file mode 100644
index 00000000000..c83e25c6073
--- /dev/null
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Enforcer.java
@@ -0,0 +1,25 @@
+package com.yahoo.vespa.hosted.controller.api.role;
+
+import com.yahoo.config.provision.SystemName;
+
+import java.net.URI;
+
+/**
+ * Checks whether {@link Role}s have the required {@link Privilege}s to perform {@link Action}s on given {@link java.net.URI}s.
+ *
+ * @author jonmv
+ */
+public class Enforcer {
+
+    private final SystemName system;
+
+    public Enforcer(SystemName system) {
+        this.system = system;
+    }
+
+    /** Returns whether {@code role} has permission to perform {@code action} on {@code resource}, in this enforcer's system. */
+    public boolean allows(Role role, Action action, URI resource) {
+        return role.definition().policies().stream().anyMatch(policy -> policy.evaluate(action, resource, role.context, system));
+    }
+
+}
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Policy.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Policy.java
index db4cca20b9a..b241d40e2a7 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Policy.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Policy.java
@@ -125,9 +125,9 @@ public enum Policy {
     }
 
     /** Returns whether action is allowed on path in given context */
-    public boolean evaluate(Action action, URI uri, Context context) {
+    public boolean evaluate(Action action, URI uri, Context context, SystemName system) {
         return privileges.stream().anyMatch(privilege -> privilege.actions().contains(action) &&
-                                                         privilege.systems().contains(context.system()) &&
+                                                         privilege.systems().contains(system) &&
                                                          privilege.pathGroups().stream()
                                                                   .anyMatch(pg -> pg.matches(uri, context)));
     }
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Role.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Role.java
index c28fa7a3fc3..c63f341c616 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Role.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/role/Role.java
@@ -24,15 +24,12 @@ public abstract class Role {
     public RoleDefinition definition() { return roleDefinition; }
 
     /** Returns whether this role is allowed to perform the given action on the given resource. */
-    public final boolean allows(Action action, URI uri) {
-        return roleDefinition.policies().stream().anyMatch(policy -> policy.evaluate(action, uri, context));
+    public final boolean allows(Action action, URI uri, Enforcer enforcer) {
+        return enforcer.allows(this, action, uri);
     }
 
     /** Returns whether the other role is a parent of this, and has a context included in this role's context. */
     public boolean implies(Role other) {
-        if ( ! context.system().equals(other.context.system()))
-            throw new IllegalStateException("Coexisting roles should always be in the same system.");
-
         return    (context.tenant().isEmpty() || context.tenant().equals(other.context.tenant()))
                && (context.application().isEmpty() || context.application().equals(other.context.application()))
                && roleDefinition.inherited().contains(other.roleDefinition);
diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/role/RoleTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/role/RoleTest.java
index 6cfe01cfb77..2bdd516aba2 100644
--- a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/role/RoleTest.java
+++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/role/RoleTest.java
@@ -16,39 +16,42 @@ import static org.junit.Assert.assertTrue;
  */
 public class RoleTest {
 
+    private static final Enforcer mainEnforcer = new Enforcer(SystemName.main);
+    private static final Enforcer vaasEnforcer = new Enforcer(SystemName.vaas);
+
     @Test
     public void operator_membership() {
         Role role = new Roles(SystemName.main).hostedOperator();
 
         // Operator actions
-        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined")));
-        assertTrue(role.allows(Action.create, URI.create("/controller/v1/foo")));
-        assertTrue(role.allows(Action.update, URI.create("/os/v1/bar")));
-        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1")));
-        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t2/application/a2")));
+        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined"), mainEnforcer));
+        assertTrue(role.allows(Action.create, URI.create("/controller/v1/foo"), mainEnforcer));
+        assertTrue(role.allows(Action.update, URI.create("/os/v1/bar"), mainEnforcer));
+        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1"), mainEnforcer));
+        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t2/application/a2"), mainEnforcer));
     }
 
     @Test
     public void tenant_membership() {
         Role role = new Roles(SystemName.main).athenzTenantAdmin(TenantName.from("t1"));
-        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined")));
-        assertFalse("Deny access to operator API", role.allows(Action.create, URI.create("/controller/v1/foo")));
-        assertFalse("Deny access to other tenant and app", role.allows(Action.update, URI.create("/application/v4/tenant/t2/application/a2")));
-        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1")));
+        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined"), mainEnforcer));
+        assertFalse("Deny access to operator API", role.allows(Action.create, URI.create("/controller/v1/foo"), mainEnforcer));
+        assertFalse("Deny access to other tenant and app", role.allows(Action.update, URI.create("/application/v4/tenant/t2/application/a2"), mainEnforcer));
+        assertTrue(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1"), mainEnforcer));
 
         Role publicSystem = new Roles(SystemName.vaas).athenzTenantAdmin(TenantName.from("t1"));
-        assertFalse(publicSystem.allows(Action.read, URI.create("/controller/v1/foo")));
-        assertTrue(publicSystem.allows(Action.read, URI.create("/badge/v1/badge")));
-        assertTrue(publicSystem.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1")));
+        assertFalse(publicSystem.allows(Action.read, URI.create("/controller/v1/foo"), vaasEnforcer));
+        assertTrue(publicSystem.allows(Action.read, URI.create("/badge/v1/badge"), vaasEnforcer));
+        assertTrue(publicSystem.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1"), vaasEnforcer));
     }
 
     @Test
     public void build_service_membership() {
         Role role = new Roles(SystemName.vaas).tenantPipeline(TenantName.from("t1"), ApplicationName.from("a1"));
-        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined")));
-        assertFalse(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1")));
-        assertTrue(role.allows(Action.create, URI.create("/application/v4/tenant/t1/application/a1/jobreport")));
-        assertFalse("No global read access", role.allows(Action.read, URI.create("/controller/v1/foo")));
+        assertFalse(role.allows(Action.create, URI.create("/not/explicitly/defined"), vaasEnforcer));
+        assertFalse(role.allows(Action.update, URI.create("/application/v4/tenant/t1/application/a1"), vaasEnforcer));
+        assertTrue(role.allows(Action.create, URI.create("/application/v4/tenant/t1/application/a1/jobreport"), vaasEnforcer));
+        assertFalse("No global read access", role.allows(Action.read, URI.create("/controller/v1/foo"), vaasEnforcer));
     }
 
     @Test
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
index 181731ef896..42e35663ef0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/restapi/filter/ControllerAuthorizationFilter.java
@@ -11,6 +11,7 @@ import com.yahoo.jdisc.http.filter.security.cors.CorsRequestFilterBase;
 import com.yahoo.log.LogLevel;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.role.Action;
+import com.yahoo.vespa.hosted.controller.api.role.Enforcer;
 import com.yahoo.vespa.hosted.controller.api.role.Role;
 import com.yahoo.vespa.hosted.controller.api.role.Roles;
 import com.yahoo.vespa.hosted.controller.api.role.SecurityContext;
@@ -30,6 +31,7 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase {
     private static final Logger log = Logger.getLogger(ControllerAuthorizationFilter.class.getName());
 
     private final Roles roles;
+    private final Enforcer enforcer;
 
     @Inject
     public ControllerAuthorizationFilter(Controller controller,
@@ -41,6 +43,7 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase {
                                   Set<String> allowedUrls) {
         super(allowedUrls);
         this.roles = new Roles(system);
+        this.enforcer = new Enforcer(system);
     }
 
     @Override
@@ -54,11 +57,11 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase {
             Action action = Action.from(HttpRequest.Method.valueOf(request.getMethod()));
 
             // Avoid expensive look-ups when request is always legal.
-            if (roles.everyone().allows(action, request.getUri()))
+            if (roles.everyone().allows(action, request.getUri(), enforcer))
                 return Optional.empty();
 
             Set<Role> roles = securityContext.get().roles();
-            if (roles.stream().anyMatch(role -> role.allows(action, request.getUri())))
+            if (roles.stream().anyMatch(role -> role.allows(action, request.getUri(), enforcer)))
                 return Optional.empty();
         }
         catch (Exception e) {