Skip to content

[Security][TwigBridge] Add access_decision() and access_decision_for_user() #61379

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 agree 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
nicolas-grekas merged 1 commit into from
Sep 3, 2025
Merged

[Security][TwigBridge] Add access_decision() and access_decision_for_user() #61379

nicolas-grekas merged 1 commit into from
Sep 3, 2025

Conversation

@florentdestremau
Copy link
Contributor

@florentdestremau florentdestremau commented Aug 10, 2025 •
edited by nicolas-grekas
Loading

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

This PR adds 2 new methods and the corresponding twig helpers to retrieve the access decision from a Voter rather than simply the result. This way, you can enumerate the AccessDecision::votes and retrieve the votes and their reason, and you can get the reason message.

For instance, in a controller or service:

$accessDecision = $this->security->getAccessDecision('post_edit', $post);
if (!$accessDecision->isGranted) {
    $this->addFlash('danger', $accessDecision->getMessage());
}

Same for a user on his behalf, and same in Twig;

{% set access = access_decision('post_edit', post) %}

<a href="/post/123/edit" 
    {% if not access.isGranted %}
    title="You don't have access to this post : {{ access.message }}"
    disabled
    {% endif %}
>
  Edit this post
</a>

@carsonbot carsonbot added this to the 7.4 milestone Aug 10, 2025
nicolas-grekas
nicolas-grekas reviewed Aug 19, 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.

looks useful to me
some tests + changelog entries would be needed

@nicolas-grekas nicolas-grekas changed the title [Security] New methods: access decision and access decision for user [Security][TwigBundle] access decision and access decision for user Aug 19, 2025
@nicolas-grekas nicolas-grekas changed the title [Security][TwigBundle] access decision and access decision for user [Security][TwigBridge] Add access_decision() and access_decision_for_user() Aug 19, 2025
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 19, 2025

Given that the main motivation for this is to explain in an API or in twig the reasons why the access is not granted, I'm tempted to pre-filter the failed votes already, or add an extra getter AccessDecision::failedVotes to help create these small extensions ?

this is already covered by the AccessDecision::getMessage method

@nicolas-grekas nicolas-grekas force-pushed the feature/access-decision branch 2 times, most recently from e97da9a to 5a74291 Compare August 29, 2025 09:03
@florentdestremau
Copy link
Contributor Author

florentdestremau commented Aug 29, 2025

Will update the changelogs in the next few days !

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 29, 2025 •
edited
Loading

They're already up to date ;)
The only thing that might be missing is test cases but that's just getters so not critical to me.

xabbuh
xabbuh reviewed Aug 29, 2025
View reviewed changes
@OskarStark OskarStark changed the title [Security][TwigBridge] Add access_decision() and access_decision_for_user() [Security][TwigBridge] Add access_decision() and access_decision_for_user() Aug 29, 2025
@nicolas-grekas nicolas-grekas force-pushed the feature/access-decision branch 3 times, most recently from 90cf011 to 226b5f8 Compare August 29, 2025 15:29
fabpot
fabpot reviewed Aug 30, 2025
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I don't have good alternative names, but I'm wondering if the method/function names couldn't be better. 🤔

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 1, 2025

I don't have good alternative names, but I'm wondering if the method/function names couldn't be better. 🤔

  • getAccessDecision() and getAccessDecisionForUser()?
  • checkAccess() / checkAccessForUser()

For twig, I can't think of a better name than access_decision[_for_user].

@florentdestremau
Copy link
Contributor Author

florentdestremau commented Sep 2, 2025

using a getter style might be easier to self-explain the fact that it's an object you're getting and not a boolean. For twig this is redundant indeed.

@nicolas-grekas nicolas-grekas force-pushed the feature/access-decision branch from 226b5f8 to 9884b2d Compare September 3, 2025 10:16
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 3, 2025

PR ready, comments addressed.

@nicolas-grekas nicolas-grekas force-pushed the feature/access-decision branch from 9884b2d to 1aead14 Compare September 3, 2025 11:36
xabbuh
xabbuh requested changes Sep 3, 2025
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the feature/access-decision branch from 1aead14 to 9b9c72d Compare September 3, 2025 11:44
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 3, 2025

Thank you @florentdestremau.

@nicolas-grekas nicolas-grekas merged commit 4919062 into symfony:7.4 Sep 3, 2025
9 of 12 checks passed
@nicolas-grekas nicolas-grekas mentioned this pull request Sep 3, 2025
Closed
@florentdestremau
Copy link
Contributor Author

florentdestremau commented Sep 3, 2025

Thank you! I feel like I just put the thing in motion but you did the hard part 😄

@florentdestremau florentdestremau deleted the feature/access-decision branch September 3, 2025 14:34
This was referenced Oct 27, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot left review comments

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

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

No one assigned

Labels

Feature Security Status: Reviewed TwigBridge

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

5 participants

@florentdestremau @nicolas-grekas @fabpot @xabbuh @carsonbot