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