Skip to content

Commit

Permalink
add locking to resolve concurent move to trashbin conflicts
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@icewind1991
icewind1991 committed May 6, 2020
1 parent d5850eb commit aa90fb3
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 24 deletions.
5 changes: 2 additions & 3 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,8 @@ protected function formatShare(IShare $share, Node $recipientNode = null): array
// TODO: It might make sense to have a dedicated setting to allow/deny converting link shares into federated ones
// For link shares, we need to have the PERMISSION_SHARE if federated is enabled
if ($this->shareManager->outgoingServer2ServerSharesAllowed()) {
if ($share->getShareType() === IShare::TYPE_LINK
|| $share->getShareType() === IShare::TYPE_EMAIL) {
$result['permissions'] |= Constants::PERMISSION_SHARE;
if ($share->getShareType() === IShare::TYPE_LINK || $share->getShareType() === IShare::TYPE_EMAIL) {
$result['permissions'] |= Constants::PERMISSION_SHARE;
}
}

Expand Down
52 changes: 39 additions & 13 deletions apps/files_trashbin/lib/Trashbin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,13 @@
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 {
Expand Down Expand Up @@ -220,8 +223,8 @@ private static function copyFilesToUser($sourcePath, $owner, $targetPath, $user,
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)) {
Expand All @@ -245,15 +248,36 @@ public static function move2trash($file_path, $ownerOnly = false) {

$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)) {
Expand Down Expand Up @@ -296,6 +320,8 @@ public static function move2trash($file_path, $ownerOnly = false) {
}
}

$trashStorage->releaseLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider);

self::scheduleExpire($user);

// if owner !== user we also need to update the owners trash size
Expand Down Expand Up @@ -345,9 +371,9 @@ private static function retainVersions($filename, $owner, $ownerPath, $timestamp
*/
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);
Expand All @@ -367,9 +393,9 @@ private static function move(View $view, $source, $target) {
*/
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);
Expand Down Expand Up @@ -465,7 +491,7 @@ private static function restoreVersions(View $view, $file, $filename, $uniqueFil

$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)) {
Expand Down Expand Up @@ -731,7 +757,7 @@ public static function expire($user) {
$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;

Expand Down Expand Up @@ -868,7 +894,7 @@ private static function getVersionsFromTrash($filename, $timestamp, $user) {
//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;
}
Expand Down
50 changes: 42 additions & 8 deletions apps/files_trashbin/tests/StorageTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
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;

/**
Expand Down Expand Up @@ -107,7 +109,7 @@ protected function tearDown(): void {
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'));

Expand All @@ -124,7 +126,7 @@ public function testSingleStorageDeleteFile() {
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'));

Expand Down Expand Up @@ -213,7 +215,7 @@ public function testDeleteVersionsOfFile() {
$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
Expand Down Expand Up @@ -242,7 +244,7 @@ public function testDeleteVersionsOfFolder() {
$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
Expand Down Expand Up @@ -296,7 +298,7 @@ public function testDeleteVersionsOfFileAsRecipient() {
$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
Expand Down Expand Up @@ -350,7 +352,7 @@ public function testDeleteVersionsOfFolderAsRecipient() {
$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
Expand Down Expand Up @@ -407,7 +409,7 @@ public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() {
$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
Expand Down Expand Up @@ -448,7 +450,7 @@ public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() {
$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
Expand Down Expand Up @@ -606,4 +608,36 @@ public function testSingleStorageDeleteFileLoggedOut() {
$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'));
}
}

0 comments on commit aa90fb3

Please sign in to comment.