8000 [Security] Allow using a callable with `#[IsGranted]` by alexandre-daubois · Pull Request #59150 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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 a 8000 gree 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
Feb 18, 2025
Merged

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

merged 1 commit into from
Feb 18, 2025

Conversation

alexandre-daubois
Copy link
Member
@alexandre-daubois alexandre-daubois commented Dec 10, 2024 •
edited
Loading
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();
    }
}

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security labels Dec 10, 2024
@carsonbot carsonbot added this to the 7.3 milestone Dec 10, 2024
@carsonbot carsonbot changed the title [DX][Security] Allow using a callable with #[IsGranted] [Security] Allow using a callable with #[IsGranted] Dec 10, 2024
@alexandre-daubois
Copy link
Member Author

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

@alexandre-daubois
Copy link
Member Author

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.

@alexandre-daubois
Copy link
Member Author

Status: Needs Review

@chalasr
Copy link
Member
chalasr commented Dec 18, 2024

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

stof
stof requested changes Dec 18, 2024
View reviewed changes
@stof
Copy link
Member
stof commented Dec 18, 2024

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

@alexandre-daubois alexandre-daubois force-pushed the is-granted-callable branch 3 times, most recently from 3c1d6cd to 2dafa16 Compare December 18, 2024 13:25
nicolas-grekas
nicolas-grekas reviewed Feb 11, 2025
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Some comments to improve the DX hopefully.

@alexandre-daubois alexandre-daubois force-pushed the is-granted-callable branch 3 times, most recently from 816c445 to f3cfa81 Compare February 12, 2025 08:10
@alexandre-daubois alexandre-daubois force-pushed the is-granted-callable branch 2 times, most recently from 54712d5 to d4bad72 Compare February 12, 2025 09:03
@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit ab6c611 into symfony:7.3 Feb 18, 2025
4 of 11 checks passed
This was referenced Feb 18, 2025
Closed
Merged
chalasr added a commit that referenced this pull request Feb 21, 2025
@chalasr
feature #59805 [Security] Improve DX of recent additions (nicolas-gre…
0c7c251 
…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
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@gharlan gharlan gharlan left review comments

@stof stof stof requested changes

@smnandre smnandre smnandre left review comments

@mtarld mtarld mtarld left review comments

@chalasr chalasr chalasr left review comments

@derrabus derrabus derrabus requested changes

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
9 participants
@alexandre-daubois @chalasr @stof @nicolas-grekas @gharlan @smnandre @derrabus @mtarld @carsonbot
0