Skip to content
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.

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

[stable30] Show error when trying to open a shared PDF without download permissions #1098

Open
wants to merge 5 commits into
base: stable30
Choose a base branch
from

Conversation

backportbot[bot]
Copy link

@backportbot backportbot bot commented Dec 7, 2024

Backport of #1077

Added extra commits due to differences between Nextcloud 31 and 30; for details please refer to #1098 (comment)

@backportbot backportbot bot requested review from Antreesy and danxuliu December 7, 2024 10:36
@backportbot backportbot bot added this to the Nextcloud 30.0.4 milestone Dec 7, 2024
@Antreesy Antreesy force-pushed the backport/1077/stable30 branch from 0f581e5 to 152cfc2 Compare December 9, 2024 11:48
@Antreesy
Copy link
Contributor

Antreesy commented Dec 9, 2024

/compile /

@Antreesy
Copy link
Contributor

Antreesy commented Dec 9, 2024

Tested for registered users, but public share links do not provide this.file.shareAttributes
Moreover, it is even possible to open preview (but not save/print/download)

"canDownload" inverted the value of the "hideDownload" setting. However,
that was not accurate, as if the download is hidden the file can still
be downloaded. Moreover, it is possible to actually disallow downloads,
which is a different setting (using share attributes) than hiding it.

Therefore, to better differentiate between a hidden download and a
disabled download the previous "canDownload" was renamed (and adjusted
as needed) to "hideDownload".

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
In order to show a PDF file it needs to be downloaded. Therefore, if a
shared PDF file does not have download permissions it is not possible to
show it, so now an error is shown instead.

The error is a custom one rather than a standard error from the viewer
(although with the same appearance) to better explain the reason.

Note that the error is shown only when the PDF file is loaded through
the viewer, which should be always the case. There is a fallback to
inject the UI in public shares in case the viewer is not available, but
handling the error also in that case was not trivial and that fallback
should never be used anyway, so it was not taken into account.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@nextcloud/files was already an implicit dependency, but now it is made
explicit to use the functions it provides in the PDF viewer code.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The viewer loads the default and registered DAV properties in the
FileInfo object provided to the PDF viewer. The "share-attributes"
property is needed to know if downloads are allowed, but it is not one
of the properties included by default. The Files app registers it during
its initialization, so it is automatically available when the PDF viewer
is opened in the Files app. However, when the PDF viewer is opened in
public share page nothing registers it, so it needs to be explicitly
registered by the PDF viewer itself before the viewer is loaded.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the backport/1077/stable30 branch from de64804 to 359f68c Compare December 31, 2024 06:44
@danxuliu
Copy link
Member

/compile /

@danxuliu
Copy link
Member

public share links do not provide this.file.shareAttributes

The viewer gets the list of DAV properties to load using @nextcloud/files. share-attributes is not in the list of default properties, so it needs to be explicitly registered. When the PDF viewer is opened in the Files app it is not needed, because the Files app registers the property; in Nextcloud 31 it also works in public share pages because they are now unified with the Files app, but that was not the case in previous Nextcloud versions. Therefore the DAV property needs to be explicitly registered in that case.

The DAV properties must be registered before the viewer is loaded, though, as the list of properties will be fixed once the viewer is initialized, they will not depend on the registered properties at the time of making the request to get the file info. Thus they need to be registered in an init script loaded when the LoadViewer event is handled (which probably works just because viewer is alphabetically after files_pdfviewer :see-no-evil:).

Moreover, it is even possible to open preview (but not save/print/download)

There is no way to disallow downloads for public shares in the UI; it needs to be done using an OCS API request. Due to that even if the error message was not shown in public pages with disallowed downloads it should not have been a problem :-) In Nextcloud 31 the UI did not change, but the API now disallows downloads when they are hidden, so in that case it was more relevant.

How to test

  • Upload a PDF file with something like curl --user admin:admin --upload-file {PATH-TO-THE-PDF-FILE-TO-UPLOAD} --header "OCS-APIRequest: true" "http://127.0.0.1:8000/remote.php/dav/files/admin/file.pdf"
  • Share it with downloads not allowed with something like curl --user admin:admin --request POST --header "OCS-APIRequest: true" "http://127.0.0.1:8000/ocs/v1.php/apps/files_sharing/api/v1/shares?path=file.pdf&shareType=3&attributes=\[\{\"scope\":\"permissions\",\"key\":\"download\",\"value\":false\}\]"
  • In a private window, open the URL of the share

Result with the extra commits

The error To view a shared PDF file, the download needs to be allowed for this file share is shown

Result without the extra commits

Nothing is shown in the PDF viewer

Signed-off-by: nextcloud-command <[email protected]>
@danxuliu
Copy link
Member

CI failure is unrelated and fixed in #1104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants