Fixed a couple of non-intended logical permission issues

Both caught in tests:
Fixed loss of permissions for admin users when entity restrictions were
active, since there are no entity-restrictions for the admin role but
we'd force generate them in joint permissions, which would be queried.
Fixed new role permission checks when permissions given with only the
action (eg. 'view'), since the type prefix would be required for role
permission checks. Was previously not needed as only the simpler form
was used in the jointpermissions after merge & calculation.
This commit is contained in:
Dan Brown 2022-07-16 20:55:32 +01:00
parent afe1a04239
commit 2332401854
No known key found for this signature in database
GPG Key ID: 46D9F943C24A2EF9
4 changed files with 17 additions and 22 deletions

View File

@ -25,11 +25,13 @@ class PermissionApplicator
{ {
$explodedPermission = explode('-', $permission); $explodedPermission = explode('-', $permission);
$action = $explodedPermission[1] ?? $explodedPermission[0]; $action = $explodedPermission[1] ?? $explodedPermission[0];
$fullPermission = count($explodedPermission) > 1 ? $permission : $ownable->getMorphClass() . '-' . $permission;
$user = $this->currentUser(); $user = $this->currentUser();
$userRoleIds = $this->getCurrentUserRoleIds(); $userRoleIds = $this->getCurrentUserRoleIds();
$allRolePermission = $user->can($permission . '-all'); $allRolePermission = $user->can($fullPermission . '-all');
$ownRolePermission = $user->can($permission . '-own'); $ownRolePermission = $user->can($fullPermission . '-own');
$nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment']; $nonJointPermissions = ['restrictions', 'image', 'attachment', 'comment'];
$ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by'; $ownerField = ($ownable instanceof Entity) ? 'owned_by' : 'created_by';
$isOwner = $user->id === $ownable->getAttribute($ownerField); $isOwner = $user->id === $ownable->getAttribute($ownerField);
@ -40,23 +42,22 @@ class PermissionApplicator
return $hasRolePermission; return $hasRolePermission;
} }
$entityPermissions = $this->getApplicableEntityPermissions($ownable, $userRoleIds, $action); $hasApplicableEntityPermissions = $this->hasEntityPermission($ownable, $userRoleIds, $action);
if (is_null($entityPermissions)) {
return $hasRolePermission;
}
return count($entityPermissions) > 0; return is_null($hasApplicableEntityPermissions) ? $hasRolePermission : $hasApplicableEntityPermissions;
} }
/** /**
* Get the permissions that are applicable for the given entity item. * Check if there are permissions that are applicable for the given entity item, action and roles.
* Returns null when no entity permissions apply otherwise entity permissions * Returns null when no entity permissions are in force.
* are active, even if the returned array is empty.
*
* @returns EntityPermission[]
*/ */
protected function getApplicableEntityPermissions(Entity $entity, array $userRoleIds, string $action): ?array protected function hasEntityPermission(Entity $entity, array $userRoleIds, string $action): ?bool
{ {
$adminRoleId = Role::getSystemRole('admin')->id;
if (in_array($adminRoleId, $userRoleIds)) {
return true;
}
$chain = [$entity]; $chain = [$entity];
if ($entity instanceof Page && $entity->chapter_id) { if ($entity instanceof Page && $entity->chapter_id) {
$chain[] = $entity->chapter; $chain[] = $entity->chapter;
@ -71,8 +72,7 @@ class PermissionApplicator
return $currentEntity->permissions() return $currentEntity->permissions()
->whereIn('role_id', $userRoleIds) ->whereIn('role_id', $userRoleIds)
->where('action', '=', $action) ->where('action', '=', $action)
->get() ->count() > 0;
->all();
} }
} }

View File

@ -91,10 +91,6 @@ class Bookshelf extends Entity implements HasCoverImage
/** /**
* Check if this shelf contains the given book. * Check if this shelf contains the given book.
*
* @param Book $book
*
* @return bool
*/ */
public function contains(Book $book): bool public function contains(Book $book): bool
{ {
@ -103,8 +99,6 @@ class Bookshelf extends Entity implements HasCoverImage
/** /**
* Add a book to the end of this shelf. * Add a book to the end of this shelf.
*
* @param Book $book
*/ */
public function appendBook(Book $book) public function appendBook(Book $book)
{ {

View File

@ -20,6 +20,7 @@ class ShelfContext
return null; return null;
} }
/** @var Bookshelf $shelf */
$shelf = Bookshelf::visible()->find($contextBookshelfId); $shelf = Bookshelf::visible()->find($contextBookshelfId);
$shelfContainsBook = $shelf && $shelf->contains($book); $shelfContainsBook = $shelf && $shelf->contains($book);

View File

@ -104,7 +104,7 @@ class BookshelfController extends Controller
public function show(ActivityQueries $activities, string $slug) public function show(ActivityQueries $activities, string $slug)
{ {
$shelf = $this->bookshelfRepo->getBySlug($slug); $shelf = $this->bookshelfRepo->getBySlug($slug);
$this->checkOwnablePermission('book-view', $shelf); $this->checkOwnablePermission('bookshelf-view', $shelf);
$sort = setting()->getForCurrentUser('shelf_books_sort', 'default'); $sort = setting()->getForCurrentUser('shelf_books_sort', 'default');
$order = setting()->getForCurrentUser('shelf_books_sort_order', 'asc'); $order = setting()->getForCurrentUser('shelf_books_sort_order', 'asc');