From e5a26695e1ec402b8e044b4c3d00915f4182acea Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 7 Aug 2026 21:54:08 -0400 Subject: [PATCH 1/4] refactor(files_trashbin): Flatten Trashbin::copyFilesToUser() Flatten logic: 1. If there is not enough space, stop. 2. Copy the item. 3. If the copy did not materialize, stop. 4. Record metadata. 5. If bookkeeping fails, log the error, stop. Clarify cross-user copy method: - existing copyFilesToUser() docblock is vague - does not explain why it exists / how failures are handled Signed-off-by: Josh --- apps/files_trashbin/lib/Trashbin.php | 58 +++++++++++++++++----------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 41301c84f41d6..7e71650694299 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -179,15 +179,22 @@ private static function setUpTrash($user): void { } /** - * copy file to owners trash + * Copy a deleted item to the trash bin of a user who is not its owner. * - * @param string $sourcePath - * @param string $owner - * @param string $targetPath - * @param string $user - * @param int $timestamp + * The owner's trash bin contains the original item. The additional copy is + * needed so that the user who deleted an item from a shared location can see + * it in their own trash bin. + * + * If the user's trash bin has insufficient available space, no copy or + * metadata entry is created. + * + * @param string $sourcePath Path to the item in the owner's files root. + * @param string $owner Owner of the deleted item. + * @param string $targetPath Original path of the item in the deleting user's files root. + * @param string $user User who deleted the item. + * @param int $timestamp Timestamp used to identify the trash item. */ - private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user, $timestamp): void { + private static function copyFilesToUser(string $sourcePath, string $owner, string $targetPath, string $user, int $timestamp): void { self::setUpTrash($owner); $targetFilename = basename($targetPath); @@ -202,22 +209,27 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user, $free = $view->free_space($target); $isUnknownOrUnlimitedFreeSpace = $free < 0; $isEnoughFreeSpaceLeft = $view->filesize($source) < $free; - if ($isUnknownOrUnlimitedFreeSpace || $isEnoughFreeSpaceLeft) { - self::copy_recursive($source, $target, $view); - } - - if ($view->file_exists($target)) { - $query = Server::get(IDBConnection::class)->getQueryBuilder(); - $query->insert('files_trash') - ->setValue('id', $query->createNamedParameter($targetFilename)) - ->setValue('timestamp', $query->createNamedParameter($timestamp)) - ->setValue('location', $query->createNamedParameter($targetLocation)) - ->setValue('user', $query->createNamedParameter($user)) - ->setValue('deleted_by', $query->createNamedParameter($user)); - $result = $query->executeStatement(); - if (!$result) { - Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated for the files owner', ['app' => 'files_trashbin']); - } + + if (!$isUnknownOrUnlimitedFreeSpace && !$isEnoughFreeSpaceLeft) { + return; + } + + self::copy_recursive($source, $target, $view); + + if (!$view->file_exists($target)) { + return; + } + + $query = Server::get(IDBConnection::class)->getQueryBuilder(); + $query->insert('files_trash') + ->setValue('id', $query->createNamedParameter($targetFilename)) + ->setValue('timestamp', $query->createNamedParameter($timestamp)) + ->setValue('location', $query->createNamedParameter($targetLocation)) + ->setValue('user', $query->createNamedParameter($user)) + ->setValue('deleted_by', $query->createNamedParameter($user)); + $result = $query->executeStatement(); + if (!$result) { + Server::get(LoggerInterface::class)->error('trash bin database couldn\'t be updated for the files owner', ['app' => 'files_trashbin']); } } From 841f0b0b3a34c45e6d40dd40be0a2bc52e405320 Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 7 Aug 2026 22:29:46 -0400 Subject: [PATCH 2/4] refactor(files_trashbin): avoid constructing View prematurely in move2Trash Also null check style improvements and make some useful but focused comment improvements: - Explain why the timestamp is incremented - Clarify the database-before-move ordering - Explain the cache update branches - Clarify the failed-original-deletion recovery - Improve the owner/user comments - Update the move2Trash() docblock Signed-off-by: Josh --- apps/files_trashbin/lib/Trashbin.php | 55 +++++++++++++++++++--------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 7e71650694299..42139e56747bd 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -234,12 +234,18 @@ private static function copyFilesToUser(string $sourcePath, string $owner, strin } /** - * move file to the trash bin + * Move a file or directory to the owner's trash bin. * - * @param string $file_path path to the deleted file/directory relative to the files root directory - * @param bool $ownerOnly delete for owner only (if file gets moved out of a shared folder) + * For items owned by another user, a copy is normally added to the + * deleting user's trash bin as well. Set $ownerOnly to skip that copy. * - * @return bool + * @param string $file_path Path to the deleted file or directory relative + * to the files root directory. + * @param bool $ownerOnly Delete for the owner only, for example when an + * item is moved out of a shared folder. + * + * @return bool True if the item was moved successfully or was already + * absent; false if the trash move failed or was rejected. */ public static function move2trash($file_path, $ownerOnly = false) { // get the user for which the filesystem is setup @@ -248,18 +254,17 @@ public static function move2trash($file_path, $ownerOnly = false) { [$owner, $ownerPath] = self::getUidAndFilename($file_path); // if no owner found (ex: ext storage + share link), will use the current user's trashbin then - if (is_null($owner)) { + if ($owner === null)) { $owner = $user; $ownerPath = $file_path; } - $ownerView = new View('/' . $owner); - // file has been deleted in between - if (is_null($ownerPath) || $ownerPath === '') { + if ($ownerPath === null || $ownerPath === '') { return true; } + $ownerView = new View('/' . $owner); $sourceInfo = $ownerView->getFileInfo('/files/' . $ownerPath); if ($sourceInfo === false) { @@ -268,7 +273,7 @@ public static function move2trash($file_path, $ownerOnly = false) { self::setUpTrash($user); if ($owner !== $user) { - // also setup for owner + // The original payload is stored in the owner's trash bin. self::setUpTrash($owner); } @@ -286,6 +291,7 @@ public static function move2trash($file_path, $ownerOnly = false) { $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); $gotLock = false; + // Keep trying until we obtain the lock for a unique trash filename. do { /** @var ILockingStorage & IStorage $trashStorage */ [$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath); @@ -293,9 +299,8 @@ public static function move2trash($file_path, $ownerOnly = false) { $trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); $gotLock = true; } catch (LockedException $e) { - // a file with the same name is being deleted concurrently - // nudge the timestamp a bit to resolve the conflict - + // Another deletion is using this filename. Incrementing the + // timestamp gives this item a distinct trash filename. $timestamp = $timestamp + 1; $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); @@ -318,6 +323,8 @@ public static function move2trash($file_path, $ownerOnly = false) { // there is still a possibility that the file has been deleted by a remote user $deletedBy = self::overwriteDeletedBy($user); + // Insert metadata before moving the payload; failed moves remove this row + // again so metadata and the trash payload remain consistent. $query = Server::get(IDBConnection::class)->getQueryBuilder(); $query->insert('files_trash') ->setValue('id', $query->createNamedParameter($filename)) @@ -359,8 +366,10 @@ public static function move2trash($file_path, $ownerOnly = false) { $inCache = $sourceStorage->getCache()->inCache($sourceInternalPath); $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); if ($inCache) { + // Preserve the existing cache entry when the source was cached. $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); } else { + // Populate the destination cache when the source had no cache entry. $sizeDifference = $sourceInfo->getSize(); if ($sizeDifference < 0) { $sizeDifference = null; @@ -377,7 +386,10 @@ public static function move2trash($file_path, $ownerOnly = false) { Server::get(LoggerInterface::class)->error('Couldn\'t move ' . $file_path . ' to the trash bin', ['app' => 'files_trashbin']); } - if ($sourceStorage->file_exists($sourceInternalPath)) { // failed to delete the original file, abort + // A successful trash move must remove the original source. + if ($sourceStorage->file_exists($sourceInternalPath)) { + // The move may have succeeded at the storage level, but the source + // is still present. Restore cache state and mark the operation failed. if ($sourceStorage->is_dir($sourceInternalPath)) { $sourceStorage->rmdir($sourceInternalPath); } else { @@ -385,7 +397,7 @@ public static function move2trash($file_path, $ownerOnly = false) { } if ($sourceStorage->file_exists($sourceInternalPath)) { - // undo the cache move + // Restore the cache relationship to match the fact that the source remains. $sourceStorage->getUpdater()->renameFromStorage($trashStorage, $trashInternalPath, $sourceInternalPath); } else { $trashStorage->getUpdater()->remove($trashInternalPath); @@ -415,12 +427,19 @@ public static function move2trash($file_path, $ownerOnly = false) { } if ($moveSuccessful) { - Util::emitHook('\OCA\Files_Trashbin\Trashbin', 'post_moveToTrash', ['filePath' => Filesystem::normalizePath($file_path), - 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp))]); + Util::emitHook( + '\OCA\Files_Trashbin\Trashbin', + 'post_moveToTrash', + [ + 'filePath' => Filesystem::normalizePath($file_path), + 'trashPath' => Filesystem::normalizePath(static::getTrashFilename($filename, $timestamp)) + ] + ); self::retainVersions($filename, $owner, $ownerPath, $timestamp); - // if owner !== user we need to also add a copy to the users trash + // Shared items are stored in the owner's trash and copied to the + // deleting user's trash for visibility there. if ($user !== $owner && $ownerOnly === false) { self::copyFilesToUser($ownerPath, $owner, $file_path, $user, $timestamp); } @@ -430,7 +449,7 @@ public static function move2trash($file_path, $ownerOnly = false) { self::scheduleExpire($user); - // if owner !== user we also need to update the owners trash size + // Expiration must be scheduled for both trash bins when they differ. if ($owner !== $user) { self::scheduleExpire($owner); } From 24938ce3ccffd9c8af4073f324cc314134ae5f5c Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 7 Aug 2026 22:38:27 -0400 Subject: [PATCH 3/4] test(file_trashbin): improve Trashbin coverage - Configured size fallback: source remains, no trash item is created. - Shared deletion: both owner and deleting user receive trash entries. Assisted-by: Copilot:gpt-5.6-luna Signed-off-by: Josh --- apps/files_trashbin/tests/TrashbinTest.php | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/apps/files_trashbin/tests/TrashbinTest.php b/apps/files_trashbin/tests/TrashbinTest.php index b6bb552517773..f6cf0a0559196 100644 --- a/apps/files_trashbin/tests/TrashbinTest.php +++ b/apps/files_trashbin/tests/TrashbinTest.php @@ -274,6 +274,34 @@ public function testExpireOldFilesShared(): void { $this->verifyArray($filesInTrashUser1AfterDelete, ['user1-2.txt', 'user1-4.txt']); } + public function testDeletingSharedFileAddsItemToBothTrashBins(): void { + $user1Folder = Server::get(IRootFolder::class)->getUserFolder(self::TEST_TRASHBIN_USER1); + $file = $user1Folder->newFile('shared.txt'); + $file->putContent('shared content'); + + $share = Server::get(\OCP\Share\IManager::class)->newShare(); + $share->setShareType(IShare::TYPE_USER) + ->setNode($file) + ->setSharedBy(self::TEST_TRASHBIN_USER1) + ->setSharedWith(self::TEST_TRASHBIN_USER2) + ->setPermissions(Constants::PERMISSION_ALL); + $share = Server::get(\OCP\Share\IManager::class)->createShare($share); + Server::get(\OCP\Share\IManager::class)->acceptShare($share, self::TEST_TRASHBIN_USER2); + + self::loginHelper(self::TEST_TRASHBIN_USER2); + $this->assertTrue(Filesystem::file_exists('shared.txt')); + Filesystem::unlink('shared.txt'); + + $this->verifyArray( + Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1), + ['shared.txt'] + ); + $this->verifyArray( + Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER2), + ['shared.txt'] + ); + } + /** * verify that the array contains the expected results * @@ -687,6 +715,40 @@ public function testTrashSizePropagation(): void { $this->assertEquals(3, $view->getFileInfo('')->getSize()); } + public function testMoveToTrashFallsBackToPermanentDeletionWhenConfiguredSizeIsReached(): void { + $config = Server::get(IConfig::class); + $config->setUserValue( + self::TEST_TRASHBIN_USER1, + 'files_trashbin', + 'trashbin_size', + '1' + ); + + try { + $userFolder = Server::get(IRootFolder::class)->getUserFolder(self::TEST_TRASHBIN_USER1); + $file = $userFolder->newFile('too-large.txt'); + $file->putContent('too large'); + + $this->assertTrue($userFolder->nodeExists('too-large.txt')); + + // Node::delete() returns void. Since the trash move is rejected, + // Storage falls back to permanently deleting the source file. + $file->delete(); + + $this->assertFalse($userFolder->nodeExists('too-large.txt')); + $this->assertCount( + 0, + Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1) + ); + } finally { + $config->deleteUserValue( + self::TEST_TRASHBIN_USER1, + 'files_trashbin', + 'trashbin_size' + ); + } + } + /** * @param string $user * @param bool $create From cb21322fd912e3d97e84185367eeba2739e9017f Mon Sep 17 00:00:00 2001 From: Josh Date: Fri, 7 Aug 2026 22:59:15 -0400 Subject: [PATCH 4/4] chore(files_trashbin): fixup typo in Trashbin class Signed-off-by: Josh --- apps/files_trashbin/lib/Trashbin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 42139e56747bd..6fbde272f1d93 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -254,7 +254,7 @@ public static function move2trash($file_path, $ownerOnly = false) { [$owner, $ownerPath] = self::getUidAndFilename($file_path); // if no owner found (ex: ext storage + share link), will use the current user's trashbin then - if ($owner === null)) { + if ($owner === null) { $owner = $user; $ownerPath = $file_path; }