Skip to content

[SecurityBundle] Fix using same handler for multiple authenticators #48937

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2023
Merged

[SecurityBundle] Fix using same handler for multiple authenticators #48937

merged 1 commit into from
Jan 13, 2023

Conversation

RobertMe
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #48923
License MIT
Doc PR -

Using a reference for custom success/failure handler breaks using the same service for multiple authenticators. This as the options will be overriden by any later authenticators. So use a ChildDefinition instead so every authenticator gets a unique instance.

@carsonbot carsonbot added this to the 5.4 milestone Jan 10, 2023
@RobertMe RobertMe force-pushed the security-fix-reusing-custom-handlers branch from acb1af8 to bb27af1 Compare January 10, 2023 12:37
@RobertMe
Copy link
Contributor Author

Regarding the updated tests I'm unsure. I've updated the actual (low level) unit tests to check that it's a ChildDefinition, but this obviously doesn't really validate that this issue is fixed (One might still think "why isn't this using a Reference?", change it to that and update the unit tests, after which the test would succeed again, while the issue this fixes isn't noticed). That's why I added the (very high level) functional tests as well. But there might be "something in between" as well (some "layer" of tests where the compiled container can be checked, without also making the HTTP calls etc).

chalasr
chalasr approved these changes Jan 11, 2023
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RobertMe @nicolas-grekas
[SecurityBundle] Fix using same handler for multiple authenticators
c589905 
Using a reference for custom success/failure handler breaks using the
same service for multiple authenticators. This as the options will be
overriden by any later authenticators. So use a ChildDefinition instead
so every authenticator gets a unique instance.

Fixes: symfony#48923
@nicolas-grekas nicolas-grekas force-pushed the security-fix-reusing-custom-handlers branch from bb27af1 to c589905 Compare January 13, 2023 08:31
@nicolas-grekas
Copy link
Member

Thank you @RobertMe.

@nicolas-grekas nicolas-grekas merged commit 3ca58ba into symfony:5.4 Jan 13, 2023
@RobertMe RobertMe deleted the security-fix-reusing-custom-handlers branch January 16, 2023 08:12
This was referenced Jan 24, 2023
Merged
Merged
Merged
Closed
@astronom astronom mentioned this pull request Feb 20, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr chalasr approved these changes

@wouterj wouterj Awaiting requested review from wouterj

Assignees
No one assigned
Labels
Bug SecurityBundle Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@RobertMe @nicolas-grekas @chalasr @carsonbot