From 0f6b10aa0a532534c40086e1a4ace94a68a14be5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julien=20Fastr=C3=A9?= Date: Fri, 18 Apr 2025 14:09:13 +0200 Subject: [PATCH] Refactor SavedExport permissions and voter logic Revised SavedExportVoter to improve consistency and streamline permission checks. Updated tests, controller logic, and templates to align with new voter structure and attributes. Fixed typos in permission constants and added checks for delete/edit actions in the UI. --- .../Controller/ExportController.php | 4 +-- .../views/SavedExport/index.html.twig | 13 +++++--- .../Authorization/SavedExportVoter.php | 13 ++++---- .../Authorization/SavedExportVoterTest.php | 32 +++++++++++++------ 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/Bundle/ChillMainBundle/Controller/ExportController.php b/src/Bundle/ChillMainBundle/Controller/ExportController.php index 70d7f10a8..6075f2f22 100644 --- a/src/Bundle/ChillMainBundle/Controller/ExportController.php +++ b/src/Bundle/ChillMainBundle/Controller/ExportController.php @@ -509,8 +509,8 @@ class ExportController extends AbstractController default => $this->savedExportOrExportGenerationRepository->findById($savedExportId), }; - if (null !== $savedExport && !$this->security->isGranted(SavedExportVoter::EDIT, $savedExport)) { - throw new AccessDeniedHttpException('saved export edition not allowed'); + if (null !== $savedExport && !$this->security->isGranted(SavedExportVoter::GENERATE, $savedExport)) { + throw new AccessDeniedHttpException('saved export generation not allowed'); } return $savedExport; diff --git a/src/Bundle/ChillMainBundle/Resources/views/SavedExport/index.html.twig b/src/Bundle/ChillMainBundle/Resources/views/SavedExport/index.html.twig index be5970d5c..54941f24f 100644 --- a/src/Bundle/ChillMainBundle/Resources/views/SavedExport/index.html.twig +++ b/src/Bundle/ChillMainBundle/Resources/views/SavedExport/index.html.twig @@ -26,9 +26,9 @@

{{ saved.title }}

- {% if app.user is same as saved.createdBy %} + {% if app.user is same as saved.user %}

- {% if app.user is same as saved.createdBy %}{{ 'saved_export.Owner'|trans }}{% endif %} + {% if app.user is same as saved.user %}{{ 'saved_export.Owner'|trans }}{% endif %}

{% endif %}

{{ saved.description|chill_markdown_to_html }}

@@ -58,9 +58,14 @@ {{ 'Actions'|trans }}
diff --git a/src/Bundle/ChillMainBundle/Security/Authorization/SavedExportVoter.php b/src/Bundle/ChillMainBundle/Security/Authorization/SavedExportVoter.php index d84b5a700..1d686dee4 100644 --- a/src/Bundle/ChillMainBundle/Security/Authorization/SavedExportVoter.php +++ b/src/Bundle/ChillMainBundle/Security/Authorization/SavedExportVoter.php @@ -16,17 +16,16 @@ use Chill\MainBundle\Entity\User; use Chill\MainBundle\Export\ExportManager; use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; use Symfony\Component\Security\Core\Authorization\Voter\Voter; -use Symfony\Component\Security\Core\Security; -class SavedExportVoter extends Voter +final class SavedExportVoter extends Voter { - final public const DELETE = 'CHLL_MAIN_EXPORT_SAVED_DELETE'; + final public const DELETE = 'CHILL_MAIN_EXPORT_SAVED_DELETE'; - final public const EDIT = 'CHLL_MAIN_EXPORT_SAVED_EDIT'; + final public const EDIT = 'CHILL_MAIN_EXPORT_SAVED_EDIT'; - final public const GENERATE = 'CHLL_MAIN_EXPORT_SAVED_GENERATE'; + final public const GENERATE = 'CHILL_MAIN_EXPORT_SAVED_GENERATE'; - final public const SHARE = 'CHLL_MAIN_EXPORT_SAVED_SHARE'; + final public const SHARE = 'CHILL_MAIN_EXPORT_SAVED_SHARE'; private const ALL = [ self::DELETE, @@ -35,7 +34,7 @@ class SavedExportVoter extends Voter self::SHARE, ]; - public function __construct(private ExportManager $exportManager) {} + public function __construct(private readonly ExportManager $exportManager) {} protected function supports($attribute, $subject): bool { diff --git a/src/Bundle/ChillMainBundle/Tests/Security/Authorization/SavedExportVoterTest.php b/src/Bundle/ChillMainBundle/Tests/Security/Authorization/SavedExportVoterTest.php index 39307ece9..efa2312cd 100644 --- a/src/Bundle/ChillMainBundle/Tests/Security/Authorization/SavedExportVoterTest.php +++ b/src/Bundle/ChillMainBundle/Tests/Security/Authorization/SavedExportVoterTest.php @@ -22,7 +22,6 @@ use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken; use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface; -use Symfony\Component\Security\Core\Security; /** * @internal @@ -36,18 +35,14 @@ class SavedExportVoterTest extends TestCase /** * @dataProvider voteProvider */ - public function testVote(string $attribute, SavedExport $savedExport, User $user, $expectedResult, ?bool $isGranted = null): void + public function testVote(string $attribute, mixed $savedExport, User $user, $expectedResult, ?bool $isGranted = null): void { - $security = $this->prophesize(Security::class); - if (null !== $isGranted) { - $security->isGranted(Argument::any(), Argument::any())->willReturn($isGranted); - } - $export = $this->prophesize(ExportInterface::class); $exportManager = $this->prophesize(ExportManager::class); - $exportManager->getExport($savedExport->getExportAlias())->willReturn($export->reveal()); + $exportManager->getExport('dummy_export')->willReturn($export->reveal()); + $exportManager->isGrantedForElement(Argument::any())->willReturn($isGranted); - $voter = new SavedExportVoter($exportManager->reveal(), $security->reveal()); + $voter = new SavedExportVoter($exportManager->reveal()); $token = new UsernamePasswordToken($user, 'default', ['ROLE_USER']); self::assertEquals($expectedResult, $voter->vote($token, $savedExport, [$attribute])); @@ -66,6 +61,25 @@ class SavedExportVoterTest extends TestCase $savedExport->setExportAlias('dummy_export'); $savedExport->setUser($userA); + // abstain + foreach ($alls as $attribute) { + yield [ + $attribute, + new \stdClass(), + $userA, + VoterInterface::ACCESS_ABSTAIN, + true, + ]; + } + + yield [ + 'dummy', + $savedExport, + $userA, + VoterInterface::ACCESS_ABSTAIN, + false, + ]; + foreach ($alls as $attribute) { yield [ $attribute,