Merge pull request #863 from nextcloud/fix/check-update

Fix lock checks and response error

Author: juliusknorr

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()]);
 		}

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();

playwright/e2e/sharing.spec.ts

@@ -0,0 +1,49 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Ferdinand Thiessen <[email protected]>
+ * 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()
+})

playwright/support/fixtures/sharing-user.ts

@@ -0,0 +1,59 @@
+/**
+ * SPDX-FileCopyrightText: 2024 Ferdinand Thiessen <[email protected]>
+ * 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<SharingUserFixture>({
+	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()
+	},
+})

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

src/main.ts

@@ -6,7 +6,6 @@
 import {
 	FileAction,
 	type Node,
-	Permission,
 	FileType,
 	registerFileAction,
 } from '@nextcloud/files'
@@ -19,6 +18,7 @@ import {
 	canLock, canUnlock,
 	getInfoLabel,
 	getLockStateFromAttributes,
+	isUpdatable,
 } from './helper'
 import { getCurrentUser } from '@nextcloud/auth'
 
@@ -144,15 +144,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) {

tests/psalm-baseline.xml

@@ -59,9 +59,6 @@
       <code><![CDATA[$qb]]></code>
       <code><![CDATA[$qb]]></code>
     </MissingDependency>
-    <UndefinedClass>
-      <code><![CDATA[UniqueConstraintViolationException]]></code>
-    </UndefinedClass>
   </file>
   <file src="lib/Db/LocksRequestBuilder.php">
     <MissingDependency>
@@ -99,13 +96,4 @@
       <code><![CDATA[$arr === null]]></code>
     </TypeDoesNotContainNull>
   </file>
-  <file src="lib/Tools/Traits/TSetup.php">
-    <MismatchingDocblockParamType>
-      <code><![CDATA[string]]></code>
-      <code><![CDATA[string]]></code>
-    </MismatchingDocblockParamType>
-    <MismatchingDocblockReturnType>
-      <code><![CDATA[string]]></code>
-    </MismatchingDocblockReturnType>
-  </file>
 </files>