Change API for building RoleMemberships to reflect the single-system property

4 files changed, 105 insertions, 51 deletions

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 b3278f667b8..b0935964950 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
@@ -149,32 +149,34 @@ public class ControllerAuthorizationFilter extends CorsRequestFilterBase {
             if ( ! (principal instanceof AthenzPrincipal))
                 throw new IllegalStateException("Expected an AthenzPrincipal to be set on the request.");
 
-            Map<Role, Set<Context>> memberships = new HashMap<>();
+            RoleMembership.Builder memberships = RoleMembership.in(system);
             AthenzIdentity identity = ((AthenzPrincipal) principal).getIdentity();
             Optional<Tenant> tenant = tenant();
             Context context = context(tenant); // TODO this way of computing a context is wrong, but we must
                                                // do it until we ask properly for roles based on token.
-            Set<Context> contexts = Set.of(context);
             if (isHostedOperator(identity)) {
-                memberships.put(Role.hostedOperator, contexts);
+                memberships.add(Role.hostedOperator);
             }
             if (tenant.isPresent() && isTenantAdmin(identity, tenant.get())) {
-                memberships.put(Role.tenantAdmin, contexts);
+                memberships.add(Role.tenantAdmin).limitedTo(tenant.get().name());
             }
             AthenzDomain principalDomain = identity.getDomain();
             if (principalDomain.equals(SCREWDRIVER_DOMAIN)) {
-                // NOTE: Only fine-grained deploy authorization for Athenz tenants
-                if (context.application().isPresent() && tenant.isPresent() && tenant.get() instanceof AthenzTenant) {
-                    AthenzDomain tenantDomain = ((AthenzTenant) tenant.get()).domain();
-                    if (hasDeployerAccess(identity, tenantDomain, context.application().get())) {
-                        memberships.put(Role.tenantPipelineOperator, contexts);
+                if (context.application().isPresent() && tenant.isPresent()) {
+                    // NOTE: Only fine-grained deploy authorization for Athenz tenants
+                    if (tenant.get() instanceof AthenzTenant) {
+                        AthenzDomain tenantDomain = ((AthenzTenant) tenant.get()).domain();
+                        if (hasDeployerAccess(identity, tenantDomain, context.application().get())) {
+                            memberships.add(Role.tenantPipelineOperator).limitedTo(tenant.get().name(), context.application().get());
+                        }
+                    }
+                    else {
+                        memberships.add(Role.tenantPipelineOperator).limitedTo(tenant.get().name(), context.application().get());
                     }
-                } else {
-                    memberships.put(Role.tenantPipelineOperator, contexts);
                 }
             }
-            memberships.put(Role.everyone, contexts);
-            return new RoleMembership(memberships);
+            memberships.add(Role.everyone);
+            return memberships.build();
         }
 
         private Optional<Tenant> tenant() {
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/RoleMembership.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/RoleMembership.java
index f9b1476463c..97d7947fb4f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/RoleMembership.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/RoleMembership.java
@@ -1,38 +1,41 @@
 // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.controller.role;
 
+import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.SystemName;
+import com.yahoo.config.provision.TenantName;
 
 import java.security.Principal;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 /**
  * A list of roles and their associated contexts. This defines the role membership of a tenant, and in which contexts
  * (see {@link Context}) those roles apply.
  *
  * @author mpolden
+ * @author jonmv
  */
 public class RoleMembership {
 
     private final Map<Role, Set<Context>> roles;
 
-    public RoleMembership(Map<Role, Set<Context>> roles) {
-        if (roles.values().stream()
-                 .flatMap(Set::stream)
-                 .map(Context::system)
-                 .distinct().count() != 1)
-            throw new IllegalArgumentException("A RoleMembership should be defined only for a single system.");
-
-        this.roles = Map.copyOf(roles);
+    private RoleMembership(Map<Role, Set<Context>> roles) {
+        this.roles = roles.entrySet().stream()
+                          .collect(Collectors.toUnmodifiableMap(entry -> entry.getKey(),
+                                                                entry -> Set.copyOf(entry.getValue())));
     }
 
     public static RoleMembership everyoneIn(SystemName system) {
-        return new RoleMembership(Map.of(Role.everyone, Set.of(Context.unlimitedIn(system))));
+        return in(system).add(Role.everyone).build();
     }
 
+    public static Builder in(SystemName system) { return new BuilderWithRole(system); }
+
     /** Returns whether any role in this allows action to take place in path */
     public boolean allows(Action action, String path) {
         return roles.entrySet().stream().anyMatch(kv -> {
@@ -55,4 +58,59 @@ public class RoleMembership {
         RoleMembership membership(Principal user);
     }
 
+    public interface Builder {
+
+        BuilderWithRole add(Role role);
+
+        RoleMembership build();
+
+    }
+
+    public static class BuilderWithRole implements Builder {
+
+        private final SystemName system;
+        private final Map<Role, Set<Context>> roles;
+
+        private Role current;
+
+        private BuilderWithRole(SystemName system) {
+            this.system = Objects.requireNonNull(system);
+            this.roles = new HashMap<>();
+        }
+
+        @Override
+        public BuilderWithRole add(Role role) {
+            if (current != null) {
+                roles.putIfAbsent(current, new HashSet<>());
+                roles.get(current).add(Context.unlimitedIn(system));
+            }
+            current = role;
+            return this;
+        }
+
+        public Builder limitedTo(TenantName tenant) {
+            roles.putIfAbsent(current, new HashSet<>());
+            roles.get(current).add(Context.limitedTo(tenant, system));
+            current = null;
+            return this;
+        }
+
+        public Builder limitedTo(TenantName tenant, ApplicationName application) {
+            roles.putIfAbsent(current, new HashSet<>());
+            roles.get(current).add(Context.limitedTo(tenant, application, system));
+            current = null;
+            return this;
+        }
+
+        @Override
+        public RoleMembership build() {
+            if (current != null) {
+                roles.putIfAbsent(current, new HashSet<>());
+                roles.get(current).add(Context.unlimitedIn(system));
+            }
+            return new RoleMembership(roles);
+        }
+
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
index 2ae17a5624e..6b7da3d2458 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/application/ApplicationApiTest.java
@@ -959,8 +959,8 @@ public class ApplicationApiTest extends ControllerContainerTest {
         // Promote application chef env for nonexistent tenant/application
         tester.assertResponse(request("/application/v4/tenant/dontexist/application/dontexist/environment/prod/region/us-west-1/instance/default/promote", POST)
                                       .screwdriverIdentity(SCREWDRIVER_ID),
-                              "{\"error-code\":\"INTERNAL_SERVER_ERROR\",\"message\":\"Unable to promote Chef environments for application\"}",
-                              500);
+                              "{\n  \"code\" : 403,\n  \"message\" : \"Access denied\"\n}",
+                              403);
 
         // Create legancy tenant name containing underscores
         tester.controller().curator().writeTenant(new AthenzTenant(TenantName.from("my_tenant"), ATHENZ_TENANT_DOMAIN,
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/RoleMembershipTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/RoleMembershipTest.java
index d4e673a02ae..2638e3f74e6 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/RoleMembershipTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/role/RoleMembershipTest.java
@@ -6,9 +6,6 @@ import com.yahoo.config.provision.SystemName;
 import com.yahoo.config.provision.TenantName;
 import org.junit.Test;
 
-import java.util.Map;
-import java.util.Set;
-
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -19,7 +16,9 @@ public class RoleMembershipTest {
 
     @Test
     public void operator_membership() {
-        RoleMembership roles = new RoleMembership(Map.of(Role.hostedOperator, Set.of(Context.unlimitedIn(SystemName.main))));
+        RoleMembership roles = RoleMembership.in(SystemName.main)
+                                             .add(Role.hostedOperator)
+                                             .build();
 
         // Operator actions
         assertFalse(roles.allows(Action.create, "/not/explicitly/defined"));
@@ -31,39 +30,34 @@ public class RoleMembershipTest {
 
     @Test
     public void tenant_membership() {
-        RoleMembership roles = new RoleMembership(Map.of(Role.tenantAdmin,
-                                                         Set.of(Context.limitedTo(TenantName.from("t1"),
-                                                                                  ApplicationName.from("a1"),
-                                                                                  SystemName.main))));
-
+        RoleMembership roles = RoleMembership.in(SystemName.main)
+                                             .add(Role.tenantAdmin).limitedTo(TenantName.from("t1"), ApplicationName.from("a1"))
+                                             .build();
         assertFalse(roles.allows(Action.create, "/not/explicitly/defined"));
         assertFalse("Deny access to operator API", roles.allows(Action.create, "/controller/v1/foo"));
         assertFalse("Deny access to other tenant and app", roles.allows(Action.update, "/application/v4/tenant/t2/application/a2"));
         assertFalse("Deny access to other app", roles.allows(Action.update, "/application/v4/tenant/t1/application/a2"));
         assertTrue(roles.allows(Action.update, "/application/v4/tenant/t1/application/a1"));
 
-        RoleMembership multiContext = new RoleMembership(Map.of(Role.tenantAdmin,
-                                                         Set.of(Context.limitedTo(TenantName.from("t1"),
-                                                                                  ApplicationName.from("a1"),
-                                                                                  SystemName.main),
-                                                                Context.limitedTo(TenantName.from("t2"),
-                                                                                  ApplicationName.from("a2"),
-                                                                                  SystemName.main))));
+        RoleMembership multiContext = RoleMembership.in(SystemName.main)
+                                                    .add(Role.tenantAdmin).limitedTo(TenantName.from("t1"), ApplicationName.from("a1"))
+                                                    .add(Role.tenantAdmin).limitedTo(TenantName.from("t2"), ApplicationName.from("a2"))
+                                                    .build();
         assertFalse("Deny access to other tenant and app", multiContext.allows(Action.update, "/application/v4/tenant/t3/application/a3"));
         assertTrue(multiContext.allows(Action.update, "/application/v4/tenant/t2/application/a2"));
         assertTrue(multiContext.allows(Action.update, "/application/v4/tenant/t1/application/a1"));
 
-        RoleMembership publicSystem = new RoleMembership(Map.of(Role.tenantAdmin,
-                                                                Set.of(Context.limitedTo(TenantName.from("t1"),
-                                                                                         ApplicationName.from("a1"),
-                                                                                         SystemName.vaas))));
+        RoleMembership publicSystem = RoleMembership.in(SystemName.vaas)
+                                                    .add(Role.tenantAdmin).limitedTo(TenantName.from("t1"), ApplicationName.from("a1"))
+                                                    .build();
         assertFalse(publicSystem.allows(Action.read, "/controller/v1/foo"));
         assertTrue(multiContext.allows(Action.update, "/application/v4/tenant/t1/application/a1"));
     }
 
     @Test
     public void build_service_membership() {
-        RoleMembership roles = new RoleMembership(Map.of(Role.tenantPipelineOperator, Set.of(Context.unlimitedIn(SystemName.main))));
+        RoleMembership roles = RoleMembership.in(SystemName.main)
+                                             .add(Role.tenantPipelineOperator).build();
         assertFalse(roles.allows(Action.create, "/not/explicitly/defined"));
         assertFalse(roles.allows(Action.update, "/application/v4/tenant/t1/application/a1"));
         assertTrue(roles.allows(Action.create, "/application/v4/tenant/t1/application/a1/jobreport"));
@@ -72,11 +66,11 @@ public class RoleMembershipTest {
 
     @Test
     public void multi_role_membership() {
-        RoleMembership roles = new RoleMembership(Map.of(Role.tenantAdmin, Set.of(Context.limitedTo(TenantName.from("t1"),
-                                                                                                    ApplicationName.from("a1"),
-                                                                                                    SystemName.main)),
-                                                         Role.tenantPipelineOperator, Set.of(Context.unlimitedIn(SystemName.main)),
-                                                         Role.everyone, Set.of(Context.unlimitedIn(SystemName.main))));
+        RoleMembership roles = RoleMembership.in(SystemName.main)
+                                             .add(Role.tenantAdmin).limitedTo(TenantName.from("t1"), ApplicationName.from("a1"))
+                                             .add(Role.tenantPipelineOperator)
+                                             .add(Role.everyone)
+                                             .build();
         assertFalse(roles.allows(Action.create, "/not/explicitly/defined"));
         assertFalse(roles.allows(Action.create, "/controller/v1/foo"));
         assertTrue(roles.allows(Action.create, "/application/v4/tenant/t1/application/a1/jobreport"));