From 1c2f7f332045176001c95c21190e74696deb7f36 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 4 Nov 2026 09:08:36 +0100 Subject: [PATCH] fix(contexts): answer an unknown and an unmanageable uuid alike on scheme import Importing a scheme answered 404 for a node whose uuid was unknown and 403 for a node on a table or view the user cannot manage. Such a node is now skipped the same way as an unknown one, with one log line, unless it already belongs to the application, the way the menu import skips the items it cannot use. A table to modify that is not here or cannot be managed is refused with the same 400 in both cases, and the controller now answers a refused import with 400 and rolls it back. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ruben van der Linde --- lib/Controller/ContextController.php | 3 + lib/Service/ContextService.php | 61 +++++++---- .../unit/Controller/ContextControllerTest.php | 48 ++++++++ tests/unit/Service/ContextServiceTest.php | 103 ++++++++++++++++-- 4 files changed, 188 insertions(+), 27 deletions(-) create mode 100644 tests/unit/Controller/ContextControllerTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index e7955ce01e..222c133d0b 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -376,6 +376,9 @@ public function importScheme(int $contextId, string $name, string $iconName, str $context = $this->contextService->importScheme($contextId, $name, $iconName, $description, $nodes, $tables, $this->userId, $menu); $this->db->commit(); return new DataResponse($context->jsonSerialize()); + } catch (BadRequestError $e) { + $this->db->rollBack(); + return $this->handleBadRequestError($e); } catch (\InvalidArgumentException $e) { $this->db->rollBack(); return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 12ffa5e96a..26fea12b1e 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -23,6 +23,7 @@ use OCA\Tables\Db\PageMapper; use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Errors\InternalError; @@ -786,14 +787,23 @@ private function logSkippedMenuItems(array $skipped): void { } /** - * The node relation of an application for an exported table or view, found by uuid. + * The table or view (by kind, `table` or `view`) with a uuid, or null when it is not here. + * + * @throws Exception */ - private function nodeRelIdByUuid(Context $context, string $kind, string $uuid): ?int { + private function findNodeByUuid(string $kind, string $uuid): Table|View|null { try { - $node = $kind === 'table' ? $this->tableMapper->findByUuid($uuid) : $this->viewMapper->findByUuid($uuid); + return $kind === 'table' ? $this->tableMapper->findByUuid($uuid) : $this->viewMapper->findByUuid($uuid); } catch (DoesNotExistException) { return null; } + } + + /** + * The node relation of an application for an exported table or view, found by uuid. + */ + private function nodeRelIdByUuid(Context $context, string $kind, string $uuid): ?int { + $node = $this->findNodeByUuid($kind, $uuid); if ($node === null) { return null; } @@ -1119,28 +1129,41 @@ public function importScheme(int $contextId, string $name, string $iconName, str throw new BadRequestError('Table scheme must include a UUID for modification.'); } $tableService->validateTableScheme($tableScheme); - $table = $this->tableMapper->findByUuid($tableScheme['uuid']); + $table = $this->findNodeByUuid('table', $tableScheme['uuid']); + if ($table === null || !$this->permissionsService->canManageNodeById(Application::NODE_TYPE_TABLE, $table->getId(), $userId)) { + throw new BadRequestError('The table ' . $tableScheme['uuid'] . ' to modify is not here or cannot be managed by the user.'); + } $tableService->updateTableStructure($table->getId(), $tableScheme['columns'], $tableScheme['views'], $tableScheme['columnOrder'], $tableScheme['sort'], $userId); } - // Resolve node ids for the new tables and views + // A node whose table or view is not here, or is new to the application and cannot be + // managed by the user, is skipped the same way, like a menu item. + $currentNodes = []; + foreach ($this->contextMapper->findById($contextId, $userId)->getNodes() ?? [] as $node) { + $currentNodes[sprintf('t%di%d', $node['node_type'], $node['node_id'])] = true; + } $resolvedNodes = []; foreach ($nodes as $node) { - if ($node['node_type'] === Application::NODE_TYPE_TABLE) { - $table = $this->tableMapper->findByUuid($node['node_uuid']); - $resolvedNodes[] = [ - 'id' => $table->getId(), - 'type' => $node['node_type'], - 'permissions' => $node['permissions'], - ]; - } elseif ($node['node_type'] === Application::NODE_TYPE_VIEW) { - $view = $this->viewMapper->findByUuid($node['node_uuid']); - $resolvedNodes[] = [ - 'id' => $view->getId(), - 'type' => $node['node_type'], - 'permissions' => $node['permissions'], - ]; + $kind = match ($node['node_type']) { + Application::NODE_TYPE_TABLE => 'table', + Application::NODE_TYPE_VIEW => 'view', + default => null, + }; + if ($kind === null) { + continue; + } + $found = $this->findNodeByUuid($kind, $node['node_uuid']); + if ($found === null + || (!isset($currentNodes[sprintf('t%di%d', $node['node_type'], $found->getId())]) + && !$this->permissionsService->canManageNodeById($node['node_type'], $found->getId(), $userId))) { + $this->logger->warning('Skipping a node of an imported scheme: no ' . $kind . ' with the uuid ' . $node['node_uuid'] . ' is here and can be managed by the user.'); + continue; } + $resolvedNodes[] = [ + 'id' => $found->getId(), + 'type' => $node['node_type'], + 'permissions' => $node['permissions'], + ]; } $context = $this->update($contextId, $userId, $name, $iconName, $description, $resolvedNodes); diff --git a/tests/unit/Controller/ContextControllerTest.php b/tests/unit/Controller/ContextControllerTest.php new file mode 100644 index 0000000000..9a8c1c7577 --- /dev/null +++ b/tests/unit/Controller/ContextControllerTest.php @@ -0,0 +1,48 @@ +createMock(ContextService::class); + $contextService->method('importScheme')->willThrowException(new BadRequestError('The table to modify is not here.')); + $db = $this->createMock(IDBConnection::class); + $db->expects($this->once())->method('rollBack'); + $controller = new ContextController( + $this->createMock(IRequest::class), + $this->createMock(LoggerInterface::class), + $this->createMock(IL10N::class), + 'user-1', + $contextService, + $this->createMock(TableService::class), + $db, + $this->createMock(ColumnService::class), + $this->createMock(ViewService::class), + ); + + $response = $controller->importScheme(7, 'Intake', 'star', '', [], ['addTables' => [], 'modifyTables' => []]); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame('The table to modify is not here.', $response->getData()['message']); + } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index ba5ae6d882..3cc64bf066 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -22,8 +22,11 @@ use OCA\Tables\Db\PageMapper; use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Service\Support\AuditLogServiceInterface; +use OCP\AppFramework\Db\DoesNotExistException; use OCP\IDBConnection; use OCP\IL10N; use OCP\INavigationManager; @@ -43,6 +46,12 @@ final class ContextServiceTest extends TestCase { private MockObject $auditLogService; private MockObject $widgetService; private MockObject $dbConnection; + private MockObject $logger; + private MockObject $permissionsService; + private MockObject $tableMapper; + private MockObject $viewMapper; + /** @var list */ + private array $serviceArguments; protected function setUp(): void { parent::setUp(); @@ -51,8 +60,10 @@ protected function setUp(): void { $this->contextNodeRelMapper = $this->createMock(ContextNodeRelationMapper::class); $this->pageMapper = $this->createMock(PageMapper::class); $this->pageContentMapper = $this->createMock(PageContentMapper::class); - $logger = $this->createMock(LoggerInterface::class); - $permissionsService = $this->createMock(PermissionsService::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->permissionsService = $this->createMock(PermissionsService::class); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->viewMapper = $this->createMock(ViewMapper::class); $this->userManager = $this->createMock(IUserManager::class); $this->auditLogService = $this->createMock(AuditLogServiceInterface::class); $this->dbConnection = $this->createMock(IDBConnection::class); @@ -63,13 +74,13 @@ protected function setUp(): void { $navigationManager = $this->createMock(INavigationManager::class); $urlGenerator = $this->createMock(IURLGenerator::class); - $this->service = new ContextService( + $this->serviceArguments = [ $this->contextMapper, $this->contextNodeRelMapper, $this->pageMapper, $this->pageContentMapper, - $logger, - $permissionsService, + $this->logger, + $this->permissionsService, $this->userManager, $this->auditLogService, $this->dbConnection, @@ -77,13 +88,14 @@ protected function setUp(): void { false, $navigationManager, $urlGenerator, - $this->createMock(TableMapper::class), - $this->createMock(ViewMapper::class), + $this->tableMapper, + $this->viewMapper, $this->widgetService, new GridWidgetTypes($l10n), $this->createMock(MenuItemMapper::class), $this->createMock(MenuService::class), - ); + ]; + $this->service = new ContextService(...$this->serviceArguments); } public function testTransferLogsPreviousAndNewOwner(): void { @@ -600,4 +612,79 @@ public function testAnImportedPageKeepsAtMostTheMaximumNumberOfWidgets(): void { self::assertCount(GridWidgetTypes::MAX_WIDGETS_PER_PAGE, $record['widgets']); } + + /** + * @return array + */ + public static function tablesThatCannotBeModified(): array { + return [ + 'not here' => [null], + 'here but cannot be managed' => [5], + ]; + } + + /** + * @dataProvider tablesThatCannotBeModified + */ + public function testImportingASchemeRefusesToModifyATableThatIsNotHereOrCannotBeManagedAlike(?int $tableId): void { + $this->tableMapper->method('findByUuid')->willReturnCallback(static function () use ($tableId): Table { + if ($tableId === null) { + throw new DoesNotExistException('No table'); + } + $table = new Table(); + $table->id = $tableId; + return $table; + }); + $this->permissionsService->method('canManageNodeById')->willReturn(false); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('The table some-uuid to modify is not here or cannot be managed by the user.'); + $this->service->importScheme(7, 'Intake', 'star', '', [], ['addTables' => [], 'modifyTables' => [['uuid' => 'some-uuid', 'title' => 'T', 'columns' => [], 'views' => [], 'columnOrder' => [], 'sort' => []]]], 'user-1'); + } + + public function testImportingASchemeSkipsANodeThatIsNotHereOrCannotBeManagedAlike(): void { + $context = new Context(); + $context->id = 7; + $context->setNodes([101 => ['id' => 101, 'node_id' => 3, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]]); + $this->contextMapper->method('findById')->willReturn($context); + $tables = []; + foreach (['shared-but-already-a-node' => 3, 'manageable' => 4, 'not-manageable' => 5] as $uuid => $id) { + $tables[$uuid] = new Table(); + $tables[$uuid]->id = $id; + } + $this->tableMapper->method('findByUuid')->willReturnCallback(static fn (string $uuid): Table => $tables[$uuid] ?? throw new DoesNotExistException('No table')); + $view = new View(); + $view->id = 6; + $this->viewMapper->method('findByUuid')->willReturnCallback(static fn (string $uuid): View => $uuid === 'not-manageable-view' ? $view : throw new DoesNotExistException('No view')); + $this->permissionsService->method('canManageNodeById')->willReturnCallback(static fn (int $type, int $id): bool => $type === Application::NODE_TYPE_TABLE && $id === 4); + $warnings = []; + $this->logger->method('warning')->willReturnCallback(static function (string $message) use (&$warnings): void { + $warnings[] = $message; + }); + $node = static fn (int $type, string $uuid): array => ['node_type' => $type, 'node_uuid' => $uuid, 'permissions' => 1]; + + $service = $this->getMockBuilder(ContextService::class)->setConstructorArgs($this->serviceArguments)->onlyMethods(['update'])->getMock(); + $service->expects($this->once())->method('update') + ->with(7, 'user-1', 'Intake', 'star', '', [ + ['id' => 3, 'type' => Application::NODE_TYPE_TABLE, 'permissions' => 1], + ['id' => 4, 'type' => Application::NODE_TYPE_TABLE, 'permissions' => 1], + ]) + ->willReturn($context); + + $service->importScheme(7, 'Intake', 'star', '', [ + $node(Application::NODE_TYPE_TABLE, 'shared-but-already-a-node'), + $node(Application::NODE_TYPE_TABLE, 'manageable'), + $node(Application::NODE_TYPE_TABLE, 'not-manageable'), + $node(Application::NODE_TYPE_TABLE, 'missing'), + $node(Application::NODE_TYPE_VIEW, 'not-manageable-view'), + $node(Application::NODE_TYPE_VIEW, 'missing-view'), + ], ['addTables' => [], 'modifyTables' => []], 'user-1'); + + $this->assertSame([ + 'Skipping a node of an imported scheme: no table with the uuid not-manageable is here and can be managed by the user.', + 'Skipping a node of an imported scheme: no table with the uuid missing is here and can be managed by the user.', + 'Skipping a node of an imported scheme: no view with the uuid not-manageable-view is here and can be managed by the user.', + 'Skipping a node of an imported scheme: no view with the uuid missing-view is here and can be managed by the user.', + ], $warnings); + } }