Skip to content

Commit

Permalink
fix(Security): Fixed NodeVoter accepting UserInterface for OpenId…
Browse files Browse the repository at this point in the history 
… accounts
  • Loading branch information
@ambroisemaupate
ambroisemaupate committed May 14, 2024
1 parent a217581 commit e9dc229
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 46 deletions.
23 changes: 21 additions & 2 deletions lib/OpenId/src/Authentication/OpenIdAuthenticator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use GuzzleHttp\Client;
use GuzzleHttp\Exception\GuzzleException;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Psr7\Query;
use Lcobucci\JWT\Token\Plain;
use Lcobucci\JWT\Validation\RequiredConstraintsViolated;
Expand Down Expand Up @@ -68,7 +69,6 @@ public function supports(Request $request): ?bool
$this->discovery->isValid() &&
$this->httpUtils->checkRequestPath($request, $this->returnPath) &&
$request->query->has('state') &&
$request->query->has('scope') &&
($request->query->has('code') || $request->query->has('error'));
}

Expand Down Expand Up @@ -132,9 +132,28 @@ public function authenticate(Request $request): Passport
]);
/** @var array $jsonResponse */
$jsonResponse = \json_decode($response->getBody()->getContents(), true);
} catch (RequestException $e) {
if (null !== $e->getResponse()) {
/** @var array $jsonResponse */
$jsonResponse = \json_decode($e->getResponse()->getBody()->getContents(), true);
$errorTitle = $jsonResponse['error'] ?? $e->getMessage();
$errorDescription = $jsonResponse['error_description'] ?? '';

throw new OpenIdAuthenticationException(
$errorTitle . ': ' . $errorDescription,
$e->getCode(),
$e
);
}

throw new OpenIdAuthenticationException(
$e->getMessage(),
$e->getCode(),
$e
);
} catch (GuzzleException $e) {
throw new OpenIdAuthenticationException(
'Cannot contact Identity provider to issue authorization_code.' . $e->getMessage(),
$e->getMessage(),
$e->getCode(),
$e
);
Expand Down
12 changes: 2 additions & 10 deletions lib/RoadizCoreBundle/src/Security/Authorization/Voter/GroupVoter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}
40 changes: 19 additions & 21 deletions lib/RoadizCoreBundle/src/Security/Authorization/Voter/NodeVoter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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);
}
Expand Down
6 changes: 2 additions & 4 deletions lib/RoadizCoreBundle/src/Security/Authorization/Voter/RoleArrayVoter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
Expand Down
14 changes: 5 additions & 9 deletions lib/RoadizCoreBundle/src/Security/Authorization/Voter/SuperAdminRoleHierarchyVoter.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down

0 comments on commit e9dc229

Please sign in to comment.