diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index 41301c84f41d6..6fbde272f1d93 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,32 +209,43 @@ 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']); } } /** - * 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 @@ -236,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) { @@ -256,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); } @@ -274,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); @@ -281,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); @@ -306,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)) @@ -347,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; @@ -365,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 { @@ -373,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); @@ -403,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); } @@ -418,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); } 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