Merge pull request #9282 from nextcloud/bugfix/9279/scrit_type_share_api

Make the ShareAPIController strict

apps/files_sharing/lib/Controller/ShareAPIController.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -36,6 +37,7 @@ use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\Constants;
+use OCP\Files\Folder;
 use OCP\Files\Node;
 use OCP\Files\NotFoundException;
 use OCP\IConfig;
@@ -51,6 +53,7 @@ use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\Exceptions\GenericShareException;
 use OCP\Lock\ILockingProvider;
 use OCP\Share\IShare;
+use OCA\Files_Sharing\External\Storage;
 
 /**
  * Class Share20OCS
@@ -65,8 +68,6 @@ class ShareAPIController extends OCSController {
 	private $groupManager;
 	/** @var IUserManager */
 	private $userManager;
-	/** @var IRequest */
-	protected $request;
 	/** @var IRootFolder */
 	private $rootFolder;
 	/** @var IURLGenerator */
@@ -95,14 +96,14 @@ class ShareAPIController extends OCSController {
 	 * @param IConfig $config
 	 */
 	public function __construct(
-		$appName,
+		string $appName,
 		IRequest $request,
 		IManager $shareManager,
 		IGroupManager $groupManager,
 		IUserManager $userManager,
 		IRootFolder $rootFolder,
 		IURLGenerator $urlGenerator,
-		$userId,
+		string $userId,
 		IL10N $l10n,
 		IConfig $config
 	) {
@@ -127,7 +128,7 @@ class ShareAPIController extends OCSController {
 	 * @return array
 	 * @throws NotFoundException In case the node can't be resolved.
 	 */
-	protected function formatShare(\OCP\Share\IShare $share, Node $recipientNode = null) {
+	protected function formatShare(\OCP\Share\IShare $share, Node $recipientNode = null): array {
 		$sharedBy = $this->userManager->get($share->getSharedBy());
 		$shareOwner = $this->userManager->get($share->getShareOwner());
 
@@ -233,7 +234,7 @@ class ShareAPIController extends OCSController {
 	 * @param string $property
 	 * @return string
 	 */
-	private function getDisplayNameFromAddressBook($query, $property) {
+	private function getDisplayNameFromAddressBook(string $query, string $property): string {
 		// FIXME: If we inject the contacts manager it gets initialized bofore any address books are registered
 		$result = \OC::$server->getContactsManager()->search($query, [$property]);
 		foreach ($result as $r) {
@@ -256,7 +257,7 @@ class ShareAPIController extends OCSController {
 	 * @return DataResponse
 	 * @throws OCSNotFoundException
 	 */
-	public function getShare($id) {
+	public function getShare(string $id): DataResponse {
 		try {
 			$share = $this->getShareById($id);
 		} catch (ShareNotFound $e) {
@@ -284,7 +285,7 @@ class ShareAPIController extends OCSController {
 	 * @return DataResponse
 	 * @throws OCSNotFoundException
 	 */
-	public function deleteShare($id) {
+	public function deleteShare(string $id): DataResponse {
 		try {
 			$share = $this->getShareById($id);
 		} catch (ShareNotFound $e) {
@@ -332,14 +333,14 @@ class ShareAPIController extends OCSController {
 	 * @suppress PhanUndeclaredClassMethod
 	 */
 	public function createShare(
-		$path = null,
-		$permissions = null,
-		$shareType = -1,
-		$shareWith = null,
-		$publicUpload = 'false',
-		$password = '',
-		$expireDate = ''
-	) {
+		string $path = null,
+		int $permissions = null,
+		int $shareType = -1,
+		string $shareWith = null,
+		string $publicUpload = 'false',
+		string $password = '',
+		string $expireDate = ''
+	): DataResponse {
 		$share = $this->shareManager->newShare();
 
 		if ($permissions === null) {
@@ -384,7 +385,7 @@ class ShareAPIController extends OCSController {
 		 * We check the permissions via webdav. But the permissions of the mount point
 		 * do not equal the share permissions. Here we fix that for federated mounts.
 		 */
-		if ($path->getStorage()->instanceOfStorage('OCA\Files_Sharing\External\Storage')) {
+		if ($path->getStorage()->instanceOfStorage(Storage::class)) {
 			$permissions &= ~($permissions & ~$path->getPermissions());
 		}
 
@@ -510,7 +511,7 @@ class ShareAPIController extends OCSController {
 	 * @param boolean $includeTags
 	 * @return DataResponse
 	 */
-	private function getSharedWithMe($node = null, $includeTags) {
+	private function getSharedWithMe($node = null, bool $includeTags): DataResponse {
 
 		$userShares = $this->shareManager->getSharedWith($this->currentUser, \OCP\Share::SHARE_TYPE_USER, $node, -1, 0);
 		$groupShares = $this->shareManager->getSharedWith($this->currentUser, \OCP\Share::SHARE_TYPE_GROUP, $node, -1, 0);
@@ -545,7 +546,7 @@ class ShareAPIController extends OCSController {
 	 * @return DataResponse
 	 * @throws OCSBadRequestException
 	 */
-	private function getSharesInDir($folder) {
+	private function getSharesInDir(Node $folder): DataResponse {
 		if (!($folder instanceof \OCP\Files\Folder)) {
 			throw new OCSBadRequestException($this->l->t('Not a directory'));
 		}
@@ -597,12 +598,12 @@ class ShareAPIController extends OCSController {
 	 * @throws OCSNotFoundException
 	 */
 	public function getShares(
-		$shared_with_me = 'false',
-		$reshares = 'false',
-		$subfiles = 'false',
-		$path = null,
-		$include_tags = 'false'
-	) {
+		string $shared_with_me = 'false',
+		string $reshares = 'false',
+		string $subfiles = 'false',
+		string $path = null,
+		string $include_tags = 'false'
+	): DataResponse {
 
 		if ($path !== null) {
 			$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
@@ -616,6 +617,8 @@ class ShareAPIController extends OCSController {
 			}
 		}
 
+		$include_tags = $include_tags === 'true';
+
 		if ($shared_with_me === 'true') {
 			$result = $this->getSharedWithMe($path, $include_tags);
 			return $result;
@@ -673,7 +676,7 @@ class ShareAPIController extends OCSController {
 	/**
 	 * @NoAdminRequired
 	 *
-	 * @param int $id
+	 * @param string $id
 	 * @param int $permissions
 	 * @param string $password
 	 * @param string $publicUpload
@@ -684,12 +687,12 @@ class ShareAPIController extends OCSController {
 	 * @throws OCSForbiddenException
 	 */
 	public function updateShare(
-		$id,
-		$permissions = null,
-		$password = null,
-		$publicUpload = null,
-		$expireDate = null
-	) {
+		string $id,
+		int $permissions = null,
+		string $password = null,
+		string $publicUpload = null,
+		string $expireDate = null
+	): DataResponse {
 		try {
 			$share = $this->getShareById($id);
 		} catch (ShareNotFound $e) {
@@ -730,7 +733,7 @@ class ShareAPIController extends OCSController {
 					Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE, // correct
 					Constants::PERMISSION_CREATE, // hidden file list
 					Constants::PERMISSION_READ | Constants::PERMISSION_UPDATE, // allow to edit single files
-				])
+				], true)
 			) {
 				throw new OCSBadRequestException($this->l->t('Can\'t change permissions for public share links'));
 			}
@@ -830,11 +833,7 @@ class ShareAPIController extends OCSController {
 		return new DataResponse($this->formatShare($share));
 	}
 
-	/**
-	 * @param \OCP\Share\IShare $share
-	 * @return bool
-	 */
-	protected function canAccessShare(\OCP\Share\IShare $share, $checkGroups = true) {
+	protected function canAccessShare(\OCP\Share\IShare $share, bool $checkGroups = true): bool {
 		// A file with permissions 0 can't be accessed by us. So Don't show it
 		if ($share->getPermissions() === 0) {
 			return false;
@@ -880,7 +879,7 @@ class ShareAPIController extends OCSController {
 	 * @throws \Exception
 	 * @return \DateTime
 	 */
-	private function parseDate($expireDate) {
+	private function parseDate(string $expireDate): \DateTime {
 		try {
 			$date = new \DateTime($expireDate);
 		} catch (\Exception $e) {
@@ -904,7 +903,7 @@ class ShareAPIController extends OCSController {
 	 * @return \OCP\Share\IShare
 	 * @throws ShareNotFound
 	 */
-	private function getShareById($id) {
+	private function getShareById(string $id): IShare {
 		$share = null;
 
 		// First check if it is an internal share.
@@ -946,6 +945,7 @@ class ShareAPIController extends OCSController {
 	 * Lock a Node
 	 *
 	 * @param \OCP\Files\Node $node
+	 * @throws LockedException
 	 */
 	private function lock(\OCP\Files\Node $node) {
 		$node->lock(ILockingProvider::LOCK_SHARED);
@@ -954,6 +954,7 @@ class ShareAPIController extends OCSController {
 
 	/**
 	 * Cleanup the remaining locks
+	 * @throws @LockedException
 	 */
 	public function cleanup() {
 		if ($this->lockedNode !== null) {

apps/files_sharing/tests/Controller/ShareAPIControllerTest.php

@@ -756,7 +756,7 @@ class ShareAPIControllerTest extends TestCase {
 			}))
 			->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_USER, 'validUser');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -863,7 +863,7 @@ class ShareAPIControllerTest extends TestCase {
 			}))
 			->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_GROUP, 'validGroup');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -998,7 +998,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_LINK, null, 'true', '', '');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1032,7 +1032,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_LINK, null, 'false', 'password', '');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1079,7 +1079,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->createShare('valid-path', \OCP\Constants::PERMISSION_ALL, \OCP\Share::SHARE_TYPE_LINK, null, 'false', '', '2000-01-01');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1254,7 +1254,7 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, '', 'false', '');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1289,7 +1289,7 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, 'password', 'true', '2000-01-01');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1323,7 +1323,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, $permissions, $password, $publicUpload, $expireDate);
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1440,7 +1440,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, 'newpassword', null, null);
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1477,7 +1477,7 @@ class ShareAPIControllerTest extends TestCase {
 			})
 		)->will($this->returnArgument(0));
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, null, null, '2010-12-23');
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1514,7 +1514,7 @@ class ShareAPIControllerTest extends TestCase {
 		$this->shareManager->method('getSharedWith')
 			->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, null, null, 'true', null);
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1550,7 +1550,7 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 7, null, null, null);
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1586,7 +1586,7 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 31, null, null, null);
 
 		$this->assertInstanceOf(get_class($expected), $result);
@@ -1615,7 +1615,7 @@ class ShareAPIControllerTest extends TestCase {
 
 		$this->shareManager->method('getSharedWith')->willReturn([]);
 
-		$expected = new DataResponse(null);
+		$expected = new DataResponse([]);
 		$result = $ocs->updateShare(42, 31, null, null, null);
 
 		$this->assertInstanceOf(get_class($expected), $result);