From 0b5044ae9820b9dcb7d3cf9b9cd564de66eb13de Mon Sep 17 00:00:00 2001 From: Matthieu Napoli Date: Thu, 19 Jun 2014 16:21:26 +0200 Subject: [PATCH] Fixes #12 Inherited authorizations are left behind when a resource is deleted --- composer.lock | 41 +++++---- src/ACL.php | 15 ++++ src/Doctrine/EntityResourcesListener.php | 10 ++- src/Repository/AuthorizationRepository.php | 34 +++++++ tests/Integration/ResourceDeletionTest.php | 88 +++++++++++++++++++ .../AuthorizationRepositoryTest.php | 37 ++++++++ 6 files changed, 206 insertions(+), 19 deletions(-) create mode 100644 tests/Integration/ResourceDeletionTest.php diff --git a/composer.lock b/composer.lock index 7c3678b..06e9ec7 100644 --- a/composer.lock +++ b/composer.lock @@ -3,7 +3,7 @@ "This file locks the dependencies of your project to a known state", "Read more about it at http://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file" ], - "hash": "54c39acd8b9391674b041e22cbd5999a", + "hash": "a8237facecdbe9164e20b5457d5655ef", "packages": [ { "name": "doctrine/annotations", @@ -221,16 +221,16 @@ }, { "name": "doctrine/common", - "version": "v2.4.1", + "version": "v2.4.2", "source": { "type": "git", "url": "https://github.com/doctrine/common.git", - "reference": "ceb18cf9b0230f3ea208b6238130fd415abda0a7" + "reference": "5db6ab40e4c531f14dad4ca96a394dfce5d4255b" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/doctrine/common/zipball/ceb18cf9b0230f3ea208b6238130fd415abda0a7", - "reference": "ceb18cf9b0230f3ea208b6238130fd415abda0a7", + "url": "https://api.github.com/repos/doctrine/common/zipball/5db6ab40e4c531f14dad4ca96a394dfce5d4255b", + "reference": "5db6ab40e4c531f14dad4ca96a394dfce5d4255b", "shasum": "" }, "require": { @@ -241,6 +241,9 @@ "doctrine/lexer": "1.*", "php": ">=5.3.2" }, + "require-dev": { + "phpunit/phpunit": "~3.7" + }, "type": "library", "extra": { "branch-alias": { @@ -292,7 +295,7 @@ "persistence", "spl" ], - "time": "2013-09-07 10:20:34" + "time": "2014-05-21 19:28:51" }, { "name": "doctrine/dbal", @@ -487,23 +490,23 @@ "source": { "type": "git", "url": "https://github.com/doctrine/doctrine2.git", - "reference": "8f688509c83f2176d0c3bb01f024ddf021d36c93" + "reference": "85c02e57b1eb328e68072650616805b9751bef00" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/doctrine/doctrine2/zipball/8f688509c83f2176d0c3bb01f024ddf021d36c93", - "reference": "8f688509c83f2176d0c3bb01f024ddf021d36c93", + "url": "https://api.github.com/repos/doctrine/doctrine2/zipball/85c02e57b1eb328e68072650616805b9751bef00", + "reference": "85c02e57b1eb328e68072650616805b9751bef00", "shasum": "" }, "require": { - "doctrine/collections": "~1.1", + "doctrine/collections": "~1.2", "doctrine/dbal": ">=2.5-dev,<2.6-dev", "ext-pdo": "*", "php": ">=5.3.2", "symfony/console": "2.*" }, "require-dev": { - "phpunit/phpunit": "~3.7", + "phpunit/phpunit": "~4.0", "satooshi/php-coveralls": "dev-master", "symfony/yaml": "~2.1" }, @@ -556,36 +559,38 @@ "database", "orm" ], - "time": "2014-03-28 13:05:20" + "time": "2014-06-19 12:33:58" }, { "name": "symfony/console", - "version": "v2.4.2", + "version": "v2.5.0", "target-dir": "Symfony/Component/Console", "source": { "type": "git", "url": "https://github.com/symfony/Console.git", - "reference": "940f217cbc3c8a33e5403e7c595495c4884400fe" + "reference": "ef4ca73b0b3a10cbac653d3ca482d0cdd4502b2c" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/symfony/Console/zipball/940f217cbc3c8a33e5403e7c595495c4884400fe", - "reference": "940f217cbc3c8a33e5403e7c595495c4884400fe", + "url": "https://api.github.com/repos/symfony/Console/zipball/ef4ca73b0b3a10cbac653d3ca482d0cdd4502b2c", + "reference": "ef4ca73b0b3a10cbac653d3ca482d0cdd4502b2c", "shasum": "" }, "require": { "php": ">=5.3.3" }, "require-dev": { + "psr/log": "~1.0", "symfony/event-dispatcher": "~2.1" }, "suggest": { + "psr/log": "For using the console logger", "symfony/event-dispatcher": "" }, "type": "library", "extra": { "branch-alias": { - "dev-master": "2.4-dev" + "dev-master": "2.5-dev" } }, "autoload": { @@ -611,7 +616,7 @@ ], "description": "Symfony Console Component", "homepage": "http://symfony.com", - "time": "2014-02-11 13:52:09" + "time": "2014-05-22 08:54:24" } ], "packages-dev": [ diff --git a/src/ACL.php b/src/ACL.php index 06fe943..406f403 100755 --- a/src/ACL.php +++ b/src/ACL.php @@ -156,6 +156,21 @@ public function processNewResource(EntityResource $resource) $repository->insertBulk($cascadedAuthorizations); } + /** + * Process a resource that has been deleted. + * + * Called by the EntityResourcesListener. + * + * @param EntityResource $resource + */ + public function processDeletedResource(EntityResource $resource) + { + /** @var AuthorizationRepository $repository */ + $repository = $this->entityManager->getRepository('MyCLabs\ACL\Model\Authorization'); + + $repository->removeAuthorizationsForResource($resource); + } + /** * Clears and rebuilds all the authorizations from the roles. */ diff --git a/src/Doctrine/EntityResourcesListener.php b/src/Doctrine/EntityResourcesListener.php index 3dd1b1d..6a4bcb3 100644 --- a/src/Doctrine/EntityResourcesListener.php +++ b/src/Doctrine/EntityResourcesListener.php @@ -52,15 +52,23 @@ public function getSubscribedEvents() public function onFlush(OnFlushEventArgs $args) { + $acl = $this->getACL(); $uow = $args->getEntityManager()->getUnitOfWork(); - // Remember new resources + // Remember new resources for after flush (we need them to have an ID) $this->newResources = []; foreach ($uow->getScheduledEntityInsertions() as $entity) { if ($entity instanceof EntityResource) { $this->newResources[] = $entity; } } + + // Process deleted resources + foreach ($uow->getScheduledEntityDeletions() as $entity) { + if ($entity instanceof EntityResource) { + $acl->processDeletedResource($entity); + } + } } public function postFlush() diff --git a/src/Repository/AuthorizationRepository.php b/src/Repository/AuthorizationRepository.php index e961c87..eaaa967 100644 --- a/src/Repository/AuthorizationRepository.php +++ b/src/Repository/AuthorizationRepository.php @@ -167,4 +167,38 @@ public function findCascadableAuthorizationsForResource(ResourceInterface $resou return $qb->getQuery()->getResult(); } + + /** + * Remove all the authorizations that apply to the given resource. + * + * @param ResourceInterface $resource + * @throws \RuntimeException If the resource is an entity, it must be persisted. + */ + public function removeAuthorizationsForResource(ResourceInterface $resource) + { + $qb = $this->_em->createQueryBuilder(); + + $qb->delete($this->getEntityName(), 'a'); + + if ($resource instanceof EntityResource) { + if ($resource->getId() === null) { + throw new \RuntimeException(sprintf( + 'The entity resource %s must be persisted (id not null) to be able to remove the authorizations', + ClassUtils::getClass($resource) + )); + } + + $qb->andWhere('a.entityClass = :entityClass'); + $qb->andWhere('a.entityId = :entityId'); + $qb->setParameter('entityClass', ClassUtils::getClass($resource)); + $qb->setParameter('entityId', $resource->getId()); + } + if ($resource instanceof ClassResource) { + $qb->andWhere('a.entityClass = :entityClass'); + $qb->andWhere('a.entityId IS NULL'); + $qb->setParameter('entityClass', $resource->getClass()); + } + + $qb->getQuery()->execute(); + } } diff --git a/tests/Integration/ResourceDeletionTest.php b/tests/Integration/ResourceDeletionTest.php new file mode 100644 index 0000000..ba711aa --- /dev/null +++ b/tests/Integration/ResourceDeletionTest.php @@ -0,0 +1,88 @@ +em->persist($resource); + $user = new User(); + $this->em->persist($user); + $this->em->flush(); + + // The role will create 1 authorization + $this->acl->grant($user, new ArticleEditorRole($user, $resource)); + $this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $resource)); + + // We need to reload the resource because the role hasn't been added automatically to + // the role collection in Article + $this->em->refresh($resource); + + // Now we delete the resource + $this->em->remove($resource); + $this->em->flush(); + + // We check that the authorization is deleted + $query = $this->em->createQuery('SELECT COUNT(a.id) FROM MyCLabs\ACL\Model\Authorization a'); + $this->assertEquals(0, $query->getSingleScalarResult(), "The authorization wasn't deleted"); + + // We check that the role is deleted too + $query = $this->em->createQuery('SELECT COUNT(r.id) FROM Tests\MyCLabs\ACL\Integration\Model\ArticleEditorRole r'); + $this->assertEquals(0, $query->getSingleScalarResult(), "The role wasn't deleted"); + } + + /** + * Here we delete a resource which had no direct roles associated. However it had authorizations + * because of a parent resource. + * + * @link https://github.com/myclabs/ACL/issues/12 + */ + public function testDeletionWithCascade() + { + $category = new Category(); + $this->em->persist($category); + $subCategory = new Category($category); + $this->em->persist($subCategory); + $user = new User(); + $this->em->persist($user); + $this->em->flush(); + + // We apply a role on the parent resource, authorizations will cascade to the sub-resource + $this->acl->grant($user, new CategoryManagerRole($user, $category)); + $this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $category)); + $this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $subCategory)); + + // We need to reload the resource because the role hasn't been added automatically to + // the role collection in Category + $this->em->refresh($category); + + // Now we delete the sub-resource + $this->em->remove($subCategory); + $this->em->flush(); + + // We check that the authorization is deleted (there should be 1 left: the one for the parent category) + $query = $this->em->createQuery('SELECT COUNT(a.id) FROM MyCLabs\ACL\Model\Authorization a'); + $this->assertEquals(1, $query->getSingleScalarResult(), "The child authorization wasn't deleted"); + + // We check that the role is not deleted + $query = $this->em->createQuery('SELECT COUNT(r.id) FROM Tests\MyCLabs\ACL\Integration\Model\CategoryManagerRole r'); + $this->assertEquals(1, $query->getSingleScalarResult()); + + // We check that isAllowed still works with the parent resource (which wasn't deleted) + $this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $category)); + } +} diff --git a/tests/Unit/Repository/AuthorizationRepositoryTest.php b/tests/Unit/Repository/AuthorizationRepositoryTest.php index bb9256e..a6c5164 100644 --- a/tests/Unit/Repository/AuthorizationRepositoryTest.php +++ b/tests/Unit/Repository/AuthorizationRepositoryTest.php @@ -194,4 +194,41 @@ public function testIsAllowedOnEntityClass() $this->assertTrue($repository->isAllowedOnEntityClass($user, Actions::VIEW, $class)); $this->assertFalse($repository->isAllowedOnEntityClass($user, Actions::EDIT, $class)); } + + /** + * @depends testInsertBulk + */ + public function testRemoveForResource() + { + $user = new User(); + $this->em->persist($user); + + $resource1 = new File(); + $this->em->persist($resource1); + $role1 = new FileOwnerRole($user, $resource1); + $this->em->persist($role1); + $this->em->flush(); + + $resource2 = new File(); + $this->em->persist($resource2); + $role2 = new FileOwnerRole($user, $resource2); + $this->em->persist($role2); + $this->em->flush(); + + $authorizations = [ + Authorization::create($role1, new Actions([ Actions::VIEW ]), $resource1), + Authorization::create($role2, new Actions([ Actions::VIEW ]), $resource2), + ]; + + /** @var AuthorizationRepository $repository */ + $repository = $this->em->getRepository('MyCLabs\ACL\Model\Authorization'); + $repository->insertBulk($authorizations); + + // We remove the authorizations for the resource 1 + $repository->removeAuthorizationsForResource($resource1); + // We check that they were removed + $this->assertFalse($repository->isAllowedOnEntity($user, Actions::VIEW, $resource1)); + // and that authorizations for the resource 2 weren't removed + $this->assertTrue($repository->isAllowedOnEntity($user, Actions::VIEW, $resource2)); + } }