X Tutup
Skip to content

Commit 547362d

Browse files
mrow4amrow4a
committed
validate reshare attributes based on supershare
1 parent c9b5ce5 commit 547362d

File tree

2 files changed

+267
-20
lines changed

2 files changed

+267
-20
lines changed

‎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 are not being disabled
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() {
9398
parent::setUp();
@@ -103,6 +108,7 @@ public function setUp() {
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() {
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')
@@ -819,26 +826,18 @@ public function dataGeneralChecks() {
819826
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_GROUP, $rootFolder, $group0, $user0, $user0, 2, null, null), 'You can\'t share your root folder', true];
820827
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_LINK, $rootFolder, null, $user0, $user0, 16, null, null), 'You can\'t share your root folder', true];
821828

822-
/**
823-
* Basic share (not re-share) with enough permission inputs
824-
*/
825-
$ownerUser = $this->createMock(IUser::class);
826-
$ownerUser->method('getUID')->willReturn('user1');
827829
$fileFullPermission = $this->createMock(File::class);
828830
$fileFullPermission->method('getPermissions')->willReturn(31);
829-
$fileFullPermission->method('getOwner')->willReturn($ownerUser);
830831
$fileFullPermission->method('isShareable')->willReturn(true);
832+
831833
$data[] = [$this->createShare(null, \OCP\Share::SHARE_TYPE_USER, $fileFullPermission, $user0, $user1, $user1, null, null, null), 'A share requires permissions', true];
832834

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

843842
return $data;
844843
}
@@ -880,11 +879,120 @@ public function testGeneralChecks($share, $exceptionMessage, $exception) {
880879
$this->assertSame($exception, $thrown);
881880
}
882881

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

889997
// Expire date in the past
890998
$past = new \DateTime();
@@ -3497,6 +3605,102 @@ public function testGetAllSharedWith() {
34973605
$this->assertSame($shares, [$share1, $share2]);
34983606
}
34993607

3608+
/**
3609+
* @dataProvider strictSubsetOfAttributesDataProvider
3610+
*
3611+
* @param IAttributes $allowedAttributes
3612+
* @param IAttributes $newAttributes
3613+
* @param boolean $expected
3614+
*/
3615+
public function testStrictSubsetOfAttributes($allowedAttributes, $newAttributes, $expected) {
3616+
$this->assertEquals(
3617+
$expected,
3618+
$this->invokePrivate(
3619+
$this->manager,
3620+
'strictSubsetOfAttributes',
3621+
[$allowedAttributes, $newAttributes]
3622+
)
3623+
);
3624+
}
3625+
3626+
public function strictSubsetOfAttributesDataProvider() {
3627+
// no exception - supershare and share are equal
3628+
$superShareAttributes = new ShareAttributes();
3629+
$shareAttributes = new ShareAttributes();
3630+
$data[] = [$superShareAttributes,$shareAttributes, true];
3631+
3632+
// no exception - supershare and share are equal
3633+
$superShareAttributes = new ShareAttributes();
3634+
$superShareAttributes->setAttribute('test', 'test', true);
3635+
$shareAttributes = new ShareAttributes();
3636+
$shareAttributes->setAttribute('test', 'test', true);
3637+
$data[] = [$superShareAttributes,$shareAttributes, true];
3638+
3639+
// no exception - disabling attribute that supershare has enabled
3640+
$superShareAttributes = new ShareAttributes();
3641+
$superShareAttributes->setAttribute('test', 'test', true);
3642+
$shareAttributes = new ShareAttributes();
3643+
$shareAttributes->setAttribute('test', 'test', false);
3644+
$data[] = [$superShareAttributes,$shareAttributes, true];
3645+
3646+
// exception - adding an attribute while supershare has none
3647+
$superShareAttributes = new ShareAttributes();
3648+
$shareAttributes = new ShareAttributes();
3649+
$shareAttributes->setAttribute('test', 'test', true);
3650+
$data[] = [$superShareAttributes,$shareAttributes, false];
3651+
3652+
// exception - enabling attribute that supershare has disabled
3653+
$superShareAttributes = new ShareAttributes();
3654+
$superShareAttributes->setAttribute('test', 'test', false);
3655+
$shareAttributes = new ShareAttributes();
3656+
$shareAttributes->setAttribute('test', 'test', true);
3657+
$data[] = [$superShareAttributes,$shareAttributes, false];
3658+
3659+
// exception - removing attribute of that supershare has enabled
3660+
$superShareAttributes = new ShareAttributes();
3661+
$superShareAttributes->setAttribute('test', 'test', true);
3662+
$shareAttributes = new ShareAttributes();
3663+
$data[] = [$superShareAttributes,$shareAttributes, false];
3664+
3665+
// exception - removing attribute of that supershare has disabled
3666+
$superShareAttributes = new ShareAttributes();
3667+
$superShareAttributes->setAttribute('test', 'test', false);
3668+
$shareAttributes = new ShareAttributes();
3669+
$data[] = [$superShareAttributes,$shareAttributes, false];
3670+
3671+
// exception - removing one of attributes of supershare
3672+
$superShareAttributes = new ShareAttributes();
3673+
$superShareAttributes->setAttribute('test', 'test1', false);
3674+
$superShareAttributes->setAttribute('test', 'test2', false);
3675+
$shareAttributes = new ShareAttributes();
3676+
$shareAttributes->setAttribute('test', 'test1', false);
3677+
$data[] = [$superShareAttributes,$shareAttributes, false];
3678+
3679+
// exception - disabling one of attributes of supershare
3680+
$superShareAttributes = new ShareAttributes();
3681+
$superShareAttributes->setAttribute('test', 'test1', false);
3682+
$superShareAttributes->setAttribute('test', 'test2', false);
3683+
$shareAttributes = new ShareAttributes();
3684+
$shareAttributes->setAttribute('test', 'test1', false);
3685+
$shareAttributes->setAttribute('test', 'test2', true);
3686+
$data[] = [$superShareAttributes,$shareAttributes, false];
3687+
3688+
// exception - swaping one of attributes of supershare
3689+
$superShareAttributes = new ShareAttributes();
3690+
$superShareAttributes->setAttribute('test', 'test1', false);
3691+
$superShareAttributes->setAttribute('test', 'test01', false);
3692+
$superShareAttributes->setAttribute('test', 'test3', true);
3693+
$superShareAttributes->setAttribute('test', 'test4', null);
3694+
$shareAttributes = new ShareAttributes();
3695+
$shareAttributes->setAttribute('test', 'test1', false);
3696+
$shareAttributes->setAttribute('test', 'test10', false);
3697+
$superShareAttributes->setAttribute('test', 'test3', null);
3698+
$superShareAttributes->setAttribute('test', 'test4', true);
3699+
$data[] = [$superShareAttributes,$shareAttributes, false];
3700+
3701+
return $data;
3702+
}
3703+
35003704
/**
35013705
* @dataProvider strictSubsetOfPermissionsDataProvider
35023706
*

0 commit comments

Comments
 (0)
X Tutup