From 65928abdf774c5f2fba0d8b83d2c073353379595 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Tue, 26 Mar 2019 21:52:15 +0100 Subject: Change API for building RoleMemberships to reflect the single-system property --- .../filter/ControllerAuthorizationFilter.java | 28 ++++---- .../hosted/controller/role/RoleMembership.java | 78 +++++++++++++++++++--- .../restapi/application/ApplicationApiTest.java | 4 +- .../hosted/controller/role/RoleMembershipTest.java | 46 ++++++------- 4 files changed, 105 insertions(+), 51 deletions(-) (limited to 'controller-server') 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> memberships = new HashMap<>(); + RoleMembership.Builder memberships = RoleMembership.in(system); AthenzIdentity identity = ((AthenzPrincipal) principal).getIdentity(); Optional 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 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() { 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> roles; - public RoleMembership(Map> 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> 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> 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")); -- cgit v1.2.3