From ecb8b8ff493b820886b9254c8a35b4fa7be05f27 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Tue, 11 Mar 2025 01:43:13 +0100 Subject: [PATCH] Add IsGranted to EntryRestController --- src/Controller/Api/EntryRestController.php | 59 ++++++------------- src/Controller/Api/WallabagRestController.php | 16 ----- src/Security/Voter/EntryVoter.php | 4 +- src/Security/Voter/MainVoter.php | 6 +- .../Api/EntryRestControllerTest.php | 13 ++-- tests/Security/Voter/EntryVoterTest.php | 14 +++++ tests/Security/Voter/MainVoterTest.php | 28 +++++++++ 7 files changed, 75 insertions(+), 65 deletions(-) diff --git a/src/Controller/Api/EntryRestController.php b/src/Controller/Api/EntryRestController.php index dedc2d14d..15b6d1c0e 100644 --- a/src/Controller/Api/EntryRestController.php +++ b/src/Controller/Api/EntryRestController.php @@ -8,6 +8,7 @@ use Nelmio\ApiDocBundle\Annotation\Operation; use OpenApi\Annotations as OA; use Pagerfanta\Pagerfanta; use Psr\Log\LoggerInterface; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; @@ -85,13 +86,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/exists.{_format}", name="api_get_entries_exists", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("LIST_ENTRIES") * * @return JsonResponse */ public function getEntriesExistsAction(Request $request, EntryRepository $entryRepository) { - $this->validateAuthentication(); - $returnId = (null === $request->query->get('return_id')) ? false : (bool) $request->query->get('return_id'); $hashedUrls = $request->query->all('hashed_urls'); @@ -300,13 +300,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries.{_format}", name="api_get_entries", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("LIST_ENTRIES") * * @return JsonResponse */ public function getEntriesAction(Request $request, EntryRepository $entryRepository) { - $this->validateAuthentication(); - $isArchived = (null === $request->query->get('archive')) ? null : (bool) $request->query->get('archive'); $isStarred = (null === $request->query->get('starred')) ? null : (bool) $request->query->get('starred'); $isPublic = (null === $request->query->get('public')) ? null : (bool) $request->query->get('public'); @@ -392,14 +391,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}.{_format}", name="api_get_entry", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("VIEW", subject="entry") * * @return JsonResponse */ public function getEntryAction(Entry $entry) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - return $this->sendResponse($entry); } @@ -436,14 +433,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}/export.{_format}", name="api_get_entry_export", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("VIEW", subject="entry") * * @return Response */ public function getEntryExportAction(Entry $entry, Request $request, EntriesExport $entriesExport) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - return $entriesExport ->setEntries($entry) ->updateTitle('entry') @@ -471,13 +466,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/list.{_format}", name="api_delete_entries_list", methods={"DELETE"}, defaults={"_format": "json"}) + * @IsGranted("DELETE_ENTRIES") * * @return JsonResponse */ public function deleteEntriesListAction(Request $request, EntryRepository $entryRepository, EventDispatcherInterface $eventDispatcher) { - $this->validateAuthentication(); - $urls = json_decode($request->query->get('urls', '[]')); if (empty($urls)) { @@ -495,7 +489,7 @@ class EntryRestController extends WallabagRestController $results[$key]['url'] = $url; - if (false !== $entry) { + if (false !== $entry && $this->authorizationChecker->isGranted('DELETE', $entry)) { // entry deleted, dispatch event about it! $eventDispatcher->dispatch(new EntryDeletedEvent($entry), EntryDeletedEvent::NAME); @@ -529,6 +523,7 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/lists.{_format}", name="api_post_entries_list", methods={"POST"}, defaults={"_format": "json"}) + * @IsGranted("CREATE_ENTRIES") * * @throws HttpException When limit is reached * @@ -536,8 +531,6 @@ class EntryRestController extends WallabagRestController */ public function postEntriesListAction(Request $request, EntryRepository $entryRepository, EventDispatcherInterface $eventDispatcher, ContentProxy $contentProxy) { - $this->validateAuthentication(); - $urls = json_decode($request->query->get('urls', '[]')); $limit = $this->getParameter('wallabag.api_limit_mass_actions'); @@ -714,6 +707,7 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries.{_format}", name="api_post_entries", methods={"POST"}, defaults={"_format": "json"}) + * @IsGranted("CREATE_ENTRIES") * * @return JsonResponse */ @@ -726,8 +720,6 @@ class EntryRestController extends WallabagRestController EventDispatcherInterface $eventDispatcher, ValidatorInterface $validator, ) { - $this->validateAuthentication(); - $url = $request->request->get('url'); $entry = $entryRepository->findByUrlAndUserId( @@ -939,14 +931,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}.{_format}", name="api_patch_entries", methods={"PATCH"}, defaults={"_format": "json"}) + * @IsGranted("EDIT", subject="entry") * * @return JsonResponse */ public function patchEntriesAction(Entry $entry, Request $request, ContentProxy $contentProxy, LoggerInterface $logger, TagsAssigner $tagsAssigner, EventDispatcherInterface $eventDispatcher) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - $data = $this->retrieveValueFromRequest($request); // this is a special case where user want to manually update the entry content @@ -1056,14 +1046,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}/reload.{_format}", name="api_patch_entries_reload", methods={"PATCH"}, defaults={"_format": "json"}) + * @IsGranted("RELOAD", subject="entry") * * @return JsonResponse */ public function patchEntriesReloadAction(Entry $entry, ContentProxy $contentProxy, LoggerInterface $logger, EventDispatcherInterface $eventDispatcher) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - try { $contentProxy->updateEntry($entry, $entry->getUrl()); } catch (\Exception $e) { @@ -1113,6 +1101,7 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}.{_format}", name="api_delete_entries", methods={"DELETE"}, defaults={"_format": "json"}) + * @IsGranted("DELETE", subject="entry") * * @return JsonResponse */ @@ -1122,8 +1111,6 @@ class EntryRestController extends WallabagRestController if (!\in_array($expect, ['id', 'entry'], true)) { throw new BadRequestHttpException(\sprintf("expect: 'id' or 'entry' expected, %s given", $expect)); } - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); $response = $this->sendResponse([ 'id' => $entry->getId(), @@ -1166,14 +1153,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}/tags.{_format}", name="api_get_entries_tags", methods={"GET"}, defaults={"_format": "json"}) + * @IsGranted("LIST_TAGS", subject="entry") * * @return JsonResponse */ public function getEntriesTagsAction(Entry $entry) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - return $this->sendResponse($entry->getTags()); } @@ -1210,14 +1195,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}/tags.{_format}", name="api_post_entries_tags", methods={"POST"}, defaults={"_format": "json"}) + * @IsGranted("TAG", subject="entry") * * @return JsonResponse */ public function postEntriesTagsAction(Request $request, Entry $entry, TagsAssigner $tagsAssigner) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - $tags = $request->request->get('tags', ''); if (!empty($tags)) { $tagsAssigner->assignTagsToEntry($entry, $tags); @@ -1262,14 +1245,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/{entry}/tags/{tag}.{_format}", name="api_delete_entries_tags", methods={"DELETE"}, defaults={"_format": "json"}) + * @IsGranted("UNTAG", subject="entry") * * @return JsonResponse */ public function deleteEntriesTagsAction(Entry $entry, Tag $tag) { - $this->validateAuthentication(); - $this->validateUserAccess($entry->getUser()->getId()); - $entry->removeTag($tag); $this->entityManager->persist($entry); @@ -1298,13 +1279,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/tags/list.{_format}", name="api_delete_entries_tags_list", methods={"DELETE"}, defaults={"_format": "json"}) + * @IsGranted("DELETE_TAGS") * * @return JsonResponse */ public function deleteEntriesTagsListAction(Request $request, TagRepository $tagRepository, EntryRepository $entryRepository) { - $this->validateAuthentication(); - $list = json_decode($request->query->get('list', '[]')); if (empty($list)) { @@ -1325,7 +1305,7 @@ class EntryRestController extends WallabagRestController $tags = $element->tags; - if (false !== $entry && !(empty($tags))) { + if (false !== $entry && !(empty($tags)) && $this->authorizationChecker->isGranted('UNTAG', $entry)) { $tags = explode(',', $tags); foreach ($tags as $label) { $label = trim($label); @@ -1365,13 +1345,12 @@ class EntryRestController extends WallabagRestController * ) * * @Route("/api/entries/tags/lists.{_format}", name="api_post_entries_tags_list", methods={"POST"}, defaults={"_format": "json"}) + * @IsGranted("CREATE_TAGS") * * @return JsonResponse */ public function postEntriesTagsListAction(Request $request, EntryRepository $entryRepository, TagsAssigner $tagsAssigner) { - $this->validateAuthentication(); - $list = json_decode($request->query->get('list', '[]')); if (empty($list)) { @@ -1392,7 +1371,7 @@ class EntryRestController extends WallabagRestController $tags = $element->tags; - if (false !== $entry && !(empty($tags))) { + if (false !== $entry && !(empty($tags)) && $this->authorizationChecker->isGranted('TAG', $entry)) { $tagsAssigner->assignTagsToEntry($entry, $tags); $this->entityManager->persist($entry); diff --git a/src/Controller/Api/WallabagRestController.php b/src/Controller/Api/WallabagRestController.php index 615878bc6..47987094e 100644 --- a/src/Controller/Api/WallabagRestController.php +++ b/src/Controller/Api/WallabagRestController.php @@ -101,22 +101,6 @@ class WallabagRestController extends AbstractFOSRestController } } - /** - * Validate that the first id is equal to the second one. - * If not, throw exception. It means a user try to access information from an other user. - * - * @param int $requestUserId User id from the requested source - */ - protected function validateUserAccess($requestUserId) - { - $user = $this->tokenStorage->getToken()->getUser(); - \assert($user instanceof User); - - if ($requestUserId !== $user->getId()) { - throw $this->createAccessDeniedException('Access forbidden. Entry user id: ' . $requestUserId . ', logged user id: ' . $user->getId()); - } - } - /** * Shortcut to send data serialized in json. * diff --git a/src/Security/Voter/EntryVoter.php b/src/Security/Voter/EntryVoter.php index e2c845648..a9b354fea 100644 --- a/src/Security/Voter/EntryVoter.php +++ b/src/Security/Voter/EntryVoter.php @@ -20,6 +20,7 @@ class EntryVoter extends Voter public const DELETE = 'DELETE'; public const LIST_ANNOTATIONS = 'LIST_ANNOTATIONS'; public const CREATE_ANNOTATIONS = 'CREATE_ANNOTATIONS'; + public const LIST_TAGS = 'LIST_TAGS'; public const TAG = 'TAG'; public const UNTAG = 'UNTAG'; @@ -29,7 +30,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::EXPORT, self::DELETE, self::LIST_ANNOTATIONS, self::CREATE_ANNOTATIONS, self::TAG, self::UNTAG], true)) { + if (!\in_array($attribute, [self::VIEW, self::EDIT, self::RELOAD, self::STAR, self::ARCHIVE, self::SHARE, self::UNSHARE, self::EXPORT, self::DELETE, self::LIST_ANNOTATIONS, self::CREATE_ANNOTATIONS, self::LIST_TAGS, self::TAG, self::UNTAG], true)) { return false; } @@ -58,6 +59,7 @@ class EntryVoter extends Voter case self::DELETE: case self::LIST_ANNOTATIONS: case self::CREATE_ANNOTATIONS: + case self::LIST_TAGS: case self::TAG: case self::UNTAG: return $user === $subject->getUser(); diff --git a/src/Security/Voter/MainVoter.php b/src/Security/Voter/MainVoter.php index 63db306ee..b1f918ece 100644 --- a/src/Security/Voter/MainVoter.php +++ b/src/Security/Voter/MainVoter.php @@ -13,8 +13,10 @@ class MainVoter extends Voter public const EDIT_ENTRIES = 'EDIT_ENTRIES'; public const EXPORT_ENTRIES = 'EXPORT_ENTRIES'; public const IMPORT_ENTRIES = 'IMPORT_ENTRIES'; + public const DELETE_ENTRIES = 'DELETE_ENTRIES'; public const LIST_TAGS = 'LIST_TAGS'; public const CREATE_TAGS = 'CREATE_TAGS'; + public const DELETE_TAGS = 'DELETE_TAGS'; public const LIST_SITE_CREDENTIALS = 'LIST_SITE_CREDENTIALS'; public const CREATE_SITE_CREDENTIALS = 'CREATE_SITE_CREDENTIALS'; public const EDIT_CONFIG = 'EDIT_CONFIG'; @@ -32,7 +34,7 @@ class MainVoter extends Voter return false; } - if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES, self::EXPORT_ENTRIES, self::IMPORT_ENTRIES, self::LIST_TAGS, self::CREATE_TAGS, self::LIST_SITE_CREDENTIALS, self::CREATE_SITE_CREDENTIALS, self::EDIT_CONFIG], true)) { + if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES, self::EXPORT_ENTRIES, self::IMPORT_ENTRIES, self::DELETE_ENTRIES, self::LIST_TAGS, self::CREATE_TAGS, self::DELETE_TAGS, self::LIST_SITE_CREDENTIALS, self::CREATE_SITE_CREDENTIALS, self::EDIT_CONFIG], true)) { return false; } @@ -47,8 +49,10 @@ class MainVoter extends Voter case self::EDIT_ENTRIES: case self::EXPORT_ENTRIES: case self::IMPORT_ENTRIES: + case self::DELETE_ENTRIES: case self::LIST_TAGS: case self::CREATE_TAGS: + case self::DELETE_TAGS: case self::LIST_SITE_CREDENTIALS: case self::CREATE_SITE_CREDENTIALS: case self::EDIT_CONFIG: diff --git a/tests/Controller/Api/EntryRestControllerTest.php b/tests/Controller/Api/EntryRestControllerTest.php index 33c6fa6c7..342f4424c 100644 --- a/tests/Controller/Api/EntryRestControllerTest.php +++ b/tests/Controller/Api/EntryRestControllerTest.php @@ -6,7 +6,6 @@ use Doctrine\ORM\EntityManagerInterface; use Symfony\Component\DependencyInjection\Container; use Wallabag\Entity\Entry; use Wallabag\Entity\Tag; -use Wallabag\Entity\User; use Wallabag\Helper\ContentProxy; class EntryRestControllerTest extends WallabagApiTestCase @@ -535,7 +534,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testDeleteEntry() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $entry = new Entry($em->getReference(User::class, 1)); + $entry = new Entry($this->user); $entry->setUrl('http://0.0.0.0/test-delete-entry'); $entry->setTitle('Test delete entry'); $em->persist($entry); @@ -569,7 +568,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testDeleteEntryExpectId() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $entry = new Entry($em->getReference(User::class, 1)); + $entry = new Entry($this->user); $entry->setUrl('http://0.0.0.0/test-delete-entry-id'); $em->persist($entry); $em->flush(); @@ -659,7 +658,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testPostSameEntry() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $entry = new Entry($em->getReference(User::class, $this->getUserId())); + $entry = new Entry($this->user); $entry->setUrl('https://www.20minutes.fr/sport/jo_2024/4095122-20240712-jo-paris-2024-saut-ange-bombe-comment-anne-hidalgo-va-plonger-seine-si-fait-vraiment'); $entry->setArchived(true); $entry->addTag((new Tag())->setLabel('google')); @@ -1355,7 +1354,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testDeleteEntriesTagsListAction() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $entry = new Entry($em->getReference(User::class, $this->getUserId())); + $entry = new Entry($this->user); $entry->setUrl('http://0.0.0.0/test-entry'); $entry->addTag((new Tag())->setLabel('foo-tag')); $entry->addTag((new Tag())->setLabel('bar-tag')); @@ -1423,7 +1422,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testDeleteEntriesListAction() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $em->persist((new Entry($em->getReference(User::class, $this->getUserId())))->setUrl('http://0.0.0.0/test-entry1')); + $em->persist((new Entry($this->user))->setUrl('http://0.0.0.0/test-entry1')); $em->flush(); @@ -1483,7 +1482,7 @@ class EntryRestControllerTest extends WallabagApiTestCase public function testRePostEntryAndReUsePublishedAt() { $em = $this->client->getContainer()->get(EntityManagerInterface::class); - $entry = new Entry($em->getReference(User::class, $this->getUserId())); + $entry = new Entry($this->user); $entry->setTitle('Antoine de Caunes : « Je veux avoir le droit de tâtonner »'); $entry->setContent('hihi'); $entry->setUrl('https://www.lemonde.fr/m-perso/article/2017/06/25/antoine-de-caunes-je-veux-avoir-le-droit-de-tatonner_5150728_4497916.html'); diff --git a/tests/Security/Voter/EntryVoterTest.php b/tests/Security/Voter/EntryVoterTest.php index bcdd61ce1..9cfb84645 100644 --- a/tests/Security/Voter/EntryVoterTest.php +++ b/tests/Security/Voter/EntryVoterTest.php @@ -189,6 +189,20 @@ class EntryVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::CREATE_ANNOTATIONS])); } + public function testVoteReturnsDeniedForNonEntryUserListTags(): void + { + $this->token->method('getUser')->willReturn(new User()); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::LIST_TAGS])); + } + + public function testVoteReturnsGrantedForEntryUserListTags(): void + { + $this->token->method('getUser')->willReturn($this->user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::LIST_TAGS])); + } + public function testVoteReturnsDeniedForNonEntryUserTag(): void { $this->token->method('getUser')->willReturn(new User()); diff --git a/tests/Security/Voter/MainVoterTest.php b/tests/Security/Voter/MainVoterTest.php index 96e685e82..45870574b 100644 --- a/tests/Security/Voter/MainVoterTest.php +++ b/tests/Security/Voter/MainVoterTest.php @@ -112,6 +112,20 @@ class MainVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::IMPORT_ENTRIES])); } + public function testVoteReturnsDeniedForNonUserDeleteEntries(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->mainVoter->vote($this->token, null, [MainVoter::DELETE_ENTRIES])); + } + + public function testVoteReturnsGrantedForUserDeleteEntries(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::DELETE_ENTRIES])); + } + public function testVoteReturnsDeniedForNonUserListTags(): void { $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); @@ -140,6 +154,20 @@ class MainVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::CREATE_TAGS])); } + public function testVoteReturnsDeniedForNonUserDeleteTags(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->mainVoter->vote($this->token, null, [MainVoter::DELETE_TAGS])); + } + + public function testVoteReturnsGrantedForUserDeleteTags(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::DELETE_TAGS])); + } + public function testVoteReturnsDeniedForNonUserListSiteCredentials(): void { $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false);