I love Symfony Voters. The Access Control List is, in the vast majority of cases, totally overkill for my security needs. Symfony’s Security Voters are a very quick and easy alternative.
Yesterday I got caught out by an edge case I had not been testing for.
A little background on this – I have been creating a RESTful API using Symfony 3, Behat 3, PHPSpec, and a few other interesting technologies. If you are interested in knowing a little more about this then be sure to check out the video tutorial series here on CodeReviewVideos where I have been documenting as I go.
The video tutorial series covers three different entities – Users (FOSUserBundle), Accounts, and Files (with FlySystem).
I want to ensure the currently logged in User has access to the resources they are requesting. I’ve been doing this with Voters.
It’s likely easier to follow along with these words if I give them more context. Here’s the code for the File Voter:
<?php
namespace spec\AppBundle\Security\Authorization\Voter;
use AppBundle\Model\AccountInterface;
use AppBundle\Model\FileInterface;
use AppBundle\Model\UserInterface;
use AppBundle\Security\Authorization\Voter\FileVoter;
use PhpSpec\ObjectBehavior;
use Prophecy\Argument;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
class FileVoterSpec extends ObjectBehavior
{
function it_is_initializable()
{
$this->shouldHaveType('AppBundle\Security\Authorization\Voter\FileVoter');
$this->shouldImplement('Symfony\Component\Security\Core\Authorization\Voter\VoterInterface');
}
function it_supports_expected_attributes()
{
$this->supportsAttribute('view')->shouldReturn(true);
}
function it_doesnt_support_unexpected_attributes()
{
$this->supportsAttribute('1')->shouldReturn(false);
}
function it_supports_expected_class(FileInterface $file)
{
$this->supportsClass($file)->shouldReturn(true);
}
function it_doesnt_support_unexpected_class()
{
$this->supportsClass(new \StdClass())->shouldReturn(false);
}
function it_abstains_if_doesnt_support_attribute(TokenInterface $token, FileInterface $file)
{
$this->vote($token, $file, ['unsupported'])->shouldReturn(VoterInterface::ACCESS_ABSTAIN);
}
function it_abstains_if_doesnt_support_this_class(TokenInterface $token)
{
$this->vote($token, new \StdClass(), [FileVoter::VIEW])->shouldReturn(VoterInterface::ACCESS_ABSTAIN);
}
function it_denies_if_not_matching(
TokenInterface $token,
UserInterface $user,
FileInterface $file
)
{
$token->getUser()->willReturn($user);
$file->belongsToUser($user)->willReturn(false);
$this->vote($token, $file, [FileVoter::VIEW])
->shouldReturn(VoterInterface::ACCESS_DENIED);
}
function it_grants_if_matching(
TokenInterface $token,
UserInterface $user,
FileInterface $file
)
{
$token->getUser()->willReturn($user);
$file->belongsToUser($user)->willReturn(true);
$this->vote($token, $file, [FileVoter::VIEW])
->shouldReturn(VoterInterface::ACCESS_GRANTED);
}
}
And the implementation:
<?php
namespace AppBundle\Security\Authorization\Voter;
use AppBundle\Model\AccountInterface;
use AppBundle\Model\FileInterface;
use AppBundle\Model\UserInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
class FileVoter implements VoterInterface
{
const VIEW = 'view';
/**
* @param string $attribute
* @return bool
*/
public function supportsAttribute($attribute)
{
return in_array($attribute, array(
self::VIEW,
));
}
/**
* @param string $class
* @return bool
*/
public function supportsClass($class)
{
$supportedClass = 'AppBundle\Model\FileInterface';
return $supportedClass === $class || is_subclass_of($class, $supportedClass);
}
/**
* @param TokenInterface $token
* @param null|FileInterface $requestedFile
* @param array $attributes
* @return int
*/
public function vote(TokenInterface $token, $requestedFile, array $attributes)
{
// check if class of this object is supported by this voter
if (!$this->supportsClass(get_class($requestedFile))) {
return VoterInterface::ACCESS_ABSTAIN;
}
// set the attribute to check against
$attribute = $attributes[0];
// check if the given attribute is covered by this voter
if (!$this->supportsAttribute($attribute)) {
return VoterInterface::ACCESS_ABSTAIN;
}
// get current logged in user
$loggedInUser = $token->getUser(); /** @var $loggedInUser UserInterface */
if ($requestedFile->belongsToUser($loggedInUser)) {
return VoterInterface::ACCESS_GRANTED;
}
return VoterInterface::ACCESS_DENIED;
}
}
This all works and is passing.
However, there is an edge case I hadn’t considered.
What happens if $requestedFile->belongsToUser($loggedInUser) fails, or returns null? Well, bad times as it happens.
Why might it fail? Well, let’s look at that interface, and the real method implementation:
<?php
namespace AppBundle\Security\Authorization;
use AppBundle\Model\UserInterface;
/**
* Interface UserOwnableInterface
* @package AppBundle\Security\Authorization
*/
interface UserOwnableInterface
{
/**
* @param UserInterface $user
* @return bool
*/
public function belongsToUser(UserInterface $user);
}
And the implementation:
/**
* @param UserInterface $user
* @return bool
*/
public function belongsToUser(UserInterface $user)
{
return $this
->getAccounts()
->filter(function ($account) use ($user) { /** @var $account AccountInterface */
return $account->isManagedBy($user);
})
->count() > 0
;
}
Due to a mistake in my fixtures, the File wasn’t correctly being added to the Account I expected, so the call of getAccounts() was returning null.
In this case, I first fixed my fixtures as that situation should never really happen.
But what if it does? I don’t want to be throwing errors to the user. It’s easier to deny them access, log the fault (using monolog, and/or to ELK), and fix it in a slightly more controlled manner.
I added the following two specs to my FileVoterSpec test file:
function it_denies_if_belongsToUser_throws(
TokenInterface $token,
UserInterface $user,
FileInterface $file
)
{
$token->getUser()->willReturn($user);
$file->belongsToUser($user)->willThrow(new \Exception());
$this->vote($token, $file, [FileVoter::VIEW])
->shouldReturn(VoterInterface::ACCESS_DENIED);
}
function it_denies_if_belongsToUser_returns_null(
TokenInterface $token,
UserInterface $user,
FileInterface $file
)
{
$token->getUser()->willReturn($user);
$file->belongsToUser($user)->willReturn(null);
$this->vote($token, $file, [FileVoter::VIEW])
->shouldReturn(VoterInterface::ACCESS_DENIED);
}
And then changed up the implementation (uglify!):
/**
* @param TokenInterface $token
* @param null|FileInterface $requestedFile
* @param array $attributes
* @return int
*/
public function vote(TokenInterface $token, $requestedFile, array $attributes)
{
// check if class of this object is supported by this voter
if (!$this->supportsClass(get_class($requestedFile))) {
return VoterInterface::ACCESS_ABSTAIN;
}
// set the attribute to check against
$attribute = $attributes[0];
// check if the given attribute is covered by this voter
if (!$this->supportsAttribute($attribute)) {
return VoterInterface::ACCESS_ABSTAIN;
}
// get current logged in user
$loggedInUser = $token->getUser(); /** @var $loggedInUser UserInterface */
try {
if ($requestedFile->belongsToUser($loggedInUser)) {
return VoterInterface::ACCESS_GRANTED;
}
} catch (\Exception $e) {
return VoterInterface::ACCESS_DENIED;
}
return VoterInterface::ACCESS_DENIED;
}
Note the try/catch block. It’s definitely made the code uglier, but it ‘fixes’ a potential problem I hadn’t considered.
I say ‘fixes’ as this should never happen. But if I have learned anything over the last 15-odd years, it’s that those things that should never happen… they happen.
