From eec6729125387c8d1cd2dcc7b23ec357c6341e55 Mon Sep 17 00:00:00 2001 From: Dayan Kodippily Date: Sun, 20 Nov 2022 03:00:17 +1300 Subject: [PATCH] Fix DefaultLdapAuthoritiesPopulator NPE Closes gh-12090 --- ...esPopulatorGetGrantedAuthoritiesTests.java | 105 ++++++++++++++++++ ...-with-undefined-group-role-attributes.ldif | 41 +++++++ .../DefaultLdapAuthoritiesPopulator.java | 15 ++- 3 files changed, 159 insertions(+), 2 deletions(-) create mode 100644 ldap/src/integration-test/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests.java create mode 100644 ldap/src/integration-test/resources/test-server-with-undefined-group-role-attributes.ldif diff --git a/ldap/src/integration-test/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests.java b/ldap/src/integration-test/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests.java new file mode 100644 index 0000000000..9321ef16c6 --- /dev/null +++ b/ldap/src/integration-test/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests.java @@ -0,0 +1,105 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.ldap.userdetails; + +import java.util.Set; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; + +import org.springframework.beans.factory.DisposableBean; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.ldap.core.ContextSource; +import org.springframework.ldap.core.DirContextAdapter; +import org.springframework.ldap.core.DistinguishedName; +import org.springframework.security.core.authority.AuthorityUtils; +import org.springframework.security.ldap.DefaultSpringSecurityContextSource; +import org.springframework.security.ldap.server.ApacheDSContainer; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * @author Dayan Kodippily + */ +@ExtendWith(SpringExtension.class) +@ContextConfiguration( + classes = DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests.ApacheDsContainerWithUndefinedGroupRoleAttributeConfig.class) +public class DefaultLdapAuthoritiesPopulatorGetGrantedAuthoritiesTests { + + @Autowired + private DefaultSpringSecurityContextSource contextSource; + + private DefaultLdapAuthoritiesPopulator populator; + + @BeforeEach + public void setUp() { + this.populator = new DefaultLdapAuthoritiesPopulator(this.contextSource, "ou=groups"); + this.populator.setIgnorePartialResultException(false); + } + + @Test + public void groupSearchDoesNotAllowNullRoles() { + this.populator.setRolePrefix("ROLE_"); + this.populator.setGroupRoleAttribute("ou"); + this.populator.setSearchSubtree(true); + this.populator.setSearchSubtree(false); + this.populator.setConvertToUpperCase(true); + this.populator.setGroupSearchFilter("(member={0})"); + + DirContextAdapter ctx = new DirContextAdapter( + new DistinguishedName("uid=dayan,ou=people,dc=springframework,dc=org")); + + Set authorities = AuthorityUtils.authorityListToSet(this.populator.getGrantedAuthorities(ctx, "dayan")); + + assertThat(authorities).as("Should have 1 role").hasSize(2); + + assertThat(authorities.contains("ROLE_DEVELOPER")).isTrue(); + assertThat(authorities.contains("ROLE_")).isTrue(); + } + + @Configuration + static class ApacheDsContainerWithUndefinedGroupRoleAttributeConfig implements DisposableBean { + + private ApacheDSContainer container; + + @Bean + ApacheDSContainer ldapContainer() throws Exception { + this.container = new ApacheDSContainer("dc=springframework,dc=org", + "classpath:test-server-with-undefined-group-role-attributes.ldif"); + this.container.setPort(0); + return this.container; + } + + @Bean + ContextSource contextSource(ApacheDSContainer ldapContainer) { + return new DefaultSpringSecurityContextSource( + "ldap://127.0.0.1:" + ldapContainer.getLocalPort() + "/dc=springframework,dc=org"); + } + + @Override + public void destroy() { + this.container.stop(); + } + + } + +} diff --git a/ldap/src/integration-test/resources/test-server-with-undefined-group-role-attributes.ldif b/ldap/src/integration-test/resources/test-server-with-undefined-group-role-attributes.ldif new file mode 100644 index 0000000000..cd9c904332 --- /dev/null +++ b/ldap/src/integration-test/resources/test-server-with-undefined-group-role-attributes.ldif @@ -0,0 +1,41 @@ +dn: ou=groups,dc=springframework,dc=org +objectclass: top +objectclass: organizationalUnit +ou: groups + +dn: ou=people,dc=springframework,dc=org +objectclass: top +objectclass: organizationalUnit +ou: people + +dn: uid=dayan,ou=people,dc=springframework,dc=org +objectclass: top +objectclass: person +objectclass: organizationalPerson +objectclass: inetOrgPerson +cn: Dayan K +sn: Dayan +uid: dayan +userPassword: dayanspassword + + + +dn: cn=managers,ou=groups,dc=springframework,dc=org +objectclass: top +objectclass: groupOfNames +cn: managers +ou: +member: uid=dayan,ou=people,dc=springframework,dc=org + +dn: cn=researchers,ou=groups,dc=springframework,dc=org +objectclass: top +objectclass: groupOfNames +cn: researchers +member: uid=dayan,ou=people,dc=springframework,dc=org + +dn: cn=developers,ou=groups,dc=springframework,dc=org +objectclass: top +objectclass: groupOfNames +cn: developers +ou: developer +member: uid=dayan,ou=people,dc=springframework,dc=org diff --git a/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java b/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java index f16ba86b28..b69985d2b4 100644 --- a/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java +++ b/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java @@ -37,6 +37,7 @@ import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.ldap.SpringSecurityLdapTemplate; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; /** * The default strategy for obtaining user role information from the directory. @@ -169,7 +170,14 @@ public class DefaultLdapAuthoritiesPopulator implements LdapAuthoritiesPopulator logger.info("Will perform group search from the context source base since groupSearchBase is empty."); } this.authorityMapper = (record) -> { - String role = record.get(this.groupRoleAttribute).get(0); + List roles = record.get(this.groupRoleAttribute); + if (CollectionUtils.isEmpty(roles)) { + return null; + } + String role = roles.get(0); + if (role == null) { + return null; + } if (this.convertToUpperCase) { role = role.toUpperCase(); } @@ -225,7 +233,10 @@ public class DefaultLdapAuthoritiesPopulator implements LdapAuthoritiesPopulator new String[] { this.groupRoleAttribute }); logger.debug(LogMessage.of(() -> "Found roles from search " + userRoles)); for (Map> role : userRoles) { - authorities.add(this.authorityMapper.apply(role)); + GrantedAuthority authority = this.authorityMapper.apply(role); + if (authority != null) { + authorities.add(authority); + } } return authorities; }