X Tutup
Skip to content

Commit dedde47

Browse files
mrow4amrow4a
committed
validate reshare attributes based on supershare
1 parent 59b3a71 commit dedde47

File tree

3 files changed

+273
-20
lines changed

3 files changed

+273
-20
lines changed

‎changelog/unreleased/36265‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
Change: Validate reshare permissions and attributes based on supershare
2+
3+
This PR uniformed a way reshare permissions and attributes are enforced in a
4+
reshare scenario - strict subset checking has been used in both cases.
5+
6+
https://github.com/owncloud/core/pull/36265

‎lib/private/Share20/Manager.php‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ protected function validatePermissions(IShare $share) {
311311
* and this check should be done in Share object's setPermission method
312312
*/
313313
if ($share->getPermissions() < 0 || $share->getPermissions() > \OCP\Constants::PERMISSION_ALL) {
314-
$message_t = $this->l->t('invalid permissionss');
314+
$message_t = $this->l->t('Invalid permissions');
315315
throw new GenericShareException($message_t, $message_t, 404);
316316
}
317317

@@ -323,6 +323,9 @@ protected function validatePermissions(IShare $share) {
323323
/* Use share node permission as default $maxPermissions */
324324
$maxPermissions = $shareNode->getPermissions();
325325

326+
/* Attributes default is null, as attributes are restricted only in reshare */
327+
$maxAttributes = null;
328+
326329
/*
327330
* Quick fix for #23536
328331
* Non moveable mount points do not have update and delete permissions
@@ -345,21 +348,28 @@ protected function validatePermissions(IShare $share) {
345348
if ($shareFileNode) {
346349
$shareFileStorage = $shareFileNode->getStorage();
347350
if ($shareFileStorage->instanceOfStorage('OCA\Files_Sharing\External\Storage')) {
348-
// if $shareFileNode is an incoming federated share, use node permission directly
351+
// if $shareFileNode is an incoming federated share, use share node permission directly
349352
$maxPermissions = $shareNode->getPermissions();
350353
} elseif ($shareFileStorage->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) {
351354
// if $shareFileNode is user/group share, use supershare permissions
352355
/** @var \OCA\Files_Sharing\SharedStorage $shareFileStorage */
353356
'@phan-var \OCA\Files_Sharing\SharedStorage $shareFileStorage';
354357
$parentShare = $shareFileStorage->getShare();
355358
$maxPermissions = $parentShare->getPermissions();
359+
$maxAttributes = $parentShare->getAttributes();
356360
}
357361
}
358362
}
359363

360364
/* Check that we do not share with more permissions than we have */
361365
if (!$this->strictSubsetOfPermissions($maxPermissions, $share->getPermissions())) {
362-
$message_t = $this->l->t('Cannot increase permissions of %s', [$share->getNode()->getPath()]);
366+
$message_t = $this->l->t('Cannot set the requested share permissions for %s', [$share->getNode()->getName()]);
367+
throw new GenericShareException($message_t, $message_t, 404);
368+
}
369+
370+
/* Check that we do not share with more attributes than we have */
371+
if ($maxAttributes !== null && !$this->strictSubsetOfAttributes($maxAttributes, $share->getAttributes())) {
372+
$message_t = $this->l->t('Cannot set the requested share attributes for %s', [$share->getNode()->getName()]);
363373
throw new GenericShareException($message_t, $message_t, 404);
364374
}
365375
}
@@ -1668,4 +1678,37 @@ private function isNewShare(IShare $share) {
16681678
private function strictSubsetOfPermissions($allowedPermissions, $newPermissions) {
16691679
return (($allowedPermissions | $newPermissions) === $allowedPermissions);
16701680
}
1681+
1682+
/**
1683+
* Check $newAttributes attribute is a subset of $allowedAttributes
1684+
*
1685+
* @param IAttributes $allowedAttributes
1686+
* @param IAttributes $newAttributes
1687+
* @return boolean ,true if $allowedAttributes enabled is super set of $newAttributes enabled, else false
1688+
*/
1689+
private function strictSubsetOfAttributes($allowedAttributes, $newAttributes) {
1690+
// if both are empty, it is strict subset
1691+
if ((!$allowedAttributes || empty($allowedAttributes->toArray()))
1692+
&& (!$newAttributes || empty($newAttributes->toArray()))) {
1693+
return true;
1694+
}
1695+
1696+
// make sure that number of attributes is the same
1697+
if (\count($allowedAttributes->toArray()) !== \count($newAttributes->toArray())) {
1698+
return false;
1699+
}
1700+
1701+
// if number of attributes is the same, make sure that attributes are
1702+
// existing in allowed set and disabled attribute is not being enabled
1703+
foreach ($newAttributes->toArray() as $newAttribute) {
1704+
$allowedEnabled = $allowedAttributes->getAttribute($newAttribute['scope'], $newAttribute['key']);
1705+
if (($newAttribute['enabled'] === true && $allowedEnabled === false)
1706+
|| ($newAttribute['enabled'] === null && $allowedEnabled !== null)
1707+
|| $allowedEnabled === null) {
1708+
return false;
1709+
}
1710+
}
1711+
1712+
return true;
1713+
}
16711714
}

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 221 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222

2323
use OC\Files\View;
2424
use OC\Share20\Manager;
25+
use OC\Share20\ShareAttributes;
2526
use OCP\Files\File;
2627
use OC\Share20\Share;
2728
use OCP\DB\QueryBuilder\IExpressionBuilder;
@@ -39,9 +40,11 @@
3940
use OCP\ILogger;
4041
use OCP\IUser;
4142
use OCP\IUserManager;
43+
use OCP\IUserSession;
4244
use OCP\Security\IHasher;
4345
use OCP\Security\ISecureRandom;
4446
use OCP\Share\Exceptions\ShareNotFound;
47+
use OCP\Share\IAttributes;
4548
use OCP\Share\IProviderFactory;
4649
use OCP\Share\IShare;
4750
use OCP\Share\IShareProvider;
@@ -88,6 +91,8 @@ class ManagerTest extends \Test\TestCase {
8891
protected $view;
8992
/** @var IDBConnection | \PHPUnit\Framework\MockObject\MockObject */
9093
protected $connection;
94+
/** @var IUserSession | \PHPUnit\Framework\MockObject\MockObject */
95+
protected $userSession;
9196

9297
public function setUp(): void {
9398
parent::setUp();
@@ -103,6 +108,7 @@ public function setUp(): void {
103108
$this->eventDispatcher = new EventDispatcher();
104109
$this->view = $this->createMock(View::class);
105110
$this->connection = $this->createMock(IDBConnection::class);
111+
$this->userSession = $this->createMock(IUserSession::class);
106112

107113
$this->l = $this->createMock('\OCP\IL10N');
108114
$this->l->method('t')
@@ -125,7 +131,8 @@ public function setUp(): void {
125131
$this->rootFolder,
126132
$this->eventDispatcher,
127133
$this->view,
128-
$this->connection
134+
$this->connection,
135+
$this->userSession
129136
);
130137

131138
$this->defaultProvider = $this->getMockBuilder('\OC\Share20\DefaultShareProvider')
@@ -822,26 +829,18 @@ public function dataGeneralChecks() {
822829
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $rootFolder, $group0, $user0, $user0, 2, null, null), 'You can\'t share your root folder', true];
823830
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $rootFolder, null, $user0, $user0, 16, null, null), 'You can\'t share your root folder', true];
824831

825-
/**
826-
* Basic share (not re-share) with enough permission inputs
827-
*/
828-
$ownerUser = $this->createMock(IUser::class);
829-
$ownerUser->method('getUID')->willReturn('user1');
830832
$fileFullPermission = $this->createMock(File::class);
831833
$fileFullPermission->method('getPermissions')->willReturn(31);
832-
$fileFullPermission->method('getOwner')->willReturn($ownerUser);
833834
$fileFullPermission->method('isShareable')->willReturn(true);
835+
834836
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, null, null, null), 'A share requires permissions', true];
835837

836-
/**
837-
* Normal share (not re-share) with not enough permission input
838-
*/
839-
$fileLessPermission = $this->createMock(File::class);
840-
$fileLessPermission->method('getPermissions')->willReturn(1);
841-
$fileLessPermission->method('getOwner')->willReturn($ownerUser);
842-
$fileLessPermission->method('isShareable')->willReturn(true);
843-
$fileLessPermission->method('getPath')->willReturn('/user1/sharedfile');
844-
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileLessPermission, $user0, $user1, $user1, 17, null, null), 'Cannot increase permissions of /user1/sharedfile', true];
838+
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, -1, null, null), 'Invalid permissions', true];
839+
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, 100, null, null), 'Invalid permissions', true];
840+
841+
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, 0, null, null), 'Cannot remove all permissions', true];
842+
843+
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, 31, null, null), null, false];
845844

846845
return $data;
847846
}
@@ -883,9 +882,118 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) {
883882
$this->assertSame($exception, $thrown);
884883
}
885884

885+
public function dataShareNotEnoughPermissions() {
886+
$file = $this->createMock(File::class);
887+
$file->method('getPermissions')->willReturn(17);
888+
$file->method('getName')->willReturn('sharedfile');
889+
$file->method('getPath')->willReturn('/user1/sharedfile');
890+
891+
// Normal share (not re-share) should just use share node permission
892+
// exception when trying to share with more permission than share has
893+
$share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'user1', 'user1', 31, null, null);
894+
$data[] = [$share, null, 'Cannot set the requested share permissions for sharedfile', true];
895+
896+
// Federated reshare should just use share node permission
897+
// exception when trying to share with more permission than node has
898+
$fileExternalStorage = $this->createMock('OCA\Files_Sharing\External\Storage');
899+
$fileExternalStorage->method('instanceOfStorage')
900+
->willReturnCallback(function ($storageClass) {
901+
return ($storageClass === 'OCA\Files_Sharing\External\Storage');
902+
});
903+
$fileExternal = $this->createMock(File::class);
904+
$fileExternal->method('getStorage')->willReturn($fileExternalStorage);
905+
$share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'user1', 'user2', 31, null, null);
906+
$data[] = [$share, $fileExternal, 'Cannot set the requested share permissions for sharedfile', true];
907+
908+
// Normal reshare should just use supershare node permission
909+
// exception when trying to share with more permission than supershare has
910+
$superShare = $this->createMock(IShare::class);
911+
$superShare->method('getPermissions')->willReturn(1);
912+
$fileReshareStorage = $this->createMock('OCA\Files_Sharing\SharedStorage');
913+
$fileReshareStorage->method('instanceOfStorage')
914+
->willReturnCallback(function ($storageClass) {
915+
return ($storageClass === 'OCA\Files_Sharing\SharedStorage');
916+
});
917+
$fileReshareStorage->method('getShare')->willReturn($superShare);
918+
$fileReshare = $this->createMock(File::class);
919+
$fileReshare->method('getStorage')->willReturn($fileReshareStorage);
920+
$share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'user1', 'user2', 17, null, null);
921+
$data[] = [$share, $fileReshare, 'Cannot set the requested share permissions for sharedfile', true];
922+
923+
// Normal reshare should just use supershare node attributes
924+
// exception when trying to share with more attributes than supershare has
925+
$superShareAttributes = new ShareAttributes();
926+
$shareAttributes = new ShareAttributes();
927+
$shareAttributes->setAttribute('test', 'test', true);
928+
$superShare = $this->createMock(IShare::class);
929+
$superShare->method('getPermissions')->willReturn(17);
930+
$superShare->method('getAttributes')->willReturn($superShareAttributes);
931+
$fileReshareStorage = $this->createMock('OCA\Files_Sharing\SharedStorage');
932+
$fileReshareStorage->method('instanceOfStorage')
933+
->willReturnCallback(function ($storageClass) {
934+
return ($storageClass === 'OCA\Files_Sharing\SharedStorage');
935+
});
936+
$fileReshareStorage->method('getShare')->willReturn($superShare);
937+
$fileReshare = $this->createMock(File::class);
938+
$fileReshare->method('getStorage')->willReturn($fileReshareStorage);
939+
$share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'user1', 'user2', 17, null, null, $shareAttributes);
940+
$data[] = [$share, $fileReshare, 'Cannot set the requested share attributes for sharedfile', true];
941+
942+
return $data;
943+
}
944+
945+
/**
946+
* @dataProvider dataShareNotEnoughPermissions
947+
*
948+
* @param $share
949+
* @param $superShareNode
950+
* @param $exceptionMessage
951+
* @param $exception
952+
*/
953+
public function testShareNotEnoughPermissions($share, $superShareNode, $exceptionMessage, $exception) {
954+
$sharer = $this->createMock(IUser::class);
955+
$sharer->method('getUID')->willReturn($share->getSharedBy());
956+
$this->userSession->method('getUser')->willReturn($sharer);
957+
958+
$userFolder = $this->createMock(Folder::class);
959+
$userFolder->method('getById')->willReturn([$superShareNode]);
960+
$this->rootFolder->method('getUserFolder')->willReturn($userFolder);
961+
962+
try {
963+
$this->invokePrivate($this->manager, 'validatePermissions', [$share]);
964+
$thrown = false;
965+
} catch (\OCP\Share\Exceptions\GenericShareException $e) {
966+
$this->assertEquals($exceptionMessage, $e->getHint());
967+
$thrown = true;
968+
}
969+
970+
$this->assertSame($exception, $thrown);
971+
}
972+
973+
/**
974+
* Non-movable mount share can be shared with delete and update permission
975+
* even if permission for file do not have this permission
976+
*/
977+
public function testShareNonMovableMountPermissions() {
978+
$nonMovableMountPoint = $this->createMock(IMountPoint::class);
979+
$file = $this->createMock(File::class);
980+
$file->method('getPermissions')->willReturn(1);
981+
$file->method('getMountPoint')->willReturn($nonMovableMountPoint);
982+
$share = $this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $file, 'user0', 'user1', 'user1', 11, null, null);
983+
984+
try {
985+
$this->invokePrivate($this->manager, 'validatePermissions', [$share]);
986+
$thrown = false;
987+
} catch (\Exception $e) {
988+
$thrown = true;
989+
}
990+
991+
$this->assertSame(false, $thrown);
992+
}
993+
886994
/**
887995
*/
888-
public function testvalidateExpirationDateInPast() {
996+
public function testValidateExpirationDateInPast() {
889997
$this->expectException(\OCP\Share\Exceptions\GenericShareException::class);
890998
$this->expectExceptionMessage('Expiration date is in the past');
891999

@@ -3523,6 +3631,102 @@ public function testGetAllSharedWith() {
35233631
$this->assertSame($shares, [$share1, $share2]);
35243632
}
35253633

3634+
/**
3635+
* @dataProvider strictSubsetOfAttributesDataProvider
3636+
*
3637+
* @param IAttributes $allowedAttributes
3638+
* @param IAttributes $newAttributes
3639+
* @param boolean $expected
3640+
*/
3641+
public function testStrictSubsetOfAttributes($allowedAttributes, $newAttributes, $expected) {
3642+
$this->assertEquals(
3643+
$expected,
3644+
$this->invokePrivate(
3645+
$this->manager,
3646+
'strictSubsetOfAttributes',
3647+
[$allowedAttributes, $newAttributes]
3648+
)
3649+
);
3650+
}
3651+
3652+
public function strictSubsetOfAttributesDataProvider() {
3653+
// no exception - supershare and share are equal
3654+
$superShareAttributes = new ShareAttributes();
3655+
$shareAttributes = new ShareAttributes();
3656+
$data[] = [$superShareAttributes,$shareAttributes, true];
3657+
3658+
// no exception - supershare and share are equal
3659+
$superShareAttributes = new ShareAttributes();
3660+
$superShareAttributes->setAttribute('test', 'test', true);
3661+
$shareAttributes = new ShareAttributes();
3662+
$shareAttributes->setAttribute('test', 'test', true);
3663+
$data[] = [$superShareAttributes,$shareAttributes, true];
3664+
3665+
// no exception - disabling attribute that supershare has enabled
3666+
$superShareAttributes = new ShareAttributes();
3667+
$superShareAttributes->setAttribute('test', 'test', true);
3668+
$shareAttributes = new ShareAttributes();
3669+
$shareAttributes->setAttribute('test', 'test', false);
3670+
$data[] = [$superShareAttributes,$shareAttributes, true];
3671+
3672+
// exception - adding an attribute while supershare has none
3673+
$superShareAttributes = new ShareAttributes();
3674+
$shareAttributes = new ShareAttributes();
3675+
$shareAttributes->setAttribute('test', 'test', true);
3676+
$data[] = [$superShareAttributes,$shareAttributes, false];
3677+
3678+
// exception - enabling attribute that supershare has disabled
3679+
$superShareAttributes = new ShareAttributes();
3680+
$superShareAttributes->setAttribute('test', 'test', false);
3681+
$shareAttributes = new ShareAttributes();
3682+
$shareAttributes->setAttribute('test', 'test', true);
3683+
$data[] = [$superShareAttributes,$shareAttributes, false];
3684+
3685+
// exception - removing attribute of that supershare has enabled
3686+
$superShareAttributes = new ShareAttributes();
3687+
$superShareAttributes->setAttribute('test', 'test', true);
3688+
$shareAttributes = new ShareAttributes();
3689+
$data[] = [$superShareAttributes,$shareAttributes, false];
3690+
3691+
// exception - removing attribute of that supershare has disabled
3692+
$superShareAttributes = new ShareAttributes();
3693+
$superShareAttributes->setAttribute('test', 'test', false);
3694+
$shareAttributes = new ShareAttributes();
3695+
$data[] = [$superShareAttributes,$shareAttributes, false];
3696+
3697+
// exception - removing one of attributes of supershare
3698+
$superShareAttributes = new ShareAttributes();
3699+
$superShareAttributes->setAttribute('test', 'test1', false);
3700+
$superShareAttributes->setAttribute('test', 'test2', false);
3701+
$shareAttributes = new ShareAttributes();
3702+
$shareAttributes->setAttribute('test', 'test1', false);
3703+
$data[] = [$superShareAttributes,$shareAttributes, false];
3704+
3705+
// exception - disabling one of attributes of supershare
3706+
$superShareAttributes = new ShareAttributes();
3707+
$superShareAttributes->setAttribute('test', 'test1', false);
3708+
$superShareAttributes->setAttribute('test', 'test2', false);
3709+
$shareAttributes = new ShareAttributes();
3710+
$shareAttributes->setAttribute('test', 'test1', false);
3711+
$shareAttributes->setAttribute('test', 'test2', true);
3712+
$data[] = [$superShareAttributes,$shareAttributes, false];
3713+
3714+
// exception - swaping one of attributes of supershare
3715+
$superShareAttributes = new ShareAttributes();
3716+
$superShareAttributes->setAttribute('test', 'test1', false);
3717+
$superShareAttributes->setAttribute('test', 'test01', false);
3718+
$superShareAttributes->setAttribute('test', 'test3', true);
3719+
$superShareAttributes->setAttribute('test', 'test4', null);
3720+
$shareAttributes = new ShareAttributes();
3721+
$shareAttributes->setAttribute('test', 'test1', false);
3722+
$shareAttributes->setAttribute('test', 'test10', false);
3723+
$superShareAttributes->setAttribute('test', 'test3', null);
3724+
$superShareAttributes->setAttribute('test', 'test4', true);
3725+
$data[] = [$superShareAttributes,$shareAttributes, false];
3726+
3727+
return $data;
3728+
}
3729+
35263730
/**
35273731
* @dataProvider strictSubsetOfPermissionsDataProvider
35283732
*

0 commit comments

Comments
 (0)
X Tutup