Skip to content

Commit

Permalink
Merge pull request #16211 from nextcloud/backport/16186/stable16
Browse files Browse the repository at this point in the history 
[stable16] Better check reshare permissions part2
  • Loading branch information
@MorrisJobke
MorrisJobke committed Jul 3, 2019
2 parents 88114a4 + b4a38cc commit 79d86aa
Show file tree
Hide file tree
Showing 5 changed files with 115 additions and 189 deletions.
34 changes: 3 additions & 31 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Expand Up @@ -971,41 +971,13 @@ public function updateShare(
}
$share->setExpirationDate($expireDate);
}

}

if ($permissions !== null && $share->getShareOwner() !== $this->currentUser) {

// Get the root mount point for the user and check the share permissions there
$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
$userNodes = $userFolder->getById($share->getNodeId());
$userNode = array_shift($userNodes);

$userMountPointId = $userNode->getMountPoint()->getStorageRootId();
$userMountPoints = $userFolder->getById($userMountPointId);
$userMountPoint = array_shift($userMountPoints);

/* Check if this is an incoming share */
$incomingShares = $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
$incomingShares = array_merge($incomingShares, $this->shareManager->getSharedWith($this->currentUser, Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0));

/** @var \OCP\Share\IShare[] $incomingShares */
if (!empty($incomingShares)) {
$maxPermissions = 0;
foreach ($incomingShares as $incomingShare) {
$maxPermissions |= $incomingShare->getPermissions();
}

if ($share->getPermissions() & ~$maxPermissions) {
throw new OCSNotFoundException($this->l->t('Cannot increase permissions'));
}
}
}


try {
$share = $this->shareManager->updateShare($share);
} catch (GenericShareException $e) {
$code = $e->getCode() === 0 ? 403 : $e->getCode();
throw new OCSException($e->getHint(), $code);
} catch (\Exception $e) {
throw new OCSBadRequestException($e->getMessage(), $e);
}
Expand Down
155 changes: 8 additions & 147 deletions apps/files_sharing/tests/Controller/ShareAPIControllerTest.php
View file
Expand Up @@ -28,6 +28,7 @@

use OCP\App\IAppManager;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCS\OCSException;
use OCP\AppFramework\OCS\OCSNotFoundException;
use OCP\Files\File;
use OCP\Files\Folder;
Expand All @@ -45,6 +46,7 @@
use OCP\IUser;
use OCP\Files\IRootFolder;
use OCP\Lock\LockedException;
use OCP\Share\Exceptions\GenericShareException;
use OCP\Share\IManager;
use OCP\Share;
use Test\TestCase;
Expand Down Expand Up @@ -2418,157 +2420,16 @@ public function testUpdateShareCannotIncreasePermissions() {
$mountPoint->method('getStorageRootId')
->willReturn(42);

$this->shareManager->expects($this->never())->method('updateShare');

try {
$ocs->updateShare(42, 31);
$this->fail();
} catch (OCSNotFoundException $e) {
$this->assertEquals('Cannot increase permissions', $e->getMessage());
}
}

public function testUpdateShareCannotIncreasePermissionsLinkShare() {
$ocs = $this->mockFormatShare();

$folder = $this->createMock(Folder::class);
$folder->method('getId')
->willReturn(42);

$share = \OC::$server->getShareManager()->newShare();
$share
->setId(42)
->setSharedBy($this->currentUser)
->setShareOwner('anotheruser')
->setShareType(\OCP\Share::SHARE_TYPE_LINK)
->setPermissions(\OCP\Constants::PERMISSION_READ)
->setNode($folder);

// note: updateShare will modify the received instance but getSharedWith will reread from the database,
// so their values will be different
$incomingShare = \OC::$server->getShareManager()->newShare();
$incomingShare
->setId(42)
->setSharedBy($this->currentUser)
->setShareOwner('anotheruser')
->setShareType(\OCP\Share::SHARE_TYPE_USER)
->setSharedWith('currentUser')
->setPermissions(\OCP\Constants::PERMISSION_READ)
->setNode($folder);

$this->shareManager->method('getShareById')->with('ocinternal:42')->willReturn($share);

$this->shareManager->expects($this->any())
->method('getSharedWith')
->will($this->returnValueMap([
['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, [$incomingShare]],
['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, []]
]));

$userFolder = $this->createMock(Folder::class);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);

$userFolder->method('getById')
->with(42)
->willReturn([$folder]);

$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
->willReturn($mountPoint);
$mountPoint->method('getStorageRootId')
->willReturn(42);

$this->shareManager->expects($this->never())->method('updateShare');
$this->shareManager->method('shareApiLinkAllowPublicUpload')->willReturn(true);

try {
$ocs->updateShare(42, null, null, null, 'true');
$this->fail();
} catch (OCSNotFoundException $e) {
$this->assertEquals('Cannot increase permissions', $e->getMessage());
}
}

public function testUpdateShareCannotIncreasePermissionsRoomShare() {
$ocs = $this->mockFormatShare();

$folder = $this->createMock(Folder::class);
$folder->method('getId')
->willReturn(42);

$share = \OC::$server->getShareManager()->newShare();
$share
->setId(42)
->setSharedBy($this->currentUser)
->setShareOwner('anotheruser')
->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
->setSharedWith('group1')
->setPermissions(\OCP\Constants::PERMISSION_READ)
->setNode($folder);

// note: updateShare will modify the received instance but getSharedWith will reread from the database,
// so their values will be different
$incomingShare = \OC::$server->getShareManager()->newShare();
$incomingShare
->setId(42)
->setSharedBy($this->currentUser)
->setShareOwner('anotheruser')
->setShareType(\OCP\Share::SHARE_TYPE_ROOM)
->setSharedWith('group1')
->setPermissions(\OCP\Constants::PERMISSION_READ)
->setNode($folder);

$this->request
->method('getParam')
->will($this->returnValueMap([
['permissions', null, '31'],
]));

$this->shareManager
->method('getShareById')
->will($this->returnCallback(
function ($id) use ($share) {
if ($id !== 'ocRoomShare:42') {
throw new \OCP\Share\Exceptions\ShareNotFound();
}

return $share;
}
));

$userFolder = $this->createMock(Folder::class);
$this->rootFolder->method('getUserFolder')
->with($this->currentUser)
->willReturn($userFolder);

$userFolder->method('getById')
->with(42)
->willReturn([$folder]);

$mountPoint = $this->createMock(IMountPoint::class);
$folder->method('getMountPoint')
->willReturn($mountPoint);
$mountPoint->method('getStorageRootId')
->willReturn(42);

$this->shareManager->expects($this->any())
->method('getSharedWith')
->will($this->returnValueMap([
['currentUser', \OCP\Share::SHARE_TYPE_USER, $share->getNode(), -1, 0, []],
['currentUser', \OCP\Share::SHARE_TYPE_GROUP, $share->getNode(), -1, 0, []],
['currentUser', \OCP\Share::SHARE_TYPE_ROOM, $share->getNode(), -1, 0, [$incomingShare]]
]));

$this->shareManager->expects($this->never())->method('updateShare');
$this->shareManager->expects($this->once())
->method('updateShare')
->with($share)
->willThrowException(new GenericShareException('Can’t increase permissions of path/file', 'Can’t increase permissions of path/file', 404));

try {
$ocs->updateShare(42, 31);
$this->fail();
} catch (OCSNotFoundException $e) {
$this->assertEquals('Cannot increase permissions', $e->getMessage());
} catch (OCSException $e) {
$this->assertEquals('Can’t increase permissions of path/file', $e->getMessage());
}
}

Expand Down
60 changes: 60 additions & 0 deletions build/integration/features/sharing-v1-part2.feature
View file
Expand Up @@ -251,6 +251,66 @@ Feature: sharing
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with additional delete permissions
As an "admin"
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And As an "user0"
And creating a share with
| path | /PARENT |
| shareType | 0 |
| shareWith | user1 |
| permissions | 16 |
And As an "user1"
When creating a share with
| path | /PARENT (2) |
| shareType | 0 |
| shareWith | user2 |
| permissions | 25 |
Then the OCS status code should be "404"
And the HTTP status code should be "200"

Scenario: User is not allowed to reshare file with additional delete permissions for files
As an "admin"
Given user "user0" exists
And user "user1" exists
And user "user2" exists
And As an "user0"
And creating a share with
| path | /textfile0.txt |
| shareType | 0 |
| shareWith | user1 |
| permissions | 16 |
And As an "user1"
When creating a share with
| path | /textfile0 (2).txt |
| shareType | 0 |
| shareWith | user2 |
| permissions | 25 |
Then the OCS status code should be "100"
And the HTTP status code should be "200"
When Getting info of last share
Then Share fields of last share match with
| id | A_NUMBER |
| item_type | file |
| item_source | A_NUMBER |
| share_type | 0 |
| share_with | user2 |
| file_source | A_NUMBER |
| file_target | /textfile0 (2).txt |
| path | /textfile0 (2).txt |
| permissions | 17 |
| stime | A_NUMBER |
| storage | A_NUMBER |
| mail_send | 0 |
| uid_owner | user1 |
| storage_id | shared::/textfile0 (2).txt |
| file_parent | A_NUMBER |
| share_with_displayname | user2 |
| displayname_owner | user1 |
| mimetype | text/plain |

Scenario: Get a share with a user which didn't received the share
Given user "user0" exists
And user "user1" exists
Expand Down
44 changes: 33 additions & 11 deletions lib/private/Share20/Manager.php
View file
Expand Up @@ -269,11 +269,13 @@ protected function generalCreateChecks(\OCP\Share\IShare $share) {

// And you can't share your rootfolder
if ($this->userManager->userExists($share->getSharedBy())) {
$sharedPath = $this->rootFolder->getUserFolder($share->getSharedBy())->getPath();
$userFolder = $this->rootFolder->getUserFolder($share->getSharedBy());
$userFolderPath = $userFolder->getPath();
} else {
$sharedPath = $this->rootFolder->getUserFolder($share->getShareOwner())->getPath();
$userFolder = $this->rootFolder->getUserFolder($share->getShareOwner());
$userFolderPath = $userFolder->getPath();
}
if ($sharedPath === $share->getNode()->getPath()) {
if ($userFolderPath === $share->getNode()->getPath()) {
throw new \InvalidArgumentException('You can’t share your root folder');
}

Expand All @@ -288,15 +290,35 @@ protected function generalCreateChecks(\OCP\Share\IShare $share) {
throw new \InvalidArgumentException('A share requires permissions');
}

/*
* Quick fix for #23536
* Non moveable mount points do not have update and delete permissions
* while we 'most likely' do have that on the storage.
*/
$permissions = $share->getNode()->getPermissions();
$mount = $share->getNode()->getMountPoint();
if (!($mount instanceof MoveableMount)) {
$permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE;
if ($share->getNode()->getOwner()->getUID() !== $share->getSharedBy()) {
// When it's a reshare use the parent share permissions as maximum
$userMountPointId = $mount->getStorageRootId();
$userMountPoints = $userFolder->getById($userMountPointId);
$userMountPoint = array_shift($userMountPoints);

/* Check if this is an incoming share */
$incomingShares = $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_USER, $userMountPoint, -1, 0);
$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_GROUP, $userMountPoint, -1, 0));
$incomingShares = array_merge($incomingShares, $this->getSharedWith($share->getSharedBy(), Share::SHARE_TYPE_ROOM, $userMountPoint, -1, 0));

/** @var \OCP\Share\IShare[] $incomingShares */
if (!empty($incomingShares)) {
$permissions = 0;
foreach ($incomingShares as $incomingShare) {
$permissions |= $incomingShare->getPermissions();
}
}
} else {
/*
* Quick fix for #23536
* Non moveable mount points do not have update and delete permissions
* while we 'most likely' do have that on the storage.
*/
$permissions = $share->getNode()->getPermissions();
if (!($mount instanceof MoveableMount)) {
$permissions |= \OCP\Constants::PERMISSION_DELETE | \OCP\Constants::PERMISSION_UPDATE;
}
}

// Check that we do not share with more permissions than we have
Expand Down
11 changes: 11 additions & 0 deletions tests/lib/Share20/ManagerTest.php
View file
Expand Up @@ -546,6 +546,9 @@ public function dataGeneralChecks() {
$user0 = 'user0';
$user2 = 'user1';
$group0 = 'group0';
$owner = $this->createMock(IUser::class);
$owner->method('getUID')
->willReturn($user0);

$file = $this->createMock(File::class);
$node = $this->createMock(Node::class);
Expand Down Expand Up @@ -580,6 +583,8 @@ public function dataGeneralChecks() {
$nonShareAble = $this->createMock(Folder::class);
$nonShareAble->method('isShareable')->willReturn(false);
$nonShareAble->method('getPath')->willReturn('path');
$nonShareAble->method('getOwner')
->willReturn($owner);

$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $nonShareAble, $user2, $user0, $user0, 31, null, null), 'You are not allowed to share path', true];
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonShareAble, $group0, $user0, $user0, 31, null, null), 'You are not allowed to share path', true];
Expand All @@ -589,6 +594,8 @@ public function dataGeneralChecks() {
$limitedPermssions->method('isShareable')->willReturn(true);
$limitedPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
$limitedPermssions->method('getPath')->willReturn('path');
$limitedPermssions->method('getOwner')
->willReturn($owner);

$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $limitedPermssions, $user2, $user0, $user0, null, null, null), 'A share requires permissions', true];
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $limitedPermssions, $group0, $user0, $user0, null, null, null), 'A share requires permissions', true];
Expand All @@ -605,6 +612,8 @@ public function dataGeneralChecks() {
$nonMoveableMountPermssions->method('isShareable')->willReturn(true);
$nonMoveableMountPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_READ);
$nonMoveableMountPermssions->method('getPath')->willReturn('path');
$nonMoveableMountPermssions->method('getOwner')
->willReturn($owner);

$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $nonMoveableMountPermssions, $user2, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false];
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $nonMoveableMountPermssions, $group0, $user0, $user0, 11, null, null), 'Can’t increase permissions of path', false];
Expand All @@ -621,6 +630,8 @@ public function dataGeneralChecks() {
$allPermssions = $this->createMock(Folder::class);
$allPermssions->method('isShareable')->willReturn(true);
$allPermssions->method('getPermissions')->willReturn(\OCP\Constants::PERMISSION_ALL);
$allPermssions->method('getOwner')
->willReturn($owner);

$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $allPermssions, $user2, $user0, $user0, 30, null, null), 'Shares need at least read permissions', true];
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $allPermssions, $group0, $user0, $user0, 2, null, null), 'Shares need at least read permissions', true];
Expand Down

0 comments on commit 79d86aa

Please sign in to comment.