spring-security: DefaultLdapAuthoritiesPopulator throws NullPointerException

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)

About this issue

Original URL
State: closed
Created 2 years ago
Comments: 25 (18 by maintainers)

Commits related to this issue

Fix DefaultLdapAuthoritiesPopulator NPE When referring an undefined or empty GroupRoleAttribute in a ldap entry to perform group search and authority mapping, this result in a NullPointerException. C... — committed to dkodippily/spring-security by dkodippily 2 years ago
Fix DefaultLdapAuthoritiesPopulator NPE Closes gh-12090 — committed to spring-projects/spring-security by dkodippily 2 years ago

Most upvoted comments

Hi @Andreas-PPI , yes we don’t need to call Assert.notNull(), this is how I tested the code, simply can check for null before calling authorities.add(authority); inside getGroupMembershipRoles().

True, since we agreed getGroupMembershipRoles() should not contain null there will be no NPEs, but I was answering your previous question, If we were to let that single null to go through AuthorityUtils.authorityListToSet() will break with a NPE due to below call, it is extracting the String role value from each GrantedAuthority. I’ll raise a PR, lets keep improving.

for (GrantedAuthority authority : userAuthorities) {
			set.add(authority.getAuthority());
		}

dkodippily on Nov 17, 2022

Also @dkodippily:

is it better to add a separate ldif file with “undefined or empty group-role-attribute” and load it in a new test class rather adding new entries to the test-server.ldif ?

Yes, feel free to add a new ldif file for this case. I think that would be the preference unless an existing test needs to be changed for some reason. I have not reviewed it to determine whether it does or not.

sjohnr on Nov 17, 2022

Hi @dkodippily , I hava a question to your code sample. authorityMapper can return null, see code sample from @sjohnr . In this case authority is null. Then Assert.notNull() will throw an IlegalArgumentException. This is not the desired behavior. Do you agree?

Why throws AuthorityUtils.authorityListToSet() a NPE? This method return a HashSet, which implementation allows null entries - more exactly: one null entry.

Regardless, we agreed that getGroupMembershipRoles() should not contain null entries in its returned Set.

Andreas-PPI on Nov 17, 2022

Hi @sjohnr ,

I believe this would be a breaking change as well, is that right?

Compared to throwing an NPE, both behaviors are breaking changes. 😉 I thought it is better to have a GrantedAuthority entry for each entry in the LDAP as well. I had overlooked that these are in a Set, same roles are mapped just like null only as 1 entry, so that the number can or will deviate anyway. In this case it is probably better not to supply the entry null, because probably nothing useful can be done with it anyway.

Andreas-PPI on Nov 16, 2022

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

Andreas-PPI on Nov 11, 2022

@sjohnr can I work on this 😃 ?

dkodippily on Nov 6, 2022