-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[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 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…custom voter
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
<?php | ||
|
||
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 voteOnAttribute | ||
* This method terminates as soon as it is able to return either ACCESS_GRANTED or ACCESS_DENIED vote | ||
* 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 (!$this->supportsClass(get_class($object))) { | ||
return VoterInterface::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. 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. |
||
|
||
$user = $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. As |
||
|
||
foreach ($attributes as $attribute) { | ||
if ($this->supportsAttribute($attribute)) { | ||
$vote = $this->voteOnAttribute($attribute, $object, $user); | ||
if (VoterInterface::ACCESS_ABSTAIN !== $vote) { | ||
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. this implementation is not consistent with the way core voters work. If you pass 2 attributes (which is not common anyway), they will grant the access as soon as one of them is accepted. Your implementation however gives more power to the first attribute by allowing it to reject everything 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! will try to replicate core voters behavior |
||
return $vote; | ||
} | ||
} | ||
} | ||
|
||
return VoterInterface::ACCESS_ABSTAIN; | ||
} | ||
|
||
/** | ||
9E1E | * 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 vote operation on a given attribute, object and (optionally) user | ||
* It is safe to assume that $attribute and $object's class pass supportsAttribute/supportsClass | ||
* | ||
* @param string $attribute | ||
* @param object $object | ||
* @param UserInterface $user | ||
* | ||
* @return int either ACCESS_GRANTED, ACCESS_ABSTAIN, or ACCESS_DENIED | ||
*/ | ||
abstract protected function voteOnAttribute($attribute, $object, UserInterface $user = null); | ||
} | ||
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. missing newline at end of file |
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.