From 9c9b8d9ca4d865c34d2b4d5b785c1708cabaa98c Mon Sep 17 00:00:00 2001
From: roadiz-ci <ambroise+ci@roadiz.io>
Date: Tue, 14 May 2024 12:51:05 +0000
Subject: [PATCH] fix(Security): Fixed `NodeVoter` accepting `UserInterface`
 for OpenId accounts

---
 .../Authorization/Voter/GroupVoter.php        | 12 +-----
 .../Authorization/Voter/NodeVoter.php         | 40 +++++++++----------
 .../Authorization/Voter/RoleArrayVoter.php    |  6 +--
 .../Voter/SuperAdminRoleHierarchyVoter.php    | 14 +++----
 4 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/src/Security/Authorization/Voter/GroupVoter.php b/src/Security/Authorization/Voter/GroupVoter.php
index 10bcfb52..a6d8a341 100644
--- a/src/Security/Authorization/Voter/GroupVoter.php
+++ b/src/Security/Authorization/Voter/GroupVoter.php
@@ -13,11 +13,8 @@
 
 class GroupVoter extends RoleVoter
 {
-    private RoleHierarchyInterface $roleHierarchy;
-
-    public function __construct(RoleHierarchyInterface $roleHierarchy, string $prefix = 'ROLE_')
+    public function __construct(private readonly RoleHierarchyInterface $roleHierarchy, string $prefix = 'ROLE_')
     {
-        $this->roleHierarchy = $roleHierarchy;
         parent::__construct($prefix);
     }
 
@@ -96,11 +93,6 @@ protected function extractGroupRoles(Group $group): array
      */
     protected function isRoleContained(string $role, array $roles): bool
     {
-        foreach ($roles as $singleRole) {
-            if ($role === $singleRole) {
-                return true;
-            }
-        }
-        return false;
+        return \in_array($role, $roles, true);
     }
 }
diff --git a/src/Security/Authorization/Voter/NodeVoter.php b/src/Security/Authorization/Voter/NodeVoter.php
index 141e01b7..7ddffd07 100644
--- a/src/Security/Authorization/Voter/NodeVoter.php
+++ b/src/Security/Authorization/Voter/NodeVoter.php
@@ -6,15 +6,13 @@
 
 use Doctrine\Persistence\ManagerRegistry;
 use Psr\Cache\CacheItemPoolInterface;
-use RZ\Roadiz\Core\Handlers\HandlerFactoryInterface;
 use RZ\Roadiz\CoreBundle\Entity\Node;
 use RZ\Roadiz\CoreBundle\Entity\NodesSources;
-use RZ\Roadiz\CoreBundle\Entity\User;
-use RZ\Roadiz\CoreBundle\EntityHandler\NodeHandler;
 use RZ\Roadiz\CoreBundle\Security\Authorization\Chroot\NodeChrootResolver;
+use Symfony\Bundle\SecurityBundle\Security;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\Voter;
-use Symfony\Bundle\SecurityBundle\Security;
+use Symfony\Component\Security\Core\User\UserInterface;
 
 /**
  * @extends Voter<'CREATE'|'DUPLICATE'|'CREATE_AT_ROOT'|'SEARCH'|'READ'|'READ_AT_ROOT'|'EMPTY_TRASH'|'READ_LOGS'|'EDIT_CONTENT'|'EDIT_TAGS'|'EDIT_REALMS'|'EDIT_SETTING'|'EDIT_STATUS'|'EDIT_ATTRIBUTE'|'DELETE', Node>
@@ -87,7 +85,7 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter
     {
         $user = $token->getUser();
 
-        if (!$user instanceof User) {
+        if (!$user instanceof UserInterface) {
             // the user must be logged in; if not, deny access
             return false;
         }
@@ -139,7 +137,7 @@ private function isNodeInsideUserChroot(Node $node, Node $chroot, bool $includeC
         return \in_array($node->getId(), $offspringIds, true);
     }
 
-    private function isGrantedWithUserChroot(Node $node, User $user, array|string $roles, bool $includeChroot): bool
+    private function isGrantedWithUserChroot(Node $node, UserInterface $user, array|string $roles, bool $includeChroot): bool
     {
         $chroot = $this->chrootResolver->getChroot($user);
         if (null === $chroot) {
@@ -150,32 +148,32 @@ private function isGrantedWithUserChroot(Node $node, User $user, array|string $r
             $this->isNodeInsideUserChroot($node, $chroot, $includeChroot);
     }
 
-    private function canCreateAtRoot(User $user): bool
+    private function canCreateAtRoot(UserInterface $user): bool
     {
         $chroot = $this->chrootResolver->getChroot($user);
         return null === $chroot && $this->security->isGranted('ROLE_ACCESS_NODES');
     }
 
-    private function canReadAtRoot(User $user): bool
+    private function canReadAtRoot(UserInterface $user): bool
     {
         $chroot = $this->chrootResolver->getChroot($user);
         return null === $chroot && $this->security->isGranted('ROLE_ACCESS_NODES');
     }
 
-    private function canSearch(User $user): bool
+    private function canSearch(UserInterface $user): bool
     {
         $chroot = $this->chrootResolver->getChroot($user);
         return null === $chroot && $this->security->isGranted('ROLE_ACCESS_NODES');
     }
 
-    private function canEmptyTrash(User $user): bool
+    private function canEmptyTrash(UserInterface $user): bool
     {
         $chroot = $this->chrootResolver->getChroot($user);
         return null === $chroot && $this->security->isGranted('ROLE_ACCESS_NODES_DELETE');
     }
 
 
-    private function canCreate(Node $node, User $user): bool
+    private function canCreate(Node $node, UserInterface $user): bool
     {
         /*
          * Creation is allowed only if node is inside user chroot,
@@ -184,7 +182,7 @@ private function canCreate(Node $node, User $user): bool
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES', true);
     }
 
-    private function canRead(Node $node, User $user): bool
+    private function canRead(Node $node, UserInterface $user): bool
     {
         /*
          * Read is allowed only if node is inside user chroot,
@@ -193,12 +191,12 @@ private function canRead(Node $node, User $user): bool
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES', true);
     }
 
-    private function canReadLogs(Node $node, User $user): bool
+    private function canReadLogs(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, ['ROLE_ACCESS_NODES', 'ROLE_ACCESS_LOGS'], false);
     }
 
-    private function canEditContent(Node $node, User $user): bool
+    private function canEditContent(Node $node, UserInterface $user): bool
     {
         /*
          * Edition is allowed only if node is inside user chroot,
@@ -207,17 +205,17 @@ private function canEditContent(Node $node, User $user): bool
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES', false);
     }
 
-    private function canEditTags(Node $node, User $user): bool
+    private function canEditTags(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, ['ROLE_ACCESS_NODES', 'ROLE_ACCESS_TAGS'], false);
     }
 
-    private function canEditRealms(Node $node, User $user): bool
+    private function canEditRealms(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, ['ROLE_ACCESS_NODES', 'ROLE_ACCESS_REALM_NODES'], false);
     }
 
-    private function canDuplicate(Node $node, User $user): bool
+    private function canDuplicate(Node $node, UserInterface $user): bool
     {
         /*
          * Duplication is allowed only if node is inside user chroot,
@@ -226,22 +224,22 @@ private function canDuplicate(Node $node, User $user): bool
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES', false);
     }
 
-    private function canEditSetting(Node $node, User $user): bool
+    private function canEditSetting(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES_SETTING', false);
     }
 
-    private function canEditStatus(Node $node, User $user): bool
+    private function canEditStatus(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES_STATUS', false);
     }
 
-    private function canDelete(Node $node, User $user): bool
+    private function canDelete(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODES_DELETE', false);
     }
 
-    private function canEditAttribute(Node $node, User $user): bool
+    private function canEditAttribute(Node $node, UserInterface $user): bool
     {
         return $this->isGrantedWithUserChroot($node, $user, 'ROLE_ACCESS_NODE_ATTRIBUTES', false);
     }
diff --git a/src/Security/Authorization/Voter/RoleArrayVoter.php b/src/Security/Authorization/Voter/RoleArrayVoter.php
index 21ae892f..b373a3c2 100644
--- a/src/Security/Authorization/Voter/RoleArrayVoter.php
+++ b/src/Security/Authorization/Voter/RoleArrayVoter.php
@@ -40,10 +40,8 @@ public function vote(TokenInterface $token, $subject, array $attributes): int
                 }
 
                 $result = VoterInterface::ACCESS_DENIED;
-                foreach ($roles as $role) {
-                    if ($singleAttribute === $role) {
-                        return VoterInterface::ACCESS_GRANTED;
-                    }
+                if (\in_array($singleAttribute, $roles, true)) {
+                    return VoterInterface::ACCESS_GRANTED;
                 }
             }
         }
diff --git a/src/Security/Authorization/Voter/SuperAdminRoleHierarchyVoter.php b/src/Security/Authorization/Voter/SuperAdminRoleHierarchyVoter.php
index e893fcbb..23ccd845 100644
--- a/src/Security/Authorization/Voter/SuperAdminRoleHierarchyVoter.php
+++ b/src/Security/Authorization/Voter/SuperAdminRoleHierarchyVoter.php
@@ -10,16 +10,9 @@
 
 final class SuperAdminRoleHierarchyVoter extends RoleArrayVoter
 {
-    private ManagerRegistry $managerRegistry;
-
-    /**
-     * @param ManagerRegistry $managerRegistry
-     * @param string $prefix
-     */
-    public function __construct(ManagerRegistry $managerRegistry, string $prefix = 'ROLE_')
+    public function __construct(private readonly ManagerRegistry $managerRegistry, string $prefix = 'ROLE_')
     {
         parent::__construct($prefix);
-        $this->managerRegistry = $managerRegistry;
     }
 
     protected function extractRoles(TokenInterface $token): array
@@ -37,7 +30,10 @@ protected function extractRoles(TokenInterface $token): array
     private function isSuperAdmin(TokenInterface $token): bool
     {
         $roleNames = parent::extractRoles($token);
-        if (\in_array('ROLE_SUPER_ADMIN', $roleNames) || \in_array('ROLE_SUPERADMIN', $roleNames)) {
+        if (
+            \in_array('ROLE_SUPER_ADMIN', $roleNames) ||
+            \in_array('ROLE_SUPERADMIN', $roleNames)
+        ) {
             return true;
         }
         return false;