Skip to content

Commit 93fc392

Browse files
skjnldsvskjnldsv
committed
fix(files): properly forward open params from short urls
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
1 parent 20cedce commit 93fc392

File tree

4 files changed

+150
-49
lines changed

4 files changed

+150
-49
lines changed

‎apps/files/lib/Controller/ViewController.php‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -82,16 +82,16 @@ protected function getStorageInfo(string $dir = '/') {
8282
*/
8383
#[NoAdminRequired]
8484
#[NoCSRFRequired]
85-
public function showFile(?string $fileid = null): Response {
85+
public function showFile(?string $fileid = null, $opendetails = null, $openfile = null): Response {
8686
if (!$fileid) {
8787
return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
8888
}
8989

9090
// This is the entry point from the `/f/{fileid}` URL which is hardcoded in the server.
9191
try {
92-
return $this->redirectToFile((int)$fileid);
92+
return $this->redirectToFile((int)$fileid, $opendetails, $openfile);
9393
} catch (NotFoundException $e) {
94-
return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', ['fileNotFound' => true]));
94+
return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index'));
9595
}
9696
}
9797

@@ -100,38 +100,35 @@ public function showFile(?string $fileid = null): Response {
100100
* @param string $dir
101101
* @param string $view
102102
* @param string $fileid
103-
* @param bool $fileNotFound
104103
* @return TemplateResponse|RedirectResponse
105104
*/
106105
#[NoAdminRequired]
107106
#[NoCSRFRequired]
108-
public function indexView($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
109-
return $this->index($dir, $view, $fileid, $fileNotFound);
107+
public function indexView($dir = '', $view = '', $fileid = null) {
108+
return $this->index($dir, $view, $fileid);
110109
}
111110

112111
/**
113112
* @param string $dir
114113
* @param string $view
115114
* @param string $fileid
116-
* @param bool $fileNotFound
117115
* @return TemplateResponse|RedirectResponse
118116
*/
119117
#[NoAdminRequired]
120118
#[NoCSRFRequired]
121-
public function indexViewFileid($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
122-
return $this->index($dir, $view, $fileid, $fileNotFound);
119+
public function indexViewFileid($dir = '', $view = '', $fileid = null) {
120+
return $this->index($dir, $view, $fileid);
123121
}
124122

125123
/**
126124
* @param string $dir
127125
* @param string $view
128126
* @param string $fileid
129-
* @param bool $fileNotFound
130127
* @return TemplateResponse|RedirectResponse
131128
*/
132129
#[NoAdminRequired]
133130
#[NoCSRFRequired]
134-
public function index($dir = '', $view = '', $fileid = null, $fileNotFound = false) {
131+
public function index($dir = '', $view = '', $fileid = null) {
135132
if ($fileid !== null && $view !== 'trashbin') {
136133
try {
137134
return $this->redirectToFileIfInTrashbin((int)$fileid);
@@ -159,8 +156,6 @@ public function index($dir = '', $view = '', $fileid = null, $fileNotFound = fal
159156
if (count($nodes) === 1 && $relativePath !== $dir && $nodePath !== $dir) {
160157
return $this->redirectToFile((int)$fileid);
161158
}
162-
} else { // fileid does not exist anywhere
163-
$fileNotFound = true;
164159
}
165160
}
166161

@@ -249,10 +244,12 @@ private function redirectToFileIfInTrashbin($fileId): RedirectResponse {
249244
* Redirects to the file list and highlight the given file id
250245
*
251246
* @param int $fileId file id to show
247+
* @param string|null $openDetails open details parameter
248+
* @param string|null $openFile open file parameter
252249
* @return RedirectResponse redirect response or not found response
253250
* @throws NotFoundException
254251
*/
255-
private function redirectToFile(int $fileId) {
252+
private function redirectToFile(int $fileId, $openDetails = null, $openFile = null): RedirectResponse {
256253
$uid = $this->userSession->getUser()->getUID();
257254
$baseFolder = $this->rootFolder->getUserFolder($uid);
258255
$node = $baseFolder->getFirstNodeById($fileId);
@@ -274,6 +271,19 @@ private function redirectToFile(int $fileId) {
274271
// open the file by default (opening the viewer)
275272
$params['openfile'] = 'true';
276273
}
274+
275+
// Forward open parameters if any.
276+
// - openfile is true by default
277+
// - opendetails is undefined by default
278+
// - both will be evaluated as truthy
279+
if ($openDetails !== null) {
280+
$params['opendetails'] = $openDetails !== 'false' ? 'true' : 'false';
281+
}
282+
283+
if ($openFile !== null) {
284+
$params['openfile'] = $openFile !== 'false' ? 'true' : 'false';
285+
}
286+
277287
return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.indexViewFileid', $params));
278288
}
279289

‎apps/files/src/components/FilesListVirtual.vue‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -220,23 +220,25 @@ export default defineComponent({
220220
},
221221
222222
openFile: {
223-
async handler(openFile) {
223+
handler(openFile) {
224224
if (!openFile || !this.fileId) {
225225
return
226226
}
227227
228-
await this.handleOpenFile(this.fileId)
228+
this.handleOpenFile(this.fileId)
229229
},
230230
immediate: true,
231231
},
232232
233233
openDetails: {
234-
handler() {
234+
handler(openDetails) {
235235
// wait for scrolling and updating the actions to settle
236236
this.$nextTick(() => {
237-
if (this.fileId && this.openDetails) {
238-
this.openSidebarForFile(this.fileId)
237+
if (!openDetails || !this.fileId) {
238+
return
239239
}
240+
241+
this.openSidebarForFile(this.fileId)
240242
})
241243
},
242244
immediate: true,
@@ -276,7 +278,9 @@ export default defineComponent({
276278
if (node && sidebarAction?.enabled?.([node], this.currentView)) {
277279
logger.debug('Opening sidebar on file ' + node.path, { node })
278280
sidebarAction.exec(node, this.currentView, this.currentFolder.path)
281+
return
279282
}
283+
logger.error(`Failed to open sidebar on file ${fileId}, file isn't cached yet !`, { fileId, node })
280284
},
281285
282286
scrollToFile(fileId: number|null, warn = true) {

‎apps/files/tests/Controller/ViewControllerTest.php‎

Lines changed: 114 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
namespace OCA\Files\Tests\Controller;
99

1010
use OC\Files\FilenameValidator;
11+
use OC\Route\Router;
12+
use OC\URLGenerator;
1113
use OCA\Files\Controller\ViewController;
1214
use OCA\Files\Service\UserConfig;
1315
use OCA\Files\Service\ViewConfig;
@@ -16,66 +18,106 @@
1618
use OCP\AppFramework\Http\RedirectResponse;
1719
use OCP\AppFramework\Http\TemplateResponse;
1820
use OCP\AppFramework\Services\IInitialState;
21+
use OCP\Diagnostics\IEventLogger;
1922
use OCP\EventDispatcher\IEventDispatcher;
2023
use OCP\Files\File;
2124
use OCP\Files\Folder;
2225
use OCP\Files\IRootFolder;
2326
use OCP\Files\Template\ITemplateManager;
27+
use OCP\ICacheFactory;
2428
use OCP\IConfig;
2529
use OCP\IL10N;
2630
use OCP\IRequest;
2731
use OCP\IURLGenerator;
2832
use OCP\IUser;
2933
use OCP\IUserSession;
3034
use PHPUnit\Framework\MockObject\MockObject;
35+
use Psr\Container\ContainerInterface;
36+
use Psr\Log\LoggerInterface;
3137
use Test\TestCase;
3238

3339
/**
3440
* Class ViewControllerTest
3541
*
42+
* @group RoutingWeirdness
43+
*
3644
* @package OCA\Files\Tests\Controller
3745
*/
3846
class ViewControllerTest extends TestCase {
39-
private IRequest&MockObject $request;
40-
private IURLGenerator&MockObject $urlGenerator;
41-
private IL10N&MockObject $l10n;
47+
private ContainerInterface&MockObject $container;
48+
private IAppManager&MockObject $appManager;
49+
private ICacheFactory&MockObject $cacheFactory;
4250
private IConfig&MockObject $config;
4351
private IEventDispatcher $eventDispatcher;
44-
private IUser&MockObject $user;
45-
private IUserSession&MockObject $userSession;
46-
private IAppManager&MockObject $appManager;
47-
private IRootFolder&MockObject $rootFolder;
52+
private IEventLogger&MockObject $eventLogger;
4853
private IInitialState&MockObject $initialState;
54+
private IL10N&MockObject $l10n;
55+
private IRequest&MockObject $request;
56+
private IRootFolder&MockObject $rootFolder;
4957
private ITemplateManager&MockObject $templateManager;
58+
private IURLGenerator $urlGenerator;
59+
private IUser&MockObject $user;
60+
private IUserSession&MockObject $userSession;
61+
private LoggerInterface&MockObject $logger;
5062
private UserConfig&MockObject $userConfig;
5163
private ViewConfig&MockObject $viewConfig;
64+
private Router $router;
5265

5366
private ViewController&MockObject $viewController;
5467

5568
protected function setUp(): void {
5669
parent::setUp();
57-
$this->request = $this->getMockBuilder(IRequest::class)->getMock();
58-
$this->urlGenerator = $this->getMockBuilder(IURLGenerator::class)->getMock();
59-
$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
60-
$this->config = $this->getMockBuilder(IConfig::class)->getMock();
70+
$this->appManager = $this->createMock(IAppManager::class);
71+
$this->config = $this->createMock(IConfig::class);
6172
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
62-
$this->userSession = $this->getMockBuilder(IUserSession::class)->getMock();
63-
$this->appManager = $this->getMockBuilder('\OCP\App\IAppManager')->getMock();
73+
$this->initialState = $this->createMock(IInitialState::class);
74+
$this->l10n = $this->createMock(IL10N::class);
75+
$this->request = $this->createMock(IRequest::class);
76+
$this->rootFolder = $this->createMock(IRootFolder::class);
77+
$this->templateManager = $this->createMock(ITemplateManager::class);
78+
$this->userConfig = $this->createMock(UserConfig::class);
79+
$this->userSession = $this->createMock(IUserSession::class);
80+
$this->viewConfig = $this->createMock(ViewConfig::class);
81+
6482
$this->user = $this->getMockBuilder(IUser::class)->getMock();
6583
$this->user->expects($this->any())
6684
->method('getUID')
6785
->willReturn('testuser1');
6886
$this->userSession->expects($this->any())
6987
->method('getUser')
7088
->willReturn($this->user);
71-
$this->rootFolder = $this->getMockBuilder('\OCP\Files\IRootFolder')->getMock();
72-
$this->initialState = $this->createMock(IInitialState::class);
73-
$this->templateManager = $this->createMock(ITemplateManager::class);
74-
$this->userConfig = $this->createMock(UserConfig::class);
75-
$this->viewConfig = $this->createMock(ViewConfig::class);
7689

77-
$filenameValidator = $this->createMock(FilenameValidator::class);
90+
// Make sure we know the app is enabled
91+
$this->appManager->expects($this->any())
92+
->method('cleanAppId')
93+
->willReturnArgument(0);
94+
$this->appManager->expects($this->any())
95+
->method('getAppPath')
96+
->willReturnCallback(fn (string $appid): string => \OC::$SERVERROOT . '/apps/' . $appid);
97+
98+
$this->cacheFactory = $this->createMock(ICacheFactory::class);
99+
$this->logger = $this->createMock(LoggerInterface::class);
100+
$this->eventLogger = $this->createMock(IEventLogger::class);
101+
$this->container = $this->createMock(ContainerInterface::class);
102+
$this->router = new Router(
103+
$this->logger,
104+
$this->request,
105+
$this->config,
106+
$this->eventLogger,
107+
$this->container,
108+
$this->appManager,
109+
);
78110

111+
// Create a real URLGenerator instance to generate URLs
112+
$this->urlGenerator = new URLGenerator(
113+
$this->config,
114+
$this->userSession,
115+
$this->cacheFactory,
116+
$this->request,
117+
$this->router
118+
);
119+
120+
$filenameValidator = $this->createMock(FilenameValidator::class);
79121
$this->viewController = $this->getMockBuilder(ViewController::class)
80122
->setConstructorArgs([
81123
'files',
@@ -147,10 +189,60 @@ public function testIndexWithRegularBrowser(): void {
147189
$this->assertEquals($expected, $this->viewController->index('MyDir', 'MyView'));
148190
}
149191

192+
public function dataTestShortRedirect(): array {
193+
// openfile is true by default
194+
// opendetails is undefined by default
195+
// both will be evaluated as truthy
196+
return [
197+
[null, null, '/index.php/apps/files/files/123456?openfile=true'],
198+
['', null, '/index.php/apps/files/files/123456?openfile=true'],
199+
[null, '', '/index.php/apps/files/files/123456?openfile=true&opendetails=true'],
200+
['', '', '/index.php/apps/files/files/123456?openfile=true&opendetails=true'],
201+
['false', '', '/index.php/apps/files/files/123456?openfile=false'],
202+
[null, 'false', '/index.php/apps/files/files/123456?openfile=true&opendetails=false'],
203+
['true', 'false', '/index.php/apps/files/files/123456?openfile=true&opendetails=false'],
204+
['false', 'true', '/index.php/apps/files/files/123456?openfile=false&opendetails=true'],
205+
['false', 'false', '/index.php/apps/files/files/123456?openfile=false&opendetails=false'],
206+
];
207+
}
208+
209+
/**
210+
* @dataProvider dataTestShortRedirect
211+
*/
212+
public function testShortRedirect($openfile, $opendetails, $result) {
213+
$this->appManager->expects($this->any())
214+
->method('isEnabledForUser')
215+
->with('files')
216+
->willReturn(true);
217+
218+
$baseFolderFiles = $this->getMockBuilder(Folder::class)->getMock();
219+
$this->rootFolder->expects($this->any())
220+
->method('getUserFolder')
221+
->with('testuser1')
222+
->willReturn($baseFolderFiles);
223+
224+
$parentNode = $this->getMockBuilder(Folder::class)->getMock();
225+
$parentNode->expects($this->once())
226+
->method('getPath')
227+
->willReturn('testuser1/files/Folder');
228+
229+
$node = $this->getMockBuilder(File::class)->getMock();
230+
$node->expects($this->once())
231+
->method('getParent')
232+
->willReturn($parentNode);
233+
234+
$baseFolderFiles->expects($this->any())
235+
->method('getFirstNodeById')
236+
->with(123456)
237+
->willReturn($node);
238+
239+
$response = $this->viewController->showFile(123456, $opendetails, $openfile);
240+
$this->assertStringContainsString($result, $response->getHeaders()['Location']);
241+
}
242+
150243
public function testShowFileRouteWithTrashedFile(): void {
151-
$this->appManager->expects($this->once())
244+
$this->appManager->expects($this->exactly(2))
152245
->method('isEnabledForUser')
153-
->with('files_trashbin')
154246
->willReturn(true);
155247

156248
$parentNode = $this->getMockBuilder(Folder::class)->getMock();
@@ -189,13 +281,7 @@ public function testShowFileRouteWithTrashedFile(): void {
189281
->with('testuser1/files_trashbin/files/test.d1462861890/sub')
190282
->willReturn('/test.d1462861890/sub');
191283

192-
$this->urlGenerator
193-
->expects($this->once())
194-
->method('linkToRoute')
195-
->with('files.view.indexViewFileid', ['view' => 'trashbin', 'dir' => '/test.d1462861890/sub', 'fileid' => '123'])
196-
->willReturn('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
197-
198-
$expected = new RedirectResponse('/apps/files/trashbin/123?dir=/test.d1462861890/sub');
284+
$expected = new RedirectResponse('/index.php/apps/files/trashbin/123?dir=/test.d1462861890/sub');
199285
$this->assertEquals($expected, $this->viewController->index('', '', '123'));
200286
}
201287
}

‎lib/private/Route/Router.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,11 +160,12 @@ public function loadRoutes($app = null) {
160160
$this->root->addCollection($collection);
161161
}
162162
}
163+
163164
if (!isset($this->loadedApps['core'])) {
164165
$this->loadedApps['core'] = true;
165166
$this->useCollection('root');
166167
$this->setupRoutes($this->getAttributeRoutes('core'), 'core');
167-
require_once __DIR__ . '/../../../core/routes.php';
168+
require __DIR__ . '/../../../core/routes.php';
168169

169170
// Also add the OCS collection
170171
$collection = $this->getCollection('root.ocs');
@@ -486,7 +487,7 @@ private function getAttributeRoutes(string $app): array {
486487
* @param string $appName
487488
*/
488489
private function requireRouteFile($file, $appName) {
489-
$this->setupRoutes(include_once $file, $appName);
490+
$this->setupRoutes(include $file, $appName);
490491
}
491492

492493

0 commit comments

Comments
 (0)