improve permission screen (#2177)

src/Controller/PermissionController.php

@@ -25,7 +25,9 @@ use Sensio\Bundle\FrameworkExtraBundle\Configuration\Security;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
 use Symfony\Component\Routing\Annotation\Route;
+use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
 
 /**
  * Controller used to manage user roles and role permissions.
@@ -35,6 +37,7 @@ use Symfony\Component\Routing\Annotation\Route;
  */
 final class PermissionController extends AbstractController
 {
+    public const TOKEN_NAME = 'user_role_permissions';
     /**
      * @var RoleService
      */
@@ -59,7 +62,7 @@ final class PermissionController extends AbstractController
      * @Route(path="", name="admin_user_permissions", methods={"GET", "POST"})
      * @Security("is_granted('role_permissions')")
      */
-    public function permissions(EventDispatcherInterface $dispatcher)
+    public function permissions(EventDispatcherInterface $dispatcher, CsrfTokenManagerInterface $csrfTokenManager)
     {
         $all = $this->roleRepository->findAll();
         $existing = [];
@@ -158,6 +161,7 @@ final class PermissionController extends AbstractController
         $dispatcher->dispatch($event);
 
         return $this->render('user/permissions.html.twig', [
+            'token' => $csrfTokenManager->refreshToken(self::TOKEN_NAME)->getValue(),
             'roles' => array_values($roles),
             'sorted' => $event->getPermissions(),
             'manager' => $this->manager,
@@ -199,11 +203,20 @@ final class PermissionController extends AbstractController
     }
 
     /**
-     * @Route(path="/roles/{id}/delete", name="admin_user_role_delete", methods={"GET", "POST"})
+     * @Route(path="/roles/{id}/delete/{token}", name="admin_user_role_delete", methods={"GET", "POST"})
      * @Security("is_granted('role_permissions')")
      */
-    public function deleteRole(Role $role, UserRepository $userRepository): Response
+    public function deleteRole(Role $role, string $token, UserRepository $userRepository, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
+        if (!$this->isCsrfTokenValid(self::TOKEN_NAME, $token)) {
+            $this->flashUpdateException(new \Exception('Invalid CSRF token'));
+
+            return $this->redirectToRoute('admin_user_permissions');
+        }
+
+        // make sure that the token can only be used once, so refresh it after successful submission
+        $csrfTokenManager->refreshToken(self::TOKEN_NAME)->getValue();
+
         try {
             // workaround, as roles is still a string array on users table
             // until this is fixed, the users must be manually updated
@@ -222,11 +235,15 @@ final class PermissionController extends AbstractController
     }
 
     /**
-     * @Route(path="/roles/{id}/{name}/{value}", name="admin_user_permission_save", methods={"GET"})
+     * @Route(path="/roles/{id}/{name}/{value}/{token}", name="admin_user_permission_save", methods={"POST"})
      * @Security("is_granted('role_permissions')")
      */
-    public function savePermission(Role $role, string $name, bool $value, RolePermissionRepository $rolePermissionRepository): Response
+    public function savePermission(Role $role, string $name, bool $value, string $token, RolePermissionRepository $rolePermissionRepository, CsrfTokenManagerInterface $csrfTokenManager): Response
     {
+        if (!$this->isCsrfTokenValid(self::TOKEN_NAME, $token)) {
+            throw new BadRequestHttpException('Invalid CSRF token');
+        }
+
         if (!$this->manager->isRegisteredPermission($name)) {
             throw $this->createNotFoundException('Unknown permission: ' . $name);
         }
@@ -245,11 +262,16 @@ final class PermissionController extends AbstractController
             $permission->setAllowed((bool) $value);
 
             $rolePermissionRepository->saveRolePermission($permission);
-            $this->flashSuccess('action.update.success');
+
+            // refreshToken instead of getToken for more security but worse UX
+            // fast clicking with slow response times would fail, as the token cannot be replaced fast enough
+            $newToken = $csrfTokenManager->getToken(self::TOKEN_NAME)->getValue();
+
+            return $this->json(['token' => $newToken]);
         } catch (\Exception $ex) {
             $this->flashUpdateException($ex);
         }
 
-        return $this->redirectToRoute('admin_user_permissions');
+        throw new BadRequestHttpException();
     }
 }

templates/user/permissions.html.twig

@@ -12,7 +12,7 @@
     {% set options = {'class': 'alwaysVisible text-center'} %}
     {% if canEditPermissions and role.name not in system_roles|keys %}
         {% set widget %}
-        &nbsp;<a href="{{ path('admin_user_role_delete', {'id': role.id}) }}" class="confirmation-link" data-question="confirm.delete">{{ widgets.icon('trash') }}</a>
+        &nbsp;<a href="{{ path('admin_user_role_delete', {'id': role.id, 'token': token}) }}" class="confirmation-link" data-question="confirm.delete">{{ widgets.icon('trash') }}</a>
         {% endset %}
         {% set options = options|merge({'html_after': widget}) %}
     {% endif %}
@@ -47,10 +47,10 @@
                                 {% if value %}
                                     {{ widgets.label('yes'|trans, 'warning') }}
                                 {% else %}
-                                    <a href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': '1'}) }}">{{ widgets.label('no'|trans, 'danger') }}</a>
+                                    <a class="togglePerm permOff" href="#" data-href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': '1', 'token': '__TOKEN__'}) }}">{{ widgets.label('no'|trans, 'danger') }}</a>
                                 {% endif %}
                             {% else %}
-                                <a href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': (value ? '0' : '1')}) }}">{{ widgets.label_boolean(value) }}</a>
+                                <a class="togglePerm {{ value ? 'permOn' : 'permOff' }}" href="#" data-href="{{ path('admin_user_permission_save', {'id': role.id, 'name': permission, 'value': '__VALUE__', 'token': '__TOKEN__'}) }}">{{ widgets.label_boolean(value) }}</a>
                             {% endif %}
                         </td>
                     {% endfor %}
@@ -66,8 +66,51 @@
 {% block javascripts %}
     {{ parent() }}
     <script type="text/javascript">
+        let PERM_TOKEN = '{{ token }}';
         document.addEventListener('kimai.initialized', function() {
+            jQuery('a.togglePerm').on('click', function(event) {
+                event.stopPropagation();
+                event.preventDefault();
+
+                let target = event.target;
+                if (!target.matches('a')) {
+                    target = target.parentNode;
+                }
+
+                let linkElement = jQuery(target);
+                linkElement.html('<i class="fas fa-spinner fa-pulse"></i>');
+
+                let isActive = linkElement.hasClass('permOn');
+                let url = linkElement.data('href').replace(/__VALUE__/, isActive ? '0' : '1').replace(/__TOKEN__/, PERM_TOKEN);
+
+                jQuery.ajax({
+                    url: url,
+                    headers: {'Content-Type':'application/json'},
+                    method: 'POST',
+                    data: [], {# data doesn't matter, everything is in the URL #}
+                    dataType: 'json',
+                    success: function(result) {
+                        kimai.getPlugin('alert').success('action.update.success');
+                        linkElement.toggleClass('permOn').toggleClass('permOff');
+                        toggleLabel(linkElement, !isActive);
+                        PERM_TOKEN = result.token;
+                    },
+                    error: function(xhr, err) {
+                        kimai.getPlugin('alert').error('action.update.error');
+                        toggleLabel(linkElement, isActive);
+                    }
+                });
+
                 KimaiReloadPageWidget.create('kimai.userRoleUpdate');
             });
+        });
+
+        function toggleLabel(element, showTrue) {
+            if (showTrue) {
+                element.html('{{ widgets.label_boolean(true)|e('js') }}');
+            } else {
+                element.html('{{ widgets.label_boolean(false)|e('js') }}');
+            }
+        }
     </script>
 {% endblock %}

tests/Controller/PermissionControllerTest.php

@@ -12,6 +12,7 @@ namespace App\Tests\Controller;
 use App\DataFixtures\UserFixtures;
 use App\Entity\RolePermission;
 use App\Entity\User;
+use Symfony\Component\Security\Csrf\CsrfToken;
 
 /**
  * @group integration
@@ -82,7 +83,7 @@ class PermissionControllerTest extends ControllerBaseTest
 
     public function testDeleteRoleIsSecured()
     {
-        $this->assertUrlIsSecured('/admin/permissions/roles/1/delete');
+        $this->assertUrlIsSecured('/admin/permissions/roles/1/delete/sdfsdfsdfsd');
     }
 
     public function testDeleteRoleIsSecuredForRole()
@@ -123,7 +124,9 @@ class PermissionControllerTest extends ControllerBaseTest
         $user = $this->getUserByName(UserFixtures::USERNAME_USER);
         $this->assertEquals(['ROLE_TEAMLEAD', 'ROLE_SUPER_ADMIN', 'TEST_ROLE', 'ROLE_USER'], $user->getRoles());
 
-        $this->request($client, '/admin/permissions/roles/1/delete');
+        /** @var CsrfToken $token */
+        $token = static::$kernel->getContainer()->get('security.csrf.token_manager')->getToken('user_role_permissions');
+        $this->request($client, '/admin/permissions/roles/1/delete/' . $token->getValue());
         $this->assertIsRedirect($client, $this->createUrl('/admin/permissions'));
         $client->followRedirect();
 
@@ -138,7 +141,7 @@ class PermissionControllerTest extends ControllerBaseTest
 
     public function testSavePermissionIsSecured()
     {
-        $this->assertUrlIsSecured('/admin/permissions/roles/1/view_user/1');
+        $this->assertUrlIsSecured('/admin/permissions/roles/1/view_user/1/asdfasdf', 'POST');
     }
 
     public function testSavePermissionIsSecuredForRole()
@@ -157,15 +160,20 @@ class PermissionControllerTest extends ControllerBaseTest
             ]
         ]);
         $this->assertIsRedirect($client, $this->createUrl('/admin/permissions'));
+        $client->followRedirect();
 
         $em = $this->getEntityManager();
         $rolePermissions = $em->getRepository(RolePermission::class)->findAll();
         $this->assertEquals(0, \count($rolePermissions));
 
         // create the permission
-        $this->request($client, '/admin/permissions/roles/1/view_user/1');
+        $token = static::$kernel->getContainer()->get('security.csrf.token_manager')->getToken('user_role_permissions');
-        $this->assertIsRedirect($client, $this->createUrl('/admin/permissions'));
+        $this->request($client, '/admin/permissions/roles/1/view_user/1/' . $token->getValue(), 'POST');
-        $client->followRedirect();
+
+        self::assertTrue($client->getResponse()->isSuccessful());
+        $result = json_decode($client->getResponse()->getContent(), true);
+        self::assertIsArray($result);
+        self::assertArrayHasKey('token', $result);
 
         $rolePermissions = $em->getRepository(RolePermission::class)->findAll();
         $this->assertEquals(1, \count($rolePermissions));
@@ -180,9 +188,13 @@ class PermissionControllerTest extends ControllerBaseTest
         $em->clear();
 
         // update the permission
-        $this->request($client, '/admin/permissions/roles/1/view_user/0');
+        $token = static::$kernel->getContainer()->get('security.csrf.token_manager')->getToken('user_role_permissions');
-        $this->assertIsRedirect($client, $this->createUrl('/admin/permissions'));
+        $this->request($client, '/admin/permissions/roles/1/view_user/0/' . $token->getValue(), 'POST');
-        $client->followRedirect();
+
+        self::assertTrue($client->getResponse()->isSuccessful());
+        $result = json_decode($client->getResponse()->getContent(), true);
+        self::assertIsArray($result);
+        self::assertArrayHasKey('token', $result);
 
         $rolePermissions = $em->getRepository(RolePermission::class)->findAll();
         $this->assertEquals(1, \count($rolePermissions));