EZP-29474: Fix wrong usage of hasAccess in repository #2413

andrerom · 2018-08-10T09:42:12Z

Question	Answer
JIRA issue	EZP-29474
Bug/Improvement	yes
New feature	no
Target version	`6.7`
BC breaks	yes (2, see below)
Tests pass	yes
Doc needed	afaik no

Fix wrong usage* of hasAccess() in repository in favor of canUser() in order to solve:

Issues when there are Role Assignment Limitation, as in this case they will return info to permission system they abstain from "voting"
As policies are now extendable, make code prepared in case someone needs to customize and add limitations to any of these functions, or we end up adding it out of the box ourselves.

In effect this makes Repo more correctly use permissions system, and will avoid quite some unneeded edge case exceptions all across the repo on these cases, the issue reported by QA is afaik just scratching the surface here (see diff).

There is also two behavior changes (in terms of api spec) which will simplify API usage:

Done in PR: loadSections(): PR proposes to change this to just filter sections (preparing it for we/others adding policy limitations on the function, & avoiding issue with role limitation)
Proposed in PR in inline comment: load user drafts: Same, proposes to filter the returned drafts instead of throwing

* Long story short, we forgot about Role limitations + extensibility when coding this, and made shortcuts hard coding that assumption, this PR fixes that.

What about #2408?

Hopefully this solves the bug, meaning the proposed changes in #2408 will hopefully get some more time to mature as a separate (new) feature/Story. As it basically changes the behavior to not follow API spec anymore, which is indication that it needs some more baking and probably we should rather design a new api method for cases where we don't have an object, as the code involved at least would need to know what kind of object (interface FQN for instance) was expected to be able to tell the permission system to safely ignore a given limitation or not (for instance abstain feature of role limitations could be extracted into new role limitations type interface as a start to try to better separate between policy and role limitation responsibilities, and can be used to check this without having object, ping me if unclear I can also do a POC on it for discussion).

TODO:

Implement feature / fix a bug.
Implement tests.
Fix new code according to Coding Standards ($ composer fix-cs).
Ask for Code Review.
Open question maybe more for a follow PR: Should we deprecate hasAccess given it can be miss-leading? (aka people only expecting it to return boolean, while it really is designed to give you a yes/no/maybe anwear, in the latter case the system needs more context basically, but besides canUser other use case have not been solved in API for this like whatCotnentTypeCanUserCreateHere(Location)) So.. what should we call it? And what should it return? What do you need?

andrerom · 2018-08-16T07:27:16Z

eZ/Publish/Core/Repository/ContentService.php

@@ -1103,6 +1103,7 @@ public function loadContentDrafts(User $user = null)
        }

        // throw early if user has absolutely no access to versionread
+


andrerom · 2018-08-16T07:27:29Z

eZ/Publish/Core/Repository/ContentService.php

@@ -1111,6 +1112,7 @@ public function loadContentDrafts(User $user = null)
        $versionInfoList = array();
        foreach ($spiVersionInfoList as $spiVersionInfo) {
            $versionInfo = $this->domainMapper->buildVersionInfoDomainObject($spiVersionInfo);
+            // TODO: Should we rather filter out items here? If so we can remove early permission check above and exception


Yes, I think this would be more reasonable.

andrerom · 2018-08-16T07:28:47Z

eZ/Publish/Core/Repository/SectionService.php

+        // REVIEW NOTE: NewSection limitation expects target to be the section, while Role Subtree limitations will abstain from checking
+        // due to target being section. We could consider changing Subtree to load targets from content and check anyway. So
+        // we make sure section assignments are limited by subtree limitations which could make sense given object is content.
+        if (!$this->permissionResolver->canUser('section', 'assign', $loadedContentInfo, [$section])) {


Question 1:

REVIEW NOTE: NewSection limitation expects target to be the section, while Role Subtree limitations will abstain from checking due to target being section.

So you're saying that if we have Subtree limitation, here we can assign section to content which belongs to another subtree? :)

Question 2:
any reason you've replaced $loadedSection with $section in this line?

any reason you've replaced $loadedSection with $section in this line?

no, that is a "mistake" and I'll change it back to use `$loadedSection . Even if I perosnally don't think we need to re-load things like this in the first place ;)

So you're saying that if we have Subtree limitation, here we can assign section to content which belongs to another subtree? :)

I'm saying both NewSection and SubtreeLimitation (role limitation) could be adhered to.

I can open that as separate PR to master so we can take that as separate thing?

I'm saying both NewSection and SubtreeLimitation (role limitation) could be adhered to.
I can open that as separate PR to master so we can take that as separate thing?

Yes please :)

alongosz

Hmm, looks okay-ish ;)
Let's make sure that we don't change the behavior for 6.7.

So while I'm in favor of filtering versions when loading drafts for user, it feels like, if you'll go that direction, it should be targeted to master.
The same goes for Trash operations, but on that one - maybe we just need to check if behavior changed.

The other changes - this fixes existing bugs, so even if something changed, it needs to, so +1

alongosz · 2018-08-16T07:59:17Z

eZ/Publish/Core/Repository/ContentService.php

@@ -1111,6 +1112,7 @@ public function loadContentDrafts(User $user = null)
        $versionInfoList = array();
        foreach ($spiVersionInfoList as $spiVersionInfo) {
            $versionInfo = $this->domainMapper->buildVersionInfoDomainObject($spiVersionInfo);
+            // TODO: Should we rather filter out items here? If so we can remove early permission check above and exception


Yes, I think this would be more reasonable.

alongosz · 2018-08-16T08:22:39Z

eZ/Publish/Core/Repository/SectionService.php

+        // REVIEW NOTE: NewSection limitation expects target to be the section, while Role Subtree limitations will abstain from checking
+        // due to target being section. We could consider changing Subtree to load targets from content and check anyway. So
+        // we make sure section assignments are limited by subtree limitations which could make sense given object is content.
+        if (!$this->permissionResolver->canUser('section', 'assign', $loadedContentInfo, [$section])) {


Question 1:

REVIEW NOTE: NewSection limitation expects target to be the section, while Role Subtree limitations will abstain from checking due to target being section.

So you're saying that if we have Subtree limitation, here we can assign section to content which belongs to another subtree? :)

Question 2:
any reason you've replaced $loadedSection with $section in this line?

alongosz · 2018-08-16T08:28:22Z

eZ/Publish/Core/Repository/TrashService.php

        $spiTrashItem = $this->persistenceHandler->trashHandler()->loadTrashItem($trashItemId);
        $trash = $this->buildDomainTrashItemObject($spiTrashItem);
        if (!$this->repository->canUser('content', 'read', $trash->getContentInfo())) {
            throw new UnauthorizedException('content', 'read');
        }

+        // TODO: Need to check (integration tests + self + QA) how Role limitation will behave with this (as there is no location)
+        // we could pass trash as target (same goes for content/read above), but again would need to check how it will behave.
+        if (!$this->repository->canUser('content', 'restore', $trash->getContentInfo())) {


Indeed. There is a risk this will fail with subtree limitation on Role. Trash item after all doesn't belong to subtree it was removed from...

alongosz · 2018-08-16T08:29:05Z

eZ/Publish/Core/Repository/TrashService.php

+        if (!$this->repository->canUser(
+            'content',
+            'restore',
+            $trashItem->getContentInfo(),


The same comment as above, but I guess we just need to check this when moved to QA.

Yes, this one needs to be tested, the change implies role limitation will kick in here only allowing you to restore within trees you have been granted role with restore rights.

Which I think makes sense here as we have a proper location which is a target of the restore action (both in case different location was provided and in case where trash location is to be the target)

As for "above", this is not the case anymore after PR updates for checks on read permission:

It does now not include the trash location as target there (as it is about checking if you can read content in trash, location is irrelevant)

PR now adapts SubtreeLimitationType to abstain if no target is provided and content is in trash to make this work.

andrerom · 2018-08-23T06:42:59Z

Ready for review, followup PR with some earlier suggested changes to behavior will come later for master / @alongosz @adamwojs

alongosz · 2018-08-25T13:31:20Z

eZ/Publish/Core/Repository/SectionService.php

@@ -33,6 +33,9 @@ class SectionService implements SectionServiceInterface
     */
    protected $repository;

+    /** @var \eZ\Publish\API\Repository\PermissionResolver */


@andrerom: nitpick - we've decided (with @lserwatka's approval) that we're not going to use single line type hinting for class members.

Reasons:

if you want to add more info for a member, you'll need to make it multiline anyway, so this is inconsistent (on a very abstract/weird level of thinking, to me, this is the same as returning either array or value depending on needs, which is bad ;) )

there are far more multiline type-hints (3095) than single line (278) across the code base.

GitHub doesn't get it (doesn't highlight it properly).

We rather use single line inside methods only, to type-hint specific variables, which have unknow type - there it makes sense to me. Of course only if we have to (the direction is rather to use strict types to avoid that, but there are places we cannot)

ok, lets hope typed properties arrive in PHP 7.4 as intended so at some point you can rid of these completely then ;)

Fixed in 15529ab

kmadejski · 2018-08-27T11:58:15Z

eZ/Publish/Core/Repository/SectionService.php

@@ -53,6 +58,7 @@ class SectionService implements SectionServiceInterface
    public function __construct(RepositoryInterface $repository, Handler $sectionHandler, array $settings = array())
    {
        $this->repository = $repository;
+        $this->permissionResolver = $repository->getPermissionResolver();


Rather just a question regarding these changes. In all other services $this->repository->canUser() is used, is SectionService different somehow and needs to has an instance of PermissionResolver?

@kmadejski

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

Line 61 in 4e0ac66

* @deprecated since 6.6, to be removed. Use PermissionResolver::canUser() instead.

@wizhippo thanks! I know about that deprecation, this is exactly why I've asked this question. As you can see, even after changes from this PR all other services still use deprecated $this->repository->canUser(). Therefore, I'm just curious why this has been changed only for SectionService.

Got it. Good question.

@kmadejski because it feels a bit out of the scope of this PR. Too many changes slow down review and approval.
But you are more than welcomed to improve our code which still uses Repository::canUser directly 😀

So yeah, I did it on this one only as here it was not to invasive, but in general it's a small step we could do all over to help us be able to de-couple this. As it makes it more clear what is using repo as this is the number one thing we want to get rid of (cyclic dependency). In order to set these services and their helpers up using clear dependency injection instead of being initiated in Repo inline.

Fix wrong usage\* of hasAccess() in repopsitory in favour of canUser in order to solve: - Issues when there are Role Assignment Limitation - As policies are now extendable, better be prepared in case someone needs to cusotmize and add limitations to any of these functions I effect this makes Repo more correclty use permissions sytem, and will avoid quite some unneeded exceptions all across the repo. There is also two behavour changes _(in terms of api spec)_ which will simplify API usage: - loadSections(): PR proposes to change this to just filter sections _(preparing it for we/others adding policy limitations on the functon, & avoiding issue with role limitation)_ - load user drafts: Same, proposes to filter the returned drafts instead of throwing \* _Long story short, we forgot about Role limitations + extensibility when coding this, and made shortcuts hard codeing that assumiption, this PR fixes that._

Also fix CS issue on test. [skip ci]

[skip ci]

…tions into account

andrerom · 2018-09-10T01:57:26Z

@vidarl Could you check why CS checked does not run?
Because of it, we end up with CS issues in branches: 28169c3

@alongosz did you consider adding cs check on travis here like on some other reposP

alongosz · 2018-09-10T07:32:31Z

did you consider adding cs check on travis here like on some other repos

@andrerom Well, for kernel it requires some performance improvements, because what we have on Travis, currently it runs for all files AFAIR. Here we have 3400 of them, so... :) I'll look into this in some spare time.

andrerom mentioned this pull request Aug 10, 2018

EZP-29474: Fix for the hasAccess method when a policy has no limitations #2408

Closed

4 tasks

This comment has been minimized.

Sign in to view

andrerom requested review from adamwojs, alongosz and kmadejski August 16, 2018 07:26

andrerom commented Aug 16, 2018

View reviewed changes

andrerom requested a review from mikadamczyk August 16, 2018 07:31

alongosz reviewed Aug 16, 2018

View reviewed changes

andrerom changed the title ~~EZP-29474: Fix wrong usage of hasAccess in repopsitory~~ EZP-29474: Fix wrong usage of hasAccess in repository Aug 24, 2018

alongosz approved these changes Aug 25, 2018

View reviewed changes

alongosz approved these changes Aug 26, 2018

View reviewed changes

kmadejski approved these changes Aug 27, 2018

View reviewed changes

andrerom added the Ready for QA label Aug 28, 2018

barbaragr self-assigned this Sep 5, 2018

andrerom mentioned this pull request Sep 6, 2018

[TMP for QA] EZP-29474: 7.2 port of Fix wrong usage of hasAccess in repository #2439

Closed

andrerom added 8 commits September 9, 2018 17:27

[API Doc] Add some api doc to clearify usage.

e9c361a

Also fix CS issue on test. [skip ci]

Update inline doc

6be36d6

[skip ci]

Update PermissionResolver.php

4d1ae12

Update PermissionResolver.php

c9f52dd

Adapt for reviews

ca95f95

Always use target location on restore to properly take subtree limita…

389575c

…tions into account

CS

3deccb8

andrerom force-pushed the repo_use_canUser branch from 15529ab to 3deccb8 Compare September 9, 2018 10:28

andrerom merged commit 8ad7759 into 6.7 Sep 10, 2018

andrerom deleted the repo_use_canUser branch September 10, 2018 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29474: Fix wrong usage of hasAccess in repository #2413

EZP-29474: Fix wrong usage of hasAccess in repository #2413

andrerom commented Aug 10, 2018 •

edited

This comment has been minimized.

andrerom Aug 16, 2018

andrerom Aug 16, 2018

alongosz Aug 16, 2018

andrerom Aug 16, 2018

alongosz Aug 16, 2018

andrerom Aug 16, 2018

andrerom Aug 16, 2018 •

edited

alongosz Aug 16, 2018 •

edited by andrerom

alongosz left a comment

alongosz Aug 16, 2018

alongosz Aug 16, 2018

alongosz Aug 16, 2018

alongosz Aug 16, 2018

andrerom Aug 23, 2018 •

edited

andrerom commented Aug 23, 2018 •

edited

alongosz Aug 25, 2018

andrerom Aug 26, 2018 •

edited

andrerom Aug 26, 2018

kmadejski Aug 27, 2018

wizhippo Aug 27, 2018

kmadejski Aug 27, 2018 •

edited

wizhippo Aug 27, 2018

alongosz Aug 27, 2018

andrerom Aug 28, 2018 •

edited

andrerom commented Sep 10, 2018

alongosz commented Sep 10, 2018

		@@ -1103,6 +1103,7 @@ public function loadContentDrafts(User $user = null)
		}

		// throw early if user has absolutely no access to versionread

EZP-29474: Fix wrong usage of hasAccess in repository #2413

EZP-29474: Fix wrong usage of hasAccess in repository #2413

Conversation

andrerom commented Aug 10, 2018 • edited

What about #2408?

This comment has been minimized.

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

andrerom Aug 16, 2018 • edited

Choose a reason for hiding this comment

alongosz Aug 16, 2018 • edited by andrerom

Choose a reason for hiding this comment

alongosz left a 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

andrerom Aug 23, 2018 • edited

Choose a reason for hiding this comment

andrerom commented Aug 23, 2018 • edited

Choose a reason for hiding this comment

andrerom Aug 26, 2018 • edited

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

kmadejski Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrerom Aug 28, 2018 • edited

Choose a reason for hiding this comment

andrerom commented Sep 10, 2018

alongosz commented Sep 10, 2018

andrerom commented Aug 10, 2018 •

edited

andrerom Aug 16, 2018 •

edited

alongosz Aug 16, 2018 •

edited by andrerom

andrerom Aug 23, 2018 •

edited

andrerom commented Aug 23, 2018 •

edited

andrerom Aug 26, 2018 •

edited

kmadejski Aug 27, 2018 •

edited

andrerom Aug 28, 2018 •

edited