Merge branch 'bugfix/authorization-helper-fix-constructor' into 'master'

authorization helper: fix consructor

See merge request Chill-Projet/chill-bundles!194

.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"
 

CHANGELOG.md

@@ -11,11 +11,13 @@ and this project adheres to
 ## Unreleased
 
 <!-- write down unreleased development here -->
+
 * [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

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();

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,40 +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
+        ScopeResolverDispatcher $scopeResolverDispatcher,
+        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;
     }
 
     /**
@@ -91,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) {
@@ -332,45 +321,32 @@ 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);
     }
 
     /**
      * 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);
     }
 }

src/Bundle/ChillMainBundle/Security/ParentRoleHelper.php

@@ -0,0 +1,71 @@
+<?php
+
+namespace Chill\MainBundle\Security;
+
+use Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface;
+use Symfony\Component\Security\Core\Role\Role;
+use Symfony\Component\Security\Core\Role\RoleHierarchyInterface;
+
+/**
+ * Helper which traverse all role to find parents
+ */
+class ParentRoleHelper
+{
+    protected RoleHierarchyInterface $roleHierarchy;
+
+    /**
+     * The role in a hierarchy, given by the parameter
+     * `security.role_hierarchy.roles` from the container.
+     *
+     * @var string[]
+     */
+    protected array $hierarchy;
+
+    public function __construct(
+        RoleHierarchyInterface $roleHierarchy,
+        ParameterBagInterface $parameterBag
+    ) {
+        $this->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);
+    }
+
+}

src/Bundle/ChillMainBundle/Tests/Authorization/ParentRoleHelperTest.php

@@ -0,0 +1,36 @@
+<?php
+
+namespace Chill\MainBundle\Tests\Security\Authorization;
+
+use Chill\MainBundle\Security\ParentRoleHelper;
+use Chill\PersonBundle\Security\Authorization\PersonVoter;
+use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+
+class ParentRoleHelperTest extends KernelTestCase
+{
+    private ParentRoleHelper $parentRoleHelper;
+
+    public function setUp()
+    {
+        self::bootKernel();
+        $this->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'));
+    }
+}

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'