From f5283e982366d9cc3f4f725dbe9b97f728ab62b3 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 30 Apr 2024 20:33:52 +0200 Subject: [PATCH 1/2] ci(integration): tests against context deletion Signed-off-by: Arthur Schiwon --- tests/integration/features/APIv2.feature | 38 ++++++++++++++++++ .../features/bootstrap/FeatureContext.php | 40 +++++++++++++++++-- 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/tests/integration/features/APIv2.feature b/tests/integration/features/APIv2.feature index 89d2f117a..854bb2d15 100644 --- a/tests/integration/features/APIv2.feature +++ b/tests/integration/features/APIv2.feature @@ -206,3 +206,41 @@ Feature: APIv2 | t1 | table | read,created,update | When user "participant1-v2" attempts to fetch Context "NON-EXISTENT" Then the reported status is "404" + + @api2 @contexts + Scenario: Delete an owned context + Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And table "Table 2 via api v2" with emoji "📸" exists for user "participant1-v2" as "t2" via v2 + And user "participant1-v2" 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,created,update | + When user "participant1-v2" deletes Context "c1" + Then the reported status is "200" + When user "participant1-v2" attempts to fetch Context "c1" + Then the reported status is "404" + + @api2 @contexts + Scenario: Delete an inaccessible context + Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And table "Table 2 via api v2" with emoji "📸" exists for user "participant1-v2" as "t2" via v2 + And user "participant1-v2" 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,created,update | + When user "participant2-v2" attempts to delete Context "c1" + Then the reported status is "404" + When user "participant2-v2" attempts to fetch Context "c1" + Then the reported status is "404" + When user "participant1-v2" fetches Context "c1" + Then the reported status is "200" + + @api2 @contexts + Scenario: Delete an non-existing context + Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And table "Table 2 via api v2" with emoji "📸" exists for user "participant1-v2" as "t2" via v2 + And user "participant1-v2" 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,created,update | + When user "participant1-v2" attempts to delete Context "NON-EXISTENT" + Then the reported status is "404" + When user "participant1-v2" attempts to fetch Context "NON-EXISTENT" + Then the reported status is "404" diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 031c487ab..5a2f84766 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -1809,7 +1809,7 @@ public function createContext(string $user, string $alias, string $name, string $newContext = $this->getDataFromResponse($this->response)['ocs']['data']; $this->collectionManager->register($newContext, 'context', $newContext['id'], $alias, function () use ($newContext) { - $this->deleteContext($newContext['id'], $newContext['owner']); + $this->deleteContextWithFetchCheck($newContext['id'], $newContext['owner']); }); Assert::assertEquals($newContext['name'], $name); @@ -1968,11 +1968,15 @@ public function deleteContext(int $contextId, string $owner): void { ); Assert::assertEquals(200, $this->response->getStatusCode()); - $deletedContext = $this->getDataFromResponse($this->response)['ocs']['data']; + } + + public function deleteContextWithFetchCheck(int $contextId, string $owner): void { + $this->deleteContext($contextId, $owner); + $this->setCurrentUser($owner); $this->sendOcsRequest( 'GET', - sprintf('/apps/tables/api/2/contexts/%d', $deletedContext['id']), + sprintf('/apps/tables/api/2/contexts/%d', $contextId), ); Assert::assertEquals(404, $this->response->getStatusCode()); } @@ -2028,4 +2032,34 @@ public function theReportedStatusIs(int $statusCode): void { Assert::assertEquals($statusCode, $this->response->getStatusCode()); } + /** + * @When user :user deletes Context :contextAlias + */ + public function userDeletesContext(string $user, string $contextAlias): void { + $context = $this->collectionManager->getByAlias('context', $contextAlias); + $this->deleteContext($context['id'], $user); + // keep the alias and id mapping, but reset the cleanup method + $this->collectionManager->update($context, 'context', $context['id'], fn () => null); + } + + /** + * @When user :user attempts to delete Context :contextAlias + */ + public function userAttemptsToDeleteContext(string $user, string $contextAlias): void { + if ($contextAlias === self::NON_EXISTING_CONTEXT_ALIAS) { + $context = ['id' => self::NON_EXISTING_CONTEXT_ID]; + } else { + $context = $this->collectionManager->getByAlias('context', $contextAlias); + } + + $exceptionCaught = false; + try { + $this->deleteContext($context['id'], $user); + } catch (ExpectationFailedException $e) { + $exceptionCaught = true; + } + + Assert::assertTrue($exceptionCaught); + } + } From b5b967cbbd4d4815366d7f801bfe4b91830c39f4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 30 Apr 2024 21:16:22 +0200 Subject: [PATCH 2/2] fix(Middleware): return reasonable status codes Signed-off-by: Arthur Schiwon --- lib/Middleware/PermissionMiddleware.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/lib/Middleware/PermissionMiddleware.php b/lib/Middleware/PermissionMiddleware.php index 49b239936..49005e15e 100644 --- a/lib/Middleware/PermissionMiddleware.php +++ b/lib/Middleware/PermissionMiddleware.php @@ -3,8 +3,10 @@ namespace OCA\Tables\Middleware; use OCA\Tables\Errors\InternalError; +use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; use OCA\Tables\Service\PermissionsService; +use OCP\AppFramework\Http; use OCP\AppFramework\Middleware; use OCP\AppFramework\Utility\IControllerMethodReflector; use OCP\IRequest; @@ -87,4 +89,14 @@ protected function assertCanManageContext(): void { } } } + + public function afterException($controller, $methodName, \Exception $exception) { + if ($exception instanceof PermissionError) { + return new Http\DataResponse(['message' => $exception->getMessage()], Http::STATUS_FORBIDDEN); + } + if ($exception instanceof NotFoundError) { + return new Http\DataResponse(['message' => $exception->getMessage()], Http::STATUS_NOT_FOUND); + } + throw $exception; + } }