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

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>

apps/files/lib/Controller/ViewController.php

@@ -135,16 +135,18 @@ class ViewController extends Controller {
 	 * @param string $fileid
 	 * @return TemplateResponse|RedirectResponse
 	 */
-	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']));
 		}
 	}
 
@@ -156,11 +158,10 @@ class ViewController extends Controller {
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
-	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);
 	}
 
 	/**
@@ -170,11 +171,10 @@ class ViewController extends Controller {
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
-	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);
 	}
 
 	/**
@@ -184,10 +184,9 @@ class ViewController extends Controller {
 	 * @param string $dir
 	 * @param string $view
 	 * @param string $fileid
-	 * @param bool $fileNotFound
 	 * @return TemplateResponse|RedirectResponse
 	 */
-	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);
@@ -228,8 +227,6 @@ class ViewController extends Controller {
 				if (count($nodes) === 1 && $relativePath !== $dir && $nodePath !== $dir) {
 					return $this->redirectToFile((int) $fileid);
 				}
-			} else { // fileid does not exist anywhere
-				$fileNotFound = true;
 			}
 		}
 
@@ -318,10 +315,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);
@@ -343,6 +341,13 @@ class ViewController extends Controller {
 				// open the file by default (opening the viewer)
 				$params['openfile'] = 'true';
 			}
+
+			// Forward openfile parameters if any.
+			// 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

@@ -263,8 +263,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 })
 		},
 
 		scrollToFile(fileId: number|null, warn = true) {

apps/files/tests/Controller/ViewControllerTest.php

@@ -32,18 +32,24 @@
  */
 namespace OCA\Files\Tests\Controller;
 
+use OC\Route\Router;
+use OC\URLGenerator;
 use OCA\Files\Activity\Helper;
 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;
@@ -51,17 +57,21 @@ use OCP\IURLGenerator;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Share\IManager;
+use Psr\Container\ContainerInterface;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /**
  * Class ViewControllerTest
  *
+ * @group RoutingWeirdness
+ *
  * @package OCA\Files\Tests\Controller
  */
 class ViewControllerTest extends TestCase {
 	/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
 	private $request;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
+	/** @var IURLGenerator */
 	private $urlGenerator;
 	/** @var IL10N */
 	private $l10n;
@@ -92,15 +102,34 @@ class ViewControllerTest extends TestCase {
 	/** @var ViewConfig|\PHPUnit\Framework\MockObject\MockObject */
 	private $viewConfig;
 
+	/** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */
+	private $cacheFactory;
+	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	private $logger;
+	/** @var IEventLogger|\PHPUnit\Framework\MockObject\MockObject */
+	private $eventLogger;
+	/** @var ContainerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	private $container;
+	/** @var Router */
+	private $router;
+
+
 	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->activityHelper = $this->createMock(Helper::class);
+		$this->shareManager = $this->createMock(IManager::class);
+
 		$this->user = $this->getMockBuilder(IUser::class)->getMock();
 		$this->user->expects($this->any())
 			->method('getUID')
@@ -108,14 +137,34 @@ class ViewControllerTest extends TestCase {
 		$this->userSession->expects($this->any())
 			->method('getUser')
 			->willReturn($this->user);
-		$this->rootFolder = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock();
-		$this->activityHelper = $this->createMock(Helper::class);
-		$this->initialState = $this->createMock(IInitialState::class);
-		$this->templateManager = $this->createMock(ITemplateManager::class);
-		$this->shareManager = $this->createMock(IManager::class);
-		$this->userConfig = $this->createMock(UserConfig::class);
-		$this->viewConfig = $this->createMock(ViewConfig::class);
-		$this->viewController = $this->getMockBuilder('\OCA\Files\Controller\ViewController')
+
+		// 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
+		);
+
+		// Create a real URLGenerator instance to generate URLs
+		$this->urlGenerator = new URLGenerator(
+			$this->config,
+			$this->userSession,
+			$this->cacheFactory,
+			$this->request,
+			$this->router
+		);
+
+		$this->viewController = $this->getMockBuilder(ViewController::class)
 			->setConstructorArgs([
 				'files',
 				$this->request,
@@ -139,7 +188,7 @@ class ViewControllerTest extends TestCase {
 		->getMock();
 	}
 
-	public function testIndexWithRegularBrowser() {
+	public function testIndexWithRegularBrowser(): void {
 		$this->viewController
 			->expects($this->any())
 			->method('getStorageInfo')
@@ -180,34 +229,66 @@ 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);
 
-		$this->activityHelper->method('getFavoriteFilePaths')
-			->with($this->user->getUID())
-			->willReturn([
-				'item' => [],
-				'folders' => [
-					'/test1',
-					'/test2/',
-					'/test3/sub4',
-					'/test5/sub6/',
-				],
-			]);
-
 		$this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView'));
 	}
 
-	public function testShowFileRouteWithTrashedFile() {
+	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->once())
 			->method('isEnabledForUser')
-			->with('files_trashbin')
 			->willReturn(true);
 
 		$parentNode = $this->getMockBuilder(Folder::class)->getMock();
@@ -246,13 +327,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'));
 	}
 }

lib/private/Route/Router.php

@@ -185,11 +185,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');
@@ -504,7 +505,7 @@ class Router implements IRouter {
 	 * @param string $appName
 	 */
 	private function requireRouteFile($file, $appName) {
-		$this->setupRoutes(include_once $file, $appName);
+		$this->setupRoutes(include $file, $appName);
 	}