Skip to content

Updating AbstractVoter so that the method receives the TokenInterface #15870

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 2 commits into from
Closed

Updating AbstractVoter so that the method receives the TokenInterface #15870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b101d24
Updating AbstractVoter so that the method receives the TokenInterface
weaverryan Sep 23, 2015
3665c14
Adding a note about the method becoming abstract in 3.0
weaverryan Sep 24, 2015
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
36 changes: 36 additions & 0 deletions UPGRADE-2.8.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,3 +406,39 @@ FrameworkBundle
session:
cookie_httponly: false
```

Security
--------

* The AbstractToken::isGranted() method was deprecated. Instead,
override the voteOnAttribute() method. This method has one small
difference: it's passed the TokenInterface instead of the user:

Before:

```php
class MyCustomVoter extends AbstractVoter
{
// ...

protected function isGranted($attribute, $object, $user = null)
{
// ...
}
}
```

After:

```php
class MyCustomVoter extends AbstractVoter
{
// ...

protected function voteOnAttribute($attribute, $object, TokenInterface $token)
{
$user = $token->getUser();
// ...
}
}
```
46 changes: 42 additions & 4 deletions src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ public function vote(TokenInterface $token, $object, array $attributes)
// abstain vote by default in case none of the attributes are supported
$vote = self::ACCESS_ABSTAIN;

$reflector = new \ReflectionMethod($this, 'voteOnAttribute');
$isNewOverwritten = $reflector->getDeclaringClass()->getName() !== 'Symfony\Component\Security\Core\Authorization\Voter\AbstractVoter';
Copy link
Member

Choose a reason for hiding this comment

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

I would use __CLASS__ here rather than the class name in a string. It makes it clearer than it is the current class, and is less likely to break if we rename things

if (!$isNewOverwritten) {
@trigger_error(sprintf("The AbstractVoter::isGranted method is deprecated since 2.8 and won't be called anymore in 3.0. Override voteOnAttribute() instead.", $reflector->class), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

you should mention the class of $this so that people can know in which class the change is needed.

}

foreach ($attributes as $attribute) {
if (!$this->supportsAttribute($attribute)) {
continue;
Expand All @@ -73,9 +79,16 @@ public function vote(TokenInterface $token, $object, array $attributes)
// as soon as at least one attribute is supported, default is to deny access
$vote = self::ACCESS_DENIED;

if ($this->isGranted($attribute, $object, $token->getUser())) {
// grant access as soon as at least one voter returns a positive response
return self::ACCESS_GRANTED;
if ($isNewOverwritten) {
if ($this->voteOnAttribute($attribute, $object, $token)) {
// grant access as soon as at least one voter returns a positive response
return self::ACCESS_GRANTED;
}
} else {
if ($this->isGranted($attribute, $object, $token->getUser())) {
// grant access as soon as at least one voter returns a positive response
return self::ACCESS_GRANTED;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This could actually become an elseif { ... }

}
}

Expand Down Expand Up @@ -107,7 +120,32 @@ abstract protected function getSupportedAttributes();
* @param object $object
* @param UserInterface|string $user
*
* @deprecated This method will be removed in 3.0 - override voteOnAttribute instead.
*
* @return bool
*/
abstract protected function isGranted($attribute, $object, $user = null);
protected function isGranted($attribute, $object, $user = null)
{
Copy link
Member

Choose a reason for hiding this comment

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

this method should trigger a deprecation warning when being called, in case someone implements voteOnAttribute by calling it.

return false;
}

/**
* Perform a single access check operation on a given attribute, object and (optionally) user
* It is safe to assume that $attribute and $object's class pass supportsAttribute/supportsClass
* $user can be one of the following:
* a UserInterface object (fully authenticated user)
* a string (anonymously authenticated user).
*
* This method will become abstract in 3.0.
*
* @param string $attribute
* @param object $object
* @param TokenInterface $token
*
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a note that this method should be abstract in 3.0 so that we do not forget that when cleaning the master branch?

* @return bool
*/
protected function voteOnAttribute($attribute, $object, TokenInterface $token)
{
return false;
Copy link
Member

Choose a reason for hiding this comment

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

the default implementation should rather throw a BadMethodCallException, to prevent people from calling parent::voteOnAttribute, which would break when making the method abstract

Copy link
Member

Choose a reason for hiding this comment

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

actually, this could even be simplified a lot, avoiding the Reflection-based logic:

  • always call voteOnAttribute in vote
  • throw a deprecation warning in AbstractVoter::voteOnAttribute saying it should be overwritten
  • implement it as return $this->isGranted($attribute, $object, $token->getUser());

Copy link
Member Author

Choose a reason for hiding this comment

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

I was sure there was a simpler way, and this looks great! See #15921

}
}
32 changes: 32 additions & 0 deletions src/Symfony/Component/Security/Tests/Core/Authentication/Voter/AbstractVoterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Security\Tests\Core\Authentication\Voter;

use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\AbstractVoter;

/**
Expand Down Expand Up @@ -46,6 +47,17 @@ public function testVote($expectedVote, $object, $attributes, $message)
$this->assertEquals($expectedVote, $this->voter->vote($this->token, $object, $attributes), $message);
}

/**
* @dataProvider getData
* @group legacy
*/
public function testVoteUsingDeprecatedIsGranted($expectedVote, $object, $attributes, $message)
{
$voter = new DeprecatedVoterFixture();

$this->assertEquals($expectedVote, $voter->vote($this->token, $object, $attributes), $message);
}

public function getData()
{
return array(
Expand Down Expand Up @@ -75,6 +87,26 @@ protected function getSupportedAttributes()
return array('foo', 'bar', 'baz');
}

protected function voteOnAttribute($attribute, $object, TokenInterface $token)
{
return $attribute === 'foo';
}
}

class DeprecatedVoterFixture extends AbstractVoter
{
protected function getSupportedClasses()
{
return array(
'Symfony\Component\Security\Tests\Core\Authentication\Voter\ObjectFixture',
);
}

protected function getSupportedAttributes()
{
return array('foo', 'bar', 'baz');
}

protected function isGranted($attribute, $object, $user = null)
{
return $attribute === 'foo';
Expand Down