diff --git a/src/Controller/PermissionController.php b/src/Controller/PermissionController.php index bca4abaf..ac0c4b26 100644 --- a/src/Controller/PermissionController.php +++ b/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(); } } diff --git a/templates/user/permissions.html.twig b/templates/user/permissions.html.twig index 1be5d843..8f847567 100644 --- a/templates/user/permissions.html.twig +++ b/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 %} -  {{ widgets.icon('trash') }} +  {{ widgets.icon('trash') }} {% endset %} {% set options = options|merge({'html_after': widget}) %} {% endif %} @@ -47,10 +47,10 @@ {% if value %} {{ widgets.label('yes'|trans, 'warning') }} {% else %} - {{ widgets.label('no'|trans, 'danger') }} + {{ widgets.label('no'|trans, 'danger') }} {% endif %} {% else %} - {{ widgets.label_boolean(value) }} + {{ widgets.label_boolean(value) }} {% endif %} {% endfor %} @@ -66,8 +66,51 @@ {% block javascripts %} {{ parent() }} {% endblock %} diff --git a/tests/Controller/PermissionControllerTest.php b/tests/Controller/PermissionControllerTest.php index e29520b7..9bd42e94 100644 --- a/tests/Controller/PermissionControllerTest.php +++ b/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'); - $this->assertIsRedirect($client, $this->createUrl('/admin/permissions')); - $client->followRedirect(); + $token = static::$kernel->getContainer()->get('security.csrf.token_manager')->getToken('user_role_permissions'); + $this->request($client, '/admin/permissions/roles/1/view_user/1/' . $token->getValue(), 'POST'); + + 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'); - $this->assertIsRedirect($client, $this->createUrl('/admin/permissions')); - $client->followRedirect(); + $token = static::$kernel->getContainer()->get('security.csrf.token_manager')->getToken('user_role_permissions'); + $this->request($client, '/admin/permissions/roles/1/view_user/0/' . $token->getValue(), 'POST'); + + 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));