From 26a8381e6efce8bf83c9c93b11df630771ec7465 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Wed, 8 Jan 2020 11:21:20 +0100 Subject: [PATCH 1/9] poc --- .../Controller/AbstractController.php | 1 + .../views/Collector/security.html.twig | 9 +-- .../Core/Authorization/AccessDecision.php | 52 +++++++++++++++ .../Authorization/AccessDecisionManager.php | 64 ++++++++++++------- .../AccessDecisionManagerInterface.php | 2 +- .../Authorization/AuthorizationChecker.php | 9 ++- .../TraceableAccessDecisionManager.php | 7 +- .../Core/Authorization/Voter/AccessTrait.php | 38 +++++++++++ .../Voter/ExplainedVoterInterface.php | 21 ++++++ .../Voter/ExplainedVoterTrait.php | 30 +++++++++ .../Authorization/Voter/TraceableVoter.php | 4 +- .../Core/Authorization/Voter/Vote.php | 62 ++++++++++++++++++ .../Core/Authorization/Voter/Voter.php | 16 +++-- .../Authorization/Voter/VoterInterface.php | 2 +- .../Security/Core/Event/VoteEvent.php | 5 +- .../Core/Exception/AccessDeniedException.php | 21 ++++++ 16 files changed, 301 insertions(+), 42 deletions(-) create mode 100644 src/Symfony/Component/Security/Core/Authorization/AccessDecision.php create mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php create mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php create mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php create mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php diff --git a/src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php b/src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php index 4818d379312a3..29ffc35757ca5 100644 --- a/src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php +++ b/src/Symfony/Bundle/FrameworkBundle/Controller/AbstractController.php @@ -232,6 +232,7 @@ protected function denyAccessUnlessGranted($attributes, $subject = null, string $exception = $this->createAccessDeniedException($message); $exception->setAttributes($attributes); $exception->setSubject($subject); + $exception->setAccessDecision($this->container->get('security.authorization_checker')->getLastAccessDecision()); throw $exception; } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig index 6b0819513fa04..86aea470390c9 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig +++ b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig @@ -346,16 +346,17 @@ attribute {{ voter_detail['attributes'][0] }} {% endif %} - {% if voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_GRANTED') %} + {% if voter_detail['vote'].access == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_GRANTED') %} ACCESS GRANTED - {% elseif voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_ABSTAIN') %} + {% elseif voter_detail['vote'].access == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_ABSTAIN') %} ACCESS ABSTAIN - {% elseif voter_detail['vote'] == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_DENIED') %} + {% elseif voter_detail['vote'].access == constant('Symfony\\Component\\Security\\Core\\Authorization\\Voter\\VoterInterface::ACCESS_DENIED') %} ACCESS DENIED {% else %} - unknown ({{ voter_detail['vote'] }}) + unknown ({{ voter_detail['vote'].access }}) {% endif %} + {{ voter_detail['vote'].reason }} {% endfor %} diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php new file mode 100644 index 0000000000000..4e8639cf690b4 --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php @@ -0,0 +1,52 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization; + +use Symfony\Component\Security\Core\Authorization\Voter\AccessTrait; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; + +class AccessDecision +{ + use AccessTrait; + + /** @var Vote[] */ + private $votes; + + private function __construct(int $access, array $votes = []) + { + $this->access = $access; + $this->votes = $votes; + } + + public static function createGranted(array $votes = []): self + { + return new self(VoterInterface::ACCESS_GRANTED, $votes); + } + + public static function createDenied(array $votes = []): self + { + return new self(VoterInterface::ACCESS_DENIED, $votes); + } + + /** + * @return Vote[] + */ + public function getVotes(int $access = null): array + { + if (null === $access) { + return $this->votes; + } + + return array_filter($this->votes, function (Vote $vote) use ($access) { return $vote->getAccess() === $access; }); + } +} diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index 8da5b06818ada..0baf03b68918e 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Security\Core\Authorization; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; use Symfony\Component\Security\Core\Exception\InvalidArgumentException; @@ -72,26 +73,27 @@ public function decide(TokenInterface $token, array $attributes, $object = null) * If all voters abstained from voting, the decision will be based on the * allowIfAllAbstainDecisions property value (defaults to false). */ - private function decideAffirmative(TokenInterface $token, array $attributes, $object = null): bool + private function decideAffirmative(TokenInterface $token, array $attributes, $object = null): AccessDecision { + $votes = []; $deny = 0; foreach ($this->voters as $voter) { - $result = $voter->vote($token, $object, $attributes); + $votes[] = $vote = $this->vote($voter, $token, $object, $attributes); - if (VoterInterface::ACCESS_GRANTED === $result) { - return true; + if ($vote->isGranted()) { + return AccessDecision::createGranted($votes); } - if (VoterInterface::ACCESS_DENIED === $result) { + if ($vote->isDenied()) { ++$deny; } } if ($deny > 0) { - return false; + return AccessDecision::createDenied($votes); } - return $this->allowIfAllAbstainDecisions; + return $this->decideIfAllAbstainDecisions(); } /** @@ -108,33 +110,37 @@ private function decideAffirmative(TokenInterface $token, array $attributes, $ob * If all voters abstained from voting, the decision will be based on the * allowIfAllAbstainDecisions property value (defaults to false). */ - private function decideConsensus(TokenInterface $token, array $attributes, $object = null): bool + private function decideConsensus(TokenInterface $token, array $attributes, $object = null): AccessDecision { + $votes = []; $grant = 0; $deny = 0; foreach ($this->voters as $voter) { - $result = $voter->vote($token, $object, $attributes); + $votes[] = $vote = $this->vote($voter, $token, $object, $attributes); - if (VoterInterface::ACCESS_GRANTED === $result) { + if ($vote->isGranted()) { ++$grant; - } elseif (VoterInterface::ACCESS_DENIED === $result) { + } elseif ($vote->isDenied()) { ++$deny; } } if ($grant > $deny) { - return true; + return AccessDecision::createGranted($votes); } if ($deny > $grant) { - return false; + return AccessDecision::createDenied($votes); } if ($grant > 0) { - return $this->allowIfEqualGrantedDeniedDecisions; + return $this->allowIfEqualGrantedDeniedDecisions + ? AccessDecision::createGranted() + : AccessDecision::createDenied() + ; } - return $this->allowIfAllAbstainDecisions; + return $this->decideIfAllAbstainDecisions(); } /** @@ -143,18 +149,19 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje * If all voters abstained from voting, the decision will be based on the * allowIfAllAbstainDecisions property value (defaults to false). */ - private function decideUnanimous(TokenInterface $token, array $attributes, $object = null): bool + private function decideUnanimous(TokenInterface $token, array $attributes, $object = null): AccessDecision { + $votes = []; $grant = 0; foreach ($this->voters as $voter) { foreach ($attributes as $attribute) { - $result = $voter->vote($token, $object, [$attribute]); + $votes[] = $vote = $this->vote($voter, $token, $object, [$attribute]); - if (VoterInterface::ACCESS_DENIED === $result) { - return false; + if ($vote->isDenied()) { + return AccessDecision::createDenied($votes); } - if (VoterInterface::ACCESS_GRANTED === $result) { + if ($vote->isGranted()) { ++$grant; } } @@ -162,10 +169,10 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje // no deny votes if ($grant > 0) { - return true; + return AccessDecision::createGranted($votes); } - return $this->allowIfAllAbstainDecisions; + return $this->decideIfAllAbstainDecisions(); } /** @@ -191,4 +198,17 @@ private function decidePriority(TokenInterface $token, array $attributes, $objec return $this->allowIfAllAbstainDecisions; } + + private function decideIfAllAbstainDecisions(): AccessDecision + { + return $this->allowIfAllAbstainDecisions + ? AccessDecision::createGranted() + : AccessDecision::createDenied() + ; + } + + private function vote(VoterInterface $voter, TokenInterface $token, $subject, array $attributes): Vote + { + return \is_int($vote = $voter->vote($token, $subject, $attributes)) ? Vote::create($vote) : $vote; + } } diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php index 97eb4f9d41a55..49d19e65993f7 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php +++ b/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 true if the access is granted, false otherwise */ public function decide(TokenInterface $token, array $attributes, $object = null); } diff --git a/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php b/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php index 9036bba029d2e..42b546ab445f5 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php +++ b/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php @@ -29,6 +29,8 @@ class AuthorizationChecker implements AuthorizationCheckerInterface private $accessDecisionManager; private $authenticationManager; private $alwaysAuthenticate; + /** @var AccessDecision */ + private $lastAccessDecision; public function __construct(TokenStorageInterface $tokenStorage, AuthenticationManagerInterface $authenticationManager, AccessDecisionManagerInterface $accessDecisionManager, bool $alwaysAuthenticate = false) { @@ -53,6 +55,11 @@ final public function isGranted($attribute, $subject = null): bool $this->tokenStorage->setToken($token = $this->authenticationManager->authenticate($token)); } - return $this->accessDecisionManager->decide($token, [$attribute], $subject); + return ($this->lastAccessDecision = $this->accessDecisionManager->decide($token, [$attribute], $subject))->isGranted(); + } + + public function getLastAccessDecision(): AccessDecision + { + return $this->lastAccessDecision; } } diff --git a/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php index 7690b3e2264bc..e6d6931eebd28 100644 --- a/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php @@ -12,6 +12,7 @@ namespace Symfony\Component\Security\Core\Authorization; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; /** @@ -48,7 +49,7 @@ public function __construct(AccessDecisionManagerInterface $manager) /** * {@inheritdoc} */ - public function decide(TokenInterface $token, array $attributes, $object = null): bool + public function decide(TokenInterface $token, array $attributes, $object = null) { $currentDecisionLog = [ 'attributes' => $attributes, @@ -71,9 +72,9 @@ public function decide(TokenInterface $token, array $attributes, $object = null) * Adds voter vote and class to the voter details. * * @param array $attributes attributes used for the vote - * @param int $vote vote of the voter + * @param Vote $vote vote of the voter */ - public function addVoterVote(VoterInterface $voter, array $attributes, int $vote) + public function addVoterVote(VoterInterface $voter, array $attributes, Vote $vote) { $currentLogIndex = \count($this->currentLog) - 1; $this->currentLog[$currentLogIndex]['voterDetails'][] = [ diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php new file mode 100644 index 0000000000000..f8d1b64e3d4ec --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php @@ -0,0 +1,38 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +trait AccessTrait +{ + /** @var int */ + private $access; + + public function getAccess(): int + { + return $this->access; + } + + public function isGranted(): bool + { + return VoterInterface::ACCESS_GRANTED === $this->access; + } + + public function isAbstain(): bool + { + return VoterInterface::ACCESS_ABSTAIN === $this->access; + } + + public function isDenied(): bool + { + return VoterInterface::ACCESS_DENIED === $this->access; + } +} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php new file mode 100644 index 0000000000000..5f9668bef7765 --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +interface ExplainedVoterInterface +{ + public function grant(string $reason = '', array $parameters = []): Vote; + + public function abstain(string $reason = '', array $parameters = []): Vote; + + public function deny(string $reason = '', array $parameters = []): Vote; +} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php new file mode 100644 index 0000000000000..a4c1e3390b48e --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php @@ -0,0 +1,30 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +trait ExplainedVoterTrait +{ + public function grant(string $reason = '', array $parameters = []): Vote + { + return Vote::createGranted($reason, $parameters); + } + + public function abstain(string $reason = '', array $parameters = []): Vote + { + return Vote::createAbstrain($reason, $parameters); + } + + public function deny(string $reason = '', array $parameters = []): Vote + { + return Vote::createDenied($reason, $parameters); + } +} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.php index 48f7651256710..210b585851b6d 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/TraceableVoter.php @@ -33,9 +33,9 @@ public function __construct(VoterInterface $voter, EventDispatcherInterface $eve $this->eventDispatcher = $eventDispatcher; } - public function vote(TokenInterface $token, $subject, array $attributes): int + public function vote(TokenInterface $token, $subject, array $attributes) { - $result = $this->voter->vote($token, $subject, $attributes); + $result = \is_int($vote = $this->voter->vote($token, $subject, $attributes)) ? Vote::create($vote) : $vote; $this->eventDispatcher->dispatch(new VoteEvent($this->voter, $subject, $attributes, $result), 'debug.security.authorization.vote'); diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php new file mode 100644 index 0000000000000..bcd369d5f6152 --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php @@ -0,0 +1,62 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization\Voter; + +class Vote +{ + use AccessTrait; + + private $reason; + private $parameters; + + private function __construct(int $access, string $reason = '', array $parameters = []) + { + $this->access = $access; + $this->reason = $reason; + $this->parameters = $parameters; + } + + public static function create(int $access, string $reason = '', array $parameters = []): self + { + return new self($access, $reason, $parameters); + } + + public static function createGranted(string $reason, array $parameters = []): self + { + return new self(VoterInterface::ACCESS_DENIED, $reason, $parameters); + } + + public static function createAbstrain(string $reason, array $parameters = []): self + { + return new self(VoterInterface::ACCESS_ABSTAIN, $reason, $parameters); + } + + public static function createDenied(string $reason, array $parameters = []): self + { + return new self(VoterInterface::ACCESS_DENIED, $reason, $parameters); + } + + public function merge(self $vote): void + { + $this->reason .= trim(' '.$vote->getReason()); + } + + public function getReason(): string + { + return $this->reason; + } + + public function getParameters(): array + { + return $this->parameters; + } +} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php index 1ef95d0e979dd..87f4ef36d52c0 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php @@ -19,15 +19,17 @@ * @author Roman Marintšenko * @author Grégoire Pineau */ -abstract class Voter implements VoterInterface +abstract class Voter implements VoterInterface, ExplainedVoterInterface { + use ExplainedVoterTrait; + /** * {@inheritdoc} */ public function vote(TokenInterface $token, $subject, array $attributes) { // abstain vote by default in case none of the attributes are supported - $vote = self::ACCESS_ABSTAIN; + $vote = $this->abstain(); foreach ($attributes as $attribute) { if (!$this->supports($attribute, $subject)) { @@ -35,11 +37,13 @@ public function vote(TokenInterface $token, $subject, array $attributes) } // as soon as at least one attribute is supported, default is to deny access - $vote = self::ACCESS_DENIED; + $vote = $this->deny(); - if ($this->voteOnAttribute($attribute, $subject, $token)) { + if (($v = $this->voteOnAttribute($attribute, $subject, $token))->isGranted()) { // grant access as soon as at least one attribute returns a positive response - return self::ACCESS_GRANTED; + return $v; + } else { + $vote->merge($v); } } @@ -62,7 +66,7 @@ abstract protected function supports(string $attribute, $subject); * * @param mixed $subject * - * @return bool + * @return bool|Vote */ abstract protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token); } diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php b/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php index 76c1968f5e2e1..8bd7d352ef548 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php +++ b/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 */ public function vote(TokenInterface $token, $subject, array $attributes); } diff --git a/src/Symfony/Component/Security/Core/Event/VoteEvent.php b/src/Symfony/Component/Security/Core/Event/VoteEvent.php index 78ac2f900ba48..aaa56f96eb2b4 100644 --- a/src/Symfony/Component/Security/Core/Event/VoteEvent.php +++ b/src/Symfony/Component/Security/Core/Event/VoteEvent.php @@ -11,6 +11,7 @@ namespace Symfony\Component\Security\Core\Event; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; use Symfony\Contracts\EventDispatcher\Event; @@ -28,7 +29,7 @@ final class VoteEvent extends Event private $attributes; private $vote; - public function __construct(VoterInterface $voter, $subject, array $attributes, int $vote) + public function __construct(VoterInterface $voter, $subject, array $attributes, Vote $vote) { $this->voter = $voter; $this->subject = $subject; @@ -51,7 +52,7 @@ public function getAttributes(): array return $this->attributes; } - public function getVote(): int + public function getVote(): Vote { return $this->vote; } diff --git a/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php b/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php index 0e59dc4077a91..c8796e0c2b2f9 100644 --- a/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php +++ b/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php @@ -11,6 +11,10 @@ namespace Symfony\Component\Security\Core\Exception; +use Symfony\Component\Security\Core\Authorization\AccessDecision; +use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; + /** * AccessDeniedException is thrown when the account has not the required role. * @@ -20,6 +24,8 @@ class AccessDeniedException extends RuntimeException { private $attributes = []; private $subject; + /** @var AccessDecision */ + private $accessDecision; public function __construct(string $message = 'Access Denied.', \Throwable $previous = null) { @@ -57,4 +63,19 @@ public function setSubject($subject) { $this->subject = $subject; } + + /** + * Sets an access decision and appends the denied reasons to the exception message. + */ + public function setAccessDecision(AccessDecision $accessDecision) + { + $this->accessDecision = $accessDecision; + $reasons = array_map(function (Vote $vote) { return $vote->getReason(); }, $this->accessDecision->getVotes(VoterInterface::ACCESS_DENIED)); + $this->message .= rtrim(' '.implode(' ', $reasons)); + } + + public function getAccessDecision(): AccessDecision + { + return $this->accessDecision; + } } From 868b9ff5046a6ec776e17a2d74380dc749867428 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 18 Feb 2020 11:29:31 +0100 Subject: [PATCH 2/9] round 2 --- .../Core/Authorization/AccessDecision.php | 36 ++++++++++++++++--- .../Authorization/AccessDecisionManager.php | 7 +++- .../Authorization/AuthorizationChecker.php | 10 +++++- .../Core/Authorization/Voter/AccessTrait.php | 2 +- .../Voter/ExplainedVoterInterface.php | 21 ----------- .../Voter/ExplainedVoterTrait.php | 30 ---------------- .../Core/Authorization/Voter/Voter.php | 31 +++++++++++++--- .../Core/Exception/AccessDeniedException.php | 3 +- 8 files changed, 75 insertions(+), 65 deletions(-) delete mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php delete mode 100644 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php index 4e8639cf690b4..12d1a770c3ebd 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php @@ -13,6 +13,7 @@ use Symfony\Component\Security\Core\Authorization\Voter\AccessTrait; use Symfony\Component\Security\Core\Authorization\Voter\Vote; +use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; class AccessDecision @@ -20,7 +21,7 @@ class AccessDecision use AccessTrait; /** @var Vote[] */ - private $votes; + private $votes = []; private function __construct(int $access, array $votes = []) { @@ -41,12 +42,37 @@ public static function createDenied(array $votes = []): self /** * @return Vote[] */ - public function getVotes(int $access = null): array + public function getVotes(): array { - if (null === $access) { - return $this->votes; - } + return $this->votes; + } + + /** + * @return Vote[] + */ + public function getGrantedVotes(): array + { + return $this->getVotesByAccess(Voter::ACCESS_GRANTED); + } + + /** + * @return Vote[] + */ + public function getAbstainedVotes(): array + { + return $this->getVotesByAccess(Voter::ACCESS_ABSTAIN); + } + + /** + * @return Vote[] + */ + public function getDeniedVotes(): array + { + return $this->getVotesByAccess(Voter::ACCESS_DENIED); + } + private function getVotesByAccess(int $access): array + { return array_filter($this->votes, function (Vote $vote) use ($access) { return $vote->getAccess() === $access; }); } } diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index 0baf03b68918e..5c8d040f9388d 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -209,6 +209,11 @@ private function decideIfAllAbstainDecisions(): AccessDecision private function vote(VoterInterface $voter, TokenInterface $token, $subject, array $attributes): Vote { - return \is_int($vote = $voter->vote($token, $subject, $attributes)) ? Vote::create($vote) : $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 "" object instead.', \get_class($this->voter), Vote::class); + $vote = Vote::create($vote); + } + + return $vote; } } diff --git a/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php b/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php index 42b546ab445f5..790f76b728316 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php +++ b/src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php @@ -55,7 +55,15 @@ final public function isGranted($attribute, $subject = null): bool $this->tokenStorage->setToken($token = $this->authenticationManager->authenticate($token)); } - return ($this->lastAccessDecision = $this->accessDecisionManager->decide($token, [$attribute], $subject))->isGranted(); + $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); + + return $this->lastAccessDecision; + } + + return $this->lastAccessDecision->isGranted(); } public function getLastAccessDecision(): AccessDecision diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php index f8d1b64e3d4ec..9584d168efedb 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php @@ -14,7 +14,7 @@ trait AccessTrait { /** @var int */ - private $access; + protected $access; public function getAccess(): int { diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php deleted file mode 100644 index 5f9668bef7765..0000000000000 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php +++ /dev/null @@ -1,21 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Authorization\Voter; - -interface ExplainedVoterInterface -{ - public function grant(string $reason = '', array $parameters = []): Vote; - - public function abstain(string $reason = '', array $parameters = []): Vote; - - public function deny(string $reason = '', array $parameters = []): Vote; -} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php b/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php deleted file mode 100644 index a4c1e3390b48e..0000000000000 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php +++ /dev/null @@ -1,30 +0,0 @@ - - * - * For the full copyright and license information, please view the LICENSE - * file that was distributed with this source code. - */ - -namespace Symfony\Component\Security\Core\Authorization\Voter; - -trait ExplainedVoterTrait -{ - public function grant(string $reason = '', array $parameters = []): Vote - { - return Vote::createGranted($reason, $parameters); - } - - public function abstain(string $reason = '', array $parameters = []): Vote - { - return Vote::createAbstrain($reason, $parameters); - } - - public function deny(string $reason = '', array $parameters = []): Vote - { - return Vote::createDenied($reason, $parameters); - } -} diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php index 87f4ef36d52c0..65efb8984e83c 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php @@ -19,10 +19,8 @@ * @author Roman Marintšenko * @author Grégoire Pineau */ -abstract class Voter implements VoterInterface, ExplainedVoterInterface +abstract class Voter implements VoterInterface { - use ExplainedVoterTrait; - /** * {@inheritdoc} */ @@ -39,7 +37,8 @@ public function vote(TokenInterface $token, $subject, array $attributes) // as soon as at least one attribute is supported, default is to deny access $vote = $this->deny(); - if (($v = $this->voteOnAttribute($attribute, $subject, $token))->isGranted()) { + $v = \is_bool($v = $this->voteOnAttribute($attribute, $subject, $token)) ? Vote::create($v) : $v; // BC layer + if ($v->isGranted()) { // grant access as soon as at least one attribute returns a positive response return $v; } else { @@ -50,6 +49,30 @@ public function vote(TokenInterface $token, $subject, array $attributes) return $vote; } + /** + * Creates an granted vote. + */ + public function grant(string $reason = '', array $parameters = []): Vote + { + return Vote::createGranted($reason, $parameters); + } + + /** + * Creates an abstained vote. + */ + public function abstain(string $reason = '', array $parameters = []): Vote + { + return Vote::createAbstrain($reason, $parameters); + } + + /** + * Creates an denied vote. + */ + public function deny(string $reason = '', array $parameters = []): Vote + { + return Vote::createDenied($reason, $parameters); + } + /** * Determines if the attribute and subject are supported by this voter. * diff --git a/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php b/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php index c8796e0c2b2f9..881431e9abf6c 100644 --- a/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php +++ b/src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php @@ -13,7 +13,6 @@ use Symfony\Component\Security\Core\Authorization\AccessDecision; use Symfony\Component\Security\Core\Authorization\Voter\Vote; -use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; /** * AccessDeniedException is thrown when the account has not the required role. @@ -70,7 +69,7 @@ public function setSubject($subject) public function setAccessDecision(AccessDecision $accessDecision) { $this->accessDecision = $accessDecision; - $reasons = array_map(function (Vote $vote) { return $vote->getReason(); }, $this->accessDecision->getVotes(VoterInterface::ACCESS_DENIED)); + $reasons = array_map(function (Vote $vote) { return $vote->getReason(); }, $this->accessDecision->getDeniedVotes()); $this->message .= rtrim(' '.implode(' ', $reasons)); } From edfc39209209a13e5068b2b03ade01802f541ca3 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 09:42:05 +0100 Subject: [PATCH 3/9] Add a "~" default filter in profiler twig view --- .../SecurityBundle/Resources/views/Collector/security.html.twig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig index 86aea470390c9..f11be0e1bf3bd 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig +++ b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig @@ -356,7 +356,7 @@ unknown ({{ voter_detail['vote'].access }}) {% endif %} - {{ voter_detail['vote'].reason }} + {{ voter_detail['vote'].reason|default('~') }} {% endfor %} From ec41df2b4fdbe26d8844fd49a4eb182147e25bbf Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 09:43:57 +0100 Subject: [PATCH 4/9] Fix the message deprecated Co-Authored-By: Antoine Makdessi --- .../Security/Core/Authorization/AccessDecisionManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index 5c8d040f9388d..1c9ef362d1535 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -210,7 +210,7 @@ private function decideIfAllAbstainDecisions(): AccessDecision 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 "" object instead.', \get_class($this->voter), Vote::class); + 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); $vote = Vote::create($vote); } From 842e59ea2fe361aa5697ab8a4cc98d0667ab4972 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 09:44:53 +0100 Subject: [PATCH 5/9] Fix a typo Co-Authored-By: Antoine Makdessi --- .../Component/Security/Core/Authorization/Voter/Voter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php index 65efb8984e83c..0d2cd5f41fa04 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php @@ -50,7 +50,7 @@ public function vote(TokenInterface $token, $subject, array $attributes) } /** - * Creates an granted vote. + * Creates a granted vote. */ public function grant(string $reason = '', array $parameters = []): Vote { From 7339108faeb762138d1ea6efb3c96d5fe44bc1b9 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 09:45:11 +0100 Subject: [PATCH 6/9] Fix a typo Co-Authored-By: Antoine Makdessi --- .../Component/Security/Core/Authorization/Voter/Voter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php index 0d2cd5f41fa04..5b7a7d1305474 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php @@ -66,7 +66,7 @@ public function abstain(string $reason = '', array $parameters = []): Vote } /** - * Creates an denied vote. + * Creates a denied vote. */ public function deny(string $reason = '', array $parameters = []): Vote { From 8ef8d19ccde9eb1195ae14c8e0801da76cb371ab Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 09:57:03 +0100 Subject: [PATCH 7/9] Make final the Vote and the AccessDecision classes --- .../Component/Security/Core/Authorization/AccessDecision.php | 2 +- .../Component/Security/Core/Authorization/Voter/Vote.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php index 12d1a770c3ebd..9e7a8c84765e0 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php @@ -16,7 +16,7 @@ use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; -class AccessDecision +final class AccessDecision { use AccessTrait; diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php index bcd369d5f6152..efc2ef1b4e71f 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php @@ -11,7 +11,7 @@ namespace Symfony\Component\Security\Core\Authorization\Voter; -class Vote +final class Vote { use AccessTrait; From c8a360c9f039634522ad83e30697426a0e74f0cb Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 11:10:08 +0100 Subject: [PATCH 8/9] Fix minor --- .../Component/Security/Core/Authorization/Voter/Vote.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php index efc2ef1b4e71f..3c30bc69a03ea 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php @@ -32,7 +32,7 @@ public static function create(int $access, string $reason = '', array $parameter public static function createGranted(string $reason, array $parameters = []): self { - return new self(VoterInterface::ACCESS_DENIED, $reason, $parameters); + return new self(VoterInterface::ACCESS_GRANTED, $reason, $parameters); } public static function createAbstrain(string $reason, array $parameters = []): self From c4cfb5f273f27c3ca418d6f1249bb25cd14a9da7 Mon Sep 17 00:00:00 2001 From: Dany Maillard Date: Tue, 3 Mar 2020 11:13:26 +0100 Subject: [PATCH 9/9] Add phpdoc --- .../Core/Authorization/AccessDecision.php | 15 +++++++++++++++ .../AccessDecisionManagerInterface.php | 2 +- .../Core/Authorization/Voter/AccessTrait.php | 5 ++++- .../Security/Core/Authorization/Voter/Vote.php | 9 +++++++++ .../Security/Core/Authorization/Voter/Voter.php | 3 ++- 5 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php index 9e7a8c84765e0..87ed37217a6db 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecision.php @@ -16,6 +16,11 @@ use Symfony\Component\Security\Core\Authorization\Voter\Voter; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; +/** + * An AccessDecision is returned by an AccessDecisionManager and contains the access verdict and all the related votes. + * + * @author Dany Maillard + */ final class AccessDecision { use AccessTrait; @@ -23,17 +28,27 @@ final class AccessDecision /** @var Vote[] */ private $votes = []; + /** + * @param int $access One of the VoterInterface::ACCESS_* constants + * @param Vote[] $votes + */ private function __construct(int $access, array $votes = []) { $this->access = $access; $this->votes = $votes; } + /** + * @param Vote[] $votes + */ public static function createGranted(array $votes = []): self { return new self(VoterInterface::ACCESS_GRANTED, $votes); } + /** + * @param Vote[] $votes + */ public static function createDenied(array $votes = []): self { return new self(VoterInterface::ACCESS_DENIED, $votes); diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php index 49d19e65993f7..11d01979c8b5e 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManagerInterface.php +++ b/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|AccessDecision 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. */ public function decide(TokenInterface $token, array $attributes, $object = null); } diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php index 9584d168efedb..c008d7cf0c4d0 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php @@ -11,9 +11,12 @@ namespace Symfony\Component\Security\Core\Authorization\Voter; +/** + * @author Dany Maillard + */ trait AccessTrait { - /** @var int */ + /** @var int One of the VoterInterface::ACCESS_* constants */ protected $access; public function getAccess(): int diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php index 3c30bc69a03ea..6a7e734b50caf 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Vote.php @@ -11,6 +11,12 @@ 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 + * explaining the why of the access which has been decided. + * + * @author Dany Maillard + */ final class Vote { use AccessTrait; @@ -18,6 +24,9 @@ final class Vote private $reason; private $parameters; + /** + * @param int $access One of the VoterInterface::ACCESS_* constants + */ private function __construct(int $access, string $reason = '', array $parameters = []) { $this->access = $access; diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php index 5b7a7d1305474..3556113700fcd 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php @@ -18,6 +18,7 @@ * * @author Roman Marintšenko * @author Grégoire Pineau + * @author Dany Maillard */ abstract class Voter implements VoterInterface { @@ -89,7 +90,7 @@ abstract protected function supports(string $attribute, $subject); * * @param mixed $subject * - * @return bool|Vote + * @return bool|Vote Returning a boolean is deprecated since Symfony 5.1. Return a Vote object instead. */ abstract protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token); }