From 2e50a39265e09ac5e8de6ec53cff1546aafe8629 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Fri, 14 Feb 2025 09:28:58 +0100 Subject: [PATCH] fix(files): properly forward open params from short urls Signed-off-by: skjnldsv --- apps/files/lib/Controller/ViewController.php | 40 +++-- .../files/src/components/FilesListVirtual.vue | 14 +- .../tests/Controller/ViewControllerTest.php | 144 ++++++++++++++---- lib/private/Route/Router.php | 5 +- 4 files changed, 153 insertions(+), 50 deletions(-) diff --git a/apps/files/lib/Controller/ViewController.php b/apps/files/lib/Controller/ViewController.php index a0b5c1c282c..50b0ae558aa 100644 --- a/apps/files/lib/Controller/ViewController.php +++ b/apps/files/lib/Controller/ViewController.php @@ -82,16 +82,18 @@ class ViewController extends Controller { */ #[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 @@ 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); @@ -159,8 +158,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; } } @@ -249,10 +246,12 @@ 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 $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 @@ class ViewController extends Controller { // 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)); } diff --git a/apps/files/src/components/FilesListVirtual.vue b/apps/files/src/components/FilesListVirtual.vue index 06f576cf843..812521330b9 100644 --- a/apps/files/src/components/FilesListVirtual.vue +++ b/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) { diff --git a/apps/files/tests/Controller/ViewControllerTest.php b/apps/files/tests/Controller/ViewControllerTest.php index 0c0647cc415..56cc7d7c6f8 100644 --- a/apps/files/tests/Controller/ViewControllerTest.php +++ b/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,11 +18,13 @@ 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; @@ -28,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') @@ -68,14 +86,38 @@ 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('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 @@ class ViewControllerTest extends TestCase { $this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView')); } - public function testShowFileRouteWithTrashedFile(): void { - $this->appManager->expects($this->once()) + 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->exactly(2)) ->method('isEnabledForUser') - ->with('files_trashbin') ->willReturn(true); $parentNode = $this->getMockBuilder(Folder::class)->getMock(); @@ -189,13 +281,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 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')); } } diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 886e70c9a35..3277b983d0f 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -160,11 +160,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'); @@ -486,7 +487,7 @@ class Router implements IRouter { * @param string $appName */ private function requireRouteFile($file, $appName) { - $this->setupRoutes(include_once $file, $appName); + $this->setupRoutes(include $file, $appName); }