[Security] Allow using a callable with `#[IsGranted]` #59150

alexandre-daubois · 2024-12-10T07:59:38Z

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

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:

use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpKernel\Attribute\AsController;
use Symfony\Component\Security\Core\Authorization\IsGrantedPayload;
use Symfony\Component\Security\Http\Attribute\IsGranted;

#[AsController]
class BlogPostViewController
{
    #[IsGranted(static function ($token, $subject, $accessDecisionManager, $trustResolver): bool {
        if ($subject->isPublished()) {
            // published, everybody can see it
            return false;
        }

        if ($subject->getAuthor() === $token->getUser()
            || $accessDecisionManager->decide($token, 'edit', $subject)
        ) {
            // the author can see it
            return true;
        }

        // or any admin of the app
        return $accessDecisionManager->decide($token, ['ROLE_ADMIN']);
    }, subject: static function (array $arguments, Request $request) {
        return $arguments['post'];
    })]
    public function __invoke(BlogPost $post): JsonResponse
    {
        return new JsonResponse();
    }
}

src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

alexandre-daubois · 2024-12-11T12:08:32Z

The WIP fix on php-src works well. I updated the code (diff), it works nicely now 🙂

src/Symfony/Component/Security/Http/Attribute/IsGranted.php

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

src/Symfony/Component/Security/Core/CHANGELOG.md

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

src/Symfony/Component/Security/Core/Authorization/IsGrantedPayload.php

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

alexandre-daubois · 2024-12-18T09:43:34Z

Thank you for your feedbacks. I updated the PR. IsGrantedPayload was removed as well as role names. Arguments are now passed individually to the closure.

Also, I'm now using instance Closure instead of is_callable() to avoid callable strings that may be used.

src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php

alexandre-daubois · 2024-12-18T10:47:02Z

Status: Needs Review

chalasr · 2024-12-18T11:00:32Z

I think the Attribute phpdoc and future docs should mention that only static closures are supported (for now).

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php

src/Symfony/Component/Security/Http/Attribute/IsGranted.php

src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php

stof · 2024-12-18T11:21:43Z

@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.

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php

src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php

nicolas-grekas

Some comments to improve the DX hopefully.

src/Symfony/Component/Security/Core/Authorization/Voter/ClosureVoter.php

src/Symfony/Component/Security/Http/Attribute/IsGranted.php

src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php

src/Symfony/Component/Security/Core/Authorization/Voter/ClosureVoter.php

src/Symfony/Component/Security/Core/Authorization/AuthorizationCheckerInterface.php

src/Symfony/Component/Security/Core/CHANGELOG.md

src/Symfony/Component/Security/Core/Tests/Authorization/Voter/ClosureVoterTest.php

src/Symfony/Component/Security/Http/Attribute/IsGranted.php

src/Symfony/Component/Security/Core/Tests/Authorization/Voter/ClosureVoterTest.php

src/Symfony/Component/Security/Http/Attribute/IsGranted.php

...y/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php

nicolas-grekas · 2025-02-12T10:30:26Z

There's one failure to check (at least on the windows build)

nicolas-grekas · 2025-02-18T09:49:22Z

Thank you @alexandre-daubois.

…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

alexandre-daubois requested a review from chalasr as a code owner December 10, 2024 07:59

carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security labels Dec 10, 2024

carsonbot added this to the 7.3 milestone Dec 10, 2024

carsonbot changed the title ~~[DX][Security] Allow using a callable with #[IsGranted]~~ [Security] Allow using a callable with #[IsGranted] Dec 10, 2024

alexandre-daubois force-pushed the is-granted-callable branch from b493c5d to d1cd7c3 Compare December 10, 2024 08:00

chalasr reviewed Dec 10, 2024

View reviewed changes

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

alexandre-daubois force-pushed the is-granted-callable branch from d1cd7c3 to 6eedfaa Compare December 11, 2024 12:04

smnandre reviewed Dec 14, 2024

View reviewed changes

src/Symfony/Component/Security/Http/Attribute/IsGranted.php Outdated Show resolved Hide resolved

smnandre reviewed Dec 14, 2024

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/CallableVoter.php Outdated Show resolved Hide resolved

mtarld reviewed Dec 16, 2024

View reviewed changes

derrabus requested changes Dec 16, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 16, 2024

alexandre-daubois force-pushed the is-granted-callable branch from 6eedfaa to a28dcfd Compare December 18, 2024 09:41

alexandre-daubois force-pushed the is-granted-callable branch from a28dcfd to da421f7 Compare December 18, 2024 09:43

mtarld reviewed Dec 18, 2024

View reviewed changes

src/Symfony/Component/Security/Http/EventListener/IsGrantedAttributeListener.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 18, 2024

stof requested changes Dec 18, 2024

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 18, 2024

stof reviewed Dec 18, 2024

View reviewed changes

alexandre-daubois force-pushed the is-granted-callable branch 3 times, most recently from 3c1d6cd to 2dafa16 Compare December 18, 2024 13:25

alexandre-daubois force-pushed the is-granted-callable branch from 2dafa16 to 6ce5080 Compare January 18, 2025 09:26

alexandre-daubois force-pushed the is-granted-callable branch from 6ce5080 to 897ac91 Compare February 10, 2025 09:36

nicolas-grekas reviewed Feb 11, 2025

View reviewed changes

alexandre-daubois force-pushed the is-granted-callable branch 3 times, most recently from 816c445 to f3cfa81 Compare February 12, 2025 08:10

nicolas-grekas approved these changes Feb 12, 2025

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Feb 12, 2025

nicolas-grekas reviewed Feb 12, 2025

View reviewed changes

src/Symfony/Component/Security/Core/Tests/Authorization/Voter/ClosureVoterTest.php Outdated Show resolved Hide resolved

alexandre-daubois force-pushed the is-granted-callable branch from f3cfa81 to 30f3f4e Compare February 12, 2025 08:27

nicolas-grekas reviewed Feb 12, 2025

View reviewed changes

src/Symfony/Component/Security/Http/Attribute/IsGranted.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Feb 12, 2025

View reviewed changes

src/Symfony/Component/Security/Http/Attribute/IsGranted.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Feb 12, 2025

View reviewed changes

...y/Component/Security/Http/Tests/Fixtures/IsGrantedAttributeMethodsWithCallableController.php Outdated Show resolved Hide resolved

alexandre-daubois force-pushed the is-granted-callable branch 2 times, most recently from 54712d5 to d4bad72 Compare February 12, 2025 09:03

alexandre-daubois force-pushed the is-granted-callable branch from d4bad72 to a53efbf Compare February 12, 2025 14:10

[Security] Allow using a callable with #[IsGranted]

9c31038

nicolas-grekas force-pushed the is-granted-callable branch from a53efbf to 9c31038 Compare February 18, 2025 09:48

nicolas-grekas merged commit ab6c611 into symfony:7.3 Feb 18, 2025
4 of 11 checks passed

This was referenced Feb 18, 2025

[Security] Allow using a callable with #[IsGranted] symfony/symfony-docs#20663

Closed

[Security] Improve DX of recent additions #59805

Merged

fabpot mentioned this pull request May 2, 2025

Release v7.3.0-BETA1 #60327

Merged

Uh oh!

[Security] Allow using a callable with #[IsGranted] #59150

[Security] Allow using a callable with #[IsGranted] #59150

Uh oh!

Conversation

alexandre-daubois commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexandre-daubois commented Dec 11, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexandre-daubois commented Dec 18, 2024

Uh oh!

Uh oh!

alexandre-daubois commented Dec 18, 2024

Uh oh!

chalasr commented Dec 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stof commented Dec 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented Feb 12, 2025

Uh oh!

nicolas-grekas commented Feb 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

[Security] Allow using a callable with `#[IsGranted]` #59150

[Security] Allow using a callable with `#[IsGranted]` #59150

alexandre-daubois commented Dec 10, 2024 •

edited

Loading