From 7dd040c86be15687e55fd34acff6c0dbe2838f17 Mon Sep 17 00:00:00 2001 From: kirangodishala Date: Fri, 19 Jul 2024 01:04:31 +0530 Subject: [PATCH] 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 | 28 +++-- 2 files changed, 88 insertions(+), 47 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 b848b4ed6..dcebc543f 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 @@ -46,12 +46,15 @@ class LdapUserRolesProviderTest extends Specification { 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) 1 * loadRoles(_ as ExternalUser) 1 * loadRolesForUser(_ as ExternalUser) - (0..1) * getUserFullDn(_ as String) + (0..2) * getPartialUserDn(_ as String) >> "notEmpty" + (0..1) * getUserDNs(_ as Collection) + (0..1) * getUserFullDn(_ as String) >> 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(_ as ExternalUser) 1 * loadRolesForUser(_ as ExternalUser) - (0..1) * getUserFullDn(_ as String) + (0..2) * getPartialUserDn(_ as String) >> "notEmpty" + (0..1) * getUserDNs(_ as Collection) + (0..1) * getUserFullDn(_ as String) >> 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: @@ -328,21 +334,28 @@ class LdapUserRolesProviderTest extends Specification { 1 * provider.loadRolesForUsers(_ as Collection) } - 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 user = externalUser("user1") + def role1 = new Role("group1").setSource(Role.Source.LDAP) + def role2 = new Role("group2").setSource(Role.Source.LDAP) def configProps = baseConfigProps() def provider = Spy(LdapUserRolesProvider) { 1 * setConfigProps(_ as LdapConfig.ConfigProps) 1 * setLdapTemplate(_ as SpringSecurityLdapTemplate) - 1 * loadRolesForUser(_ as ExternalUser) - 1 * getUserFullDn(_ as String) + 1 * loadRolesForUser(user) + 1 * getPartialUserDn(_ as String) + 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) @@ -353,9 +366,10 @@ class LdapUserRolesProviderTest extends Specification { def roles = provider.loadRolesForUser(user) then: - roles == [] + roles.sort() == [role1, role2].sort() } + private static ExternalUser externalUser(String id) { return new ExternalUser().setId(id) }