From 4e7cfbf3c89f5c2cbd07927d52e21d0fbcbfe735 Mon Sep 17 00:00:00 2001
From: Andreas Schempp <andreas.schempp@terminal42.ch>
Date: Fri, 10 Jul 2020 09:05:46 +0200
Subject: [PATCH] Extend voter for all backend user permissions (see #1864)

Description
-----------

The `BackendAccessVoter` already allowed to vote on user permissions.

```php
// whether a user is allowed to access the news module in the back end
$security->isGranted('contao_user.modules', 'news');
```

This extends the existing voter to vote on remaining user access:

```php
// whether the user can access any field in tl_page
$security->isGranted('contao_user.can_edit_fields', 'tl_page');

// whether user can edit the given page (array of row or page model)
$security->isGranted('contao_user.can_edit_page', [/* row of page data */]);
$security->isGranted('contao_user.can_edit_page', $pageModel);
```

I wonder if we should add constants somewhere for these properties, if it's Symfony best practice to add something like `ContaoPermission` or `ContaoAccess` class similar to the (now deprecated) `ContaoEvents` just for the constants?

Commits
-------

2d02a80d Extend voter for all backend user permissions
7d07b1c5 Add example of attributes as constants
ec90c1ce Fixed CS and renamed constants class
66aa939f Added permission constants for all bundles
c43c139e Added explanations for permission check methods
9b333805 CS
42120c2d phpstan
8bced7a7 Improve naming
9f07fab3 Apply suggestions from code review

Co-authored-by: Leo Feyer <github@contao.org>
59532b99 Early-return on invalid user
---
 .../Security/ContaoCalendarPermissions.php    |  24 +++
 .../src/Security/ContaoCorePermissions.php    | 152 ++++++++++++++++++
 .../src/Security/Voter/BackendAccessVoter.php |  74 ++++++++-
 .../Security/Voter/BackendAccessVoterTest.php | 110 ++++++++++++-
 .../src/Security/ContaoFaqPermissions.php     |  20 +++
 .../src/Security/ContaoNewsPermissions.php    |  24 +++
 .../Security/ContaoNewsletterPermissions.php  |  20 +++
 7 files changed, 418 insertions(+), 6 deletions(-)
 create mode 100644 calendar-bundle/src/Security/ContaoCalendarPermissions.php
 create mode 100644 core-bundle/src/Security/ContaoCorePermissions.php
 create mode 100644 faq-bundle/src/Security/ContaoFaqPermissions.php
 create mode 100644 news-bundle/src/Security/ContaoNewsPermissions.php
 create mode 100644 newsletter-bundle/src/Security/ContaoNewsletterPermissions.php

diff --git a/calendar-bundle/src/Security/ContaoCalendarPermissions.php b/calendar-bundle/src/Security/ContaoCalendarPermissions.php
new file mode 100644
index 00000000000..0c75e988ba1
--- /dev/null
+++ b/calendar-bundle/src/Security/ContaoCalendarPermissions.php
@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CalendarBundle\Security;
+
+final class ContaoCalendarPermissions
+{
+    public const USER_CAN_EDIT_CALENDAR = 'contao_user.calendars';
+    public const USER_CAN_CREATE_CALENDARS = 'contao_user.calendarp.create';
+    public const USER_CAN_DELETE_CALENDARS = 'contao_user.calendarp.delete';
+
+    public const USER_CAN_EDIT_FEED = 'contao_user.calendarfeeds';
+    public const USER_CAN_CREATE_FEEDS = 'contao_user.calendarfeedp.create';
+    public const USER_CAN_DELETE_FEEDS = 'contao_user.calendarfeedp.delete';
+}
diff --git a/core-bundle/src/Security/ContaoCorePermissions.php b/core-bundle/src/Security/ContaoCorePermissions.php
new file mode 100644
index 00000000000..19410220145
--- /dev/null
+++ b/core-bundle/src/Security/ContaoCorePermissions.php
@@ -0,0 +1,152 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CoreBundle\Security;
+
+final class ContaoCorePermissions
+{
+    /**
+     * Access is granted if the current user can edit the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_EDIT_PAGE = 'contao_user.can_edit_page';
+
+    /**
+     * Access is granted if the current user can change the hierarchy of the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_EDIT_PAGE_HIERARCHY = 'contao_user.can_edit_page_hierarchy';
+
+    /**
+     * Access is granted if the current user can can delete the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_DELETE_PAGE = 'contao_user.can_delete_page';
+
+    /**
+     * Access is granted if the current user can edit articles of the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_EDIT_ARTICLES = 'contao_user.can_edit_articles';
+
+    /**
+     * Access is granted if the current user can change the hierarchy of articles of the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_EDIT_ARTICLE_HIERARCHY = 'contao_user.can_edit_article_hierarchy';
+
+    /**
+     * Access is granted if the current user can delete articles of the given page.
+     * Subject must be a PageModel or a tl_page record as array.
+     */
+    public const USER_CAN_DELETE_ARTICLES = 'contao_user.can_delete_articles';
+
+    /**
+     * Access is granted if the current user can upload files to the server.
+     */
+    public const USER_CAN_UPLOAD_FILES = 'contao_user.fop.f1';
+
+    /**
+     * Access is granted if the current user can edit, copy or move files and folders.
+     */
+    public const USER_CAN_RENAME_FILE = 'contao_user.fop.f2';
+
+    /**
+     * Access is granted if the current user can delete single files and empty folders.
+     */
+    public const USER_CAN_DELETE_FILE = 'contao_user.fop.f3';
+
+    /**
+     * Access is granted if the current user can delete folders including all files and subfolders.
+     */
+    public const USER_CAN_DELETE_RECURSIVELY = 'contao_user.fop.f4';
+
+    /**
+     * Access is granted if the current user can edit files in the source editor.
+     */
+    public const USER_CAN_EDIT_FILE = 'contao_user.fop.f5';
+
+    /**
+     * Access is granted if the current user can synchronize the file system with the database.
+     */
+    public const USER_CAN_SYNC_DBAFS = 'contao_user.fop.f6';
+
+    /**
+     * Access is granted if the current user can edit at least one field of the table.
+     * Subject must be a table name (e.g. "tl_page").
+     */
+    public const USER_CAN_EDIT_FIELDS_OF_TABLE = 'contao_user.can_edit_fields';
+
+    /**
+     * Access is granted if the current user can edit the field of a table.
+     * Subject must be a table and field separated by two colons (e.g. "tl_page::title").
+     */
+    public const USER_CAN_EDIT_FIELD_OF_TABLE = 'contao_user.alexf';
+
+    /**
+     * Access is granted if the current user can access the back end module.
+     * Subject must be a module name (e.g. "article").
+     */
+    public const USER_CAN_ACCESS_MODULE = 'contao_user.modules';
+
+    /**
+     * Access is granted if the current user can access the content element type.
+     * Subject must be a content element type (e.g. "text").
+     */
+    public const USER_CAN_ACCESS_ELEMENT_TYPE = 'contao_user.elements';
+
+    /**
+     * Access is granted if the current user can access the form field type.
+     * Subject must be a content element type (e.g. "hidden").
+     */
+    public const USER_CAN_ACCESS_FIELD_TYPE = 'contao_user.fields';
+
+    /**
+     * Access is granted if the current user can access the theme.
+     * Subject must be an ID of table tl_theme.
+     */
+    public const USER_CAN_ACCESS_THEME = 'contao_user.themes';
+
+    /**
+     * Access is granted if the current user can access the page type.
+     * Subject must be a page type as string (e.g. "regular").
+     */
+    public const USER_CAN_ACCESS_PAGE_TYPE = 'contao_user.alpty';
+
+    /**
+     * Access is granted if the given path is mounted for the current user.
+     * Subject must be path as string (e.g. "files/content/foo").
+     */
+    public const USER_CAN_ACCESS_PATH = 'contao_user.filemounts';
+
+    /**
+     * Access is granted if the current user can access the image size.
+     * Subject must be an image size ID from tl_image_size or a configuration name (e.g. "crop").
+     */
+    public const USER_CAN_ACCESS_IMAGE_SIZE = 'contao_user.imageSizes';
+
+    /**
+     * Access is granted if the current user can access the form.
+     * Subject must be a form ID from tl_form.
+     */
+    public const USER_CAN_ACCESS_FORM = 'contao_user.forms';
+
+    /**
+     * Access is granted if the current user can create forms.
+     */
+    public const USER_CAN_CREATE_FORMS = 'contao_user.formp.create';
+
+    /**
+     * Access is granted if the current user can delete forms.
+     */
+    public const USER_CAN_DELETE_FORMS = 'contao_user.formp.delete';
+}
diff --git a/core-bundle/src/Security/Voter/BackendAccessVoter.php b/core-bundle/src/Security/Voter/BackendAccessVoter.php
index 80ee6d71d2b..0e4d60cb804 100644
--- a/core-bundle/src/Security/Voter/BackendAccessVoter.php
+++ b/core-bundle/src/Security/Voter/BackendAccessVoter.php
@@ -13,11 +13,21 @@
 namespace Contao\CoreBundle\Security\Voter;
 
 use Contao\BackendUser;
+use Contao\PageModel;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\Voter;
 
 class BackendAccessVoter extends Voter
 {
+    private const PAGE_PERMISSIONS = [
+        'can_edit_page' => BackendUser::CAN_EDIT_PAGE,
+        'can_edit_page_hierarchy' => BackendUser::CAN_EDIT_PAGE_HIERARCHY,
+        'can_delete_page' => BackendUser::CAN_DELETE_PAGE,
+        'can_edit_articles' => BackendUser::CAN_EDIT_ARTICLES,
+        'can_edit_article_hierarchy' => BackendUser::CAN_EDIT_ARTICLE_HIERARCHY,
+        'can_delete_articles' => BackendUser::CAN_DELETE_ARTICLES,
+    ];
+
     protected function supports($attribute, $subject): bool
     {
         return \is_string($attribute) && 0 === strncmp($attribute, 'contao_user.', 12);
@@ -26,12 +36,72 @@ protected function supports($attribute, $subject): bool
     protected function voteOnAttribute($attribute, $subject, TokenInterface $token): bool
     {
         $user = $token->getUser();
-        [, $field] = explode('.', $attribute, 2);
 
-        if (!$user instanceof BackendUser || (!is_scalar($subject) && !\is_array($subject))) {
+        if (!$user instanceof BackendUser) {
+            return false;
+        }
+
+        $permission = explode('.', $attribute, 3);
+
+        if ('contao_user' !== $permission[0] || !isset($permission[1])) {
+            return false;
+        }
+
+        $field = $permission[1];
+
+        if (!$subject && isset($permission[2])) {
+            $subject = $permission[2];
+        }
+
+        if ('can_edit_fields' === $field) {
+            return $this->canEditFieldsOf($subject, $user);
+        }
+
+        if (isset(self::PAGE_PERMISSIONS[$field])) {
+            return $this->isAllowed($subject, self::PAGE_PERMISSIONS[$field], $user);
+        }
+
+        return $this->hasAccess($subject, $field, $user);
+    }
+
+    /**
+     * Checks the user permissions against a field in tl_user(_group).
+     */
+    private function hasAccess($subject, string $field, BackendUser $user): bool
+    {
+        if (!is_scalar($subject) && !\is_array($subject)) {
             return false;
         }
 
         return $user->hasAccess($subject, $field);
     }
+
+    /**
+     * Checks if the user has access to a given page (tl_page.includeChmod et al.).
+     */
+    private function isAllowed($subject, int $flag, BackendUser $user): bool
+    {
+        if ($subject instanceof PageModel) {
+            $subject->loadDetails();
+            $subject = $subject->row();
+        }
+
+        if (!\is_array($subject)) {
+            return false;
+        }
+
+        return $user->isAllowed($flag, $subject);
+    }
+
+    /**
+     * Checks if the user has access to any field of a table (against tl_user(_group).alexf).
+     */
+    private function canEditFieldsOf($subject, BackendUser $user): bool
+    {
+        if (!\is_string($subject)) {
+            return false;
+        }
+
+        return $user->canEditFieldsOf($subject);
+    }
 }
diff --git a/core-bundle/tests/Security/Voter/BackendAccessVoterTest.php b/core-bundle/tests/Security/Voter/BackendAccessVoterTest.php
index 1cf6e707cba..01caf65b967 100644
--- a/core-bundle/tests/Security/Voter/BackendAccessVoterTest.php
+++ b/core-bundle/tests/Security/Voter/BackendAccessVoterTest.php
@@ -15,6 +15,7 @@
 use Contao\BackendUser;
 use Contao\CoreBundle\Security\Voter\BackendAccessVoter;
 use Contao\CoreBundle\Tests\TestCase;
+use Contao\PageModel;
 use Symfony\Component\ExpressionLanguage\Expression;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
@@ -58,7 +59,7 @@ public function testDeniesAccessIfTheTokenDoesNotHaveABackendUser(): void
             ->willReturn(null)
         ;
 
-        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, 'alexf', ['contao_user.']));
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, 'foo', ['contao_user.alexf']));
     }
 
     public function testDeniesAccessIfTheSubjectIsNotAScalarOrArray(): void
@@ -70,7 +71,7 @@ public function testDeniesAccessIfTheSubjectIsNotAScalarOrArray(): void
             ->willReturn($this->createMock(BackendUser::class))
         ;
 
-        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, new \stdClass(), ['contao_user.']));
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, new \stdClass(), ['contao_user.alexf']));
     }
 
     public function testDeniesAccessIfTheUserObjectDeniesAccess(): void
@@ -89,7 +90,7 @@ public function testDeniesAccessIfTheUserObjectDeniesAccess(): void
             ->willReturn($user)
         ;
 
-        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, 'alexf', ['contao_user.']));
+        $this->assertSame(VoterInterface::ACCESS_DENIED, $this->voter->vote($token, 'foo', ['contao_user.alexf']));
     }
 
     public function testGrantsAccessIfTheUserObjectGrantsAccess(): void
@@ -98,6 +99,7 @@ public function testGrantsAccessIfTheUserObjectGrantsAccess(): void
         $user
             ->expects($this->once())
             ->method('hasAccess')
+            ->with('foo', 'alexf')
             ->willReturn(true)
         ;
 
@@ -108,6 +110,106 @@ public function testGrantsAccessIfTheUserObjectGrantsAccess(): void
             ->willReturn($user)
         ;
 
-        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, 'alexf', ['contao_user.']));
+        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, 'foo', ['contao_user.alexf']));
+    }
+
+    public function testGrantsAccessIfUserCanEditFieldsOfTable(): void
+    {
+        $user = $this->createMock(BackendUser::class);
+        $user
+            ->expects($this->once())
+            ->method('canEditFieldsOf')
+            ->with('tl_foobar')
+            ->willReturn(true)
+        ;
+
+        $token = $this->createMock(TokenInterface::class);
+        $token
+            ->expects($this->once())
+            ->method('getUser')
+            ->willReturn($user)
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, 'tl_foobar', ['contao_user.can_edit_fields']));
+    }
+
+    public function testGrantsAccessIfUserCanEditPageAsArray(): void
+    {
+        $user = $this->createMock(BackendUser::class);
+        $user
+            ->expects($this->once())
+            ->method('isAllowed')
+            ->with(1, [])
+            ->willReturn(true)
+        ;
+
+        $token = $this->createMock(TokenInterface::class);
+        $token
+            ->expects($this->once())
+            ->method('getUser')
+            ->willReturn($user)
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, [], ['contao_user.can_edit_page']));
+    }
+
+    public function testGrantsAccessIfUserCanEditPageModel(): void
+    {
+        $page = $this->createMock(PageModel::class);
+        $page
+            ->expects($this->once())
+            ->method('row')
+            ->willReturn([])
+        ;
+
+        $user = $this->createMock(BackendUser::class);
+        $user
+            ->expects($this->once())
+            ->method('isAllowed')
+            ->with(1, [])
+            ->willReturn(true)
+        ;
+
+        $token = $this->createMock(TokenInterface::class);
+        $token
+            ->expects($this->once())
+            ->method('getUser')
+            ->willReturn($user)
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, $page, ['contao_user.can_edit_page']));
+    }
+
+    /**
+     * @dataProvider isAllowedProvider
+     */
+    public function testGrantsAccessIfUserIsAllowed(string $field, int $permission): void
+    {
+        $user = $this->createMock(BackendUser::class);
+        $user
+            ->expects($this->once())
+            ->method('isAllowed')
+            ->with($permission, [])
+            ->willReturn(true)
+        ;
+
+        $token = $this->createMock(TokenInterface::class);
+        $token
+            ->expects($this->once())
+            ->method('getUser')
+            ->willReturn($user)
+        ;
+
+        $this->assertSame(VoterInterface::ACCESS_GRANTED, $this->voter->vote($token, [], ['contao_user.'.$field]));
+    }
+
+    public function isAllowedProvider(): \Generator
+    {
+        yield ['can_edit_page', 1];
+        yield ['can_edit_page_hierarchy', 2];
+        yield ['can_delete_page', 3];
+        yield ['can_edit_articles', 4];
+        yield ['can_edit_article_hierarchy', 5];
+        yield ['can_delete_articles', 6];
     }
 }
diff --git a/faq-bundle/src/Security/ContaoFaqPermissions.php b/faq-bundle/src/Security/ContaoFaqPermissions.php
new file mode 100644
index 00000000000..fcb7eaf9f1c
--- /dev/null
+++ b/faq-bundle/src/Security/ContaoFaqPermissions.php
@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\FaqBundle\Security;
+
+final class ContaoFaqPermissions
+{
+    public const USER_CAN_EDIT_CATEGORY = 'contao_user.faqs';
+    public const USER_CAN_CREATE_CATEGORIES = 'contao_user.faqp.create';
+    public const USER_CAN_DELETE_CATEGORIES = 'contao_user.faqp.delete';
+}
diff --git a/news-bundle/src/Security/ContaoNewsPermissions.php b/news-bundle/src/Security/ContaoNewsPermissions.php
new file mode 100644
index 00000000000..c19280dc760
--- /dev/null
+++ b/news-bundle/src/Security/ContaoNewsPermissions.php
@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\NewsBundle\Security;
+
+final class ContaoNewsPermissions
+{
+    public const USER_CAN_EDIT_ARCHIVE = 'contao_user.news';
+    public const USER_CAN_CREATE_ARCHIVES = 'contao_user.newp.create';
+    public const USER_CAN_DELETE_ARCHIVES = 'contao_user.newp.delete';
+
+    public const USER_CAN_EDIT_FEED = 'contao_user.newsfeeds';
+    public const USER_CAN_CREATE_FEEDS = 'contao_user.newsfeedp.create';
+    public const USER_CAN_DELETE_FEEDS = 'contao_user.newsfeedp.delete';
+}
diff --git a/newsletter-bundle/src/Security/ContaoNewsletterPermissions.php b/newsletter-bundle/src/Security/ContaoNewsletterPermissions.php
new file mode 100644
index 00000000000..a14e76c93b8
--- /dev/null
+++ b/newsletter-bundle/src/Security/ContaoNewsletterPermissions.php
@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\NewsletterBundle\Security;
+
+final class ContaoNewsletterPermissions
+{
+    public const USER_CAN_EDIT_CHANNEL = 'contao_user.newsletters';
+    public const USER_CAN_CREATE_CHANNELS = 'contao_user.newsletterp.create';
+    public const USER_CAN_DELETE_CHANNELS = 'contao_user.newsletterp.create';
+}