DefaultLdapAuthoritiesPopulator throws NullPointerException

[Andreas-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 63607ee213c74d84ef52167ac08a2f67eb0b2f89 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.

https://github.com/spring-projects/spring-security/blob/f9a2d22b405905e635351ea78579831d7b321869/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java#L227-L229

[dkodippily]

@sjohnr can I work on this :) ?

[sjohnr]

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

[Andreas-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.

[Andreas-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.

https://github.com/spring-projects/spring-security/blob/f9a2d22b405905e635351ea78579831d7b321869/ldap/src/main/java/org/springframework/security/ldap/userdetails/DefaultLdapAuthoritiesPopulator.java#L227-L229

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)); } }

[Andreas-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.

[dkodippily]

Hi @sjohnr I Agree. , @Andreas-PPI

Allowing null returns from the authorityMapper is a much safer way of doing it, for the two scenarios the group-role-attribute is defined without a value assignment or completely omitted i can check and return null from the authorityMapper as below.

However through the calling method Set<GrantedAuthority> getGroupMembershipRoles the returning Set will allow a single null value, and I can remove it as shown below.

[Andreas-PPI]

Hi @sjohnr , I agree with your points. Whether that meant a breaking change, I wasn't sure. But I can understand your argument.

Hi @dkodippily , I agree with your first change, so that authorityMapper returns null for the two scenarios "undefined or empty group-role-attribute". I would omit the second change. This way the caller can decide for himself what to do with a GrantedAuthority = null. In case of doubt it can be removed there itself.

[Andreas-PPI]

Or is there a problem I do not see with the null entry?

[sjohnr]

Hi @dkodippily

i can check and return null from the authorityMapper as below.

If you wouldn't mind, please share code samples instead of screenshots. I can add notes on a PR but I'd recommend calling record.get only once and using CollectionUtils and StringUtils to check the values respectively. Something like:

this.authorityMapper = (record) -> {
    List<String> roles = record.get(this.groupRoleAttribute);
    if (CollectionUtils.isEmpty(roles)) {
        return null;
    }
    String role = roles.get(0);
    if (!StringUtils.hasLength(role)) {
        return null;
    }
    if (this.convertToUpperCase) {
        role = role.toUpperCase();
    }
    return new SimpleGrantedAuthority(this.rolePrefix + role);
};

I can remove it as shown below.

I think it would be better not to allow a null to be added at all.

Hi @Andreas-PPI

Or is there a problem I do not see with the null entry?

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

[Andreas-PPI]

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.

[dkodippily]

Hi @sjohnr Thanks for the feedback , @Andreas-PPI , yes a null would not make any sense, also it will again throw a NPE when mapping the authority list to a Set through AuthorityUtils.authorityListToSet , this method only asserts if the input list is null or not , not the elements. As @sjohnr suggested better not to allow null at all.

for (Map<String, List<String>> role : userRoles) {
            GrantedAuthority authority = this.authorityMapper.apply(role);
            Assert.notNull(authority, "authority cannot be null");
            authorities.add(authority);
        }

@sjohnr for testing , 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 ?

[Andreas-PPI]

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.

[sjohnr]

Hi @dkodippily. Sorry if I was unclear. When I said

I think it would be better not to allow a null to be added at all.

I meant add a null check and do not add the null to the collection (e.g. skip the element). At this point, I think I'd prefer if you submitted a PR and we can review the code more easily there. What do you think?

[dkodippily]

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());
        }

[sjohnr]

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.

[dkodippily]

Hi @dkodippily. Sorry if I was unclear. When I said

I think it would be better not to allow a null to be added at all.

I meant add a null check and do not add the null to the collection (e.g. skip the element). At this point, I think I'd prefer if you submitted a PR and we can review the code more easily there. What do you think?

Hi @sjohnr , Yes its high time now :)

[dkodippily]

Hi @sjohnr , @Andreas-PPI PR is raised #12258

[dkodippily]

Hi @sjohnr, appreciate any feedback on this PR #12258

[sjohnr]

@dkodippily thanks for the PR! I will look into it at the first opportunity. Just a heads up, I'm currently working hard on content for the SpringOne virtual conference to be recorded soon, so reviews and responses may be delayed.

[dkodippily]

@dkodippily thanks for the PR! I will look into it at the first opportunity. Just a heads up, I'm currently working hard on content for the SpringOne virtual conference to be recorded soon, so reviews and responses may be delayed.

Thanks and good luck with the conference.