Merge pull request #20841 from nextcloud/trashbin-locking

add locking to resolve concurent move to trashbin conflicts

apps/files_trashbin/lib/Trashbin.php

@@ -47,10 +47,13 @@ use OC\Files\Filesystem;
 use OC\Files\View;
 use OCA\Files_Trashbin\AppInfo\Application;
 use OCA\Files_Trashbin\Command\Expire;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
+use OCP\Lock\ILockingProvider;
+use OCP\Lock\LockedException;
 use OCP\User;
 
 class Trashbin {
@@ -220,8 +223,8 @@ class Trashbin {
 	public static function move2trash($file_path, $ownerOnly = false) {
 		// get the user for which the filesystem is setup
 		$root = Filesystem::getRoot();
-		list(, $user) = explode('/', $root);
+		[, $user] = explode('/', $root);
-		list($owner, $ownerPath) = self::getUidAndFilename($file_path);
+		[$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)) {
@@ -245,15 +248,36 @@ class Trashbin {
 
 		$filename = $path_parts['basename'];
 		$location = $path_parts['dirname'];
-		$timestamp = time();
+		/** @var ITimeFactory $timeFactory */
+		$timeFactory = \OC::$server->query(ITimeFactory::class);
+		$timestamp = $timeFactory->getTime();
+
+		$lockingProvider = \OC::$server->getLockingProvider();
 
 		// disable proxy to prevent recursive calls
 		$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
+		$gotLock = false;
 
+		while (!$gotLock) {
+			try {
 				/** @var \OC\Files\Storage\Storage $trashStorage */
-		list($trashStorage, $trashInternalPath) = $ownerView->resolvePath($trashPath);
+				[$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath);
+
+				$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
+
+				$timestamp = $timestamp + 1;
+
+				$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
+			}
+		}
+
 		/** @var \OC\Files\Storage\Storage $sourceStorage */
-		list($sourceStorage, $sourceInternalPath) = $ownerView->resolvePath('/files/' . $ownerPath);
+		[$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath);
+
 		try {
 			$moveSuccessful = true;
 			if ($trashStorage->file_exists($trashInternalPath)) {
@@ -296,6 +320,8 @@ class Trashbin {
 			}
 		}
 
+		$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
 		self::scheduleExpire($user);
 
 		// if owner !== user we also need to update the owners trash size
@@ -345,9 +371,9 @@ class Trashbin {
 	 */
 	private static function move(View $view, $source, $target) {
 		/** @var \OC\Files\Storage\Storage $sourceStorage */
-		list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
+		[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
 		/** @var \OC\Files\Storage\Storage $targetStorage */
-		list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
+		[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
 		/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
 
 		$result = $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@@ -367,9 +393,9 @@ class Trashbin {
 	 */
 	private static function copy(View $view, $source, $target) {
 		/** @var \OC\Files\Storage\Storage $sourceStorage */
-		list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
+		[$sourceStorage, $sourceInternalPath] = $view->resolvePath($source);
 		/** @var \OC\Files\Storage\Storage $targetStorage */
-		list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
+		[$targetStorage, $targetInternalPath] = $view->resolvePath($target);
 		/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
 
 		$result = $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
@@ -465,7 +491,7 @@ class Trashbin {
 
 			$target = Filesystem::normalizePath('/' . $location . '/' . $uniqueFilename);
 
-			list($owner, $ownerPath) = self::getUidAndFilename($target);
+			[$owner, $ownerPath] = self::getUidAndFilename($target);
 
 			// file has been deleted in between
 			if (empty($ownerPath)) {
@@ -731,7 +757,7 @@ class Trashbin {
 		$dirContent = Helper::getTrashFiles('/', $user, 'mtime');
 
 		// delete all files older then $retention_obligation
-		list($delSize, $count) = self::deleteExpiredFiles($dirContent, $user);
+		[$delSize, $count] = self::deleteExpiredFiles($dirContent, $user);
 
 		$availableSpace += $delSize;
 
@@ -868,7 +894,7 @@ class Trashbin {
 		//force rescan of versions, local storage may not have updated the cache
 		if (!self::$scannedVersions) {
 			/** @var \OC\Files\Storage\Storage $storage */
-			list($storage,) = $view->resolvePath('/');
+			[$storage,] = $view->resolvePath('/');
 			$storage->getScanner()->scan('files_trashbin/versions');
 			self::$scannedVersions = true;
 		}

apps/files_trashbin/tests/StorageTest.php

@@ -37,12 +37,14 @@ use OC\Files\Storage\Temporary;
 use OCA\Files_Trashbin\Events\MoveToTrashEvent;
 use OCA\Files_Trashbin\Storage;
 use OCA\Files_Trashbin\Trash\ITrashManager;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Files\Cache\ICache;
 use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\Files\Node;
 use OCP\ILogger;
 use OCP\IUserManager;
+use OCP\Lock\ILockingProvider;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 
 /**
@@ -107,7 +109,7 @@ class StorageTest extends \Test\TestCase {
 	public function testSingleStorageDeleteFile() {
 		$this->assertTrue($this->userView->file_exists('test.txt'));
 		$this->userView->unlink('test.txt');
-		list($storage,) = $this->userView->resolvePath('test.txt');
+		[$storage,] = $this->userView->resolvePath('test.txt');
 		$storage->getScanner()->scan(''); // make sure we check the storage
 		$this->assertFalse($this->userView->getFileInfo('test.txt'));
 
@@ -124,7 +126,7 @@ class StorageTest extends \Test\TestCase {
 	public function testSingleStorageDeleteFolder() {
 		$this->assertTrue($this->userView->file_exists('folder/inside.txt'));
 		$this->userView->rmdir('folder');
-		list($storage,) = $this->userView->resolvePath('folder/inside.txt');
+		[$storage,] = $this->userView->resolvePath('folder/inside.txt');
 		$storage->getScanner()->scan(''); // make sure we check the storage
 		$this->assertFalse($this->userView->getFileInfo('folder'));
 
@@ -213,7 +215,7 @@ class StorageTest extends \Test\TestCase {
 		$this->userView->unlink('test.txt');
 
 		// rescan trash storage
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// check if versions are in trashbin
@@ -242,7 +244,7 @@ class StorageTest extends \Test\TestCase {
 		$this->userView->rmdir('folder');
 
 		// rescan trash storage
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// check if versions are in trashbin
@@ -296,7 +298,7 @@ class StorageTest extends \Test\TestCase {
 		$recipientView->unlink('share/test.txt');
 
 		// rescan trash storage for both users
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// check if versions are in trashbin for both users
@@ -350,7 +352,7 @@ class StorageTest extends \Test\TestCase {
 		$recipientView->rmdir('share/folder');
 
 		// rescan trash storage
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// check if versions are in trashbin for owner
@@ -407,7 +409,7 @@ class StorageTest extends \Test\TestCase {
 		$this->assertTrue($this->userView->file_exists('substorage/test.txt'));
 
 		// rescan trash storage
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// versions were moved too
@@ -448,7 +450,7 @@ class StorageTest extends \Test\TestCase {
 		$this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt'));
 
 		// rescan trash storage
-		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		[$rootStorage,] = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// versions were moved too
@@ -606,4 +608,36 @@ class StorageTest extends \Test\TestCase {
 			$this->addToAssertionCount(1);
 		}
 	}
+
+	public function testTrashbinCollision() {
+		$this->userView->file_put_contents('test.txt', 'foo');
+		$this->userView->file_put_contents('folder/test.txt', 'bar');
+
+		$timeFactory = $this->createMock(ITimeFactory::class);
+		$timeFactory->method('getTime')
+			->willReturn(1000);
+
+		$lockingProvider = \OC::$server->getLockingProvider();
+
+		$this->overwriteService(ITimeFactory::class, $timeFactory);
+
+		$this->userView->unlink('test.txt');
+
+		$this->assertTrue($this->rootView->file_exists('/' . $this->user . '/files_trashbin/files/test.txt.d1000'));
+
+		/** @var \OC\Files\Storage\Storage $trashStorage */
+		[$trashStorage, $trashInternalPath] = $this->rootView->resolvePath('/' . $this->user . '/files_trashbin/files/test.txt.d1000');
+
+		/// simulate a concurrent delete
+		$trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+		$this->userView->unlink('folder/test.txt');
+
+		$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);
+
+		$this->assertTrue($this->rootView->file_exists($this->user . '/files_trashbin/files/test.txt.d1001'));
+
+		$this->assertEquals('foo', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1000'));
+		$this->assertEquals('bar', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1001'));
+	}
 }