Fix downloading files from public share

[danxuliu]

Fixes #3442, which is a regression introduced in 4a65c8c

When a file is open in the viewer the Photos app provides an object with the file information. The object includes the source attribute, which is used to get the URL to download the file open in the viewer.

Before 4a65c8c the source was created by appending the file name (/photospublic/TOKEN/FILE) to the remote DAV URL (https://DOMAIN/remote.php/dav). In 4a65c8c1b7b8f5016d1189a0f4eb403be77bfaee instead of the remote DAV URL defined in the app the default remote URL provided by @nextcloud/files/dav is used, which in the case of public pages is https://DOMAIN/public.php/dav. However, the file name is not just appended to the default remote URL, but generated through a URL object. As the file name is relative to the root only the site part of the base URL is used (in this case, the default remote URL), so the final "assembled" URL becomes https://DOMAIN/photospublic/TOKEN/file.

A bit later, in 999d49b, genFileInfo was replaced with resultToNode from @nextcloud/files/dav, which composes the source attribute by appending the file name to the default remote URL, so it becomes https://DOMAIN/public.php/dav/photospublic/TOKEN/FILE. While it is now using a "valid" Nextcloud URL the shared files can not be downloaded from it.

The problem with /public.php/dav is that it can be used only for "normal" public shares, as it uses a custom DAV server that does not load the additional authentication backends and always uses the public DAV authentication, which treats the token as a share token and therefore fails. On the other hand, /remote.php/dav uses a full DAV server, which loads the additional authentication backends and therefore allows access to /photospublic/ URLs.

While it might make sense to explicitly support additional authentication backends in /public.php/dav I am not familiar with any of this to know if it would be the right approach or not. Moreover, the DAV client used for public albums explicitly uses /remote.php/dav rather than the default remote URL (/public.php/dav), so for now the provided fix just passes /remote.php/dav to resultToNode when generating the album node. Note that there are other resultToNode calls throughout the code, although I only touched the one that fixes the reproduction steps below (and the added E2E test). It might be necessary to change others as well.

Another possibility to fix this without explicitly passing the remote URL would have been to remove the isPublic element from the template, so @nextcloud/files does not replace remote.php with public.php. However, conceptually it makes sense to provide the isPublic element.

But, moreover, that seems to actually be a brittle hack. Apparently the isPublic element would still be needed in PhotosAppPublic.vue (not only based on the comment, but also because if it is removed the public share no longer works); I have not digged into why that one is not used by @nextcloud/files, or why the one in the template is not used by whatever uses the one in PhotosAppPublic.vue, but it seems that the element from the template is used only during the app startup or something like that 🤔

Independently of that, note that CI will keep failing until the breaking API change in server is fixed, either by merging #3475 or by adjusting the API change in server to be backwards compatible. For now this pull request is just based on #3475 to be able to run the tests.

How to test

• Create an album
• Add photos to the album
• Create a public share for the album
• In a private window, open the link to the public share
• Open a photo in the viewer
• Download the photo

Result with this pull request

The photo is download

Result without this pull request

A 503 Service Unavailable error is returned

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[danxuliu]

/compile /

[danxuliu]

/backport to stable33 please

[danxuliu]

/backport to stable32 please

[artonge]

Looks good, any reason this is still a draft?

[danxuliu]

Looks good, any reason this is still a draft?

I forgot to mention it in the pull request description, sorry 🤦 I kept it as a draft to ensure that it would not be merged before rebasing first onto master (as it is currently based on #3475, but that is something independent).

Great, and now I undrafted it by mistake with the touchpad 😅 Anyway, as it has conflicts I will rebase onto master and recompile.

[danxuliu]

/compile /

[danxuliu]

Please note that I can not merge myself :-) , as the CI will be red until #3475 is merged or the API break is fixed in server 🤷

[artonge]

What's preventing us to merge #3475?

[danxuliu]

What's preventing us to merge #3475?

I kept it as draft because I do not know if the API break will be fixed in server master or not, and if the break is fixed then #3475 is not needed. But... even if it is fixed the change in #3475, although unneeded, would still work. So I have removed the draft state in case you want to merge :-)