DefaultLdapAuthoritiesPopulator throws NullPointerException

[ans-PPI]

Describe the bug
I want to evaluate the LDAP entries of a user to derive the role of the user. The attribute I have chosen is 'description' of the groups the user is a member of. The corresponding configuration in the security.xml is as follows:

<ldap-authentication-provider server-ref=...
  group-search-filter="(member={0})" group-role-attribute="description" role-prefix="none"
  user-context-mapper-ref="instantUserContextMapper" />

If 'description' is not assigned for one of the entries, a NullPointerException occurs in the class DefaultLdapAuthoritiesPopulator.
I think this happens in line 172:
String role = record.get(this.groupRoleAttribute).get(0);
In this case record.get(this.groupRoleAttribute) returns null.

I'm using spring-security-ldap:5.7.3 - the newest version which works with Java 8.
Previously I used V3.1.1, no NullPointerException was thrown there. Instead such entries were skipped, which I think is the better behavior.

To Reproduce
Add a group without description to your LDAP-groups or remove description from existing group.

Expected behavior
Unspecified attribute values should be treated as empty strings.
Alternatively, the affected entries can be omitted so that DefaultLdapAuthoritiesPopulator.getGrantedAuthorities()
returns fewer entries than are present in the LDAP.

Stack trace

java.lang.NullPointerException
  at org.springframework.security.ldap.userdetails.DefaultLdapAuthoritiesPopulator.lambda$new$0(DefaultLdapAuthoritiesPopulator.java:172)
  at org.springframework.security.ldap.userdetails.DefaultLdapAuthoritiesPopulator.getGroupMembershipRoles(DefaultLdapAuthoritiesPopulator.java:228)
  at org.springframework.security.ldap.userdetails.DefaultLdapAuthoritiesPopulator.getGrantedAuthorities(DefaultLdapAuthoritiesPopulator.java:202)
  at org.springframework.security.ldap.authentication.LdapAuthenticationProvider.loadUserAuthorities(LdapAuthenticationProvider.java:197)
  at org.springframework.security.ldap.authentication.AbstractLdapAuthenticationProvider.authenticate(AbstractLdapAuthenticationProvider.java:81)
  at org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:182)
  at org.springframework.security.authentication.ProviderManager.authenticate(ProviderManager.java:201)
  at org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter.attemptAuthentication(UsernamePasswordAuthenticationFilter.java:85)
  at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:227)
  at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:217)
  at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
  at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:103)
  at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:89)
  at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
  at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:55)
  at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:117)
  at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
  at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:112)
  at org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:82)
  at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:346)
  at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:221)
  at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:186)
  at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:354)
  at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:267)
  at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
  at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
  at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:202)
  at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96)
  at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:541)
  at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:139)
  at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92)
  at org.apache.catalina.valves.AbstractAccessLogValve.invoke(AbstractAccessLogValve.java:690)
  at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:74)
  at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:343)
  at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:373)
  at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
  at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:868)
  at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1590)
  at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
  at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
  at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
  at java.lang.Thread.run(Thread.java:748)

[sjohnr]

Thanks for reporting this, @Andreas-PPI!

It looks as though the change in 63607ee changed the way attribute values are searched/returned and subsequently mapped, resulting in the possible NPE. Would you be interested in submitting a PR to fix the issue?

I believe the authorityMapper would need to be changed to return null in this case, and the for-loop below would skip null return values.

spring-security/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java

Lines 227 to 229 in f9a2d22

	for (Map<String, List<String>> role : userRoles) {
	authorities.add(this.authorityMapper.apply(role));
	}

[dkodippily]

@sjohnr can I work on this :) ?

[sjohnr]

Sure @dkodippily! Just checking, @Andreas-PPI that you're not already working on it?

[ans-PPI]

Hi @sjohnr , hi @dkodippily ,
I checked out the source code but didn't change anything. I wanted to start with it this morning, but if you'd like, you can take it over.

[dkodippily]

Thanks @Andreas-PPI , have you done anything on this yet, if not I'll pick this.

[ans-PPI]

Hi @dkodippily , no I've done nothing until now. I'm looking forward for your changes.

[dkodippily]

@Andreas-PPI @sjohnr , managed to run some code and reproduce the issue, but I'm facing some issues building the latest master, Execution failed for task ':checkstyleNohttp', companing about documents with http, trying to get my build right at the moment.

[dkodippily]

Thanks for reporting this, @Andreas-PPI!

It looks as though the change in 63607ee changed the way attribute values are searched/returned and subsequently mapped, resulting in the possible NPE. Would you be interested in submitting a PR to fix the issue?

I believe the authorityMapper would need to be changed to return null in this case, and the for-loop below would skip null return values.

spring-security/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java

Lines 227 to 229 in f9a2d22

	for (Map<String, List<String>> role : userRoles) {
	authorities.add(this.authorityMapper.apply(role));
	}

Returning null from the authorityMapper is still results in a NPE, how about filtering it from the method calling point as below
for (Map<String, List<String>> role : userRoles) { if(role.keySet().contains(this.groupRoleAttribute)) { authorities.add(this.authorityMapper.apply(role)); } }

[ans-PPI]

Hi @dkodippily ,
I think this will work. It would also have the advantage that the solution would work for other similar implementations of authorityMapper.

[sjohnr]

Hi @dkodippily, @Andreas-PPI!

Returning null from the authorityMapper is still results in a NPE

Did you see the part of my comment that says "and the for-loop below would skip null return values."? If so, I'm not sure what you're referring to that would cause an NPE?

how about filtering it from the method calling point as below

I believe this would be a breaking change, as you would be changing what gets passed into the mapper. Do you agree? If so, I believe allowing null to be returned from the mapper would be the safer change and wouldn't surprise users who have provided a custom authorityMapper.

It would also have the advantage that the solution would work for other similar implementations of authorityMapper.

See comment above. I think that would be up to the custom mapper to handle. Since we're providing a Map<String, List<String>>, the calling code probably shouldn't try to predict what the mapper will do with that collection. Do you agree? I do see your point about possibly benefiting custom mappers though, and I agree it would be nice to prevent a null-pointer in custom code if it weren't a breaking change.