Skip to content

AuthorityAuthorizationManager incorrectly compares GrantedAuthority #10566

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

Closed
legas1 opened this issue Nov 30, 2021 · 4 comments · Fixed by #10588
Closed

AuthorityAuthorizationManager incorrectly compares GrantedAuthority #10566

legas1 opened this issue Nov 30, 2021 · 4 comments · Fixed by #10588
Assignees
Labels
in: core An issue in spring-security-core status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@legas1
Copy link

legas1 commented Nov 30, 2021

Describe the bug
Hi, in commit 86c24da there was a slight change in comparison of allowed authorities to endpoints. However in our use case this was huge breaking change.

Lines:
86c24da#diff-8c62a1a24d0860e1da929cdb7cdbf50a8e7daa565fd03f05673299917891f33bR40
86c24da#diff-eb4576063aa24fd635f700152f04a2590973ce56a20899414957e53a2584da74R38

In our Kotlin environment, we implement enum which implements GrantedAuthority and custom Authentication class, when this enum reaches ReactiveAuthorizationManager check method, its compared to SimpleGrantedAuthority (class implementing GrantedAuthority, breaking change from changes above) instead of underlying string authority, which results in non equality and thus denying access to endpoint.

To Reproduce

  1. Have custom implementation of AuthenticationManager which have custom Authentication implementation and custom GrantedAuthority implementation.
  2. Attempt to use protected endpoint with some authority.
  3. Access denied.

Expected behavior
Allow access to given endpoint.

Sample
https://github.com/legas1/ss.grantedauthority.demo

Does it make sense or do you need more clarification? Or I am completely wrong about my understanding of upper changes? The solution in my opinion would be to revert those changes or maybe adjust equal method of SimpleGrantedAuthority to count with GrantedAuthority interface.

Cheers, Daniel

@legas1 legas1 added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Nov 30, 2021
@legas1 legas1 changed the title GrantedAuthority interface should be compared by its implementing value GrantedAuthority interface should be compared by its implemented value Nov 30, 2021
@legas1
Copy link
Author

legas1 commented Dec 1, 2021

Added example.

@terminux
Copy link
Contributor

terminux commented Dec 6, 2021

When using a custom GrantedAuthority, there will indeed be bugs that fail to match, i submitted a PR to fix it. #10588

@marcusdacoregio marcusdacoregio added in: core An issue in spring-security-core and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 7, 2021
@marcusdacoregio marcusdacoregio modified the milestones: 5.6.1, 5.7.0-M1 Dec 7, 2021
@marcusdacoregio marcusdacoregio changed the title GrantedAuthority interface should be compared by its implemented value AuthorityAuthorizationManager incorrectly compares GrantedAuthority Dec 7, 2021
terminux added a commit to terminux/spring-security that referenced this issue Dec 8, 2021
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label Dec 8, 2021
@marcusdacoregio
Copy link
Contributor

Fixed via #10588

@marcusdacoregio
Copy link
Contributor

marcusdacoregio commented Dec 8, 2021

Thanks folks. This is now merged into 5.7.x and backported to 5.6.x. The next version 5.6.1 is gonna have the fix. Can you try the 5.6.1-SNAPSHOT to validate the fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants