diff --git a/apps/files_sharing/lib/API/Share20OCS.php b/apps/files_sharing/lib/API/Share20OCS.php
index 53b27aae0b8fa4c2a0cb8236ae7bbcb69ebb78d8..436b8d15ac8f909102cd1f84ec8cd574d34cf64f 100644
--- a/apps/files_sharing/lib/API/Share20OCS.php
+++ b/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) {
diff --git a/apps/files_sharing/tests/API/Share20OCSTest.php b/apps/files_sharing/tests/API/Share20OCSTest.php
index b760a0f47a0148f281fdb21e1e3ac4b0c0920930..6435c992f25c1148442a63ef870c5c9d6d9fd783 100644
--- a/apps/files_sharing/tests/API/Share20OCSTest.php
+++ b/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;
})
diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php
index 058b0c4758c46d96c14796ce4101468a53c5ae9d..40c9085353ceb795596695cf25b4e3992e2d9f1b 100644
--- a/apps/files_sharing/tests/ApiTest.php
+++ b/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);
diff --git a/build/integration/features/sharing-v1.feature b/build/integration/features/sharing-v1.feature
index 79dc1326f3c1eca437d4af222efd52506fc50b17..3878e741f60e2abbef3b4c456faf9047156b70ea 100644
--- a/build/integration/features/sharing-v1.feature
+++ b/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 |
diff --git a/core/js/sharedialoglinkshareview.js b/core/js/sharedialoglinkshareview.js
index 817c3408e7e98cf9e99da77859404cc38e9f84a0..59f7ffcae036b5d48e6a3f7cebe6e2ec6f64879b 100644
--- a/core/js/sharedialoglinkshareview.js
+++ b/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({
diff --git a/lib/private/Repair/RepairInvalidShares.php b/lib/private/Repair/RepairInvalidShares.php
index 30f67a1f39460f93e6792aa3bdba48fe69a1c1fb..728632486d0137344a715e4697038e1f7cc711d4 100644
--- a/lib/private/Repair/RepairInvalidShares.php
+++ b/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);
}
diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
index e2730f4d5fb0e0e4fa6d11521f803668dfe24fc5..2c08e616f82a9874ee2a465b399722dc3f94bdf6 100644
--- a/lib/private/Share20/Manager.php
+++ b/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');
}
}
diff --git a/tests/lib/Repair/RepairInvalidSharesTest.php b/tests/lib/Repair/RepairInvalidSharesTest.php
index a1e871bcc80c00312a02e8e19e01d72e2e1a2bd8..1ac42e53bf6ab71b2e46c47583619535284df0e3 100644
--- a/tests/lib/Repair/RepairInvalidSharesTest.php
+++ b/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
*/
diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php
index a50ea6c892ad40fe479bb8130fc4905f7ea2e5fa..d2539f8577cbb7ec711d91987a9b92cde3556c48 100644
--- a/tests/lib/Share20/ManagerTest.php
+++ b/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
diff --git a/version.php b/version.php
index 3015d976e5301e56d34c92cadfc3c68e45805e22..b439ffbbdad85c9ade21718e3eaf6134ce060867 100644
--- a/version.php
+++ b/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';