Skip to content

Commit 0b5044a

Browse files
mnapolimnapoli
committed
Fixes #12 Inherited authorizations are left behind when a resource is deleted
1 parent 5c89494 commit 0b5044a

File tree

6 files changed

+206
-19
lines changed

6 files changed

+206
-19
lines changed

‎composer.lock‎

Lines changed: 23 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/ACL.php‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,21 @@ public function processNewResource(EntityResource $resource)
156156
$repository->insertBulk($cascadedAuthorizations);
157157
}
158158

159+
/**
160+
* Process a resource that has been deleted.
161+
*
162+
* Called by the EntityResourcesListener.
163+
*
164+
* @param EntityResource $resource
165+
*/
166+
public function processDeletedResource(EntityResource $resource)
167+
{
168+
/** @var AuthorizationRepository $repository */
169+
$repository = $this->entityManager->getRepository('MyCLabs\ACL\Model\Authorization');
170+
171+
$repository->removeAuthorizationsForResource($resource);
172+
}
173+
159174
/**
160175
* Clears and rebuilds all the authorizations from the roles.
161176
*/

‎src/Doctrine/EntityResourcesListener.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,23 @@ public function getSubscribedEvents()
5252

5353
public function onFlush(OnFlushEventArgs $args)
5454
{
55+
$acl = $this->getACL();
5556
$uow = $args->getEntityManager()->getUnitOfWork();
5657

57-
// Remember new resources
58+
// Remember new resources for after flush (we need them to have an ID)
5859
$this->newResources = [];
5960
foreach ($uow->getScheduledEntityInsertions() as $entity) {
6061
if ($entity instanceof EntityResource) {
6162
$this->newResources[] = $entity;
6263
}
6364
}
65+
66+
// Process deleted resources
67+
foreach ($uow->getScheduledEntityDeletions() as $entity) {
68+
if ($entity instanceof EntityResource) {
69+
$acl->processDeletedResource($entity);
70+
}
71+
}
6472
}
6573

6674
public function postFlush()

‎src/Repository/AuthorizationRepository.php‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,4 +167,38 @@ public function findCascadableAuthorizationsForResource(ResourceInterface $resou
167167

168168
return $qb->getQuery()->getResult();
169169
}
170+
171+
/**
172+
* Remove all the authorizations that apply to the given resource.
173+
*
174+
* @param ResourceInterface $resource
175+
* @throws \RuntimeException If the resource is an entity, it must be persisted.
176+
*/
177+
public function removeAuthorizationsForResource(ResourceInterface $resource)
178+
{
179+
$qb = $this->_em->createQueryBuilder();
180+
181+
$qb->delete($this->getEntityName(), 'a');
182+
183+
if ($resource instanceof EntityResource) {
184+
if ($resource->getId() === null) {
185+
throw new \RuntimeException(sprintf(
186+
'The entity resource %s must be persisted (id not null) to be able to remove the authorizations',
187+
ClassUtils::getClass($resource)
188+
));
189+
}
190+
191+
$qb->andWhere('a.entityClass = :entityClass');
192+
$qb->andWhere('a.entityId = :entityId');
193+
$qb->setParameter('entityClass', ClassUtils::getClass($resource));
194+
$qb->setParameter('entityId', $resource->getId());
195+
}
196+
if ($resource instanceof ClassResource) {
197+
$qb->andWhere('a.entityClass = :entityClass');
198+
$qb->andWhere('a.entityId IS NULL');
199+
$qb->setParameter('entityClass', $resource->getClass());
200+
}
201+
202+
$qb->getQuery()->execute();
203+
}
170204
}

‎tests/Integration/ResourceDeletionTest.php‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
<?php
2+
3+
namespace Tests\MyCLabs\ACL\Integration;
4+
5+
use MyCLabs\ACL\Model\Actions;
6+
use Tests\MyCLabs\ACL\Integration\Model\Article;
7+
use Tests\MyCLabs\ACL\Integration\Model\ArticleEditorRole;
8+
use Tests\MyCLabs\ACL\Integration\Model\Category;
9+
use Tests\MyCLabs\ACL\Integration\Model\CategoryManagerRole;
10+
use Tests\MyCLabs\ACL\Integration\Model\User;
11+
12+
/**
13+
* Tests that authorizations are deleted when a resource is deleted.
14+
*
15+
* @coversNothing
16+
*/
17+
class ResourceDeletionTest extends AbstractIntegrationTest
18+
{
19+
public function testSimple()
20+
{
21+
$resource = new Article();
22+
$this->em->persist($resource);
23+
$user = new User();
24+
$this->em->persist($user);
25+
$this->em->flush();
26+
27+
// The role will create 1 authorization
28+
$this->acl->grant($user, new ArticleEditorRole($user, $resource));
29+
$this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $resource));
30+
31+
// We need to reload the resource because the role hasn't been added automatically to
32+
// the role collection in Article
33+
$this->em->refresh($resource);
34+
35+
// Now we delete the resource
36+
$this->em->remove($resource);
37+
$this->em->flush();
38+
39+
// We check that the authorization is deleted
40+
$query = $this->em->createQuery('SELECT COUNT(a.id) FROM MyCLabs\ACL\Model\Authorization a');
41+
$this->assertEquals(0, $query->getSingleScalarResult(), "The authorization wasn't deleted");
42+
43+
// We check that the role is deleted too
44+
$query = $this->em->createQuery('SELECT COUNT(r.id) FROM Tests\MyCLabs\ACL\Integration\Model\ArticleEditorRole r');
45+
$this->assertEquals(0, $query->getSingleScalarResult(), "The role wasn't deleted");
46+
}
47+
48+
/**
49+
* Here we delete a resource which had no direct roles associated. However it had authorizations
50+
* because of a parent resource.
51+
*
52+
* @link https://github.com/myclabs/ACL/issues/12
53+
*/
54+
public function testDeletionWithCascade()
55+
{
56+
$category = new Category();
57+
$this->em->persist($category);
58+
$subCategory = new Category($category);
59+
$this->em->persist($subCategory);
60+
$user = new User();
61+
$this->em->persist($user);
62+
$this->em->flush();
63+
64+
// We apply a role on the parent resource, authorizations will cascade to the sub-resource
65+
$this->acl->grant($user, new CategoryManagerRole($user, $category));
66+
$this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $category));
67+
$this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $subCategory));
68+
69+
// We need to reload the resource because the role hasn't been added automatically to
70+
// the role collection in Category
71+
$this->em->refresh($category);
72+
73+
// Now we delete the sub-resource
74+
$this->em->remove($subCategory);
75+
$this->em->flush();
76+
77+
// We check that the authorization is deleted (there should be 1 left: the one for the parent category)
78+
$query = $this->em->createQuery('SELECT COUNT(a.id) FROM MyCLabs\ACL\Model\Authorization a');
79+
$this->assertEquals(1, $query->getSingleScalarResult(), "The child authorization wasn't deleted");
80+
81+
// We check that the role is not deleted
82+
$query = $this->em->createQuery('SELECT COUNT(r.id) FROM Tests\MyCLabs\ACL\Integration\Model\CategoryManagerRole r');
83+
$this->assertEquals(1, $query->getSingleScalarResult());
84+
85+
// We check that isAllowed still works with the parent resource (which wasn't deleted)
86+
$this->assertTrue($this->acl->isAllowed($user, Actions::VIEW, $category));
87+
}
88+
}

‎tests/Unit/Repository/AuthorizationRepositoryTest.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,4 +194,41 @@ public function testIsAllowedOnEntityClass()
194194
$this->assertTrue($repository->isAllowedOnEntityClass($user, Actions::VIEW, $class));
195195
$this->assertFalse($repository->isAllowedOnEntityClass($user, Actions::EDIT, $class));
196196
}
197+
198+
/**
199+
* @depends testInsertBulk
200+
*/
201+
public function testRemoveForResource()
202+
{
203+
$user = new User();
204+
$this->em->persist($user);
205+
206+
$resource1 = new File();
207+
$this->em->persist($resource1);
208+
$role1 = new FileOwnerRole($user, $resource1);
209+
$this->em->persist($role1);
210+
$this->em->flush();
211+
212+
$resource2 = new File();
213+
$this->em->persist($resource2);
214+
$role2 = new FileOwnerRole($user, $resource2);
215+
$this->em->persist($role2);
216+
$this->em->flush();
217+
218+
$authorizations = [
219+
Authorization::create($role1, new Actions([ Actions::VIEW ]), $resource1),
220+
Authorization::create($role2, new Actions([ Actions::VIEW ]), $resource2),
221+
];
222+
223+
/** @var AuthorizationRepository $repository */
224+
$repository = $this->em->getRepository('MyCLabs\ACL\Model\Authorization');
225+
$repository->insertBulk($authorizations);
226+
227+
// We remove the authorizations for the resource 1
228+
$repository->removeAuthorizationsForResource($resource1);
229+
// We check that they were removed
230+
$this->assertFalse($repository->isAllowedOnEntity($user, Actions::VIEW, $resource1));
231+
// and that authorizations for the resource 2 weren't removed
232+
$this->assertTrue($repository->isAllowedOnEntity($user, Actions::VIEW, $resource2));
233+
}
197234
}

0 commit comments

Comments
 (0)