add locking to resolve concurent move to trashbin conflicts

uses a lock to prevent two requests from moving a file to the trashbin concurrently
(causing sql duplicate key errors)

Signed-off-by: Robin Appelman <robin@icewind.nl>

diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php
index a06a5145d999be72f12c4ab348b120fcd7976296..ef406349c300111430ba748814154ec5d6c8914c 100644 (file)
--- a/apps/files_trashbin/lib/Trashbin.php
+++ b/apps/files_trashbin/lib/Trashbin.php
@@ -45,10 +45,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 {
@@ -218,8 +221,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);
-               list($owner, $ownerPath) = self::getUidAndFilename($file_path);
+               [, $user] = explode('/', $root);
+               [$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)) {
@@ -243,15 +246,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 */
+                               [$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 $trashStorage */
-               list($trashStorage, $trashInternalPath) = $ownerView->resolvePath($trashPath);
                /** @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)) {
@@ -294,6 +318,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);
@@ -467,7 +493,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)) {
@@ -734,7 +760,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;
 
@@ -869,7 +895,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;
                }

diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php
index eff33f9b30c517213891d7673031e1e4cfebd795..939c3209ff830bd7bb1fe0b9e6a1959c2a55fd1b 100644 (file)
--- a/apps/files_trashbin/tests/StorageTest.php
+++ b/apps/files_trashbin/tests/StorageTest.php
@@ -35,11 +35,13 @@ use OC\Files\Filesystem;
 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\IRootFolder;
 use OCP\Files\Node;
 use OCP\ILogger;
 use OCP\IUserManager;
+use OCP\Lock\ILockingProvider;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 
 /**
@@ -102,7 +104,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'));
 
@@ -119,7 +121,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'));
 
@@ -208,7 +210,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
@@ -237,7 +239,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
@@ -290,7 +292,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
@@ -343,7 +345,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
@@ -400,7 +402,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
@@ -441,7 +443,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
@@ -598,4 +600,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'));
+       }
 }