From 87e16ec24f7457cac85c3e11a753e34e57b1647d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 3 Nov 2021 12:05:04 +0100 Subject: [PATCH 1/5] authorization helper: fix consructor --- .../Security/Authorization/AuthorizationHelper.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php index 8713d6ce3..0cfb6ed3c 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php @@ -72,7 +72,8 @@ class AuthorizationHelper implements AuthorizationHelperInterface EntityManagerInterface $em, CenterResolverDispatcher $centerResolverDispatcher, LoggerInterface $logger, - ScopeResolverDispatcher $scopeResolverDispatcher + ScopeResolverDispatcher $scopeResolverDispatcher, + UserACLAwareRepositoryInterface $userACLAwareRepository ) { $this->roleHierarchy = $roleHierarchy; $this->hierarchy = $parameterBag->get('security.role_hierarchy.roles'); @@ -80,6 +81,7 @@ class AuthorizationHelper implements AuthorizationHelperInterface $this->centerResolverDispatcher = $centerResolverDispatcher; $this->logger = $logger; $this->scopeResolverDispatcher = $scopeResolverDispatcher; + $this->userACLAwareRepository = $userACLAwareRepository; } /** From 6911ace412bf60c1ceb041962531a0d93e3cf245 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 3 Nov 2021 13:14:23 +0100 Subject: [PATCH 2/5] Refactor authorization helper to separate some methods Methods regarding to role hierarchi are now delegated to a parent role helper. --- .../Authorization/AuthorizationHelper.php | 63 +++++----------- .../Security/ParentRoleHelper.php | 71 +++++++++++++++++++ .../Authorization/ParentRoleHelperTest.php | 36 ++++++++++ .../config/services/security.yaml | 4 ++ 4 files changed, 129 insertions(+), 45 deletions(-) create mode 100644 src/Bundle/ChillMainBundle/Security/ParentRoleHelper.php create mode 100644 src/Bundle/ChillMainBundle/Tests/Authorization/ParentRoleHelperTest.php diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php index 0cfb6ed3c..f075b650f 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php @@ -25,6 +25,7 @@ use Chill\MainBundle\Entity\HasCenterInterface; use Chill\MainBundle\Entity\HasScopeInterface; use Chill\MainBundle\Repository\UserACLAwareRepository; use Chill\MainBundle\Repository\UserACLAwareRepositoryInterface; +use Chill\MainBundle\Security\ParentRoleHelper; use Chill\MainBundle\Security\Resolver\CenterResolverDispatcher; use Chill\MainBundle\Security\Resolver\ScopeResolverDispatcher; use Chill\MainBundle\Security\Resolver\ScopeResolverInterface; @@ -46,42 +47,28 @@ use Chill\MainBundle\Entity\RoleScope; */ class AuthorizationHelper implements AuthorizationHelperInterface { - protected RoleHierarchyInterface $roleHierarchy; + private CenterResolverDispatcher $centerResolverDispatcher; - /** - * The role in a hierarchy, given by the parameter - * `security.role_hierarchy.roles` from the container. - * - * @var string[] - */ - protected array $hierarchy; + private ScopeResolverDispatcher $scopeResolverDispatcher; - protected EntityManagerInterface $em; - - protected CenterResolverDispatcher $centerResolverDispatcher; - - protected ScopeResolverDispatcher $scopeResolverDispatcher; - - protected LoggerInterface $logger; + private LoggerInterface $logger; private UserACLAwareRepositoryInterface $userACLAwareRepository; + private ParentRoleHelper $parentRoleHelper; + public function __construct( - RoleHierarchyInterface $roleHierarchy, - ParameterBagInterface $parameterBag, - EntityManagerInterface $em, CenterResolverDispatcher $centerResolverDispatcher, LoggerInterface $logger, ScopeResolverDispatcher $scopeResolverDispatcher, - UserACLAwareRepositoryInterface $userACLAwareRepository + UserACLAwareRepositoryInterface $userACLAwareRepository, + ParentRoleHelper $parentRoleHelper ) { - $this->roleHierarchy = $roleHierarchy; - $this->hierarchy = $parameterBag->get('security.role_hierarchy.roles'); - $this->em = $em; $this->centerResolverDispatcher = $centerResolverDispatcher; $this->logger = $logger; $this->scopeResolverDispatcher = $scopeResolverDispatcher; $this->userACLAwareRepository = $userACLAwareRepository; + $this->parentRoleHelper = $parentRoleHelper; } /** @@ -93,7 +80,7 @@ class AuthorizationHelper implements AuthorizationHelperInterface * @param Center|Center[] $center May be an array of center * @return bool */ - public function userCanReachCenter(User $user, $center) + public function userCanReachCenter(User $user, $center): bool { if ($center instanceof \Traversable) { foreach ($center as $c) { @@ -340,39 +327,25 @@ class AuthorizationHelper implements AuthorizationHelperInterface /** * Test if a parent role may give access to a given child role * - * @param Role $childRole The role we want to test if he is reachable - * @param Role $parentRole The role which should give access to $childRole + * @param string $childRole The role we want to test if he is reachable + * @param string $parentRole The role which should give access to $childRole * @return boolean true if the child role is granted by parent role */ - protected function isRoleReached($childRole, $parentRole) + private function isRoleReached(string $childRole, string $parentRole) { - $reachableRoles = $this->roleHierarchy - ->getReachableRoleNames([$parentRole]); - - return in_array($childRole, $reachableRoles); + return $this->parentRoleHelper->isRoleReached($childRole, $parentRole); } /** * Return all the role which give access to the given role. Only the role * which are registered into Chill are taken into account. * - * @param Role $role + * @param string $role * @return string[] the role which give access to the given $role */ - public function getParentRoles($role): array + public function getParentRoles(string $role): array { - $parentRoles = []; - // transform the roles from role hierarchy from string to Role - $roles = \array_keys($this->hierarchy); - - foreach ($roles as $r) { - $childRoles = $this->roleHierarchy->getReachableRoleNames([$r]); - - if (\in_array($role, $childRoles)) { - $parentRoles[] = $r; - } - } - - return $parentRoles; + trigger_deprecation('Chill\MainBundle', '2.0', 'use ParentRoleHelper::getParentRoles instead'); + return $this->parentRoleHelper->getParentRoles($role); } } diff --git a/src/Bundle/ChillMainBundle/Security/ParentRoleHelper.php b/src/Bundle/ChillMainBundle/Security/ParentRoleHelper.php new file mode 100644 index 000000000..07366dafa --- /dev/null +++ b/src/Bundle/ChillMainBundle/Security/ParentRoleHelper.php @@ -0,0 +1,71 @@ +roleHierarchy = $roleHierarchy; + $this->hierarchy = $parameterBag->get('security.role_hierarchy.roles'); + } + + /** + * Return all the role which give access to the given role. Only the role + * which are registered into Chill are taken into account. + * + * @param string $role + * @return string[] the role which give access to the given $role + */ + public function getParentRoles(string $role): array + { + $parentRoles = []; + // transform the roles from role hierarchy from string to Role + $roles = \array_keys($this->hierarchy); + + foreach ($roles as $r) { + $childRoles = $this->roleHierarchy->getReachableRoleNames([$r]); + + if (\in_array($role, $childRoles)) { + $parentRoles[] = $r; + } + } + + return $parentRoles; + } + + /** + * Test if a parent role may give access to a given child role + * + * @param string $childRole The role we want to test if he is reachable + * @param string $parentRole The role which should give access to $childRole + * @return bool true if the child role is granted by parent role + */ + public function isRoleReached($childRole, $parentRole): bool + { + $reachableRoles = $this->roleHierarchy + ->getReachableRoleNames([$parentRole]); + + return in_array($childRole, $reachableRoles); + } + +} diff --git a/src/Bundle/ChillMainBundle/Tests/Authorization/ParentRoleHelperTest.php b/src/Bundle/ChillMainBundle/Tests/Authorization/ParentRoleHelperTest.php new file mode 100644 index 000000000..0d718ec09 --- /dev/null +++ b/src/Bundle/ChillMainBundle/Tests/Authorization/ParentRoleHelperTest.php @@ -0,0 +1,36 @@ +parentRoleHelper = self::$container->get(ParentRoleHelper::class); + } + + public function testGetReachableRoles() + { + // this test will be valid until the role hierarchy for person is changed. + // this is not perfect but spare us a mock + + $parentRoles = $this->parentRoleHelper->getParentRoles(PersonVoter::SEE); + + $this->assertCount(2, $parentRoles); + $this->assertContains(PersonVoter::CREATE, $parentRoles); + $this->assertContains(PersonVoter::UPDATE, $parentRoles); + } + + public function testIsRoleReached() + { + $this->assertTrue($this->parentRoleHelper->isRoleReached(PersonVoter::SEE, PersonVoter::CREATE)); + $this->assertFalse($this->parentRoleHelper->isRoleReached(PersonVoter::SEE, 'foo')); + } +} diff --git a/src/Bundle/ChillMainBundle/config/services/security.yaml b/src/Bundle/ChillMainBundle/config/services/security.yaml index 15d6f7da5..dc7949556 100644 --- a/src/Bundle/ChillMainBundle/config/services/security.yaml +++ b/src/Bundle/ChillMainBundle/config/services/security.yaml @@ -40,6 +40,10 @@ services: Chill\MainBundle\Security\Authorization\AuthorizationHelper: '@chill.main.security.authorization.helper' Chill\MainBundle\Security\Authorization\AuthorizationHelperInterface: '@chill.main.security.authorization.helper' + Chill\MainBundle\Security\ParentRoleHelper: + autowire: true + autoconfigure: true + chill.main.role_provider: class: Chill\MainBundle\Security\RoleProvider Chill\MainBundle\Security\RoleProvider: '@chill.main.role_provider' From 35154f5ae17c975f4261be6df1aa0e31285069ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 3 Nov 2021 15:33:03 +0100 Subject: [PATCH 3/5] fix constructor into acl aware repository --- .../Repository/UserACLAwareRepository.php | 12 ++++++++++-- .../Security/Authorization/AuthorizationHelper.php | 3 ++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Repository/UserACLAwareRepository.php b/src/Bundle/ChillMainBundle/Repository/UserACLAwareRepository.php index 44399000b..de0ded2c2 100644 --- a/src/Bundle/ChillMainBundle/Repository/UserACLAwareRepository.php +++ b/src/Bundle/ChillMainBundle/Repository/UserACLAwareRepository.php @@ -6,15 +6,23 @@ use Chill\MainBundle\Entity\Center; use Chill\MainBundle\Entity\Scope; use Chill\MainBundle\Entity\User; use Chill\MainBundle\Security\Authorization\AuthorizationHelper; +use Chill\MainBundle\Security\ParentRoleHelper; +use Doctrine\ORM\EntityManagerInterface; class UserACLAwareRepository implements UserACLAwareRepositoryInterface { + private ParentRoleHelper $parentRoleHelper; + private EntityManagerInterface $em; - private AuthorizationHelper $authorizationHelper; + public function __construct(ParentRoleHelper $parentRoleHelper, EntityManagerInterface $em) + { + $this->parentRoleHelper = $parentRoleHelper; + $this->em = $em; + } public function findUsersByReachedACL(string $role, $center, $scope = null, bool $onlyEnabled = true): array { - $parents = $this->authorizationHelper->getParentRoles($role); + $parents = $this->parentRoleHelper->getParentRoles($role); $parents[] = $role; $qb = $this->em->createQueryBuilder(); diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php index f075b650f..03adbdbc9 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/AuthorizationHelper.php @@ -321,7 +321,8 @@ class AuthorizationHelper implements AuthorizationHelperInterface */ public function findUsersReaching(string $role, $center, $scope = null, bool $onlyEnabled = true): array { - return $this->userACLAwareRepository->findUsersByReachedACL($role, $center, $scope, $onlyEnabled); + return $this->userACLAwareRepository + ->findUsersByReachedACL($role, $center, $scope, $onlyEnabled); } /** From 7c5d0fce5114f47a588a0cbcf945470cd7689dd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 3 Nov 2021 15:35:12 +0100 Subject: [PATCH 4/5] update changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 507d4dc40..2a4d8aa69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,11 +11,13 @@ and this project adheres to ## Unreleased + * [tasks] improve tasks with filter order * [tasks] refactor singleControllerTasks: limit the number of conditions from the context * [validations] validation of accompanying period added: no duplicate participations or resources (https://gitlab.com/champs-libres/departement-de-la-vendee/accent-suivi-developpement/-/issues/60). * [renderbox] If gender of person is not defined, no icon is displayed instead of neuter-icon (https://gitlab.com/champs-libres/departement-de-la-vendee/accent-suivi-developpement/-/issues/129). * [confidential information] module added to blur confidential information (https://gitlab.com/champs-libres/departement-de-la-vendee/chill/-/issues/248). +* refactor `AuthorizationHelper` and `UserACLAwareRepository` to fix constructor, and separate logic for parent role helper into `ParentRoleHelper` ## Test releases From 8c5bf6786bcb6e0004db0df65f79162f6dd528d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Wed, 3 Nov 2021 15:46:00 +0100 Subject: [PATCH 5/5] increase memory on gitlab-ci operations --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index a4588498e..68b1bcf08 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -16,7 +16,7 @@ before_script: - php -d memory_limit=2G composer.phar install - php tests/app/bin/console doctrine:migrations:migrate -n - php -d memory_limit=2G tests/app/bin/console cache:clear --env=dev - - php -d memory_limit=2G tests/app/bin/console doctrine:fixtures:load -n + - php -d memory_limit=3G tests/app/bin/console doctrine:fixtures:load -n - php -d memory_limit=2G tests/app/bin/console cache:clear --env=test - echo "before_script finished"