feat(roles): Support passing additional user details to `UserRolesPro…

…vider` (#242)

build.gradle

@@ -36,7 +36,7 @@ allprojects {
   apply plugin: 'groovy'
 
   ext {
-    spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.161.1'
+    spinnakerDependenciesVersion = project.hasProperty('spinnakerDependenciesVersion') ? project.property('spinnakerDependenciesVersion') : '0.161.4'
   }
 
   def checkLocalVersions = [spinnakerDependenciesVersion: spinnakerDependenciesVersion]

fiat-file/src/main/java/com/netflix/spinnaker/fiat/roles/file/FileBasedUserRolesProvider.java

@@ -19,6 +19,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
 import com.netflix.spinnaker.fiat.model.resources.Role;
+import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
 import lombok.Data;
 import lombok.extern.slf4j.Slf4j;
@@ -58,18 +59,19 @@ private Map<String, List<Role>> parse(Reader source) throws IOException {
   }
 
   @Override
-  public List<Role> loadRoles(String userId) {
+  public List<Role> loadRoles(ExternalUser user) {
     try {
-      return new ArrayList<>(parse().get(userId));
+      return new ArrayList<>(parse().get(user.getId()));
     } catch (IOException io) {
-      log.error("Couldn't load roles for user " + userId + " from file", io);
+      log.error("Couldn't load roles for user " + user.getId() + " from file", io);
     }
     return Collections.emptyList();
   }
 
   @Override
-  public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userIds) {
+  public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
     try {
+      Collection<String> userIds = users.stream().map(ExternalUser::getId).collect(Collectors.toList());
       return parse().entrySet()
                     .stream()
                     .filter(e -> userIds.contains(e.getKey()))

fiat-file/src/test/groovy/com/netflix/spinnaker/fiat/roles/file/FileBasedUserRolesProviderSpec.groovy

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.fiat.roles.file
 
 import com.google.common.io.Resources
+import com.netflix.spinnaker.fiat.permissions.ExternalUser
 import spock.lang.Specification
 
 class FileBasedUserRolesProviderSpec extends Specification {
@@ -30,21 +31,25 @@ class FileBasedUserRolesProviderSpec extends Specification {
     FileBasedUserRolesProvider provider = new FileBasedUserRolesProvider(configProps: configProps)
 
     when:
-    def result1 = provider.loadRoles("batman")
-    def result2 = provider.loadRoles("foo")
+    def result1 = provider.loadRoles(externalUser("batman"))
+    def result2 = provider.loadRoles(externalUser("foo"))
 
     then:
     result1.name.containsAll(["crimefighter", "jokerjailer"])
     result2.name.containsAll(["bar", "baz"])
 
     when:
-    def result3 = provider.multiLoadRoles(["batman"])
-    def result4 = provider.multiLoadRoles(["batman", "foo"])
+    def result3 = provider.multiLoadRoles([externalUser("batman")])
+    def result4 = provider.multiLoadRoles([externalUser("batman"), externalUser("foo")])
 
     then:
     result3.containsKey("batman")
     !result3.containsKey("foo")
 
     result4.keySet().containsAll("batman", "foo")
   }
+
+  private static ExternalUser externalUser(String id) {
+    return new ExternalUser().setId(id)
+  }
 }

fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java

@@ -20,6 +20,7 @@
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.netflix.spinnaker.fiat.model.resources.Role;
+import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
 import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
 import com.netflix.spinnaker.fiat.roles.github.model.Team;
@@ -100,7 +101,9 @@ public Boolean load(CacheKey key) {
   }
 
   @Override
-  public List<Role> loadRoles(String username) {
+  public List<Role> loadRoles(ExternalUser user) {
+    String username = user.getId();
+
     log.debug("loadRoles for user " + username);
     if (StringUtils.isEmpty(username) || StringUtils.isEmpty(gitHubProperties.getOrganization())) {
       return new ArrayList<>();
@@ -216,13 +219,13 @@ private static Role toRole(String name) {
   }
 
   @Override
-  public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userEmails) {
-    if (userEmails == null || userEmails.isEmpty()) {
+  public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
+    if (users == null || users.isEmpty()) {
       return new HashMap<>();
     }
 
     val emailGroupsMap = new HashMap<String, Collection<Role>>();
-    userEmails.forEach(email -> emailGroupsMap.put(email, loadRoles(email)));
+    users.forEach(u -> emailGroupsMap.put(u.getId(), loadRoles(u)));
 
     return emailGroupsMap;
   }

fiat-google-groups/src/main/java/com/netflix/spinnaker/fiat/roles/google/GoogleDirectoryUserRolesProvider.java

@@ -33,6 +33,7 @@
 import com.google.api.services.admin.directory.model.Group;
 import com.google.api.services.admin.directory.model.Groups;
 import com.netflix.spinnaker.fiat.model.resources.Role;
+import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
 import lombok.Data;
 import lombok.EqualsAndHashCode;
@@ -107,10 +108,12 @@ public void onSuccess(Groups groups, HttpHeaders responseHeaders) throws IOExcep
   }
 
   @Override
-  public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userEmails) {
-    if (userEmails == null || userEmails.isEmpty()) {
+  public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
+    if (users == null || users.isEmpty()) {
       return new HashMap<>();
     }
+
+    Collection<String> userEmails = users.stream().map(ExternalUser::getId).collect(Collectors.toList());
     HashMap<String, Collection<Role>> emailGroupsMap = new HashMap<>();
     Directory service = getDirectoryService();
     BatchRequest batch = service.batch();
@@ -147,12 +150,14 @@ public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userEmail
   }
 
   @Override
-  public List<Role> loadRoles(String userEmail) {
-    if (userEmail == null || userEmail.isEmpty()) {
+  public List<Role> loadRoles(ExternalUser user) {
+    if (user == null || user.getId().isEmpty()) {
       return new ArrayList<>();
     }
 
+    String userEmail = user.getId();
     Directory service = getDirectoryService();
+
     try {
       Groups groups = service.groups().list().setDomain(config.getDomain()).setUserKey(userEmail).execute();
       if (groups == null || groups.getGroups() == null || groups.getGroups().isEmpty()) {

fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java

@@ -57,7 +57,9 @@ public class LdapUserRolesProvider implements UserRolesProvider {
   private LdapConfig.ConfigProps configProps;
 
   @Override
-  public List<Role> loadRoles(String userId) {
+  public List<Role> loadRoles(ExternalUser user) {
+    String userId = user.getId();
+
     log.debug("loadRoles for user " + userId);
     if (StringUtils.isEmpty(configProps.getGroupSearchBase())) {
       return new ArrayList<>();
@@ -92,15 +94,15 @@ public List<Role> loadRoles(String userId) {
   }
 
   @Override
-  public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userIds) {
+  public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
     if (StringUtils.isEmpty(configProps.getGroupSearchBase())) {
       return new HashMap<>();
     }
 
     // ExternalUser is used here as a simple data type to hold the username/roles combination.
-    return userIds.stream()
-                  .map(userId -> new ExternalUser().setId(userId).setExternalRoles(loadRoles(userId)))
-                  .collect(Collectors.toMap(ExternalUser::getId, ExternalUser::getExternalRoles));
+    return users.stream()
+        .map(u -> u.setExternalRoles(loadRoles(u)))
+        .collect(Collectors.toMap(ExternalUser::getId, ExternalUser::getExternalRoles));
   }
 
   private String getUserFullDn(String userId) {

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolver.java

@@ -84,7 +84,7 @@ public UserPermission resolveAndMerge(@NonNull ExternalUser user) {
     List<Role> roles;
     try {
       log.debug("Loading roles for user " + user);
-      roles = userRolesProvider.loadRoles(user.getId());
+      roles = userRolesProvider.loadRoles(user);
       log.debug("Got roles " + roles + " for user " + user);
     } catch (ProviderException pe) {
       throw new PermissionResolutionException("Failed to resolve user permission for user " + user.getId(), pe);
@@ -141,11 +141,7 @@ public Map<String, UserPermission> resolve(@NonNull Collection<ExternalUser> use
   }
 
   private Map<String, Collection<Role>> getAndMergeUserRoles(@NonNull Collection<ExternalUser> users) {
-    List<String> usernames = users.stream()
-                                  .map(ExternalUser::getId)
-                                  .collect(Collectors.toList());
-
-    Map<String, Collection<Role>> userToRoles = userRolesProvider.multiLoadRoles(usernames);
+    Map<String, Collection<Role>> userToRoles = userRolesProvider.multiLoadRoles(users);
 
     users.forEach(user -> {
       userToRoles.computeIfAbsent(user.getId(), ignored -> new ArrayList<>())

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesProvider.java

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.fiat.roles;
 
 import com.netflix.spinnaker.fiat.model.resources.Role;
+import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 
 import java.util.Collection;
 import java.util.List;
@@ -27,17 +28,17 @@ public interface UserRolesProvider {
   /**
    * Load the roles assigned to the {@link com.netflix.spinnaker.security.User User}.
    *
-   * @param userId identify a user. Can be email or username.
+   * @param user to load roles for
    * @return Roles assigned to the {@link com.netflix.spinnaker.security.User User} with the given userEmail.
    */
-  List<Role> loadRoles(String userId);
+  List<Role> loadRoles(ExternalUser user);
 
   /**
    * Load the roles assigned to each {@link com.netflix.spinnaker.security.User User's} email in userEmails.
    *
-   * @param userIds
+   * @param users to load roles for
    * @return Map whose keys are the {@link com.netflix.spinnaker.security.User User's} email and values are their
    * assigned roles.
    */
-  Map<String, Collection<Role>> multiLoadRoles(Collection<String> userIds);
+  Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users);
 }

fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolverSpec.groovy

@@ -116,7 +116,7 @@ class DefaultPermissionsResolverSpec extends Specification {
     def result = resolver.resolve("unknownUser")
 
     then:
-    1 * userRolesProvider.loadRoles("unknownUser") >> []
+    1 * userRolesProvider.loadRoles({ u -> u.getId() == "unknownUser" }) >> []
     def expected = new UserPermission().setId("unknownUser")
     result == expected
 
@@ -125,14 +125,14 @@ class DefaultPermissionsResolverSpec extends Specification {
     expected = new UserPermission().setId(testUserId)
 
     then:
-    1 * userRolesProvider.loadRoles(testUserId) >> []
+    1 * userRolesProvider.loadRoles({ u -> u.getId() == testUserId }) >> []
     result == expected
 
     when:
     result = resolver.resolve(testUserId)
 
     then:
-    1 * userRolesProvider.loadRoles(testUserId) >> [role2]
+    1 * userRolesProvider.loadRoles({ u -> u.getId() == testUserId }) >> [role2]
     expected.setAccounts([reqGroup1and2Acct] as Set)
             .setServiceAccounts([group2SvcAcct] as Set)
             .setRoles([role2] as Set)
@@ -142,7 +142,7 @@ class DefaultPermissionsResolverSpec extends Specification {
     result = resolver.resolveAndMerge(testUser)
 
     then:
-    1 * userRolesProvider.loadRoles(testUserId) >> [role2]
+    1 * userRolesProvider.loadRoles({ u -> u.getId() == testUserId }) >> [role2]
     expected.setAccounts([reqGroup1Acct, reqGroup1and2Acct] as Set)
             .setServiceAccounts([group1SvcAcct, group2SvcAcct] as Set)
             .setRoles([role1, role2] as Set)
@@ -168,7 +168,7 @@ class DefaultPermissionsResolverSpec extends Specification {
     def result = resolver.resolveAndMerge(testUser)
 
     then:
-    1 * userRolesProvider.loadRoles(testUserId) >> [role1]
+    1 * userRolesProvider.loadRoles({ u -> u.getId() == testUserId }) >> [role1]
     def expected = new UserPermission().setId("testUserId")
     expected.setRoles([role1] as Set).setAdmin(true)
             .setServiceAccounts([group1SvcAcct, group2SvcAcct] as Set)

fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/FiatConfig.java

@@ -3,6 +3,7 @@
 import com.google.common.collect.ImmutableList;
 import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.fiat.model.resources.Role;
+import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
 import com.netflix.spinnaker.filters.AuthenticatedRequestFilter;
 import com.netflix.spinnaker.kork.web.interceptors.MetricsInterceptor;
@@ -56,12 +57,12 @@ public void configureContentNegotiation(ContentNegotiationConfigurer configurer)
   UserRolesProvider defaultUserRolesProvider() {
     return new UserRolesProvider() {
       @Override
-      public Map<String, Collection<Role>> multiLoadRoles(Collection<String> userIds) {
+      public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
         return new HashMap<>();
       }
 
       @Override
-      public List<Role> loadRoles(String userId) {
+      public List<Role> loadRoles(ExternalUser user) {
         return new ArrayList<>();
       }
     };

fiat-web/src/test/groovy/com/netflix/spinnaker/config/TestUserRoleProviderConfig.groovy

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.config
 
 import com.netflix.spinnaker.fiat.model.resources.Role
+import com.netflix.spinnaker.fiat.permissions.ExternalUser
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider
 import org.springframework.context.annotation.Bean
 import org.springframework.context.annotation.Configuration
@@ -34,14 +35,14 @@ class TestUserRoleProviderConfig {
     Map<String, List<Role>> userToRoles = new HashMap<>();
 
     @Override
-    List<Role> loadRoles(String userId) {
-      return userToRoles.get(userId) ?: []
+    List<Role> loadRoles(ExternalUser user) {
+      return userToRoles.get(user.getId()) ?: []
     }
 
     @Override
-    Map<String, Collection<Role>> multiLoadRoles(Collection<String> userIds) {
-      return userIds.collectEntries{ String userId ->
-        [(userId): loadRoles(userId) ]
+    Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> users) {
+      return users.collectEntries { u ->
+        [(u.getId()): loadRoles(u) ]
       }
     }
   }