Merge pull request #50877 from nextcloud/backport/50807/stable30

[stable30] fix(files): properly forward open params from short urls

apps/files/lib/Controller/ViewController.php

@@ -36,6 +36,7 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IURLGenerator;
 use OCP\IUserSession;
+use OCP\Util;
 
 /**
  * @package OCA\Files\Controller
@@ -80,16 +81,18 @@ class ViewController extends Controller {
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
-	public function showFile(?string $fileid = null): Response {
+	public function showFile(?string $fileid = null, ?string $openfile = null): Response {
 		if (!$fileid) {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
 		}
 
 		// This is the entry point from the `/f/{fileid}` URL which is hardcoded in the server.
 		try {
-			return $this->redirectToFile((int) $fileid);
+			return $this->redirectToFile((int)$fileid, $openfile);
 		} catch (NotFoundException $e) {
-			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', ['fileNotFound' => true]));
+			// Keep the fileid even if not found, it will be used
+			// to detect the file could not be found and warn the user
+			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', ['fileid' => $fileid, 'view' => 'files']));
 		}
 	}
 
@@ -98,49 +101,46 @@ class ViewController extends Controller {
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
-	public function indexView($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
-		return $this->index($dir, $view, $fileid, $fileNotFound);
+	public function indexView($dir = '', $view = '', $fileid = null) {
+		return $this->index($dir, $view, $fileid);
 	}
 
 	/**
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
-	public function indexViewFileid($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
-		return $this->index($dir, $view, $fileid, $fileNotFound);
+	public function indexViewFileid($dir = '', $view = '', $fileid = null) {
+		return $this->index($dir, $view, $fileid);
 	}
 
 	/**
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
-	public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
+	public function index($dir = '', $view = '', $fileid = null) {
 		if ($fileid !== null && $view !== 'trashbin') {
 			try {
-				return $this->redirectToFileIfInTrashbin((int) $fileid);
+				return $this->redirectToFileIfInTrashbin((int)$fileid);
 			} catch (NotFoundException $e) {
 			}
 		}
 
 		// Load the files we need
-		\OCP\Util::addInitScript('files', 'init');
-		\OCP\Util::addStyle('files', 'merged');
-		\OCP\Util::addScript('files', 'main');
+		Util::addInitScript('files', 'init');
+		Util::addStyle('files', 'merged');
+		Util::addScript('files', 'main');
 
 		$userId = $this->userSession->getUser()->getUID();
 
@@ -149,24 +149,22 @@ class ViewController extends Controller {
 		// in the correct folder
 		if ($fileid && $dir !== '') {
 			$baseFolder = $this->rootFolder->getUserFolder($userId);
-			$nodes = $baseFolder->getById((int) $fileid);
+			$nodes = $baseFolder->getById((int)$fileid);
 			if (!empty($nodes)) {
 				$nodePath = $baseFolder->getRelativePath($nodes[0]->getPath());
 				$relativePath = $nodePath ? dirname($nodePath) : '';
 				// If the requested path does not contain the file id
 				// or if the requested path is not the file id itself
 				if (count($nodes) === 1 && $relativePath !== $dir && $nodePath !== $dir) {
-					return $this->redirectToFile((int) $fileid);
+					return $this->redirectToFile((int)$fileid);
 				}
-			} else { // fileid does not exist anywhere
-				$fileNotFound = true;
 			}
 		}
 
 		try {
 			// If view is files, we use the directory, otherwise we use the root storage
 			$storageInfo = $this->getStorageInfo(($view === 'files' && $dir) ? $dir : '/');
-		} catch(\Exception $e) {
+		} catch (\Exception $e) {
 			$storageInfo = $this->getStorageInfo();
 		}
 
@@ -247,10 +245,11 @@ class ViewController extends Controller {
 	 * Redirects to the file list and highlight the given file id
 	 *
 	 * @param int $fileId file id to show
+	 * @param string|null $openFile open file parameter
 	 * @return RedirectResponse redirect response or not found response
 	 * @throws NotFoundException
 	 */
-	private function redirectToFile(int $fileId) {
+	private function redirectToFile(int $fileId, ?string $openFile = null): RedirectResponse {
 		$uid = $this->userSession->getUser()->getUID();
 		$baseFolder = $this->rootFolder->getUserFolder($uid);
 		$node = $baseFolder->getFirstNodeById($fileId);
@@ -272,6 +271,13 @@ class ViewController extends Controller {
 				// open the file by default (opening the viewer)
 				$params['openfile'] = 'true';
 			}
+
+			// Forward openfile parameters if any.
+			// It will be evaluated as truthy
+			if ($openFile !== null) {
+				$params['openfile'] = $openFile !== 'false' ? 'true' : 'false';
+			}
+
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
 		}
 

apps/files/src/components/FilesListVirtual.vue

@@ -247,7 +247,10 @@ export default defineComponent({
 				if (node && sidebarAction?.enabled?.([node], this.currentView)) {
 					logger.debug('Opening sidebar on file ' + node.path, { node })
 					sidebarAction.exec(node, this.currentView, this.currentFolder.path)
+					return
 				}
+
+				logger.error(`Failed to open sidebar on file ${fileId}, file isn't cached yet !`, { fileId, node })
 			}
 		},
 

apps/files/src/views/FilesList.vue

@@ -493,14 +493,24 @@ export default defineComponent({
 		},
 	},
 
-	mounted() {
-		this.fetchContent()
-
+	async mounted() {
 		subscribe('files:node:deleted', this.onNodeDeleted)
 		subscribe('files:node:updated', this.onUpdatedNode)
 
 		// reload on settings change
 		subscribe('files:config:updated', this.fetchContent)
+
+		// Finally, fetch the current directory contents
+		await this.fetchContent()
+		if (this.fileId) {
+			// If we have a fileId, let's check if the file exists
+			const node = this.dirContents.find(node => node.fileid.toString() === this.fileId.toString())
+			// If the file isn't in the current directory nor if
+			// the current directory is the file, we show an error
+			if (!node && this.currentFolder.fileid.toString() !== this.fileId.toString()) {
+				showError(t('files', 'The file could not be found'))
+			}
+		}
 	},
 
 	unmounted() {

apps/files/tests/Controller/ViewControllerTest.php

@@ -8,17 +8,23 @@
 namespace OCA\Files\Tests\Controller;
 
 use OC\Files\FilenameValidator;
+use OC\Route\Router;
+use OC\URLGenerator;
 use OCA\Files\Controller\ViewController;
 use OCA\Files\Service\UserConfig;
 use OCA\Files\Service\ViewConfig;
 use OCP\App\IAppManager;
-use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\ContentSecurityPolicy;
+use OCP\AppFramework\Http\RedirectResponse;
+use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
+use OCP\Diagnostics\IEventLogger;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\File;
 use OCP\Files\Folder;
 use OCP\Files\IRootFolder;
 use OCP\Files\Template\ITemplateManager;
+use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IL10N;
 use OCP\IRequest;
@@ -26,39 +32,53 @@ use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Container\ContainerInterface;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /**
  * Class ViewControllerTest
  *
+ * @group RoutingWeirdness
+ *
  * @package OCA\Files\Tests\Controller
  */
 class ViewControllerTest extends TestCase {
-	private IRequest&MockObject $request;
-	private IURLGenerator&MockObject $urlGenerator;
-	private IL10N&MockObject $l10n;
+	private ContainerInterface&MockObject $container;
+	private IAppManager&MockObject $appManager;
+	private ICacheFactory&MockObject $cacheFactory;
 	private IConfig&MockObject $config;
 	private IEventDispatcher $eventDispatcher;
+	private IEventLogger&MockObject $eventLogger;
+	private IInitialState&MockObject $initialState;
+	private IL10N&MockObject $l10n;
+	private IRequest&MockObject $request;
+	private IRootFolder&MockObject $rootFolder;
+	private ITemplateManager&MockObject $templateManager;
+	private IURLGenerator $urlGenerator;
 	private IUser&MockObject $user;
 	private IUserSession&MockObject $userSession;
-	private IAppManager&MockObject $appManager;
-	private IRootFolder&MockObject $rootFolder;
-	private IInitialState&MockObject $initialState;
-	private ITemplateManager&MockObject $templateManager;
+	private LoggerInterface&MockObject $logger;
 	private UserConfig&MockObject $userConfig;
 	private ViewConfig&MockObject $viewConfig;
+	private Router $router;
 
 	private ViewController&MockObject $viewController;
 
 	protected function setUp(): void {
 		parent::setUp();
-		$this->request = $this->getMockBuilder(IRequest::class)->getMock();
-		$this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock();
-		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
-		$this->config = $this->getMockBuilder(IConfig::class)->getMock();
+		$this->appManager = $this->createMock(IAppManager::class);
+		$this->config = $this->createMock(IConfig::class);
 		$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
-		$this->userSession = $this->getMockBuilder(IUserSession::class)->getMock();
-		$this->appManager = $this->getMockBuilder('\OCP\App\IAppManager')->getMock();
+		$this->initialState = $this->createMock(IInitialState::class);
+		$this->l10n = $this->createMock(IL10N::class);
+		$this->request = $this->createMock(IRequest::class);
+		$this->rootFolder = $this->createMock(IRootFolder::class);
+		$this->templateManager = $this->createMock(ITemplateManager::class);
+		$this->userConfig = $this->createMock(UserConfig::class);
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->viewConfig = $this->createMock(ViewConfig::class);
+
 		$this->user = $this->getMockBuilder(IUser::class)->getMock();
 		$this->user->expects($this->any())
 			->method('getUID')
@@ -66,14 +86,35 @@ class ViewControllerTest extends TestCase {
 		$this->userSession->expects($this->any())
 			->method('getUser')
 			->willReturn($this->user);
-		$this->rootFolder = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock();
-		$this->initialState = $this->createMock(IInitialState::class);
-		$this->templateManager = $this->createMock(ITemplateManager::class);
-		$this->userConfig = $this->createMock(UserConfig::class);
-		$this->viewConfig = $this->createMock(ViewConfig::class);
+
+		// Make sure we know the app is enabled
+		$this->appManager->expects($this->any())
+			->method('getAppPath')
+			->willReturnCallback(fn (string $appid): string => \OC::$SERVERROOT . '/apps/' . $appid);
+
+		$this->cacheFactory = $this->createMock(ICacheFactory::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->eventLogger = $this->createMock(IEventLogger::class);
+		$this->container = $this->createMock(ContainerInterface::class);
+		$this->router = new Router(
+			$this->logger,
+			$this->request,
+			$this->config,
+			$this->eventLogger,
+			$this->container,
+			$this->appManager,
+		);
+
+		// Create a real URLGenerator instance to generate URLs
+		$this->urlGenerator = new URLGenerator(
+			$this->config,
+			$this->userSession,
+			$this->cacheFactory,
+			$this->request,
+			$this->router
+		);
 
 		$filenameValidator = $this->createMock(FilenameValidator::class);
-
 		$this->viewController = $this->getMockBuilder(ViewController::class)
 			->setConstructorArgs([
 				'files',
@@ -91,13 +132,13 @@ class ViewControllerTest extends TestCase {
 				$this->viewConfig,
 				$filenameValidator,
 			])
-		->onlyMethods([
-			'getStorageInfo',
-		])
-		->getMock();
+			->onlyMethods([
+				'getStorageInfo',
+			])
+			->getMock();
 	}
 
-	public function testIndexWithRegularBrowser() {
+	public function testIndexWithRegularBrowser(): void {
 		$this->viewController
 			->expects($this->any())
 			->method('getStorageInfo')
@@ -133,11 +174,11 @@ class ViewControllerTest extends TestCase {
 			->method('getAppValue')
 			->willReturnArgument(2);
 
-		$expected = new Http\TemplateResponse(
+		$expected = new TemplateResponse(
 			'files',
 			'index',
 		);
-		$policy = new Http\ContentSecurityPolicy();
+		$policy = new ContentSecurityPolicy();
 		$policy->addAllowedWorkerSrcDomain('\'self\'');
 		$policy->addAllowedFrameDomain('\'self\'');
 		$expected->setContentSecurityPolicy($policy);
@@ -145,10 +186,54 @@ class ViewControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView'));
 	}
 
-	public function testShowFileRouteWithTrashedFile() {
-		$this->appManager->expects($this->once())
+	public function dataTestShortRedirect(): array {
+		// openfile is true by default
+		// and will be evaluated as truthy
+		return [
+			[null,		'/index.php/apps/files/files/123456?openfile=true'],
+			['',		'/index.php/apps/files/files/123456?openfile=true'],
+			['true',		'/index.php/apps/files/files/123456?openfile=true'],
+			['false',	'/index.php/apps/files/files/123456?openfile=false'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataTestShortRedirect
+	 */
+	public function testShortRedirect($openfile, $result) {
+		$this->appManager->expects($this->any())
+			->method('isEnabledForUser')
+			->with('files')
+			->willReturn(true);
+
+		$baseFolderFiles = $this->getMockBuilder(Folder::class)->getMock();
+		$this->rootFolder->expects($this->any())
+			->method('getUserFolder')
+			->with('testuser1')
+			->willReturn($baseFolderFiles);
+
+		$parentNode = $this->getMockBuilder(Folder::class)->getMock();
+		$parentNode->expects($this->once())
+			->method('getPath')
+			->willReturn('testuser1/files/Folder');
+
+		$node = $this->getMockBuilder(File::class)->getMock();
+		$node->expects($this->once())
+			->method('getParent')
+			->willReturn($parentNode);
+
+		$baseFolderFiles->expects($this->any())
+			->method('getFirstNodeById')
+			->with(123456)
+			->willReturn($node);
+
+		$response = $this->viewController->showFile(123456, $openfile);
+		$this->assertStringContainsString($result, $response->getHeaders()['Location']);
+	}
+
+	public function testShowFileRouteWithTrashedFile(): void {
+		$this->appManager->expects($this->exactly(2))
 			->method('isEnabledForUser')
-			->with('files_trashbin')
 			->willReturn(true);
 
 		$parentNode = $this->getMockBuilder(Folder::class)->getMock();
@@ -187,13 +272,7 @@ class ViewControllerTest extends TestCase {
 			->with('testuser1/files_trashbin/files/test.d1462861890/sub')
 			->willReturn('/test.d1462861890/sub');
 
-		$this->urlGenerator
-			->expects($this->once())
-			->method('linkToRoute')
-			->with('files.view.indexViewFileid', ['view' => 'trashbin', 'dir' => '/test.d1462861890/sub', 'fileid' => '123'])
-			->willReturn('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
-
-		$expected = new Http\RedirectResponse('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
+		$expected = new RedirectResponse('/index.php/apps/files/trashbin/123?dir=/test.d1462861890/sub');
 		$this->assertEquals($expected, $this->viewController->index('', '', '123'));
 	}
 }

cypress/e2e/files/files.cy.ts

@@ -1,16 +1,57 @@
+import type { User } from "@nextcloud/cypress"
+
 /**
  * SPDX-FileCopyrightText: 2022 Nextcloud GmbH and Nextcloud contributors
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
 describe('Files', { testIsolation: true }, () => {
+	let currentUser: User
+
 	beforeEach(() => {
 		cy.createRandomUser().then((user) => {
-			cy.login(user)
+			currentUser = user
 		})
 	})
 
 	it('Login with a user and open the files app', () => {
+		cy.login(currentUser)
 		cy.visit('/apps/files')
 		cy.get('[data-cy-files-list] [data-cy-files-list-row-name="welcome.txt"]').should('be.visible')
 	})
+
+	it('Opens a valid file shows it as active', () => {
+		cy.uploadContent(currentUser, new Blob(), 'text/plain', '/original.txt').then((response) => {
+			const fileId = Number.parseInt(response.headers['oc-fileid'] ?? '0')
+
+			cy.login(currentUser)
+			cy.visit('/apps/files/files/' + fileId)
+
+			cy.get(`[data-cy-files-list-row-fileid=${fileId}]`)
+				.should('be.visible')
+			cy.get(`[data-cy-files-list-row-fileid=${fileId}]`)
+				.invoke('attr', 'data-cy-files-list-row-name').should('eq', 'original.txt')
+			cy.get(`[data-cy-files-list-row-fileid=${fileId}]`)
+				.invoke('attr', 'class').should('contain', 'active')
+			cy.contains('The file could not be found').should('not.exist')
+		})
+	})
+
+	it('Opens a valid folder shows its content', () => {
+		cy.mkdir(currentUser, '/folder').then(() => {
+			cy.login(currentUser)
+			cy.visit('/apps/files/files?dir=/folder')
+
+			cy.get('[data-cy-files-content-breadcrumbs]').contains('folder').should('be.visible')
+			cy.contains('The file could not be found').should('not.exist')
+		})
+	})
+
+	it('Opens an unknown file show an error', () => {
+		cy.intercept('PROPFIND', /\/remote.php\/dav\//).as('propfind')
+		cy.login(currentUser)
+		cy.visit('/apps/files/files/123456')
+
+		cy.wait('@propfind')
+		cy.contains('The file could not be found').should('be.visible')
+	})
 })

lib/private/Route/Router.php

@@ -156,11 +156,12 @@ class Router implements IRouter {
 				$this->root->addCollection($collection);
 			}
 		}
+
 		if (!isset($this->loadedApps['core'])) {
 			$this->loadedApps['core'] = true;
 			$this->useCollection('root');
 			$this->setupRoutes($this->getAttributeRoutes('core'), 'core');
-			require_once __DIR__ . '/../../../core/routes.php';
+			require __DIR__ . '/../../../core/routes.php';
 
 			// Also add the OCS collection
 			$collection = $this->getCollection('root.ocs');
@@ -475,7 +476,7 @@ class Router implements IRouter {
 	 * @param string $appName
 	 */
 	private function requireRouteFile($file, $appName) {
-		$this->setupRoutes(include_once $file, $appName);
+		$this->setupRoutes(include $file, $appName);
 	}