Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Make request always available to#[IsGranted]#48080
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
[Security] Make request always available to#[IsGranted]#48080
Uh oh!
There was an error while loading.Please reload this page.
Conversation
#[IsGranted]#[IsGranted]#[IsGranted]#[IsGranted]nicolas-grekas commentedNov 2, 2022
Thank you@HypeMC. |
nicolas-grekas commentedNov 2, 2022
@HypeMC can you please send a PR to the doc to mention this? |
stof left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the case of using the request variable in an expression is not covered by tests
| } | ||
| if (!\array_key_exists($subjectRef,$arguments)) { | ||
| if ('request' ===$subjectRef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think we should add that special case. It can be surprising as strings mean argument names. Having special string values makes things harder to explainrequest means the current request only when you don't have an argument named$request in this code).
To me, voting on the Request object is something that case be solved by using the Expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for the feedback, will create a followup PR to address this issue.
HypeMC commentedNov 3, 2022
@stof There are two tests that check if the request variable is passed to the |
stof commentedNov 3, 2022
indeed, this is probably covered by your update of the existing tests using expressions. |
…ect (HypeMC)This PR was merged into the 6.2 branch.Discussion----------[Security] Remove special case for `#[IsGranted()]` subject| Q | A| ------------- | ---| Branch? | 6.2| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets |Fix#48080 (comment)| License | MIT| Doc PR | -Addresses a comment by `@stof`Instead of having `request` as a special case, an expression can be used instead:```diff-#[IsGranted(attribute: 'SOME_ATTRIBUTE', subject: 'request')]+#[IsGranted(attribute: 'SOME_ATTRIBUTE', subject: new Expression('request'))]public function index(){}```Commits-------3e0ac4f [Security] Remove special case for #[IsGranted()] subject
… (HypeMC)This PR was merged into the 6.2 branch.Discussion----------[Security] Use expression for `#[IsGranted()]` subjectsymfony/symfony#46978symfony/symfony#48080symfony/symfony#48102Commits-------9d4045f [Security] Use expression for #[IsGranted()] subject
… (HypeMC)This PR was merged into the 6.2 branch.Discussion----------[Security] Use expression for `#[IsGranted()]` subjectsymfony/symfony#46978symfony/symfony#48080symfony/symfony#48102Commits-------9d4045f [Security] Use expression for #[IsGranted()] subject
Currently, the request is only available to the
#[IsGranted]attribute when it's a controller argument, eg:However, since the
$requestvariable might not always be needed in the controller itself, it seems kind of weird to have to add it as an argument just so the#[IsGranted]attribute could work. With this PR, the request will always be available to the attribute:Don't know if this qualifies as a tweak for 6.2 or feature for 6.3.