Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

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

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
26a8381
poc
maidmaid Jan 8, 2020
868b9ff
round 2
maidmaid Feb 18, 2020
edfc392
Add a "~" default filter in profiler twig view
maidmaid Mar 3, 2020
ec41df2
Fix the message deprecated
maidmaid Mar 3, 2020
842e59e
Fix a typo
maidmaid Mar 3, 2020
7339108
Fix a typo
maidmaid Mar 3, 2020
8ef8d19
Make final the Vote and the AccessDecision classes
maidmaid Mar 3, 2020
c8a360c
Fix minor
maidmaid Mar 3, 2020
c4cfb5f
Add phpdoc
maidmaid Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
round 2
  • Loading branch information
@maidmaid
maidmaid committed Feb 18, 2020
commit 868b9ff5046a6ec776e17a2d74380dc749867428
36 changes: 31 additions & 5 deletions 36 src/Symfony/Component/Security/Core/Authorization/AccessDecision.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@

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
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
{
use AccessTrait;

/** @var Vote[] */
private $votes;
private $votes = [];

private function __construct(int $access, array $votes = [])
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
{
Expand All @@ -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; });
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
}
}
7 changes: 6 additions & 1 deletion 7 src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
$vote = Vote::create($vote);
}

return $vote;
}
}
10 changes: 9 additions & 1 deletion 10 src/Symfony/Component/Security/Core/Authorization/AuthorizationChecker.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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);


return $this->lastAccessDecision;
}

return $this->lastAccessDecision->isGranted();
}

public function getLastAccessDecision(): AccessDecision
Expand Down
2 changes: 1 addition & 1 deletion 2 src/Symfony/Component/Security/Core/Authorization/Voter/AccessTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
trait AccessTrait
{
/** @var int */
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
private $access;
protected $access;
Copy link
Member

Choose a reason for hiding this comment

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

Should be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?


public function getAccess(): int
{
Expand Down
21 changes: 0 additions & 21 deletions 21 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterInterface.php
View file
Open in desktop

This file was deleted.

30 changes: 0 additions & 30 deletions 30 src/Symfony/Component/Security/Core/Authorization/Voter/ExplainedVoterTrait.php
View file
Open in desktop

This file was deleted.

31 changes: 27 additions & 4 deletions 31 src/Symfony/Component/Security/Core/Authorization/Voter/Voter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@
* @author Roman Marintšenko <inoryy@gmail.com>
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
abstract class Voter implements VoterInterface, ExplainedVoterInterface
abstract class Voter implements VoterInterface
{
use ExplainedVoterTrait;

/**
* {@inheritdoc}
*/
Expand All @@ -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 {
Expand All @@ -50,6 +49,30 @@ public function vote(TokenInterface $token, $subject, array $attributes)
return $vote;
}

/**
* Creates an granted vote.
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
*/
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.
maidmaid marked this conversation as resolved.
Show resolved Hide resolved
*/
public function deny(string $reason = '', array $parameters = []): Vote
{
return Vote::createDenied($reason, $parameters);
}

/**
* Determines if the attribute and subject are supported by this voter.
*
Expand Down
3 changes: 1 addition & 2 deletions 3 src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -70,7 +69,7 @@ public function setSubject($subject)
public function setAccessDecision(AccessDecision $accessDecision)
Copy link
Contributor

Choose a reason for hiding this comment

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

this cannot be stateful IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
$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));
}

Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.