diff --git a/lib/Controller/Api1Controller.php b/lib/Controller/Api1Controller.php index 911845b79..839d71971 100644 --- a/lib/Controller/Api1Controller.php +++ b/lib/Controller/Api1Controller.php @@ -11,6 +11,7 @@ use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Middleware\Attribute\RequirePermission; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ImportService; @@ -476,7 +477,7 @@ public function indexTableShares(int $tableId): DataResponse { * @NoCSRFRequired * * @param int $nodeId Node ID - * @param 'table'|'view' $nodeType Node type + * @param 'table'|'view'|'context' $nodeType Node type * @param string $receiver Receiver ID * @param 'user'|'group' $receiverType Receiver type * @param bool $permissionRead Permission if receiver can read data @@ -491,6 +492,7 @@ public function indexTableShares(int $tableId): DataResponse { * 403: No permissions * 404: Not found */ + #[RequirePermission(permission: Application::PERMISSION_MANAGE)] public function createShare( int $nodeId, string $nodeType, @@ -1363,6 +1365,7 @@ public function importInView(int $viewId, string $path, bool $createMissingColum * 403: No permissions * 404: Not found */ + #[RequirePermission(permission: Application::PERMISSION_MANAGE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')] public function createTableShare(int $tableId, string $receiver, string $receiverType, bool $permissionRead, bool $permissionCreate, bool $permissionUpdate, bool $permissionDelete, bool $permissionManage): DataResponse { try { return new DataResponse($this->shareService->create($tableId, 'table', $receiver, $receiverType, $permissionRead, $permissionCreate, $permissionUpdate, $permissionDelete, $permissionManage, 0)->jsonSerialize()); diff --git a/lib/Controller/ShareController.php b/lib/Controller/ShareController.php index 16c761e69..04532090e 100644 --- a/lib/Controller/ShareController.php +++ b/lib/Controller/ShareController.php @@ -3,6 +3,7 @@ namespace OCA\Tables\Controller; use OCA\Tables\AppInfo\Application; +use OCA\Tables\Middleware\Attribute\RequirePermission; use OCA\Tables\Service\ShareService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; @@ -61,6 +62,7 @@ public function show(int $id): DataResponse { /** * @NoAdminRequired */ + #[RequirePermission(permission: Application::PERMISSION_MANAGE)] public function create( int $nodeId, string $nodeType, diff --git a/lib/Middleware/Attribute/RequirePermission.php b/lib/Middleware/Attribute/RequirePermission.php new file mode 100644 index 000000000..51ad32475 --- /dev/null +++ b/lib/Middleware/Attribute/RequirePermission.php @@ -0,0 +1,32 @@ +permission; + } + + public function getTypeParam(): string { + return $this->typeParam; + } + + public function getIdParam(): string { + return $this->idParam; + } + + public function getType(): ?int { + return $this->type; + } +} diff --git a/lib/Middleware/PermissionMiddleware.php b/lib/Middleware/PermissionMiddleware.php index 49005e15e..a3d98878b 100644 --- a/lib/Middleware/PermissionMiddleware.php +++ b/lib/Middleware/PermissionMiddleware.php @@ -2,9 +2,11 @@ namespace OCA\Tables\Middleware; +use OCA\Tables\AppInfo\Application; use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Middleware\Attribute\RequirePermission; use OCA\Tables\Service\PermissionsService; use OCP\AppFramework\Http; use OCP\AppFramework\Middleware; @@ -37,10 +39,82 @@ public function __construct( public function beforeController($controller, $methodName): void { // we can have type hinting in the signature only after dropping NC26 – calling parent to enforce on newer releases parent::beforeController($controller, $methodName); + $this->assertPermission($controller, $methodName); $this->assertCanManageNode(); $this->assertCanManageContext(); } + protected function assertPermission($controller, $methodName): void { + $reflectionMethod = new \ReflectionMethod($controller, $methodName); + $permissionReqs = $reflectionMethod->getAttributes(RequirePermission::class); + if ($permissionReqs) { + foreach ($permissionReqs as $permissionReqAttribute) { + /** @var RequirePermission $attribute */ + $attribute = $permissionReqAttribute->newInstance(); + $this->checkPermission($attribute); + } + } + } + + /** + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ + protected function checkPermission(RequirePermission $attribute): void { + $nodeId = $this->request->getParam($attribute->getIdParam()); + if (!is_numeric($nodeId)) { + throw new InternalError('Invalid node ID'); + } + $nodeId = (int)$nodeId; + + $nodeType = $attribute->getType() ?? $this->request->getParam($attribute->getTypeParam()); + $isContext = false; + if (!is_numeric($nodeType)) { + if ($nodeType === 'context') { + // contexts are not considered nodes, but we deal with them as well + // currently they are only passed as string as well + $isContext = true; + } else { + $nodeType = match ($nodeType) { + 'table' => Application::NODE_TYPE_TABLE, + 'view' => Application::NODE_TYPE_VIEW, + default => throw new InternalError('Invalid node type'), + }; + } + } elseif (!in_array((int)$nodeType, [Application::NODE_TYPE_TABLE, Application::NODE_TYPE_VIEW], true)) { + throw new InternalError('Invalid node type'); + } + $nodeType = (int)$nodeType; + + // pre-test: if the node is not accessible in first place, we do not have to reveal it + if ($isContext) { + if (!$this->permissionsService->canAccessContextById($nodeId, $this->userId)) { + throw new NotFoundError(); + } + } else { + if (!$this->permissionsService->canAccessNodeById($nodeType, $nodeId, $this->userId)) { + throw new NotFoundError(); + } + } + + if ($attribute->getPermission() === Application::PERMISSION_MANAGE) { + if (!$isContext) { + if (!$this->permissionsService->canManageNodeById($nodeType, $nodeId, $this->userId)) { + throw new PermissionError(sprintf('User %s cannot manage node %d (type %d)', + $this->userId, $nodeId, $nodeType + )); + } + } else { + if (!$this->permissionsService->canManageContextById($nodeId, $this->userId)) { + throw new PermissionError(sprintf('User %s cannot manage context %d', + $this->userId, $nodeId + )); + } + } + } + } + /** * @throws PermissionError * @throws InternalError diff --git a/lib/Service/PermissionsService.php b/lib/Service/PermissionsService.php index 93ed1bb8a..c10492ea9 100644 --- a/lib/Service/PermissionsService.php +++ b/lib/Service/PermissionsService.php @@ -154,7 +154,7 @@ public function canManageContextById(int $contextId, ?string $userId = null): bo } /** - * @throws Exception + * @throws NotFoundError */ public function canAccessContextById(int $contextId, ?string $userId = null): bool { try { diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index 68a3939f4..9621e2583 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -218,9 +218,6 @@ public function create(int $nodeId, string $nodeType, string $receiver, string $ $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': '.$e->getMessage()); } - // TODO Do we need to check if create is allowed for requested nodeId?! - // should also return not found if share_node could not be found - // should also return no permission if sender don't have permission to create a share for node $time = new DateTime(); $item = new Share(); $item->setSender($this->userId); diff --git a/openapi.json b/openapi.json index b2419928f..202211f0d 100644 --- a/openapi.json +++ b/openapi.json @@ -2370,7 +2370,8 @@ "type": "string", "enum": [ "table", - "view" + "view", + "context" ] } }, diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 2cac211b6..0b1d7b1c7 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1077,7 +1077,7 @@ export type operations = { /** @description Node ID */ nodeId: number; /** @description Node type */ - nodeType: "table" | "view"; + nodeType: "table" | "view" | "context"; /** @description Receiver ID */ receiver: string; /** @description Receiver type */ diff --git a/tests/integration/features/APIv1.feature b/tests/integration/features/APIv1.feature index c92f90e2a..eb2bc8bda 100644 --- a/tests/integration/features/APIv1.feature +++ b/tests/integration/features/APIv1.feature @@ -48,6 +48,15 @@ Feature: APIv1 Then user "participant2" has the following tables | Tutorial | + @api1 @table-sharing + Scenario: Inaccessible table sharing with a user + Given table "Ready to share" with emoji "🥪" exists for user "participant1" as "base1" + And user "participant3" exists + When user "participant2" attempts to share the table with user "participant3" + Then the reported status is "404" + And user "participant3" has the following tables + | Tutorial | + @api1 Scenario: Table sharing with a group Given table "Ready to share" with emoji "🥪" exists for user "participant1" as "base1" @@ -217,3 +226,35 @@ Feature: APIv1 | updated first view | When user "participant1" deletes view "first-view" Then table "view-test" has the following views for user "participant1" + + @api1 @contexts @contexts-sharing + Scenario: Share an owned context + Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1" as "t1" via v2 + And table "Table 2 via api v2" with emoji "📸" exists for user "participant1" as "t2" via v2 + And user "participant1" creates the Context "c1" with name "Enchanting Guitar" with icon "tennis" and description "Lorem ipsum dolor etc pp" and nodes: + | alias | type | permissions | + | t1 | table | read,create,update | + | t2 | table | read | + When user "participant2" attempts to fetch Context "c1" + Then the reported status is "404" + When user "participant1" shares the Context "c1" to "user" "participant2" + Then the reported status is "200" + When user "participant2" fetches Context "c1" + Then the fetched Context "c1" has following data: + | field | value | + | node | table:t1:read,create,update | + | node | table:t2:read | + + @api1 @contexts @contexts-sharing + Scenario: Share an inaccessible context + Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1" as "t1" via v2 + And table "Table 2 via api v2" with emoji "📸" exists for user "participant1" as "t2" via v2 + And user "participant1" creates the Context "c1" with name "Enchanting Guitar" with icon "tennis" and description "Lorem ipsum dolor etc pp" and nodes: + | alias | type | permissions | + | t1 | table | read,create,update | + | t2 | table | read | + And user "participant3" exists + When user "participant2" shares the Context "c1" to "user" "participant3" + Then the reported status is "404" + When user "participant3" attempts to fetch Context "c1" + Then the reported status is "404" diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index cb83822e1..425c2d4d3 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -892,6 +892,29 @@ public function deleteTable(string $user, string $keyword): void { $this->tableColumns = []; } + /** + * @When user :user attempts to share the table with user :receiver + */ + public function userAttemptsToShareTheTableWithUser(string $user, string $receiver): void { + $this->setCurrentUser($user); + + $permissions = [ + 'permissionRead' => true, + 'permissionCreate' => true, + 'permissionUpdate' => true, + 'permissionDelete' => false, + 'permissionManage' => false + ]; + $this->sendRequest( + 'POST', + sprintf('/apps/tables/api/1/tables/%d/shares', $this->tableId), + array_merge($permissions, [ + 'receiverType' => 'user', + 'receiver' => $receiver + ]) + ); + } + /** * @Then user :user shares table with user :receiver * @@ -2159,7 +2182,36 @@ public function userTransfersTheTheContextTo(string $user, string $contextAlias, $this->collectionManager->update($context, 'context', $context['id'], function () use ($context, $recipientUser) { $this->deleteContextWithFetchCheck($context['id'], $recipientUser); }); + } + } + + /** + * @When user :sharer shares the Context :contextAlias to :shareeType :sharee + */ + public function userSharesTheContextTo(string $sharer, string $contextAlias, string $shareeType, string $sharee): void { + $this->setCurrentUser($sharer); + $context = $this->collectionManager->getByAlias('context', $contextAlias); + + $this->sendRequest( + 'POST', + '/apps/tables/api/1/shares', + [ + 'nodeId' => $context['id'], + 'nodeType' => 'context', + 'receiver' => $sharee, + 'receiverType' => $shareeType, + 'displayMode' => 2, + ] + ); + + if ($this->response->getStatusCode() === 200) { + $share = $this->getDataFromResponse($this->response); + $this->shareId = $share['id']; + Assert::assertEquals($share['nodeType'], 'context'); + Assert::assertEquals($share['nodeId'], $context['id']); + Assert::assertEquals($share['receiverType'], $shareeType); + Assert::assertEquals($share['receiver'], $sharee); } }