Skip to content

[Security] Store original token in token storage when implicitly exiting impersonation #58754

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

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Nov 4, 2024

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

If you impersonate user A and then start impersonation for user B, Symfony explicitly exits the first impersonation before starting the second one. However, we did not update the token in the token storage at this moment.

This creates issues when using a custom voter like the one documented, as this uses Security::isGranted(), which relies on the token in the token storage. So instead of checking if the original user can impersonate, it will check if user A can impersonate.

@fabpot
Copy link
Member

fabpot commented Nov 4, 2024

Thank you @wouterj.

@fabpot fabpot merged commit c905bb4 into symfony:5.4 Nov 4, 2024
11 of 12 checks passed
@wouterj wouterj deleted the security-implicit-impersonation branch November 4, 2024 12:51
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Nov 19, 2024
…token to vote on (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Security] use access decision manager to control which token to vote on

Following symfony/symfony#58754: calling. `Security::isGranted()` inside a voter has the drawback that we do not know if the checks performed here act on the same token that we have in our voter as the token inside the token storage might have change or may change in between.

Commits
-------

fc0030a use access decision manager to control which token to vote on
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants