EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them #2536

pawbuj · 2019-02-01T08:32:55Z

Question	Answer
JIRA issue	EZP-29539
Bug/Improvement	yes
New feature	no
Target version	`7.2`
BC breaks	no
Tests pass	yes
Doc needed	no

Checking permission to remove content for all elements in the subtree.

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.

mikadamczyk · 2019-02-01T08:50:53Z

eZ/Publish/Core/Repository/TrashService.php

-        $this->settings = $settings + array(
-            //'defaultSetting' => array(),
-        );
+        $this->settings = $settings + array(//'defaultSetting' => array(),


Suggested change

$this->settings = $settings + array(//'defaultSetting' => array(),

$this->settings = $settings;

eZ/Publish/Core/Repository/TrashService.php

mikadamczyk · 2019-02-01T08:52:11Z

eZ/Publish/Core/Repository/TrashService.php

@@ -294,7 +301,8 @@ public function findTrashItems(Query $query)

            foreach ($query->sortClauses as $sortClause) {
                if (!$sortClause instanceof SortClause) {
-                    throw new InvalidArgumentValue('query->sortClauses', 'only instances of SortClause class are allowed');
+                    throw new InvalidArgumentValue('query->sortClauses',


Please, fix CS

Actually, such changes shouldn't be in PR until they are necessary to fix a bug 😉

mikadamczyk · 2019-02-01T08:57:31Z

eZ/Publish/Core/Repository/TrashService.php

+
+        $contentRemoveCriterion = $this->permissionCriterionResolver->getPermissionsCriterion('content', 'remove');
+
+        if ($contentRemoveCriterion === false) {


maybe

Suggested change

if ($contentRemoveCriterion === false) {

if (is_bool($contentRemoveCriterion)) {

return $contentRemoveCriterion;

}

?

adamwojs

unit/integration tests are missing also CI failure is related.

adamwojs · 2019-02-01T12:45:48Z

eZ/Publish/Core/Repository/TrashService.php

@@ -63,15 +69,16 @@ public function __construct(
        RepositoryInterface $repository,
        Handler $handler,
        Helper\NameSchemaService $nameSchemaService,
-        array $settings = array()
+        array $settings = array(),
+        PermissionCriterionResolver $permissionCriterionResolver


There is no BC promise for constructors so please move $permissionCriterionResolver before $settings

adamwojs · 2019-02-01T12:47:16Z

eZ/Publish/Core/Repository/TrashService.php

@@ -12,6 +12,8 @@
 use eZ\Publish\API\Repository\Repository as RepositoryInterface;
 use eZ\Publish\API\Repository\Values\Content\Content;
 use eZ\Publish\API\Repository\Exceptions\UnauthorizedException as APIUnauthorizedException;
+use eZ\Publish\Core\REST\Client\Values\Content\ContentInfo;


Suggested change

use eZ\Publish\Core\REST\Client\Values\Content\ContentInfo;

mateuszbieniek · 2019-02-01T12:57:13Z

eZ/Publish/Core/Repository/TrashService.php

+            return false;
+        }
+
+        $contentRemoveCriterion = $this->permissionCriterionResolver->getPermissionsCriterion('content', 'remove');


Is there any scenario that this can return true?

according to docs, yes

ezpublish-kernel/eZ/Publish/API/Repository/PermissionCriterionResolver.php

Lines 16 to 26 in ae9dff7

/**

* Get criteria representation for a permission.

*

* Will return a criteria if current user has limited access to the given module/function,

* however if user has either full or no access then boolean is returned.

*

* @param string $module

* @param string $function

*

* @return bool|\eZ\Publish\API\Repository\Values\Content\Query\Criterion

*/

And that was my point ;)

andrerom

+1, but as this is bug it might be something that should be backported further back?

mikadamczyk · 2019-02-06T11:49:34Z

eZ/Publish/Core/Repository/TrashService.php

@@ -23,6 +24,10 @@
 use eZ\Publish\API\Repository\Values\Content\Trash\SearchResult;
 use eZ\Publish\API\Repository\Values\Content\Query\Criterion;
 use eZ\Publish\API\Repository\Values\Content\Query\SortClause;
+use eZ\Publish\API\Repository\PermissionCriterionResolver;
+use eZ\Publish\API\Repository\Values\Content\Query\Criterion\LogicalAnd as CriterionLogicalAnd;


Suggested change

use eZ\Publish\API\Repository\Values\Content\Query\Criterion\LogicalAnd as CriterionLogicalAnd;

use eZ\Publish\API\Repository\Values\Content\Query\Criterion;

in many places in the kernel it is used like this

lserwatka · 2019-02-19T12:47:55Z

@barbaragr @pawbuj what is the status here?

barbaragr · 2019-02-19T14:07:29Z

@lserwatka as Andre said: it might be something that should be backported further back, so I'm waiting for possibility to test on v 1.7. @pawbuj promised to let me know when it will be ready.

lserwatka · 2019-02-19T14:10:08Z

@andrerom but does it really make sense to back-port it to 1.7? 1.x is in SPA architecture, with zero permissions check, this might require work on frontend side to handle those exceptions properly. I don't see any benefit here especially that report was related to 2.x only.

andrerom · 2019-02-19T19:32:35Z

@lserwatka The way I read this it's a kernel bug affecting all use of the application, api's and UI, it can also be argued to be security issue. Did QA uncover any major 1.x UI issues happening with this? I would assume that is more in regards to how we handle this in REST API which is maybe anyway a todo here on this PR(?)

lserwatka · 2019-02-20T09:21:04Z

@andrerom we managed to reproduce it in 1.x too, unfortunately. @pawbuj could you change target branch to 6.7?

…er does not have permission to delete them

micszo

Retest OK on eZ Platform EE v1.7.8.
On v1.13.4 retesting before merge is blocked by http-cache dependencies.
Adding v2 test status in second PR.

lserwatka · 2019-02-27T14:07:02Z

Could you merge it up?

micszo · 2019-02-27T14:32:51Z

Retest OK on eZ Platform EE v1.13.4.

pawbuj · 2019-02-27T15:00:51Z

merged

pawbuj requested review from andrerom, alongosz, adamwojs, micszo, mikadamczyk, Nattfarinn, ViniTou and webhdx February 1, 2019 08:33

pawbuj self-assigned this Feb 1, 2019

pawbuj added Ready for review Bug labels Feb 1, 2019

mikadamczyk requested changes Feb 1, 2019

View reviewed changes

adamwojs requested changes Feb 1, 2019

View reviewed changes

mateuszbieniek reviewed Feb 1, 2019

View reviewed changes

pawbuj force-pushed the EZP-29539 branch 3 times, most recently from d17ca05 to eee0379 Compare February 6, 2019 07:38

alongosz approved these changes Feb 6, 2019

View reviewed changes

andrerom approved these changes Feb 6, 2019

View reviewed changes

mikadamczyk approved these changes Feb 6, 2019

View reviewed changes

ViniTou changed the title ~~EZP-29539: Deleting object will remove all subtree items even when us…~~ EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them Feb 6, 2019

adamwojs approved these changes Feb 11, 2019

View reviewed changes

pawbuj added Ready for QA and removed Ready for review labels Feb 11, 2019

barbaragr self-assigned this Feb 12, 2019

pawbuj changed the base branch from 7.2 to 6.7 February 20, 2019 21:00

pawbuj force-pushed the EZP-29539 branch from 39231f2 to 0cae71d Compare February 21, 2019 10:38

This comment has been minimized.

# to view

pawbuj force-pushed the EZP-29539 branch 2 times, most recently from f3f2a69 to 63f3721 Compare February 25, 2019 13:57

EZP-29539: Deleting object will remove all subtree items even when us…

d578e8a

…er does not have permission to delete them

pawbuj force-pushed the EZP-29539 branch from 63f3721 to d578e8a Compare February 26, 2019 14:19

fixes the failing test after rebase to 6.7

45ae5f9

pawbuj force-pushed the EZP-29539 branch from 0623ec3 to 509840f Compare February 26, 2019 20:55

removes syntax unsupported in php5

930e843

pawbuj force-pushed the EZP-29539 branch from 509840f to 930e843 Compare February 27, 2019 08:29

micszo self-assigned this Feb 27, 2019

micszo approved these changes Feb 27, 2019

View reviewed changes

lserwatka merged commit f2a100d into 6.7 Feb 27, 2019

lserwatka deleted the EZP-29539 branch February 27, 2019 14:06

micszo removed the Ready for QA label Feb 27, 2019

micszo removed their assignment Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them #2536

EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them #2536

pawbuj commented Feb 1, 2019 •

edited by ViniTou

Loading

mikadamczyk Feb 1, 2019

mikadamczyk Feb 1, 2019

adamwojs Feb 4, 2019

mikadamczyk Feb 1, 2019

adamwojs left a comment •

edited

Loading

adamwojs Feb 1, 2019

adamwojs Feb 1, 2019

mateuszbieniek Feb 1, 2019

pawbuj Feb 4, 2019 •

edited

Loading

mateuszbieniek Feb 19, 2019

andrerom left a comment

mikadamczyk Feb 6, 2019

lserwatka commented Feb 19, 2019

barbaragr commented Feb 19, 2019

lserwatka commented Feb 19, 2019 •

edited

Loading

andrerom commented Feb 19, 2019 •

edited

Loading

lserwatka commented Feb 20, 2019

This comment has been minimized.

micszo left a comment

lserwatka commented Feb 27, 2019

micszo commented Feb 27, 2019

pawbuj commented Feb 27, 2019

	$this->settings = $settings + array(//'defaultSetting' => array(),
	$this->settings = $settings;


		$contentRemoveCriterion = $this->permissionCriterionResolver->getPermissionsCriterion('content', 'remove');

		if ($contentRemoveCriterion === false) {

-        if ($contentRemoveCriterion === false) {
+        if (is_bool($contentRemoveCriterion)) {
+            return $contentRemoveCriterion;
+        }

	/**
	* Get criteria representation for a permission.
	*
	* Will return a criteria if current user has limited access to the given module/function,
	* however if user has either full or no access then boolean is returned.
	*
	* @param string $module
	* @param string $function
	*
	* @return bool\|\eZ\Publish\API\Repository\Values\Content\Query\Criterion
	*/

	use eZ\Publish\API\Repository\Values\Content\Query\Criterion\LogicalAnd as CriterionLogicalAnd;
	use eZ\Publish\API\Repository\Values\Content\Query\Criterion;

EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them #2536

EZP-29539: Deleting object will remove all subtree items even when user does not have permission to delete them #2536

Conversation

pawbuj commented Feb 1, 2019 • edited by ViniTou 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

adamwojs left a comment • 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

pawbuj Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lserwatka commented Feb 19, 2019

barbaragr commented Feb 19, 2019

lserwatka commented Feb 19, 2019 • edited Loading

andrerom commented Feb 19, 2019 • edited Loading

lserwatka commented Feb 20, 2019

This comment has been minimized.

micszo left a comment

Choose a reason for hiding this comment

lserwatka commented Feb 27, 2019

micszo commented Feb 27, 2019

pawbuj commented Feb 27, 2019

pawbuj commented Feb 1, 2019 •

edited by ViniTou

Loading

adamwojs left a comment •

edited

Loading

pawbuj Feb 4, 2019 •

edited

Loading

lserwatka commented Feb 19, 2019 •

edited

Loading

andrerom commented Feb 19, 2019 •

edited

Loading