Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
skjnldsv merged 3 commits into from
Feb 18, 2025
Merged

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

skjnldsv merged 3 commits into from
Feb 18, 2025

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Feb 14, 2025 •
edited
Loading

Fix #50155

@skjnldsv skjnldsv force-pushed the fix/redirect-openfile-param branch from 93c4754 to 0aeb29e Compare February 14, 2025 09:47
@skjnldsv skjnldsv self-assigned this Feb 14, 2025
@skjnldsv skjnldsv added this to the Nextcloud 31 milestone Feb 14, 2025
@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 14, 2025

/backport to stable31

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 14, 2025

/backport to stable30

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 14, 2025

/backport to stable29

@skjnldsv skjnldsv force-pushed the fix/redirect-openfile-param branch from 0aeb29e to adcd9b9 Compare February 14, 2025 09:49
@skjnldsv skjnldsv marked this pull request as ready for review February 14, 2025 09:50
@skjnldsv skjnldsv force-pushed the fix/redirect-openfile-param branch from adcd9b9 to 70394e0 Compare February 14, 2025 09:50
skjnldsv
skjnldsv commented Feb 14, 2025
View reviewed changes
lib/private/Route/Router.php
$this->useCollection('root');
$this->setupRoutes($this->getAttributeRoutes('core'), 'core');
require_once __DIR__ . '/../../../core/routes.php';
require __DIR__ . '/../../../core/routes.php';
Copy link
Member Author

@skjnldsv skjnldsv Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an important point.
After a debug session with Christoph, we realised that it prevented routes.php files to be included again when running tests. Making any subsequent route generation fail.

Considering we already test that app have been loaded with isset($this->loadedApps['core']) and isset($this->loadedApps[$app]), we assumed it was safe to require/include without _once.

@skjnldsv skjnldsv force-pushed the fix/redirect-openfile-param branch 2 times, most recently from d4e9a9c to 93fc392 Compare February 14, 2025 10:15
@skjnldsv skjnldsv requested review from a team, ArtificialOwl, ChristophWurst, come-nc, icewind1991 and susnux and removed request for a team February 14, 2025 10:15
@skjnldsv skjnldsv mentioned this pull request Feb 14, 2025
Closed
8 tasks
susnux
susnux reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully sure as the opendetails is a new feature. Also I do not see any reason for neither having one of them for this endpoint.

Meaning: if openfile is false set opendetails, setting both or none makes no sense.

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025

Meaning: if openfile is false set opendetails, setting both or none makes no sense.

I don't understand this sentence.
openfile and opendetails are two different properties, they have no link between each others.
you can absolkutely have opendetails to true and openfile to false 🤔

come-nc
come-nc reviewed Feb 18, 2025
View reviewed changes
come-nc
come-nc reviewed Feb 18, 2025
View reviewed changes
come-nc
come-nc reviewed Feb 18, 2025
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with susnux comments about adding typing.
The logic around opendetails/openfile looks solid, so my only blocker here is this fileNotFound situation.

@skjnldsv skjnldsv force-pushed the fix/redirect-openfile-param branch from 93fc392 to 66c8e70 Compare February 18, 2025 10:09
@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025

Reference in new issue

Addressed and added dedicated cypress tests!

@skjnldsv skjnldsv requested review from come-nc and susnux February 18, 2025 10:09
@susnux
Copy link
Contributor

susnux commented Feb 18, 2025

I don't understand this sentence.
openfile and opendetails are two different properties, they have no link between each others.
you can absolkutely have opendetails to true and openfile to false 🤔

Not sure about all apps (if you can control the sidebar when the file is opened), but if thats the case then maybe.

But my point was more like: That "internal link" feature make no sense with openfile=false&opendetails=false it also makes rather no sense with openfile&opendetails.
So what I mean is:

  • if /f/1234?openfile
    • then /apps/files/files/1234?openfile
  • if /f/1234
    • then /apps/files/files/1234?openfile (default open)
  • if /f/1234?openfile=false
    • then /apps/files/files/1234?opendetails

Meaning there is no need for opendetails on the /f/1234 endpoint as its value is implicit from the openfile value, because having both true or both false makes no sense for internal links.

That beeing said its no blocker from my side, having this new feature (parsing `opendetails´ on the internal-link endpoint) just makes no sense to me, but maybe I am overlooking something here 😅

susnux
susnux approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks sane ✅

apps/files/lib/Controller/ViewController.php
#[NoAdminRequired]
#[NoCSRFRequired]
public function showFile(?string $fileid = null): Response {
public function showFile(?string $fileid = null, ?string $opendetails = null, ?string $openfile = null): Response {
Copy link
Contributor

@susnux susnux Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is adding openfile here is a bugfix ✅
Adding opendetails is a feature I do not see the usecase on this endpoint.

Copy link
Member Author

@skjnldsv skjnldsv Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, it's kinda both you're right!
I'll adjust the backports to only cover the openfile anyway :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025 •
edited
Loading

Meaning there is no need for opendetails on the /f/1234 endpoint as its value is implicit from the openfile value, because having both true or both false makes no sense for internal links.

Ah, but this is needed, because otherwise the opendetails will be stripped from the URL.
Since We're fixing the open param for openfile, better do it properly and also cover the other open param: opendetails.

It's not about being a bugfix or not, it's mainly like: "since we're here, better do it properly and make it feature complete" :)

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025

/backport 2e50a39 4f795e1 to stable31

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025

/backport 2e50a39 4f795e1 to stable30

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 18, 2025

/backport 2e50a39 4f795e1 to stable29

@skjnldsv skjnldsv merged commit 5d1bc70 into master Feb 18, 2025
187 of 189 checks passed
@skjnldsv skjnldsv deleted the fix/redirect-openfile-param branch February 18, 2025 12:39
This was referenced Feb 18, 2025
Merged
Merged
Merged
@nextcloud-bot nextcloud-bot mentioned this pull request Aug 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux approved these changes

@come-nc come-nc come-nc approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

Assignees

@skjnldsv skjnldsv

Labels

3. to review Waiting for reviews bug feature: files

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

[Bug]: Internal links lose openfile parameter value

4 participants

@skjnldsv @susnux @come-nc