From beaca32493fe32914b0724586549dabfe53b12c8 Mon Sep 17 00:00:00 2001 From: Yassine Guedidi Date: Mon, 5 Feb 2024 22:02:50 +0100 Subject: [PATCH] Use IsGranted in UserController --- app/config/security.yml | 1 - src/Controller/UserController.php | 5 ++ src/Security/Voter/AdminVoter.php | 47 ++++++++++++++++ src/Security/Voter/UserVoter.php | 53 ++++++++++++++++++ templates/User/edit.html.twig | 10 +++- templates/User/index.html.twig | 38 +++++++------ templates/layout.html.twig | 4 +- tests/Security/Voter/AdminVoterTest.php | 67 +++++++++++++++++++++++ tests/Security/Voter/UserVoterTest.php | 73 +++++++++++++++++++++++++ 9 files changed, 277 insertions(+), 21 deletions(-) create mode 100644 src/Security/Voter/AdminVoter.php create mode 100644 src/Security/Voter/UserVoter.php create mode 100644 tests/Security/Voter/AdminVoterTest.php create mode 100644 tests/Security/Voter/UserVoterTest.php diff --git a/app/config/security.yml b/app/config/security.yml index 474774d41..76f10db70 100644 --- a/app/config/security.yml +++ b/app/config/security.yml @@ -76,6 +76,5 @@ security: - { path: ^/settings, roles: ROLE_SUPER_ADMIN } - { path: ^/annotations, roles: ROLE_USER } - { path: ^/2fa, role: IS_AUTHENTICATED_2FA_IN_PROGRESS } - - { path: ^/users, roles: ROLE_SUPER_ADMIN } - { path: ^/ignore-origin-instance-rules, roles: ROLE_SUPER_ADMIN } - { path: ^/, roles: ROLE_USER } diff --git a/src/Controller/UserController.php b/src/Controller/UserController.php index 49b914215..f9fba7b07 100644 --- a/src/Controller/UserController.php +++ b/src/Controller/UserController.php @@ -10,6 +10,7 @@ use Pagerfanta\Doctrine\ORM\QueryAdapter as DoctrineORMAdapter; use Pagerfanta\Exception\OutOfRangeCurrentPageException; use Pagerfanta\Pagerfanta; use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\Google\GoogleAuthenticatorInterface; +use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\Form\Form; use Symfony\Component\Form\FormInterface; @@ -41,6 +42,7 @@ class UserController extends AbstractController * Creates a new User entity. * * @Route("/users/new", name="user_new", methods={"GET", "POST"}) + * @IsGranted("CREATE_USERS") */ public function newAction(Request $request, UserManagerInterface $userManager, EventDispatcherInterface $eventDispatcher) { @@ -77,6 +79,7 @@ class UserController extends AbstractController * Displays a form to edit an existing User entity. * * @Route("/users/{id}/edit", name="user_edit", methods={"GET", "POST"}) + * @IsGranted("EDIT", subject="user") */ public function editAction(Request $request, User $user, UserManagerInterface $userManager, GoogleAuthenticatorInterface $googleAuthenticator) { @@ -119,6 +122,7 @@ class UserController extends AbstractController * Deletes a User entity. * * @Route("/users/{id}", name="user_delete", methods={"DELETE"}) + * @IsGranted("DELETE", subject="user") */ public function deleteAction(Request $request, User $user) { @@ -142,6 +146,7 @@ class UserController extends AbstractController * @param int $page * * @Route("/users/list/{page}", name="user_index", defaults={"page" = 1}) + * @IsGranted("LIST_USERS") * * Default parameter for page is hardcoded (in duplication of the defaults from the Route) * because this controller is also called inside the layout template without any page as argument diff --git a/src/Security/Voter/AdminVoter.php b/src/Security/Voter/AdminVoter.php new file mode 100644 index 000000000..2351ec4cc --- /dev/null +++ b/src/Security/Voter/AdminVoter.php @@ -0,0 +1,47 @@ +security = $security; + } + + protected function supports(string $attribute, $subject): bool + { + if (!\in_array($attribute, [self::LIST_USERS, self::CREATE_USERS], true)) { + return false; + } + + return true; + } + + protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool + { + $user = $token->getUser(); + + if (!$user instanceof User) { + return false; + } + + switch ($attribute) { + case self::LIST_USERS: + case self::CREATE_USERS: + return $this->security->isGranted('ROLE_SUPER_ADMIN'); + } + + return false; + } +} diff --git a/src/Security/Voter/UserVoter.php b/src/Security/Voter/UserVoter.php new file mode 100644 index 000000000..876f29aff --- /dev/null +++ b/src/Security/Voter/UserVoter.php @@ -0,0 +1,53 @@ +security = $security; + } + + protected function supports(string $attribute, $subject): bool + { + if (!$subject instanceof User) { + return false; + } + + if (!\in_array($attribute, [self::EDIT, self::DELETE], true)) { + return false; + } + + return true; + } + + protected function voteOnAttribute(string $attribute, $subject, TokenInterface $token): bool + { + $user = $token->getUser(); + \assert($user instanceof User); + + switch ($attribute) { + case self::EDIT: + return $this->security->isGranted('ROLE_SUPER_ADMIN'); + case self::DELETE: + if ($user === $subject) { + return false; + } + + return $this->security->isGranted('ROLE_SUPER_ADMIN'); + } + + return false; + } +} diff --git a/templates/User/edit.html.twig b/templates/User/edit.html.twig index 1181c60bc..7bf52f77a 100644 --- a/templates/User/edit.html.twig +++ b/templates/User/edit.html.twig @@ -66,9 +66,13 @@ {{ form_widget(edit_form._token) }}

- {{ form_start(delete_form) }} - - {{ form_end(delete_form) }} + {% if is_granted('DELETE', user) %} + {{ form_start(delete_form) }} + + {{ form_end(delete_form) }} + {% else %} + + {% endif %}

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

diff --git a/templates/User/index.html.twig b/templates/User/index.html.twig index 91ab41abd..20c037590 100644 --- a/templates/User/index.html.twig +++ b/templates/User/index.html.twig @@ -15,21 +15,23 @@

{{ 'user.description'|trans|raw }}

-
-
- {% if form_errors(searchForm) %} - {{ form_errors(searchForm) }} - {% endif %} + {% if is_granted('LIST_USERS') %} +
+ + {% if form_errors(searchForm) %} + {{ form_errors(searchForm) }} + {% endif %} - {% if form_errors(searchForm.term) %} - {{ form_errors(searchForm.term) }} - {% endif %} + {% if form_errors(searchForm.term) %} + {{ form_errors(searchForm.term) }} + {% endif %} - {{ form_widget(searchForm.term, {'attr': {'autocomplete': 'off', 'placeholder': 'user.search.placeholder'}}) }} + {{ form_widget(searchForm.term, {'attr': {'autocomplete': 'off', 'placeholder': 'user.search.placeholder'}}) }} - {{ form_rest(searchForm) }} - -
+ {{ form_rest(searchForm) }} + +
+ {% endif %}
@@ -48,16 +50,20 @@ {% endfor %}
{{ user.email }} {% if user.lastLogin %}{{ user.lastLogin|date('Y-m-d H:i:s') }}{% endif %} - {{ 'user.list.edit_action'|trans }} + {% if is_granted('EDIT', user) %} + {{ 'user.list.edit_action'|trans }} + {% endif %}

-

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

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

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

+ {% endif %} {% if users.getNbPages > 1 %} {{ pagerfanta(users, 'default_wallabag') }} {% endif %} diff --git a/templates/layout.html.twig b/templates/layout.html.twig index bf2753049..5acca264a 100644 --- a/templates/layout.html.twig +++ b/templates/layout.html.twig @@ -124,8 +124,10 @@
  • vpn_key {{ 'menu.left.site_credentials'|trans }}
    • {% endif %}
  • - {% if is_granted('ROLE_SUPER_ADMIN') %} + {% if is_granted('LIST_USERS') %}
  • people{{ 'menu.left.users_management'|trans }}
    • + {% endif %} + {% if is_granted('ROLE_SUPER_ADMIN') %}
  • settings {{ 'menu.left.internal_settings'|trans }}
  • build {{ 'menu.left.ignore_origin_instance_rules'|trans }}
  • diff --git a/tests/Security/Voter/AdminVoterTest.php b/tests/Security/Voter/AdminVoterTest.php new file mode 100644 index 000000000..50f9ec39d --- /dev/null +++ b/tests/Security/Voter/AdminVoterTest.php @@ -0,0 +1,67 @@ +security = $this->createMock(Security::class); + + $this->token = $this->createMock(TokenInterface::class); + $this->token->method('getUser')->willReturn(new User()); + + $this->adminVoter = new AdminVoter($this->security); + } + + public function testVoteReturnsAbstainForInvalidAttribute(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->adminVoter->vote($this->token, null, ['INVALID'])); + } + + public function testVoteReturnsDeniedForInvalidUser(): void + { + $this->token->method('getUser')->willReturn(new \stdClass()); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->adminVoter->vote($this->token, null, [AdminVoter::LIST_USERS])); + } + + public function testVoteReturnsDeniedForNonSuperAdminListUsers(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->adminVoter->vote($this->token, null, [AdminVoter::LIST_USERS])); + } + + public function testVoteReturnsGrantedForSuperAdminListUsers(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->adminVoter->vote($this->token, null, [AdminVoter::LIST_USERS])); + } + + public function testVoteReturnsDeniedForNonSuperAdminCreateUsers(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->adminVoter->vote($this->token, null, [AdminVoter::CREATE_USERS])); + } + + public function testVoteReturnsGrantedForSuperAdminCreateUsers(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->adminVoter->vote($this->token, null, [AdminVoter::CREATE_USERS])); + } +} diff --git a/tests/Security/Voter/UserVoterTest.php b/tests/Security/Voter/UserVoterTest.php new file mode 100644 index 000000000..d76f61a22 --- /dev/null +++ b/tests/Security/Voter/UserVoterTest.php @@ -0,0 +1,73 @@ +security = $this->createMock(Security::class); + + $this->token = $this->createMock(TokenInterface::class); + $this->token->method('getUser')->willReturn(new User()); + + $this->userVoter = new UserVoter($this->security); + } + + public function testVoteReturnsAbstainForInvalidSubject(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->userVoter->vote($this->token, new \stdClass(), [UserVoter::EDIT])); + } + + public function testVoteReturnsAbstainForInvalidAttribute(): void + { + $this->assertSame(VoterInterface::ACCESS_ABSTAIN, $this->userVoter->vote($this->token, new User(), ['INVALID'])); + } + + public function testVoteReturnsDeniedForNonSuperAdminEdit(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->userVoter->vote($this->token, new User(), [UserVoter::EDIT])); + } + + public function testVoteReturnsGrantedForSuperAdminEdit(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->userVoter->vote($this->token, new User(), [UserVoter::EDIT])); + } + + public function testVoteReturnsDeniedForSelfDelete(): void + { + $user = new User(); + $this->token->method('getUser')->willReturn($user); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->userVoter->vote($this->token, $user, [UserVoter::DELETE])); + } + + public function testVoteReturnsDeniedForNonSuperAdminDelete(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(false); + + $this->assertSame(VoterInterface::ACCESS_DENIED, $this->userVoter->vote($this->token, new User(), [UserVoter::DELETE])); + } + + public function testVoteReturnsGrantedForSuperAdminDelete(): void + { + $this->security->method('isGranted')->with('ROLE_SUPER_ADMIN')->willReturn(true); + + $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->userVoter->vote($this->token, new User(), [UserVoter::DELETE])); + } +}