-
Notifications
You must be signed in to change notification settings - Fork 42
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
Show error when trying to open a shared PDF without download permissions #1077
Show error when trying to open a shared PDF without download permissions #1077
Conversation
/compile / |
/backport to stable30 |
/backport to stable29 |
/backport to stable28 |
src/views/PDFView.vue
Outdated
<div v-if="!isDownloadable" | ||
id="emptycontent"> | ||
<div class="icon-error" /> | ||
<h2>{{ t('files_pdfviewer', 'To view a shared PDF file the sharer needs to allow downloads of the file') }}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To view a shared PDF file, the owner must allow a download of the file
?
There is also a case, when files_pdfviewer is not in the whitelist of Guests app, and guest account is trying to access it with 403.
Possibly unrelated, but I could follow-up on top of that PR later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To view a shared PDF file, the owner must allow a download of the file
?
Mmm, maybe the person should not be mentioned at all, because if a file is reshared and the owner disallows the download I guess (I have not checked) that it will be disallowed in the reshare, but if the owner allows it and the resharer disallows it then it would be also disallowed 🤔
There is also a case, when files_pdfviewer is not in the whitelist of Guests app, and guest account is trying to access it with 403. Possibly unrelated, but I could follow-up on top of that PR later
Thanks, I was not aware of that. A follow up sounds good :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any news on the follow up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR covers all shares (with users, guests, oublic links), where download is disabled.
For guests accounts + shareswith enabled download, it returns 500 Server error, which is correct, and not to be handled by PDF Viewer.
Instead, this app should be whitelisted in admin settings. Issue is tracked here: nextcloud/guests#1249
Hi @danxuliu, I wanted to test this. Unfortunately when I try to install the pdfviewer dependencies, I run into the following error: This should be easily reproducible with
|
Does it work with master/main branch of pdf_viewer? As there is only frontend changes and composer dependencies shouldn't affect it Tested with Julius' docker, Viewer content is replaced: |
@Antreesy since you were able to test this, can you approve or is something missing from your Pov? |
"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 <danxuliu@gmail.com>
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 <danxuliu@gmail.com>
762d781
to
44b9067
Compare
/compile / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased, recompiled, adjusted wording to user-insensitive: To view a shared PDF file, the download needs to be allowed for this file share
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Thanks @Antreesy |
Fixes #649
Although the best fix would be to fallback to Collabora if available currently it does not seem to be possible to do that, so for now just an error message is shown.
Also, while it would be possible to use
this.$emit('error')
to notify the viewer that the file could not be loaded currently it does not seem possible to customize the message, so the standard Viewer error with Error loading {name of the file} would be shown. In this case it is worth explaning the reason, so the error is handled directly in the PDF viewer instead (but with the markup and style for the error element copied from the viewer). Maybe custom error messages will be possible in the future with the Viewer 4.0 API and then that code can go away :-)Regarding the error message itself feel free to improve the wording :-) After several tries this is the best I could come up with 🤷
Finally, note that before Nextcloud 28 when the PDF viewer was opened in public pages for a single share the UI was directly injected. When it was changed to use the viewer instead a fallback to directly inject the UI was left just in case the viewer was not available. This pull request only handles the disabled download permissions when the viewer is used, but not in the fallback case:
public.js
will go away (in another pull request) because it is no longer needed, so there will be no fallback at all. But also in Nextcloud 28 to 30 the fallback should never be used anyway; the viewer is expected to be always available, and if it is not or it did not properly load then something would need to be fixed, but keeping the fallback was just being overly cautious and it is not really needed.How to test
Allow download and sync
Result with this pull request
Result without this pull request
The PDF viewer is loaded, but contents are empty