Fix shares read permissions

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
nextcloud · Aug 16, 2019 · ec56209 · ec56209
1 parent 6db3558
commit ec56209
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 50 deletions.
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -304,16 +304,12 @@ public function getShare(string $id): DataResponse {
 			throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
 
-		if ($this->canAccessShare($share)) {
-			try {
-				$share = $this->formatShare($share);
-				return new DataResponse([$share]);
-			} catch (NotFoundException $e) {
-				//Fall trough
-			}
+		try {
+			$share = $this->formatShare($share);
+			return new DataResponse([$share]);
+		} catch (NotFoundException $e) {
+			throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
-
-		throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
 	}
 
 	/**
@@ -338,7 +334,7 @@ public function deleteShare(string $id): DataResponse {
 			throw new OCSNotFoundException($this->l->t('could not delete share'));
 		}
 
-		if (!$this->canAccessShare($share)) {
+		if (!$this->canEditShare($share)) {
 			throw new OCSNotFoundException($this->l->t('Could not delete share'));
 		}
 
@@ -597,7 +593,7 @@ private function getSharedWithMe($node = null, bool $includeTags): DataResponse
 
 		$formatted = [];
 		foreach ($shares as $share) {
-			if ($this->canAccessShare($share)) {
+			if ($this->canEditShare($share)) {
 				try {
 					$formatted[] = $this->formatShare($share);
 				} catch (NotFoundException $e) {
@@ -824,7 +820,7 @@ public function updateShare(
 
 		$this->lock($share->getNode());
 
-		if (!$this->canAccessShare($share, false)) {
+		if (!$this->canEditShare($share, false)) {
 			throw new OCSNotFoundException($this->l->t('Wrong share ID, share doesn\'t exist'));
 		}
 
@@ -988,7 +984,7 @@ public function updateShare(
 	/**
 	 * @suppress PhanUndeclaredClassMethod
 	 */
-	protected function canAccessShare(\OCP\Share\IShare $share, bool $checkGroups = true): bool {
+	protected function canEditShare(\OCP\Share\IShare $share, bool $checkGroups = true): bool {
 		// A file with permissions 0 can't be accessed by us. So Don't show it
 		if ($share->getPermissions() === 0) {
 			return false;
@@ -1023,7 +1019,7 @@ protected function canAccessShare(\OCP\Share\IShare $share, bool $checkGroups =
 
 		if ($share->getShareType() === Share::SHARE_TYPE_ROOM) {
 			try {
-				return $this->getRoomShareHelper()->canAccessShare($share, $this->currentUser);
+				return $this->getRoomShareHelper()->canEditShare($share, $this->currentUser);
 			} catch (QueryException $e) {
 				return false;
 			}

diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
@@ -483,11 +483,11 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) {
 					$this->config,
 					$this->appManager,
 					$this->serverContainer
-				])->setMethods(['canAccessShare'])
+				])->setMethods(['canEditShare'])
 				->getMock();
 
 		$ocs->expects($this->any())
-			->method('canAccessShare')
+			->method('canEditShare')
 			->willReturn(true);
 
 		$this->shareManager
@@ -542,43 +542,24 @@ public function testGetShare(\OCP\Share\IShare $share, array $result) {
 		$this->assertEquals($result, $ocs->getShare($share->getId())->getData()[0]);
 	}
 
-	/**
-	 * @expectedException \OCP\AppFramework\OCS\OCSNotFoundException
-	 * @expectedExceptionMessage Wrong share ID, share doesn't exist
-	 */
-	public function testGetShareInvalidNode() {
-		$share = \OC::$server->getShareManager()->newShare();
-		$share->setSharedBy('initiator')
-			->setSharedWith('recipient')
-			->setShareOwner('owner');
-
-		$this->shareManager
-			->expects($this->once())
-			->method('getShareById')
-			->with('ocinternal:42', 'currentUser')
-			->willReturn($share);
-
-		$this->ocs->getShare(42);
-	}
-
-	public function testCanAccessShare() {
+	public function testcanEditShare() {
 		$share = $this->getMockBuilder(IShare::class)->getMock();
 		$share->method('getShareOwner')->willReturn($this->currentUser);
-		$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->getMockBuilder(IShare::class)->getMock();
 		$share->method('getSharedBy')->willReturn($this->currentUser);
-		$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->getMockBuilder(IShare::class)->getMock();
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER);
 		$share->method('getSharedWith')->willReturn($this->currentUser);
-		$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->getMockBuilder(IShare::class)->getMock();
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_USER);
 		$share->method('getSharedWith')->willReturn($this->getMockBuilder(IUser::class)->getMock());
-		$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->getMockBuilder(IShare::class)->getMock();
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP);
@@ -599,22 +580,22 @@ public function testCanAccessShare() {
 			['group2', $group2],
 			['groupnull', null],
 		]));
-		$this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertTrue($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->createMock(IShare::class);
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP);
 		$share->method('getSharedWith')->willReturn('group2');
-		$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		// null group
 		$share = $this->createMock(IShare::class);
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP);
 		$share->method('getSharedWith')->willReturn('groupnull');
-		$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 
 		$share = $this->createMock(IShare::class);
 		$share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_LINK);
-		$this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertFalse($this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 	}
 
 	public function dataCanAccessRoomShare() {
@@ -649,9 +630,9 @@ public function dataCanAccessRoomShare() {
 	 * @param bool $expects
 	 * @param \OCP\Share\IShare $share
 	 * @param bool helperAvailable
-	 * @param bool canAccessShareByHelper
+	 * @param bool canEditShareByHelper
 	 */
-	public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share, bool $helperAvailable, bool $canAccessShareByHelper) {
+	public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share, bool $helperAvailable, bool $canEditShareByHelper) {
 		if (!$helperAvailable) {
 			$this->appManager->method('isEnabledForUser')
 				->with('spreed')
@@ -662,18 +643,18 @@ public function testCanAccessRoomShare(bool $expected, \OCP\Share\IShare $share,
 				->willReturn(true);
 
 			$helper = $this->getMockBuilder('\OCA\Spreed\Share\Helper\ShareAPIController')
-				->setMethods(array('canAccessShare'))
+				->setMethods(array('canEditShare'))
 				->getMock();
-			$helper->method('canAccessShare')
+			$helper->method('canEditShare')
 				->with($share, $this->currentUser)
-				->willReturn($canAccessShareByHelper);
+				->willReturn($canEditShareByHelper);
 
 			$this->serverContainer->method('query')
 				->with('\OCA\Spreed\Share\Helper\ShareAPIController')
 				->willReturn($helper);
 		}
 
-		$this->assertEquals($expected, $this->invokePrivate($this->ocs, 'canAccessShare', [$share]));
+		$this->assertEquals($expected, $this->invokePrivate($this->ocs, 'canEditShare', [$share]));
 	}
 
 	/**