From 03b1791cca3e0334637aa232d1f7c11850793646 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 25 May 2022 09:55:22 +0200 Subject: Fix share attribute related tests + code style Signed-off-by: Vincent Petry --- apps/dav/lib/DAV/ViewOnlyPlugin.php | 1 + .../lib/Controller/ShareAPIController.php | 43 +++++++++++++--------- apps/files_sharing/tests/ApiTest.php | 11 +++++- apps/files_sharing/tests/ApplicationTest.php | 19 +++++----- .../tests/Controller/ShareAPIControllerTest.php | 34 ++++++++++++----- apps/files_sharing/tests/MountProviderTest.php | 7 ++-- lib/private/Share20/Manager.php | 15 +------- lib/private/Share20/Share.php | 4 +- lib/public/Share/IAttributes.php | 8 ++-- lib/public/Share/IShare.php | 8 ++-- tests/lib/Share20/ManagerTest.php | 3 ++ 11 files changed, 87 insertions(+), 66 deletions(-) diff --git a/apps/dav/lib/DAV/ViewOnlyPlugin.php b/apps/dav/lib/DAV/ViewOnlyPlugin.php index b6cd85a69a0..cd322572dcc 100644 --- a/apps/dav/lib/DAV/ViewOnlyPlugin.php +++ b/apps/dav/lib/DAV/ViewOnlyPlugin.php @@ -48,6 +48,7 @@ class ViewOnlyPlugin extends ServerPlugin { */ public function __construct(ILogger $logger) { $this->logger = $logger; + $this->server = null; } /** diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index f5c64c5d0d0..6b43e0f9dcf 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -45,7 +45,6 @@ declare(strict_types=1); namespace OCA\Files_Sharing\Controller; use OC\Files\FileInfo; -use OCA\DAV\DAV\ViewOnlyPlugin; use OCA\Files_Sharing\Exceptions\SharingRightsException; use OCA\Files_Sharing\External\Storage; use OCA\Files\Helper; @@ -442,6 +441,7 @@ class ShareAPIController extends OCSController { * @param string $sendPasswordByTalk * @param string $expireDate * @param string $label + * @param string $attributes * * @return DataResponse * @throws NotFoundException @@ -462,7 +462,8 @@ class ShareAPIController extends OCSController { string $sendPasswordByTalk = null, string $expireDate = '', string $note = '', - string $label = '' + string $label = '', + string $attributes = null ): DataResponse { $share = $this->shareManager->newShare(); @@ -680,7 +681,7 @@ class ShareAPIController extends OCSController { $share->setNote($note); } - $share = $this->setShareAttributes($share, $this->request->getParam('attributes', null)); + $share = $this->setShareAttributes($share, $attributes); try { $share = $this->shareManager->createShare($share); @@ -1043,6 +1044,7 @@ class ShareAPIController extends OCSController { * @param string $note * @param string $label * @param string $hideDownload + * @param string $attributes * @return DataResponse * @throws LockedException * @throws NotFoundException @@ -1059,7 +1061,8 @@ class ShareAPIController extends OCSController { string $expireDate = null, string $note = null, string $label = null, - string $hideDownload = null + string $hideDownload = null, + string $attributes = null ): DataResponse { try { $share = $this->getShareById($id); @@ -1077,8 +1080,6 @@ class ShareAPIController extends OCSController { throw new OCSForbiddenException('You are not allowed to edit incoming shares'); } - $shareAttributes = $this->request->getParam('attributes', null); - if ( $permissions === null && $password === null && @@ -1088,7 +1089,7 @@ class ShareAPIController extends OCSController { $note === null && $label === null && $hideDownload === null && - $shareAttributes === null + $attributes === null ) { throw new OCSBadRequestException($this->l->t('Wrong or no update parameter given')); } @@ -1227,7 +1228,7 @@ class ShareAPIController extends OCSController { } } - $share = $this->setShareAttributes($share, $shareAttributes); + $share = $this->setShareAttributes($share, $attributes); try { $share = $this->shareManager->updateShare($share); @@ -1848,18 +1849,24 @@ class ShareAPIController extends OCSController { /** * @param IShare $share - * @param string[][]|null $formattedShareAttributes + * @param string|null $attributesString * @return IShare modified share */ - private function setShareAttributes(IShare $share, $formattedShareAttributes) { - $newShareAttributes = $this->shareManager->newShare()->newAttributes(); - if ($formattedShareAttributes !== null) { - foreach ($formattedShareAttributes as $formattedAttr) { - $newShareAttributes->setAttribute( - $formattedAttr['scope'], - $formattedAttr['key'], - (bool) \json_decode($formattedAttr['enabled']) - ); + private function setShareAttributes(IShare $share, ?string $attributesString) { + $newShareAttributes = null; + if ($attributesString !== null) { + $newShareAttributes = $this->shareManager->newShare()->newAttributes(); + $formattedShareAttributes = \json_decode($attributesString, true); + if (is_array($formattedShareAttributes)) { + foreach ($formattedShareAttributes as $formattedAttr) { + $newShareAttributes->setAttribute( + $formattedAttr['scope'], + $formattedAttr['key'], + is_string($formattedAttr['enabled']) ? (bool) \json_decode($formattedAttr['enabled']) : $formattedAttr['enabled'] + ); + } + } else { + throw new OCSBadRequestException('Invalid share attributes provided: \"' . $attributesString . '\"'); } } $share->setAttributes($newShareAttributes); diff --git a/apps/files_sharing/tests/ApiTest.php b/apps/files_sharing/tests/ApiTest.php index 50869b7a9f8..98d0c171681 100644 --- a/apps/files_sharing/tests/ApiTest.php +++ b/apps/files_sharing/tests/ApiTest.php @@ -950,9 +950,13 @@ class ApiTest extends TestCase { ->setShareType(IShare::TYPE_USER) ->setPermissions(19) ->setAttributes($this->shareManager->newShare()->newAttributes()); + + $this->assertNotNull($share1->getAttributes()); $share1 = $this->shareManager->createShare($share1); + $this->assertNull($share1->getAttributes()); $this->assertEquals(19, $share1->getPermissions()); - $this->assertEquals(null, $share1->getAttributes()); + // attributes get cleared when empty + $this->assertNull($share1->getAttributes()); $share2 = $this->shareManager->newShare(); $share2->setNode($node1) @@ -964,7 +968,10 @@ class ApiTest extends TestCase { // update permissions $ocs = $this->createOCS(self::TEST_FILES_SHARING_API_USER1); - $ocs->updateShare($share1->getId(), 1); + $ocs->updateShare( + $share1->getId(), 1, null, null, null, null, null, null, null, + '[{"scope": "app1", "key": "attr1", "enabled": true}]' + ); $ocs->cleanup(); $share1 = $this->shareManager->getShareById('ocinternal:' . $share1->getId()); diff --git a/apps/files_sharing/tests/ApplicationTest.php b/apps/files_sharing/tests/ApplicationTest.php index 3f3164b233e..ee04996ac15 100644 --- a/apps/files_sharing/tests/ApplicationTest.php +++ b/apps/files_sharing/tests/ApplicationTest.php @@ -65,8 +65,7 @@ class ApplicationTest extends TestCase { $this->application = new Application([]); - // FIXME: how to mock this one ?? - $symfonyDispatcher = $this->createMock(SymfonyDispatcher::class); + $symfonyDispatcher = new SymfonyDispatcher(); $this->eventDispatcher = new EventDispatcher( $symfonyDispatcher, $this->createMock(IServerContainer::class), @@ -133,9 +132,9 @@ class ApplicationTest extends TestCase { */ public function testCheckDirectCanBeDownloaded($path, $userFolder, $run) { $user = $this->createMock(IUser::class); - $user->method("getUID")->willReturn("test"); - $this->userSession->method("getUser")->willReturn($user); - $this->userSession->method("isLoggedIn")->willReturn(true); + $user->method('getUID')->willReturn('test'); + $this->userSession->method('getUser')->willReturn($user); + $this->userSession->method('isLoggedIn')->willReturn(true); $this->rootFolder->method('getUserFolder')->willReturn($userFolder); // Simulate direct download of file @@ -210,11 +209,11 @@ class ApplicationTest extends TestCase { */ public function testCheckZipCanBeDownloaded($dir, $files, $userFolder, $run) { $user = $this->createMock(IUser::class); - $user->method("getUID")->willReturn("test"); - $this->userSession->method("getUser")->willReturn($user); - $this->userSession->method("isLoggedIn")->willReturn(true); + $user->method('getUID')->willReturn('test'); + $this->userSession->method('getUser')->willReturn($user); + $this->userSession->method('isLoggedIn')->willReturn(true); - $this->rootFolder->method('getUserFolder')->with("test")->willReturn($userFolder); + $this->rootFolder->method('getUserFolder')->with('test')->willReturn($userFolder); // Simulate zip download of folder folder $event = new GenericEvent(null, ['dir' => $dir, 'files' => $files, 'run' => true]); @@ -225,7 +224,7 @@ class ApplicationTest extends TestCase { } public function testCheckFileUserNotFound() { - $this->userSession->method("isLoggedIn")->willReturn(false); + $this->userSession->method('isLoggedIn')->willReturn(false); // Simulate zip download of folder folder $event = new GenericEvent(null, ['dir' => '/test', 'files' => ['test.txt'], 'run' => true]); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index f6568415727..a6a81bd672c 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -58,6 +58,7 @@ use OCP\IUser; use OCP\IUserManager; use OCP\Lock\LockedException; use OCP\Share\Exceptions\GenericShareException; +use OCP\Share\IAttributes as IShareAttributes; use OCP\Share\IManager; use OCP\Share\IShare; use Test\TestCase; @@ -125,10 +126,6 @@ class ShareAPIControllerTest extends TestCase { $this->shareManager ->expects($this->any()) ->method('shareProviderExists')->willReturn(true); - $this->shareManager - ->expects($this->any()) - ->method('newShare') - ->willReturn($this->newShare()); $this->groupManager = $this->createMock(IGroupManager::class); $this->userManager = $this->createMock(IUserManager::class); $this->request = $this->createMock(IRequest::class); @@ -201,11 +198,9 @@ class ShareAPIControllerTest extends TestCase { private function mockShareAttributes() { $formattedShareAttributes = [ [ - [ - 'scope' => 'permissions', - 'key' => 'download', - 'enabled' => true - ] + 'scope' => 'permissions', + 'key' => 'download', + 'enabled' => true ] ]; @@ -641,6 +636,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'status' => [], + 'attributes' => null, ]; $data[] = [$share, $expected]; @@ -692,6 +688,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ]; $data[] = [$share, $expected]; @@ -749,6 +746,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ]; $data[] = [$share, $expected]; @@ -3808,6 +3806,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'status' => [], + 'attributes' => '[{"scope":"permissions","key":"download","enabled":true}]', ], $share, [], false ]; // User backend up @@ -3845,6 +3844,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'status' => [], + 'attributes' => '[{"scope":"permissions","key":"download","enabled":true}]', ], $share, [ ['owner', $owner], ['initiator', $initiator], @@ -3898,6 +3898,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'status' => [], + 'attributes' => null, ], $share, [], false ]; @@ -3947,6 +3948,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => true, 'can_delete' => true, 'status' => [], + 'attributes' => null, ], $share, [], false ]; @@ -3996,6 +3998,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4042,6 +4045,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4095,6 +4099,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4148,6 +4153,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4195,6 +4201,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4242,6 +4249,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4292,6 +4300,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4339,6 +4348,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4386,6 +4396,7 @@ class ShareAPIControllerTest extends TestCase { 'hide_download' => 0, 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, [], false ]; @@ -4450,6 +4461,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'password_expiration_time' => null, + 'attributes' => null, ], $share, [], false ]; @@ -4500,6 +4512,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => false, 'can_delete' => false, 'password_expiration_time' => null, + 'attributes' => null, ], $share, [], false ]; @@ -4549,6 +4562,7 @@ class ShareAPIControllerTest extends TestCase { 'can_edit' => true, 'can_delete' => true, 'status' => [], + 'attributes' => null, ], $share, [], false ]; @@ -4700,6 +4714,7 @@ class ShareAPIControllerTest extends TestCase { 'label' => '', 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, false, [] ]; @@ -4746,6 +4761,7 @@ class ShareAPIControllerTest extends TestCase { 'label' => '', 'can_edit' => false, 'can_delete' => false, + 'attributes' => null, ], $share, true, [ 'share_with_displayname' => 'recipientRoomName' ] diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 740c7c89eb7..37e7e3d9d03 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -39,6 +39,7 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; +use OCP\Share\IAttributes as IShareAttributes; use OCP\Share\IManager; use OCP\Share\IShare; @@ -102,7 +103,7 @@ class MountProviderTest extends \Test\TestCase { return $shareAttributes; } - private function makeMockShare($id, $nodeId, $owner = 'user2', $target = null, $permissions = 31, $attributes) { + private function makeMockShare($id, $nodeId, $owner = 'user2', $target = null, $permissions = 31, $attributes = null) { $share = $this->createMock(IShare::class); $share->expects($this->any()) ->method('getPermissions') @@ -371,10 +372,10 @@ class MountProviderTest extends \Test\TestCase { $userManager = $this->createMock(IUserManager::class); $userShares = array_map(function ($shareSpec) { - return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4]); + return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4], $shareSpec[5]); }, $userShares); $groupShares = array_map(function ($shareSpec) { - return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4]); + return $this->makeMockShare($shareSpec[0], $shareSpec[1], $shareSpec[2], $shareSpec[3], $shareSpec[4], $shareSpec[5]); }, $groupShares); $this->user->expects($this->any()) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index d0120a299be..a46126b7ac4 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -70,7 +70,6 @@ use OCP\Share; use OCP\Share\Exceptions\AlreadySharedException; use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; -use OCP\Share\IAttributes; use OCP\Share\IManager; use OCP\Share\IProviderFactory; use OCP\Share\IShare; @@ -1094,7 +1093,7 @@ class Manager implements IManager { 'shareWith' => $share->getSharedWith(), 'uidOwner' => $share->getSharedBy(), 'permissions' => $share->getPermissions(), - 'attributes' => $share->getAttributes(), + 'attributes' => $share->getAttributes() !== null ? $share->getAttributes()->toArray() : null, 'path' => $userFolder->getRelativePath($share->getNode()->getPath()), ]); } @@ -2089,16 +2088,4 @@ class Manager implements IManager { yield from $provider->getAllShares(); } } - - /** - * @param IAttributes|null $perms - * @return string - */ - private function hashAttributes($perms) { - if ($perms === null || empty($perms->toArray())) { - return ""; - } - - return \md5(\json_encode($perms->toArray())); - } } diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index 1aeb79d4d31..9ba5db5252e 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -345,7 +345,7 @@ class Share implements IShare { /** * @inheritdoc */ - public function setAttributes(IAttributes $attributes) { + public function setAttributes(?IAttributes $attributes) { $this->attributes = $attributes; return $this; } @@ -353,7 +353,7 @@ class Share implements IShare { /** * @inheritdoc */ - public function getAttributes() { + public function getAttributes(): ?IAttributes { return $this->attributes; } diff --git a/lib/public/Share/IAttributes.php b/lib/public/Share/IAttributes.php index 9f2556e4005..6e4cee08b12 100644 --- a/lib/public/Share/IAttributes.php +++ b/lib/public/Share/IAttributes.php @@ -24,7 +24,7 @@ namespace OCP\Share; * Interface IAttributes * * @package OCP\Share - * @since 10.2.0 + * @since 25.0.0 */ interface IAttributes { @@ -35,7 +35,7 @@ interface IAttributes { * @param string $key key * @param bool $enabled enabled * @return IAttributes The modified object - * @since 10.2.0 + * @since 25.0.0 */ public function setAttribute($scope, $key, $enabled); @@ -46,7 +46,7 @@ interface IAttributes { * @param string $scope scope * @param string $key key * @return bool|null - * @since 10.2.0 + * @since 25.0.0 */ public function getAttribute($scope, $key); @@ -62,7 +62,7 @@ interface IAttributes { * ] * * @return array formatted IAttributes - * @since 10.2.0 + * @since 25.0.0 */ public function toArray(); } diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index d81f263b464..f86e1ec542d 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -325,19 +325,19 @@ interface IShare { /** * Set share attributes * - * @param IAttributes $attributes + * @param ?IAttributes $attributes * @since 25.0.0 * @return IShare The modified object */ - public function setAttributes(IAttributes $attributes); + public function setAttributes(?IAttributes $attributes); /** * Get share attributes * * @since 25.0.0 - * @return IAttributes + * @return ?IAttributes */ - public function getAttributes(); + public function getAttributes(): ?IAttributes; /** * Set the accepted status diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 797a5ebf683..ab296172a3c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -3132,6 +3132,8 @@ class ManagerTest extends \Test\TestCase { $manager->expects($this->once())->method('getShareById')->with('foo:42')->willReturn($originalShare); $share = $this->manager->newShare(); + $attrs = $this->manager->newShare()->newAttributes(); + $attrs->setAttribute('app1', 'perm1', true); $share->setProviderId('foo') ->setId('42') ->setShareType(IShare::TYPE_USER) @@ -3164,6 +3166,7 @@ class ManagerTest extends \Test\TestCase { 'uidOwner' => 'sharer', 'permissions' => 31, 'path' => '/myPath', + 'attributes' => $attrs->toArray(), ]); $manager->updateShare($share); -- cgit v1.2.3