Add checks for whether a user with access to a share can delete it

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
nextcloud · Oct 4, 2019 · b1069b2 · b1069b2
1 parent f02cff1
commit b1069b2
Show file tree

Hide file tree

Showing 2 changed files with 292 additions and 12 deletions.
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -336,21 +336,24 @@ public function deleteShare(string $id): DataResponse {
 		try {
 			$this->lock($share->getNode());
 		} catch (LockedException $e) {
-			throw new OCSNotFoundException($this->l->t('could not delete share'));
+			throw new OCSNotFoundException($this->l->t('Could not delete share'));
 		}
 
 		if (!$this->canAccessShare($share)) {
-			throw new OCSNotFoundException($this->l->t('Could not delete share'));
+			throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
 
-		if ((
-				$share->getShareType() === Share::SHARE_TYPE_GROUP
-				|| $share->getShareType() === Share::SHARE_TYPE_ROOM
-			)
-			&& $share->getShareOwner() !== $this->currentUser
-			&& $share->getSharedBy() !== $this->currentUser) {
+		// if it's a group share or a room share
+		// we don't delete the share, but only the
+		// mount point. Allowing it to be restored
+		// from the deleted shares
+		if ($this->canDeleteShareFromSelf($share)) {
 			$this->shareManager->deleteFromSelf($share, $this->currentUser);
 		} else {
+			if (!$this->canDeleteShare($share)) {
+				throw new OCSForbiddenException($this->l->t('Could not delete share'));
+			}
+
 			$this->shareManager->deleteShare($share);
 		}
 
@@ -500,7 +503,6 @@ public function createShare(
 				}
 			}
 
-
 			if ($sendPasswordByTalk === 'true') {
 				if (!$this->appManager->isEnabledForUser('spreed')) {
 					throw new OCSForbiddenException($this->l->t('Sharing %s sending the password by Nextcloud Talk failed because Nextcloud Talk is not enabled', [$path->getPath()]));
@@ -1052,6 +1054,83 @@ protected function canEditShare(\OCP\Share\IShare $share): bool {
 		return false;
 	}
 
+	/**
+	 * Does the user have delete permission on the share
+	 *
+	 * @param \OCP\Share\IShare $share the share to check
+	 * @return boolean
+	 */
+	protected function canDeleteShare(\OCP\Share\IShare $share): bool {
+		// A file with permissions 0 can't be accessed by us. So Don't show it
+		if ($share->getPermissions() === 0) {
+			return false;
+		}
+
+		// if the user is the recipient, i can unshare
+		// the share with self
+		if ($share->getShareType() === Share::SHARE_TYPE_USER &&
+			$share->getSharedWith() === $this->currentUser
+		) {
+			return true;
+		}
+
+		// The owner of the file and the creator of the share
+		// can always delete the share
+		if ($share->getShareOwner() === $this->currentUser ||
+			$share->getSharedBy() === $this->currentUser
+		) {
+			return true;
+		}
+
+		return false;
+	}
+
+	/**
+	 * Does the user have delete permission on the share
+	 * This differs from the canDeleteShare function as it only
+	 * remove the share for the current user. It does NOT
+	 * completely delete the share but only the mount point.
+	 * It can then be restored from the deleted shares section.
+	 *
+	 * @param \OCP\Share\IShare $share the share to check
+	 * @return boolean
+	 *
+	 * @suppress PhanUndeclaredClassMethod
+	 */
+	protected function canDeleteShareFromSelf(\OCP\Share\IShare $share): bool {
+		if ($share->getShareType() !== Share::SHARE_TYPE_GROUP &&
+			$share->getShareType() !== Share::SHARE_TYPE_ROOM
+		) {
+			return false;
+		}
+
+		if ($share->getShareOwner() === $this->currentUser ||
+			$share->getSharedBy() === $this->currentUser
+		) {
+			// Delete the whole share, not just for self
+			return false;
+		}
+
+		// If in the recipient group, you can delete the share from self
+		if ($share->getShareType() === Share::SHARE_TYPE_GROUP) {
+			$sharedWith = $this->groupManager->get($share->getSharedWith());
+			$user = $this->userManager->get($this->currentUser);
+			if ($user !== null && $sharedWith !== null && $sharedWith->inGroup($user)) {
+				return true;
+			}
+		}
+
+		if ($share->getShareType() === Share::SHARE_TYPE_ROOM) {
+			try {
+				return $this->getRoomShareHelper()->canAccessShare($share, $this->currentUser);
+			} catch (QueryException $e) {
+				return false;
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * Make sure that the passed date is valid ISO 8601
 	 * So YYYY-MM-DD
@@ -1228,4 +1307,5 @@ private function shareProviderResharingRights(string $userId, IShare $share, $no
 
 		return false;
 	}
+
 }
diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -213,19 +213,20 @@ public function testDeleteShare() {
 
 	/**
 	 * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException
-	 * @expectedExceptionMessage could not delete share
+	 * @expectedExceptionMessage Could not delete share
 	 */
 	public function testDeleteShareLocked() {
 		$node = $this->getMockBuilder(File::class)->getMock();
 
 		$share = $this->newShare();
-		$share->setSharedBy($this->currentUser)
-			->setNode($node);
+		$share->setNode($node);
+
 		$this->shareManager
 			->expects($this->once())
 			->method('getShareById')
 			->with('ocinternal:42')
 			->willReturn($share);
+
 		$this->shareManager
 			->expects($this->never())
 			->method('deleteShare')
@@ -235,6 +236,205 @@ public function testDeleteShareLocked() {
 			->method('lock')
 			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED)
 			->will($this->throwException(new LockedException('mypath')));
+
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteFromSelf', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
+
+		$this->ocs->deleteShare(42);
+	}
+
+	/**
+	 * You can always remove a share that was shared with you 
+	 */
+	public function testDeleteShareWithMe() {
+		$node = $this->getMockBuilder(File::class)->getMock();
+
+		$share = $this->newShare();
+		$share->setSharedWith($this->currentUser)
+			->setShareType(\OCP\Share::SHARE_TYPE_USER)
+			->setNode($node);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('deleteShare')
+			->with($share);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteFromSelf', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
+
+		$this->ocs->deleteShare(42);
+	}
+
+	/**
+	 * You can always delete a share you own
+	 */
+	public function testDeleteShareOwner() {
+		$node = $this->getMockBuilder(File::class)->getMock();
+
+		$share = $this->newShare();
+		$share->setSharedBy($this->currentUser)
+			->setNode($node);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('deleteShare')
+			->with($share);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteFromSelf', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
+
+		$this->ocs->deleteShare(42);
+	}
+
+	/**
+	 * You can always delete a share when you own
+	 * the file path it belong to
+	 */
+	public function testDeleteShareFileOwner() {
+		$node = $this->getMockBuilder(File::class)->getMock();
+
+		$share = $this->newShare();
+		$share->setShareOwner($this->currentUser)
+			->setNode($node);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('deleteShare')
+			->with($share);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteFromSelf', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
+
+		$this->ocs->deleteShare(42);
+	}
+
+	/**
+	 * You can remove (the mountpoint, not the share)
+	 * a share if you're in the group the share is shared with
+	 */
+	public function testDeleteSharedWithMyGroup() {
+		$node = $this->getMockBuilder(File::class)->getMock();
+
+		$share = $this->newShare();
+		$share->setShareType(\OCP\Share::SHARE_TYPE_GROUP)
+			->setSharedWith('group')
+			->setNode($node);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+
+		// canDeleteShareFromSelf
+		$user = $this->createMock(IUser::class);
+		$group = $this->getMockBuilder('OCP\IGroup')->getMock();
+		$this->groupManager
+			->method('get')
+			->with('group')
+			->willReturn($group);
+		$this->userManager
+			->method('get')
+			->with($this->currentUser)
+			->willReturn($user);
+		$group->method('inGroup')
+			->with($user)
+			->willReturn(true);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->shareManager->expects($this->once())
+			->method('deleteFromSelf')
+			->with($share, $this->currentUser);
+
+		$this->shareManager->expects($this->never())
+			->method('deleteShare');
+
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canDeleteShareFromSelf', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
+
+		$this->ocs->deleteShare(42);
+	}
+
+	/**
+	 * You cannot remove a share if you're not
+	 * in the group the share is shared with
+	 * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException
+	 * @expectedExceptionMessage Wrong share ID, share doesn't exist
+	 */
+	public function testDeleteSharedWithGroupIDontBelongTo() {
+		$node = $this->getMockBuilder(File::class)->getMock();
+
+		$share = $this->newShare();
+		$share->setShareType(\OCP\Share::SHARE_TYPE_GROUP)
+			->setSharedWith('group')
+			->setNode($node);
+
+		$this->shareManager
+			->expects($this->once())
+			->method('getShareById')
+			->with('ocinternal:42')
+			->willReturn($share);
+
+		// canDeleteShareFromSelf
+		$user = $this->createMock(IUser::class);
+		$group = $this->getMockBuilder('OCP\IGroup')->getMock();
+		$this->groupManager
+			->method('get')
+			->with('group')
+			->willReturn($group);
+		$this->userManager
+			->method('get')
+			->with($this->currentUser)
+			->willReturn($user);
+		$group->method('inGroup')
+			->with($user)
+			->willReturn(false);
+
+		$node->expects($this->once())
+			->method('lock')
+			->with(\OCP\Lock\ILockingProvider::LOCK_SHARED);
+
+		$this->shareManager->expects($this->never())
+			->method('deleteFromSelf');
+
+		$this->shareManager->expects($this->never())
+			->method('deleteShare');
+
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteShareFromSelf', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canDeleteShare', [$share]));
 
 		$this->ocs->deleteShare(42);
 	}