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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override the access decision strategy instead of the manager #7021

Merged
merged 7 commits into from Mar 14, 2024

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Mar 14, 2024

Contao is decorating the Symfony access decision manager, because we require to use the priority strategy in our firewall, but we don't want to alter the strategy in a possible firewall outside of Contao. Unfortunately, Symfony does not allow to define the voter strategy per firewall.

Our decoration causes several issues, mostly described in symfony/symfony#54225. A quick fix was added in #7008.

Decorating the access decision manager causes all kind of issues in the profiler toolbar. We worked around some by decorating our own access decision manager with the Symfony TraceableAccessDecisionManager in debug mode. But Symfony also decorates all voters automatically to trace the voter definition, and these traced voters are only replaced on the original access decision manager. Our profiler therefore did not show all relevant information.

Because we only need to influence the strategy, we can simply decorate that instead of the full manager. Tracing and everything then works out of the box as usual in Symfony. We can even show the correct strategy in the profiler depending on the firewall context 馃帀

@fritzmg I think that closes symfony/symfony#54225 (at least for us)
Also fixes #6988

@aschempp aschempp added the bug label Mar 14, 2024
@aschempp aschempp added this to the 5.3 milestone Mar 14, 2024
@aschempp aschempp requested review from fritzmg and a team March 14, 2024 15:01
@aschempp aschempp self-assigned this Mar 14, 2024
@aschempp aschempp linked an issue Mar 14, 2024 that may be closed by this pull request
Toflar
Toflar previously approved these changes Mar 14, 2024
fritzmg
fritzmg previously approved these changes Mar 14, 2024
Toflar
Toflar previously approved these changes Mar 14, 2024
@aschempp aschempp dismissed stale reviews from Toflar and fritzmg via 30e58fa March 14, 2024 15:15
@leofeyer leofeyer merged commit da1b9ea into contao:5.3 Mar 14, 2024
17 checks passed
@leofeyer
Copy link
Member

Thank you @aschempp.

@aschempp aschempp deleted the fix/voter-trace branch March 15, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symfony Profiler: type error in Security panel
5 participants