Unverified Commit 2ca13014 authored by Vincent Petry's avatar Vincent Petry
Browse files

Add explicit delete permission to link shares

Link shares always allowed deletion, however internally the permissions
were stored as 7 which lacked delete permissions. This created an
inconsistency in the Webdav permissions.

This fix makes sure we include delete permissions in the share
permissions, which now become 15.

In case a client is still passing 7 for legacy reasons, it gets
converted automatically to 15.
parent dc78d26f
...@@ -354,7 +354,8 @@ class Share20OCS { ...@@ -354,7 +354,8 @@ class Share20OCS {
$share->setPermissions( $share->setPermissions(
\OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_READ |
\OCP\Constants::PERMISSION_CREATE | \OCP\Constants::PERMISSION_CREATE |
\OCP\Constants::PERMISSION_UPDATE \OCP\Constants::PERMISSION_UPDATE |
\OCP\Constants::PERMISSION_DELETE
); );
} else { } else {
$share->setPermissions(\OCP\Constants::PERMISSION_READ); $share->setPermissions(\OCP\Constants::PERMISSION_READ);
...@@ -591,7 +592,7 @@ class Share20OCS { ...@@ -591,7 +592,7 @@ class Share20OCS {
$newPermissions = null; $newPermissions = null;
if ($publicUpload === 'true') { 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') { } else if ($publicUpload === 'false') {
$newPermissions = \OCP\Constants::PERMISSION_READ; $newPermissions = \OCP\Constants::PERMISSION_READ;
} }
...@@ -602,12 +603,21 @@ class Share20OCS { ...@@ -602,12 +603,21 @@ class Share20OCS {
if ($newPermissions !== null && if ($newPermissions !== null &&
$newPermissions !== \OCP\Constants::PERMISSION_READ && $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); $share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new \OC_OCS_Result(null, 400, $this->l->t('Can\'t change permissions for public share links')); 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()) { if (!$this->shareManager->shareApiLinkAllowPublicUpload()) {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED); $share->getNode()->unlock(ILockingProvider::LOCK_SHARED);
return new \OC_OCS_Result(null, 403, $this->l->t('Public upload disabled by the administrator')); return new \OC_OCS_Result(null, 403, $this->l->t('Public upload disabled by the administrator'));
...@@ -617,6 +627,9 @@ class Share20OCS { ...@@ -617,6 +627,9 @@ class Share20OCS {
$share->getNode()->unlock(ILockingProvider::LOCK_SHARED); $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')); 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) { if ($newPermissions !== null) {
......
...@@ -1035,7 +1035,7 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1035,7 +1035,7 @@ class Share20OCSTest extends \Test\TestCase {
$this->callback(function (\OCP\Share\IShare $share) use ($path) { $this->callback(function (\OCP\Share\IShare $share) use ($path) {
return $share->getNode() === $path && return $share->getNode() === $path &&
$share->getShareType() === \OCP\Share::SHARE_TYPE_LINK && $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->getSharedBy() === 'currentUser' &&
$share->getPassword() === null && $share->getPassword() === null &&
$share->getExpirationDate() === null; $share->getExpirationDate() === null;
...@@ -1366,7 +1366,7 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1366,7 +1366,7 @@ class Share20OCSTest extends \Test\TestCase {
$date = new \DateTime('2000-01-01'); $date = new \DateTime('2000-01-01');
$date->setTime(0,0,0); $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->getPassword() === 'password' &&
$share->getExpirationDate() == $date; $share->getExpirationDate() == $date;
}) })
...@@ -1379,6 +1379,44 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1379,6 +1379,44 @@ class Share20OCSTest extends \Test\TestCase {
$this->assertEquals($expected->getData(), $result->getData()); $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() { public function testUpdateLinkShareInvalidDate() {
$ocs = $this->mockFormatShare(); $ocs = $this->mockFormatShare();
...@@ -1408,7 +1446,30 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1408,7 +1446,30 @@ class Share20OCSTest extends \Test\TestCase {
$this->assertEquals($expected->getData(), $result->getData()); $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(); $ocs = $this->mockFormatShare();
$folder = $this->getMock('\OCP\Files\Folder'); $folder = $this->getMock('\OCP\Files\Folder');
...@@ -1421,11 +1482,7 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1421,11 +1482,7 @@ class Share20OCSTest extends \Test\TestCase {
$this->request $this->request
->method('getParam') ->method('getParam')
->will($this->returnValueMap([ ->will($this->returnValueMap($params));
['publicUpload', null, 'true'],
['expireDate', '', null],
['password', '', 'password'],
]));
$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share); $this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);
$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(false); $this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(false);
...@@ -1585,7 +1642,7 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1585,7 +1642,7 @@ class Share20OCSTest extends \Test\TestCase {
$this->shareManager->expects($this->once())->method('updateShare')->with( $this->shareManager->expects($this->once())->method('updateShare')->with(
$this->callback(function (\OCP\Share\IShare $share) use ($date) { $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->getPassword() === 'password' &&
$share->getExpirationDate() === $date; $share->getExpirationDate() === $date;
}) })
...@@ -1625,7 +1682,7 @@ class Share20OCSTest extends \Test\TestCase { ...@@ -1625,7 +1682,7 @@ class Share20OCSTest extends \Test\TestCase {
$this->shareManager->expects($this->once())->method('updateShare')->with( $this->shareManager->expects($this->once())->method('updateShare')->with(
$this->callback(function (\OCP\Share\IShare $share) use ($date) { $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->getPassword() === 'password' &&
$share->getExpirationDate() === $date; $share->getExpirationDate() === $date;
}) })
......
...@@ -257,7 +257,13 @@ class ApiTest extends TestCase { ...@@ -257,7 +257,13 @@ class ApiTest extends TestCase {
$this->assertTrue($result->succeeded()); $this->assertTrue($result->succeeded());
$data = $result->getData(); $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->assertEmpty($data['expiration']);
$this->assertTrue(is_string($data['token'])); $this->assertTrue(is_string($data['token']));
...@@ -1081,7 +1087,13 @@ class ApiTest extends TestCase { ...@@ -1081,7 +1087,13 @@ class ApiTest extends TestCase {
$this->assertTrue($result->succeeded()); $this->assertTrue($result->succeeded());
$share1 = $this->shareManager->getShareById($share1->getFullId()); $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 // cleanup
$this->shareManager->deleteShare($share1); $this->shareManager->deleteShare($share1);
......
...@@ -61,7 +61,7 @@ Feature: sharing ...@@ -61,7 +61,7 @@ Feature: sharing
And the HTTP status code should be "200" And the HTTP status code should be "200"
And Share fields of last share match with And Share fields of last share match with
| id | A_NUMBER | | id | A_NUMBER |
| permissions | 7 | | permissions | 15 |
| expiration | +3 days | | expiration | +3 days |
| url | AN_URL | | url | AN_URL |
| token | A_TOKEN | | token | A_TOKEN |
...@@ -159,7 +159,7 @@ Feature: sharing ...@@ -159,7 +159,7 @@ Feature: sharing
| share_type | 3 | | share_type | 3 |
| file_source | A_NUMBER | | file_source | A_NUMBER |
| file_target | /FOLDER | | file_target | /FOLDER |
| permissions | 7 | | permissions | 15 |
| stime | A_NUMBER | | stime | A_NUMBER |
| token | A_TOKEN | | token | A_TOKEN |
| storage | A_NUMBER | | storage | A_NUMBER |
...@@ -189,7 +189,7 @@ Feature: sharing ...@@ -189,7 +189,7 @@ Feature: sharing
| share_type | 3 | | share_type | 3 |
| file_source | A_NUMBER | | file_source | A_NUMBER |
| file_target | /FOLDER | | file_target | /FOLDER |
| permissions | 7 | | permissions | 15 |
| stime | A_NUMBER | | stime | A_NUMBER |
| token | A_TOKEN | | token | A_TOKEN |
| storage | A_NUMBER | | storage | A_NUMBER |
......
...@@ -202,7 +202,7 @@ ...@@ -202,7 +202,7 @@
var permissions = OC.PERMISSION_READ; var permissions = OC.PERMISSION_READ;
if($checkbox.is(':checked')) { 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({ this.model.saveLinkShare({
......
...@@ -446,14 +446,9 @@ class Manager implements IManager { ...@@ -446,14 +446,9 @@ class Manager implements IManager {
throw new \InvalidArgumentException('Link shares can\'t have reshare permissions'); 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 // Check if public upload is allowed
if (!$this->shareApiLinkAllowPublicUpload() && 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'); throw new \InvalidArgumentException('Public upload not allowed');
} }
} }
......
...@@ -1318,24 +1318,6 @@ class ManagerTest extends \Test\TestCase { ...@@ -1318,24 +1318,6 @@ class ManagerTest extends \Test\TestCase {
$this->invokePrivate($this->manager, 'linkCreateChecks', [$share]); $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 * @expectedException Exception
* @expectedExceptionMessage Public upload not allowed * @expectedExceptionMessage Public upload not allowed
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment