Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

alexandre-daubois
Copy link
Member

@alexandre-dauboisalexandre-daubois commentedDec 10, 2024•
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

Thanks tothe 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:

useSymfony\Component\HttpFoundation\JsonResponse;useSymfony\Component\HttpKernel\Attribute\AsController;useSymfony\Component\Security\Core\Authorization\IsGrantedPayload;useSymfony\Component\Security\Http\Attribute\IsGranted;#[AsController]class BlogPostViewController{    #[IsGranted(staticfunction ($token,$subject,$accessDecisionManager,$trustResolver):bool {if ($subject->isPublished()) {// published, everybody can see itreturnfalse;        }if ($subject->getAuthor() ===$token->getUser()            ||$accessDecisionManager->decide($token,'edit',$subject)        ) {// the author can see itreturntrue;        }// or any admin of the appreturn$accessDecisionManager->decide($token, ['ROLE_ADMIN']);    }, subject:staticfunction (array$arguments,Request$request) {return$arguments['post'];    })]publicfunction__invoke(BlogPost$post):JsonResponse    {returnnewJsonResponse();    }}

@carsonbotcarsonbot added Status: Needs Review DXDX = Developer eXperience (anything that improves the experience of using Symfony) Feature Security labelsDec 10, 2024
@carsonbotcarsonbot added this to the7.3 milestoneDec 10, 2024
@carsonbotcarsonbot 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
MemberAuthor

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

@alexandre-daubois
Copy link
MemberAuthor

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 usinginstance Closure instead ofis_callable() to avoid callable strings that may be used.

smnandre reacted with thumbs up emoji

@alexandre-daubois
Copy link
MemberAuthor

Status: Needs Review

@chalasr
Copy link
Member

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

mtarld reacted with thumbs up emoji

@stof
Copy link
Member

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

Copy link
Member

@nicolas-grekasnicolas-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-dauboisalexandre-dauboisforce-pushed theis-granted-callable branch 3 times, most recently from816c445 tof3cfa81CompareFebruary 12, 2025 08:10
@alexandre-dauboisalexandre-dauboisforce-pushed theis-granted-callable branch 2 times, most recently from54712d5 tod4bad72CompareFebruary 12, 2025 09:03
@nicolas-grekas
Copy link
Member

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

alexandre-daubois reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Thank you@alexandre-daubois.

chalasr reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commitab6c611 intosymfony:7.3Feb 18, 2025
4 of 11 checks passed
chalasr added a commit that referenced this pull requestFeb 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       | MITThis 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
@fabpotfabpot mentioned this pull requestMay 2, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@gharlangharlangharlan left review comments

@stofstofstof requested changes

@smnandresmnandresmnandre left review comments

@mtarldmtarldmtarld left review comments

@chalasrchalasrchalasr left review comments

@derrabusderrabusderrabus requested changes

Assignees
No one assigned
Labels
DXDX = Developer eXperience (anything that improves the experience of using Symfony)FeatureSecurityStatus: 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
[8]ページ先頭
©2009-2025 Movatter.jp