[Security] Add the ability for voter to return decision reason #43147

yellow1912 · 2021-09-23T11:34:40Z

Sometimes we need to know why the voter makes a certain decision.
This changes allows us to return a vote object which contains a reason.

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	Fix #27995, #26343, #35592
License	MIT
Doc PR	n/a

This is a PR continuing the work started by @maidmaid and @noniagriconomie in #35592

All the code is contributed by azjezz, I only merge the changes into 5.4 branch since he/she does not have time right now.
(Original PR: #40711)

The initial PR is meant to see if all tests passed (I expect tests to fail). More changes will come later since this PR contains BC.

Sometimes we need to know why the voter makes a certain decision. This changes allows us to return a vote object which contains a reason.

carsonbot · 2021-09-23T11:34:44Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

We cannot use the interface at the moment because the new method is not available in the interface yet. We also cannot put the new method inside the interface because it can break current/old code.

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

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

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

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

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

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

src/Symfony/Component/Security/Core/Event/VoteEvent.php

src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php

1. Add missing getDecision method to AuthorizationChecker 2. Do not force int return on TraceableVoter::vote for now

1. Change internal methods from public to protected 2. Remove useless phpdoc

Add missing message passed through constructor

noniagriconomie · 2021-09-23T14:14:37Z

@yellow1912 thank you taking over this!
As I mentionned on the other PR, my "work" here is only PR review :)

Feel free to ping me when you consider this PR finaly reviewable, I will be glad to do so 👍

yellow1912 · 2021-10-02T08:24:49Z

@noniagriconomie can you check and let me know if there is anything else I should fix? I see unittests still fail but not entirely sure why, I see only notices.

noniagriconomie · 2021-10-15T14:30:08Z

@yellow1912 unfortunatly, i may not be the best to answer if there is anything else I should fix here
maybe ping some core maintainers and/or code owners :)
but iirc the v5.4 is feature freezed

yellow1912 · 2021-10-17T13:20:08Z

@stof Please let me know if:

There is anything else I should fix?
Should I bump this to another branch if 5.4 is frozen? And which one should that be?

chalasr · 2021-10-20T09:18:31Z

I'll take time to test this thoroughly during the last week of October. My main concern is about the BC layer, for 3rd party bundles especially, but I feel like we are quite close.

yellow1912 · 2021-11-10T02:35:41Z

@chalasr Have you got the time to take a look at this? Once you checked everything I can update it to 6.x instead because it seems like 5.x is closed now.

chalasr · 2021-11-11T16:22:35Z

@yellow1912 We will need to make this happen on 6.1, yes.

The core team' focus is currently about stabilizing core code/docs and syncing community projects for 5.4 and 6.0.

I will revisit this early in the 6.1 development stage. Thanks for your patience.

derrabus · 2021-11-11T17:29:38Z

In the meantime, you can already rebase onto the 6.0 branch (6.1 is not open yet) which also means you can use all the juicy new PHP 8.0 features. 😎

noniagriconomie · 2022-01-18T10:12:30Z

@yellow1912 I've reminded myself this PR, did you had time/energy to rebase it for sf v6?

thank you :)

yellow1912 · 2022-01-19T17:32:43Z

@yellow1912 I've reminded myself this PR, did you had time/energy to rebase it for sf v6?

thank you :)

I haven't got the time to do that, but I will try to finish it within the next 2 weeks, too many things on my disk at the moment.

fabpot · 2022-07-22T12:07:51Z

Closing in favor of #46493

…e (nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Add ability for voters to explain their vote | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #27995, #26343, #35592, #43147 | License | MIT This PR takes over #58107, which itself took over from #46493, etc. It takes the only approach I could think of that would preserve BC. The visible tip of what this achieves is: ![image](https://github.com/user-attachments/assets/67bd0678-868f-40ed-b2c6-3b1f10ffe101) ![image](https://github.com/user-attachments/assets/715814d8-e4de-47f0-aa0e-c412092389ff) Internally, this provides a new access audit log infrastructure that relies on new `AccessDecision` and `Vote` objects. Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from #58107 / #46493 as much as possible. Commits ------- c6eb9ae [Security] Add ability for voters to explain their vote

Add the ability for voter to return decision reason

81f7499

Sometimes we need to know why the voter makes a certain decision. This changes allows us to return a vote object which contains a reason.

yellow1912 requested review from chalasr and wouterj as code owners September 23, 2021 11:34

carsonbot added Status: Needs Review Deprecation Feature labels Sep 23, 2021

carsonbot added this to the 5.4 milestone Sep 23, 2021

Create mock with DecisionManager class

85a9b3d

We cannot use the interface at the moment because the new method is not available in the interface yet. We also cannot put the new method inside the interface because it can break current/old code.

stof requested changes Sep 23, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 23, 2021

yellow1912 added 12 commits September 23, 2021 19:16

Fix phpunit test errors

293a057

1. Add missing getDecision method to AuthorizationChecker 2. Do not force int return on TraceableVoter::vote for now

Standardize Voter code

66d49ac

1. Change internal methods from public to protected 2. Remove useless phpdoc

Remove AccessTrait

b43c6e5

Change $access property to private as requested

cba2283

Remove bool return to avoid BC

6675182

Fix return typehint for decide method

c15b796

Switch to using constructor for Vote

7e5feef

Fix AccessDeniedException

55bc4cd

Add missing message passed through constructor

Fix coding standards

219a3be

Fix test results

8c5fcb0

Fix deprecation to match with test

d970920

Fix decide method deprecation test matching

a9578c4

yellow1912 added 5 commits September 23, 2021 21:30

Fix tests in AccessListenerTest

1357b53

Add missing AbstractToken in AccessListenerTest

b949317

getDecisionLog return object, not boolean

6b9ec12

Attempt to fix test in AccessListenerTest

dfec312

Fix testAccessDecisionManagerCalledByVoter

95e9682

Allow getAccessDecision to return null in AccessDeniedException

9d59fdd

chalasr added the Security label Oct 7, 2021

carsonbot changed the title ~~Add the ability for voter to return decision reason~~ [Security] Add the ability for voter to return decision reason Oct 7, 2021

Merge security.html.twig with new changes

7a4dc72

yellow1912 added 2 commits October 17, 2021 20:12

Fix coding standards

93c0c10

Add missing dot to comply with standards

5ebf1b5

javiereguiluz mentioned this pull request Oct 27, 2021

Customize Voter's "access denied" symfony/symfony-docs#16011

Closed

fabpot modified the milestones: 5.4, 6.1 Nov 3, 2021

derrabus changed the base branch from 5.4 to 6.0 November 11, 2021 17:29

derrabus changed the base branch from 6.0 to 6.1 January 18, 2022 12:36

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

alamirault added a commit to alamirault/symfony that referenced this pull request May 26, 2022

Begin merge 6.2 symfony#43147 symfony#35592

0d1e7e5

alamirault mentioned this pull request May 29, 2022

[Security] Add the ability for voter to return decision reason #46493

Closed

3 tasks

fabpot closed this Jul 22, 2022

eltharin mentioned this pull request Aug 27, 2024

[Security][SecurityBundle] Add messages and score on votes #58107

Closed

eltharin mentioned this pull request Jan 21, 2025

[Security] Continuation : Add messages on votes #59573

Closed

nicolas-grekas mentioned this pull request Feb 13, 2025

[Security] Add ability for voters to explain their vote #59771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Add the ability for voter to return decision reason #43147

[Security] Add the ability for voter to return decision reason #43147

yellow1912 commented Sep 23, 2021 •

edited

Loading

carsonbot commented Sep 23, 2021

noniagriconomie commented Sep 23, 2021

yellow1912 commented Oct 2, 2021

noniagriconomie commented Oct 15, 2021

yellow1912 commented Oct 17, 2021

chalasr commented Oct 20, 2021

yellow1912 commented Nov 10, 2021

chalasr commented Nov 11, 2021

derrabus commented Nov 11, 2021

noniagriconomie commented Jan 18, 2022

yellow1912 commented Jan 19, 2022

fabpot commented Jul 22, 2022 •

edited

Loading

[Security] Add the ability for voter to return decision reason #43147

[Security] Add the ability for voter to return decision reason #43147

Conversation

yellow1912 commented Sep 23, 2021 • edited Loading

carsonbot commented Sep 23, 2021

noniagriconomie commented Sep 23, 2021

yellow1912 commented Oct 2, 2021

noniagriconomie commented Oct 15, 2021

yellow1912 commented Oct 17, 2021

chalasr commented Oct 20, 2021

yellow1912 commented Nov 10, 2021

chalasr commented Nov 11, 2021

derrabus commented Nov 11, 2021

noniagriconomie commented Jan 18, 2022

yellow1912 commented Jan 19, 2022

fabpot commented Jul 22, 2022 • edited Loading

yellow1912 commented Sep 23, 2021 •

edited

Loading

fabpot commented Jul 22, 2022 •

edited

Loading