Skip to content

Commit dfb763d

Browse files
philipsorstphilipsorst
committed
fix: Fixing READ security
1 parent d1570c2 commit dfb763d

6 files changed

Lines changed: 95 additions & 43 deletions

File tree

‎src/Security/AccessCheckerProvider.php‎

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -54,47 +54,24 @@ public function provide(Operation $operation, array $uriVariables = [], array $c
5454

5555
/* Check if operation is allowed without data */
5656
if (null === $this->event) {
57-
if (CrudOperation::LIST === $crudOperation) {
58-
if (!$this->authorizationChecker->isGranted(CrudOperation::LIST->value, $operation->getClass())) {
59-
throw new AccessDeniedException();
60-
}
61-
}
62-
63-
if (CrudOperation::CREATE === $crudOperation) {
64-
if (!$this->authorizationChecker->isGranted(CrudOperation::CREATE->value, $operation->getClass())) {
57+
if (in_array($crudOperation, [CrudOperation::LIST, CrudOperation::CREATE], true)) {
58+
if (!$this->authorizationChecker->isGranted($crudOperation->value, $operation->getClass())) {
6559
throw new AccessDeniedException();
6660
}
6761
}
6862
}
6963

7064
$data = $this->decorated->provide($operation, $uriVariables, $context);
7165

72-
$crudOperation = match ($operation::class) {
73-
GetCollection::class => CrudOperation::LIST,
74-
Post::class => CrudOperation::CREATE,
75-
Get::class => CrudOperation::READ,
76-
Put::class, Patch::class => CrudOperation::UPDATE,
77-
Delete::class => CrudOperation::DELETE,
78-
default => null,
79-
};
80-
8166
if (null === $this->event) {
82-
if (CrudOperation::DELETE === $crudOperation) {
83-
if (!$this->authorizationChecker->isGranted(CrudOperation::DELETE->value, $data)) {
84-
throw new AccessDeniedException();
85-
}
86-
}
87-
}
88-
89-
/* Check if operation is allowed with data after denormalization */
90-
if (in_array($this->event, ['post_denormalize', 'post_validate'], true)) {
91-
if (CrudOperation::CREATE === $crudOperation) {
92-
if (!$this->authorizationChecker->isGranted(CrudOperation::CREATE->value, $data)) {
67+
if (in_array($crudOperation, [CrudOperation::DELETE, CrudOperation::READ], true)) {
68+
if (!$this->authorizationChecker->isGranted($crudOperation->value, $data)) {
9369
throw new AccessDeniedException();
9470
}
9571
}
96-
if (CrudOperation::UPDATE === $crudOperation) {
97-
if (!$this->authorizationChecker->isGranted(CrudOperation::UPDATE->value, $data)) {
72+
} elseif (in_array($this->event, ['post_denormalize', 'post_validate'], true)) {
73+
if (in_array($crudOperation, [CrudOperation::CREATE, CrudOperation::UPDATE], true)) {
74+
if (!$this->authorizationChecker->isGranted($crudOperation->value, $data)) {
9875
throw new AccessDeniedException();
9976
}
10077
}

‎tests/Acceptance/GroupEndpointTest.php‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?php
2+
3+
namespace Dontdrinkandroot\ApiPlatformBundle\Tests\Acceptance;
4+
5+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\DataFixtures\Group\GroupOne;
6+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\DataFixtures\Group\GroupTwo;
7+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\DataFixtures\User\UserOne;
8+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\DataFixtures\User\UserTwo;
9+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\Entity\Group;
10+
use Symfony\Component\HttpFoundation\Response;
11+
12+
class GroupEndpointTest extends AbstractAcceptanceTest
13+
{
14+
public function testReadProtected(): void
15+
{
16+
$client = self::createClient();
17+
$referenceRepository = self::loadFixtures([GroupOne::class, GroupTwo::class]);
18+
$group2 = $referenceRepository->getReference(GroupTwo::class, Group::class);
19+
$response = $this->jsonGet(
20+
$client,
21+
sprintf("/groups/%d", $group2->getId()),
22+
);
23+
self::assertEquals(Response::HTTP_UNAUTHORIZED, $response->getStatusCode());
24+
25+
$response = $this->jsonGet(
26+
$client,
27+
sprintf("/groups/%d", $group2->getId()),
28+
[],
29+
$this->addBasicAuthorizationHeader(UserOne::USERNAME, UserOne::PASSWORD)
30+
);
31+
self::assertEquals(Response::HTTP_FORBIDDEN, $response->getStatusCode());
32+
}
33+
34+
public function testReadAccessibleForGroupMember(): void
35+
{
36+
$client = self::createClient();
37+
$referenceRepository = self::loadFixtures([GroupOne::class, GroupTwo::class]);
38+
$group2 = $referenceRepository->getReference(GroupTwo::class, Group::class);
39+
$response = $this->jsonGet(
40+
$client,
41+
sprintf("/groups/%d", $group2->getId()),
42+
[],
43+
$this->addBasicAuthorizationHeader(UserTwo::USERNAME, UserTwo::PASSWORD)
44+
);
45+
self::assertEquals(Response::HTTP_OK, $response->getStatusCode());
46+
}
47+
}

‎tests/TestApp/Repository/GroupRepository.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@ class GroupRepository extends ServiceEntityRepository
1313
{
1414
public function __construct(ManagerRegistry $registry)
1515
{
16-
parent::__construct($registry, GroupRepository::class);
16+
parent::__construct($registry, Group::class);
1717
}
1818
}

‎tests/TestApp/Security/AdminVoter.php‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,17 @@
55
use Override;
66
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
77
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
8+
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
89

9-
/**
10-
* @extends Voter<string,mixed>
11-
*/
12-
class AdminVoter extends Voter
10+
class AdminVoter implements VoterInterface
1311
{
1412
#[Override]
15-
protected function supports(string $attribute, mixed $subject): bool
13+
public function vote(TokenInterface $token, mixed $subject, array $attributes)
1614
{
17-
return true;
18-
}
15+
if (in_array('ROLE_ADMIN', $token->getRoleNames(), true)) {
16+
return Voter::ACCESS_GRANTED;
17+
}
1918

20-
#[Override]
21-
protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool
22-
{
23-
return in_array('ROLE_ADMIN', $token->getRoleNames(), true);
19+
return Voter::ACCESS_ABSTAIN;
2420
}
2521
}

‎tests/TestApp/Security/GroupVoter.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<?php
2+
3+
namespace Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\Security;
4+
5+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\Entity\Group;
6+
use Dontdrinkandroot\ApiPlatformBundle\Tests\TestApp\Entity\User;
7+
use Dontdrinkandroot\Common\CrudOperation;
8+
use Override;
9+
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
10+
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
11+
12+
/**
13+
* @extends Voter<'READ',Group>
14+
*/
15+
class GroupVoter extends Voter
16+
{
17+
#[Override]
18+
protected function supports(string $attribute, mixed $subject): bool
19+
{
20+
return CrudOperation::READ->value === $attribute && $subject instanceof Group;
21+
}
22+
23+
#[Override]
24+
protected function voteOnAttribute(string $attribute, mixed $subject, TokenInterface $token): bool
25+
{
26+
if (!($user = $token->getUser()) instanceof User) {
27+
return false;
28+
}
29+
30+
return $user->groups->contains($subject);
31+
}
32+
}

tests/TestApp/Security/UserClassVoter.php renamed to tests/TestApp/Security/UserVoter.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
/**
1212
* @extends Voter<'READ',User>
1313
*/
14-
class UserClassVoter extends Voter
14+
class UserVoter extends Voter
1515
{
1616
#[Override]
1717
protected function supports(string $attribute, $subject): bool

0 commit comments

Comments
 (0)