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)
Comment From: 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
Comment From: dkodippily
@sjohnr can I work on this :) ?
Comment From: sjohnr
Sure @dkodippily! Just checking, @Andreas-PPI that you're not already working on it?
Comment From: 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.
Comment From: dkodippily
Thanks @Andreas-PPI , have you done anything on this yet, if not I'll pick this.
Comment From: Andreas-PPI
Hi @dkodippily , no I've done nothing until now. I'm looking forward for your changes.
Comment From: 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.
Comment From: 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
authorityMapperwould need to be changed to returnnullin this case, and the for-loop below would skipnullreturn 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));
}
}
Comment From: 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.
Comment From: 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.
Comment From: 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.
Comment From: 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.
Comment From: Andreas-PPI
Or is there a problem I do not see with the null entry?
Comment From: 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
nullentry?
I believe this would be a breaking change as well, is that right?
Comment From: 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.
Comment From: 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 ?
Comment From: 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.
Comment From: 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?
Comment From: 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());
}
Comment From: 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.
Comment From: 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
nullto 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 :)
Comment From: dkodippily
Hi @sjohnr , @Andreas-PPI PR is raised #12258
Comment From: dkodippily
Hi @sjohnr, appreciate any feedback on this PR #12258
Comment From: 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.
Comment From: 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.