Multiple fixesenh/noid/share-attributes

- Fix tests
- Use non deprecated event stuff
- Add a bit of type hinting to the new stuff
- More safe handling of instanceOfStorage (share might not be the first
  wrapper)
- Fix resharing

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

4 files changed, 70 insertions, 57 deletions

diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php
index ae039520c5b..63fdced9011 100644
--- a/apps/files_sharing/lib/AppInfo/Application.php
+++ b/apps/files_sharing/lib/AppInfo/Application.php
@@ -50,6 +50,7 @@ use OCA\Files_Sharing\Notification\Listener;
 use OCA\Files_Sharing\Notification\Notifier;
 use OCA\Files\Event\LoadAdditionalScriptsEvent;
 use OCA\Files\Event\LoadSidebar;
+use OCP\Files\Event\BeforeDirectGetEvent;
 use OCA\Files_Sharing\ShareBackend\File;
 use OCA\Files_Sharing\ShareBackend\Folder;
 use OCA\Files_Sharing\ViewOnly;
@@ -62,6 +63,8 @@ use OCP\EventDispatcher\IEventDispatcher;
 use OCP\EventDispatcher\GenericEvent;
 use OCP\Federation\ICloudIdManager;
 use OCP\Files\Config\IMountProviderCollection;
+use OCP\Files\Events\BeforeDirectFileDownloadEvent;
+use OCP\Files\Events\BeforeZipCreatedEvent;
 use OCP\Files\IRootFolder;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\IDBConnection;
@@ -157,59 +160,53 @@ class Application extends App implements IBootstrap {
 
 	public function registerDownloadEvents(
 		IEventDispatcher $dispatcher,
-		?IUserSession $userSession,
+		IUserSession $userSession,
 		IRootFolder $rootFolder
 	): void {
 
 		$dispatcher->addListener(
-			'file.beforeGetDirect',
-			function (GenericEvent $event) use ($userSession, $rootFolder) {
-				$pathsToCheck = [$event->getArgument('path')];
-				$event->setArgument('run', true);
-
+			BeforeDirectFileDownloadEvent::class,
+			function (BeforeDirectFileDownloadEvent $event) use ($userSession, $rootFolder): void {
+				$pathsToCheck = [$event->getPath()];
 				// Check only for user/group shares. Don't restrict e.g. share links
-				if ($userSession && $userSession->isLoggedIn()) {
-					$uid = $userSession->getUser()->getUID();
+				$user = $userSession->getUser();
+				if ($user) {
 					$viewOnlyHandler = new ViewOnly(
-						$rootFolder->getUserFolder($uid)
+						$rootFolder->getUserFolder($user->getUID())
 					);
 					if (!$viewOnlyHandler->check($pathsToCheck)) {
-						$event->setArgument('run', false);
-						$event->setArgument('errorMessage', 'Access to this resource or one of its sub-items has been denied.');
+						$event->setSuccessful(false);
+						$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
 					}
 				}
 			}
 		);
 
 		$dispatcher->addListener(
-			'file.beforeCreateZip',
-			function (GenericEvent $event) use ($userSession, $rootFolder) {
-				$dir = $event->getArgument('dir');
-				$files = $event->getArgument('files');
+			BeforeZipCreatedEvent::class,
+			function (BeforeZipCreatedEvent $event) use ($userSession, $rootFolder): void {
+				$dir = $event->getDirectory();
+				$files = $event->getFiles();
 
 				$pathsToCheck = [];
-				if (\is_array($files)) {
-					foreach ($files as $file) {
-						$pathsToCheck[] = $dir . '/' . $file;
-					}
-				} elseif (\is_string($files)) {
-					$pathsToCheck[] = $dir . '/' . $files;
+				foreach ($files as $file) {
+					$pathsToCheck[] = $dir . '/' . $file;
 				}
 
 				// Check only for user/group shares. Don't restrict e.g. share links
-				if ($userSession && $userSession->isLoggedIn()) {
-					$uid = $userSession->getUser()->getUID();
+				$user = $userSession->getUser();
+				if ($user) {
 					$viewOnlyHandler = new ViewOnly(
-						$rootFolder->getUserFolder($uid)
+						$rootFolder->getUserFolder($user->getUID())
 					);
 					if (!$viewOnlyHandler->check($pathsToCheck)) {
-						$event->setArgument('errorMessage', 'Access to this resource or one of its sub-items has been denied.');
-						$event->setArgument('run', false);
+						$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
+						$event->setSuccessful(false);
 					} else {
-						$event->setArgument('run', true);
+						$event->setSuccessful(true);
 					}
 				} else {
-					$event->setArgument('run', true);
+					$event->setSuccessful(true);
 				}
 			}
 		);
diff --git a/apps/files_sharing/lib/Controller/PublicPreviewController.php b/apps/files_sharing/lib/Controller/PublicPreviewController.php
index 4a16afa7ac0..98c4d8cafb4 100644
--- a/apps/files_sharing/lib/Controller/PublicPreviewController.php
+++ b/apps/files_sharing/lib/Controller/PublicPreviewController.php
@@ -136,7 +136,7 @@ class PublicPreviewController extends PublicShareController {
 	 * @param $token
 	 * @return DataResponse|FileDisplayResponse
 	 */
-	public function directLink($token) {
+	public function directLink(string $token) {
 		// No token no image
 		if ($token === '') {
 			return new DataResponse([], Http::STATUS_BAD_REQUEST);
diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index c72fd8ba8af..59089390667 100644
--- a/apps/files_sharing/lib/Controller/ShareAPIController.php
+++ b/apps/files_sharing/lib/Controller/ShareAPIController.php
@@ -45,6 +45,7 @@ declare(strict_types=1);
 namespace OCA\Files_Sharing\Controller;
 
 use OC\Files\FileInfo;
+use OC\Files\Storage\Wrapper\Wrapper;
 use OCA\Files_Sharing\Exceptions\SharingRightsException;
 use OCA\Files_Sharing\External\Storage;
 use OCA\Files_Sharing\SharedStorage;
@@ -524,14 +525,7 @@ class ShareAPIController extends OCSController {
 			$permissions &= ~($permissions & ~$node->getPermissions());
 		}
 
-		if ($share->getNode()->getStorage()->instanceOfStorage(SharedStorage::class)) {
-			/** @var \OCA\Files_Sharing\SharedStorage $storage */
-			$inheritedAttributes = $share->getNode()->getStorage()->getShare()->getAttributes();
-			if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) {
-				$share->setHideDownload(true);
-			}
-		}
-
+		$this->checkInheritedAttributes($share);
 
 		if ($shareType === IShare::TYPE_USER) {
 			// Valid user is required to share
@@ -1110,6 +1104,25 @@ class ShareAPIController extends OCSController {
 			$share->setNote($note);
 		}
 
+		$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
+
+		// get the node with the point of view of the current user
+		$nodes = $userFolder->getById($share->getNode()->getId());
+		if (count($nodes) > 0) {
+			$node = $nodes[0];
+			$storage = $node->getStorage();
+			if ($storage && $storage->instanceOfStorage(SharedStorage::class)) {
+				/** @var \OCA\Files_Sharing\SharedStorage $storage */
+				$inheritedAttributes = $storage->getShare()->getAttributes();
+				if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) {
+					if ($hideDownload === 'false') {
+						throw new OCSBadRequestException($this->l->t('Cannot increase permissions'));
+					}
+					$share->setHideDownload(true);
+				}
+			}
+		}
+
 		/**
 		 * expirationdate, password and publicUpload only make sense for link shares
 		 */
@@ -1135,24 +1148,6 @@ class ShareAPIController extends OCSController {
 				$share->setHideDownload(false);
 			}
 
-			$userFolder = $this->rootFolder->getUserFolder($this->currentUser);
-			// get the node with the point of view of the current user
-			$nodes = $userFolder->getById($share->getNode()->getId());
-			if (count($nodes) > 0) {
-				$node = $nodes[0];
-				$storage = $node->getStorage();
-				if ($storage->instanceOfStorage(SharedStorage::class)) {
-					/** @var \OCA\Files_Sharing\SharedStorage $storage */
-					$inheritedAttributes = $storage->getShare()->getAttributes();
-					if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) {
-						if ($hideDownload === 'false') {
-							throw new OCSBadRequestException($this->l->t('Cannot increate permissions'));
-						}
-						$share->setHideDownload(true);
-					}
-				}
-			}
-
 			$newPermissions = null;
 			if ($publicUpload === 'true') {
 				$newPermissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE;
@@ -1905,4 +1900,24 @@ class ShareAPIController extends OCSController {
 
 		return $share;
 	}
+
+	private function checkInheritedAttributes(IShare $share): void {
+		if ($share->getNode()->getStorage()->instanceOfStorage(SharedStorage::class)) {
+			$storage = $share->getNode()->getStorage();
+			if ($storage instanceof Wrapper) {
+				$storage = $storage->getInstanceOfStorage(SharedStorage::class);
+				if ($storage === null) {
+					throw new \RuntimeException('Should not happen, instanceOfStorage but getInstanceOfStorage return null');
+				}
+			} else {
+				throw new \RuntimeException('Should not happen, instanceOfStorage but not a wrapper');
+			}
+			/** @var \OCA\Files_Sharing\SharedStorage $storage */
+			$inheritedAttributes = $storage->getShare()->getAttributes();
+			if ($inheritedAttributes !== null && $inheritedAttributes->getAttribute('permissions', 'download') === false) {
+				$share->setHideDownload(true);
+			}
+		}
+
+	}
 }
diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php
index 30e398d663b..954c9cf70e6 100644
--- a/apps/files_sharing/lib/MountProvider.php
+++ b/apps/files_sharing/lib/MountProvider.php
@@ -242,8 +242,9 @@ class MountProvider implements IMountProvider {
 				$superPermissions |= $share->getPermissions();
 
 				// update share permission attributes
-				if ($share->getAttributes() !== null) {
-					foreach ($share->getAttributes()->toArray() as $attribute) {
+				$attributes = $share->getAttributes();
+				if ($attributes !== null) {
+					foreach ($attributes->toArray() as $attribute) {
 						if ($superAttributes->getAttribute($attribute['scope'], $attribute['key']) === true) {
 							// if super share attribute is already enabled, it is most permissive
 							continue;