Use more specific GrantedAuthority for mapped roles

[heruan]

As discussed in #5929 this adds an implementation of LdapAuthoritiesPopulator which uses LdapAuthority instead or the generic SimpleGrantedAuthority.

The new implementation shares the same code as the original so that the old one can be deprecated and then removed.

Keep in mind that NestedLdapAuthoritiesProvider still extends DefaultLdapAuthoritiesProvider.

Alternative

Change DefaultLdapAuthoritiesPopulator implementation to use LdapAuthority instead of SimpleGrantedAuthority: this would be a "breaking" change if and only if an existing logic is based on the fact that authorities from the populator are instances of SimpleGrantedAuthority and I can't figure out any case where this would make sense. Of course this will wait a next major, but would not introduce an additional implementation that might create confusion and introduce duplicated code.

Fixes #5929

[rwinch]

Thanks for the PR!

I'm sorry but after looking at this PR I think I have steered you in the wrong direction. There is a lot of code duplication and it doesn't solve the problem for NestedLdapAuthoritiesProvider which leads me to think we need a better solution.

I'm wondering if the existing DefaultLdapAuthoritiesProvider could have a ContextMapper (or something similar) injected into it that determines the type that is extracted out. I haven't been able to look into this extensively, but I feel like that might be the best (and most flexible) approach.

Thoughts?

[heruan]

Yeah, I agree we need a better solution. One might be introducing a mapper as you said in DefaultLdapAuthoritiesProvider, which by default builds a SimpleGrantedAuthority (non-breaking) but can be customized to build the more specific LdapAuthority. E.g.

var populator = new DefaultLdapAuthoritiesPopulator();
populator.setAuthorityMapper(record -> {
	var roleDn = record.get(SpringSecurityLdapTemplate.DN_KEY).get(0);
	var role = record.get(this.groupRoleAttribute).get(0);
	return new LdapAuthority(role, roleDn);
});

In either case NestedLdapAuthoritiesMapper wouldn't be affected as it already returns instances of LdapAuthority.

When time comes for a breaking change then, the default mapper could be changed to return the more correct LdapAuthority.

[rwinch]

This seems more reasonable. Would you mind cleaning the PR to include tests and that there aren't unnecessary formatting changes introduced?

[heruan]

I don't think I've introduced formatting changes (I just added a couple of missing blank lines). About tests, the existing test already covers the default mapping function, or am I missing something?

[rwinch]

I don't think I've introduced formatting changes (I just added a couple of missing blank lines).

My mistake on that. There was code changes there too. Please ignore.

About tests, the existing test already covers the default mapping function, or am I missing something?

Yes they do. However, it would be good to add a test to ensure that if a custom authority mapper is set, then the behavior changes (i.e. add a test for your specific use case of returning an LdapAuthority). Also add a test for setting a null mapper to ensure an exception is thrown.

[heruan]

Test added!

[rwinch]

Thanks for the test! It appears the checkstyle is failing. Could you please run ./gradlew checkstyleMain checkstyleTest and fix the checkstyle errors?

[heruan]

Fixed! It was a couple of unused imports, not sure how they ended up in a file I didn't modify in the latest push (and the previous passed the checks)!

[heruan]

@rwinch not sure if anything is pending here from my side!

[eleftherias]

@heruan thanks for the PR!
It appears that the tests you added are failing.

The one test customAuthoritiesMappingFunctionThrowsIfNull is expecting a NullPointerException, but really it should expect an IllegalArgumentException.

The other test customAuthoritiesMappingFunction is checking that a list of Strings are instances of LdapAuthority, which will not be true. If you remove the transformation AuthorityUtils.authorityListToSet, then you should get the expected result.

Let me know if you can update the tests and we can get this PR merged.

[eleftherias]

I have left a few comments on the tests.

[heruan]

Thank you @eleftherias for the hints, I'll fix the tests: let's see if they pass!

[eleftherias]

@heruan The tests look good now, thank you. One last request, could you please squash your 2 commits into one. That will make the commit history clearer when this is merged.

[heruan]

Done! I was confident they were going to be squashed on merge 🙂

[eleftherias]

Thanks for the PR @heruan! This is now merged into master.