From a62dd8ec158c1a44e182cc68393497a2a92dfc41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 22 Feb 2024 17:45:26 +0100 Subject: [PATCH 1/5] fix: Proper error handling and validation of passed in node type and node id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl tmp: revert to work around segfault Signed-off-by: Julius Härtl --- lib/Command/RenameTable.php | 2 +- lib/Controller/ApiFavoriteController.php | 30 ++++++----- lib/Service/FavoritesService.php | 68 ++++++++++++++++++++++-- lib/Service/PermissionsService.php | 12 +++++ 4 files changed, 93 insertions(+), 19 deletions(-) diff --git a/lib/Command/RenameTable.php b/lib/Command/RenameTable.php index f2a3f57bf..3a41153b5 100644 --- a/lib/Command/RenameTable.php +++ b/lib/Command/RenameTable.php @@ -45,7 +45,7 @@ public function __construct(TableService $tableService, LoggerInterface $logger) protected function configure(): void { $this ->setName('tables:update') - ->setAliases('tables:rename') + ->setAliases(['tables:rename']) ->setDescription('Rename a table.') ->addArgument( 'ID', diff --git a/lib/Controller/ApiFavoriteController.php b/lib/Controller/ApiFavoriteController.php index 24b19f082..df36b54b3 100644 --- a/lib/Controller/ApiFavoriteController.php +++ b/lib/Controller/ApiFavoriteController.php @@ -10,6 +10,7 @@ use OCA\Tables\Service\FavoritesService; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; +use OCP\DB\Exception as DBException; use OCP\IL10N; use OCP\IRequest; use Psr\Log\LoggerInterface; @@ -35,19 +36,21 @@ public function __construct( * * @NoAdminRequired * - * @param string $title Title of the table - * @param string|null $emoji Emoji for the table - * @param string $template Template to use if wanted - * - * @return DataResponse|DataResponse + * @param int $nodeType + * @param int $nodeId + * @return DataResponse|DataResponse * * 200: Tables returned */ public function create(int $nodeType, int $nodeId): DataResponse { try { $this->service->addFavorite($nodeType, $nodeId); - return new DataResponse(['ok']); - } catch (InternalError|Exception $e) { + return new DataResponse(); + } catch (NotFoundError $e) { + return $this->handleNotFoundError($e); + } catch (PermissionError $e) { + return $this->handlePermissionError($e); + } catch (InternalError|DBException|Exception $e) { return $this->handleError($e); } } @@ -58,8 +61,9 @@ public function create(int $nodeType, int $nodeId): DataResponse { * * @NoAdminRequired * - * @param int $id Table ID - * @return DataResponse|DataResponse + * @param int $nodeType + * @param int $nodeId + * @return DataResponse|DataResponse * * 200: Deleted table returned * 403: No permissions @@ -68,13 +72,13 @@ public function create(int $nodeType, int $nodeId): DataResponse { public function destroy(int $nodeType, int $nodeId): DataResponse { try { $this->service->removeFavorite($nodeType, $nodeId); - return new DataResponse(['ok']); + return new DataResponse(); + } catch (NotFoundError $e) { + return $this->handleNotFoundError($e); } catch (PermissionError $e) { return $this->handlePermissionError($e); - } catch (InternalError $e) { + } catch (InternalError|DBException|Exception $e) { return $this->handleError($e); - } catch (NotFoundError $e) { - return $this->handleNotFoundError($e); } } } diff --git a/lib/Service/FavoritesService.php b/lib/Service/FavoritesService.php index 4c8b1fc5e..8a9a7b6da 100644 --- a/lib/Service/FavoritesService.php +++ b/lib/Service/FavoritesService.php @@ -24,24 +24,39 @@ namespace OCA\Tables\Service; +use OCA\Tables\AppInfo\Application; +use OCA\Tables\Errors\InternalError; +use OCA\Tables\Errors\NotFoundError; +use OCA\Tables\Errors\PermissionError; use OCP\Cache\CappedMemoryCache; +use OCP\DB\Exception; use OCP\IDBConnection; class FavoritesService { + private IDBConnection $connection; + private PermissionsService $permissionsService; + private ?string $userId; private CappedMemoryCache $cache; - public function __construct(private IDBConnection $connection, private ?string $userId) { + public function __construct( + IDBConnection $connection, + PermissionsService $permissionsService, + ?string $userId + ) { + $this->connection = $connection; + $this->permissionsService = $permissionsService; + $this->userId = $userId; $this->cache = new CappedMemoryCache(); } public function isFavorite(int $nodeType, int $id): bool { - if ($cached = $this->cache->get($this->userId . '_' . $nodeType . '_' . $id)) { + $cacheKey = $this->userId . '_' . $nodeType . '_' . $id; + if ($cached = $this->cache->get($cacheKey)) { return $cached; } - // We still might run this multiple times - + $this->cache->clear(); $qb = $this->connection->getQueryBuilder(); $qb->select('*') ->from('tables_favorites') @@ -52,10 +67,24 @@ public function isFavorite(int $nodeType, int $id): bool { $this->cache->set($this->userId . '_' . $row['node_type'] . '_' . $row['node_id'], true); } - return $this->cache->get($this->userId . '_' . $nodeType . '_' . $id) ?? false; + // Set the cache for not found matches still to avoid further queries + if (!$this->cache->get($cacheKey)) { + $this->cache->set($cacheKey, false); + } + + return $this->cache->get($cacheKey); } + /** + * @throws Exception + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ public function addFavorite(int $nodeType, int $id): void { + $this->checkValidNodeType($nodeType); + $this->checkAccessToNode($nodeType, $id); + $qb = $this->connection->getQueryBuilder(); $qb->insert('tables_favorites') ->values([ @@ -67,7 +96,16 @@ public function addFavorite(int $nodeType, int $id): void { $this->cache->set($this->userId . '_' . $nodeType . '_' . $id, true); } + /** + * @throws Exception + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ public function removeFavorite(int $nodeType, int $id): void { + $this->checkValidNodeType($nodeType); + $this->checkAccessToNode($nodeType, $id); + $qb = $this->connection->getQueryBuilder(); $qb->delete('tables_favorites') ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($this->userId))) @@ -77,4 +115,24 @@ public function removeFavorite(int $nodeType, int $id): void { $this->cache->set($this->userId . '_' . $nodeType . '_' . $id, false); } + /** + * @throws InternalError + */ + private function checkValidNodeType(int $nodeType): void { + if (!in_array($nodeType, [Application::NODE_TYPE_TABLE, Application::NODE_TYPE_VIEW])) { + throw new InternalError('Invalid node type'); + } + } + + /** + * @throws PermissionError + */ + private function checkAccessToNode(int $nodeType, int $nodeId): void { + if ($this->permissionsService->canAccessNodeById($nodeType, $nodeId)) { + return; + } + + throw new PermissionError('Invalid node type and id'); + } + } diff --git a/lib/Service/PermissionsService.php b/lib/Service/PermissionsService.php index 8b3c851de..989c463cf 100644 --- a/lib/Service/PermissionsService.php +++ b/lib/Service/PermissionsService.php @@ -2,6 +2,7 @@ namespace OCA\Tables\Service; +use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Share; use OCA\Tables\Db\ShareMapper; use OCA\Tables\Db\Table; @@ -95,6 +96,17 @@ public function canUpdateTable(Table $table, ?string $userId = null): bool { return $this->canManageTable($table, $userId); } + public function canAccessNodeById(int $nodeType, int $nodeId, ?string $userId = null): bool { + if ($nodeType === Application::NODE_TYPE_TABLE) { + return $this->canReadColumnsByTableId($nodeId, $this->userId); + } + if ($nodeType === Application::NODE_TYPE_VIEW) { + return $this->canReadColumnsByViewId($nodeId, $this->userId); + } + + return false; + } + public function canAccessView(View $view, ?string $userId = null): bool { if($this->basisCheck($view, 'view', $userId)) { return true; From 7b2b70d9c64b0c7171c7ac7fd949acde604060e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 22 Feb 2024 17:45:48 +0100 Subject: [PATCH 2/5] chore: Make psalm output more readable for the main usage when we are only interested in errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index b8d66398a..7d7694377 100644 --- a/composer.json +++ b/composer.json @@ -35,7 +35,7 @@ "lint": "find . -name \\*.php -not -path './vendor/*' -not -path './build/*' -print0 | xargs -0 -n1 php -l", "cs:check": "php-cs-fixer fix --dry-run --diff", "cs:fix": "php-cs-fixer fix", - "psalm": "./vendor/bin/psalm.phar --show-info=true --no-cache", + "psalm": "./vendor/bin/psalm.phar --show-info=false --no-cache", "psalm:update-baseline": "./vendor/bin/psalm.phar --update-baseline", "psalm:fix": "./vendor/bin/psalm.phar --no-cache --alter --issues=InvalidReturnType,InvalidNullableReturnType,MismatchingDocblockParamType,MismatchingDocblockReturnType,MissingParamType,InvalidFalsableReturnType", "psalm:fix:dry": "./vendor/bin/psalm.phar --no-cache --alter --issues=InvalidReturnType,InvalidNullableReturnType,MismatchingDocblockParamType,MismatchingDocblockReturnType,MissingParamType,InvalidFalsableReturnType --dry-run", From d962301c4f53752fa46730ab049755ee930d65ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 22 Feb 2024 17:47:20 +0100 Subject: [PATCH 3/5] feat: Expose api feature flags thorugh capabilities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Capabilities.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/Capabilities.php b/lib/Capabilities.php index 2d7573ad2..f80adc6b5 100644 --- a/lib/Capabilities.php +++ b/lib/Capabilities.php @@ -46,7 +46,7 @@ public function __construct(IAppManager $appManager, LoggerInterface $logger, IC /** * - * @return array{tables: array{enabled: bool, version: string, apiVersions: string[], column_types: string[]}} + * @return array{tables: array{enabled: bool, version: string, apiVersions: string[], features: string[], column_types: string[]}} * * @inheritDoc */ @@ -63,6 +63,10 @@ public function getCapabilities(): array { 'apiVersions' => [ '1.0' ], + 'features' => [ + 'favorite', + 'archive', + ], 'column_types' => [ 'text-line', $textColumnVariant, From 78738e34ebe36f41e888ef0187b170c93a22d1ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 23 Feb 2024 09:27:14 +0100 Subject: [PATCH 4/5] tests: Add integration tests for archive and favorites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- tests/integration/features/APIv2.feature | 27 ++++ .../features/bootstrap/FeatureContext.php | 117 ++++++++++++++++-- 2 files changed, 131 insertions(+), 13 deletions(-) diff --git a/tests/integration/features/APIv2.feature b/tests/integration/features/APIv2.feature index 7afbabba7..1848ce77e 100644 --- a/tests/integration/features/APIv2.feature +++ b/tests/integration/features/APIv2.feature @@ -2,6 +2,7 @@ Feature: APIv2 Background: Given user "participant1-v2" exists Given user "participant2-v2" exists + Given user "participant3-v2" exists @api2 Scenario: Test initial setup @@ -15,11 +16,37 @@ Feature: APIv2 Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 Then user "participant1-v2" has the following tables via v2 | Table 1 via api v2 | + And user "participant1-v2" sees the following table attributes on table "t1" + | archived | 0 | Then user "participant1-v2" updates table "t1" set title "updated title" and emoji "⛵︎" via v2 Then user "participant1-v2" has the following tables via v2 | updated title | + Then user "participant1-v2" updates table "t1" set archived 1 via v2 + And user "participant1-v2" sees the following table attributes on table "t1" + | archived | 1 | + Then user "participant1-v2" updates table "t1" set archived 0 via v2 + And user "participant1-v2" sees the following table attributes on table "t1" + | archived | 0 | Then user "participant1-v2" deletes table "t1" via v2 + @api2 + Scenario: Favorite tables + Given table "Own table" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And user "participant1-v2" shares table with user "participant2-v2" + And user "participant1-v2" adds the table "t1" to favorites + And user "participant1-v2" sees the following table attributes on table "t1" + | favorite | 1 | + And user "participant2-v2" fetches table info for table "t1" + And user "participant2-v2" sees the following table attributes on table "t1" + | favorite | 0 | + When user "participant1-v2" removes the table "t1" from favorites + And user "participant1-v2" sees the following table attributes on table "t1" + | favorite | 0 | + When user "participant3-v2" adds the table "t1" to favorites + Then the last response should have a "403" status code + + + @api2 Scenario: Basic column actions Given table "Table 2" with emoji "👋" exists for user "participant1-v2" as "t2" via v2 diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 959cbc870..7a4757e24 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -70,6 +70,10 @@ class FeatureContext implements Context { private array $viewIds = []; private array $columnIds = []; + // Store data from last request to perform assertions, id is used as a key + private array $tableData = []; + private array $viewData = []; + // use CommandLineTrait; /** @@ -127,16 +131,30 @@ public function createTableV2(string $user, string $title, string $tableName, st Assert::assertEquals($newTable['emoji'], $emoji); Assert::assertEquals($newTable['ownership'], $user); + $tableToVerify = $this->userFetchesTableInfo($user, $tableName); + Assert::assertEquals(200, $this->response->getStatusCode()); + Assert::assertEquals($tableToVerify['title'], $title); + Assert::assertEquals($tableToVerify['emoji'], $emoji); + Assert::assertEquals($tableToVerify['ownership'], $user); + } + + /** + * @Given user :user fetches table info for table :tableName + */ + public function userFetchesTableInfo($user, $tableName) { + $this->setCurrentUser($user); + $tableId = $this->tableIds[$tableName]; + $this->sendOcsRequest( 'GET', - '/apps/tables/api/2/tables/'.$newTable['id'], + '/apps/tables/api/2/tables/'.$tableId, ); $tableToVerify = $this->getDataFromResponse($this->response)['ocs']['data']; - Assert::assertEquals(200, $this->response->getStatusCode()); - Assert::assertEquals($tableToVerify['title'], $title); - Assert::assertEquals($tableToVerify['emoji'], $emoji); - Assert::assertEquals($tableToVerify['ownership'], $user); + $this->tableData[$tableName] = $tableToVerify; + $this->tableId = $tableToVerify['id']; + + return $tableToVerify; } /** @@ -223,6 +241,7 @@ public function initialResourcesV2(string $user, TableNode $body = null): void { /** * @Then user :user updates table :tableName set title :title and emoji :emoji via v2 + * @Then user :user updates table :tableName set archived :archived via v2 * * @param string $user * @param string $title @@ -230,13 +249,26 @@ public function initialResourcesV2(string $user, TableNode $body = null): void { * @param string $tableName * @throws Exception */ - public function updateTableV2(string $user, string $title, ?string $emoji, string $tableName): void { + public function updateTableV2(string $user, string $tableName, string $title = null, ?string $emoji = null, ?bool $archived = null): void { $this->setCurrentUser($user); - $data = ['title' => $title]; + $this->sendOcsRequest( + 'GET', + '/apps/tables/api/2/tables/'.$this->tableIds[$tableName], + ); + + $previousData = $this->getDataFromResponse($this->response)['ocs']['data']; + + $data = []; + if ($title !== null) { + $data['title'] = $title; + } if ($emoji !== null) { $data['emoji'] = $emoji; } + if ($archived !== null) { + $data['archived'] = $archived; + } $this->sendOcsRequest( 'PUT', @@ -247,9 +279,10 @@ public function updateTableV2(string $user, string $title, ?string $emoji, strin $updatedTable = $this->getDataFromResponse($this->response)['ocs']['data']; Assert::assertEquals(200, $this->response->getStatusCode()); - Assert::assertEquals($updatedTable['title'], $title); - Assert::assertEquals($updatedTable['emoji'], $emoji); - Assert::assertEquals($updatedTable['ownership'], $user); + Assert::assertEquals($updatedTable['title'], $title ?? $previousData['title']); + Assert::assertEquals($updatedTable['emoji'], $emoji ?? $previousData['emoji']); + Assert::assertEquals($updatedTable['ownership'], $user ?? $previousData['ownership']); + Assert::assertEquals($updatedTable['archived'], $archived ?? $previousData['archived']); $this->sendOcsRequest( 'GET', @@ -258,9 +291,12 @@ public function updateTableV2(string $user, string $title, ?string $emoji, strin $tableToVerify = $this->getDataFromResponse($this->response)['ocs']['data']; Assert::assertEquals(200, $this->response->getStatusCode()); - Assert::assertEquals($tableToVerify['title'], $title); - Assert::assertEquals($tableToVerify['emoji'], $emoji); - Assert::assertEquals($tableToVerify['ownership'], $user); + Assert::assertEquals($tableToVerify['title'], $title ?? $previousData['title']); + Assert::assertEquals($tableToVerify['emoji'], $emoji ?? $previousData['emoji']); + Assert::assertEquals($tableToVerify['ownership'], $user ?? $previousData['ownership']); + Assert::assertEquals($tableToVerify['archived'], $archived ?? $previousData['archived']); + + $this->tableData[$tableName] = $tableToVerify; } /** @@ -1634,4 +1670,59 @@ protected function assertStatusCode(ResponseInterface $response, int $statusCode Assert::assertEquals($statusCode, $response->getStatusCode(), $message); } } + + /** + * @Given user :user sees the following table attributes on table :tableName + */ + public function userSeesTheFollowingTableAttributesOnTable($user, $tableName, TableNode $table) { + foreach ($table->getRows() as $row) { + $attribute = $row[0]; + $value = $row[1]; + if (in_array($attribute, ['archived', 'favorite'])) { + $value = (bool)$value; + } + Assert::assertEquals($value, $this->tableData[$tableName][$attribute]); + } + } + + /** + * @Given user :user adds the table :tableName to favorites + */ + public function userAddsTheTableToFavorites($user, $tableName) { + $this->setCurrentUser($user); + $nodeType = 0; + $tableId = $this->tableIds[$tableName]; + + $this->sendOcsRequest( + 'POST', + '/apps/tables/api/2/favorites/' . $nodeType. '/' . $tableId, + ); + if ($this->response->getStatusCode() === 200) { + $this->userFetchesTableInfo($user, $tableName); + } + } + + /** + * @Given user :user removes the table :tableName from favorites + */ + public function userRemovesTheTableFromFavorites($user, $tableName) { + $this->setCurrentUser($user); + $nodeType = 0; + $tableId = $this->tableIds[$tableName]; + + $this->sendOcsRequest( + 'DELETE', + '/apps/tables/api/2/favorites/' . $nodeType. '/' . $tableId, + ); + if ($this->response->getStatusCode() === 200) { + $this->userFetchesTableInfo($user, $tableName); + } + } + + /** + * @Then /^the last response should have a "([^"]*)" status code$/ + */ + public function theLastResponseShouldHaveAStatusCode(int $statusCode) { + Assert::assertEquals($statusCode, $this->response->getStatusCode()); + } } From 9bdf855f8ee4b9a89121ba77f11bfc7afed1c66d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 23 Feb 2024 09:37:44 +0100 Subject: [PATCH 5/5] fixup! fix: Proper error handling and validation of passed in node type and node id --- lib/Controller/ApiFavoriteController.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Controller/ApiFavoriteController.php b/lib/Controller/ApiFavoriteController.php index df36b54b3..6c08cec6a 100644 --- a/lib/Controller/ApiFavoriteController.php +++ b/lib/Controller/ApiFavoriteController.php @@ -32,7 +32,7 @@ public function __construct( } /** - * [api v2] Create a new table and return it + * [api v2] Add a node (table or view) to user favorites * * @NoAdminRequired * @@ -57,7 +57,7 @@ public function create(int $nodeType, int $nodeId): DataResponse { /** - * [api v2] Delete a table + * [api v2] Remove a node (table or view) to from favorites * * @NoAdminRequired *