Merge pull request #61070 from nextcloud/backport/60453/stable33

[stable33] fix(dav): finalize upload metadata before post-write hooks

apps/dav/lib/Connector/Sabre/File.php

@@ -31,6 +31,7 @@ use OCP\Files\InvalidPathException;
 use OCP\Files\LockNotAcquiredException;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
+use OCP\Files\Storage\IStorage;
 use OCP\Files\Storage\IWriteStreamStorage;
 use OCP\Files\StorageNotAvailableException;
 use OCP\IConfig;
@@ -327,51 +328,7 @@ class File extends Node implements IFile {
 				}
 			}
 
-			// since we skipped the view we need to scan and emit the hooks ourselves
-			$storage->getUpdater()->update($internalPath);
-
-			try {
-				$this->changeLock(ILockingProvider::LOCK_SHARED);
-			} catch (LockedException $e) {
-				throw new FileLocked($e->getMessage(), $e->getCode(), $e);
-			}
-
-			// allow sync clients to send the mtime along in a header
-			$mtimeHeader = $this->request->getHeader('x-oc-mtime');
-			if ($mtimeHeader !== '') {
-				$mtime = $this->sanitizeMtime($mtimeHeader);
-				if ($this->fileView->touch($this->path, $mtime)) {
-					$this->header('X-OC-MTime: accepted');
-				}
-			}
-
-			$fileInfoUpdate = [
-				'upload_time' => time()
-			];
-
-			// allow sync clients to send the creation time along in a header
-			$ctimeHeader = $this->request->getHeader('x-oc-ctime');
-			if ($ctimeHeader) {
-				$ctime = $this->sanitizeMtime($ctimeHeader);
-				$fileInfoUpdate['creation_time'] = $ctime;
-				$this->header('X-OC-CTime: accepted');
-			}
-
-			$this->fileView->putFileInfo($this->path, $fileInfoUpdate);
-
-			if ($view) {
-				$this->emitPostHooks($exists);
-			}
-
-			$this->refreshInfo();
-
-			$checksumHeader = $this->request->getHeader('oc-checksum');
-			if ($checksumHeader) {
-				$checksum = trim($checksumHeader);
-				$this->setChecksum($checksum);
-			} elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') {
-				$this->setChecksum('');
-			}
+			$this->finalizeUpload($storage, $internalPath, $exists, $view);
 		} catch (StorageNotAvailableException $e) {
 			throw new ServiceUnavailable($this->l10n->t('Failed to check file size: %1$s', [$e->getMessage()]), 0, $e);
 		}
@@ -379,6 +336,58 @@ class File extends Node implements IFile {
 		return '"' . $this->info->getEtag() . '"';
 	}
 
+	private function finalizeUpload(IStorage $storage, string $internalPath, bool $exists, ?View $view): void {
+		// Since we skipped the view for the final publish step, finalize the file
+		// state explicitly here: update cache/bookkeeping, persist metadata, then
+		// downgrade to a shared lock before emitting post-write hooks so listeners
+		// can still access the file.
+		$storage->getUpdater()->update($internalPath);
+
+		$fileInfoUpdate = [
+			'upload_time' => time(),
+		];
+
+		// allow sync clients to send the mtime along in a header
+		$mtimeHeader = $this->request->getHeader('x-oc-mtime');
+		if ($mtimeHeader !== '') {
+			$mtime = $this->sanitizeMtime($mtimeHeader);
+			if ($this->fileView->touch($this->path, $mtime)) {
+				$this->header('X-OC-MTime: accepted');
+			}
+		}
+
+		// allow sync clients to send the creation time along in a header
+		$ctimeHeader = $this->request->getHeader('x-oc-ctime');
+		if ($ctimeHeader !== '') {
+			$ctime = $this->sanitizeMtime($ctimeHeader);
+			$fileInfoUpdate['creation_time'] = $ctime;
+			$this->header('X-OC-CTime: accepted');
+		}
+
+		// Persist checksum before post hooks so observers see fully finalized metadata.
+		$checksumHeader = $this->request->getHeader('oc-checksum');
+		if ($checksumHeader) {
+			$fileInfoUpdate['checksum'] = trim($checksumHeader);
+		} elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') {
+			$fileInfoUpdate['checksum'] = '';
+		}
+
+		$this->fileView->putFileInfo($this->path, $fileInfoUpdate);
+		$this->refreshInfo();
+
+		// Downgrade to shared lock before post hooks so legacy hook consumers can
+		// still access the file during post_write.
+		try {
+			$this->changeLock(ILockingProvider::LOCK_SHARED);
+		} catch (LockedException $e) {
+			throw new FileLocked($e->getMessage(), $e->getCode(), $e);
+		}
+
+		if ($view) {
+			$this->emitPostHooks($exists);
+		}
+	}
+
 	private function getPartFileBasePath($path) {
 		$partFileInStorage = Server::get(IConfig::class)->getSystemValue('part_file_in_storage', true);
 		if ($partFileInStorage) {

apps/dav/tests/unit/Connector/Sabre/FileTest.php

@@ -230,7 +230,8 @@ class FileTest extends TestCase {
 			null,
 			[
 				'permissions' => Constants::PERMISSION_ALL,
-				'type' => FileInfo::TYPE_FOLDER,
+				'type' => FileInfo::TYPE_FILE,
+				'checksum' => '',
 			],
 			null
 		);
@@ -776,7 +777,13 @@ class FileTest extends TestCase {
 	}
 
 	/**
-	 * Test whether locks are set before and after the operation
+	 * Test that PUT keeps hook-time lock semantics compatible:
+	 * - pre-write hooks run while the file is shared-locked
+	 * - post-write hooks also run while the file is shared-locked
+	 *
+	 * Post-write hooks are expected to observe a fully finalized file state,
+	 * but should still be able to access the file without exclusive-lock
+	 * contention.
 	 */
 	public function testPutLocking(): void {
 		$view = new View('/' . $this->user . '/files/');
@@ -788,7 +795,8 @@ class FileTest extends TestCase {
 			null,
 			[
 				'permissions' => Constants::PERMISSION_ALL,
-				'type' => FileInfo::TYPE_FOLDER,
+				'type' => FileInfo::TYPE_FILE,
+				'checksum' => '',
 			],
 			null
 		);
@@ -810,8 +818,8 @@ class FileTest extends TestCase {
 			->addMethods(['writeCallback', 'postWriteCallback'])
 			->getMock();
 
-		// both pre and post hooks might need access to the file,
-		// so only shared lock is acceptable
+		// Pre-write hooks should run under a shared lock so observers can safely
+		// inspect the target while the write is in progress.
 		$eventHandler->expects($this->once())
 			->method('writeCallback')
 			->willReturnCallback(
@@ -820,6 +828,10 @@ class FileTest extends TestCase {
 					$wasLockedPre = $wasLockedPre && !$this->isFileLocked($view, $path, ILockingProvider::LOCK_EXCLUSIVE);
 				}
 			);
+
+		// Post-write hooks should also run under a shared lock. They are expected to
+		// see fully finalized metadata/state, but still be able to access the file
+		// during the callback.
 		$eventHandler->expects($this->once())
 			->method('postWriteCallback')
 			->willReturnCallback(
@@ -850,8 +862,8 @@ class FileTest extends TestCase {
 		// afterMethod unlocks
 		$view->unlockFile($path, ILockingProvider::LOCK_SHARED);
 
-		$this->assertTrue($wasLockedPre, 'File was locked during pre-hooks');
-		$this->assertTrue($wasLockedPost, 'File was locked during post-hooks');
+		$this->assertTrue($wasLockedPre, 'File was shared-locked during pre-hooks');
+		$this->assertTrue($wasLockedPost, 'File was shared-locked during post-hooks');
 
 		$this->assertFalse(
 			$this->isFileLocked($view, $path, ILockingProvider::LOCK_SHARED),
@@ -1015,7 +1027,8 @@ class FileTest extends TestCase {
 			null,
 			[
 				'permissions' => Constants::PERMISSION_ALL,
-				'type' => FileInfo::TYPE_FOLDER,
+				'type' => FileInfo::TYPE_FILE,
+				'checksum' => '',
 			],
 			null
 		);

lib/private/Files/FileInfo.php

@@ -368,7 +368,7 @@ class FileInfo implements \OCP\Files\FileInfo, \ArrayAccess {
 	 * @inheritdoc
 	 */
 	public function getChecksum() {
-		return $this->data['checksum'];
+		return $this->data['checksum'] ?? '';
 	}
 
 	public function getExtension(): string {