Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AnonymousConfigurer.authorities only accepts GrantedAuthorities but no subtypes of GrantedAuthorities #14435

Open
tobias-lippert opened this issue Jan 10, 2024 · 3 comments
Assignees
Labels
in: config An issue in spring-security-config status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement

Comments

@tobias-lippert
Copy link

tobias-lippert commented Jan 10, 2024

Expected Behavior

I expected the builder method of AnonymousConfigurer.authorities(authorities) to take a List<? extends GrantedAuthority> authorities as parameter

Current Behavior

AnonymousConfigurer.authorities(authorities) takes a List<GrantedAuthority> as parameter

Context
As of now, I cannot think of a reason to not allow subtypes of GrantedAuthorities.
E.g., when creating a new User, one can specify a Collection<? extends GrantedAuthority>

How has this issue affected you?
We converted a list of Strings to GrantedAuthorities but since this is an interface type, we created objects of type SimpleGrantedAuthority. Using Stream.toList() to convert this stream of objects into an unmodifiable list results in a List<SimpleGrantedAuthority>.

What are you trying to accomplish?
Directly using a List<SimpleGrantedAuthority> to avoid any casting.

What other alternatives have you considered?
Cast the list elements, so the list elements have the correct type.

Are you aware of any workarounds?
See above

I'm happy to contribute a PR if this is accepted

@tobias-lippert tobias-lippert added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Jan 10, 2024
@sjohnr sjohnr self-assigned this Jan 11, 2024
@sjohnr
Copy link
Member

sjohnr commented Jan 11, 2024

@tobias-lippert thanks for reaching out!

It looks like this has been an issue for some time. However, the use of Stream#toList may be the real culprit here because it is only aware of the types in the Stream, and not the intended target of the expression (whether an assignment or passing to another method, in this case authorities(...)). In fact, the default method implementation actually does an unchecked cast.

It appears that Stream#toList method has this limitation in general, such that the following will not compile:

List<GrantedAuthority> authorities = List.of("a", "b", "c").stream()
    .map(SimpleGrantedAuthority::new)
    .toList();

I'd argue that this should compile, but it does not. However, this does compile:

List<GrantedAuthority> authorities = List.of("a", "b", "c").stream()
    .map(SimpleGrantedAuthority::new)
    .collect(Collectors.toList());

As a workaround, I recommend the use of Stream#collect as in the following example:

List<String> authorities = List.of("a", "b", "c");
http.anonymous((anonymous) -> anonymous
    .authorities(authorities.stream()
        .map(SimpleGrantedAuthority::new)
        .collect(Collectors.toList())
    )
);

Alternatively, you can cast the entire stream like this:

List<String> authorities = List.of("a", "b", "c");
http.anonymous((anonymous) -> anonymous
    .authorities(authorities.stream()
        .map(SimpleGrantedAuthority::new)
        .map(GrantedAuthority.class::cast)
        .toList()
    )
);

In terms of a fix, we will need to check that changing the method signature does not break backwards compability. My initial instict right now is that such a change would be a breaking change, and as such would need to wait for the next major release. If you have any thoughts, please let me know.

@sjohnr sjohnr added the in: config An issue in spring-security-config label Jan 11, 2024
@tobias-lippert
Copy link
Author

Thanks, @sjohnr, for your quick reply. I agree with you.
I had done the same analysis as you before I opened the issue, and we currently use your second alternative in our code.
Your first proposed alternative is also viable, but be aware that Stream#collect(Collectors.toList()) returns a modifiable list in contrast to Stream#toList, which might not be desirable in this context.
However, I thought it might make sense to change the behavior of Spring Security to accommodate this (in my opinion, questionable) implementation of the JDK. I saw it's implemented like the change I've proposed in other places of Spring Security.

I didn't know that breaking changes require a major release. I thought a minor release was sufficient, but since it's not critical but rather a nice-to-have improvement, I'm fine with waiting till a major release.

Once you conclude if it's breaking or not and which release should be targeted, I'm happy to contribute a PR, so please keep me updated.

@sjohnr
Copy link
Member

sjohnr commented Jan 12, 2024

Thanks @tobias-lippert.

Your first proposed alternative is also viable, but be aware that Stream#collect(Collectors.toList()) returns a modifiable list in contrast to Stream#toList, which might not be desirable in this context.

That is a good point. However, the authorities list is made unmodifiable when passed to the AnonymousAuthenticationToken (extends AbstractAuthenticationToken), so in terms of ensuring that the Authentication is immutable, it is fine. It is a bit strange that the AnonymousAuthenticationFilter does not make it immutable though, as it should really be the job of the recipient of the List to make it immutable.

Anyway, I digress. 😃 Thanks for the reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants