Merge pull request #16143 from nextcloud/backport/16097/stable14

[stabel 14] Better check reshare permissions

4 files changed, 212 insertions, 7 deletions

diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 21da5d72b10..ee4a67ed441 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -895,10 +895,19 @@ class ShareAPIController extends OCSController {
 		}
 
 		if ($permissions !== null && $share->getShareOwner() !== $this->currentUser) {
-			/* Check if this is an incomming share */
-			$incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $share->getNode(), -1, 0);
-			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0));
-			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0));
+			// Get the root mount point for the user and check the share permissions there
+			$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
+			$userNodes = $userFolder->getById($share->getNodeId());
+			$userNode = array_shift($userNodes);
+
+			$userMountPointId = $userNode->getMountPoint()->getStorageRootId();
+			$userMountPoints = $userFolder->getById($userMountPointId);
+			$userMountPoint = array_shift($userMountPoints);
+
+			/* Check if this is an incoming share */
+			$incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
+			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
+			$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0));
 
 			/** @var \OCP\Share\IShare[] $incomingShares */
 			if (!empty($incomingShares)) {
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
index 15c4071bc46..71af69f8779 100644
--- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
+++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -31,6 +31,7 @@ use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\Files\File;
 use OCP\Files\Folder;
+use OCP\Files\Mount\IMountPoint;
 use OCP\Files\Storage;
 use OCP\IConfig;
 use OCP\IL10N;
@@ -1498,6 +1499,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$node = $this->getMockBuilder(Folder::class)->getMock();
+		$node->method('getId')
+			->willReturn(42);
 		$share = $this->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
 			->setSharedBy($this->currentUser)
@@ -1524,6 +1527,19 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$node]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$node->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, '', null, 'false', '');
 
@@ -1535,6 +1551,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->getMockBuilder(Folder::class)->getMock();
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1559,6 +1577,19 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, 'password', null, 'true', '2000-01-01');
 
@@ -1573,6 +1604,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->getMockBuilder(Folder::class)->getMock();
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1593,6 +1626,19 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, $permissions, $password, null, $publicUpload, $expireDate);
 
@@ -1760,6 +1806,8 @@ class ShareAPIControllerTest extends TestCase {
 		$date = new \DateTime('2000-01-01');
 
 		$folder = $this->getMockBuilder(Folder::class)->getMock();
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1784,6 +1832,19 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, null, null, 'true', null);
 
@@ -1797,6 +1858,8 @@ class ShareAPIControllerTest extends TestCase {
 		$date = new \DateTime('2000-01-01');
 
 		$folder = $this->getMockBuilder(Folder::class)->getMock();
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1820,6 +1883,19 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 7, null, null, null, null);
 
@@ -1833,6 +1909,8 @@ class ShareAPIControllerTest extends TestCase {
 		$date = new \DateTime('2000-01-01');
 
 		$folder = $this->getMockBuilder(Folder::class)->getMock();
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1856,6 +1934,19 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 31, null, null, null, null);
 
@@ -1867,6 +1958,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$file = $this->getMockBuilder(File::class)->getMock();
+		$file->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
@@ -1885,6 +1978,19 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$file]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$file->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 31, null, null, null, null);
 
@@ -1896,6 +2002,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->createMock(Folder::class);
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share
@@ -1935,6 +2043,19 @@ class ShareAPIControllerTest extends TestCase {
 				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
 			]));
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$this->shareManager->expects($this->never())->method('updateShare');
 
 		try {
@@ -1949,6 +2070,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->createMock(Folder::class);
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share
@@ -1981,6 +2104,19 @@ class ShareAPIControllerTest extends TestCase {
 				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
 			]));
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$this->shareManager->expects($this->never())->method('updateShare');
 		$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true);
 
@@ -1996,6 +2132,8 @@ class ShareAPIControllerTest extends TestCase {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->createMock(Folder::class);
+		$folder->method('getId')
+			->willReturn(42);
 
 		$share = \OC::$server->getShareManager()->newShare();
 		$share
@@ -2045,6 +2183,19 @@ class ShareAPIControllerTest extends TestCase {
 				['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, [$incomingShare]]
 			]));
 
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->with($this->currentUser)
+			->willReturn($userFolder);
+		$userFolder->method('getById')
+			->with(42)
+			->willReturn([$folder]);
+		$mountPoint = $this->createMock(IMountPoint::class);
+		$folder->method('getMountPoint')
+			->willReturn($mountPoint);
+		$mountPoint->method('getStorageRootId')
+			->willReturn(42);
+
 		$this->shareManager->expects($this->never())->method('updateShare');
 
 		try {
diff --git a/build/integration/features/sharing-v1-part2.feature b/build/integration/features/sharing-v1-part2.feature
index e90d44d1a61..f6532ea564d 100644
--- a/build/integration/features/sharing-v1-part2.feature
+++ b/build/integration/features/sharing-v1-part2.feature
@@ -417,6 +417,28 @@ Feature: sharing
       | permissions | 31 |
     Then the OCS status code should be "404"
 
+  Scenario: Do not allow sub reshare to exceed permissions
+    Given user "user0" exists
+    And user "user1" exists
+    And user "user2" exists
+    And user "user0" created a folder "/TMP"
+    And user "user0" created a folder "/TMP/SUB"
+    And As an "user0"
+    And creating a share with
+      | path | /TMP |
+      | shareType | 0 |
+      | shareWith | user1 |
+      | permissions | 21 |
+    And As an "user1"
+    And creating a share with
+      | path | /TMP/SUB |
+      | shareType | 0 |
+      | shareWith | user2 |
+      | permissions | 21 |
+    When Updating last share with
+      | permissions | 31 |
+    Then the OCS status code should be "404"
+
   Scenario: Only allow 1 link share per file/folder
     Given user "user0" exists
     And As an "user0"
diff --git a/build/integration/features/sharing-v1-part3.feature b/build/integration/features/sharing-v1-part3.feature
index 44a41341a02..c559008c43c 100644
--- a/build/integration/features/sharing-v1-part3.feature
+++ b/build/integration/features/sharing-v1-part3.feature
@@ -339,14 +339,16 @@ Feature: sharing
 
   Scenario: do not allow to increase link share permissions on reshare
     Given As an "admin"
-    And user "admin" created a folder "/TMP"
     And user "user0" exists
+    And user "user1" exists
+    And user "user0" created a folder "/TMP"
+    And As an "user0"
     And creating a share with
       | path | TMP |
       | shareType | 0 |
-      | shareWith | user0 |
+      | shareWith | user1 |
       | permissions | 17  |
-    When As an "user0"
+    When As an "user1"
     And creating a share with
       | path | TMP |
       | shareType | 3 |
@@ -355,6 +357,27 @@ Feature: sharing
     Then the OCS status code should be "404"
     And the HTTP status code should be "200"
 
+  Scenario: do not allow to increase link share permissions on sub reshare
+    Given As an "admin"
+    And user "user0" exists
+    And user "user1" exists
+    And user "user0" created a folder "/TMP"
+    And user "user0" created a folder "/TMP/SUB"
+    And As an "user0"
+    And creating a share with
+      | path | TMP |
+      | shareType | 0 |
+      | shareWith | user1 |
+      | permissions | 17  |
+    When As an "user1"
+    And creating a share with
+      | path | TMP/SUB |
+      | shareType | 3 |
+    And Updating last share with
+      | publicUpload | true |
+    Then the OCS status code should be "404"
+    And the HTTP status code should be "200"
+
   Scenario: deleting file out of a share as recipient creates a backup for the owner
     Given As an "admin"
     And user "user0" exists