From d2dd7f78d31323c221c9f74a597328f7530dc8fd Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Mon, 10 Mar 2025 23:17:43 +0100 Subject: [PATCH] Add IsGranted to ExportController --- src/Controller/ExportController.php | 21 ++++-------- src/Security/Voter/EntryVoter.php | 4 ++- src/Security/Voter/MainVoter.php | 4 ++- templates/Entry/entries.html.twig | 44 +++++++++++++------------ templates/Entry/entry.html.twig | 36 ++++++++++---------- tests/Security/Voter/EntryVoterTest.php | 14 ++++++++ tests/Security/Voter/MainVoterTest.php | 14 ++++++++ 7 files changed, 83 insertions(+), 54 deletions(-) diff --git a/src/Controller/ExportController.php b/src/Controller/ExportController.php index d48b54894..1a65c7a9e 100644 --- a/src/Controller/ExportController.php +++ b/src/Controller/ExportController.php @@ -2,10 +2,12 @@ namespace Wallabag\Controller; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Routing\Annotation\Route; +use Wallabag\Entity\Entry; use Wallabag\Helper\EntriesExport; use Wallabag\Repository\EntryRepository; use Wallabag\Repository\TagRepository; @@ -19,27 +21,17 @@ class ExportController extends AbstractController /** * Gets one entry content. * - * @Route("/export/{id}.{format}", name="export_entry", methods={"GET"}, requirements={ + * @Route("/export/{entry}.{format}", name="export_entry", methods={"GET"}, requirements={ * "format": "epub|pdf|json|xml|txt|csv|md", - * "id": "\d+" + * "entry": "\d+" * }) + * @IsGranted("EXPORT", subject="entry") * * @return Response */ - public function downloadEntryAction(Request $request, EntryRepository $entryRepository, EntriesExport $entriesExport, string $format, int $id) + public function downloadEntryAction(Request $request, EntryRepository $entryRepository, EntriesExport $entriesExport, string $format, Entry $entry) { try { - $entry = $entryRepository->find($id); - - /* - * We duplicate EntryController::checkUserAction here as a quick fix for an improper authorization vulnerability - * - * This should be eventually rewritten - */ - if (null === $entry || null === $this->getUser() || $this->getUser()->getId() !== $entry->getUser()->getId()) { - throw new NotFoundHttpException(); - } - return $entriesExport ->setEntries($entry) ->updateTitle('entry') @@ -57,6 +49,7 @@ class ExportController extends AbstractController * "format": "epub|pdf|json|xml|txt|csv|md", * "category": "all|unread|starred|archive|tag_entries|untagged|search|annotated|same_domain" * }) + * @IsGranted("EXPORT_ENTRIES") * * @return Response */ diff --git a/src/Security/Voter/EntryVoter.php b/src/Security/Voter/EntryVoter.php index baeca0408..11806ea19 100644 --- a/src/Security/Voter/EntryVoter.php +++ b/src/Security/Voter/EntryVoter.php @@ -16,6 +16,7 @@ class EntryVoter extends Voter public const ARCHIVE = 'ARCHIVE'; public const SHARE = 'SHARE'; public const UNSHARE = 'UNSHARE'; + public const EXPORT = 'EXPORT'; public const DELETE = 'DELETE'; public const LIST_ANNOTATIONS = 'LIST_ANNOTATIONS'; public const CREATE_ANNOTATIONS = 'CREATE_ANNOTATIONS'; @@ -26,7 +27,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, self::LIST_ANNOTATIONS, self::CREATE_ANNOTATIONS], 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], true)) { return false; } @@ -51,6 +52,7 @@ class EntryVoter extends Voter case self::ARCHIVE: case self::SHARE: case self::UNSHARE: + case self::EXPORT: case self::DELETE: case self::LIST_ANNOTATIONS: case self::CREATE_ANNOTATIONS: diff --git a/src/Security/Voter/MainVoter.php b/src/Security/Voter/MainVoter.php index b036cc0cb..e9b22ea90 100644 --- a/src/Security/Voter/MainVoter.php +++ b/src/Security/Voter/MainVoter.php @@ -11,6 +11,7 @@ class MainVoter extends Voter public const LIST_ENTRIES = 'LIST_ENTRIES'; public const CREATE_ENTRIES = 'CREATE_ENTRIES'; public const EDIT_ENTRIES = 'EDIT_ENTRIES'; + public const EXPORT_ENTRIES = 'EXPORT_ENTRIES'; public const LIST_SITE_CREDENTIALS = 'LIST_SITE_CREDENTIALS'; public const CREATE_SITE_CREDENTIALS = 'CREATE_SITE_CREDENTIALS'; @@ -27,7 +28,7 @@ class MainVoter extends Voter return false; } - if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES, self::LIST_SITE_CREDENTIALS, self::CREATE_SITE_CREDENTIALS], true)) { + if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES, self::EXPORT_ENTRIES, self::LIST_SITE_CREDENTIALS, self::CREATE_SITE_CREDENTIALS], true)) { return false; } @@ -40,6 +41,7 @@ class MainVoter extends Voter case self::LIST_ENTRIES: case self::CREATE_ENTRIES: case self::EDIT_ENTRIES: + case self::EXPORT_ENTRIES: case self::LIST_SITE_CREDENTIALS: case self::CREATE_SITE_CREDENTIALS: return $this->security->isGranted('ROLE_USER'); diff --git a/templates/Entry/entries.html.twig b/templates/Entry/entries.html.twig index 22b77a0c3..bb1653fec 100644 --- a/templates/Entry/entries.html.twig +++ b/templates/Entry/entries.html.twig @@ -86,28 +86,30 @@ {% endif %} -
- {% set current_tag = null %} - {% if tag is defined %} - {% set current_tag = tag.slug %} + {% if is_granted('EXPORT_ENTRIES') %} +
+ {% set current_tag = null %} + {% if tag is defined %} + {% set current_tag = tag.slug %} + {% endif %} + {% set export_search_term = null %} + {% if searchTerm is defined %} + {% set export_search_term = searchTerm %} + {% endif %} + {% set entry = app.request.attributes.get('entry') %} + {% set previous_route = app.request.attributes.get('currentRoute') %} +

{{ 'entry.list.export_title'|trans }}

+
    + {% if craue_setting('export_epub') %}
  • EPUB
    • {% endif %} + {% if craue_setting('export_pdf') %}
  • PDF
    • {% endif %} + {% if craue_setting('export_json') %}
  • JSON
    • {% endif %} + {% if craue_setting('export_csv') %}
  • CSV
    • {% endif %} + {% if craue_setting('export_txt') %}
  • TXT
    • {% endif %} + {% if craue_setting('export_xml') %}
  • XML
    • {% endif %} + {% if craue_setting('export_md') %}
  • Markdown
    • {% endif %} +
+
{% endif %} - {% set export_search_term = null %} - {% if searchTerm is defined %} - {% set export_search_term = searchTerm %} - {% endif %} - {% set entry = app.request.attributes.get('id') %} - {% set previous_route = app.request.attributes.get('currentRoute') %} -

{{ 'entry.list.export_title'|trans }}

- -
{% if form is not null and is_granted('LIST_ENTRIES') %} diff --git a/templates/Entry/entry.html.twig b/templates/Entry/entry.html.twig index 13bb249bc..12325c430 100644 --- a/templates/Entry/entry.html.twig +++ b/templates/Entry/entry.html.twig @@ -229,23 +229,25 @@ {% endif %} -
  • - - file_download - {{ 'entry.view.left_menu.export'|trans }} - -
    -
      - {% if craue_setting('export_epub') %}
    • EPUB
      • {% endif %} - {% if craue_setting('export_pdf') %}
    • PDF
      • {% endif %} - {% if craue_setting('export_csv') %}
    • CSV
      • {% endif %} - {% if craue_setting('export_json') %}
    • JSON
      • {% endif %} - {% if craue_setting('export_txt') %}
    • TXT
      • {% endif %} - {% if craue_setting('export_xml') %}
    • XML
      • {% endif %} - {% if craue_setting('export_md') %}
    • Markdown
      • {% endif %} -
    -
    -
    • + {% if is_granted('EXPORT', entry) %} +
  • + + file_download + {{ 'entry.view.left_menu.export'|trans }} + +
    +
      + {% if craue_setting('export_epub') %}
    • EPUB
      • {% endif %} + {% if craue_setting('export_pdf') %}
    • PDF
      • {% endif %} + {% if craue_setting('export_csv') %}
    • CSV
      • {% endif %} + {% if craue_setting('export_json') %}
    • JSON
      • {% endif %} + {% if craue_setting('export_txt') %}
    • TXT
      • {% endif %} + {% if craue_setting('export_xml') %}
    • XML
      • {% endif %} + {% if craue_setting('export_md') %}
    • Markdown
      • {% endif %} +
    +
    +
    • + {% endif %}
  • diff --git a/tests/Security/Voter/EntryVoterTest.php b/tests/Security/Voter/EntryVoterTest.php index 04e066880..1d6e0bbce 100644 --- a/tests/Security/Voter/EntryVoterTest.php +++ b/tests/Security/Voter/EntryVoterTest.php @@ -133,6 +133,20 @@ class EntryVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::UNSHARE])); } + public function testVoteReturnsDeniedForNonEntryUserExport(): void + { + $this->token->method('getUser')->willReturn(new User()); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::EXPORT])); + } + + public function testVoteReturnsGrantedForEntryUserExport(): void + { + $this->token->method('getUser')->willReturn($this->user); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->entryVoter->vote($this->token, $this->entry, [EntryVoter::EXPORT])); + } + public function testVoteReturnsDeniedForNonEntryUserDelete(): void { $this->token->method('getUser')->willReturn(new User()); diff --git a/tests/Security/Voter/MainVoterTest.php b/tests/Security/Voter/MainVoterTest.php index eec969c3c..2530c0336 100644 --- a/tests/Security/Voter/MainVoterTest.php +++ b/tests/Security/Voter/MainVoterTest.php @@ -84,6 +84,20 @@ class MainVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::EDIT_ENTRIES])); } + public function testVoteReturnsDeniedForNonUserExportEntries(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->mainVoter->vote($this->token, null, [MainVoter::EXPORT_ENTRIES])); + } + + public function testVoteReturnsGrantedForUserExportEntries(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::EXPORT_ENTRIES])); + } + public function testVoteReturnsDeniedForNonUserListSiteCredentials(): void { $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false);