EZP-29742: Implement permissions for Content/Create in Content item view #713

mikadamczyk · 2018-11-13T07:59:56Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-29742
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	yes/no
License	GPL-2.0

This PR adds additional checks when a user tries to add new content. If the user has no permission to create content in location than "create button" is disabled. When user can create content only in some of languages or CT those lists are limited.

https://github.com/ezsystems/ezplatform-page-builder/pull/262

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

webhdx

A few remarks + my initial comments reported via Slack.

webhdx · 2018-11-14T08:38:14Z

src/lib/Form/Type/ChoiceList/Provider/PermissionAwareContentTypeChoiceListProvider.php

+use EzSystems\EzPlatformAdminUi\Util\PermissionUtilInterface;
+use eZ\Publish\API\Repository\Values\ContentType\ContentType;
+
+class PermissionAwareContentTypeChoiceListProvider implements ChoiceListProviderInterface


No need to provide your own interface: Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface

webhdx · 2018-11-14T08:40:57Z

src/lib/Util/PermissionUtilInterface.php

+
+use eZ\Publish\API\Repository\Values\Content\Location;
+
+interface PermissionUtilInterface


Utility class with an interface is bad by a design. This is clearly more than a utility class thus it can be renamed to e.g. PermissionChecker. Also I'd think about extracting some parts to other classes i.e. part related to user groups.

webhdx · 2018-11-14T08:43:36Z

src/lib/Util/PermissionUtil.php

@@ -18,8 +141,14 @@ class PermissionUtil
     *
     * @return array
     */
-    public function flattenArrayOfLimitations(array $hasAccess): array
+    private function flattenArrayOfLimitationsForCurrentUser(array $hasAccess): array


If you go as far as changing class name please leave original PermissionUtil in place to avoid breaking changes (here you are changing method visibility).

webhdx · 2018-11-14T08:44:52Z

src/bundle/Resources/config/services.yml

+
+    EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Provider\ContentTypeChoiceListProvider: ~
+
+    EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Provider\LanguageChoiceListProvider:


Idea behind services/ directory was that we put service definitions there to avoid services.yml getting bloated.

adamwojs · 2018-11-15T09:24:27Z

src/lib/Form/Type/ChoiceList/Loader/ContentTypeChoiceLoader.php

+    }
+
+    /**
+     * @return array


Suggested change

* @return array

{@inheritdoc}

adamwojs · 2018-11-15T09:24:51Z

src/lib/Form/Type/ChoiceList/Loader/LanguageChoiceLoader.php

+    }
+
+    /**
+     * @return array


Suggested change

* @return array

* {@inheritdoc}

adamwojs · 2018-11-15T09:26:02Z

src/lib/Form/Type/Content/Draft/ContentCreateType.php

    }

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
+        dump($this->languageChoiceLoader);


Suggested change

dump($this->languageChoiceLoader);

adamwojs · 2018-11-15T09:27:29Z

src/lib/UniversalDiscovery/Event/Subscriber/ContentCreate.php

+    }
+
+    /**
+     * @return array


Suggested change

* @return array

* {@inheritdoc}

adamwojs · 2018-11-15T09:29:59Z

src/lib/Form/Type/ChoiceList/Loader/LanguageChoiceLoader.php

+    /** @var \eZ\Publish\API\Repository\LanguageService */
+    protected $languageService;
+
+    /** @var array */


Suggested change

/** @var array */

/** @var string[] */

adamwojs · 2018-11-15T09:38:59Z

src/lib/Permission/PermissionChecker.php

+                return strpos($location->pathString, $restrictedSubtree) === 0;
+            }));
+
+        return $canCreateInParentContentType


There is no need to check further criterions if $canCreateInParentContentType fails, etc. so we should return false immediately without further checks.

webhdx · 2018-11-15T10:24:17Z

src/bundle/Resources/config/services/permissions.yml

+        public: false
+
+    EzSystems\EzPlatformAdminUi\Permission\:
+        resource: "../../../lib/Permission"


Quite risky. Given Permission is a broad namespace you might not want every class to become a service.

webhdx · 2018-11-15T10:30:30Z

src/lib/Form/Type/ContentType/ContentTypeChoiceType.php

-
-                    return $contentTypes;
-                }),
+                'choice_loader' => $this->contentTypeChoiceLoader,


Does it need to be registered as a service? I'm pretty sure in Symfony's codebase they are initializing ChoiceLoaders on demand which for me makes sense as ChoiceLoader living in the container wastes memory. Large number of services makes your application slower.

Okay it would make Type class bloated with dependencies so I'm fine with your code.

webhdx · 2018-11-15T10:32:33Z

src/lib/Permission/PermissionChecker.php

+
+class PermissionChecker implements PermissionCheckerInterface
+{
+    private const USER_GROUPS_LIMIT = 1;


Limit of one will make large number of queries to the database.

webhdx · 2018-11-15T10:34:13Z

src/lib/Permission/PermissionCheckerInterface.php

+     *
+     * @return bool
+     */
+    public function canCreateInLocation(Location $location, $hasAccess): bool;


I'd like to hear others opinion on this but for me Permission namespace is very broad so PermissionCheckerInterface having canCreateInLocation method makes it badly designed. It should has more specific name i.e. LocationBasedPermissionCheckerInterface or the method should be a part of completely different interface.

You might be right, but for now, it's fine. If this class will begin to gain weight then we will refactor it by splitting into more specific classes.

barbaragr · 2018-11-16T15:26:38Z

Works fine. After rebasing and retesting https://github.com/ezsystems/ezplatform-page-builder/pull/262#issuecomment-439427895 it can be approved by QA.

…iew - refactor for assign section

…iew - part 1

…iew - choice loaders

…iew - content create policy in UDW

mikadamczyk self-assigned this Nov 13, 2018

mikadamczyk requested review from adamwojs and ViniTou November 13, 2018 07:59

mikadamczyk force-pushed the EZP-29742-wip branch from 1435abc to 51d7a63 Compare November 13, 2018 08:04

mikadamczyk requested a review from webhdx November 13, 2018 13:25

mikadamczyk force-pushed the EZP-29742-wip branch from ab69c5d to ac60882 Compare November 13, 2018 14:37

webhdx suggested changes Nov 14, 2018

View reviewed changes

mikadamczyk force-pushed the EZP-29742-wip branch 3 times, most recently from 7fe4dae to 6c703dd Compare November 15, 2018 08:42

ezsystems deleted a comment from ezrobot Nov 15, 2018

mikadamczyk force-pushed the EZP-29742-wip branch from 6c703dd to a3a4bd8 Compare November 15, 2018 09:37

adamwojs reviewed Nov 15, 2018

View reviewed changes

mikadamczyk force-pushed the EZP-29742-wip branch from a3a4bd8 to aa79e65 Compare November 15, 2018 09:54

webhdx suggested changes Nov 15, 2018

View reviewed changes

mikadamczyk added the Ready for review label Nov 15, 2018

mikadamczyk force-pushed the EZP-29742-wip branch from aa79e65 to 73943b8 Compare November 15, 2018 11:36

mikadamczyk changed the title ~~[WiP] EZP-29742: Implement permissions for Content/Create in Content item view~~ EZP-29742: Implement permissions for Content/Create in Content item view Nov 15, 2018

webhdx approved these changes Nov 15, 2018

View reviewed changes

barbaragr self-assigned this Nov 16, 2018

adamwojs approved these changes Nov 16, 2018

View reviewed changes

mikadamczyk added 5 commits November 20, 2018 08:37

EZP-29742: Implement permissions for Content/Create in Content item view

86b4770

EZP-29742: Implement permissions for Content/Create in Content item v…

99411e6

…iew - refactor for assign section

EZP-29742: Implement permissions for Content/Create in Content item v…

916f410

…iew - part 1

EZP-29742: Implement permissions for Content/Create in Content item v…

afe3a5f

…iew - choice loaders

EZP-29742: Implement permissions for Content/Create in Content item v…

b169ec9

…iew - content create policy in UDW

mikadamczyk force-pushed the EZP-29742-wip branch from 73943b8 to b169ec9 Compare November 20, 2018 07:37

katarzynazawada added the QA approved label Nov 20, 2018

katarzynazawada approved these changes Nov 20, 2018

View reviewed changes

lserwatka merged commit deb399f into ezsystems:master Nov 20, 2018

ViniTou pushed a commit that referenced this pull request May 12, 2023

Merge branch '2.3' of ezsystems/ezplatform-admin-ui into 4.3 (#713)

ffcc71a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29742: Implement permissions for Content/Create in Content item view #713

EZP-29742: Implement permissions for Content/Create in Content item view #713

mikadamczyk commented Nov 13, 2018 •

edited

Loading

webhdx left a comment

webhdx Nov 14, 2018

webhdx Nov 14, 2018 •

edited

Loading

webhdx Nov 14, 2018

webhdx Nov 14, 2018

adamwojs Nov 15, 2018

adamwojs Nov 15, 2018

adamwojs Nov 15, 2018

adamwojs Nov 15, 2018

adamwojs Nov 15, 2018

adamwojs Nov 15, 2018

webhdx Nov 15, 2018

webhdx Nov 15, 2018 •

edited

Loading

webhdx Nov 15, 2018

webhdx Nov 15, 2018

adamwojs Nov 15, 2018

barbaragr commented Nov 16, 2018


		use eZ\Publish\API\Repository\Values\Content\Location;

		interface PermissionUtilInterface


		EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Provider\ContentTypeChoiceListProvider: ~

		EzSystems\EzPlatformAdminUi\Form\Type\ChoiceList\Provider\LanguageChoiceListProvider:

EZP-29742: Implement permissions for Content/Create in Content item view #713

EZP-29742: Implement permissions for Content/Create in Content item view #713

Conversation

mikadamczyk commented Nov 13, 2018 • edited Loading

Checklist:

webhdx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webhdx Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barbaragr commented Nov 16, 2018

mikadamczyk commented Nov 13, 2018 •

edited

Loading

webhdx Nov 14, 2018 •

edited

Loading

webhdx Nov 15, 2018 •

edited

Loading