Merge pull request #7360 from yguedidi/use-isgranted-in-usercontroller

Use IsGranted in UserController

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 }

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

src/Security/Voter/AdminVoter.php

@@ -0,0 +1,47 @@
+<?php
+
+namespace Wallabag\Security\Voter;
+
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\Voter;
+use Symfony\Component\Security\Core\Security;
+use Wallabag\Entity\User;
+
+class AdminVoter extends Voter
+{
+    public const LIST_USERS = 'LIST_USERS';
+    public const CREATE_USERS = 'CREATE_USERS';
+
+    private Security $security;
+
+    public function __construct(Security $security)
+    {
+        $this->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;
+    }
+}

src/Security/Voter/UserVoter.php

@@ -0,0 +1,53 @@
+<?php
+
+namespace Wallabag\Security\Voter;
+
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\Voter;
+use Symfony\Component\Security\Core\Security;
+use Wallabag\Entity\User;
+
+class UserVoter extends Voter
+{
+    public const EDIT = 'EDIT';
+    public const DELETE = 'DELETE';
+
+    private Security $security;
+
+    public function __construct(Security $security)
+    {
+        $this->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;
+    }
+}

templates/User/edit.html.twig

@@ -66,9 +66,13 @@
                                 {{ form_widget(edit_form._token) }}
                             </form>
                             <p>
-                                {{ form_start(delete_form) }}
-                                    <button {% if app.user.id == user.id %}disabled="disabled"{% endif %} onclick="return confirm('{{ 'user.form.delete_confirm'|trans|escape('js') }}')" type="submit" class="btn waves-effect waves-light red">{{ 'user.form.delete'|trans }}</button>
-                                {{ form_end(delete_form) }}
+                                {% if is_granted('DELETE', user) %}
+                                    {{ form_start(delete_form) }}
+                                        <button onclick="return confirm('{{ 'user.form.delete_confirm'|trans|escape('js') }}')" type="submit" class="btn waves-effect waves-light red">{{ 'user.form.delete'|trans }}</button>
+                                    {{ form_end(delete_form) }}
+                                {% else %}
+                                    <button disabled="disabled" type="button" class="btn waves-effect waves-light red">{{ 'user.form.delete'|trans }}</button>
+                                {% endif %}
                             </p>
                             <p><a class="waves-effect waves-light btn blue-grey" href="{{ path('user_index') }}">{{ 'user.form.back_to_list'|trans }}</a></p>
                         </div>

templates/User/index.html.twig

@@ -15,21 +15,23 @@
                         <p class="help">{{ 'user.description'|trans|raw }}</p>
                     </div>
                     <div class="col s6">
-                        <div class="input-field">
-                            <form name="search_users" method="GET" action="{{ path('user_index') }}">
-                                {% if form_errors(searchForm) %}
-                                    <span class="black-text">{{ form_errors(searchForm) }}</span>
-                                {% endif %}
+                        {% if is_granted('LIST_USERS') %}
+                            <div class="input-field">
+                                <form name="search_users" method="GET" action="{{ path('user_index') }}">
+                                    {% if form_errors(searchForm) %}
+                                        <span class="black-text">{{ form_errors(searchForm) }}</span>
+                                    {% endif %}
 
-                                {% if form_errors(searchForm.term) %}
-                                    <span class="black-text">{{ form_errors(searchForm.term) }}</span>
-                                {% endif %}
+                                    {% if form_errors(searchForm.term) %}
+                                        <span class="black-text">{{ form_errors(searchForm.term) }}</span>
+                                    {% 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>
-                        </div>
+                                    {{ form_rest(searchForm) }}
+                                </form>
+                            </div>
+                        {% endif %}
                     </div>
 
                     <table class="bordered">
@@ -48,16 +50,20 @@
                                 <td>{{ user.email }}</td>
                                 <td>{% if user.lastLogin %}{{ user.lastLogin|date('Y-m-d H:i:s') }}{% endif %}</td>
                                 <td>
-                                    <a href="{{ path('user_edit', {'id': user.id}) }}">{{ 'user.list.edit_action'|trans }}</a>
+                                    {% if is_granted('EDIT', user) %}
+                                        <a href="{{ path('user_edit', {'id': user.id}) }}">{{ 'user.list.edit_action'|trans }}</a>
+                                    {% endif %}
                                 </td>
                             </tr>
                         {% endfor %}
                         </tbody>
                     </table>
                     <br />
-                    <p>
-                        <a href="{{ path('user_new') }}" class="waves-effect waves-light btn">{{ 'user.list.create_new_one'|trans }}</a>
-                    </p>
+                    {% if is_granted('CREATE_USERS') %}
+                        <p>
+                            <a href="{{ path('user_new') }}" class="waves-effect waves-light btn">{{ 'user.list.create_new_one'|trans }}</a>
+                        </p>
+                    {% endif %}
                     {% if users.getNbPages > 1 %}
                         {{ pagerfanta(users, 'default_wallabag') }}
                     {% endif %}

templates/layout.html.twig

@@ -124,8 +124,10 @@
                         <li><a href="{{ path('site_credentials_index') }}"><i class="material-icons">vpn_key</i> {{ 'menu.left.site_credentials'|trans }}</a></li>
                     {% endif %}
                     <li class="divider"></li>
-                    {% if is_granted('ROLE_SUPER_ADMIN') %}
+                    {% if is_granted('LIST_USERS') %}
                         <li><a href="{{ path('user_index') }}"><i class="material-icons">people</i>{{ 'menu.left.users_management'|trans }}</a></li>
+                    {% endif %}
+                    {% if is_granted('ROLE_SUPER_ADMIN') %}
                         <li><a href="{{ path('craue_config_settings_modify') }}"><i class="material-icons">settings</i> {{ 'menu.left.internal_settings'|trans }}</a></li>
                         <li><a href="{{ path('ignore_origin_instance_rules_index') }}"><i class="material-icons">build</i> {{ 'menu.left.ignore_origin_instance_rules'|trans }}</a></li>
                         <li class="divider"></li>

tests/Security/Voter/AdminVoterTest.php

@@ -0,0 +1,67 @@
+<?php
+
+namespace Tests\Wallabag\Security\Voter;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
+use Symfony\Component\Security\Core\Security;
+use Wallabag\Entity\User;
+use Wallabag\Security\Voter\AdminVoter;
+
+class AdminVoterTest extends TestCase
+{
+    private $security;
+    private $token;
+    private $adminVoter;
+
+    protected function setUp(): void
+    {
+        $this->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]));
+    }
+}

tests/Security/Voter/UserVoterTest.php

@@ -0,0 +1,73 @@
+<?php
+
+namespace Tests\Wallabag\Security\Voter;
+
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
+use Symfony\Component\Security\Core\Security;
+use Wallabag\Entity\User;
+use Wallabag\Security\Voter\UserVoter;
+
+class UserVoterTest extends TestCase
+{
+    private $security;
+    private $token;
+    private $userVoter;
+
+    protected function setUp(): void
+    {
+        $this->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]));
+    }
+}