diff --git a/app/config/security.yml b/app/config/security.yml index b05e54b01..05400966d 100644 --- a/app/config/security.yml +++ b/app/config/security.yml @@ -74,6 +74,5 @@ security: - { path: /(unread|starred|archive|annotated).xml$, roles: IS_AUTHENTICATED_ANONYMOUSLY } # For backwards compatibility - { path: ^/share, roles: IS_AUTHENTICATED_ANONYMOUSLY } - { path: ^/settings, roles: ROLE_SUPER_ADMIN } - - { path: ^/annotations, roles: ROLE_USER } - { path: ^/2fa, role: IS_AUTHENTICATED_2FA_IN_PROGRESS } - { path: ^/, roles: ROLE_USER } diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index a3a1e281f..4cc1d7c17 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -6,7 +6,7 @@ parameters: path: src/Controller/AnnotationController.php - - message: "#^Method Wallabag\\\\Controller\\\\AnnotationController\\:\\:putAnnotationAction\\(\\) should return Symfony\\\\Component\\\\HttpFoundation\\\\JsonResponse but returns Symfony\\\\Component\\\\Form\\\\FormInterface\\\\.$#" + message: "#^Method Wallabag\\\\Controller\\\\AnnotationController\\:\\:putAnnotationAction\\(\\) should return Symfony\\\\Component\\\\HttpFoundation\\\\JsonResponse but returns Symfony\\\\Component\\\\Form\\\\FormInterface\\\\.$#" count: 1 path: src/Controller/AnnotationController.php diff --git a/src/Controller/AnnotationController.php b/src/Controller/AnnotationController.php index 5d177bc0d..71ca4ded2 100644 --- a/src/Controller/AnnotationController.php +++ b/src/Controller/AnnotationController.php @@ -5,6 +5,7 @@ namespace Wallabag\Controller; use Doctrine\ORM\EntityManagerInterface; use FOS\RestBundle\Controller\AbstractFOSRestController; use JMS\Serializer\SerializerInterface; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\Form\FormFactoryInterface; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; @@ -36,6 +37,7 @@ class AnnotationController extends AbstractFOSRestController * @see Api\WallabagRestController * * @Route("/annotations/{entry}.{_format}", name="annotations_get_annotations", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("LIST_ANNOTATIONS", subject="entry") * * @return JsonResponse */ @@ -57,6 +59,7 @@ class AnnotationController extends AbstractFOSRestController * @see Api\WallabagRestController * * @Route("/annotations/{entry}.{_format}", name="annotations_post_annotation", methods={"POST"}, defaults={"_format": "json"}) + * @IsGranted("CREATE_ANNOTATIONS", subject="entry") * * @return JsonResponse */ @@ -91,14 +94,13 @@ class AnnotationController extends AbstractFOSRestController * @see Api\WallabagRestController * * @Route("/annotations/{annotation}.{_format}", name="annotations_put_annotation", methods={"PUT"}, defaults={"_format": "json"}) + * @IsGranted("EDIT", subject="annotation") * * @return JsonResponse */ - public function putAnnotationAction(Request $request, AnnotationRepository $annotationRepository, int $annotation) + public function putAnnotationAction(Request $request, Annotation $annotation) { try { - $annotation = $this->validateAnnotation($annotationRepository, $annotation, $this->getUser()->getId()); - $data = json_decode($request->getContent(), true, 512, \JSON_THROW_ON_ERROR); $form = $this->formFactory->createNamed('', EditAnnotationType::class, $annotation, [ @@ -128,14 +130,13 @@ class AnnotationController extends AbstractFOSRestController * @see Api\WallabagRestController * * @Route("/annotations/{annotation}.{_format}", name="annotations_delete_annotation", methods={"DELETE"}, defaults={"_format": "json"}) + * @IsGranted("DELETE", subject="annotation") * * @return JsonResponse */ - public function deleteAnnotationAction(AnnotationRepository $annotationRepository, int $annotation) + public function deleteAnnotationAction(Annotation $annotation) { try { - $annotation = $this->validateAnnotation($annotationRepository, $annotation, $this->getUser()->getId()); - $this->entityManager->remove($annotation); $this->entityManager->flush(); @@ -157,15 +158,4 @@ class AnnotationController extends AbstractFOSRestController return $user; } - - private function validateAnnotation(AnnotationRepository $annotationRepository, int $annotationId, int $userId) - { - $annotation = $annotationRepository->findOneByIdAndUserId($annotationId, $userId); - - if (null === $annotation) { - throw new NotFoundHttpException(); - } - - return $annotation; - } } diff --git a/src/Security/Voter/AnnotationVoter.php b/src/Security/Voter/AnnotationVoter.php new file mode 100644 index 000000000..7bed72dbc --- /dev/null +++ b/src/Security/Voter/AnnotationVoter.php @@ -0,0 +1,46 @@ +getUser(); + + if (!$user instanceof User) { + return false; + } + + switch ($attribute) { + case self::EDIT: + case self::DELETE: + return $subject->getUser() === $user; + } + + return false; + } +} diff --git a/src/Security/Voter/EntryVoter.php b/src/Security/Voter/EntryVoter.php index 5a8f8a709..baeca0408 100644 --- a/src/Security/Voter/EntryVoter.php +++ b/src/Security/Voter/EntryVoter.php @@ -17,6 +17,8 @@ class EntryVoter extends Voter public const SHARE = 'SHARE'; public const UNSHARE = 'UNSHARE'; public const DELETE = 'DELETE'; + public const LIST_ANNOTATIONS = 'LIST_ANNOTATIONS'; + public const CREATE_ANNOTATIONS = 'CREATE_ANNOTATIONS'; protected function supports(string $attribute, $subject): bool { @@ -24,7 +26,7 @@ class EntryVoter extends Voter return false; } - if (!\in_array($attribute, [self::VIEW, self::EDIT, self::RELOAD, self::STAR, self::ARCHIVE, self::SHARE, self::UNSHARE, self::DELETE], true)) { + if (!\in_array($attribute, [self::VIEW, self::EDIT, self::RELOAD, self::STAR, self::ARCHIVE, self::SHARE, self::UNSHARE, self::DELETE, self::LIST_ANNOTATIONS, self::CREATE_ANNOTATIONS], true)) { return false; } @@ -50,6 +52,8 @@ class EntryVoter extends Voter case self::SHARE: case self::UNSHARE: case self::DELETE: + case self::LIST_ANNOTATIONS: + case self::CREATE_ANNOTATIONS: return $user === $subject->getUser(); } diff --git a/tests/Controller/AnnotationControllerTest.php b/tests/Controller/AnnotationControllerTest.php index b55e86efa..f4339955c 100644 --- a/tests/Controller/AnnotationControllerTest.php +++ b/tests/Controller/AnnotationControllerTest.php @@ -82,10 +82,7 @@ class AnnotationControllerTest extends WallabagTestCase } $this->client->request('GET', $prefixUrl . '/' . $entry->getId() . '.json'); - $this->assertSame(200, $this->client->getResponse()->getStatusCode()); - - $content = json_decode($this->client->getResponse()->getContent(), true); - $this->assertGreaterThanOrEqual(0, $content['total']); + $this->assertSame(404, $this->client->getResponse()->getStatusCode()); } /** diff --git a/tests/Security/Voter/AnnotationVoterTest.php b/tests/Security/Voter/AnnotationVoterTest.php new file mode 100644 index 000000000..920eb9cf4 --- /dev/null +++ b/tests/Security/Voter/AnnotationVoterTest.php @@ -0,0 +1,85 @@ +token = $this->createMock(TokenInterface::class); + + $this->annotationVoter = new AnnotationVoter(); + } + + public function testVoteReturnsAbstainForInvalidSubject(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->annotationVoter->vote($this->token, new \stdClass(), [AnnotationVoter::EDIT])); + } + + public function testVoteReturnsAbstainForInvalidAttribute(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->annotationVoter->vote($this->token, new Annotation(new User()), ['INVALID'])); + } + + public function testVoteReturnsDeniedForUnauthenticatedEdit(): void + { + $this->token->method('getUser')->willReturn(null); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->annotationVoter->vote($this->token, new Annotation(new User()), [AnnotationVoter::EDIT])); + } + + public function testVoteReturnsDeniedForOtherUserEdit(): void + { + $currentUser = new User(); + $annotationUser = new User(); + + $this->token->method('getUser')->willReturn($currentUser); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->annotationVoter->vote($this->token, new Annotation($annotationUser), [AnnotationVoter::EDIT])); + } + + public function testVoteReturnsGrantedForAnnotationUserEdit(): void + { + $user = new User(); + + $this->token->method('getUser')->willReturn($user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->annotationVoter->vote($this->token, new Annotation($user), [AnnotationVoter::EDIT])); + } + + public function testVoteReturnsDeniedForUnauthenticatedDelete(): void + { + $this->token->method('getUser')->willReturn(null); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->annotationVoter->vote($this->token, new Annotation(new User()), [AnnotationVoter::DELETE])); + } + + public function testVoteReturnsDeniedForOtherUserDelete(): void + { + $currentUser = new User(); + $annotationUser = new User(); + + $this->token->method('getUser')->willReturn($currentUser); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->annotationVoter->vote($this->token, new Annotation($annotationUser), [AnnotationVoter::DELETE])); + } + + public function testVoteReturnsGrantedForAnnotationUserDelete(): void + { + $user = new User(); + + $this->token->method('getUser')->willReturn($user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->annotationVoter->vote($this->token, new Annotation($user), [AnnotationVoter::DELETE])); + } +} diff --git a/tests/Security/Voter/EntryVoterTest.php b/tests/Security/Voter/EntryVoterTest.php index 949858511..04e066880 100644 --- a/tests/Security/Voter/EntryVoterTest.php +++ b/tests/Security/Voter/EntryVoterTest.php @@ -146,4 +146,32 @@ class EntryVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::DELETE])); } + + public function testVoteReturnsDeniedForNonEntryUserListAnnotations(): void + { + $this->token->method('getUser')->willReturn(new User()); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::LIST_ANNOTATIONS])); + } + + public function testVoteReturnsGrantedForEntryUserListAnnotations(): void + { + $this->token->method('getUser')->willReturn($this->user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::LIST_ANNOTATIONS])); + } + + public function testVoteReturnsDeniedForNonEntryUserCreateAnnotations(): void + { + $this->token->method('getUser')->willReturn(new User()); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::CREATE_ANNOTATIONS])); + } + + public function testVoteReturnsGrantedForEntryUserCreateAnnotations(): void + { + $this->token->method('getUser')->willReturn($this->user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::CREATE_ANNOTATIONS])); + } }