Cleaner RoleMembership and test for AthenzRoleFilter

3 files changed, 65 insertions, 67 deletions

diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Role.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Role.java
index 75f9fa6faa2..d3f43ad895f 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Role.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/role/Role.java
@@ -111,26 +111,17 @@ public class Role implements RoleInSystem, RoleInSystemWithTenant, RoleInSystemW
 
     @Override
     public RoleMembership limitedTo(SystemName system) {
-        return new RoleWithContext(this, Context.unlimitedIn(system));
+        return new RoleMembership(Map.of(this, Set.of(Context.unlimitedIn(system))));
     }
 
     @Override
     public RoleMembership limitedTo(TenantName tenant, SystemName system) {
-        return new RoleWithContext(this, Context.limitedTo(tenant, system));
+        return new RoleMembership(Map.of(this, Set.of(Context.limitedTo(tenant, system))));
     }
 
     @Override
     public RoleMembership limitedTo(ApplicationName application, TenantName tenant, SystemName system) {
-        return new RoleWithContext(this, Context.limitedTo(tenant, application, system));
-    }
-
-
-    public static class RoleWithContext extends RoleMembership { // TODO fix.
-
-        private RoleWithContext(Role role, Context context) {
-            super(Map.of(role, Set.of(context)));
-        }
-
+        return new RoleMembership(Map.of(this, Set.of(Context.limitedTo(tenant, application, system))));
     }
 
 }
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 034b7567609..e0311bebbba 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
@@ -2,10 +2,9 @@
 package com.yahoo.vespa.hosted.controller.role;
 
 import java.net.URI;
-import java.security.Principal;
 import java.util.Collections;
 import java.util.Map;
-import java.util.Optional;
+import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -17,7 +16,7 @@ import java.util.stream.Stream;
  * @author mpolden
  * @author jonmv
  */
-public class RoleMembership { // TODO replace with Set<RoleWithContext>
+public class RoleMembership {
 
     private final Map<Role, Set<Context>> roles;
 
@@ -59,4 +58,16 @@ public class RoleMembership { // TODO replace with Set<RoleWithContext>
         return "roles " + roles;
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if ( ! (o instanceof RoleMembership)) return false;
+        return Objects.equals(roles, ((RoleMembership) o).roles);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(roles);
+    }
+
 }
diff --git a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java
index bd6bccb1150..b0084c38754 100644
--- a/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java
+++ b/controller-server/src/test/java/com/yahoo/vespa/hosted/controller/restapi/filter/AthenzRoleFilterTest.java
@@ -1,6 +1,5 @@
 package com.yahoo.vespa.hosted.controller.restapi.filter;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
 import com.yahoo.config.provision.ApplicationName;
 import com.yahoo.config.provision.TenantName;
 import com.yahoo.jdisc.http.filter.security.cors.CorsFilterConfig;
@@ -16,16 +15,12 @@ import com.yahoo.vespa.hosted.controller.athenz.HostedAthenzIdentities;
 import com.yahoo.vespa.hosted.controller.athenz.impl.AthenzFacade;
 import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzClientFactoryMock;
 import com.yahoo.vespa.hosted.controller.athenz.mock.AthenzDbMock;
-import com.yahoo.vespa.hosted.controller.role.Context;
 import com.yahoo.vespa.hosted.controller.role.Role;
 import org.junit.Before;
 import org.junit.Test;
 
 import java.net.URI;
-import java.util.Optional;
-import java.util.Set;
 
-import static java.util.Collections.emptySet;
 import static org.junit.Assert.assertEquals;
 
 /**
@@ -71,53 +66,54 @@ public class AthenzRoleFilterTest {
     @Test
     public void testTranslations() {
 
-        // Only unprivileged users are members of the everyone role.
-        assertEquals(emptySet(),
-                     filter.membership(HOSTED_OPERATOR, APPLICATION_CONTEXT_PATH).contextsFor(Role.everyone));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_ADMIN, TENANT_CONTEXT_PATH).contextsFor(Role.everyone));
-        assertEquals(Set.of(Context.unlimitedIn(tester.controller().system())),
-                     filter.membership(TENANT_ADMIN, TENANT2_CONTEXT_PATH).contextsFor(Role.everyone));
-        assertEquals(Set.of(Context.unlimitedIn(tester.controller().system())),
-                     filter.membership(TENANT_PIPELINE, NO_CONTEXT_PATH).contextsFor(Role.everyone));
-        assertEquals(Set.of(Context.unlimitedIn(tester.controller().system())),
-                     filter.membership(USER, APPLICATION_CONTEXT_PATH).contextsFor(Role.everyone));
-
-        // Only operators are members of the operator role.
-        assertEquals(Set.of(Context.unlimitedIn(tester.controller().system())),
-                     filter.membership(HOSTED_OPERATOR, TENANT_CONTEXT_PATH).contextsFor(Role.hostedOperator));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_ADMIN, NO_CONTEXT_PATH).contextsFor(Role.hostedOperator));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_PIPELINE, APPLICATION_CONTEXT_PATH).contextsFor(Role.hostedOperator));
-        assertEquals(emptySet(),
-                     filter.membership(USER, TENANT_CONTEXT_PATH).contextsFor(Role.hostedOperator));
-
-        // Only tenant admins are tenant admins of their tenants.
-        assertEquals(emptySet(),
-                     filter.membership(HOSTED_OPERATOR, APPLICATION_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-        assertEquals(emptySet(), // TODO this is wrong, but we can't do better until we ask ZMS for roles.
-                     filter.membership(TENANT_ADMIN, NO_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-        assertEquals(Set.of(Context.limitedTo(TENANT, tester.controller().system())),
-                     filter.membership(TENANT_ADMIN, TENANT_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_ADMIN, TENANT2_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_PIPELINE, APPLICATION_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-        assertEquals(emptySet(),
-                     filter.membership(USER, TENANT_CONTEXT_PATH).contextsFor(Role.athenzTenantAdmin));
-
-        // Only build services are pipeline operators of their applications.
-        assertEquals(emptySet(),
-                     filter.membership(HOSTED_OPERATOR, APPLICATION_CONTEXT_PATH).contextsFor(Role.tenantPipeline));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_ADMIN, APPLICATION_CONTEXT_PATH).contextsFor(Role.tenantPipeline));
-        assertEquals(Set.of(Context.limitedTo(TENANT, APPLICATION, tester.controller().system())),
-                     filter.membership(TENANT_PIPELINE, APPLICATION_CONTEXT_PATH).contextsFor(Role.tenantPipeline));
-        assertEquals(emptySet(),
-                     filter.membership(TENANT_PIPELINE, APPLICATION2_CONTEXT_PATH).contextsFor(Role.tenantPipeline));
-        assertEquals(emptySet(),
-                     filter.membership(USER, APPLICATION_CONTEXT_PATH).contextsFor(Role.tenantPipeline));
+        // Hosted operators are always members of the hostedOperator role.
+        assertEquals(Role.hostedOperator.limitedTo(tester.controller().system()),
+                     filter.membership(HOSTED_OPERATOR, NO_CONTEXT_PATH));
+
+        assertEquals(Role.hostedOperator.limitedTo(tester.controller().system()),
+                     filter.membership(HOSTED_OPERATOR, TENANT_CONTEXT_PATH));
+
+        assertEquals(Role.hostedOperator.limitedTo(tester.controller().system()),
+                     filter.membership(HOSTED_OPERATOR, APPLICATION_CONTEXT_PATH));
+
+        // Tenant admins are members of the athenzTenantAdmin role within their tenant subtree.
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_PIPELINE, NO_CONTEXT_PATH));
+
+        assertEquals(Role.athenzTenantAdmin.limitedTo(TENANT, tester.controller().system()),
+                     filter.membership(TENANT_ADMIN, TENANT_CONTEXT_PATH));
+
+        assertEquals(Role.athenzTenantAdmin.limitedTo(TENANT, tester.controller().system()),
+                     filter.membership(TENANT_ADMIN, APPLICATION_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_ADMIN, TENANT2_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_ADMIN, APPLICATION2_CONTEXT_PATH));
+
+        // Build services are members of the tenantPipeline role within their application subtree.
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_PIPELINE, NO_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_PIPELINE, TENANT_CONTEXT_PATH));
+
+        assertEquals(Role.tenantPipeline.limitedTo(APPLICATION, TENANT, tester.controller().system()),
+                     filter.membership(TENANT_PIPELINE, APPLICATION_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(TENANT_PIPELINE, APPLICATION2_CONTEXT_PATH));
+
+        // Unprivileged users are just members of the everyone role.
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(USER, NO_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(USER, TENANT_CONTEXT_PATH));
+
+        assertEquals(Role.everyone.limitedTo(tester.controller().system()),
+                     filter.membership(USER, APPLICATION_CONTEXT_PATH));
     }
 
 }