Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EZP-31480: Introduced strict types for PermissionResolver & PermissionCriterionResolver #2986

Merged
merged 1 commit into from
Mar 16, 2020
Merged

EZP-31480: Introduced strict types for PermissionResolver & PermissionCriterionResolver #2986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion eZ/Publish/API/Repository/PermissionCriterionResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\API\Repository;

/**
Expand All @@ -25,5 +27,5 @@ interface PermissionCriterionResolver
*
* @return bool|\eZ\Publish\API\Repository\Values\Content\Query\Criterion
*/
public function getPermissionsCriterion($module, $function, ?array $targets = null);
public function getPermissionsCriterion(string $module = 'content', string $function = 'read', ?array $targets = null);
Copy link
Member

@alongosz alongosz Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use this opportunity to drop default arguments here?

Reasoning: I honestly would have never guessed that

$permissionCriterionResolver->getPermissionsCriterion();

would give me Criteria for content/read - this is one of many policies.

Copy link
Member Author

@adamwojs adamwojs Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely! @mikadamczyk requested this as well on Slack

}
10 changes: 6 additions & 4 deletions eZ/Publish/API/Repository/PermissionResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\API\Repository;

use eZ\Publish\API\Repository\Values\User\LookupLimitationResult;
Expand All @@ -20,14 +22,14 @@ interface PermissionResolver
*
* @return \eZ\Publish\API\Repository\Values\User\UserReference
*/
public function getCurrentUserReference();
public function getCurrentUserReference(): UserReference;

/**
* Sets the current user to the given $user.
*
* @param \eZ\Publish\API\Repository\Values\User\UserReference $userReference
*/
public function setCurrentUserReference(UserReference $userReference);
public function setCurrentUserReference(UserReference $userReference): void;

/**
* Low level permission function: Returns boolean value, or an array of limitations that user permission depends on.
Expand All @@ -47,7 +49,7 @@ public function setCurrentUserReference(UserReference $userReference);
*
* @return bool|array if limitations are on this function an array of limitations is returned
*/
public function hasAccess($module, $function, UserReference $userReference = null);
public function hasAccess(string $module, string $function, ?UserReference $userReference = null);

/**
* Indicates if the current user is allowed to perform an action given by the function on the given
Expand All @@ -70,7 +72,7 @@ public function hasAccess($module, $function, UserReference $userReference = nul
*
* @return bool
*/
public function canUser($module, $function, ValueObject $object, array $targets = []);
public function canUser(string $module, string $function, ValueObject $object, array $targets = []): bool;

/**
* @param string $module The module, aka controller identifier to check permissions on
Expand Down
17 changes: 9 additions & 8 deletions eZ/Publish/Core/Repository/Permission/CachedPermissionService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\Core\Repository\Permission;

use eZ\Publish\API\Repository\PermissionResolver as APIPermissionResolver;
Expand Down Expand Up @@ -69,31 +71,30 @@ class CachedPermissionService implements APIPermissionResolver, APIPermissionCri
public function __construct(
APIPermissionResolver $permissionResolver,
APIPermissionCriterionResolver $permissionCriterionResolver,
$cacheTTL = 5
int $cacheTTL = 5
) {
$this->permissionResolver = $permissionResolver;
$this->permissionCriterionResolver = $permissionCriterionResolver;
$this->cacheTTL = $cacheTTL;
}

public function getCurrentUserReference()
public function getCurrentUserReference(): UserReference
{
return $this->permissionResolver->getCurrentUserReference();
}

public function setCurrentUserReference(UserReference $userReference)
public function setCurrentUserReference(UserReference $userReference): void
{
$this->permissionCriterion = null;

return $this->permissionResolver->setCurrentUserReference($userReference);
$this->permissionResolver->setCurrentUserReference($userReference);
}

public function hasAccess($module, $function, UserReference $userReference = null)
public function hasAccess(string $module, string $function, ?UserReference $userReference = null)
{
return $this->permissionResolver->hasAccess($module, $function, $userReference);
}

public function canUser($module, $function, ValueObject $object, array $targets = [])
public function canUser(string $module, string $function, ValueObject $object, array $targets = []): bool
{
return $this->permissionResolver->canUser($module, $function, $object, $targets);
}
Expand All @@ -111,7 +112,7 @@ public function lookupLimitations(
return $this->permissionResolver->lookupLimitations($module, $function, $object, $targets, $limitations);
}

public function getPermissionsCriterion($module = 'content', $function = 'read', ?array $targets = null)
public function getPermissionsCriterion(string $module = 'content', string $function = 'read', ?array $targets = null)
{
// We only cache content/read lookup as those are the once frequently done, and it's only one we can safely
// do that won't harm the system if it becomes stale (but user might experience permissions exceptions if it do)
Expand Down
4 changes: 3 additions & 1 deletion eZ/Publish/Core/Repository/Permission/PermissionCriterionResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\Core\Repository\Permission;

use eZ\Publish\API\Repository\PermissionCriterionResolver as APIPermissionCriterionResolver;
Expand Down Expand Up @@ -53,7 +55,7 @@ public function __construct(
*
* @return bool|\eZ\Publish\API\Repository\Values\Content\Query\Criterion
*/
public function getPermissionsCriterion($module = 'content', $function = 'read', ?array $targets = null)
public function getPermissionsCriterion(string $module = 'content', string $function = 'read', ?array $targets = null)
{
$permissionSets = $this->permissionResolver->hasAccess($module, $function);
if (is_bool($permissionSets)) {
Expand Down
10 changes: 6 additions & 4 deletions eZ/Publish/Core/Repository/Permission/PermissionResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\Core\Repository\Permission;

use eZ\Publish\API\Repository\PermissionResolver as PermissionResolverInterface;
Expand Down Expand Up @@ -77,12 +79,12 @@ public function __construct(
$this->policyMap = $policyMap;
}

public function getCurrentUserReference()
public function getCurrentUserReference(): APIUserReference
{
return $this->currentUserRef;
}

public function setCurrentUserReference(APIUserReference $userReference)
public function setCurrentUserReference(APIUserReference $userReference): void
{
$id = $userReference->getUserId();
if (!$id) {
Expand All @@ -92,7 +94,7 @@ public function setCurrentUserReference(APIUserReference $userReference)
$this->currentUserRef = $userReference;
}

public function hasAccess($module, $function, APIUserReference $userReference = null)
public function hasAccess(string $module, string $function, ?APIUserReference $userReference = null)
{
if (!isset($this->policyMap[$module])) {
throw new InvalidArgumentValue('module', "module: {$module}/ function: {$function}");
Expand Down Expand Up @@ -158,7 +160,7 @@ public function hasAccess($module, $function, APIUserReference $userReference =
return false; // No policies matching $module and $function, or they contained limitations
}

public function canUser($module, $function, ValueObject $object, array $targets = [])
public function canUser(string $module, string $function, ValueObject $object, array $targets = []): bool
{
$permissionSets = $this->hasAccess($module, $function);
if ($permissionSets === false || $permissionSets === true) {
Expand Down
21 changes: 14 additions & 7 deletions eZ/Publish/Core/Repository/Tests/Permission/CachedPermissionServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
<?php

/**
* File contains: eZ\Publish\Core\Repository\Tests\Service\Mock\PermissionCriterionHandlerTest class.
*
* @copyright Copyright (C) eZ Systems AS. All rights reserved.
* @license For full copyright and license information view LICENSE file distributed with this source code.
*/
declare(strict_types=1);

namespace eZ\Publish\Core\Repository\Permission;

/**
Expand Down Expand Up @@ -73,11 +73,18 @@ public function providerForTestPermissionResolverPassTrough()
*/
public function testPermissionResolverPassTrough($method, array $arguments, $expectedReturn)
{
$this->getPermissionResolverMock([$method])
->expects($this->once())
->method($method)
->with(...$arguments)
->willReturn($expectedReturn);
if ($expectedReturn !== null) {
$this->getPermissionResolverMock([$method])
->expects($this->once())
->method($method)
->with(...$arguments)
->willReturn($expectedReturn);
} else {
$this->getPermissionResolverMock([$method])
->expects($this->once())
->method($method)
->with(...$arguments);
}

$cachedService = $this->getCachedPermissionService();

Expand Down