optimized AclVoter, added unit test

src/Symfony/Bundle/FrameworkBundle/Resources/config/security_acl.xml

@@ -15,6 +15,7 @@
         <parameter key="security.acl.permission_granting_strategy.class">Symfony\Component\Security\Acl\Domain\PermissionGrantingStrategy</parameter>
         
         <parameter key="security.acl.voter.class">Symfony\Component\Security\Acl\Voter\AclVoter</parameter>
+        <parameter key="security.acl.voter.allow_if_object_identity_unavailable">true</parameter>
         <parameter key="security.acl.permission.map.class">Symfony\Component\Security\Acl\Permission\BasicPermissionMap</parameter>
         
         <parameter key="security.acl.object_identity_retrieval_strategy.class">Symfony\Component\Security\Acl\Domain\ObjectIdentityRetrievalStrategy</parameter>
@@ -70,6 +71,8 @@
             <argument type="service" id="security.acl.object_identity_retrieval_strategy" />
             <argument type="service" id="security.acl.security_identity_retrieval_strategy" />
             <argument type="service" id="security.acl.permission.map" />
+            <argument type="service" id="logger" on-invalid="null" />
+            <argument>%security.acl.voter.allow_if_object_identity_unavailable%</argument>
             <tag name="security.voter" />
         </service>
     </services>

src/Symfony/Component/Security/Acl/Domain/ObjectIdentity.php

@@ -58,10 +58,14 @@ class ObjectIdentity implements ObjectIdentityInterface
             throw new InvalidDomainObjectException('$domainObject must be an object.');
         }
 
-        if ($domainObject instanceof DomainObjectInterface) {
-            return new self($domainObject->getObjectIdentifier(), get_class($domainObject));
-        } else if (method_exists($domainObject, 'getId')) {
-            return new self($domainObject->getId(), get_class($domainObject));
+        try {
+            if ($domainObject instanceof DomainObjectInterface) {
+                return new self($domainObject->getObjectIdentifier(), get_class($domainObject));
+            } else if (method_exists($domainObject, 'getId')) {
+                return new self($domainObject->getId(), get_class($domainObject));
+            }
+        } catch (\InvalidArgumentException $invalid) {
+            throw new InvalidDomainObjectException($invalid->getMessage(), 0, $invalid);
         }
 
         throw new InvalidDomainObjectException('$domainObject must either implement the DomainObjectInterface, or have a method named "getId".');

src/Symfony/Component/Security/Acl/Voter/AclVoter.php

@@ -2,6 +2,7 @@
 
 namespace Symfony\Component\Security\Acl\Voter;
 
+use Symfony\Component\HttpKernel\Log\LoggerInterface;
 use Symfony\Component\Security\Acl\Domain\ObjectIdentity;
 use Symfony\Component\Security\Acl\Domain\RoleSecurityIdentity;
 use Symfony\Component\Security\Acl\Domain\UserSecurityIdentity;
@@ -35,13 +36,17 @@ class AclVoter implements VoterInterface
     protected $permissionMap;
     protected $objectIdentityRetrievalStrategy;
     protected $securityIdentityRetrievalStrategy;
+    protected $allowIfObjectIdentityUnavailable;
+    protected $logger;
 
-    public function __construct(AclProviderInterface $aclProvider, ObjectIdentityRetrievalStrategyInterface $oidRetrievalStrategy, SecurityIdentityRetrievalStrategyInterface $sidRetrievalStrategy, PermissionMapInterface $permissionMap)
+    public function __construct(AclProviderInterface $aclProvider, ObjectIdentityRetrievalStrategyInterface $oidRetrievalStrategy, SecurityIdentityRetrievalStrategyInterface $sidRetrievalStrategy, PermissionMapInterface $permissionMap, LoggerInterface $logger = null, $allowIfObjectIdentityUnavailable = true)
     {
         $this->aclProvider = $aclProvider;
         $this->permissionMap = $permissionMap;
         $this->objectIdentityRetrievalStrategy = $oidRetrievalStrategy;
         $this->securityIdentityRetrievalStrategy = $sidRetrievalStrategy;
+        $this->logger = $logger;
+        $this->allowIfObjectIdentityUnavailable = $allowIfObjectIdentityUnavailable;
     }
 
     public function supportsAttribute($attribute)
@@ -51,44 +56,78 @@ class AclVoter implements VoterInterface
 
     public function vote(TokenInterface $token, $object, array $attributes)
     {
-        if (null === $object) {
-            return self::ACCESS_ABSTAIN;
-        } else if ($object instanceof FieldVote) {
-            $field = $object->getField();
-            $object = $object->getDomainObject();
-        } else {
-            $field = null;
-        }
-
-        if (null === $oid = $this->objectIdentityRetrievalStrategy->getObjectIdentity($object)) {
-            return self::ACCESS_ABSTAIN;
-        }
-        $sids = $this->securityIdentityRetrievalStrategy->getSecurityIdentities($token);
-
+        $firstCall = true;
         foreach ($attributes as $attribute) {
             if (!$this->supportsAttribute($attribute)) {
                 continue;
             }
 
-            try {
-                $acl = $this->aclProvider->findAcl($oid, $sids);
-            } catch (AclNotFoundException $noAcl) {
-                return self::ACCESS_DENIED;
+            if ($firstCall) {
+                $firstCall = false;
+
+                if (null === $object) {
+                    if (null !== $this->logger) {
+                        $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
+                    }
+
+                    return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
+                } else if ($object instanceof FieldVote) {
+                    $field = $object->getField();
+                    $object = $object->getDomainObject();
+                } else {
+                    $field = null;
+                }
+
+                if (null === $oid = $this->objectIdentityRetrievalStrategy->getObjectIdentity($object)) {
+                    if (null !== $this->logger) {
+                        $this->logger->debug(sprintf('Object identity unavailable. Voting to %s', $this->allowIfObjectIdentityUnavailable? 'grant access' : 'abstain'));
+                    }
+
+                    return $this->allowIfObjectIdentityUnavailable ? self::ACCESS_GRANTED : self::ACCESS_ABSTAIN;
+                }
+                $sids = $this->securityIdentityRetrievalStrategy->getSecurityIdentities($token);
+
+                try {
+                    $acl = $this->aclProvider->findAcl($oid, $sids);
+                } catch (AclNotFoundException $noAcl) {
+                    if (null !== $this->logger) {
+                        $this->logger->debug('No ACL found for the object identity. Voting to deny access.');
+                    }
+
+                    return self::ACCESS_DENIED;
+                }
             }
 
             try {
                 if (null === $field && $acl->isGranted($this->permissionMap->getMasks($attribute), $sids, false)) {
+                    if (null !== $this->logger) {
+                        $this->logger->debug('ACL found, permission granted. Voting to grant access');
+                    }
+
                     return self::ACCESS_GRANTED;
                 } else if (null !== $field && $acl->isFieldGranted($field, $this->permissionMap->getMasks($attribute), $sids, false)) {
+                    if (null !== $this->logger) {
+                        $this->logger->debug('ACL found, permission granted. Voting to grant access');
+                    }
+
                     return self::ACCESS_GRANTED;
                 } else {
+                    if (null !== $this->logger) {
+                        $this->logger->debug('ACL found, insufficient permissions. Voting to deny access.');
+                    }
+
                     return self::ACCESS_DENIED;
                 }
             } catch (NoAceFoundException $noAce) {
+                if (null !== $this->logger) {
+                    $this->logger->debug('ACL found, no ACE applicable. Voting to deny access.');
+                }
+
                 return self::ACCESS_DENIED;
             }
         }
 
+        // no attribute was supported
         return self::ACCESS_ABSTAIN;
     }
 

tests/Symfony/Tests/Component/Security/Acl/Voter/AclVoterTest.php

@@ -0,0 +1,375 @@
+<?php
+
+namespace Symfony\Tests\Component\Security\Acl\Voter;
+
+use Symfony\Component\Security\Acl\Exception\NoAceFoundException;
+use Symfony\Component\Security\Acl\Voter\FieldVote;
+use Symfony\Component\Security\Acl\Exception\AclNotFoundException;
+use Symfony\Component\Security\Acl\Domain\RoleSecurityIdentity;
+use Symfony\Component\Security\Acl\Domain\UserSecurityIdentity;
+use Symfony\Component\Security\Acl\Domain\ObjectIdentity;
+use Symfony\Component\Security\Authorization\Voter\VoterInterface;
+use Symfony\Component\Security\Acl\Voter\AclVoter;
+
+class AclVoterTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @dataProvider getSupportsAttributeTests
+     */
+    public function testSupportsAttribute($attribute, $supported)
+    {
+        list($voter,, $permissionMap,,) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->with($this->identicalTo($attribute))
+            ->will($this->returnValue($supported))
+        ;
+
+        $this->assertSame($supported, $voter->supportsAttribute($attribute));
+    }
+
+    public function getSupportsAttributeTests()
+    {
+        return array(
+            array('foo', true),
+            array('foo', false),
+        );
+    }
+
+    /**
+     * @dataProvider getSupportsClassTests
+     */
+    public function testSupportsClass($class)
+    {
+        list($voter,,,,) = $this->getVoter();
+
+        $this->assertTrue($voter->supportsClass($class));
+    }
+
+    public function getSupportsClassTests()
+    {
+        return array(
+            array('foo'),
+            array('bar'),
+            array('moo'),
+        );
+    }
+
+    public function testVote()
+    {
+        list($voter,, $permissionMap,,) = $this->getVoter();
+        $permissionMap
+            ->expects($this->atLeastOnce())
+            ->method('contains')
+            ->will($this->returnValue(false))
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $voter->vote($this->getToken(), null, array('VIEW', 'EDIT', 'DELETE')));
+    }
+
+    /**
+     * @dataProvider getTrueFalseTests
+     */
+    public function testVoteWhenNoObjectIsPassed($allowIfObjectIdentityUnavailable)
+    {
+        list($voter,, $permissionMap,,) = $this->getVoter($allowIfObjectIdentityUnavailable);
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+
+        if ($allowIfObjectIdentityUnavailable) {
+            $vote = VoterInterface::ACCESS_GRANTED;
+        } else {
+            $vote = VoterInterface::ACCESS_ABSTAIN;
+        }
+
+        $this->assertSame($vote, $voter->vote($this->getToken(), null, array('VIEW')));
+    }
+
+    /**
+     * @dataProvider getTrueFalseTests
+     */
+    public function testVoteWhenOidStrategyReturnsNull($allowIfUnavailable)
+    {
+        list($voter,, $permissionMap, $oidStrategy,) = $this->getVoter($allowIfUnavailable);
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue(null))
+        ;
+
+        if ($allowIfUnavailable) {
+            $vote = VoterInterface::ACCESS_GRANTED;
+        } else {
+            $vote = VoterInterface::ACCESS_ABSTAIN;
+        }
+
+        $this->assertSame($vote, $voter->vote($this->getToken(), new \stdClass(), array('VIEW')));
+    }
+
+    public function getTrueFalseTests()
+    {
+        return array(array(true), array(false));
+    }
+
+    public function testVoteNoAclFound()
+    {
+        list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue($oid = new ObjectIdentity('1', 'Foo')))
+        ;
+
+        $sidStrategy
+            ->expects($this->once())
+            ->method('getSecurityIdentities')
+            ->will($this->returnValue($sids = array(new UserSecurityIdentity('johannes', 'Foo'), new RoleSecurityIdentity('ROLE_FOO'))))
+        ;
+
+        $provider
+            ->expects($this->once())
+            ->method('findAcl')
+            ->with($this->equalTo($oid), $this->equalTo($sids))
+            ->will($this->throwException(new AclNotFoundException('Not found.')))
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $voter->vote($this->getToken(), new \stdClass(), array('VIEW')));
+    }
+
+    /**
+     * @dataProvider getTrueFalseTests
+     */
+    public function testVoteGrantsAccess($grant)
+    {
+        list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+        $permissionMap
+            ->expects($this->once())
+            ->method('getMasks')
+            ->with($this->equalTo('VIEW'))
+            ->will($this->returnValue($masks = array(1, 2, 3)))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue($oid = new ObjectIdentity('1', 'Foo')))
+        ;
+
+        $sidStrategy
+            ->expects($this->once())
+            ->method('getSecurityIdentities')
+            ->will($this->returnValue($sids = array(new UserSecurityIdentity('johannes', 'Foo'), new RoleSecurityIdentity('ROLE_FOO'))))
+        ;
+
+        $provider
+            ->expects($this->once())
+            ->method('findAcl')
+            ->with($this->equalTo($oid), $this->equalTo($sids))
+            ->will($this->returnValue($acl = $this->getMock('Symfony\Component\Security\Acl\Model\AclInterface')))
+        ;
+
+        $acl
+            ->expects($this->once())
+            ->method('isGranted')
+            ->with($this->identicalTo($masks), $this->equalTo($sids), $this->isFalse())
+            ->will($this->returnValue($grant))
+        ;
+
+        if ($grant) {
+            $vote = VoterInterface::ACCESS_GRANTED;
+        } else {
+            $vote = VoterInterface::ACCESS_DENIED;
+        }
+
+        $this->assertSame($vote, $voter->vote($this->getToken(), new \stdClass(), array('VIEW')));
+    }
+
+    public function testVoteNoAceFound()
+    {
+        list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+        $permissionMap
+            ->expects($this->once())
+            ->method('getMasks')
+            ->with($this->equalTo('VIEW'))
+            ->will($this->returnValue($masks = array(1, 2, 3)))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue($oid = new ObjectIdentity('1', 'Foo')))
+        ;
+
+        $sidStrategy
+            ->expects($this->once())
+            ->method('getSecurityIdentities')
+            ->will($this->returnValue($sids = array(new UserSecurityIdentity('johannes', 'Foo'), new RoleSecurityIdentity('ROLE_FOO'))))
+        ;
+
+        $provider
+            ->expects($this->once())
+            ->method('findAcl')
+            ->with($this->equalTo($oid), $this->equalTo($sids))
+            ->will($this->returnValue($acl = $this->getMock('Symfony\Component\Security\Acl\Model\AclInterface')))
+        ;
+
+        $acl
+            ->expects($this->once())
+            ->method('isGranted')
+            ->with($this->identicalTo($masks), $this->equalTo($sids), $this->isFalse())
+            ->will($this->throwException(new NoAceFoundException('No ACE')))
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $voter->vote($this->getToken(), new \stdClass(), array('VIEW')));
+    }
+
+    /**
+     * @dataProvider getTrueFalseTests
+     */
+    public function testVoteGrantsFieldAccess($grant)
+    {
+        list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+        $permissionMap
+            ->expects($this->once())
+            ->method('getMasks')
+            ->with($this->equalTo('VIEW'))
+            ->will($this->returnValue($masks = array(1, 2, 3)))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue($oid = new ObjectIdentity('1', 'Foo')))
+        ;
+
+        $sidStrategy
+            ->expects($this->once())
+            ->method('getSecurityIdentities')
+            ->will($this->returnValue($sids = array(new UserSecurityIdentity('johannes', 'Foo'), new RoleSecurityIdentity('ROLE_FOO'))))
+        ;
+
+        $provider
+            ->expects($this->once())
+            ->method('findAcl')
+            ->with($this->equalTo($oid), $this->equalTo($sids))
+            ->will($this->returnValue($acl = $this->getMock('Symfony\Component\Security\Acl\Model\AclInterface')))
+        ;
+
+        $acl
+            ->expects($this->once())
+            ->method('isFieldGranted')
+            ->with($this->identicalTo('foo'), $this->identicalTo($masks), $this->equalTo($sids), $this->isFalse())
+            ->will($this->returnValue($grant))
+        ;
+
+        if ($grant) {
+            $vote = VoterInterface::ACCESS_GRANTED;
+        } else {
+            $vote = VoterInterface::ACCESS_DENIED;
+        }
+
+        $this->assertSame($vote, $voter->vote($this->getToken(), new FieldVote(new \stdClass(), 'foo'), array('VIEW')));
+    }
+
+    public function testVoteNoFieldAceFound()
+    {
+        list($voter, $provider, $permissionMap, $oidStrategy, $sidStrategy) = $this->getVoter();
+
+        $permissionMap
+            ->expects($this->once())
+            ->method('contains')
+            ->will($this->returnValue(true))
+        ;
+        $permissionMap
+            ->expects($this->once())
+            ->method('getMasks')
+            ->with($this->equalTo('VIEW'))
+            ->will($this->returnValue($masks = array(1, 2, 3)))
+        ;
+
+        $oidStrategy
+            ->expects($this->once())
+            ->method('getObjectIdentity')
+            ->will($this->returnValue($oid = new ObjectIdentity('1', 'Foo')))
+        ;
+
+        $sidStrategy
+            ->expects($this->once())
+            ->method('getSecurityIdentities')
+            ->will($this->returnValue($sids = array(new UserSecurityIdentity('johannes', 'Foo'), new RoleSecurityIdentity('ROLE_FOO'))))
+        ;
+
+        $provider
+            ->expects($this->once())
+            ->method('findAcl')
+            ->with($this->equalTo($oid), $this->equalTo($sids))
+            ->will($this->returnValue($acl = $this->getMock('Symfony\Component\Security\Acl\Model\AclInterface')))
+        ;
+
+        $acl
+            ->expects($this->once())
+            ->method('isFieldGranted')
+            ->with($this->identicalTo('foo'), $this->identicalTo($masks), $this->equalTo($sids), $this->isFalse())
+            ->will($this->throwException(new NoAceFoundException('No ACE')))
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $voter->vote($this->getToken(), new FieldVote(new \stdClass(), 'foo'), array('VIEW')));
+    }
+
+    protected function getToken()
+    {
+        return $this->getMock('Symfony\Component\Security\Authentication\Token\TokenInterface');
+    }
+
+    protected function getVoter($allowIfObjectIdentityUnavailable = true)
+    {
+        $provider = $this->getMock('Symfony\Component\Security\Acl\Model\AclProviderInterface');
+        $permissionMap = $this->getMock('Symfony\Component\Security\Acl\Permission\PermissionMapInterface');
+        $oidStrategy = $this->getMock('Symfony\Component\Security\Acl\Model\ObjectIdentityRetrievalStrategyInterface');
+        $sidStrategy = $this->getMock('Symfony\Component\Security\Acl\Model\SecurityIdentityRetrievalStrategyInterface');
+
+        return array(
+            new AclVoter($provider, $oidStrategy, $sidStrategy, $permissionMap, null, $allowIfObjectIdentityUnavailable),
+            $provider,
+            $permissionMap,
+            $oidStrategy,
+            $sidStrategy,
+        );
+    }
+}