Skip to content

[Security] add an AbstractVoter implementation #11183

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.

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 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* 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;

use Symfony\Component\Security\Core\User\UserInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

/**
* Abstract Voter implementation that reduces boilerplate code required to create a custom Voter
*
* @author Roman Marintšenko <inoryy@gmail.com>
*/
abstract class AbstractVoter implements VoterInterface
{
/**
* {@inheritdoc}
*/
public function supportsAttribute($attribute)
{
return in_array($attribute, $this->getSupportedAttributes());
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it's worth normalizing case here so it will work whether the user is doing is_granted('create', foo) or is_granted('CREATE', foo).

Copy link
Member

Choose a reason for hiding this comment

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

That's an interesting idea. I might be +1 to this. But I think an even better solution is if the security system were able to throw a clear exception if no voters voted on an attribute. So if you did a typo (e.g. CREETE), you'd see a clear message, rather than nobody voting and access being granted/denied 100% of the time based on your voting strategy

Copy link
Contributor

Choose a reason for hiding this comment

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

So if all voters abstain, rather than denying access, you think an exception should be thrown? If I've understood correctly that'd be a change in the AccessDecisionManager, which I would say is outside of the scope of this..

Copy link
Member

Choose a reason for hiding this comment

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

that would be a BC break

Copy link
Member

Choose a reason for hiding this comment

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

Yea, this is exactly what I would like, but Stof is right that it would be a BC break and I agree it's also outside of the scope of this :). But yes, in a perfect world, this seems like the right behavior to me.

}

/**
* {@inheritdoc}
*/
public function supportsClass($class)
{
foreach ($this->getSupportedClasses() as $supportedClass) {
if ($supportedClass === $class || is_subclass_of($class, $supportedClass)) {
Copy link
Member

Choose a reason for hiding this comment

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

http://php.net/is_a is easier IMHO

Copy link
Member

Choose a reason for hiding this comment

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

I think is_a still requires the first argument to be an object (all we have here are class names).

Copy link
Member

Choose a reason for hiding this comment

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

But you don't have to use get_class on line 50 ;)

Copy link
Member

Choose a reason for hiding this comment

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

supportsClass expects a string in the VoterInterface

Copy link
Member

Choose a reason for hiding this comment

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

ah ... Did not know ;) I never like this interface. It should be tweaked in SF3.0 IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, supportsClass and supportAttribute are actually never used in Symfony except by voters themselves. They should be removed from the interface in 3.0. But in the meantime, the interface should be respected

Copy link
Member

Choose a reason for hiding this comment

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

Just ran few tests:

php > var_dump(is_a('BadFunctionCallException', 'Exception', true));
bool(true)
php > var_dump(is_a('Exception', 'Exception', true));
bool(true)
php > var_dump(is_a('Foobar', 'Exception', true));
bool(false)

Copy link
Member

Choose a reason for hiding this comment

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

@weaverryan:

I think is_a still requires the first argument to be an object (all we have here are class names).

See the php doc:

allow_string

If this parameter set to FALSE, string class name as object is not allowed. This also prevents from calling autoloader if the class doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

in 5.3.3 to 5.3.6, is_a does not trigger the autoloading when passing a string. This won't cause issue for the behavior in the voter (as the class name comes from get_class and so the class is already autoloaded) but it could for other usages of the method.

Copy link
Member

Choose a reason for hiding this comment

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

good point ;)

return true;
}
}

return false;
}

/**
* Iteratively check all given attributes by calling isGranted
*
* This method terminates as soon as it is able to return ACCESS_GRANTED
* If at least one attribute is supported, but access not granted, then ACCESS_DENIED is returned
* Otherwise it will return ACCESS_ABSTAIN
*
* @param TokenInterface $token A TokenInterface instance
* @param object $object The object 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
*/
public function vote(TokenInterface $token, $object, array $attributes)
{
if (!$object || !$this->supportsClass(get_class($object))) {
return self::ACCESS_ABSTAIN;

Choose a reason for hiding this comment

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

Why returning ACCESS_ABSTAIN so early? This avoids using is_granted(MY_ATTRIBUTE) without passing object.

Take the role checking as example: is_granted('ROLE_ADMIN') ... no object is passed. Ans this kind of implementation is not possible here.

At first glance, I would do something like if(!!$object && !$this->supportsClass(...)

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 open an issue. At first glance, I agree that this is something we overlooked!

}
Copy link
Member

Choose a reason for hiding this comment

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

What if $object is null? I suppose we should only bother checking supportsClass if there is an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm conflicted on this.
The VoterInterface API/doc doesn't actually support $object to be null: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php#L52

But then again I know for a fact that it's possible to have non-object voters..

I suppose adding as simple check that object isn't null won't hurt

Copy link
Member

Choose a reason for hiding this comment

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

it supports it (see the RoleVoter). It is a phpdoc issue.


// abstain vote by default in case none of the attributes are supported
$vote = self::ACCESS_ABSTAIN;

foreach ($attributes as $attribute) {
if (!$this->supportsAttribute($attribute)) {
continue;
}

// as soon as at least one attribute is supported, default is to deny access
$vote = self::ACCESS_DENIED;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this line below the next if check.

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's the benefit?

Copy link
Contributor

Choose a reason for hiding this comment

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

The benefit is to avoid needless assignment if the next if check evaluates to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer increased readability over nano-optimization

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a matter of taste. Personally, I don't see, how one would be more readable than the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weaverryan or @javiereguiluz can you weight in here? I think our discussion with @rybakit has stalled due to personal preferences, so only way to move forward is via neutral position's opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Symfony project always prefer code readability over micro-optimizations, so maybe we should leave the original code as is.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the flow here is identical to the existing RoleVoter. I think we all agree that the difference is minor if anything, so I think we should keep it as it is.


if ($this->isGranted($attribute, $object, $token->getUser())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move $token->getUser() call out of the loop, as it was before?

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's the benefit of that?

Copy link
Contributor

Choose a reason for hiding this comment

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

As the result of $token->getUser() is not changed during the loop, I see no reason to call it on every iteration. Moreover it could be an expensive call depending on how one will implement this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Token/TokenInterface.php#L53

Nothing prevents a user from creating a custom token, like:

FooToken implements TokenInterface
{
    ...

    public function getUser()
    {
        // do a remote call here 
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weaverryan or @javiereguiluz can you weight in here? I think our discussion with @rybakit has stalled due to personal preferences, so only way to move forward is via neutral position's opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I'm really divided in this one :( Let's wait for @weaverryan to see if he has a preference for any of the two options.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't have a preference, so let's leave it. The argument that @rybakit is making is valid. But someone else could make the argument that calling $token->getUser() before the if statement could in theory mean that you're calling this function even if you don't need the user (i.e. if supportsAttribute returns false for all attributes). Both are micro-optimizations, but you can't support both. Also, even though we're supporting it, you really shouldn't be calling the voter with an array of attributes - so that makes this even a little less common.

Thanks guys!

Choose a reason for hiding this comment

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

I think you shouldn't call $token->getUser() at all. Passing the token itself would allow more flexibility for end users.

Copy link
Member

Choose a reason for hiding this comment

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

Most of the time you don't need the token, so we're catering for the most common use case (most developers don't really understand - or need to understand - what a token is). But you're certainly right, and if you need access to the token, you should just skip extending this AbstractVoter and implement the vote method on your own class with whatever logic you need.

// 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.

We are returning here as soon as we get a granted attribute. This will mean that any further attributes for this vote will be ignored. This could be an issue, if, for example, you have is_granted( ['VIEW', 'DELETE'], $obj) and the user has view permissions, but not delete permissions, as it will grant permission for both attributes.

May be better to negate the if and return as soon as a denied is returned.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we had this conversation previously here: #11183 (diff)

The implementation here is consistent with how the core voters (e.g. RoleVoter) handles multiple attributes, which I think is the best we can do. Imo, people shouldn't pass an array of attributes, since it's not obvious how that will be handled. But if they do, we'll handle them in the same way as the core voters (and of course, you can always not use the Abstract class if you need to customize this detail).

Copy link
Contributor

Choose a reason for hiding this comment

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

people shouldn't pass an array of attributes

Yeah I was surprised it was supported when I looked at this before. It's quite little effort to make this support multiple attributes if we did want to, but yeah whatever really.

Copy link
Member

Choose a reason for hiding this comment

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

Haha, yea I was surprised too once upon a time :). The problem will always be whether you make the voter support "AND" logic or "OR" logic for the multiple attributes. We can't do both, but we have to do one.

Copy link
Member

Choose a reason for hiding this comment

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

all core voters are implementing OR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weaverryan @stof so my implementation is good to go, right?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me as is: your code looks identical to RoleVoter.

}
}

return $vote;
}

/**
* Return an array of supported classes. This will be called by supportsClass
*
* @return array an array of supported classes, i.e. ['\Acme\DemoBundle\Model\Product']
*/
abstract protected function getSupportedClasses();

/**
* Return an array of supported attributes. This will be called by supportsAttribute
*
* @return array an array of supported attributes, i.e. ['CREATE', 'READ']
*/
abstract protected function getSupportedAttributes();

/**
* 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a string user doesn't necessary mean it is an anonymously authenticated user

Copy link
Member

Choose a reason for hiding this comment

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

That's technically true, but I've never seen a case in practice other than the anonymous user. I'd rather be a little technically incorrect here to cover the vast majority of the use-cases (and if you are authentication a user without an object, it's likely you are more advanced and understand what's going on).

*
* @param string $attribute
* @param object $object
* @param UserInterface|string $user
*
* @return bool
*/
abstract protected function isGranted($attribute, $object, $user = null);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

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

use Symfony\Component\Security\Core\Authorization\Voter\AbstractVoter;

/**
* @author Roman Marintšenko <inoryy@gmail.com>
*/
class AbstractVoterTest extends \PHPUnit_Framework_TestCase
{
/**
* @var AbstractVoter
*/
private $voter;

private $token;

public function setUp()
{
$this->voter = new VoterFixture();

$tokenMock = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
$tokenMock
->expects($this->any())
->method('getUser')
->will($this->returnValue('user'));

$this->token = $tokenMock;
}

/**
* @dataProvider getData
*/
public function testVote($expectedVote, $object, $attributes, $message)
{
$this->assertEquals($expectedVote, $this->voter->vote($this->token, $object, $attributes), $message);
}

public function getData()
{
return array(
array(AbstractVoter::ACCESS_ABSTAIN, null, array(), 'ACCESS_ABSTAIN for null objects'),
array(AbstractVoter::ACCESS_ABSTAIN, new UnsupportedObjectFixture(), array(), 'ACCESS_ABSTAIN for objects with unsupported class'),
array(AbstractVoter::ACCESS_ABSTAIN, new ObjectFixture(), array(), 'ACCESS_ABSTAIN for no attributes'),
array(AbstractVoter::ACCESS_ABSTAIN, new ObjectFixture(), array('foobar'), 'ACCESS_ABSTAIN for unsupported attributes'),
array(AbstractVoter::ACCESS_GRANTED, new ObjectFixture(), array('foo'), 'ACCESS_GRANTED if attribute grants access'),
array(AbstractVoter::ACCESS_GRANTED, new ObjectFixture(), array('bar', 'foo'), 'ACCESS_GRANTED if *at least one* attribute grants access'),
array(AbstractVoter::ACCESS_GRANTED, new ObjectFixture(), array('foobar', 'foo'), 'ACCESS_GRANTED if *at least one* attribute grants access'),
array(AbstractVoter::ACCESS_DENIED, new ObjectFixture(), array('bar', 'baz'), 'ACCESS_DENIED for if no attribute grants access'),
);
}
}

class VoterFixture 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';
}
}

class ObjectFixture
{
}

class UnsupportedObjectFixture
{
}