From d3b68f8f8fac030b41534ab5b36c1fbe22f5431d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 13 Sep 2023 10:08:44 +0200 Subject: [PATCH] AuthorizationHelper: compare center and scope based on id, not on equality For an unknown reason, in some circumstances, the use of the `===` comparator does not work when comparing Center instances and Scope instances. Then, we compare them based on the id. --- .../Authorization/AuthorizationHelper.php | 19 ++++++--- .../Test/PrepareCenterTrait.php | 24 +++++------ .../Authorization/AuthorizationHelperTest.php | 41 ++++++++++++++----- 3 files changed, 53 insertions(+), 31 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php index 1980a524a..574d98b90 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php @@ -33,7 +33,13 @@ use function get_class; */ class AuthorizationHelper implements AuthorizationHelperInterface { - public function __construct(private readonly CenterResolverManagerInterface $centerResolverManager, private readonly LoggerInterface $logger, private readonly ScopeResolverDispatcher $scopeResolverDispatcher, private readonly UserACLAwareRepositoryInterface $userACLAwareRepository, private readonly ParentRoleHelper $parentRoleHelper) {} + public function __construct( + private readonly CenterResolverManagerInterface $centerResolverManager, + private readonly LoggerInterface $logger, + private readonly ScopeResolverDispatcher $scopeResolverDispatcher, + private readonly UserACLAwareRepositoryInterface $userACLAwareRepository, + private readonly ParentRoleHelper $parentRoleHelper + ) {} /** * Filter an array of centers, return only center which are reachable. @@ -233,7 +239,7 @@ class AuthorizationHelper implements AuthorizationHelperInterface return $this->parentRoleHelper->isRoleReached($childRole, $parentRole); } - private function userHasAccessForCenter(User $user, Center $center, $entity, $attribute): bool + private function userHasAccessForCenter(User $user, Center $center, mixed $entity, $attribute): bool { if (!$this->userCanReachCenter($user, $center)) { $this->logger->debug('user cannot reach center of entity', [ @@ -243,10 +249,11 @@ class AuthorizationHelper implements AuthorizationHelperInterface return false; } - foreach ($user->getGroupCenters() as $groupCenter) { - //filter on center - if ($groupCenter->getCenter() === $center) { + // filter on center + // in some case, the center can be the same, but have different object hashes, + // we cannot compare the objects: we must compare the ids here + if ($groupCenter->getCenter()->getId() === $center->getId()) { $permissionGroup = $groupCenter->getPermissionsGroup(); //iterate on roleScopes foreach ($permissionGroup->getRoleScopes() as $roleScope) { @@ -263,7 +270,7 @@ class AuthorizationHelper implements AuthorizationHelperInterface if (is_iterable($scope)) { foreach ($scope as $s) { - if ($roleScope->getScope() === $s) { + if ($roleScope->getScope()->getId() === $s->getId()) { return true; } } diff --git a/src/Bundle/ChillMainBundle/Test/PrepareCenterTrait.php b/src/Bundle/ChillMainBundle/Test/PrepareCenterTrait.php index e43cc8fdb..00045e889 100644 --- a/src/Bundle/ChillMainBundle/Test/PrepareCenterTrait.php +++ b/src/Bundle/ChillMainBundle/Test/PrepareCenterTrait.php @@ -11,6 +11,8 @@ declare(strict_types=1); namespace Chill\MainBundle\Test; +use Chill\MainBundle\Entity\Center; + /** * A trait to prepare center. * @@ -25,23 +27,17 @@ trait PrepareCenterTrait /** * prepare a mocked center, with and id and name given. - * - * @param int $id - * @param string $name - * - * @return \Chill\MainBundle\Entity\Center */ - protected function prepareCenter($id, $name) + protected function prepareCenter(int $id, string $name): Center { - if (null === $this->centerProphet) { - $this->centerProphet = new \Prophecy\Prophet(); - } + $center = new Center(); + $center->setName($name); - $center = $this->centerProphet->prophesize(); - $center->willExtend('\\' . \Chill\MainBundle\Entity\Center::class); - $center->getId()->willReturn($id); - $center->getName()->willReturn($name); + $reflectionClass = new \ReflectionClass($center); + $reflectionId = $reflectionClass->getProperty('id'); + $reflectionId->setAccessible(true); + $reflectionId->setValue($center, $id); - return $center->reveal(); + return $center; } } diff --git a/src/Bundle/ChillMainBundle/Tests/Security/Authorization/AuthorizationHelperTest.php b/src/Bundle/ChillMainBundle/Tests/Security/Authorization/AuthorizationHelperTest.php index 8d81012a7..5f6bf09c8 100644 --- a/src/Bundle/ChillMainBundle/Tests/Security/Authorization/AuthorizationHelperTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Security/Authorization/AuthorizationHelperTest.php @@ -47,7 +47,8 @@ final class AuthorizationHelperTest extends KernelTestCase public function dataProvider_getReachableCenters() { - $this->setUp(); + self::bootKernel(); + $centerA = $this->prepareCenter(1, 'center A'); $centerB = $this->prepareCenter(2, 'center B'); $scopeA = $this->prepareScope(1, 'scope default'); @@ -311,9 +312,9 @@ final class AuthorizationHelperTest extends KernelTestCase public function testUserHasAccessEntityMultiScope() { $centerA = $this->prepareCenter(1, 'center'); - $centerB = $this->prepareCenter(1, 'centerB'); - $scopeA = $this->prepareScope(2, 'other'); //the user will be granted this scope - $scopeB = $this->prepareScope(2, 'other'); //the user will be granted this scope + $centerB = $this->prepareCenter(2, 'centerB'); + $scopeA = $this->prepareScope(3, 'other'); //the user will be granted this scope + $scopeB = $this->prepareScope(4, 'other'); //the user will be granted this scope $user = $this->prepareUser([ [ 'center' => $centerA, 'permissionsGroup' => [ @@ -331,6 +332,24 @@ final class AuthorizationHelperTest extends KernelTestCase $this->assertTrue($helper->userHasAccess($user, $entity->reveal(), 'CHILL_ROLE')); } + public function testUserHasAccessEntityIsCenter() + { + $centerA = $this->prepareCenter(1, 'center'); + $centerB = $this->prepareCenter(2, 'centerB'); + $user = $this->prepareUser([ + [ + 'center' => $centerA, 'permissionsGroup' => [ + ['scope' => null, 'role' => 'CHILL_ROLE'], + ], + ], + ]); + + $helper = $this->getAuthorizationHelper(); + + $this->assertFalse($helper->userHasAccess($user, $centerB, 'CHILL_ROLE')); + $this->assertTrue($helper->userHasAccess($user, $centerA, 'CHILL_ROLE')); + } + public function testUserHasAccessMultiCenterEntityWithoutScope() { $center = $this->prepareCenter(1, 'center'); @@ -515,10 +534,10 @@ final class AuthorizationHelperTest extends KernelTestCase public function testUserHasNoAccessEntityMultiScope() { $centerA = $this->prepareCenter(1, 'center'); - $centerB = $this->prepareCenter(1, 'centerB'); - $scopeA = $this->prepareScope(2, 'other'); //the user will be granted this scope - $scopeB = $this->prepareScope(2, 'other'); //the user will be granted this scope - $scopeC = $this->prepareScope(2, 'other'); //the user will be granted this scope + $centerB = $this->prepareCenter(2, 'centerB'); + $scopeA = $this->prepareScope(3, 'other'); //the user will be granted this scope + $scopeB = $this->prepareScope(4, 'other'); //the user will be granted this scope + $scopeC = $this->prepareScope(5, 'other'); //the user will be granted this scope $user = $this->prepareUser([ [ 'center' => $centerA, 'permissionsGroup' => [ @@ -539,9 +558,9 @@ final class AuthorizationHelperTest extends KernelTestCase public function testUserHasNoAccessMultiCenterEntityWithoutScope() { $center = $this->prepareCenter(1, 'center'); - $centerB = $this->prepareCenter(1, 'centerB'); - $centerC = $this->prepareCenter(1, 'centerC'); - $scopeB = $this->prepareScope(2, 'other'); //the user will be granted this scope + $centerB = $this->prepareCenter(2, 'centerB'); + $centerC = $this->prepareCenter(3, 'centerC'); + $scopeB = $this->prepareScope(4, 'other'); //the user will be granted this scope $user = $this->prepareUser([ [ 'center' => $center, 'permissionsGroup' => [