Multiple fixes

- 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>
nextcloud · Jul 31, 2022 · 7b72381 · 7b72381
1 parent ab1a205
commit 7b72381
Show file tree

Hide file tree

Showing 29 changed files with 540 additions and 208 deletions.
diff --git a/apps/dav/lib/Connector/Sabre/Node.php b/apps/dav/lib/Connector/Sabre/Node.php
@@ -38,6 +38,7 @@
 use OC\Files\Mount\MoveableMount;
 use OC\Files\Node\File;
 use OC\Files\Node\Folder;
+use OC\Files\Storage\Wrapper\Wrapper;
 use OC\Files\View;
 use OCA\DAV\Connector\Sabre\Exception\InvalidPath;
 use OCP\Files\FileInfo;
@@ -334,9 +335,14 @@ public function getShareAttributes(): array {
 			$storage = null;
 		}
 
-		if ($storage && $storage->instanceOfStorage('\OCA\Files_Sharing\SharedStorage')) {
+		if ($storage && $storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
 			/** @var \OCA\Files_Sharing\SharedStorage $storage */
-			$attributes = $storage->getShare()->getAttributes()->toArray();
+			$attributes = $storage->getShare()->getAttributes();
+			if ($attributes === null) {
+				return [];
+			} else {
+				return $attributes->toArray();
+			}
 		}
 
 		return $attributes;

diff --git a/apps/dav/lib/Controller/DirectController.php b/apps/dav/lib/Controller/DirectController.php
@@ -36,6 +36,7 @@
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\EventDispatcher\GenericEvent;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Files\Events\BeforeDirectFileDownloadEvent;
 use OCP\Files\File;
 use OCP\Files\IRootFolder;
 use OCP\IRequest;
@@ -106,10 +107,10 @@ public function getUrl(int $fileId, int $expirationTime = 60 * 60 * 8): DataResp
 			throw new OCSBadRequestException('Direct download only works for files');
 		}
 
-		$event = new GenericEvent(null, ['path' => $userFolder->getRelativePath($file->getPath())]);
-		$this->eventDispatcher->dispatch('file.beforeGetDirect', $event);
+		$event = new BeforeDirectFileDownloadEvent($userFolder->getRelativePath($file->getPath()));
+		$this->eventDispatcher->dispatchTyped($event);
 
-		if ($event->getArgument('run') === false) {
+		if ($event->isSuccessful() === false) {
 			throw new OCSForbiddenException('Permission denied to download file');
 		}
 

diff --git a/apps/dav/lib/DAV/ViewOnlyPlugin.php b/apps/dav/lib/DAV/ViewOnlyPlugin.php
@@ -37,18 +37,11 @@
  */
 class ViewOnlyPlugin extends ServerPlugin {
 
-	/** @var Server $server */
-	private $server;
+	private ?Server $server = null;
+	private LoggerInterface $logger;
 
-	/** @var LoggerInterface $logger */
-	private $logger;
-
-	/**
-	 * @param LoggerInterface $logger
-	 */
 	public function __construct(LoggerInterface $logger) {
 		$this->logger = $logger;
-		$this->server = null;
 	}
 
 	/**
@@ -58,11 +51,8 @@ public function __construct(LoggerInterface $logger) {
 	 * addPlugin is called.
 	 *
 	 * This method should set up the required event subscriptions.
-	 *
-	 * @param Server $server
-	 * @return void
 	 */
-	public function initialize(Server $server) {
+	public function initialize(Server $server): void {
 		$this->server = $server;
 		//priority 90 to make sure the plugin is called before
 		//Sabre\DAV\CorePlugin::httpGet
@@ -73,17 +63,14 @@ public function initialize(Server $server) {
 	 * Disallow download via DAV Api in case file being received share
 	 * and having special permission
 	 *
-	 * @param RequestInterface $request request object
-	 * @return boolean
 	 * @throws Forbidden
 	 * @throws NotFoundException
 	 */
-	public function checkViewOnly(
-		RequestInterface $request
-	) {
+	public function checkViewOnly(RequestInterface $request): bool {
 		$path = $request->getPath();
 
 		try {
+			assert($this->server !== null);
 			$davNode = $this->server->tree->getNodeForPath($path);
 			if (!($davNode instanceof DavFile)) {
 				return true;
@@ -92,21 +79,28 @@ public function checkViewOnly(
 			$node = $davNode->getNode();
 
 			$storage = $node->getStorage();
-			// using string as we have no guarantee that "files_sharing" app is loaded
-			if (!$storage->instanceOfStorage('OCA\Files_Sharing\SharedStorage')) {
+
+			if (!$storage->instanceOfStorage(\OCA\Files_Sharing\SharedStorage::class)) {
 				return true;
 			}
 			// Extract extra permissions
 			/** @var \OCA\Files_Sharing\SharedStorage $storage */
 			$share = $storage->getShare();
 
+			$attributes = $share->getAttributes();
+			if ($attributes === null) {
+				return true;
+			}
+
 			// Check if read-only and on whether permission can download is both set and disabled.
-			$canDownload = $share->getAttributes()->getAttribute('permissions', 'download');
+			$canDownload = $attributes->getAttribute('permissions', 'download');
 			if ($canDownload !== null && !$canDownload) {
 				throw new Forbidden('Access to this resource has been denied because it is in view-only mode.');
 			}
 		} catch (NotFound $e) {
-			$this->logger->warning($e->getMessage());
+			$this->logger->warning($e->getMessage(), [
+				'exception' => $e,
+			]);
 		}
 
 		return true;

diff --git a/apps/dav/tests/unit/Controller/DirectControllerTest.php b/apps/dav/tests/unit/Controller/DirectControllerTest.php
@@ -34,11 +34,12 @@
 use OCP\AppFramework\OCS\OCSBadRequestException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\IRequest;
-use OCP\IURLGenerator;
+use OCP\IUrlGenerator;
 use OCP\Security\ISecureRandom;
 use Test\TestCase;
 
@@ -56,11 +57,13 @@ class DirectControllerTest extends TestCase {
 	/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
 	private $timeFactory;
 
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IUrlGenerator|\PHPUnit\Framework\MockObject\MockObject */
 	private $urlGenerator;
 
-	/** @var DirectController */
-	private $controller;
+	/** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */
+	private $eventDispatcher;
+
+	private DirectController $controller;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -69,7 +72,8 @@ protected function setUp(): void {
 		$this->directMapper = $this->createMock(DirectMapper::class);
 		$this->random = $this->createMock(ISecureRandom::class);
 		$this->timeFactory = $this->createMock(ITimeFactory::class);
-		$this->urlGenerator = $this->createMock(IURLGenerator::class);
+		$this->urlGenerator = $this->createMock(IUrlGenerator::class);
+		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
 
 		$this->controller = new DirectController(
 			'dav',
@@ -79,11 +83,12 @@ protected function setUp(): void {
 			$this->directMapper,
 			$this->random,
 			$this->timeFactory,
-			$this->urlGenerator
+			$this->urlGenerator,
+			$this->eventDispatcher
 		);
 	}
 
-	public function testGetUrlNonExistingFileId() {
+	public function testGetUrlNonExistingFileId(): void {
 		$userFolder = $this->createMock(Folder::class);
 		$this->rootFolder->method('getUserFolder')
 			->with('awesomeUser')
@@ -97,7 +102,7 @@ public function testGetUrlNonExistingFileId() {
 		$this->controller->getUrl(101);
 	}
 
-	public function testGetUrlForFolder() {
+	public function testGetUrlForFolder(): void {
 		$userFolder = $this->createMock(Folder::class);
 		$this->rootFolder->method('getUserFolder')
 			->with('awesomeUser')
@@ -113,7 +118,7 @@ public function testGetUrlForFolder() {
 		$this->controller->getUrl(101);
 	}
 
-	public function testGetUrlValid() {
+	public function testGetUrlValid(): void {
 		$userFolder = $this->createMock(Folder::class);
 		$this->rootFolder->method('getUserFolder')
 			->with('awesomeUser')
@@ -128,6 +133,9 @@ public function testGetUrlValid() {
 			->with(101)
 			->willReturn([$file]);
 
+		$userFolder->method('getRelativePath')
+			->willReturn('/path');
+
 		$this->random->method('generate')
 			->with(
 				60,

diff --git a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php
@@ -36,8 +36,7 @@
 
 class ViewOnlyPluginTest extends TestCase {
 
-	/** @var ViewOnlyPlugin */
-	private $plugin;
+	private ViewOnlyPlugin $plugin;
 	/** @var Tree | \PHPUnit\Framework\MockObject\MockObject */
 	private $tree;
 	/** @var RequestInterface | \PHPUnit\Framework\MockObject\MockObject */
@@ -56,14 +55,14 @@ public function setUp(): void {
 		$this->plugin->initialize($server);
 	}
 
-	public function testCanGetNonDav() {
+	public function testCanGetNonDav(): void {
 		$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
 		$this->tree->method('getNodeForPath')->willReturn(null);
 
 		$this->assertTrue($this->plugin->checkViewOnly($this->request));
 	}
 
-	public function testCanGetNonShared() {
+	public function testCanGetNonShared(): void {
 		$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
 		$davNode = $this->createMock(DavFile::class);
 		$this->tree->method('getNodeForPath')->willReturn($davNode);
@@ -78,7 +77,7 @@ public function testCanGetNonShared() {
 		$this->assertTrue($this->plugin->checkViewOnly($this->request));
 	}
 
-	public function providesDataForCanGet() {
+	public function providesDataForCanGet(): array {
 		return [
 			// has attribute permissions-download enabled - can get file
 			[ $this->createMock(File::class), true, true],
@@ -92,7 +91,7 @@ public function providesDataForCanGet() {
 	/**
 	 * @dataProvider providesDataForCanGet
 	 */
-	public function testCanGet($nodeInfo, $attrEnabled, $expectCanDownloadFile) {
+	public function testCanGet(File $nodeInfo, ?bool $attrEnabled, bool $expectCanDownloadFile): void {
 		$this->request->expects($this->once())->method('getPath')->willReturn('files/test/target');
 
 		$davNode = $this->createMock(DavFile::class);

diff --git 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\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\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 @@ public function registerEventsScripts(IEventDispatcher $dispatcher, EventDispatc
 
 	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
@@ -136,7 +136,7 @@ public function getPreview(
 	 * @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);