Skip to content

[stable15] Better check reshare permissions part2 #16215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 3 commits into from
Jul 3, 2019
Merged

[stable15] Better check reshare permissions part2 #16215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
029ecb7
Better check reshare permissions when creating a share
nickvergessen Jul 2, 2019
1329023
Unify the permission checking in one place only
nickvergessen Jul 3, 2019
f3ad463
Remove duplicate tests
nickvergessen Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 3 additions & 30 deletions apps/files_sharing/lib/Controller/ShareAPIController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,40 +951,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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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