Merge pull request #25250 from owncloud/linkshare-includedeletewithuploadperms

Add explicit delete permission to link shares

apps/files_sharing/lib/API/Share20OCS.php

@@ -354,7 +354,8 @@ class Share20OCS {
 				$share->setPermissions(
 					\OCP\Constants::PERMISSION_READ |
 					\OCP\Constants::PERMISSION_CREATE |
-					\OCP\Constants::PERMISSION_UPDATE
+					\OCP\Constants::PERMISSION_UPDATE |
+					\OCP\Constants::PERMISSION_DELETE
 				);
 			} else {
 				$share->setPermissions(\OCP\Constants::PERMISSION_READ);
@@ -591,7 +592,7 @@ class Share20OCS {
 
 			$newPermissions = null;
 			if ($publicUpload === 'true') {
-				$newPermissions = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE;
+				$newPermissions = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
 			} else if ($publicUpload === 'false') {
 				$newPermissions = \OCP\Constants::PERMISSION_READ;
 			}
@@ -602,12 +603,21 @@ class Share20OCS {
 
 			if ($newPermissions !== null &&
 				$newPermissions !== \OCP\Constants::PERMISSION_READ &&
-				$newPermissions !== (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE)) {
+				// legacy
+				$newPermissions !== (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE) &&
+				// correct
+				$newPermissions !== (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE)
+			) {
 				$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 				return new \OC_OCS_Result(null, 400, $this->l->t('Can\'t change permissions for public share links'));
 			}
 
-			if ($newPermissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE)) {
+			if (
+				// legacy
+				$newPermissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE) ||
+				// correct
+				$newPermissions === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE)
+			) {
 				if (!$this->shareManager->shareApiLinkAllowPublicUpload()) {
 					$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 					return new \OC_OCS_Result(null, 403, $this->l->t('Public upload disabled by the administrator'));
@@ -617,6 +627,9 @@ class Share20OCS {
 					$share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
 					return new \OC_OCS_Result(null, 400, $this->l->t('Public upload is only possible for publicly shared folders'));
 				}
+
+				// normalize to correct public upload permissions
+				$newPermissions = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
 			}
 
 			if ($newPermissions !== null) {

apps/files_sharing/tests/API/Share20OCSTest.php

@@ -1035,7 +1035,7 @@ class Share20OCSTest extends \Test\TestCase {
 			$this->callback(function (\OCP\Share\IShare $share) use ($path) {
 				return $share->getNode() === $path &&
 					$share->getShareType() === \OCP\Share::SHARE_TYPE_LINK &&
-					$share->getPermissions() === \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE &&
+					$share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) &&
 					$share->getSharedBy() === 'currentUser' &&
 					$share->getPassword() === null &&
 					$share->getExpirationDate() === null;
@@ -1366,7 +1366,7 @@ class Share20OCSTest extends \Test\TestCase {
 				$date = new \DateTime('2000-01-01');
 				$date->setTime(0,0,0);
 
-				return $share->getPermissions() === \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE && \OCP\Constants::PERMISSION_DELETE &&
+				return $share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) &&
 				$share->getPassword() === 'password' &&
 				$share->getExpirationDate() == $date;
 			})
@@ -1379,6 +1379,44 @@ class Share20OCSTest extends \Test\TestCase {
 		$this->assertEquals($expected->getData(), $result->getData());
 	}
 
+	/**
+	 * @dataProvider publicUploadParamsProvider
+	 */
+	public function testUpdateLinkShareEnablePublicUpload($params) {
+		$ocs = $this->mockFormatShare();
+
+		$folder = $this->getMock('\OCP\Files\Folder');
+
+		$share = \OC::$server->getShareManager()->newShare();
+		$share->setPermissions(\OCP\Constants::PERMISSION_ALL)
+			->setSharedBy($this->currentUser->getUID())
+			->setShareType(\OCP\Share::SHARE_TYPE_LINK)
+			->setPassword('password')
+			->setNode($folder);
+
+		$this->request
+			->method('getParam')
+			->will($this->returnValueMap($params));
+
+		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
+		$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true);
+		$this->shareManager->method('getSharedWith')->willReturn([]);
+
+		$this->shareManager->expects($this->once())->method('updateShare')->with(
+			$this->callback(function (\OCP\Share\IShare $share) {
+				return $share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) &&
+				$share->getPassword() === 'password' &&
+				$share->getExpirationDate() === null;
+			})
+		)->will($this->returnArgument(0));
+
+		$expected = new \OC_OCS_Result(null);
+		$result = $ocs->updateShare(42);
+
+		$this->assertEquals($expected->getMeta(), $result->getMeta());
+		$this->assertEquals($expected->getData(), $result->getData());
+	}
+
 	public function testUpdateLinkShareInvalidDate() {
 		$ocs = $this->mockFormatShare();
 
@@ -1408,7 +1446,30 @@ class Share20OCSTest extends \Test\TestCase {
 		$this->assertEquals($expected->getData(), $result->getData());
 	}
 
-	public function testUpdateLinkSharePublicUploadNotAllowed() {
+	public function publicUploadParamsProvider() {
+		return [
+			[[
+				['publicUpload', null, 'true'],
+				['expireDate', '', null],
+				['password', '', 'password'],
+			]], [[
+				// legacy had no delete
+				['permissions', null, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE],
+				['expireDate', '', null],
+				['password', '', 'password'],
+			]], [[
+				// correct
+				['permissions', null, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE],
+				['expireDate', '', null],
+				['password', '', 'password'],
+			]],
+		];
+	}
+
+	/**
+	 * @dataProvider publicUploadParamsProvider
+	 */
+	public function testUpdateLinkSharePublicUploadNotAllowed($params) {
 		$ocs = $this->mockFormatShare();
 
 		$folder = $this->getMock('\OCP\Files\Folder');
@@ -1421,11 +1482,7 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$this->request
 			->method('getParam')
-			->will($this->returnValueMap([
-				['publicUpload', null, 'true'],
-				['expireDate', '', null],
-				['password', '', 'password'],
-			]));
+			->will($this->returnValueMap($params));
 
 		$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
 		$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(false);
@@ -1585,7 +1642,7 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$this->shareManager->expects($this->once())->method('updateShare')->with(
 			$this->callback(function (\OCP\Share\IShare $share) use ($date) {
-				return $share->getPermissions() === \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE &&
+				return $share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) &&
 				$share->getPassword() === 'password' &&
 				$share->getExpirationDate() === $date;
 			})
@@ -1625,7 +1682,7 @@ class Share20OCSTest extends \Test\TestCase {
 
 		$this->shareManager->expects($this->once())->method('updateShare')->with(
 			$this->callback(function (\OCP\Share\IShare $share) use ($date) {
-				return $share->getPermissions() === \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_DELETE &&
+				return $share->getPermissions() === (\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE) &&
 				$share->getPassword() === 'password' &&
 				$share->getExpirationDate() === $date;
 			})

apps/files_sharing/tests/ApiTest.php

@@ -257,7 +257,13 @@ class ApiTest extends TestCase {
 		$this->assertTrue($result->succeeded());
 
 		$data = $result->getData();
-		$this->assertEquals(7, $data['permissions']);
+		$this->assertEquals(
+			\OCP\Constants::PERMISSION_READ |
+			\OCP\Constants::PERMISSION_CREATE |
+			\OCP\Constants::PERMISSION_UPDATE |
+			\OCP\Constants::PERMISSION_DELETE,
+			$data['permissions']
+		);
 		$this->assertEmpty($data['expiration']);
 		$this->assertTrue(is_string($data['token']));
 
@@ -1081,7 +1087,13 @@ class ApiTest extends TestCase {
 		$this->assertTrue($result->succeeded());
 
 		$share1 = $this->shareManager->getShareById($share1->getFullId());
-		$this->assertEquals(7, $share1->getPermissions());
+		$this->assertEquals(
+			\OCP\Constants::PERMISSION_READ |
+			\OCP\Constants::PERMISSION_CREATE |
+			\OCP\Constants::PERMISSION_UPDATE |
+			\OCP\Constants::PERMISSION_DELETE,
+			$share1->getPermissions()
+		);
 
 		// cleanup
 		$this->shareManager->deleteShare($share1);

build/integration/features/sharing-v1.feature

@@ -61,7 +61,7 @@ Feature: sharing
     And the HTTP status code should be "200"
     And Share fields of last share match with
       | id | A_NUMBER |
-      | permissions | 7 |
+      | permissions | 15 |
       | expiration | +3 days |
       | url | AN_URL |
       | token | A_TOKEN |
@@ -159,7 +159,7 @@ Feature: sharing
       | share_type | 3 |
       | file_source | A_NUMBER |
       | file_target | /FOLDER |
-      | permissions | 7 |
+      | permissions | 15 |
       | stime | A_NUMBER |
       | token | A_TOKEN |
       | storage | A_NUMBER |
@@ -189,7 +189,7 @@ Feature: sharing
       | share_type | 3 |
       | file_source | A_NUMBER |
       | file_target | /FOLDER |
-      | permissions | 7 |
+      | permissions | 15 |
       | stime | A_NUMBER |
       | token | A_TOKEN |
       | storage | A_NUMBER |

core/js/sharedialoglinkshareview.js

@@ -202,7 +202,7 @@
 
 			var permissions = OC.PERMISSION_READ;
 			if($checkbox.is(':checked')) {
-				permissions = OC.PERMISSION_UPDATE | OC.PERMISSION_CREATE | OC.PERMISSION_READ;
+				permissions = OC.PERMISSION_UPDATE | OC.PERMISSION_CREATE | OC.PERMISSION_READ | OC.PERMISSION_DELETE;
 			}
 
 			this.model.saveLinkShare({

lib/private/Repair/RepairInvalidShares.php

@@ -71,6 +71,25 @@ class RepairInvalidShares implements IRepairStep {
 		}
 	}
 
+	/**
+	 * In the past link shares with public upload enabled were missing the delete permission.
+	 */
+	private function addShareLinkDeletePermission(IOutput $out) {
+		$oldPerms = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE;
+		$newPerms = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
+		$builder = $this->connection->getQueryBuilder();
+		$builder
+			->update('share')
+			->set('permissions', $builder->expr()->literal($newPerms))
+			->where($builder->expr()->eq('share_type', $builder->expr()->literal(\OC\Share\Constants::SHARE_TYPE_LINK)))
+			->andWhere($builder->expr()->eq('permissions', $builder->expr()->literal($oldPerms)));
+
+		$updatedEntries = $builder->execute();
+		if ($updatedEntries > 0) {
+			$out->info('Fixed link share permissions for ' . $updatedEntries . ' shares');
+		}
+	}
+
 	/**
 	 * Remove shares where the parent share does not exist anymore
 	 */
@@ -113,6 +132,10 @@ class RepairInvalidShares implements IRepairStep {
 			// this situation was only possible before 8.2
 			$this->removeExpirationDateFromNonLinkShares($out);
 		}
+		if (version_compare($ocVersionFromBeforeUpdate, '9.1.0.9', '<')) {
+			// this situation was only possible before 9.1
+			$this->addShareLinkDeletePermission($out);
+		}
 
 		$this->removeSharesNonExistingParent($out);
 	}

lib/private/Share20/Manager.php

@@ -446,14 +446,9 @@ class Manager implements IManager {
 			throw new \InvalidArgumentException('Link shares can\'t have reshare permissions');
 		}
 
-		// We don't allow deletion on link shares
-		if ($share->getPermissions() & \OCP\Constants::PERMISSION_DELETE) {
-			throw new \InvalidArgumentException('Link shares can\'t have delete permissions');
-		}
-
 		// Check if public upload is allowed
 		if (!$this->shareApiLinkAllowPublicUpload() &&
-			($share->getPermissions() & (\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE))) {
+			($share->getPermissions() & (\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE))) {
 			throw new \InvalidArgumentException('Public upload not allowed');
 		}
 	}

tests/lib/Repair/RepairInvalidSharesTest.php

@@ -123,6 +123,93 @@ class RepairInvalidSharesTest extends TestCase {
 		$this->assertNotNull($linkShare['expiration'], 'valid link share expiration date still there');
 	}
 
+	/**
+	 * Test remove expiration date for non-link shares
+	 */
+	public function testAddShareLinkDeletePermission() {
+		$oldPerms = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE;
+		$newPerms = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_DELETE;
+
+		// share with old permissions
+		$qb = $this->connection->getQueryBuilder();
+		$qb->insert('share')
+			->values([
+				'share_type' => $qb->expr()->literal(Constants::SHARE_TYPE_LINK),
+				'uid_owner' => $qb->expr()->literal('user1'),
+				'item_type' => $qb->expr()->literal('folder'),
+				'item_source' => $qb->expr()->literal(123),
+				'item_target' => $qb->expr()->literal('/123'),
+				'file_source' => $qb->expr()->literal(123),
+				'file_target' => $qb->expr()->literal('/test'),
+				'permissions' => $qb->expr()->literal($oldPerms),
+				'stime' => $qb->expr()->literal(time()),
+			])
+			->execute();
+
+		$bogusShareId = $this->getLastShareId();
+
+		// share with read-only permissions
+		$qb = $this->connection->getQueryBuilder();
+		$qb->insert('share')
+			->values([
+				'share_type' => $qb->expr()->literal(Constants::SHARE_TYPE_LINK),
+				'uid_owner' => $qb->expr()->literal('user1'),
+				'item_type' => $qb->expr()->literal('folder'),
+				'item_source' => $qb->expr()->literal(123),
+				'item_target' => $qb->expr()->literal('/123'),
+				'file_source' => $qb->expr()->literal(123),
+				'file_target' => $qb->expr()->literal('/test'),
+				'permissions' => $qb->expr()->literal(\OCP\Constants::PERMISSION_READ),
+				'stime' => $qb->expr()->literal(time()),
+			])
+			->execute();
+
+		$keepThisShareId = $this->getLastShareId();
+
+		// user share to keep
+		$qb = $this->connection->getQueryBuilder();
+		$qb->insert('share')
+			->values([
+				'share_type' => $qb->expr()->literal(Constants::SHARE_TYPE_USER),
+				'share_with' => $qb->expr()->literal('recipientuser1'),
+				'uid_owner' => $qb->expr()->literal('user1'),
+				'item_type' => $qb->expr()->literal('folder'),
+				'item_source' => $qb->expr()->literal(123),
+				'item_target' => $qb->expr()->literal('/123'),
+				'file_source' => $qb->expr()->literal(123),
+				'file_target' => $qb->expr()->literal('/test'),
+				'permissions' => $qb->expr()->literal(3),
+				'stime' => $qb->expr()->literal(time()),
+			])
+			->execute();
+
+		$keepThisShareId2 = $this->getLastShareId();
+
+		/** @var IOutput | \PHPUnit_Framework_MockObject_MockObject $outputMock */
+		$outputMock = $this->getMockBuilder('\OCP\Migration\IOutput')
+			->disableOriginalConstructor()
+			->getMock();
+
+		$this->repair->run($outputMock);
+
+		$results = $this->connection->getQueryBuilder()
+			->select('*')
+			->from('share')
+			->orderBy('permissions', 'ASC')
+			->execute()
+			->fetchAll();
+
+		$this->assertCount(3, $results);
+
+		$untouchedShare = $results[0];
+		$untouchedShare2 = $results[1];
+		$updatedShare = $results[2];
+		$this->assertEquals($keepThisShareId, $untouchedShare['id'], 'sanity check');
+		$this->assertEquals($keepThisShareId2, $untouchedShare2['id'], 'sanity check');
+		$this->assertEquals($bogusShareId, $updatedShare['id'], 'sanity check');
+		$this->assertEquals($newPerms, $updatedShare['permissions'], 'delete permission was added');
+	}
+
 	/**
 	 * Test remove shares where the parent share does not exist anymore
 	 */

tests/lib/Share20/ManagerTest.php

@@ -1318,24 +1318,6 @@ class ManagerTest extends \Test\TestCase {
 		$this->invokePrivate($this->manager, 'linkCreateChecks', [$share]);
 	}
 
-	/**
-	 * @expectedException Exception
-	 * @expectedExceptionMessage Link shares can't have delete permissions
-	 */
-	public function testLinkCreateChecksDeletePermissions() {
-		$share = $this->manager->newShare();
-
-		$share->setPermissions(\OCP\Constants::PERMISSION_DELETE);
-
-		$this->config
-			->method('getAppValue')
-			->will($this->returnValueMap([
-				['core', 'shareapi_allow_links', 'yes', 'yes'],
-			]));
-
-		$this->invokePrivate($this->manager, 'linkCreateChecks', [$share]);
-	}
-
 	/**
 	 * @expectedException Exception
 	 * @expectedExceptionMessage Public upload not allowed

version.php

@@ -25,7 +25,7 @@
 // We only can count up. The 4. digit is only for the internal patchlevel to trigger DB upgrades
 // between betas, final and RCs. This is _not_ the public version number. Reset minor/patchlevel
 // when updating major/minor version number.
-$OC_Version = array(9, 1, 0, 9);
+$OC_Version = array(9, 1, 0, 10);
 
 // The human readable string
 $OC_VersionString = '9.1.0 beta 2';