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

apps/files/lib/Controller/ViewController.php

@@ -80,7 +80,7 @@ protected function getStorageInfo(string $dir = '/') {
 	 */
 	#[NoAdminRequired]
 	#[NoCSRFRequired]
-	public function showFile(?string $fileid = null): Response {
+	public function showFile(?string $fileid = null, ?string $opendetails = null, ?string $openfile = null): Response {
 		if (!$fileid) {
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
 		}
@@ -89,7 +89,9 @@ public function showFile(?string $fileid = null): Response {
 		try {
 			return $this->redirectToFile((int) $fileid);
 		} 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,38 +100,35 @@ public function showFile(?string $fileid = null): Response {
 	 * @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);
@@ -158,8 +157,6 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal
 				if (count($nodes) === 1 && $relativePath !== $dir && $nodePath !== $dir) {
 					return $this->redirectToFile((int) $fileid);
 				}
-			} else { // fileid does not exist anywhere
-				$fileNotFound = true;
 			}
 		}
 
@@ -247,10 +244,12 @@ private function redirectToFileIfInTrashbin($fileId): RedirectResponse {
 	 * Redirects to the file list and highlight the given file id
 	 *
 	 * @param int $fileId file id to show
+	 * @param string|null $openDetails open details parameter
+	 * @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 $openDetails = null, ?string $openFile = null): RedirectResponse {
 		$uid = $this->userSession->getUser()->getUID();
 		$baseFolder = $this->rootFolder->getUserFolder($uid);
 		$node = $baseFolder->getFirstNodeById($fileId);
@@ -272,6 +271,19 @@ private function redirectToFile(int $fileId) {
 				// open the file by default (opening the viewer)
 				$params['openfile'] = 'true';
 			}
+
+			// Forward open parameters if any.
+			// - openfile is true by default
+			// - opendetails is undefined by default
+			// - both will be evaluated as truthy
+			if ($openDetails !== null) {
+				$params['opendetails'] = $openDetails !== 'false' ? 'true' : 'false';
+			}
+
+			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

@@ -209,6 +209,8 @@ export default defineComponent({
 							this.unselectFile()
 						}
 					}
+
+					this.openSidebarForFile(this.fileId)
 				})
 			},
 			immediate: true,
@@ -249,6 +251,7 @@ export default defineComponent({
 					sidebarAction.exec(node, this.currentView, this.currentFolder.path)
 				}
 			}
+			logger.error(`Failed to open sidebar on file ${fileId}, file isn't cached yet !`, { fileId, node })
 		},
 
 		scrollToFile(fileId: number|null, warn = true) {

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,72 +8,114 @@
 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\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;
 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 IUser&MockObject $user;
-	private IUserSession&MockObject $userSession;
-	private IAppManager&MockObject $appManager;
-	private IRootFolder&MockObject $rootFolder;
+	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 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')
 			->willReturn('testuser1');
 		$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);
 
-		$filenameValidator = $this->createMock(FilenameValidator::class);
+		// Make sure we know the app is enabled
+		$this->appManager->expects($this->any())
+			->method('cleanAppId')
+			->willReturnArgument(0);
+		$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',
@@ -148,7 +190,6 @@ public function testIndexWithRegularBrowser() {
 	public function testShowFileRouteWithTrashedFile() {
 		$this->appManager->expects($this->once())
 			->method('isEnabledForUser')
-			->with('files_trashbin')
 			->willReturn(true);
 
 		$parentNode = $this->getMockBuilder(Folder::class)->getMock();

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 @@ public function loadRoutes($app = null) {
 				$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 @@ private function getAttributeRoutes(string $app): array {
 	 * @param string $appName
 	 */
 	private function requireRouteFile($file, $appName) {
-		$this->setupRoutes(include_once $file, $appName);
+		$this->setupRoutes(include $file, $appName);
 	}