Merge pull request #1129 from nextcloud/enh/noid/integration-tests-sh…

…aring

ci(integration): test against context sharing

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());

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,

lib/Middleware/Attribute/RequirePermission.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace OCA\Tables\Middleware\Attribute;
+
+use Attribute;
+
+#[Attribute(Attribute::TARGET_METHOD)]
+class RequirePermission {
+	public function __construct(
+		protected int $permission,
+		protected ?int $type = null,
+		protected string $typeParam = 'nodeType',
+		protected string $idParam = 'nodeId',
+	) {
+	}
+
+	public function getPermission(): int {
+		return $this->permission;
+	}
+
+	public function getTypeParam(): string {
+		return $this->typeParam;
+	}
+
+	public function getIdParam(): string {
+		return $this->idParam;
+	}
+
+	public function getType(): ?int {
+		return $this->type;
+	}
+}

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

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 {

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);

openapi.json

@@ -2370,7 +2370,8 @@
                             "type": "string",
                             "enum": [
                                 "table",
-                                "view"
+                                "view",
+                                "context"
                             ]
                         }
                     },

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 */

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"

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);
 		}
 	}