From 91d899ff1ee090161a4b257964aa078a2fc8c880 Mon Sep 17 00:00:00 2001 From: Benjamin Frueh Date: Wed, 21 Oct 2026 13:22:12 +0200 Subject: [PATCH] refactor: consolidate relation target resolution Signed-off-by: Benjamin Frueh Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Db/Column.php | 3 + lib/Service/ColumnService.php | 98 ++++++++++++++++++- lib/Service/PermissionsService.php | 9 ++ lib/Service/RelationService.php | 19 +++- tests/integration/features/APIv1.feature | 46 +++++++++ .../features/bootstrap/FeatureContext.php | 93 ++++++------------ tests/unit/Service/PermissionsServiceTest.php | 46 +++++++++ 7 files changed, 246 insertions(+), 68 deletions(-) diff --git a/lib/Db/Column.php b/lib/Db/Column.php index a039dd33c7..3e0ddf5b04 100644 --- a/lib/Db/Column.php +++ b/lib/Db/Column.php @@ -121,6 +121,9 @@ class Column extends EntitySuper implements JsonSerializable { public const RELATION_TYPE = 'relationType'; public const RELATION_TARGET_ID = 'targetId'; public const RELATION_LABEL_COLUMN = 'labelColumn'; + public const RELATION_TARGET_SET_BY = 'targetSetBy'; + public const RELATION_TYPE_TABLE = 'table'; + public const RELATION_TYPE_VIEW = 'view'; protected ?string $uuid = null; protected ?string $title = null; diff --git a/lib/Service/ColumnService.php b/lib/Service/ColumnService.php index 7bb5c79d59..37166f471b 100644 --- a/lib/Service/ColumnService.php +++ b/lib/Service/ColumnService.php @@ -294,6 +294,7 @@ public function create( $item = Column::fromDto($columnDto); $item->setTitle($newTitle); $item->setTableId($table->getId()); + $item->setCustomSettings($this->authorizeRelationTarget($item->getCustomSettings(), $userId)); $this->updateMetadata($item, $userId, true); try { @@ -423,7 +424,7 @@ public function update( $item->setUsergroupSelectTeams($columnDto->getUsergroupSelectTeams()); $item->setShowUserStatus($columnDto->getShowUserStatus()); $this->validateCustomSettings($columnDto->getCustomSettings()); - $item->setCustomSettings($columnDto->getCustomSettings()); + $item->setCustomSettings($this->authorizeRelationTarget($columnDto->getCustomSettings(), $userId, $item->getCustomSettings())); $this->updateMetadata($item, $userId); try { @@ -446,7 +447,7 @@ public function update( } catch (\OCP\DB\Exception $e) { $this->handleColumnPersistDbException($e, static::class . ' - ' . __FUNCTION__); } - } catch (BadRequestError $e) { + } catch (BadRequestError|PermissionError|NotFoundError $e) { throw $e; } catch (Exception $e) { $this->logger->error($e->getMessage()); @@ -507,6 +508,97 @@ private function validateCustomSettings(?string $customSettings): void { } } + private function hasRelationTarget(array $settings): bool { + return !empty($settings[Column::RELATION_TYPE]) + && !empty($settings[Column::RELATION_TARGET_ID]) + && !empty($settings[Column::RELATION_LABEL_COLUMN]); + } + + /** + * @return array{type: string, targetId: int, labelColumn: int}|null + */ + private function parseRelationTarget(?string $customSettings): ?array { + $settings = $customSettings !== null ? json_decode($customSettings, true) : null; + if (!is_array($settings) || !$this->hasRelationTarget($settings)) { + return null; + } + + return [ + 'type' => (string)$settings[Column::RELATION_TYPE], + 'targetId' => (int)$settings[Column::RELATION_TARGET_ID], + 'labelColumn' => (int)$settings[Column::RELATION_LABEL_COLUMN], + ]; + } + + private function setRelationTargetAuthor(?string $customSettings, ?string $userId): ?string { + if ($customSettings === null) { + return null; + } + $settings = json_decode($customSettings, true); + if (!is_array($settings)) { + return $customSettings; + } + + unset($settings[Column::RELATION_TARGET_SET_BY]); + if ((string)$userId !== '' && $this->hasRelationTarget($settings)) { + $settings[Column::RELATION_TARGET_SET_BY] = $userId; + } + + return json_encode($settings, JSON_THROW_ON_ERROR); + } + + /** + * @throws BadRequestError + * @throws NotFoundError + * @throws PermissionError + */ + private function authorizeRelationTarget(?string $customSettings, ?string $userId, ?string $storedSettings = null): ?string { + $target = $this->parseRelationTarget($customSettings); + if ($target !== null && $target === $this->parseRelationTarget($storedSettings)) { + return $this->setRelationTargetAuthor($customSettings, $this->getRelationTargetAuthor($storedSettings)); + } + if ($target !== null) { + $this->assertCanReadRelationTarget($target['type'], $target['targetId'], $target['labelColumn'], $userId); + } + return $this->setRelationTargetAuthor($customSettings, $userId); + } + + private function getRelationTargetAuthor(?string $customSettings): ?string { + $settings = $customSettings !== null ? json_decode($customSettings, true) : null; + return is_array($settings) && isset($settings[Column::RELATION_TARGET_SET_BY]) ? (string)$settings[Column::RELATION_TARGET_SET_BY] : null; + } + + /** + * @throws BadRequestError + * @throws NotFoundError + * @throws PermissionError + */ + private function assertCanReadRelationTarget(string $relationType, int $targetId, int $labelColumnId, ?string $userId): void { + if (!in_array($relationType, [Column::RELATION_TYPE_TABLE, Column::RELATION_TYPE_VIEW], true)) { + throw new BadRequestError('Unknown relation type ' . $relationType . '.'); + } + if (!$this->permissionsService->canReadRelationTarget($relationType, $targetId, $userId)) { + throw new PermissionError('No permission to use ' . $relationType . ' ' . $targetId . ' as relation target.'); + } + + if ($relationType === Column::RELATION_TYPE_VIEW) { + $view = $this->viewService->find($targetId, true, $userId); + if (!in_array($labelColumnId, $view->getColumnIds(), true)) { + throw new BadRequestError('Relation label column does not belong to the relation target.'); + } + return; + } + + try { + $labelColumn = $this->mapper->find($labelColumnId); + } catch (DoesNotExistException|MultipleObjectsReturnedException|\OCP\DB\Exception) { + throw new BadRequestError('Relation label column does not belong to the relation target.'); + } + if ($labelColumn->getTableId() !== $targetId) { + throw new BadRequestError('Relation label column does not belong to the relation target.'); + } + } + private function normalizeTitle(?string $title, bool $required): ?string { if ($title === null) { if ($required) { @@ -779,7 +871,7 @@ public function importColumn(Table $table, array $column): int { $item->setUsergroupSelectGroups($column['usergroupSelectGroups']); $item->setUsergroupSelectTeams($column['usergroupSelectTeams']); $item->setShowUserStatus($column['showUserStatus']); - $item->setCustomSettings(json_encode($column['customSettings'])); + $item->setCustomSettings($this->setRelationTargetAuthor(json_encode($column['customSettings']) ?: null, $this->userId)); try { $newColumn = $this->mapper->insert($item); diff --git a/lib/Service/PermissionsService.php b/lib/Service/PermissionsService.php index dc68798e23..68803539c4 100644 --- a/lib/Service/PermissionsService.php +++ b/lib/Service/PermissionsService.php @@ -8,6 +8,7 @@ namespace OCA\Tables\Service; use OCA\Tables\AppInfo\Application; +use OCA\Tables\Db\Column; use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\Share; @@ -271,6 +272,14 @@ public function canReadColumnsByTableId(int $tableId, ?string $userId = null): b return $canCreateRows || $canReadRows; } + public function canReadRelationTarget(string $relationType, int $targetId, ?string $userId = null): bool { + return match ($relationType) { + Column::RELATION_TYPE_TABLE => $this->canReadRowsByElementId($targetId, 'table', $userId), + Column::RELATION_TYPE_VIEW => $this->canReadRowsByElementId($targetId, 'view', $userId), + default => false, + }; + } + /** * @param Table $table * @param string|null $userId diff --git a/lib/Service/RelationService.php b/lib/Service/RelationService.php index 6d0ca9fade..6b5754f3df 100644 --- a/lib/Service/RelationService.php +++ b/lib/Service/RelationService.php @@ -25,6 +25,7 @@ public function __construct( private readonly ViewMapper $viewMapper, private readonly Row2Mapper $row2Mapper, private readonly ColumnService $columnService, + private readonly PermissionsService $permissionsService, private readonly ?string $userId, ) { } @@ -103,7 +104,7 @@ private function groupColumnsByTarget(array $columns): array { continue; } - $target = sprintf('%s_%s_%s', $settings['relationType'], $settings['targetId'], $settings['labelColumn']); + $target = sprintf('%s_%s_%s_%s', $settings['relationType'], $settings['targetId'], $settings['labelColumn'], $settings[Column::RELATION_TARGET_SET_BY] ?? ''); $groups[$target] ??= []; $groups[$target][] = $column; } @@ -127,7 +128,7 @@ public function getRelationData(Column $column): array { return []; } - $target = sprintf('%s_%s_%s', $settings['relationType'], $settings['targetId'], $settings['labelColumn']); + $target = sprintf('%s_%s_%s_%s', $settings['relationType'], $settings['targetId'], $settings['labelColumn'], $settings[Column::RELATION_TARGET_SET_BY] ?? ''); return $this->getRelationDataForTarget($target, $column); } @@ -153,13 +154,25 @@ private function getRelationDataForTarget(string $target, Column $column): array return []; } - $isView = $settings[Column::RELATION_TYPE] === 'view'; + $relationType = (string)$settings[Column::RELATION_TYPE]; + $isView = $relationType === Column::RELATION_TYPE_VIEW; $targetId = $settings[Column::RELATION_TARGET_ID] ?? null; + $targetSetBy = $settings[Column::RELATION_TARGET_SET_BY] ?? ''; + $authorizeAs = $targetSetBy !== '' ? (string)$targetSetBy : $this->userId; + if (!$this->permissionsService->canReadRelationTarget($relationType, (int)$targetId, $authorizeAs)) { + $this->cacheRelationData[$cacheKey] = []; + return []; + } + try { $targetColumn = $this->columnMapper->find($settings[Column::RELATION_LABEL_COLUMN]); if ($isView) { $view = $this->viewMapper->find($targetId); + if (!in_array($targetColumn->getId(), $view->getColumnIds(), true)) { + $this->cacheRelationData[$cacheKey] = []; + return []; + } $rows = $this->row2Mapper->findAll( [$targetColumn->getId()], $view->getTableId(), diff --git a/tests/integration/features/APIv1.feature b/tests/integration/features/APIv1.feature index 925ce4c1ae..1abdc59828 100644 --- a/tests/integration/features/APIv1.feature +++ b/tests/integration/features/APIv1.feature @@ -764,6 +764,7 @@ Feature: APIv1 And user "participant1" sets filter to view "available-fruits" | column | operator | value | | available | is-equal | true | + And user "participant1" sets columnSettings "name,available" to view "available-fruits" Given table "Smoothies" with emoji "🥤" exists for user "participant1" as "smoothies" Then relation column "ingredient" exists on table "smoothies" pointing to view "available-fruits" using label column "name" When user "participant1" fetches relations for table "smoothies" @@ -804,6 +805,51 @@ Feature: APIv1 When user "participant2" fetches relations for table "secret-orders" Then the reported status is "404" + @api1 @relation @table-sharing + Scenario: Unauthorized user cannot create a relation column to a table + Given table "Secret Products" with emoji "🔒" exists for user "participant1" as "secret-products" + Then column "name" exists with following properties + | type | text | + | subtype | line | + | mandatory | 0 | + Given table "My Orders" with emoji "🛒" exists for user "participant2" as "my-orders" + When user "participant2" attempts to create relation column "product" on table "my-orders" pointing to table "secret-products" using label column "name" + Then the reported status is "403" + + @api1 @relation @table-sharing + Scenario: Shared relation column shows labels without access to the target table + Given table "Products" with emoji "📦" exists for user "participant1" as "products" + Then column "name" exists with following properties + | type | text | + | subtype | line | + | mandatory | 0 | + Then row exists with following values + | name | Apple | + Given table "Orders" with emoji "🛒" exists for user "participant1" as "orders" + Then relation column "product" exists on table "orders" pointing to table "products" using label column "name" + Then user "participant1" shares table with user "participant2" + When user "participant2" fetches relations for table "orders" + Then the reported status is "200" + Then the relations response contains 1 entries for column "product" + Then the relations response for column "product" has an entry with label "Apple" + + @api1 @relation + Scenario: Relation column to a view cannot use a column hidden in the view + Given table "Fruits" with emoji "🍎" exists for user "participant1" as "fruits" + Then column "name" exists with following properties + | type | text | + | subtype | line | + | mandatory | 0 | + Then column "price" exists with following properties + | type | text | + | subtype | line | + | mandatory | 0 | + When user "participant1" create view "Names only" with emoji "🏷️" for "fruits" as "names-only" + And user "participant1" sets columnSettings "name" to view "names-only" + Given table "Smoothies" with emoji "🥤" exists for user "participant1" as "smoothies" + When user "participant1" attempts to create relation column "ingredient" on table "smoothies" pointing to view "names-only" using label column "price" + Then the reported status is "400" + @api1 @columns @rows Scenario: Column technicalName is returned and dataByAlias maps alias to row values Given table "Alias test" with emoji "🔖" exists for user "participant1" as "base1" diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 37a5fdea98..3783a869ed 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -2595,96 +2595,65 @@ public function theReportedStatusIs(int $statusCode): void { * @Then relation column :title exists on table :tableName pointing to table :targetTableName using label column :labelColumnName */ public function createRelationColumnToTable(string $title, string $tableName, string $targetTableName, string $labelColumnName): void { - $targetId = $this->tableIds[$targetTableName]; - $labelColumnId = $this->collectionManager->getByAlias('column', $labelColumnName)['id']; - - $props = [ - 'title' => $title, - 'type' => 'relation', - 'mandatory' => 0, - 'customSettings' => [ - 'relationType' => 'table', - 'targetId' => $targetId, - 'labelColumn' => $labelColumnId, - ], - ]; - - $this->sendRequest( - 'POST', - '/apps/tables/api/1/tables/' . $this->tableIds[$tableName] . '/columns', - $props - ); - - $newColumn = $this->getDataFromResponse($this->response); - $this->columnId = $newColumn['id']; - - Assert::assertEquals(200, $this->response->getStatusCode()); - Assert::assertEquals($title, $newColumn['title']); - Assert::assertEquals('relation', $newColumn['type']); - - $this->collectionManager->register($newColumn, 'column', $newColumn['id'], $title); + $this->sendCreateRelationColumnRequest($title, $tableName, 'table', $this->tableIds[$targetTableName], $labelColumnName); + $this->assertRelationColumnCreated($title); } /** * @Then relation column :title exists on table :tableName pointing to table :targetTableName using label column :labelColumnName added to view :viewName */ public function createRelationColumnToTableAddedToView(string $title, string $tableName, string $targetTableName, string $labelColumnName, string $viewName): void { - $targetId = $this->tableIds[$targetTableName]; - $labelColumnId = $this->collectionManager->getByAlias('column', $labelColumnName)['id']; - $viewId = $this->viewIds[$viewName]; - - $props = [ - 'title' => $title, - 'type' => 'relation', - 'mandatory' => 0, - 'selectedViewIds' => [$viewId], - 'customSettings' => [ - 'relationType' => 'table', - 'targetId' => $targetId, - 'labelColumn' => $labelColumnId, - ], - ]; - - $this->sendRequest( - 'POST', - '/apps/tables/api/1/tables/' . $this->tableIds[$tableName] . '/columns', - $props - ); - - $newColumn = $this->getDataFromResponse($this->response); - $this->columnId = $newColumn['id']; - - Assert::assertEquals(200, $this->response->getStatusCode()); - Assert::assertEquals($title, $newColumn['title']); - Assert::assertEquals('relation', $newColumn['type']); - - $this->collectionManager->register($newColumn, 'column', $newColumn['id'], $title); + $this->sendCreateRelationColumnRequest($title, $tableName, 'table', $this->tableIds[$targetTableName], $labelColumnName, [$this->viewIds[$viewName]]); + $this->assertRelationColumnCreated($title); } /** * @Then relation column :title exists on table :tableName pointing to view :targetViewName using label column :labelColumnName */ public function createRelationColumnToView(string $title, string $tableName, string $targetViewName, string $labelColumnName): void { - $targetId = $this->viewIds[$targetViewName]; - $labelColumnId = $this->collectionManager->getByAlias('column', $labelColumnName)['id']; + $this->sendCreateRelationColumnRequest($title, $tableName, 'view', $this->viewIds[$targetViewName], $labelColumnName); + $this->assertRelationColumnCreated($title); + } + + /** + * @When user :user attempts to create relation column :title on table :tableName pointing to table :targetTableName using label column :labelColumnName + */ + public function userAttemptsToCreateRelationColumnToTable(string $user, string $title, string $tableName, string $targetTableName, string $labelColumnName): void { + $this->setCurrentUser($user); + $this->sendCreateRelationColumnRequest($title, $tableName, 'table', $this->tableIds[$targetTableName], $labelColumnName); + } + /** + * @When user :user attempts to create relation column :title on table :tableName pointing to view :targetViewName using label column :labelColumnName + */ + public function userAttemptsToCreateRelationColumnToView(string $user, string $title, string $tableName, string $targetViewName, string $labelColumnName): void { + $this->setCurrentUser($user); + $this->sendCreateRelationColumnRequest($title, $tableName, 'view', $this->viewIds[$targetViewName], $labelColumnName); + } + + private function sendCreateRelationColumnRequest(string $title, string $tableName, string $relationType, int $targetId, string $labelColumnName, array $selectedViewIds = []): void { $props = [ 'title' => $title, 'type' => 'relation', 'mandatory' => 0, 'customSettings' => [ - 'relationType' => 'view', + 'relationType' => $relationType, 'targetId' => $targetId, - 'labelColumn' => $labelColumnId, + 'labelColumn' => $this->collectionManager->getByAlias('column', $labelColumnName)['id'], ], ]; + if ($selectedViewIds !== []) { + $props['selectedViewIds'] = $selectedViewIds; + } $this->sendRequest( 'POST', '/apps/tables/api/1/tables/' . $this->tableIds[$tableName] . '/columns', $props ); + } + private function assertRelationColumnCreated(string $title): void { $newColumn = $this->getDataFromResponse($this->response); $this->columnId = $newColumn['id']; diff --git a/tests/unit/Service/PermissionsServiceTest.php b/tests/unit/Service/PermissionsServiceTest.php index 7d1dfe8468..d164e2b0fc 100644 --- a/tests/unit/Service/PermissionsServiceTest.php +++ b/tests/unit/Service/PermissionsServiceTest.php @@ -16,6 +16,7 @@ use OCA\Tables\Errors\InternalError; use OCA\Tables\Helper\CircleHelper; use OCA\Tables\Helper\UserHelper; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -71,4 +72,49 @@ public function testPreCheckUserIdNoUserAllowed() { self::assertEquals($userId, $permissionsService->preCheckUserId($userId)); } + + /** + * @dataProvider relationTargetTypeProvider + */ + public function testCanReadRelationTargetChecksRowReadPermission(string $relationType) { + $permissionsService = $this->getPermissionServiceWithMockedRowReadCheck(); + $permissionsService->expects(self::once()) + ->method('canReadRowsByElementId') + ->with(5, $relationType, 'user1') + ->willReturn(true); + + self::assertTrue($permissionsService->canReadRelationTarget($relationType, 5, 'user1')); + } + + public static function relationTargetTypeProvider(): array { + return [ + 'table' => ['table'], + 'view' => ['view'], + ]; + } + + public function testCanReadRelationTargetRejectsUnknownType() { + $permissionsService = $this->getPermissionServiceWithMockedRowReadCheck(); + $permissionsService->expects(self::never()) + ->method('canReadRowsByElementId'); + + self::assertFalse($permissionsService->canReadRelationTarget('context', 5, 'user1')); + } + + protected function getPermissionServiceWithMockedRowReadCheck(): PermissionsService&MockObject { + return $this->getMockBuilder(PermissionsService::class) + ->setConstructorArgs([ + $this->createMock(LoggerInterface::class), + 'user1', + $this->createMock(TableMapper::class), + $this->createMock(ViewMapper::class), + $this->createMock(ShareMapper::class), + $this->createMock(ContextMapper::class), + $this->createMock(UserHelper::class), + $this->createMock(CircleHelper::class), + false, + ]) + ->onlyMethods(['canReadRowsByElementId']) + ->getMock(); + } }