Add locking to modifying operation of the OCS Share API

Fixes #17243

This is done in the OCS Share API instead of the share manager since we
want lazy shares in general. However when doing modifying calls via the
OCS Share API it is fine to force real nodes.

* Updated unit tests to work with logging

apps/files_sharing/api/share20ocs.php

@@ -28,6 +28,7 @@ use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\Files\IRootFolder;
+use OCP\Lock\LockedException;
 use OCP\Share;
 use OCP\Share\IManager;
 
@@ -205,12 +206,20 @@ class Share20OCS {
 			return new \OC_OCS_Result(null, 404, $this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
 
+		try {
+			$share->getNode()->lock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		} catch (LockedException $e) {
+			return new \OC_OCS_Result(null, 404, 'could not delete share');
+		}
+
 		if (!$this->canAccessShare($share)) {
 			return new \OC_OCS_Result(null, 404, $this->l->t('Could not delete share'));
 		}
 
 		$this->shareManager->deleteShare($share);
 
+		$share->getNode()->unlock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		return new \OC_OCS_Result();
 	}
 
@@ -238,6 +247,7 @@ class Share20OCS {
 		}
 
 		$share->setNode($path);
+		$share->getNode()->lock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		// Parse permissions (if available)
 		$permissions = $this->request->getParam('permissions', null);
@@ -368,8 +378,11 @@ class Share20OCS {
 			return new \OC_OCS_Result(null, 403, $e->getMessage());
 		}
 
-		$share = $this->formatShare($share);
-		return new \OC_OCS_Result($share);
+		$output = $this->formatShare($share);
+
+		$share->getNode()->unlock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		return new \OC_OCS_Result($output);
 	}
 
 	/**
@@ -512,6 +525,8 @@ class Share20OCS {
 			return new \OC_OCS_Result(null, 404, $this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
 
+		$share->getNode()->lock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		if (!$this->canAccessShare($share)) {
 			return new \OC_OCS_Result(null, 404, $this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
@@ -611,6 +626,8 @@ class Share20OCS {
 			return new \OC_OCS_Result(null, 400, $e->getMessage());
 		}
 
+		$share->getNode()->unlock(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		return new \OC_OCS_Result($this->formatShare($share));
 	}
 

apps/files_sharing/tests/api/share20ocstest.php

@@ -29,6 +29,7 @@ use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\Files\IRootFolder;
+use OCP\Lock\LockedException;
 
 /**
  * Class Share20OCSTest
@@ -137,8 +138,11 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testDeleteShare() {
+		$node = $this->getMock('\OCP\Files\File');
+
 		$share = $this->newShare();
-		$share->setSharedBy($this->currentUser->getUID());
+		$share->setSharedBy($this->currentUser->getUID())
+			->setNode($node);
 		$this->shareManager
 			->expects($this->once())
 			->method('getShareById')
@@ -149,10 +153,45 @@ class Share20OCSTest extends \Test\TestCase {
 			->method('deleteShare')
 			->with($share);
 
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$node->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$expected = new \OC_OCS_Result();
 		$this->assertEquals($expected, $this->ocs->deleteShare(42));
 	}
 
+	public function testDeleteShareLocked() {
+		$node = $this->getMock('\OCP\Files\File');
+
+		$share = $this->newShare();
+		$share->setSharedBy($this->currentUser->getUID())
+			->setNode($node);
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+		$this->shareManager
+			->expects($this->never())
+			->method('deleteShare')
+			->with($share);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED)
+			->will($this->throwException(new LockedException('mypath')));
+		$node->expects($this->never())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$expected = new \OC_OCS_Result(null, 404, 'could not delete share');
+		$this->assertEquals($expected, $this->ocs->deleteShare(42));
+	}
+
 	/*
 	 * FIXME: Enable once we have a federated Share Provider
 
@@ -526,7 +565,7 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareInvalidPermissions() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
 
 		$this->request
@@ -548,6 +587,10 @@ class Share20OCSTest extends \Test\TestCase {
 				->with('valid-path')
 				->willReturn($path);
 
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$expected = new \OC_OCS_Result(null, 404, 'invalid permissions');
 
 		$result = $this->ocs->createShare();
@@ -557,7 +600,7 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareUserNoShareWith() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
 
 		$this->request
@@ -585,6 +628,10 @@ class Share20OCSTest extends \Test\TestCase {
 			->with('valid-path')
 			->willReturn($path);
 
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$expected = new \OC_OCS_Result(null, 404, 'Please specify a valid user');
 
 		$result = $this->ocs->createShare();
@@ -594,7 +641,7 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareUserNoValidShareWith() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
 
 		$this->request
@@ -625,6 +672,10 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$expected = new \OC_OCS_Result(null, 404, 'Please specify a valid user');
 
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$result = $this->ocs->createShare();
 
 		$this->assertEquals($expected->getMeta(), $result->getMeta());
@@ -632,9 +683,8 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareUser() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
-		$this->shareManager->method('createShare')->will($this->returnArgument(0));
 
 		$ocs = $this->getMockBuilder('OCA\Files_Sharing\API\Share20OCS')
 			->setConstructorArgs([
@@ -677,15 +727,26 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$this->userManager->method('userExists')->with('validUser')->willReturn(true);
 
-		$share->method('setNode')->with($path);
-		$share->method('setPermissions')
-			->with(
-				\OCP\Constants::PERMISSION_ALL &
-				~\OCP\Constants::PERMISSION_DELETE &
-				~\OCP\Constants::PERMISSION_CREATE);
-		$share->method('setShareType')->with(\OCP\Share::SHARE_TYPE_USER);
-		$share->method('setSharedWith')->with('validUser');
-		$share->method('setSharedBy')->with('currentUser');
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$path->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->shareManager->method('createShare')
+			->with($this->callback(function (\OCP\Share\IShare $share) use ($path) {
+				return $share->getNode() === $path &&
+					$share->getPermissions() === (
+						\OCP\Constants::PERMISSION_ALL &
+						~\OCP\Constants::PERMISSION_DELETE &
+						~\OCP\Constants::PERMISSION_CREATE
+					) &&
+					$share->getShareType() === \OCP\Share::SHARE_TYPE_USER &&
+					$share->getSharedWith() === 'validUser' &&
+					$share->getSharedBy() === 'currentUser';
+			}))
+			->will($this->returnArgument(0));
 
 		$expected = new \OC_OCS_Result();
 		$result = $ocs->createShare();
@@ -695,7 +756,7 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareGroupNoValidShareWith() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
 		$this->shareManager->method('createShare')->will($this->returnArgument(0));
 
@@ -727,6 +788,10 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$expected = new \OC_OCS_Result(null, 404, 'Please specify a valid user');
 
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$result = $this->ocs->createShare();
 
 		$this->assertEquals($expected->getMeta(), $result->getMeta());
@@ -734,9 +799,8 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareGroup() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
-		$this->shareManager->method('createShare')->will($this->returnArgument(0));
 
 		$ocs = $this->getMockBuilder('OCA\Files_Sharing\API\Share20OCS')
 			->setConstructorArgs([
@@ -783,11 +847,22 @@ class Share20OCSTest extends \Test\TestCase {
 			->method('allowGroupSharing')
 			->willReturn(true);
 
-		$share->method('setNode')->with($path);
-		$share->method('setPermissions')->with(\OCP\Constants::PERMISSION_ALL);
-		$share->method('setShareType')->with(\OCP\Share::SHARE_TYPE_GROUP);
-		$share->method('setSharedWith')->with('validGroup');
-		$share->method('setSharedBy')->with('currentUser');
+		$path->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$path->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->shareManager->method('createShare')
+			->with($this->callback(function (\OCP\Share\IShare $share) use ($path) {
+				return $share->getNode() === $path &&
+					$share->getPermissions() === \OCP\Constants::PERMISSION_ALL &&
+					$share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP &&
+					$share->getSharedWith() === 'validGroup' &&
+					$share->getSharedBy() === 'currentUser';
+			}))
+			->will($this->returnArgument(0));
 
 		$expected = new \OC_OCS_Result();
 		$result = $ocs->createShare();
@@ -797,7 +872,7 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testCreateShareGroupNotAllowed() {
-		$share = $this->getMock('\OCP\Share\IShare');
+		$share = $this->newShare();
 		$this->shareManager->method('newShare')->willReturn($share);
 
 		$this->request
@@ -832,9 +907,8 @@ class Share20OCSTest extends \Test\TestCase {
 			->method('allowGroupSharing')
 			->willReturn(false);
 
-		$share->method('setNode')->with($path);
-
 		$expected = new \OC_OCS_Result(null, 404, 'Group sharing is disabled by the administrator');
+
 		$result = $this->ocs->createShare();
 
 		$this->assertEquals($expected->getMeta(), $result->getMeta());
@@ -1154,7 +1228,13 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testUpdateShareCantAccess() {
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\Folder');
+		$share = $this->newShare();
+		$share->setNode($node);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
 
@@ -1166,10 +1246,16 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testUpdateNoParametersLink() {
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\Folder');
+		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser->getUID())
-			->setShareType(\OCP\Share::SHARE_TYPE_LINK);
+			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
+			->setNode($node);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
 
@@ -1181,10 +1267,16 @@ class Share20OCSTest extends \Test\TestCase {
 	}
 
 	public function testUpdateNoParametersOther() {
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\Folder');
+		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser->getUID())
-			->setShareType(\OCP\Share::SHARE_TYPE_GROUP);
+			->setShareType(\OCP\Share::SHARE_TYPE_GROUP)
+			->setNode($node);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
 
@@ -1198,13 +1290,22 @@ class Share20OCSTest extends \Test\TestCase {
 	public function testUpdateLinkShareClear() {
 		$ocs = $this->mockFormatShare();
 
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\Folder');
+		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser->getUID())
 			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
 			->setPassword('password')
 			->setExpirationDate(new \DateTime())
-			->setPermissions(\OCP\Constants::PERMISSION_ALL);
+			->setPermissions(\OCP\Constants::PERMISSION_ALL)
+			->setNode($node);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$node->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		$this->request
 			->method('getParam')
@@ -1364,13 +1465,22 @@ class Share20OCSTest extends \Test\TestCase {
 		$date = new \DateTime('2000-01-01');
 		$date->setTime(0,0,0);
 
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\File');
+		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser->getUID())
 			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
 			->setPassword('password')
 			->setExpirationDate($date)
-			->setPermissions(\OCP\Constants::PERMISSION_ALL);
+			->setPermissions(\OCP\Constants::PERMISSION_ALL)
+			->setNode($node);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$node->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
 
 		$this->request
 			->method('getParam')
@@ -1398,13 +1508,15 @@ class Share20OCSTest extends \Test\TestCase {
 	public function testUpdateLinkShareExpireDateDoesNotChangeOther() {
 		$ocs = $this->mockFormatShare();
 
-		$share = \OC::$server->getShareManager()->newShare();
+		$node = $this->getMock('\OCP\Files\File');
+		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser->getUID())
 			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
 			->setPassword('password')
 			->setExpirationDate(new \DateTime())
-			->setPermissions(\OCP\Constants::PERMISSION_ALL);
+			->setPermissions(\OCP\Constants::PERMISSION_ALL)
+			->setNode($node);
 
 		$this->request
 			->method('getParam')
@@ -1412,6 +1524,13 @@ class Share20OCSTest extends \Test\TestCase {
 				['expireDate', null, '2010-12-23'],
 			]));
 
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+		$node->expects($this->once())
+			->method('unlock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
 		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
 
 		$this->shareManager->expects($this->once())->method('updateShare')->with(