Fix share attribute related tests + code style

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

5 files changed, 72 insertions, 42 deletions

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())