[stable10] Backport of The recipient of internal share should not inc…

…rease permission The recipient of internal share, should not be able to increase the permission. Signed-off-by: Sujith H <sharidasan@owncloud.com>
owncloud · Jun 13, 2019 · 4ae39f7 · 4ae39f7
1 parent 6d85eb8
commit 4ae39f7
Show file tree

Hide file tree

Showing 2 changed files with 118 additions and 3 deletions.
diff --git a/apps/files_sharing/lib/Controller/Share20OcsController.php b/apps/files_sharing/lib/Controller/Share20OcsController.php
@@ -848,6 +848,10 @@ public function updateShare($id) {
 				$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 				return new Result(null, 400, $this->l->t('Wrong or no update parameter given'));
 			} else {
+				if (($share->getPermissions() < $permissions) &&
+					!$this->canIncreasePermission($share)) {
+					return new Result(null, 404, 'Cannot increase permission of ' . $share->getTarget());
+				}
 				$permissions = (int)$permissions;
 				$share->setPermissions($permissions);
 			}
@@ -889,6 +893,31 @@ public function updateShare($id) {
 		return new Result($this->formatShare($share));
 	}
 
+	/**
+	 * If the user is not the owner of the share, then lets not allow to increase
+	 * the permission of the share.
+	 *
+	 * @param IShare $share
+	 * @return bool , true if permission is allowed to increase, else false. Also false if no user session available
+	 */
+	private function canIncreasePermission(IShare $share) {
+		$userObj = $this->userSession->getUser();
+		if ($userObj === null) {
+			return false;
+		}
+
+		$shareHome = $this->rootFolder->getUserFolder($share->getSharedBy());
+		$nodes = $shareHome->getById($share->getNode()->getId());
+		foreach ($nodes as $node) {
+			$nodeOwner = $node->getOwner()->getUID();
+			if ($nodeOwner !== $userObj->getUID()) {
+				return false;
+			}
+		}
+
+		return true;
+	}
+
 	/**
 	 * @NoCSRFRequired
 	 * @NoAdminRequired

diff --git a/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php b/apps/files_sharing/tests/Controller/Share20OcsControllerTest.php
@@ -2232,7 +2232,8 @@ public function testUpdateShareCannotIncreasePermissions() {
 			->setShareType(Share::SHARE_TYPE_GROUP)
 			->setSharedWith('group1')
 			->setPermissions(\OCP\Constants::PERMISSION_READ)
-			->setNode($folder);
+			->setNode($folder)
+			->setTarget('/folderShare');
 
 		$this->request
 			->method('getParam')
@@ -2251,13 +2252,89 @@ public function testUpdateShareCannotIncreasePermissions() {
 
 		$this->shareManager->expects($this->never())->method('updateShare');
 
-		$expected = new \OC\OCS\Result(null, 404, 'Cannot increase permissions');
+		$folderOwner = $this->createMock(IUser::class);
+		$folderOwner->method('getUID')
+			->willReturn('foo1234');
+		$userHomeFolder = $this->createMock(Folder::class);
+		$userNode = $this->createMock(Folder::class);
+		$userNode->method('getOwner')
+			->willReturn($folderOwner);
+		$userHomeFolder->method('getById')
+			->willReturn([$userNode]);
+		$this->rootFolder->method('getUserFolder')
+			->willReturn($userHomeFolder);
+
+		$expected = new Result(null, 404, 'Cannot increase permission of ' . $share->getTarget());
 		$result = $ocs->updateShare(42);
 
 		$this->assertEquals($expected->getMeta(), $result->getMeta());
 		$this->assertEquals($expected->getData(), $result->getData());
 	}
 
+	public function providesDataForTestingIncreasePermissionRegularShare() {
+		return [
+			['file', 16, 17],
+			['file', 17, 19],
+			['folder', 17, 19],
+			['folder', 19, 21],
+			['folder', 21, 23],
+		];
+	}
+
+	/**
+	 * @dataProvider providesDataForTestingIncreasePermissionRegularShare
+	 *
+	 * @param string $nodeType
+	 * @param int $currentPermission
+	 * @param string $updatePermissionTo
+	 */
+	public function testRegularShareRecipientCannotIncreasePermission($nodeType, $currentPermission, $updatePermissionTo) {
+		$ocs = $this->mockFormatShare();
+
+		$share = \OC::$server->getShareManager()->newShare();
+		$share
+			->setId(42)
+			->setSharedBy($this->currentUser->getUID())
+			->setShareOwner('anotheruser')
+			->setShareType(Share::SHARE_TYPE_USER)
+			->setSharedWith('user1')
+			->setPermissions($currentPermission);
+
+		if ($nodeType === 'file') {
+			$file = $this->createMock(File::class);
+			$share->setNode($file);
+			$share->setTarget('/testFile');
+		} else {
+			$folder = $this->createMock(Folder::class);
+			$share->setNode($folder);
+			$share->setTarget('/testFolder');
+		}
+
+		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
+
+		$this->request
+			->method('getParam')
+			->will($this->returnValueMap([
+				['permissions', null, $updatePermissionTo],
+			]));
+
+		$folderOwner = $this->createMock(IUser::class);
+		$folderOwner->method('getUID')
+			->willReturn('foo1234');
+		$userHomeFolder = $this->createMock(Folder::class);
+		$userNode = $this->createMock(Folder::class);
+		$userNode->method('getOwner')
+			->willReturn($folderOwner);
+		$userHomeFolder->method('getById')
+			->willReturn([$userNode]);
+		$this->rootFolder->method('getUserFolder')
+			->willReturn($userHomeFolder);
+
+		$result = $ocs->updateShare(42);
+		$this->assertEquals(404, $result->getStatusCode());
+		$this->assertEquals('Cannot increase permission of ' . $share->getTarget(), $result->getMeta()['message']);
+	}
+
 	/**
 	 * @dataProvider publicUploadParamsProvider
 	 */
@@ -2360,7 +2437,16 @@ public function testUpdateShareCanIncreasePermissionsIfOwner() {
 			->with($share)
 			->willReturn($share);
 
-		$expected = new \OC\OCS\Result();
+		$userHomeFolder = $this->createMock(Folder::class);
+		$userNode = $this->createMock(Folder::class);
+		$userNode->method('getOwner')
+			->willReturn($this->currentUser);
+		$userHomeFolder->method('getById')
+			->willReturn([$userNode]);
+		$this->rootFolder->method('getUserFolder')
+			->willReturn($userHomeFolder);
+
+		$expected = new Result();
 		$result = $ocs->updateShare(42);
 
 		$this->assertEquals($expected->getMeta(), $result->getMeta());