From 1c367c520e0541ec200af54f26903fae67a41d34 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Tue, 30 Jul 2024 22:56:42 +0530 Subject: [PATCH 1/3] test(ldap/roles): strengthen assertions in existing tests --- .../ldap/LdapUserRolesProviderTest.groovy | 94 ++++++++++++++----- 1 file changed, 70 insertions(+), 24 deletions(-) diff --git a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy index a071f54c9..dbb387a26 100644 --- a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy +++ b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy @@ -23,31 +23,35 @@ import org.apache.commons.lang3.tuple.Pair import org.springframework.dao.IncorrectResultSizeDataAccessException import org.springframework.ldap.control.PagedResultsDirContextProcessor import org.springframework.security.ldap.SpringSecurityLdapTemplate -import spock.lang.Shared import spock.lang.Specification -import spock.lang.Subject import spock.lang.Unroll import javax.naming.directory.SearchControls class LdapUserRolesProviderTest extends Specification { - @Shared - @Subject - def provider = new LdapUserRolesProvider() - @Unroll void "loadRoles should return no roles for serviceAccounts when userSearchFilter present"() { given: - def user = new ExternalUser(id: 'foo', externalRoles: [new Role(name: 'bar')]) + def id = 'foo' + def user = new ExternalUser(id: id, externalRoles: [new Role(name: 'bar')]) def configProps = baseConfigProps() configProps.groupSearchBase = groupSearchBase configProps.userSearchFilter = "notEmpty" + def provider = Spy(LdapUserRolesProvider){ + 1 * setConfigProps(configProps) + 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) + 1 * loadRoles(user) + 1 * loadRolesForUser(user) + (0..1) * getUserFullDn(id) + 0 * _ + } provider.configProps = configProps provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) { - _ * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } + (0..1) * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } + 0 * _ } when: @@ -66,15 +70,26 @@ class LdapUserRolesProviderTest extends Specification { @Unroll void "loadRoles should return no roles for serviceAccouts when userSearchFilter absent"() { given: - def user = new ExternalUser(id: 'foo', externalRoles: [new Role(name: 'bar')]) + def id = 'id' + def user = new ExternalUser(id: id, externalRoles: [new Role(name: 'bar')]) def configProps = baseConfigProps() configProps.groupSearchBase = groupSearchBase + def provider = Spy(LdapUserRolesProvider){ + 1 * setConfigProps(configProps) + 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) + 1 * loadRoles(user) + 1 * loadRolesForUser(user) + (0..1) * getUserFullDn(id) + 0 * _ + } + provider.configProps = configProps provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) { (0..1) * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } (0..1) * searchForSingleAttributeValues(*_) >> new HashSet<>() + 0 * _ } when: @@ -97,7 +112,9 @@ class LdapUserRolesProviderTest extends Specification { def configProps = baseConfigProps() def provider = Spy(LdapUserRolesProvider){ - loadRoles(_ as ExternalUser) >>> [[role1], [role2]] + 2 * loadRoles(_ as ExternalUser) >>> [[role1], [role2]] + 1 * setConfigProps(configProps) + 0 * _ } provider.setConfigProps(configProps) @@ -107,6 +124,8 @@ class LdapUserRolesProviderTest extends Specification { then: roles == [user1: [], user2: []] + 1 * provider.multiLoadRoles(users) + 1 * provider.loadRolesForUsers(users) when: configProps.groupSearchBase = "notEmpty" @@ -114,6 +133,8 @@ class LdapUserRolesProviderTest extends Specification { then: roles == [user1: [role1], user2: [role2]] + 1 * provider.multiLoadRoles(users) + 1 * provider.loadRolesForUsers(users) } void "multiLoadRoles should use groupUserAttributes when groupUserAttributes is not empty"() { @@ -127,7 +148,9 @@ class LdapUserRolesProviderTest extends Specification { .setGroupSearchBase("ou=groups") .setGroupUserAttributes("member") def provider = Spy(LdapUserRolesProvider){ - 2 * loadRoles(_) >>> [[role1], [role2]] + 2 * loadRoles(_ as ExternalUser) >>> [[role1], [role2]] + 1 * setConfigProps(configProps) + 0 * _ } provider.setConfigProps(configProps) @@ -136,6 +159,8 @@ class LdapUserRolesProviderTest extends Specification { def roles = provider.multiLoadRoles(users) then: "should use loadRoles" + 1 * provider.multiLoadRoles(users) + 1 * provider.loadRolesForUsers(users) roles == [user1: [role1], user2: [role2]] when: "users count is greater than thresholdToUseGroupMembership and enableDnBasedMultiLoad is false" @@ -146,14 +171,16 @@ class LdapUserRolesProviderTest extends Specification { [Pair.of("user2", role2)], [Pair.of("unknown", role2)] ] + 0 * _ } roles = provider.multiLoadRoles(users) then: "should compile user DNs locally and query memberships for all groups to load roles" roles == [user1: [role1], user2: [role2]] users.each {0 * provider.getUserFullDn(it.id) } - 0 * provider.doMultiLoadRoles(_) - 0 * provider.doMultiLoadRolesPaginated(_) + 1 * provider.setLdapTemplate(_ as SpringSecurityLdapTemplate) + 1 * provider.multiLoadRoles(users) + 1 * provider.loadRolesForUsers(users) when: "thresholdToUseGroupMembership is breached, userSearchFilter is empty and enableDnBasedMultiLoad is true" // Test to make sure that when the thresholdToUseGroupMembership is breached: @@ -168,14 +195,18 @@ class LdapUserRolesProviderTest extends Specification { [Pair.of("uid=user2,ou=users,dc=springframework,dc=org", role2)], [Pair.of("unknown", role2)] ] + 0 * _ } roles = provider.multiLoadRoles(users) then: "should compile user DNs locally and query memberships for all groups to load roles" roles == [user1: [role1], user2: [role2]] users.each {1 * provider.getUserFullDn(it.id) } + 1 * provider.multiLoadRoles(users) + 1 * provider.setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * provider.doMultiLoadRoles(_) - 0 * provider.doMultiLoadRolesPaginated(_) + 1 * provider.getUserDNs(_ as Collection) + 1 * provider.loadRolesForUsers(users) when: "thresholdToUseGroupMembership is breached and userSearchFilter is set" // Test to make sure that when the thresholdToUseGroupMembership is breached: @@ -199,9 +230,11 @@ class LdapUserRolesProviderTest extends Specification { then: "should fetch user DNs from LDAP and query memberships for all groups to load roles" roles == ["user1@foo.com": [role1], "user2@foo.com": [role2]] - 0 * provider.getUserFullDn(_) + 1 * provider.multiLoadRoles(_ as Collection) + 1 * provider.setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * provider.doMultiLoadRoles(_) - 0 * provider.doMultiLoadRolesPaginated(_) + 1 * provider.getUserDNs(_ as Collection) + 1 * provider.loadRolesForUsers(_ as Collection) } void "multiLoadRoles should use pagination when enabled"() { @@ -211,7 +244,11 @@ class LdapUserRolesProviderTest extends Specification { def role2 = new Role("group2") def configProps = baseConfigProps() - provider = Spy(provider) + def provider = Spy(LdapUserRolesProvider){ + 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) + 1 * setConfigProps(_ as LdapConfig.ConfigProps) + 0 * _ + } provider.setConfigProps(configProps) when: "pagination is enabled" @@ -244,6 +281,7 @@ class LdapUserRolesProviderTest extends Specification { [Pair.of("${it.id}@foo.com" as String, role1), Pair.of("${it.id}@foo.com" as String, role2)] }] + 0 * _ } def spiedProcessor = Spy(new PagedResultsDirContextProcessor(5, null)) 1 * provider.getPagedResultsDirContextProcessor(_) >> spiedProcessor @@ -253,23 +291,29 @@ class LdapUserRolesProviderTest extends Specification { then: "should fetch ldap roles using pagination" roles == users.collectEntries { ["${it.id}" as String, [role1, role2]] } - 0 * provider.doMultiLoadRoles(_) - 1 * provider.doMultiLoadRolesPaginated(_) + 1 * provider.multiLoadRoles(users) + 1 * provider.doMultiLoadRolesPaginated(_ as Collection) + 1 * provider.loadRolesForUsers(_ as Collection) + 1 * provider.getUserDNs(_ as Collection) } void "multiLoadRoles should merge roles when multiple DNs exist for a user id"(){ given: - def users = [externalUser("user1")] + def id = 'user1' + def ids = [id] as Set + def users = [externalUser(id)] def role1 = new Role("group1") def role2 = new Role("group2") def role3 = new Role("group3") def configProps = baseConfigProps() def provider = Spy(LdapUserRolesProvider){ - getUserDNs(_ as Collection) >> ['uid=dn1,ou=users,dc=springframework,dc=org': 'user1', - 'uid=dn2,ou=users,dc=springframework,dc=org': 'user1'] - doMultiLoadRoles(_ as Collection) >> ['uid=dn1,ou=users,dc=springframework,dc=org' : [role1,role2], - 'uid=dn2,ou=users,dc=springframework,dc=org': [role3] ] + 1 * getUserDNs(ids) >> ['uid=dn1,ou=users,dc=springframework,dc=org': id, + 'uid=dn2,ou=users,dc=springframework,dc=org': id] + 1 * doMultiLoadRoles(_) >> ['uid=dn1,ou=users,dc=springframework,dc=org' : [role1,role2], + 'uid=dn2,ou=users,dc=springframework,dc=org': [role3] ] + 1 * setConfigProps(configProps) + 0 * _ } provider.setConfigProps(configProps) @@ -282,6 +326,8 @@ class LdapUserRolesProviderTest extends Specification { then: roles == [user1: [role1, role2, role3]] + 1 * provider.multiLoadRoles(users) + 1 * provider.loadRolesForUsers(users) } From 6e711209e204597d89b74d1c7a1c107fd0b0ff57 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Tue, 30 Jul 2024 23:28:33 +0530 Subject: [PATCH 2/3] test(ldap/roles): add test to demonstrate the bug when multiple DNs exist for a user id. --- .../ldap/LdapUserRolesProviderTest.groovy | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy index dbb387a26..8acdb4c38 100644 --- a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy +++ b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy @@ -330,6 +330,34 @@ class LdapUserRolesProviderTest extends Specification { 1 * provider.loadRolesForUsers(users) } + void "loadRolesForUser returns no roles when multiple DNs exist for a user id"(){ + given: + def id = 'user1' + def user = externalUser(id) + + def configProps = baseConfigProps() + + def provider = Spy(LdapUserRolesProvider) { + 1 * setConfigProps(configProps) + 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) + 1 * loadRolesForUser(user) + 1 * getUserFullDn(id) + 0 * _ + } + provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) { + 1 * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } //due to multiple DNs + 0 * _ + } + provider.setConfigProps(configProps) + + when: + configProps.groupSearchBase = "notEmpty" + configProps.userSearchFilter = "notEmpty" + def roles = provider.loadRolesForUser(user) + + then: + roles == [] + } private static ExternalUser externalUser(String id) { return new ExternalUser().setId(id) From 7ac5388db2585d3602d401f2060e3624faab1110 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Fri, 19 Jul 2024 01:04:31 +0530 Subject: [PATCH 3/3] fix(ldap/roles): fix the issue of multiple entries in Ldap for single user * update tests as per the new code changes --- .../roles/ldap/LdapUserRolesProvider.java | 107 +++++++++++------- .../ldap/LdapUserRolesProviderTest.groovy | 25 +++- 2 files changed, 86 insertions(+), 46 deletions(-) diff --git a/fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java b/fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java index 80d3968fd..eefc66803 100644 --- a/fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java +++ b/fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java @@ -71,50 +71,69 @@ public void setConfigProps(LdapConfig.ConfigProps configProps) { @Override protected List loadRolesForUser(ExternalUser user) { String userId = user.getId(); - + Set userDns = new HashSet<>(); + String[] params; + Set userRolesPerDn; + Set userRoles = new HashSet<>(); log.debug("loadRoles for user " + userId); if (StringUtils.isEmpty(configProps.getGroupSearchBase())) { return new ArrayList<>(); } - String fullUserDn = getUserFullDn(userId); + // handle multiple DNs in Ldap for single user + if (!isSingleDnToUserId(userId)) { + Map userToDnMap = getUserDNs(List.of(userId)); + userDns = new HashSet<>(userToDnMap.keySet()); + log.debug("userDns : {} for the user : {}", userDns, userId); + } else { + String fullUserDn = getUserFullDn(userId); + if (fullUserDn != null) { + userDns = Set.of(fullUserDn); + } + } - if (fullUserDn == null) { + if (userDns.isEmpty()) { // Likely a service account - log.debug("fullUserDn is null for {}", userId); + log.debug("userDn is empty for {}", userId); return new ArrayList<>(); } - String[] params = new String[] {fullUserDn, userId}; + for (String userDn : userDns) { + params = new String[] {userDn, userId}; + + if (log.isDebugEnabled()) { + log.debug( + new StringBuilder("Searching for groups using ") + .append("\ngroupSearchBase: ") + .append(configProps.getGroupSearchBase()) + .append("\ngroupSearchFilter: ") + .append(configProps.getGroupSearchFilter()) + .append("\nparams: ") + .append(StringUtils.join(params, " :: ")) + .append("\ngroupRoleAttributes: ") + .append(configProps.getGroupRoleAttributes()) + .toString()); + } - if (log.isDebugEnabled()) { - log.debug( - new StringBuilder("Searching for groups using ") - .append("\ngroupSearchBase: ") - .append(configProps.getGroupSearchBase()) - .append("\ngroupSearchFilter: ") - .append(configProps.getGroupSearchFilter()) - .append("\nparams: ") - .append(StringUtils.join(params, " :: ")) - .append("\ngroupRoleAttributes: ") - .append(configProps.getGroupRoleAttributes()) - .toString()); + // Copied from org.springframework.security.ldap.userdetails.DefaultLdapAuthoritiesPopulator. + userRolesPerDn = + ldapTemplate.searchForSingleAttributeValues( + configProps.getGroupSearchBase(), + configProps.getGroupSearchFilter(), + params, + configProps.getGroupRoleAttributes()); + userRoles.addAll(userRolesPerDn); } - - // Copied from org.springframework.security.ldap.userdetails.DefaultLdapAuthoritiesPopulator. - Set userRoles = - ldapTemplate.searchForSingleAttributeValues( - configProps.getGroupSearchBase(), - configProps.getGroupSearchFilter(), - params, - configProps.getGroupRoleAttributes()); - log.debug("Got roles for user " + userId + ": " + userRoles); return userRoles.stream() .map(role -> new Role(role).setSource(Role.Source.LDAP)) .collect(Collectors.toList()); } + private boolean isSingleDnToUserId(String userId) { + return getPartialUserDn(userId) != null; + } + /** Mapper for mapping user ids to their groups */ class UserGroupMapper implements AttributesMapper>> { @@ -248,8 +267,27 @@ String getUserFullDn(String userId) { DistinguishedName root = new DistinguishedName(rootDn); log.debug("Root DN: " + root.toString()); - String[] formatArgs = new String[] {LdapEncoder.nameEncode(userId)}; + String partialUserDn = getPartialUserDn(userId); + if (partialUserDn == null) { + return null; + } + + DistinguishedName user = new DistinguishedName(partialUserDn); + log.debug("User portion: " + user.toString()); + + try { + Name fullUser = root.addAll(user); + log.debug("Full user DN: " + fullUser.toString()); + return fullUser.toString(); + } catch (InvalidNameException ine) { + log.error("Could not assemble full userDn", ine); + } + return null; + } + @VisibleForTesting + String getPartialUserDn(String userId) { + String[] formatArgs = new String[] {LdapEncoder.nameEncode(userId)}; String partialUserDn; if (!StringUtils.isEmpty(configProps.getUserSearchFilter())) { try { @@ -258,24 +296,13 @@ String getUserFullDn(String userId) { configProps.getUserSearchBase(), configProps.getUserSearchFilter(), formatArgs); partialUserDn = res.getDn().toString(); } catch (IncorrectResultSizeDataAccessException e) { - log.error("Unable to find a single user entry for {}", userId, e); + log.warn("Unable to find a single user entry for {}", userId, e); return null; } } else { partialUserDn = configProps.getUserDnPattern().format(formatArgs); } - - DistinguishedName user = new DistinguishedName(partialUserDn); - log.debug("User portion: " + user.toString()); - - try { - Name fullUser = root.addAll(user); - log.debug("Full user DN: " + fullUser.toString()); - return fullUser.toString(); - } catch (InvalidNameException ine) { - log.error("Could not assemble full userDn", ine); - } - return null; + return partialUserDn; } /** diff --git a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy index 8acdb4c38..76c68eed8 100644 --- a/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy +++ b/fiat-ldap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy @@ -45,12 +45,15 @@ class LdapUserRolesProviderTest extends Specification { 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * loadRoles(user) 1 * loadRolesForUser(user) - (0..1) * getUserFullDn(id) + (0..2) * getPartialUserDn(id) >> "notEmpty" + (0..1) * getUserDNs([id]) + (0..1) * getUserFullDn(id) >> null 0 * _ } provider.configProps = configProps provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) { (0..1) * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } + (0..1) * search(*_) 0 * _ } @@ -81,7 +84,9 @@ class LdapUserRolesProviderTest extends Specification { 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * loadRoles(user) 1 * loadRolesForUser(user) - (0..1) * getUserFullDn(id) + (0..2) * getPartialUserDn(id) >> "notEmpty" + (0..1) * getUserDNs([id]) + (0..1) * getUserFullDn(id) >> null 0 * _ } @@ -207,6 +212,7 @@ class LdapUserRolesProviderTest extends Specification { 1 * provider.doMultiLoadRoles(_) 1 * provider.getUserDNs(_ as Collection) 1 * provider.loadRolesForUsers(users) + 2 * provider.getPartialUserDn(_ as String) when: "thresholdToUseGroupMembership is breached and userSearchFilter is set" // Test to make sure that when the thresholdToUseGroupMembership is breached: @@ -330,10 +336,13 @@ class LdapUserRolesProviderTest extends Specification { 1 * provider.loadRolesForUsers(users) } - void "loadRolesForUser returns no roles when multiple DNs exist for a user id"(){ + @Unroll + void "loadRolesForUser should merge roles when multiple DNs exist for a user id"() { given: def id = 'user1' def user = externalUser(id) + def role1 = new Role("group1").setSource(Role.Source.LDAP) + def role2 = new Role("group2").setSource(Role.Source.LDAP) def configProps = baseConfigProps() @@ -341,11 +350,15 @@ class LdapUserRolesProviderTest extends Specification { 1 * setConfigProps(configProps) 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * loadRolesForUser(user) - 1 * getUserFullDn(id) + 1 * getPartialUserDn(id) + 1 * getUserDNs(_ as Collection) >> [ 'uid=dn1,ou=users,dc=springframework,dc=org' : 'user1', + 'uid=dn2,ou=users,dc=springframework,dc=org' : 'user1'] 0 * _ } provider.ldapTemplate = Mock(SpringSecurityLdapTemplate) { - 1 * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } //due to multiple DNs + 1 * searchForSingleEntry(*_) >> { throw new IncorrectResultSizeDataAccessException(1) } // due to multiple DNs + 1 * searchForSingleAttributeValues(_, _, ['uid=dn1,ou=users,dc=springframework,dc=org','user1'], _) >> ['group1'] + 1 * searchForSingleAttributeValues(_, _, ['uid=dn2,ou=users,dc=springframework,dc=org','user1'], _) >> ['group2'] 0 * _ } provider.setConfigProps(configProps) @@ -356,7 +369,7 @@ class LdapUserRolesProviderTest extends Specification { def roles = provider.loadRolesForUser(user) then: - roles == [] + roles.sort() == [role1, role2].sort() } private static ExternalUser externalUser(String id) {