Skip to content

Commit

Permalink
Merge pull request #1173 from nextcloud/fix/1131/fix-undefined-key-fo…
Browse files Browse the repository at this point in the history 
…r-good

fix(Backend): use object over loose array for permissions
  • Loading branch information
@juliushaertl
juliushaertl authored Jul 4, 2024
2 parents f59f405 + 8086f01 commit 741349d
Show file tree
Hide file tree
Showing 8 changed files with 90 additions and 84 deletions.
15 changes: 6 additions & 9 deletions lib/Db/Table.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace OCA\Tables\Db;

use JsonSerializable;
use OCA\Tables\Model\Permissions;
use OCA\Tables\ResponseDefinitions;
use OCP\AppFramework\Db\Entity;

Expand All @@ -25,8 +26,8 @@
* @method setOwnerDisplayName(string $ownerDisplayName)
* @method getIsShared(): bool
* @method setIsShared(bool $isShared)
* @method getOnSharePermissions(): array
* @method setOnSharePermissions(array $onSharePermissions)
* @method getOnSharePermissions(): ?Permissions
* @method setOnSharePermissions(Permissions $onSharePermissions)
* @method getHasShares(): bool
* @method setHasShares(bool $hasShares)
* @method getFavorite(): bool
Expand Down Expand Up @@ -59,7 +60,7 @@ class Table extends Entity implements JsonSerializable {
protected ?string $lastEditAt = null;
protected bool $archived = false;
protected ?bool $isShared = null;
protected ?array $onSharePermissions = null;
protected ?Permissions $onSharePermissions = null;

protected ?bool $hasShares = false;
protected ?bool $favorite = false;
Expand Down Expand Up @@ -91,7 +92,7 @@ public function jsonSerialize(): array {
'archived' => $this->archived,
'isShared' => !!$this->isShared,
'favorite' => $this->favorite,
'onSharePermissions' => $this->getSharePermissions(),
'onSharePermissions' => $this->getSharePermissions()?->jsonSerialize(),
'hasShares' => !!$this->hasShares,
'rowsCount' => $this->rowsCount ?: 0,
'columnsCount' => $this->columnsCount ?: 0,
Expand All @@ -100,11 +101,7 @@ public function jsonSerialize(): array {
];
}

/**
* @psalm-suppress MismatchingDocblockReturnType
* @return array{read: bool, create: bool, update: bool, delete: bool, manage: bool}|null
*/
private function getSharePermissions(): ?array {
private function getSharePermissions(): ?Permissions {
return $this->onSharePermissions;
}

Expand Down
15 changes: 6 additions & 9 deletions lib/Db/View.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace OCA\Tables\Db;

use JsonSerializable;
use OCA\Tables\Model\Permissions;
use OCA\Tables\ResponseDefinitions;
use OCP\AppFramework\Db\Entity;

Expand Down Expand Up @@ -35,8 +36,8 @@
* @method setDescription(string $description)
* @method getIsShared(): bool
* @method setIsShared(bool $isShared)
* @method getOnSharePermissions(): array{create: bool,delete: bool,manage: bool,read: bool,update: bool}|null
* @method setOnSharePermissions(array $onSharePermissions)
* @method getOnSharePermissions(): ?Permissions
* @method setOnSharePermissions(Permissions $onSharePermissions)
* @method getHasShares(): bool
* @method setHasShares(bool $hasShares)
* @method getFavorite(): bool
Expand All @@ -61,7 +62,7 @@ class View extends Entity implements JsonSerializable {
protected ?string $sort = null; // json
protected ?string $filter = null; // json
protected ?bool $isShared = null;
protected ?array $onSharePermissions = null;
protected ?Permissions $onSharePermissions = null;
protected ?bool $hasShares = false;
protected bool $favorite = false;
protected ?int $rowsCount = 0;
Expand Down Expand Up @@ -117,11 +118,7 @@ public function setFilterArray(array $array):void {
$this->setFilter(\json_encode($array));
}

/**
* @psalm-suppress MismatchingDocblockReturnType
* @return array{create: bool, delete: bool, manage: bool, read: bool, update: bool}|null
*/
private function getSharePermissions(): ?array {
private function getSharePermissions(): ?Permissions {
return $this->getOnSharePermissions();
}

Expand Down Expand Up @@ -152,7 +149,7 @@ public function jsonSerialize(): array {
'sort' => $this->getSortArray(),
'isShared' => !!$this->isShared,
'favorite' => $this->favorite,
'onSharePermissions' => $this->getSharePermissions(),
'onSharePermissions' => $this->getSharePermissions()?->jsonSerialize(),
'hasShares' => !!$this->hasShares,
'rowsCount' => $this->rowsCount ?: 0,
'ownerDisplayName' => $this->ownerDisplayName,
Expand Down
18 changes: 17 additions & 1 deletion lib/Model/Permissions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

namespace OCA\Tables\Model;

class Permissions {
use JsonSerializable;

class Permissions implements JsonSerializable {
public function __construct(
public bool $read = false,
public bool $create = false,
Expand All @@ -12,4 +14,18 @@ public function __construct(
public bool $manageTable = false,
) {
}

/**
* @return array{read: bool, create: bool, update: bool, delete: bool, manage: bool}
*/
public function jsonSerialize(): array {
// manageTable is not serialized as it is used in the backend only
return [
'read' => $this->read,
'create' => $this->create,
'update' => $this->update,
'delete' => $this->delete,
'manage' => $this->manage,
];
}
}
2 changes: 1 addition & 1 deletion lib/ResponseDefinitions.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
* create: bool,
* update: bool,
* delete: bool,
* manage: bool
* manage: bool,
* },
* hasShares: bool,
* rowsCount: int,
Expand Down
54 changes: 28 additions & 26 deletions lib/Service/PermissionsService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use OCA\Tables\Errors\NotFoundError;
use OCA\Tables\Helper\ConversionHelper;
use OCA\Tables\Helper\UserHelper;
use OCA\Tables\Model\Permissions;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\DB\Exception;
Expand Down Expand Up @@ -414,31 +415,32 @@ public function canReadShare(Share $share, ?string $userId = null): bool {
* @param int $elementId
* @param 'table'|'view' $elementType
* @param string $userId
* @return array
* @throws NotFoundError
*/
public function getSharedPermissionsIfSharedWithMe(int $elementId, string $elementType, string $userId): array {
public function getSharedPermissionsIfSharedWithMe(int $elementId, string $elementType, string $userId): Permissions {
try {
$shares = $this->shareMapper->findAllSharesForNodeFor($elementType, $elementId, $userId);
} catch (Exception $e) {
$this->logger->warning('Exception occurred: '.$e->getMessage().' Permission denied.');
return [];
return new Permissions();
}

try {
$userGroups = $this->userHelper->getGroupsForUser($userId);
} catch (InternalError $e) {
$this->logger->warning('Exception occurred: '.$e->getMessage().' Permission denied.');
return [];
return new Permissions();
}
$additionalShares = [];
foreach ($userGroups as $userGroup) {
try {
$shares = array_merge($shares, $this->shareMapper->findAllSharesForNodeFor($elementType, $elementId, $userGroup->getGid(), 'group'));
$additionalShares[] = $this->shareMapper->findAllSharesForNodeFor($elementType, $elementId, $userGroup->getGid(), 'group');
} catch (Exception $e) {
$this->logger->warning('Exception occurred: '.$e->getMessage().' Permission denied.');
return [];
return new Permissions();
}
}
$shares = array_merge($shares, ...$additionalShares);
if (count($shares) > 0) {
$read = array_reduce($shares, function ($carry, $share) {
return $carry || ($share->getPermissionRead());
Expand All @@ -456,13 +458,13 @@ public function getSharedPermissionsIfSharedWithMe(int $elementId, string $eleme
return $carry || ($share->getPermissionManage());
}, false);

return [
'read' => $read || $update || $delete || $manage,
'create' => $create || $manage,
'update' => $update || $manage,
'delete' => $delete || $manage,
'manage' => $manage,
];
return new Permissions(
read: $read || $update || $delete || $manage,
create: $create || $manage,
update: $update || $manage,
delete: $delete || $manage,
manage: $manage,
);
}
throw new NotFoundError('No share for '.$elementType.' and given user ID found.');
}
Expand Down Expand Up @@ -498,15 +500,15 @@ public function getPermissionIfAvailableThroughContext(int $nodeId, string $node
/**
* @throws NotFoundError
*/
public function getPermissionArrayForNodeFromContexts(int $nodeId, string $nodeType, string $userId) {
public function getPermissionArrayForNodeFromContexts(int $nodeId, string $nodeType, string $userId): Permissions {
$permissions = $this->getPermissionIfAvailableThroughContext($nodeId, $nodeType, $userId);
return [
'read' => (bool)($permissions & Application::PERMISSION_READ),
'create' => (bool)($permissions & Application::PERMISSION_CREATE),
'update' => (bool)($permissions & Application::PERMISSION_UPDATE),
'delete' => (bool)($permissions & Application::PERMISSION_DELETE),
'manage' => (bool)($permissions & Application::PERMISSION_MANAGE),
];
return new Permissions(
read: (bool)($permissions & Application::PERMISSION_READ),
create: (bool)($permissions & Application::PERMISSION_CREATE),
update: (bool)($permissions & Application::PERMISSION_UPDATE),
delete: (bool)($permissions & Application::PERMISSION_DELETE),
manage: (bool)($permissions & Application::PERMISSION_MANAGE),
);
}

private function hasPermission(int $existingPermissions, string $permissionName): bool {
Expand All @@ -527,7 +529,7 @@ private function hasPermission(int $existingPermissions, string $permissionName)
/**
* @param Table|View|Context $element
* @param 'table'|'view'|'context' $nodeType
* @param string $permission
* @param 'read'|'create'|'update'|'delete'|'manage'|'manageTable' $permission
* @param string|null $userId
* @return bool
*/
Expand All @@ -541,7 +543,7 @@ private function checkPermission(Table|View|Context $element, string $nodeType,
}

try {
return $this->getSharedPermissionsIfSharedWithMe($element->getId(), $nodeType, $userId)[$permission];
return $this->getSharedPermissionsIfSharedWithMe($element->getId(), $nodeType, $userId)->$permission;
} catch (NotFoundError $e) {
try {
if ($nodeType !== 'context'
Expand All @@ -560,7 +562,7 @@ private function checkPermission(Table|View|Context $element, string $nodeType,
/**
* @param int $elementId
* @param 'table'|'view' $nodeType
* @param string $permission
* @param 'read'|'create'|'update'|'delete'|'manage'|'manageTable' $permission
* @param string|null $userId
* @return bool
*/
Expand All @@ -570,7 +572,7 @@ private function checkPermissionById(int $elementId, string $nodeType, string $p
}
if ($userId) {
try {
return $this->getSharedPermissionsIfSharedWithMe($elementId, $nodeType, $userId)[$permission];
return $this->getSharedPermissionsIfSharedWithMe($elementId, $nodeType, $userId)->$permission;
} catch (NotFoundError $e) {
try {
if ($this->hasPermission($this->getPermissionIfAvailableThroughContext($elementId, $nodeType, $userId), $permission)) {
Expand Down Expand Up @@ -602,7 +604,7 @@ private function basisCheck(Table|View|Context $element, string $nodeType, ?stri
}
try {
$permissions = $this->getSharedPermissionsIfSharedWithMe($nodeType === 'view' ? $element->getTableId() : $element->getId(), 'table', $userId);
if($permissions['manage']) {
if ($permissions->manage) {
return true;
}
} catch (NotFoundError $e) {
Expand Down
6 changes: 3 additions & 3 deletions lib/Service/ShareService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use OCA\Tables\Helper\GroupHelper;
use OCA\Tables\Helper\UserHelper;

use OCA\Tables\Model\Permissions;
use OCA\Tables\ResponseDefinitions;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
Expand Down Expand Up @@ -186,15 +187,14 @@ private function findElementsSharedWithMe(string $elementType = 'table', ?string
* @param int $elementId
* @param 'table'|'view' $elementType
* @param string|null $userId
* @return array
* @throws NotFoundError
*/
public function getSharedPermissionsIfSharedWithMe(int $elementId, string $elementType = 'table', ?string $userId = null): array {
public function getSharedPermissionsIfSharedWithMe(int $elementId, string $elementType = 'table', ?string $userId = null): Permissions {
try {
$userId = $this->permissionsService->preCheckUserId($userId);
} catch (InternalError $e) {
$this->logger->warning('Could not pre check user: '.$e->getMessage().' Permission denied.');
return [];
return new Permissions();
}
return $this->permissionsService->getSharedPermissionsIfSharedWithMe($elementId, $elementType, $userId);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Service/TableService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ private function enhanceTable(Table $table, string $userId): void {

}
}
if (!$table->getIsShared() || $table->getOnSharePermissions()['manage']) {
if (!$table->getIsShared() || $table->getOnSharePermissions()->manage) {
// add the corresponding views if it is an own table, or you have table manage rights
$table->setViews($this->viewService->findAll($table));
}
Expand Down
Loading

0 comments on commit 741349d

Please sign in to comment.