Fix view-only code after code review comments

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

6 files changed, 22 insertions, 17 deletions

diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php
index 5b9471d537c..4c57f3412e3 100644
--- a/apps/dav/lib/Connector/Sabre/ServerFactory.php
+++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php
@@ -161,7 +161,7 @@ class ServerFactory {
 
 			// Allow view-only plugin for webdav requests
 			$server->addPlugin(new ViewOnlyPlugin(
-				\OC::$server->getLogger()
+				$this->logger
 			));
 
 			if ($this->userSession->isLoggedIn()) {
diff --git a/apps/dav/lib/DAV/ViewOnlyPlugin.php b/apps/dav/lib/DAV/ViewOnlyPlugin.php
index cd322572dcc..17b0823b7d7 100644
--- a/apps/dav/lib/DAV/ViewOnlyPlugin.php
+++ b/apps/dav/lib/DAV/ViewOnlyPlugin.php
@@ -26,7 +26,7 @@ use OCA\DAV\Connector\Sabre\File as DavFile;
 use OCA\DAV\Meta\MetaFile;
 use OCP\Files\FileInfo;
 use OCP\Files\NotFoundException;
-use OCP\ILogger;
+use Psr\Log\LoggerInterface;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
 use Sabre\HTTP\RequestInterface;
@@ -40,13 +40,13 @@ class ViewOnlyPlugin extends ServerPlugin {
 	/** @var Server $server */
 	private $server;
 
-	/** @var ILogger $logger */
+	/** @var LoggerInterface $logger */
 	private $logger;
 
 	/**
-	 * @param ILogger $logger
+	 * @param LoggerInterface $logger
 	 */
-	public function __construct(ILogger $logger) {
+	public function __construct(LoggerInterface $logger) {
 		$this->logger = $logger;
 		$this->server = null;
 	}
diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php
index 5c83e5c2963..2cfcb3f5393 100644
--- a/apps/dav/lib/Server.php
+++ b/apps/dav/lib/Server.php
@@ -232,7 +232,7 @@ class Server {
 
 		// Allow view-only plugin for webdav requests
 		$this->server->addPlugin(new ViewOnlyPlugin(
-			\OC::$server->getLogger()
+			$logger
 		));
 
 		if (BrowserErrorPagePlugin::isBrowserRequest($request)) {
diff --git a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php
index 59a557d7006..3bd7a2d6dde 100644
--- a/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php
+++ b/apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php
@@ -25,9 +25,9 @@ use OCA\Files_Sharing\SharedStorage;
 use OCA\DAV\Connector\Sabre\File as DavFile;
 use OCP\Files\File;
 use OCP\Files\Storage\IStorage;
-use OCP\ILogger;
 use OCP\Share\IAttributes;
 use OCP\Share\IShare;
+use Psr\Log\LoggerInterface;
 use Sabre\DAV\Server;
 use Sabre\DAV\Tree;
 use Test\TestCase;
@@ -45,7 +45,7 @@ class ViewOnlyPluginTest extends TestCase {
 
 	public function setUp(): void {
 		$this->plugin = new ViewOnlyPlugin(
-			$this->createMock(ILogger::class)
+			$this->createMock(LoggerInterface::class)
 		);
 		$this->request = $this->createMock(RequestInterface::class);
 		$this->tree = $this->createMock(Tree::class);
diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php
index fef579a11c0..451d6b6557a 100644
--- a/apps/files_sharing/lib/AppInfo/Application.php
+++ b/apps/files_sharing/lib/AppInfo/Application.php
@@ -125,12 +125,12 @@ class Application extends App implements IBootstrap {
 	}
 
 
-	public function registerMountProviders(IMountProviderCollection $mountProviderCollection, MountProvider $mountProvider, ExternalMountProvider $externalMountProvider) {
+	public function registerMountProviders(IMountProviderCollection $mountProviderCollection, MountProvider $mountProvider, ExternalMountProvider $externalMountProvider): void {
 		$mountProviderCollection->registerProvider($mountProvider);
 		$mountProviderCollection->registerProvider($externalMountProvider);
 	}
 
-	public function registerEventsScripts(IEventDispatcher $dispatcher, EventDispatcherInterface $oldDispatcher) {
+	public function registerEventsScripts(IEventDispatcher $dispatcher, EventDispatcherInterface $oldDispatcher): void {
 		// sidebar and files scripts
 		$dispatcher->addServiceListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class);
 		$dispatcher->addServiceListener(BeforeTemplateRenderedEvent::class, LegacyBeforeTemplateRenderedListener::class);
@@ -159,12 +159,12 @@ class Application extends App implements IBootstrap {
 		IEventDispatcher $dispatcher,
 		?IUserSession $userSession,
 		IRootFolder $rootFolder
-	) {
+	): void {
 
 		$dispatcher->addListener(
 			'file.beforeGetDirect',
 			function (GenericEvent $event) use ($userSession, $rootFolder) {
-				$pathsToCheck[] = $event->getArgument('path');
+				$pathsToCheck = [$event->getArgument('path')];
 
 				// Check only for user/group shares. Don't restrict e.g. share links
 				if ($userSession && $userSession->isLoggedIn()) {
@@ -213,7 +213,7 @@ class Application extends App implements IBootstrap {
 		);
 	}
 
-	public function setupSharingMenus(IManager $shareManager, IFactory $l10nFactory, IUserSession $userSession) {
+	public function setupSharingMenus(IManager $shareManager, IFactory $l10nFactory, IUserSession $userSession): void {
 		if (!$shareManager->shareApiEnabled() || !class_exists('\OCA\Files\App')) {
 			return;
 		}
diff --git a/apps/files_sharing/lib/ViewOnly.php b/apps/files_sharing/lib/ViewOnly.php
index 58ff2e7f2b4..26e8e43a871 100644
--- a/apps/files_sharing/lib/ViewOnly.php
+++ b/apps/files_sharing/lib/ViewOnly.php
@@ -42,7 +42,7 @@ class ViewOnly {
 	 * @param string[] $pathsToCheck
 	 * @return bool
 	 */
-	public function check($pathsToCheck) {
+	public function check(array $pathsToCheck): bool {
 		// If any of elements cannot be downloaded, prevent whole download
 		foreach ($pathsToCheck as $file) {
 			try {
@@ -70,7 +70,7 @@ class ViewOnly {
 	 * @return bool
 	 * @throws NotFoundException
 	 */
-	private function dirRecursiveCheck(Folder $dirInfo) {
+	private function dirRecursiveCheck(Folder $dirInfo): bool {
 		if (!$this->checkFileInfo($dirInfo)) {
 			return false;
 		}
@@ -94,7 +94,7 @@ class ViewOnly {
 	 * @return bool
 	 * @throws NotFoundException
 	 */
-	private function checkFileInfo(Node $fileInfo) {
+	private function checkFileInfo(Node $fileInfo): bool {
 		// Restrict view-only to nodes which are shared
 		$storage = $fileInfo->getStorage();
 		if (!$storage->instanceOfStorage(SharedStorage::class)) {
@@ -105,9 +105,14 @@ class ViewOnly {
 		/** @var \OCA\Files_Sharing\SharedStorage $storage */
 		$share = $storage->getShare();
 
+		$canDownload = true;
+
 		// Check if read-only and on whether permission can download is both set and disabled.
+		$attributes = $share->getAttributes();
+		if ($attributes !== null) {
+			$canDownload = $attributes->getAttribute('permissions', 'download');
+		}
 
-		$canDownload = $share->getAttributes()->getAttribute('permissions', 'download');
 		if ($canDownload !== null && !$canDownload) {
 			return false;
 		}