[Security] Allow using a callable with #[IsGranted]
#59150
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#[IsGranted]#[IsGranted]
b493c5d to
d1cd7c3
Compare
chalasr
reviewed
Dec 10, 2024
src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
d1cd7c3 to
6eedfaa
Compare
|
The WIP fix on |
smnandre
reviewed
Dec 14, 2024
smnandre
reviewed
Dec 14, 2024
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
mtarld
reviewed
Dec 16, 2024
src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resol
8000
ved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
derrabus
requested changes
Dec 16, 2024
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
6eedfaa to
a28dcfd
Compare
|
Thank you for your feedbacks. I updated the PR. Also, I'm now using |
a28dcfd to
da421f7
Compare
mtarld
reviewed
Dec 18, 2024
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
|
Status: Needs Review |
|
I think the Attribute phpdoc and future docs should mention that only static closures are supported (for now). |
stof
requested changes
Dec 18, 2024
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
|
@chalasr supporting static closures only is not a limitation of our code. It is a limitation of PHP attributes. If PHP supports more closures (quite unlikely though, as I don't see how they would work), the code in Symfony would not require any change to support them. |
stof
reviewed
Dec 18, 2024
src/Symfony/Bundle/SecurityBundle/Resources/config/security.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
3c1d6cd to
2dafa16
Compare
2dafa16 to
6ce5080
Compare
6ce5080 to
897ac91
Compare
src/Symfony/Component/Security/Core/Authorization/Voter/ClosureVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/Voter/ClosureVoter.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php
Outdated
Show resolved
Hide resolved
816c445 to
f3cfa81
Compare
nicolas-grekas
approved these changes
Feb 12, 2025
src/Symfony/Component/Security/Core/Authorization/Voter/ClosureVoter.php
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Authorization/AuthorizationCheckerInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/Voter/ClosureVoterTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Core/Tests/Authorization/Voter/ClosureVoterTest.php
Outdated
Show resolved
Hide resolved
f3cfa81 to
30f3f4e
Compare
...y/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php
Outdated
Show resolved
Hide resolved
54712d5 to
d4bad72
Compare
|
There's one failure to check (at least on the windows build) |
d4bad72 to
a53efbf
Compare
a53efbf to
9c31038
Compare
|
Thank you @alexandre-daubois. |
This was referenced Feb 18, 2025
chalasr
added a commit
that referenced
this pull request
Feb 21, 2025
…kas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Improve DX of recent additions | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | not really | Deprecations? | no | Issues | - | License | MIT This is a follow up of #48142 and #59150 which were merged recently into 7.3. Summary of the changes: - `UserAuthorizationChecker`, the implementation of the corresponding interface is merged into the existing `AuthorizationChecker`. - `AuthorizationChecker::isGranted()` is made aware of token changed by its new `isGrantedForUser()` method, so that calls to `is_granted()` nested into `is_granted_for_user()` calls will check the provided user, not the logged in one. - Replace the many arguments passed to `IsGranted`'s closures by 1. a new `IsGrantedContext` and 2. the `$subject`. This makes everything simpler, easier to discover, and more extensible. Thanks to the previous item, IsGrantedContext only needs the auth-checker, not the access-decision-manager anymore. Simpler again. - Fix to the tests, those were broken in both PRs. Commits ------- aa38eb8 [Security] Improve DX of recent additions
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks to the latest RFC that successfully passed for the next version of PHP, closures are now allowed in attributes. Symfony could leverage this at multiple places, especially where the ExpressionLanguage is currently used. What's nice is that, compared to expressions, closures can benefit from typing, autocomplete, etc. Way better for the DX.
This PR propose to leverage this new feature in
#[IsGranted], which would allow to write such code: