Skip to content

Commit

Permalink
fix(ldap/roles): fix the issue of multiple entries in Ldap for single…
Browse files Browse the repository at this point in the history 
… user

* update tests as per the new code changes
  • Loading branch information
@kirangodishala
kirangodishala committed Jul 30, 2024
1 parent ff31f1a commit 7dd040c
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 47 deletions.
107 changes: 67 additions & 40 deletions fiat-ldap/src/main/java/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,50 +71,69 @@ public void setConfigProps(LdapConfig.ConfigProps configProps) {
@Override
protected List<Role> loadRolesForUser(ExternalUser user) {
String userId = user.getId();

Set<String> userDns = new HashSet<>();
String[] params;
Set<String> userRolesPerDn;
Set<String> 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<String, String> 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<String> 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<List<Pair<String, Role>>> {

Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}

/**
Expand Down
28 changes: 21 additions & 7 deletions ...ap/src/test/groovy/com/netflix/spinnaker/fiat/roles/ldap/LdapUserRolesProviderTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>)
(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 * _
}

Expand Down Expand Up @@ -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<String>)
(0..1) * getUserFullDn(_ as String) >> null
0 * _
}

Expand Down Expand Up @@ -207,6 +212,7 @@ class LdapUserRolesProviderTest extends Specification {
1 * provider.doMultiLoadRoles(_)
1 * provider.getUserDNs(_ as Collection<String>)
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:
Expand Down Expand Up @@ -328,21 +334,28 @@ class LdapUserRolesProviderTest extends Specification {
1 * provider.loadRolesForUsers(_ as Collection<ExternalUser>)
}

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<String>) >> [ '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)
Expand All @@ -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)
}
Expand Down

0 comments on commit 7dd040c

Please sign in to comment.