diff --git a/apps/files_sharing/lib/API/Share20OCS.php b/apps/files_sharing/lib/API/Share20OCS.php index 53b27aae0b..436b8d15ac 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 b760a0f47a..6435c992f2 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 058b0c4758..40c9085353 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 79dc1326f3..3878e741f6 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 817c3408e7..59f7ffcae0 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/Share20/Manager.php b/lib/private/Share20/Manager.php index e2730f4d5f..2c08e616f8 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/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index a50ea6c892..d2539f8577 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