diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 7f5e049c32912..40afe0f2f25b7 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -31,6 +31,7 @@ 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; @@ -334,56 +335,64 @@ public function put($data) { } } - // 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'); - } - } + $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); + } - $fileInfoUpdate = [ - 'upload_time' => time() - ]; + return '"' . $this->info->getEtag() . '"'; + } - // 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'); + 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'); } + } - $this->fileView->putFileInfo($this->path, $fileInfoUpdate); + // 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'); + } - if ($view) { - $this->emitPostHooks($exists); - } + // 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->refreshInfo(); + $this->fileView->putFileInfo($this->path, $fileInfoUpdate); + $this->refreshInfo(); - $checksumHeader = $this->request->getHeader('oc-checksum'); - if ($checksumHeader) { - $checksum = trim($checksumHeader); - $this->setChecksum($checksum); - } elseif ($this->getChecksum() !== null && $this->getChecksum() !== '') { - $this->setChecksum(''); - } - } catch (StorageNotAvailableException $e) { - throw new ServiceUnavailable($this->l10n->t('Failed to check file size: %1$s', [$e->getMessage()]), 0, $e); + // 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); } - return '"' . $this->info->getEtag() . '"'; + if ($view) { + $this->emitPostHooks($exists); + } } private function getPartFileBasePath($path) { diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 25b09db1c54b9..1431c6be3d311 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -238,7 +238,8 @@ private function doPut(string $path, ?string $viewRoot = null, ?Request $request null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -800,7 +801,13 @@ protected function assertHookCall($callData, $signal, $hookPath) { } /** - * 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/'); @@ -812,7 +819,8 @@ public function testPutLocking(): void { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); @@ -832,8 +840,8 @@ public function testPutLocking(): void { $wasLockedPost = false; $eventHandler = $this->createMock(EventHandlerMock::class); - // 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( @@ -842,6 +850,10 @@ function () use ($view, $path, &$wasLockedPre): void { $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( @@ -872,8 +884,8 @@ function () use ($view, $path, &$wasLockedPost): void { // 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), @@ -1037,7 +1049,8 @@ public function testPutLockExpired(): void { null, [ 'permissions' => Constants::PERMISSION_ALL, - 'type' => FileInfo::TYPE_FOLDER, + 'type' => FileInfo::TYPE_FILE, + 'checksum' => '', ], null ); diff --git a/lib/private/Files/FileInfo.php b/lib/private/Files/FileInfo.php index 65280b0f31580..d7b8545c5ab50 100644 --- a/lib/private/Files/FileInfo.php +++ b/lib/private/Files/FileInfo.php @@ -382,7 +382,7 @@ public function addSubEntry($data, $entryPath) { */ #[\Override] public function getChecksum() { - return $this->data['checksum']; + return $this->data['checksum'] ?? ''; } #[\Override]