fix(ldap): Allow to use any ldap attribute as user id in batch

roles/sync

Author: krasilnikov-dmitriy

fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/config/LdapConfig.java

@@ -55,10 +55,8 @@ public static class ConfigProps {
     MessageFormat userDnPattern = new MessageFormat("uid={0},ou=users");
     String userSearchBase = "";
     String userSearchFilter;
+    String userIdAttribute;
     String groupSearchFilter = "(uniqueMember={0})";
     String groupRoleAttributes = "cn";
-    String groupUserAttributes = "";
-
-    int thresholdToUseGroupMembership = 100;
   }
 }

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

@@ -20,13 +20,10 @@
 import com.netflix.spinnaker.fiat.model.resources.Role;
 import com.netflix.spinnaker.fiat.permissions.ExternalUser;
 import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
-import java.text.MessageFormat;
-import java.text.ParseException;
 import java.util.*;
 import java.util.stream.Collectors;
 import javax.naming.InvalidNameException;
 import javax.naming.Name;
-import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
 import javax.naming.directory.Attributes;
 import lombok.Setter;
@@ -100,22 +97,27 @@ public List<Role> loadRoles(ExternalUser user) {
         .collect(Collectors.toList());
   }
 
-  private class UserGroupMapper implements AttributesMapper<List<Pair<String, Role>>> {
-    public List<Pair<String, Role>> mapFromAttributes(Attributes attrs) throws NamingException {
-      String group = attrs.get(configProps.getGroupRoleAttributes()).get().toString();
-      Role role = new Role(group).setSource(Role.Source.LDAP);
-      List<Pair<String, Role>> member = new ArrayList<>();
-      for (NamingEnumeration<?> members = attrs.get(configProps.getGroupUserAttributes()).getAll();
-          members.hasMore(); ) {
-        try {
-          String user =
-              String.valueOf(configProps.getUserDnPattern().parse(members.next().toString())[0]);
-          member.add(Pair.of(user, role));
-        } catch (ParseException e) {
-          e.printStackTrace();
-        }
-      }
-      return member;
+  private class RoleFullDNtoUserRoleMapper implements AttributesMapper<Pair<String, Role>> {
+    @Override
+    public Pair<String, Role> mapFromAttributes(Attributes attrs) throws NamingException {
+      return Pair.of(
+          attrs.get("distinguishedname").get().toString(),
+          new Role(attrs.get(configProps.getGroupRoleAttributes()).get().toString())
+              .setSource(Role.Source.LDAP));
+    }
+  }
+
+  private class UserGroupMapper implements AttributesMapper<Pair<String, Role>> {
+
+    private Role role;
+
+    public UserGroupMapper(Role role) {
+      this.role = role;
+    }
+
+    @Override
+    public Pair<String, Role> mapFromAttributes(Attributes attrs) throws NamingException {
+      return Pair.of(attrs.get(configProps.getUserIdAttribute()).get().toString(), role);
     }
   }
 
@@ -125,30 +127,25 @@ public Map<String, Collection<Role>> multiLoadRoles(Collection<ExternalUser> use
       return new HashMap<>();
     }
 
-    if (users.size() > configProps.getThresholdToUseGroupMembership()
-        && StringUtils.isNotEmpty(configProps.getGroupUserAttributes())) {
-      Set<String> userIds = users.stream().map(ExternalUser::getId).collect(Collectors.toSet());
-      return ldapTemplate
-          .search(
-              configProps.getGroupSearchBase(),
-              MessageFormat.format(
-                  configProps.getGroupSearchFilter(),
-                  "*",
-                  "*"), // Passing two wildcard params like loadRoles
-              new UserGroupMapper())
-          .stream()
-          .flatMap(List::stream)
-          .filter(p -> userIds.contains(p.getKey()))
-          .collect(
-              Collectors.groupingBy(
-                  Pair::getKey,
-                  Collectors.mapping(Pair::getValue, Collectors.toCollection(ArrayList::new))));
-    }
-
-    // ExternalUser is used here as a simple data type to hold the username/roles combination.
-    return users.stream()
-        .map(u -> new ExternalUser().setId(u.getId()).setExternalRoles(loadRoles(u)))
-        .collect(Collectors.toMap(ExternalUser::getId, ExternalUser::getExternalRoles));
+    Set<String> userIds = users.stream().map(ExternalUser::getId).collect(Collectors.toSet());
+    return ldapTemplate
+        .search(
+            configProps.getGroupSearchBase(),
+            configProps.getGroupSearchFilter(),
+            new RoleFullDNtoUserRoleMapper())
+        .stream()
+        .map(
+            r ->
+                ldapTemplate.search(
+                    configProps.getUserSearchBase(),
+                    String.format("(&(objectCategory=user)(memberOf=%s))", r.getKey()),
+                    new UserGroupMapper(r.getValue())))
+        .flatMap(List::stream)
+        .filter(p -> userIds.contains(p.getKey()))
+        .collect(
+            Collectors.groupingBy(
+                Pair::getKey,
+                Collectors.mapping(Pair::getValue, Collectors.toCollection(ArrayList::new))));
   }
 
   private String getUserFullDn(String userId) {

fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy

@@ -86,58 +86,29 @@ class LdapUserRolesProviderTest extends Specification {
     "notEmpty"     |_
   }
 
-  void "multiLoadRoles should use loadRoles when groupUserAttributes is empty"() {
+  void "multiLoadRoles should call ldap only N+1 times for groups and users in each group"() {
     given:
     def users = [externalUser("user1"), externalUser("user2")]
     def role1 = new Role("group1")
     def role2 = new Role("group2")
 
     def configProps = baseConfigProps()
-    def provider = Spy(LdapUserRolesProvider){
-      loadRoles(_ as ExternalUser) >>> [[role1], [role2]]
-    }.setConfigProps(configProps)
+    configProps.groupSearchBase = "group search base"
+    configProps.userSearchBase = "user search base"
+    configProps.userIdAttribute = "sAMAccountName"
 
-    when:
-    configProps.groupSearchBase = ""
-    def roles = provider.multiLoadRoles(users)
+    provider.configProps = configProps
 
-    then:
-    roles == [:]
+    provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) {
+      1 * search(configProps.groupSearchBase, *_) >> [Pair.of("group1 dn", role1) , Pair.of("group2 dn", role2)]
+      1 * search(configProps.userSearchBase, { it.contains("group1 dn") }, _) >> [Pair.of("user1", role1)]
+      1 * search(configProps.userSearchBase, { it.contains("group2 dn") }, _) >> [Pair.of("user2", role2)]
+    }
 
     when:
-    configProps.groupSearchBase = "notEmpty"
-    roles = provider.multiLoadRoles(users)
-
-    then:
-    roles == [user1: [role1], user2: [role2]]
-  }
-
-  void "multiLoadRoles should use groupUserAttributes when groupUserAttributes is not empty"() {
-    given:
-    def users = [externalUser("user1"), externalUser("user2")]
-    def role1 = new Role("group1")
-    def role2 = new Role("group2")
-
-    def configProps = baseConfigProps().setGroupSearchBase("notEmpty").setGroupUserAttributes("member")
-    def provider = Spy(LdapUserRolesProvider){
-      2 * loadRoles(_) >>> [[role1], [role2]]
-    }.setConfigProps(configProps)
-
-    when: "thresholdToUseGroupMembership is too high"
-    configProps.thresholdToUseGroupMembership = 100
     def roles = provider.multiLoadRoles(users)
 
-    then: "should use loadRoles"
-    roles == [user1: [role1], user2: [role2]]
-
-    when: "users count is greater than thresholdToUseGroupMembership"
-    configProps.thresholdToUseGroupMembership = 1
-    provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) {
-      1 * search(*_) >> [[Pair.of("user1",role1)], [Pair.of("user2", role2)], [Pair.of("unknown", role2)]]
-    }
-    roles = provider.multiLoadRoles(users)
-
-    then: "should use ldapTemplate.search method"
+    then:
     roles == [user1: [role1], user2: [role2]]
   }