diff --git a/lib/Controller/LockController.php b/lib/Controller/LockController.php index 7967fb7b..585d4814 100644 --- a/lib/Controller/LockController.php +++ b/lib/Controller/LockController.php @@ -125,7 +125,8 @@ private function buildOCSResponse($format, DataResponse $data) { $message = null; if ($data->getStatus() === Http::STATUS_LOCKED) { /** @var FileLock $lock */ - $lock = $data->getData(); + $lock = new FileLock(); + $lock->import($data->getData()); $this->lockService->injectMetadata($lock); $message = $this->l10n->t('File is currently locked by %s', [$lock->getDisplayName()]); } diff --git a/lib/Service/LockService.php b/lib/Service/LockService.php index 4de1a204..968f1640 100644 --- a/lib/Service/LockService.php +++ b/lib/Service/LockService.php @@ -16,6 +16,7 @@ use OCA\FilesLock\Model\FileLock; use OCA\FilesLock\Tools\Traits\TStringTools; use OCP\App\IAppManager; +use OCP\Constants; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\InvalidPathException; use OCP\Files\Lock\ILock; @@ -142,6 +143,8 @@ public function getLockForNodeIds(array $nodeIds): array { } public function lock(LockContext $lockScope): FileLock { + $this->canLock($lockScope); + try { $known = $this->getLockFromFileId($lockScope->getNode()->getId()); @@ -205,6 +208,14 @@ public function enableUserOverride(): void { $this->allowUserOverride = true; } + public function canLock(LockContext $request, ?FileLock $current = null): void { + if (($request->getNode()->getPermissions() & Constants::PERMISSION_UPDATE) === 0) { + throw new UnauthorizedUnlockException( + $this->l10n->t('File can only be locked with update permissions.') + ); + } + } + public function canUnlock(LockContext $request, FileLock $current): void { $isSameUser = $current->getOwner() === $this->userSession->getUser()?->getUID(); $isSameToken = $request->getOwner() === $current->getToken(); diff --git a/playwright/e2e/sharing.spec.ts b/playwright/e2e/sharing.spec.ts new file mode 100644 index 00000000..dfe438e5 --- /dev/null +++ b/playwright/e2e/sharing.spec.ts @@ -0,0 +1,49 @@ +/** + * SPDX-FileCopyrightText: 2024 Ferdinand Thiessen + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { expect } from '@playwright/test' +import { test } from '../support/fixtures/sharing-user' + +test('Share a file read only that cannot be locked by the recipient', async ({ pageOwner, pageRecipient, owner, recipient, request }) => { + // Create a test file as owner + const filename = 'test-share.txt' + const response = await request.put(`/remote.php/dav/files/${owner.userId}/${filename}`, { + headers: { + Authorization: `Basic ${Buffer.from(`${owner.userId}:${owner.password}`).toString('base64')}`, + }, + data: 'Test file content', + }) + expect(response.ok()).toBeTruthy() + + // Share the file with recipient + const shareResponse = await request.post('/ocs/v2.php/apps/files_sharing/api/v1/shares', { + headers: { + 'OCS-APIRequest': 'true', + 'Content-Type': 'application/json', + Authorization: `Basic ${Buffer.from(`${owner.userId}:${owner.password}`).toString('base64')}`, + }, + data: { + path: `/${filename}`, + shareType: '0', // User share + shareWith: recipient.userId, + permissions: '1', + }, + }) + expect(shareResponse.ok()).toBeTruthy() + + // Check recipient cannot lock + await pageRecipient.goto('/apps/files') + await pageRecipient.waitForURL(/apps\/files/) + const rowRecipient = await pageRecipient.getByRole('row', { name: filename }) + await rowRecipient.getByRole('button', { name: 'Actions' }).click() + await expect(pageRecipient.getByRole('menuitem', { name: 'Lock file' })).not.toBeVisible() + + // Check owner can still lock + await pageOwner.goto('/apps/files') + await pageOwner.waitForURL(/apps\/files/) + const rowOwner = await pageOwner.getByRole('row', { name: filename }) + await rowOwner.getByRole('button', { name: 'Actions' }).click() + await expect(pageOwner.getByRole('menuitem', { name: 'Lock file' })).toBeVisible() +}) diff --git a/playwright/support/fixtures/sharing-user.ts b/playwright/support/fixtures/sharing-user.ts new file mode 100644 index 00000000..07d33114 --- /dev/null +++ b/playwright/support/fixtures/sharing-user.ts @@ -0,0 +1,59 @@ +/** + * SPDX-FileCopyrightText: 2024 Ferdinand Thiessen + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { test as base } from '@playwright/test' +import { createRandomUser, login } from '@nextcloud/e2e-test-server/playwright' + +// The User type from e2e-test-server +interface User { + userId: string + password: string + language: string +} + +interface SharingUserFixture { + owner: User + recipient: User + pageOwner: Page + pageRecipient: Page +} + +/** + * This test fixture ensures two new random users are created: + * - owner: The user who will be sharing files/folders + * - recipient: The user who will receive the shares + */ +export const test = base.extend({ + owner: async ({ }, use) => { + const user = await createRandomUser() + await use(user) + }, + recipient: async ({ }, use) => { + const user = await createRandomUser() + await use(user) + }, + pageOwner: async ({ browser, baseURL, owner }, use) => { + const page = await browser.newPage({ + storageState: undefined, + baseURL, + }) + + await login(page.request, owner) + + await use(page) + await page.close() + }, + pageRecipient: async ({ browser, baseURL, recipient }, use) => { + const page = await browser.newPage({ + storageState: undefined, + baseURL, + }) + + await login(page.request, recipient) + + await use(page) + await page.close() + }, +}) diff --git a/src/helper.ts b/src/helper.ts index b76ed6a6..383e29f4 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { type Node } from '@nextcloud/files' +import { Permission, type Node } from '@nextcloud/files' import { generateUrl } from '@nextcloud/router' import { type LockState, LockType } from './types' import { translate as t } from '@nextcloud/l10n' @@ -23,7 +23,7 @@ export const getLockStateFromAttributes = (node: Node): LockState => { export const canLock = (node: Node): boolean => { const state = getLockStateFromAttributes(node) - if (!state.isLocked) { + if (!state.isLocked && isUpdatable(node)) { return true } @@ -37,6 +37,10 @@ export const canUnlock = (node: Node): boolean => { return false } + if (!isUpdatable(node)) { + return false + } + if (state.lockOwnerType === LockType.User && state.lockOwner === getCurrentUser()?.uid) { return true } @@ -76,3 +80,7 @@ export const getInfoLabel = (node: Node): string => { return '' } + +export const isUpdatable = (node: Node): boolean => { + return (node.permissions & Permission.UPDATE) !== 0 && (node.attributes['share-permissions'] & Permission.UPDATE) !== 0 +} diff --git a/src/main.ts b/src/main.ts index 61dd3c55..1bf53976 100644 --- a/src/main.ts +++ b/src/main.ts @@ -7,7 +7,6 @@ import Vue from 'vue' import { FileAction, type Node, - Permission, FileType, registerFileAction, } from '@nextcloud/files' @@ -20,6 +19,7 @@ import { canLock, canUnlock, getInfoLabel, getLockStateFromAttributes, + isUpdatable, } from './helper' import { getCurrentUser } from '@nextcloud/auth' @@ -129,15 +129,15 @@ const menuAction = new FileAction({ enabled(nodes: Node[]) { // Only works on single node - if (nodes.length !== 1) { + const node = nodes.length === 1 ? nodes[0] : null + if (!node) { return false } - const canToggleLock = canLock(nodes[0]) || canUnlock(nodes[0]) - const isLocked = getLockStateFromAttributes(nodes[0]).isLocked - const isUpdatable = (nodes[0].permissions & Permission.UPDATE) !== 0 + const canToggleLock = canLock(node) || canUnlock(node) + const isLocked = getLockStateFromAttributes(node).isLocked - return nodes[0].type === FileType.File && canToggleLock && (isUpdatable || isLocked) + return node.type === FileType.File && canToggleLock && (isUpdatable(node) || isLocked) }, async exec(node: Node) {