[Security] Be able to know the reasons of the denied access #35592

maidmaid · 2020-02-04T15:35:07Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	#27995, #26343
License	MIT
Doc PR	/

About the voters, the gif of the the original issue sums up the situation well :

Currently, the voters can only say yes or no. This PR allows to give a reason from the voters.

Before :

class PostVoter extends Voter
{
    // ...

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
    {
        if ($subject->getAuthor() !== $token->getUser()) {
            return false;
        }

        if ($subject->getStatus() !== 'draft') {
            return false;
        }

        return true;
    }
}

After :

class PostVoter extends Voter
{
    // ...

    protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
    {
        if ($subject->getAuthor() !== $token->getUser()) {
            return $this->deny('You are not the author.')
        }

        if ($subject->getStatus() !== 'draft') {
            return $this->deny('The post is no more a draft.')
        }

        return $this->grant();
    }
}

A voter returns no more an int based on the constant ACCESS_{GRANTED,ABSTAIN,DENIED} but a Vote object which contains the access result and the reason. Thus, the security panel of the profiler can display the reason of each vote.

The access decision manager returns no more a bool for the final verdict but an AccessDecision object which contains the final verdict and all the votes. Thus, the 403 response can display all the denied raisons.

It's in a PoC state. Todo : BC layer, migration of the others voters, update current tests, add new tests...

Pierstoval

I like this PR very much 😄 I hope it's gonna be appreciated by the core team!
In the meantime I have a few comments :)

src/Symfony/Component/Security/Core/Authorization/AccessDecision.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/TraceableVoter.php

ro0NL

we should re-evaluate #21094 (comment) first

AccessDecision vs Vote is confusing, there should be only 1 IMHO. AFAIK there's no real reason to merge multiple votes now, which is a new behavior

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

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

ro0NL · 2020-02-04T19:51:40Z

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

+    /**
+     * Sets an access decision and appends the denied reasons to the exception message.
+     */
+    public function setAccessDecision(AccessDecision $accessDecision)


this cannot be stateful IMHO

What do you suggest ?

We could make it stateless and have a specific exception render for AccessDeniedException. WDYT ?

maidmaid · 2020-02-05T08:48:00Z

AccessDecision vs Vote is confusing, there should be only 1

We need a distinction between 1 vote amongst others and the final decision. E.g. 3 votes in + 4 votes against = decision to deny (in a consensus strategy). So, a voter votes (it returns a Vote object), whereas the access decision manager decides the final access (it returns an AccessDecision object). Maybe, AccessDecision could be renamed to something like Verdict for clarity ?

chalasr · 2020-02-05T09:23:00Z

AccessDecision could be renamed to something like Verdict for clarity ?

Not sure it would bring much: AccessDecisionManager takes access decisions, verdict is just a synonym, might be more confusing than helpful.

src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig

noniagriconomie · 2020-02-05T21:13:46Z

src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig

                                                {% endif %}
                                            </td>
+                                            <td class="font-normal text-small">{{ voter_detail['vote'].reason }}</td>


maybe only display if not empty reason?

The td html node must always be present.

I believe we are used to display ~ or (unknown) to not let it blank. Could you check and apply what looks better?

Much better imo ;)

@javiereguiluz would you have a suggestion here maybe ?

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

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

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

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

src/Symfony/Component/Security/Core/Authorization/Voter/Voter.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/Authorization/AccessDecision.php

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

chalasr · 2020-02-18T21:10:29Z

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

+trait AccessTrait
+{
+    /** @var int */
+    protected $access;


Should be private.

Already discussed here #35592 (comment). Ok for you ?

Not sure to understand the rationale there.
If a class uses this trait, it does have write access to the private properties defined by the trait. Using protected would only serve for inheritance purpose, which is not needed here AFAIK.
Am I missing something?

Co-Authored-By: Antoine Makdessi <antoine.makdessi@agriconomie.com>

maidmaid · 2020-03-03T10:20:54Z

Changes committed from your feedbacks, thanks !

This PR looks appreciated and its design seems validated, so I'm gonna complete it by writting the tests soon.

noniagriconomie · 2020-03-03T10:29:03Z

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

@@ -11,13 +11,22 @@

 namespace Symfony\Component\Security\Core\Authorization\Voter;

+/**
+ * A Vote is returned by a Voter and contains the access (granted, abstain or denied). It can also contains a reason


Suggested change

* A Vote is returned by a Voter and contains the access (granted, abstain or denied). It can also contains a reason

* A Vote is returned by a Voter and contains the access (granted, abstain or denied).

* It can also contains a reason explaining the vote decision.

stephanvierkant · 2020-04-24T09:16:10Z

Thanks for create this PR!

What about creating multiple reasons to deny access, like you can with workflows?

wouterj

I like this idea. I think I also agree on the difference between Vote and AccessDecision. An access decision is a collection of votes.

As commented, there is a need for BC layers in the usage part. I guess the only possible method to change this would be to create a new method/new interface. In that case, I think it makes sense to also think a little bit about how we're going to solve #28868 (comment) in the future, as it might also require a different authorization API. If we need to create a new authorization API for this change, I think it's best to take that change into account as well (not fix it in this PR, but designing an API that allows it to happen without change). On second thought, let's not combine these things and only focus on this feature. The other issue can be implemented using the current interface as well.

wouterj · 2020-04-24T11:57:57Z

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

@@ -33,7 +33,7 @@ interface VoterInterface
     * @param mixed $subject    The subject to secure
     * @param array $attributes An array of attributes associated with the method being invoked
     *
-     * @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED
+     * @return int|Vote either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED


The same thing applies here: I don't think we can change the method return signature from int to Vote, as it'll impact how you need to use this method.

wouterj · 2020-04-24T11:59:40Z

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

@@ -26,7 +26,7 @@ interface AccessDecisionManagerInterface
     * @param array  $attributes An array of attributes associated with the method being invoked
     * @param object $object     The object to secure
     *
-     * @return bool true if the access is granted, false otherwise
+     * @return bool|AccessDecision Returning a boolean is deprecated since Symfony 5.1. Return an AccessDecision object instead.


This interface is not internal, so we should also be BC with the usage of the method AFAIK. I don't think we currently preserve backwards compatibility in e.g. this example:

if (!$accessDecisionManager->decide(...)) { throw new AccessDeniedException(); }

If an AccessDecision::createDenied() is returned, the condition !(AccessDecision::createDenied()) is false, causing the access denied exception not to be throw (while it was thrown using the old bool signature).

wouterj · 2020-04-24T12:01:44Z

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

+        $this->lastAccessDecision = $this->accessDecisionManager->decide($token, [$attribute], $subject);
+
+        if (\is_bool($this->lastAccessDecision)) {
+            trigger_deprecation('symfony/security', 5.1, 'Returning a boolean from the "%s::decide()" method is deprecated. Return an "%s" object instead', \get_class($this->accessDecisionManager), AccessDecision::class);


Suggested change

trigger_deprecation('symfony/security', 5.1, 'Returning a boolean from the "%s::decide()" method is deprecated. Return an "%s" object instead', \get_class($this->accessDecisionManager), AccessDecision::class);

trigger_deprecation('symfony/security-core', 5.1, 'Returning a boolean from the "%s::decide()" method is deprecated, return an "%s" object instead', \get_class($this->accessDecisionManager), AccessDecision::class);

wouterj · 2020-04-24T12:02:06Z

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

+    private function vote(VoterInterface $voter, TokenInterface $token, $subject, array $attributes): Vote
+    {
+        if (\is_int($vote = $voter->vote($token, $subject, $attributes))) {
+            trigger_deprecation('symfony/security', 5.1, 'Returning an int from the "%s::vote()" method is deprecated. Return a "%s" object instead.', \get_class($this->voter), Vote::class);


Suggested change

trigger_deprecation('symfony/security', 5.1, 'Returning an int from the "%s::vote()" method is deprecated. Return a "%s" object instead.', \get_class($this->voter), Vote::class);

trigger_deprecation('symfony/security-core', 5.1, 'Returning an int from the "%s::vote()" method is deprecated, return a "%s" object instead.', \get_class($this->voter), Vote::class);

wouterj · 2020-04-24T14:45:44Z

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

+        return new self(VoterInterface::ACCESS_DENIED, $reason, $parameters);
+    }
+
+    public function merge(self $vote): void


I don't really see the value of this method. It doesn't fullfill what I would call "merge" (i.e. it only merges the reason, what happens if we pass denied into the merge of a granted vote?) and it's used only in one specific case. I would rather add a setReason() method and doing this directly in the voter.

fabpot · 2020-08-12T07:50:27Z

@wouterj @maidmaid What would be the next steps to move this PR forward?

wouterj · 2020-08-13T10:18:37Z

I think this comment from my review is the most important: How can we make this change BC?

deguif · 2020-09-16T13:24:15Z

Will we be able to get in in next release before feature freeze?

fabpot · 2020-09-16T13:41:37Z

@deguif Depends on when it's ready to be merged. We still have 2 weeks for 5.2.

noniagriconomie · 2020-09-16T14:23:30Z

@maidmaid @wouterj @fabpot

Cant we use the same logic as the https://github.com/symfony/symfony/blob/master/src/Symfony/Component/EventDispatcher/LegacyEventDispatcherProxy.php?

a proxy class that handles previous/new voting feature? (I am unsure if it can be applied here)

concerning the feature itself, i think it is usefull :) (like the workflow transition blocker)

(i can help test it if needed :))

dkarlovi · 2020-09-16T16:04:13Z

This seems like very close to what was done for Workflow, guards got the same ability there from setting a boolean value, likely the same approach can be taken.

fabpot · 2020-09-30T06:05:58Z

@maidmaid Still interested in working on this? This can still be part of 5.2 if approved before tomorrow.

maidmaid · 2020-10-02T11:44:37Z

Sorry for the delay. I missed the 5.2 train I guess. I'm gonna resume this work soon. Let's wait 5.3 :)

fabpot · 2020-10-07T15:54:34Z

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

noniagriconomie · 2021-02-22T07:40:13Z

@maidmaid friendly ping,
are you able to complete this code for the next version 5.3? If not, can help be provided?
(related to #27995 (comment) and following comments)

maidmaid · 2021-03-05T11:19:33Z

@noniagriconomie I don't have a lot of time to finish this work. I would be happy if someone finished it :)

azjezz · 2021-04-04T17:54:10Z

Hey @maidmaid, i would like to pick up where you left here and continue working on this PR, is that okay with you?

noniagriconomie · 2021-04-06T07:03:17Z

@azjezz i would be pleased to help you, review or testing in real project if you need :)

azjezz · 2021-04-06T09:37:24Z

@ noniagriconomie see #40711

…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

carsonbot added Status: Needs Review Security Feature Deprecation labels Feb 4, 2020

maidmaid force-pushed the vote branch from 949cf7d to d905dad Compare February 4, 2020 15:37

Pierstoval reviewed Feb 4, 2020

View reviewed changes

ro0NL reviewed Feb 4, 2020

View reviewed changes

chalasr added this to the next milestone Feb 5, 2020

noniagriconomie reviewed Feb 5, 2020

View reviewed changes

poc

26a8381

maidmaid force-pushed the vote branch 2 times, most recently from 2261746 to 69e8d07 Compare February 18, 2020 10:31

round 2

868b9ff

maidmaid force-pushed the vote branch from 69e8d07 to 868b9ff Compare February 18, 2020 10:31

This comment has been minimized.

Sign in to view