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

Signed-off-by: skjnldsv <[email protected]>

apps/files/lib/Controller/ViewController.php

@@ -82,16 +82,18 @@ 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'));
 		}
 
 		// 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, $opendetails, $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']));
 		}
 	}
 
@@ -100,38 +102,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);
@@ -159,8 +158,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;
 			}
 		}
 
@@ -249,10 +246,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);
@@ -274,6 +273,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

@@ -220,23 +220,25 @@ export default defineComponent({
 		},
 
 		openFile: {
-			async handler(openFile) {
+			handler(openFile) {
 				if (!openFile || !this.fileId) {
 					return
 				}
 
-				await this.handleOpenFile(this.fileId)
+				this.handleOpenFile(this.fileId)
 			},
 			immediate: true,
 		},
 
 		openDetails: {
-			handler() {
+			handler(openDetails) {
 				// wait for scrolling and updating the actions to settle
 				this.$nextTick(() => {
-					if (this.fileId && this.openDetails) {
-						this.openSidebarForFile(this.fileId)
+					if (!openDetails || !this.fileId) {
+						return
 					}
+
+					this.openSidebarForFile(this.fileId)
 				})
 			},
 			immediate: true,
@@ -276,7 +278,9 @@ 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 })
 		},
 
 		scrollToFile(fileId: number|null, warn = true) {

apps/files/tests/Controller/ViewControllerTest.php

@@ -8,6 +8,8 @@
 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;
@@ -16,66 +18,106 @@
 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;
 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',
@@ -147,10 +189,60 @@ public function testIndexWithRegularBrowser(): void {
 		$this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView'));
 	}
 
+	public function dataTestShortRedirect(): array {
+		// openfile is true by default
+		// opendetails is undefined by default
+		// both will be evaluated as truthy
+		return [
+			[null,		null,		'/index.php/apps/files/files/123456?openfile=true'],
+			['',		null,		'/index.php/apps/files/files/123456?openfile=true'],
+			[null,		'',			'/index.php/apps/files/files/123456?openfile=true&opendetails=true'],
+			['',		'', 		'/index.php/apps/files/files/123456?openfile=true&opendetails=true'],
+			['false',	'',			'/index.php/apps/files/files/123456?openfile=false'],
+			[null,		'false',	'/index.php/apps/files/files/123456?openfile=true&opendetails=false'],
+			['true',	'false',	'/index.php/apps/files/files/123456?openfile=true&opendetails=false'],
+			['false',	'true',		'/index.php/apps/files/files/123456?openfile=false&opendetails=true'],
+			['false',	'false',	'/index.php/apps/files/files/123456?openfile=false&opendetails=false'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataTestShortRedirect
+	 */
+	public function testShortRedirect($openfile, $opendetails, $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, $opendetails, $openfile);
+		$this->assertStringContainsString($result, $response->getHeaders()['Location']);
+	}
+
 	public function testShowFileRouteWithTrashedFile(): void {
-		$this->appManager->expects($this->once())
+		$this->appManager->expects($this->exactly(2))
 			->method('isEnabledForUser')
-			->with('files_trashbin')
 			->willReturn(true);
 
 		$parentNode = $this->getMockBuilder(Folder::class)->getMock();
@@ -189,13 +281,7 @@ public function testShowFileRouteWithTrashedFile(): void {
 			->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 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'));
 	}
 }

lib/private/Route/Router.php

@@ -160,11 +160,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');
@@ -486,7 +487,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);
 	}