Skip to content
Permalink
Browse files

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

* EZP-29474: Fix wrong usage of hasAccess in repository

Fix wrong usage\* of `hasAccess()` in repository in favor 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 customize and add limitations to any of these functions

I effect this makes Repo more correctly use permissions system, 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 function, & 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 coding that assumption, this PR fixes that._

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

Also fix CS issue on test.

Other fixes:
* Always use target location on trash restore to properly take subtree limitations into account
* Adapt SubtreeLimitation to abstain on trashed content when no location target is provided (abstain is for for role limitations, means they will be ignored)
  • Loading branch information...
andrerom committed Sep 10, 2018
1 parent ec4621e commit 8ad7759195e76a20e7757397ce2931b4d3caeb2d
@@ -29,10 +29,13 @@ public function getCurrentUserReference();
public function setCurrentUserReference(UserReference $userReference);
/**
* Returns boolean value or an array of limitations describing user's permissions
* on the given module and function.
* Low level permission function: Returns boolean value, or an array of limitations that user permission depends on.
*
* Note: boolean value describes full access (true) or no access at all (false).
* Note: boolean value describes full access (true) or no access at all (false), array can be seen as a maybe..
*
* WARNING: This is a low level method, if possible strongly prefer to use canUser() as it is able to handle limitations.
* This includes Role Assignment limitations, but also future policy limitations added in kernel,
* or as plain user configuration and/or extending the system.
*
* @param string $module The module, aka controller identifier to check permissions on
* @param string $function The function, aka the controller action to check permissions on
@@ -137,10 +137,15 @@ public function evaluate(APILimitationValue $value, APIUserReference $currentUse
// Load locations if no specific placement was provided
if ($targets === null) {
if ($object->published) {
// Skip check if content is in trash and no location is provided to check against
if ($object->isTrashed()) {
return self::ACCESS_ABSTAIN;
}
if ($object->isPublished()) {
$targets = $this->persistence->locationHandler()->loadLocationsByContent($object->id);
} else {
// @todo Need support for draft locations to to work correctly
// @todo Need support for draft locations to work correctly
$targets = $this->persistence->locationHandler()->loadParentLocationsForDraftContent($object->id);
}
}
@@ -283,7 +283,7 @@ public function providerForTestEvaluate()
$versionInfoMock
->expects($this->once())
->method('getContentInfo')
->will($this->returnValue(new ContentInfo(array('published' => true))));
->willReturn(new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]));
$versionInfoMock2 = $this->getMock(
'eZ\\Publish\\API\\Repository\\Values\\Content\\VersionInfo',
@@ -296,61 +296,61 @@ public function providerForTestEvaluate()
$versionInfoMock2
->expects($this->once())
->method('getContentInfo')
->will($this->returnValue(new ContentInfo(array('published' => true))));
->willReturn(new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]));
return array(
// ContentInfo, with targets, no access
array(
'limitation' => new SubtreeLimitation(),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => array(new Location()),
'persistence' => array(),
'expected' => LimitationType::ACCESS_DENIED,
),
// ContentInfo, with targets, no access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/'))),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => array(new Location(array('pathString' => '/1/55/'))),
'persistence' => array(),
'expected' => LimitationType::ACCESS_DENIED,
),
// ContentInfo, with targets, with access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/'))),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => array(new Location(array('pathString' => '/1/2/'))),
'persistence' => array(),
'expected' => LimitationType::ACCESS_GRANTED,
),
// ContentInfo, no targets, with access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/'))),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => null,
'persistence' => array(new Location(array('pathString' => '/1/2/'))),
'expected' => LimitationType::ACCESS_GRANTED,
),
// ContentInfo, no targets, no access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/', '/1/43/'))),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => null,
'persistence' => array(new Location(array('pathString' => '/1/55/'))),
'expected' => LimitationType::ACCESS_DENIED,
),
// ContentInfo, no targets, un-published, with access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/'))),
'object' => new ContentInfo(array('published' => false)),
'object' => new ContentInfo(['published' => false, 'status' => ContentInfo::STATUS_DRAFT]),
'targets' => null,
'persistence' => array(new Location(array('pathString' => '/1/2/'))),
'expected' => LimitationType::ACCESS_GRANTED,
),
// ContentInfo, no targets, un-published, no access
array(
'limitation' => new SubtreeLimitation(array('limitationValues' => array('/1/2/', '/1/43/'))),
'object' => new ContentInfo(array('published' => false)),
'object' => new ContentInfo(['published' => false, 'status' => ContentInfo::STATUS_DRAFT]),
'targets' => null,
'persistence' => array(new Location(array('pathString' => '/1/55/'))),
'expected' => LimitationType::ACCESS_DENIED,
@@ -406,7 +406,7 @@ public function providerForTestEvaluate()
// invalid target
array(
'limitation' => new SubtreeLimitation(),
'object' => new ContentInfo(array('published' => true)),
'object' => new ContentInfo(['published' => true, 'status' => ContentInfo::STATUS_PUBLISHED]),
'targets' => array(new ObjectStateLimitation()),
'persistence' => array(),
'expected' => LimitationType::ACCESS_ABSTAIN,
@@ -1111,6 +1111,7 @@ public function loadContentDrafts(User $user = null)
$versionInfoList = array();
foreach ($spiVersionInfoList as $spiVersionInfo) {
$versionInfo = $this->domainMapper->buildVersionInfoDomainObject($spiVersionInfo);
// @todo: Change this to filter returned drafts by permissions instead of throwing
if (!$this->repository->canUser('content', 'versionread', $versionInfo)) {
throw new UnauthorizedException('content', 'versionread', array('contentId' => $versionInfo->contentInfo->id));
}
@@ -116,7 +116,7 @@ public function __construct(
*/
public function createContentTypeGroup(ContentTypeGroupCreateStruct $contentTypeGroupCreateStruct)
{
if ($this->repository->hasAccess('class', 'create') !== true) {
if (!$this->repository->canUser('class', 'create', $contentTypeGroupCreateStruct)) {
throw new UnauthorizedException('ContentType', 'create');
}
@@ -235,7 +235,7 @@ public function loadContentTypeGroups()
*/
public function updateContentTypeGroup(APIContentTypeGroup $contentTypeGroup, ContentTypeGroupUpdateStruct $contentTypeGroupUpdateStruct)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeGroup)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -298,7 +298,7 @@ public function updateContentTypeGroup(APIContentTypeGroup $contentTypeGroup, Co
*/
public function deleteContentTypeGroup(APIContentTypeGroup $contentTypeGroup)
{
if ($this->repository->hasAccess('class', 'delete') !== true) {
if (!$this->repository->canUser('class', 'delete', $contentTypeGroup)) {
throw new UnauthorizedException('ContentType', 'delete');
}
@@ -655,7 +655,7 @@ protected function validateInputFieldDefinitionCreateStruct(
*/
public function createContentType(APIContentTypeCreateStruct $contentTypeCreateStruct, array $contentTypeGroups)
{
if ($this->repository->hasAccess('class', 'create') !== true) {
if (!$this->repository->canUser('class', 'create', $contentTypeCreateStruct, $contentTypeGroups)) {
throw new UnauthorizedException('ContentType', 'create');
}
@@ -1022,7 +1022,7 @@ function ($id) {
*/
public function createContentTypeDraft(APIContentType $contentType)
{
if ($this->repository->hasAccess('class', 'create') !== true) {
if (!$this->repository->canUser('class', 'create', $contentType)) {
throw new UnauthorizedException('ContentType', 'create');
}
@@ -1075,7 +1075,7 @@ function ($id) {
*/
public function updateContentTypeDraft(APIContentTypeDraft $contentTypeDraft, ContentTypeUpdateStruct $contentTypeUpdateStruct)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeDraft)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1149,7 +1149,7 @@ public function updateContentTypeDraft(APIContentTypeDraft $contentTypeDraft, Co
*/
public function deleteContentType(APIContentType $contentType)
{
if ($this->repository->hasAccess('class', 'delete') !== true) {
if (!$this->repository->canUser('class', 'delete', $contentType)) {
throw new UnauthorizedException('ContentType', 'delete');
}
@@ -1189,7 +1189,7 @@ public function deleteContentType(APIContentType $contentType)
*/
public function copyContentType(APIContentType $contentType, User $creator = null)
{
if ($this->repository->hasAccess('class', 'create') !== true) {
if (!$this->repository->canUser('class', 'create', $contentType)) {
throw new UnauthorizedException('ContentType', 'create');
}
@@ -1224,7 +1224,7 @@ public function copyContentType(APIContentType $contentType, User $creator = nul
*/
public function assignContentTypeGroup(APIContentType $contentType, APIContentTypeGroup $contentTypeGroup)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentType)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1266,7 +1266,7 @@ public function assignContentTypeGroup(APIContentType $contentType, APIContentTy
*/
public function unassignContentTypeGroup(APIContentType $contentType, APIContentTypeGroup $contentTypeGroup)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentType, [$contentTypeGroup])) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1322,7 +1322,7 @@ public function unassignContentTypeGroup(APIContentType $contentType, APIContent
*/
public function addFieldDefinition(APIContentTypeDraft $contentTypeDraft, FieldDefinitionCreateStruct $fieldDefinitionCreateStruct)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeDraft)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1395,7 +1395,7 @@ public function addFieldDefinition(APIContentTypeDraft $contentTypeDraft, FieldD
*/
public function removeFieldDefinition(APIContentTypeDraft $contentTypeDraft, APIFieldDefinition $fieldDefinition)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeDraft)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1439,7 +1439,7 @@ public function removeFieldDefinition(APIContentTypeDraft $contentTypeDraft, API
*/
public function updateFieldDefinition(APIContentTypeDraft $contentTypeDraft, APIFieldDefinition $fieldDefinition, FieldDefinitionUpdateStruct $fieldDefinitionUpdateStruct)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeDraft)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -1494,7 +1494,7 @@ public function updateFieldDefinition(APIContentTypeDraft $contentTypeDraft, API
*/
public function publishContentTypeDraft(APIContentTypeDraft $contentTypeDraft)
{
if ($this->repository->hasAccess('class', 'update') !== true) {
if (!$this->repository->canUser('class', 'update', $contentTypeDraft)) {
throw new UnauthorizedException('ContentType', 'update');
}
@@ -83,7 +83,7 @@ public function createLanguage(LanguageCreateStruct $languageCreateStruct)
throw new InvalidArgumentValue('enabled', $languageCreateStruct->enabled, 'LanguageCreateStruct');
}
if ($this->repository->hasAccess('content', 'translations') !== true) {
if (!$this->repository->canUser('content', 'translations', $languageCreateStruct)) {
throw new UnauthorizedException('content', 'translations');
}
@@ -133,7 +133,7 @@ public function updateLanguageName(Language $language, $newName)
throw new InvalidArgumentValue('newName', $newName);
}
if ($this->repository->hasAccess('content', 'translations') !== true) {
if (!$this->repository->canUser('content', 'translations', $language)) {
throw new UnauthorizedException('content', 'translations');
}
@@ -171,7 +171,7 @@ public function updateLanguageName(Language $language, $newName)
*/
public function enableLanguage(Language $language)
{
if ($this->repository->hasAccess('content', 'translations') !== true) {
if (!$this->repository->canUser('content', 'translations', $language)) {
throw new UnauthorizedException('content', 'translations');
}
@@ -209,7 +209,7 @@ public function enableLanguage(Language $language)
*/
public function disableLanguage(Language $language)
{
if ($this->repository->hasAccess('content', 'translations') !== true) {
if (!$this->repository->canUser('content', 'translations', $language)) {
throw new UnauthorizedException('content', 'translations');
}
@@ -303,7 +303,7 @@ public function loadLanguageById($languageId)
*/
public function deleteLanguage(Language $language)
{
if ($this->repository->hasAccess('content', 'translations') !== true) {
if (!$this->repository->canUser('content', 'translations', $language)) {
throw new UnauthorizedException('content', 'translations');
}
@@ -81,7 +81,7 @@ public function __construct(RepositoryInterface $repository, Handler $objectStat
*/
public function createObjectStateGroup(ObjectStateGroupCreateStruct $objectStateGroupCreateStruct)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectStateGroupCreateStruct)) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -182,7 +182,7 @@ public function loadObjectStates(APIObjectStateGroup $objectStateGroup)
*/
public function updateObjectStateGroup(APIObjectStateGroup $objectStateGroup, ObjectStateGroupUpdateStruct $objectStateGroupUpdateStruct)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectStateGroup)) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -234,7 +234,7 @@ public function updateObjectStateGroup(APIObjectStateGroup $objectStateGroup, Ob
*/
public function deleteObjectStateGroup(APIObjectStateGroup $objectStateGroup)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectStateGroup)) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -266,7 +266,7 @@ public function deleteObjectStateGroup(APIObjectStateGroup $objectStateGroup)
*/
public function createObjectState(APIObjectStateGroup $objectStateGroup, ObjectStateCreateStruct $objectStateCreateStruct)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectStateCreateStruct, [$objectStateGroup])) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -340,7 +340,7 @@ public function loadObjectState($stateId)
*/
public function updateObjectState(APIObjectState $objectState, ObjectStateUpdateStruct $objectStateUpdateStruct)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectState)) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -401,7 +401,7 @@ public function setPriorityOfObjectState(APIObjectState $objectState, $priority)
throw new InvalidArgumentValue('priority', $priority);
}
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectState)) {
throw new UnauthorizedException('state', 'administrate');
}
@@ -430,7 +430,7 @@ public function setPriorityOfObjectState(APIObjectState $objectState, $priority)
*/
public function deleteObjectState(APIObjectState $objectState)
{
if ($this->repository->hasAccess('state', 'administrate') !== true) {
if (!$this->repository->canUser('state', 'administrate', $objectState)) {
throw new UnauthorizedException('state', 'administrate');
}

0 comments on commit 8ad7759

Please sign in to comment.
You can’t perform that action at this time.