-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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()); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function supportsClass($class) | ||
{ | ||
foreach ($this->getSupportedClasses() as $supportedClass) { | ||
if ($supportedClass === $class || is_subclass_of($class, $supportedClass)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. http://php.net/is_a is easier IMHO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But you don't have to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. supportsClass expects a string in the VoterInterface There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just ran few tests:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See the php doc: allow_string
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in 5.3.3 to 5.3.6, There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why returning Take the role checking as example: At first glance, I would do something like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm conflicted on this. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would move this line below the next There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The benefit is to avoid needless assignment if the next There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer increased readability over nano-optimization There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, the flow here is identical to the existing |
||
|
||
if ($this->isGranted($attribute, $object, $token->getUser())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the benefit of that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the result of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing prevents a user from creating a custom token, like: FooToken implements TokenInterface
{
...
public function getUser()
{
// do a remote call here
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Thanks guys! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you shouldn't call There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// grant access as soon as at least one voter returns a positive response | ||
return self::ACCESS_GRANTED; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 May be better to negate the if and return as soon as a denied is returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all core voters are implementing OR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weaverryan @stof so my implementation is good to go, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
{ | ||
} |
There was a problem hiding this comment.
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)
oris_granted('CREATE', foo)
.There was a problem hiding this comment.
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 strategyThere was a problem hiding this comment.
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..There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.