Skip to content

feat: enhance relation permissions and accessibility checks #2765

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
enjeck wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: enhance relation permissions and accessibility checks #2765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ class Application extends App implements IBootstrap {
public const NODE_TYPE_TABLE = 0;
public const NODE_TYPE_VIEW = 1;

public const NODE_TYPE_NAME_TABLE = 'table';
public const NODE_TYPE_NAME_VIEW = 'view';
public const NODE_TYPE_NAME_CONTEXT = 'context';

public const OWNER_TYPE_USER = 0;

public const NAV_ENTRY_MODE_HIDDEN = 0;
Expand Down
42 changes: 42 additions & 0 deletions lib/Service/ColumnService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

use DateTime;
use Exception;
use OCA\Tables\AppInfo\Application;
use OCA\Tables\Constants\ColumnType;
use OCA\Tables\Db\Column;
use OCA\Tables\Db\ColumnMapper;
Expand Down Expand Up @@ -304,6 +305,9 @@ public function create(
}

$this->validateCustomSettings($columnDto->getCustomSettings());
if ($columnDto->getType() === Column::TYPE_RELATION) {
$this->validateRelationTargetAccess($columnDto->getCustomSettings(), $userId);
}

$item = Column::fromDto($columnDto);
$item->setTitle($newTitle);
Expand Down Expand Up @@ -408,6 +412,9 @@ public function update(
$item->setUsergroupSelectTeams($columnDto->getUsergroupSelectTeams());
$item->setShowUserStatus($columnDto->getShowUserStatus());
$this->validateCustomSettings($columnDto->getCustomSettings());
if ($columnDto->getType() === Column::TYPE_RELATION || $item->getType() === Column::TYPE_RELATION) {
$this->validateRelationTargetAccess($columnDto->getCustomSettings(), $userId);
}
$item->setCustomSettings($columnDto->getCustomSettings());

$this->updateMetadata($item, $userId);
Expand Down Expand Up @@ -450,6 +457,41 @@ private function validateCustomSettings(?string $customSettings): void {
}
}

/**
* Ensure the user configuring a relation column can actually read the target
*
* @param string|null $customSettings
* @throws BadRequestError
*/
private function validateRelationTargetAccess(?string $customSettings, ?string $userId): void {
if ($customSettings === null) {
return;
}
$settings = json_decode($customSettings, true);
if (!is_array($settings)) {
return;
}

$relationType = $settings[Column::RELATION_TYPE] ?? null;
$targetId = isset($settings[Column::RELATION_TARGET_ID]) ? (int)$settings[Column::RELATION_TARGET_ID] : null;
if (empty($relationType) || empty($targetId)) {
return;
}

if ($relationType === Application::NODE_TYPE_NAME_VIEW) {
$canRead = $this->permissionsService->canReadColumnsByViewId($targetId, $userId);
} elseif ($relationType === Application::NODE_TYPE_NAME_TABLE) {
$canRead = $this->permissionsService->canReadColumnsByTableId($targetId, $userId);
} else {
$canRead = false;
}

if (!$canRead) {
$translatedMessage = $this->l->t('You can only link to a table or view that you have access to.');
throw new BadRequestError($translatedMessage, 0, null, $translatedMessage);
}
}

private function normalizeTitle(?string $title, bool $required): ?string {
if ($title === null) {
if ($required) {
Expand Down
76 changes: 73 additions & 3 deletions lib/Service/RelationService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@

namespace OCA\Tables\Service;

use OCA\Tables\AppInfo\Application;
use OCA\Tables\Db\Column;
use OCA\Tables\Db\ColumnMapper;
use OCA\Tables\Db\Row2Mapper;
use OCA\Tables\Db\TableMapper;
use OCA\Tables\Db\ViewMapper;
use OCA\Tables\Errors\InternalError;
use OCA\Tables\Errors\NotFoundError;
Expand All @@ -20,11 +22,17 @@ class RelationService {
/** @var array<string, array> Cache for relation data */
private array $cacheRelationData = [];

/** @var array<string, bool> Cache for manager accessibility decisions, keyed by host table + target */
private array $cacheManagerAccess = [];

public function __construct(
private ColumnMapper $columnMapper,
private ViewMapper $viewMapper,
private TableMapper $tableMapper,
private Row2Mapper $row2Mapper,
private ColumnService $columnService,
private PermissionsService $permissionsService,
private ShareService $shareService,
private ?string $userId,
) {
}
Expand Down Expand Up @@ -83,9 +91,10 @@ private function getRelationsForColumns(array $relationColumns): array {
foreach ($groupedColumns as $target => $columns) {
$relationData = $this->getRelationDataForTarget($target, $columns[0]);

// Assign the same data to all columns with this target
// Assign the same data to all columns with this target, but only when
// the relation is still backed by a manager of the hosting table.
foreach ($columns as $column) {
$result[$column->getId()] = $relationData;
$result[$column->getId()] = $this->isTargetAccessibleByManager($column) ? $relationData : [];
}
}

Expand Down Expand Up @@ -133,11 +142,72 @@ public function getRelationData(Column $column): array {
return [];
}

if (!$this->isTargetAccessibleByManager($column)) {
return [];
}

$target = sprintf('%s_%s_%s', $settings['relationType'], $settings['targetId'], $settings['labelColumn']);

return $this->getRelationDataForTarget($target, $column);
}

public function isTargetAccessibleByManager(Column $relationColumn): bool {
$settings = $relationColumn->getCustomSettingsArray();
$relationType = $settings[Column::RELATION_TYPE] ?? null;
$targetId = isset($settings[Column::RELATION_TARGET_ID]) ? (int)$settings[Column::RELATION_TARGET_ID] : null;
if (empty($relationType) || empty($targetId)) {
return false;
}

$hostTableId = $relationColumn->getTableId();
$cacheKey = sprintf('%s_%s_%s', $hostTableId, $relationType, $targetId);
if (isset($this->cacheManagerAccess[$cacheKey])) {
return $this->cacheManagerAccess[$cacheKey];
}

$candidateUserIds = [];
try {
$hostTable = $this->tableMapper->find($hostTableId);
if ($hostTable->getOwnership() !== null && $hostTable->getOwnership() !== '') {
$candidateUserIds[] = $hostTable->getOwnership();
}
} catch (DoesNotExistException|\OCP\AppFramework\Db\MultipleObjectsReturnedException|\OCP\DB\Exception $e) {
// host table gone, so nothing to expose
$this->cacheManagerAccess[$cacheKey] = false;
return false;
}

try {
$candidateUserIds = array_unique(array_merge(
$candidateUserIds,
$this->shareService->findManagerUserIds($hostTableId, Application::NODE_TYPE_NAME_TABLE),
));
} catch (InternalError $e) {
// fall back to the owner only
}

$accessible = false;
foreach ($candidateUserIds as $candidateUserId) {
if ($candidateUserId === null || $candidateUserId === '') {
continue;
}
if ($relationType === Application::NODE_TYPE_NAME_VIEW) {
$canRead = $this->permissionsService->canReadColumnsByViewId($targetId, $candidateUserId);
} elseif ($relationType === Application::NODE_TYPE_NAME_TABLE) {
$canRead = $this->permissionsService->canReadColumnsByTableId($targetId, $candidateUserId);
} else {
$canRead = false;
}
if ($canRead) {
$accessible = true;
break;
}
}

$this->cacheManagerAccess[$cacheKey] = $accessible;
return $accessible;
}

/**
* Get relation data for a specific target
*
Expand All @@ -159,7 +229,7 @@ private function getRelationDataForTarget(string $target, Column $column): array
return [];
}

$isView = $settings[Column::RELATION_TYPE] === 'view';
$isView = $settings[Column::RELATION_TYPE] === Application::NODE_TYPE_NAME_VIEW;
$targetId = $settings[Column::RELATION_TARGET_ID] ?? null;

try {
Expand Down
62 changes: 45 additions & 17 deletions lib/Service/ShareService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,29 +794,57 @@ public function transferSharesForContext(int $contextId, string $newOwnerId, str
public function findSharedWithUserIds(int $elementId, string $elementType): array {
try {
$shares = $this->mapper->findAllSharesForNode($elementType, $elementId, '');
$sharedWithUserIds = [];

foreach ($shares as $share) {
if ($share->getReceiverType() === ShareReceiverType::USER) {
$sharedWithUserIds[$share->getReceiver()] = 1;
}
if ($share->getReceiverType() === ShareReceiverType::CIRCLE && $this->circleHelper->isCirclesEnabled()) {
$userIds = $this->circleHelper->getUserIdsInCircle($share->getReceiver());
$sharedWithUserIds += array_fill_keys($userIds, 1);
}
if ($share->getReceiverType() === ShareReceiverType::GROUP) {
$userIds = $this->groupHelper->getUserIdsInGroup($share->getReceiver());
$sharedWithUserIds += array_fill_keys($userIds, 1);
}
}

return array_keys($sharedWithUserIds);
return $this->resolveShareReceiversToUserIds($shares);
} catch (Exception $e) {
$this->logger->error('Could not find shared with users: ' . $e->getMessage(), ['exception' => $e]);
throw new InternalError('Could not find shared with users');
}
}

/**
* Returns the IDs of all users that hold the manage permission on the given node
*
* @param int $elementId
* @param string $elementType
* @return string[]
* @throws InternalError
*/
public function findManagerUserIds(int $elementId, string $elementType): array {
try {
$shares = $this->mapper->findAllSharesForNode($elementType, $elementId, '');
return $this->resolveShareReceiversToUserIds($shares, true);
} catch (Exception $e) {
$this->logger->error('Could not find managing users: ' . $e->getMessage(), ['exception' => $e]);
throw new InternalError('Could not find managing users');
}
}

/**
* Resolves a list of shares into the set of user IDs they grant access to
*
* @param Share[] $shares
* @param bool $requireManage when true, only shares carrying the manage permission are considered
* @return string[]
*/
private function resolveShareReceiversToUserIds(array $shares, bool $requireManage = false): array {
$userIds = [];

foreach ($shares as $share) {
if ($requireManage && !$share->getPermissionManage()) {
continue;
}
if ($share->getReceiverType() === ShareReceiverType::USER) {
$userIds[$share->getReceiver()] = 1;
} elseif ($share->getReceiverType() === ShareReceiverType::CIRCLE && $this->circleHelper->isCirclesEnabled()) {
$userIds += array_fill_keys($this->circleHelper->getUserIdsInCircle($share->getReceiver()), 1);
} elseif ($share->getReceiverType() === ShareReceiverType::GROUP) {
$userIds += array_fill_keys($this->groupHelper->getUserIdsInGroup($share->getReceiver()), 1);
}
}

return array_keys($userIds);
}

/**
* @param int $nodeId
* @param array $share
Expand Down
43 changes: 33 additions & 10 deletions src/shared/components/ncTable/partials/columnTypePartials/forms/RelationForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,25 +55,32 @@
<IconInformation :size="16" class="info-icon" />
<span>{{ t('tables', 'Only text and number columns can be used as label') }}</span>
</div>

<NcNoteCard v-if="targetInaccessible"
type="warning"
:text="t('tables', 'The linked table or view is no longer accessible to you, so this relation is disabled. Ask its owner to share it with you, or pick a different target.')" />
</div>
</template>

<script>
import { NcSelect } from '@nextcloud/vue'
import { NcSelect, NcNoteCard } from '@nextcloud/vue'
import { translate as t } from '@nextcloud/l10n'
import { mapState } from 'pinia'
import { useTablesStore } from '../../../../../../store/store.js'
import { useDataStore } from '../../../../../../store/data.js'
import NumberColumn from '../../../mixins/columnsTypes/number.js'
import TextLineColumn from '../../../mixins/columnsTypes/textLine.js'
import permissionsMixin from '../../../mixins/permissionsMixin.js'
import IconInformation from 'vue-material-design-icons/InformationOutline.vue'

export default {
name: 'RelationForm',
components: {
IconInformation,
NcSelect,
NcNoteCard,
},
mixins: [permissionsMixin],
props: {
column: {
type: Object,
Expand All @@ -88,6 +95,7 @@ export default {
relationType: this.column.customSettings.relationType ?? 'table',
},
loadingColumns: false,
targetInaccessible: false,
relationTypeOptions: [
{ id: 'table', label: t('tables', 'Table') },
{ id: 'view', label: t('tables', 'View') },
Expand All @@ -96,20 +104,29 @@ export default {
}
},
computed: {
...mapState(useTablesStore, ['tables', 'views']),
...mapState(useTablesStore, ['tables', 'views', 'activeTable', 'activeView']),
hostTableId() {
return this.activeView ? this.activeView.tableId : (this.activeTable?.id ?? null)
},
availableTargets() {
if (this.customSettings.relationType === 'table') {
return this.tables.map(table => ({
id: table.id,
label: `${table.emoji} ${table.title}`,
}))
return this.tables
// exclude the host table (no self-reference) and anything the
// user cannot read
.filter(table => table.id !== this.hostTableId && this.canReadData(table))
.map(table => ({
id: table.id,
label: `${table.emoji} ${table.title}`,
}))
}

if (this.customSettings.relationType === 'view') {
return this.views.map(view => ({
id: view.id,
label: `${view.emoji} ${view.title}`,
}))
return this.views
.filter(view => view.tableId !== this.hostTableId && this.canReadData(view))
.map(view => ({
id: view.id,
label: `${view.emoji} ${view.title}`,
}))
}

return []
Expand All @@ -129,17 +146,23 @@ export default {
}

this.loadingColumns = true
this.targetInaccessible = false
try {
const dataStore = useDataStore()
const columns = await dataStore.getColumnsFromBE({
tableId: this.customSettings.relationType === 'table' ? this.customSettings.targetId : null,
viewId: this.customSettings.relationType === 'view' ? this.customSettings.targetId : null,
showError: false,
})
this.availableLabelColumns = columns
.filter(column =>
column instanceof NumberColumn || column instanceof TextLineColumn,
)
.map(column => ({ id: column.id, label: column.title }))
} catch (e) {
// The target is no longer readable (e.g. it was unshared)
this.availableLabelColumns = []
this.targetInaccessible = true
} finally {
this.loadingColumns = false
}
Expand Down
Loading
Loading