From 247894209c2b22c7dd3a72911d1abde9734c0460 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Sat, 23 Mar 2024 23:36:33 +0100 Subject: [PATCH] Use IsGranted in SiteCredentialController --- src/Controller/SiteCredentialController.php | 21 ++----- src/Security/Voter/MainVoter.php | 6 +- src/Security/Voter/SiteCredentialVoter.php | 46 +++++++++++++++ templates/SiteCredential/edit.html.twig | 12 ++-- templates/SiteCredential/index.html.twig | 14 +++-- templates/layout.html.twig | 2 +- tests/Security/Voter/MainVoterTest.php | 28 +++++++++ .../Voter/SiteCredentialVoterTest.php | 57 +++++++++++++++++++ 8 files changed, 158 insertions(+), 28 deletions(-) create mode 100644 src/Security/Voter/SiteCredentialVoter.php create mode 100644 tests/Security/Voter/SiteCredentialVoterTest.php diff --git a/src/Controller/SiteCredentialController.php b/src/Controller/SiteCredentialController.php index c1b806f70..e9cfb203f 100644 --- a/src/Controller/SiteCredentialController.php +++ b/src/Controller/SiteCredentialController.php @@ -4,6 +4,7 @@ namespace Wallabag\Controller; use Craue\ConfigBundle\Util\Config; use Doctrine\ORM\EntityManagerInterface; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\Form\Form; use Symfony\Component\Form\FormInterface; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -41,6 +42,7 @@ class SiteCredentialController extends AbstractController * Lists all User entities. * * @Route("/", name="site_credentials_index", methods={"GET"}) + * @IsGranted("LIST_SITE_CREDENTIALS") */ public function indexAction(SiteCredentialRepository $repository) { @@ -57,6 +59,7 @@ class SiteCredentialController extends AbstractController * Creates a new site credential entity. * * @Route("/new", name="site_credentials_new", methods={"GET", "POST"}) + * @IsGranted("CREATE_SITE_CREDENTIALS") * * @return Response */ @@ -94,6 +97,7 @@ class SiteCredentialController extends AbstractController * Displays a form to edit an existing site credential entity. * * @Route("/{id}/edit", name="site_credentials_edit", methods={"GET", "POST"}) + * @IsGranted("EDIT", subject="siteCredential") * * @return Response */ @@ -101,8 +105,6 @@ class SiteCredentialController extends AbstractController { $this->isSiteCredentialsEnabled(); - $this->checkUserAction($siteCredential); - $deleteForm = $this->createDeleteForm($siteCredential); $editForm = $this->createForm(SiteCredentialType::class, $siteCredential); $editForm->handleRequest($request); @@ -133,6 +135,7 @@ class SiteCredentialController extends AbstractController * Deletes a site credential entity. * * @Route("/{id}", name="site_credentials_delete", methods={"DELETE"}) + * @IsGranted("DELETE", subject="siteCredential") * * @return RedirectResponse */ @@ -140,8 +143,6 @@ class SiteCredentialController extends AbstractController { $this->isSiteCredentialsEnabled(); - $this->checkUserAction($siteCredential); - $form = $this->createDeleteForm($siteCredential); $form->handleRequest($request); @@ -183,16 +184,4 @@ class SiteCredentialController extends AbstractController ->getForm() ; } - - /** - * Check if the logged user can manage the given site credential. - * - * @param SiteCredential $siteCredential The site credential entity - */ - private function checkUserAction(SiteCredential $siteCredential) - { - if (null === $this->getUser() || $this->getUser()->getId() !== $siteCredential->getUser()->getId()) { - throw $this->createAccessDeniedException('You can not access this site credential.'); - } - } } diff --git a/src/Security/Voter/MainVoter.php b/src/Security/Voter/MainVoter.php index 015bac4b3..b036cc0cb 100644 --- a/src/Security/Voter/MainVoter.php +++ b/src/Security/Voter/MainVoter.php @@ -11,6 +11,8 @@ class MainVoter extends Voter public const LIST_ENTRIES = 'LIST_ENTRIES'; public const CREATE_ENTRIES = 'CREATE_ENTRIES'; public const EDIT_ENTRIES = 'EDIT_ENTRIES'; + public const LIST_SITE_CREDENTIALS = 'LIST_SITE_CREDENTIALS'; + public const CREATE_SITE_CREDENTIALS = 'CREATE_SITE_CREDENTIALS'; private Security $security; @@ -25,7 +27,7 @@ class MainVoter extends Voter return false; } - if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES], true)) { + if (!\in_array($attribute, [self::LIST_ENTRIES, self::CREATE_ENTRIES, self::EDIT_ENTRIES, self::LIST_SITE_CREDENTIALS, self::CREATE_SITE_CREDENTIALS], true)) { return false; } @@ -38,6 +40,8 @@ class MainVoter extends Voter case self::LIST_ENTRIES: case self::CREATE_ENTRIES: case self::EDIT_ENTRIES: + case self::LIST_SITE_CREDENTIALS: + case self::CREATE_SITE_CREDENTIALS: return $this->security->isGranted('ROLE_USER'); } diff --git a/src/Security/Voter/SiteCredentialVoter.php b/src/Security/Voter/SiteCredentialVoter.php new file mode 100644 index 000000000..35ab28484 --- /dev/null +++ b/src/Security/Voter/SiteCredentialVoter.php @@ -0,0 +1,46 @@ +getUser(); + + if (!$user instanceof User) { + return false; + } + + switch ($attribute) { + case self::EDIT: + case self::DELETE: + return $user === $subject->getUser(); + } + + return false; + } +} diff --git a/templates/SiteCredential/edit.html.twig b/templates/SiteCredential/edit.html.twig index aec6f9480..04d8f066b 100644 --- a/templates/SiteCredential/edit.html.twig +++ b/templates/SiteCredential/edit.html.twig @@ -44,11 +44,13 @@ {{ form_widget(edit_form.save, {'attr': {'class': 'btn waves-effect waves-light'}}) }} {{ form_widget(edit_form._token) }} -

- {{ form_start(delete_form) }} - - {{ form_end(delete_form) }} -

+ {% if is_granted('DELETE', credential) %} +

+ {{ form_start(delete_form) }} + + {{ form_end(delete_form) }} +

+ {% endif %}

{{ 'site_credential.form.back_to_list'|trans }}

diff --git a/templates/SiteCredential/index.html.twig b/templates/SiteCredential/index.html.twig index a85118a5a..5da1a8b0a 100644 --- a/templates/SiteCredential/index.html.twig +++ b/templates/SiteCredential/index.html.twig @@ -23,16 +23,20 @@ {{ credential.host }} - {{ 'site_credential.list.edit_action'|trans }} + {% if is_granted('EDIT', credential) %} + {{ 'site_credential.list.edit_action'|trans }} + {% endif %} {% endfor %} -
-

- {{ 'site_credential.list.create_new_one'|trans }} -

+ {% if is_granted('CREATE_SITE_CREDENTIALS') %} +
+

+ {{ 'site_credential.list.create_new_one'|trans }} +

+ {% endif %} diff --git a/templates/layout.html.twig b/templates/layout.html.twig index 1711b48f6..98e1adfaa 100644 --- a/templates/layout.html.twig +++ b/templates/layout.html.twig @@ -126,7 +126,7 @@
  • settings {{ 'menu.left.config'|trans }}
  • smartphone {{ 'menu.left.developer'|trans }}
  • import_export {{ 'menu.left.import'|trans }}
    • - {% if craue_setting('restricted_access') %} + {% if craue_setting('restricted_access') and is_granted('LIST_SITE_CREDENTIALS') %}
  • vpn_key {{ 'menu.left.site_credentials'|trans }}
    • {% endif %}
  • diff --git a/tests/Security/Voter/MainVoterTest.php b/tests/Security/Voter/MainVoterTest.php index ba7b6a5a7..eec969c3c 100644 --- a/tests/Security/Voter/MainVoterTest.php +++ b/tests/Security/Voter/MainVoterTest.php @@ -83,4 +83,32 @@ class MainVoterTest extends TestCase $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::EDIT_ENTRIES])); } + + public function testVoteReturnsDeniedForNonUserListSiteCredentials(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->mainVoter->vote($this->token, null, [MainVoter::LIST_SITE_CREDENTIALS])); + } + + public function testVoteReturnsGrantedForUserListSiteCredentials(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::LIST_SITE_CREDENTIALS])); + } + + public function testVoteReturnsDeniedForNonUserCreateSiteCredentials(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->mainVoter->vote($this->token, null, [MainVoter::CREATE_SITE_CREDENTIALS])); + } + + public function testVoteReturnsGrantedForUserCreateSiteCredentials(): void + { + $this->security->method('isGranted')->with('ROLE_USER')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->mainVoter->vote($this->token, null, [MainVoter::CREATE_SITE_CREDENTIALS])); + } } diff --git a/tests/Security/Voter/SiteCredentialVoterTest.php b/tests/Security/Voter/SiteCredentialVoterTest.php new file mode 100644 index 000000000..dadfebe40 --- /dev/null +++ b/tests/Security/Voter/SiteCredentialVoterTest.php @@ -0,0 +1,57 @@ +user = new User(); + + $this->token = $this->createMock(TokenInterface::class); + $this->token->method('getUser')->willReturn($this->user); + + $this->siteCredentialVoter = new SiteCredentialVoter(); + } + + public function testVoteReturnsAbstainForInvalidSubject(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->siteCredentialVoter->vote($this->token, new \stdClass(), [SiteCredentialVoter::EDIT])); + } + + public function testVoteReturnsAbstainForInvalidAttribute(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->siteCredentialVoter->vote($this->token, new SiteCredential(new User()), ['INVALID'])); + } + + public function testVoteReturnsDeniedForNonSiteCredentialUserEdit(): void + { + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->siteCredentialVoter->vote($this->token, new SiteCredential(new User()), [SiteCredentialVoter::EDIT])); + } + + public function testVoteReturnsGrantedForSiteCredentialUserEdit(): void + { + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->siteCredentialVoter->vote($this->token, new SiteCredential($this->user), [SiteCredentialVoter::EDIT])); + } + + public function testVoteReturnsDeniedForNonSiteCredentialUserDelete(): void + { + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->siteCredentialVoter->vote($this->token, new SiteCredential(new User()), [SiteCredentialVoter::DELETE])); + } + + public function testVoteReturnsGrantedForSiteCredentialUserDelete(): void + { + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->siteCredentialVoter->vote($this->token, new SiteCredential($this->user), [SiteCredentialVoter::DELETE])); + } +}