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

authorities in the User between Spring Security Core version 6.1.0 and 6.1.2 #13734

Closed
CyrilStar opened this issue Aug 23, 2023 · 3 comments
Closed
Assignees
Labels
status: invalid An issue that we don't feel is valid type: bug A general bug

Comments

@CyrilStar
Copy link

my code
i defined an in-memory User object.
image
see the breakpoint result
image
image

source code differ
Spring Security Core version 6.1.0 used "addAll();" but Spring Security Core version 6.1.2 is "new ArrayList<>(authorities);"
image
image
I want to know what the official team's intention was in making these modifications.

@CyrilStar CyrilStar added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 23, 2023
@andreilisa
Copy link

andreilisa commented Aug 23, 2023

@CyrilStar, just have a look to answer from this question and i think it is the reason.

@codingtim
Copy link

It appears they first added a clear before the addAll in commit:
4def405
This was done under issues: #12533
Which is weird cause the initial fix for that issue was committed on January 30th and the commit that added the clear call was added June 13th.

Then the implementation was changed from clear + addAll to new ArrayList as a "polish" in commit:
1f04baa

Now the initial change of adding clear was done so calling authorities twice would result in replacing the authorities inside the builder instead of adding.
I can understand that.
BUT the roles method on the builder calls authorities so this results in authorities being replaced.
You aren't the first one to notice this. Someone else opened similar ticket #12958 which was closed as a duplicate of the ticket that caused the issue 🤯

Bottom line: it seems impossible to use both roles() and authorities() as they both replace all authorities.
Was this intentional? I don't think so as the tests only use authorities() and no calls to roles() is made.

Just another nice Spring Security breaking change in a patch release.

@sjohnr
Copy link
Member

sjohnr commented Aug 31, 2023

Apologies for the confusion everyone. Please see release notes for release 6.1.1 which includes gh-13290.

Here's the timeline, hopefully it clears everything up:

  • An enhancement was requested via Allow UserBuilder to easily build a user without any authorities #12533 to allow empty authorities by default (meaning you don't need to specify roles(...) or authorities(...) when building the user).
  • This was added via 3229bfa, which erroneously changed the behavior to add authorities to the list. This change partially addressed the enhancement but was not intended to change behavior.
  • This comment on the commit noted that this broke backwards compatibility because you could no longer overwrite the authorities on the builder.
  • PR UserBuilder does not allow authorities to be overridden #13290 was opened to address this bug.
  • I added polish commit 1f04baa to simplify the code so it resembled the code prior to the original change, but still included the original enhancement.

In conclusion, the UserBuilder should now function as it did before, with the added ability to have empty authorities by default and omit a call to authorities(...). So the intention of the latest fix in the patch release (6.1.1) was to restore the original behavior because it was not intended to be a breaking change.

Having said that, the behavior outlined in the OP of this issue is not intended, and since we can't have it act both ways, we will keep it the way it was prior to 6.1. I'm going to go ahead and close this issue based on that explanation.

@sjohnr sjohnr closed this as completed Aug 31, 2023
@sjohnr sjohnr added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Aug 31, 2023
@sjohnr sjohnr self-assigned this Aug 31, 2023
@sjohnr sjohnr added the type: bug A general bug label Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants