Do not load PDF viewer if public share has a download limit #654
Conversation
|
/backport to stable24 |
|
/backport to stable23 |
| // If the event has a scope it is not the default share page but, for | ||
| // example, the authentication page | ||
| if ($event->getScope() !== null) { | ||
| return; |
There was a problem hiding this comment.
Are we sure no other area uses the Template without a scope?
There was a problem hiding this comment.
In server OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent is emitted only by the ShareController.
Is it emitted by any other app? I do not know, but in that case it would be the same as before, the JavaScript code would check if it is a public share page and if it is not the PDF viewer will not be loaded.
| private function getDownloadLimit(string $shareToken): int { | ||
| try { | ||
| $limitMapper = $this->serverContainer->get('\OCA\Files_DownloadLimit\Db\LimitMapper'); | ||
| } catch (QueryException $e) { | ||
| return -1; | ||
| } | ||
|
|
||
| try { | ||
| $shareLimit = $limitMapper->get($shareToken); | ||
| } catch (\Exception $e) { | ||
| return -1; | ||
| } | ||
|
|
||
| return $shareLimit->getLimit(); | ||
| } |
There was a problem hiding this comment.
I don't like this, this is an anti pattern. Using another app as a dependency is a no-go from me.
If there is a limit, I would make the Files_DownloadLimit handle the event and do its changes though there ?
How do we manage for images/video for example?
There was a problem hiding this comment.
I don't like this, this is an anti pattern. Using another app as a dependency is a no-go from me.
If there is a limit, I would make the Files_DownloadLimit handle the event and do its changes though there ?
Sorry, I did not understand that. How would you prevent the PDF viewer from being loaded by handling the event in the files_downloadlimit app? And wouldn't that make the files_Ddwnloadlimit app also depend on another app?
How do we manage for images/video for example?
Apparently we do not 🤷 I have just checked and the same issue happens with images and videos. In fact, with videos is even worse, because they are downloaded in several chunks, so just opening the public share page of a video reduces the download limit several times.
There was a problem hiding this comment.
maybe we need a new event:
- define a new event BeforeDownloadFile with a method
isMetered()or something that tells us if the download is counted (download limit) or completely blocked (when download limit reached or "secure view" aka "view-only" aka "download forbidden"): - then trigger the event in the PDF viewer (and any other places that want to query that), like: https://github.com/nextcloud/server/blob/v25.0.0/apps/dav/lib/Controller/DirectController.php#L113
- and here put the handler in the download limit app: https://github.com/nextcloud/server/blob/v25.0.0/apps/files_sharing/lib/AppInfo/Application.php#L167
we also have one for when a Zip file is getting created
There was a problem hiding this comment.
The event sounds like a good approach 👍
However, please note that the reason to implement the check directly in the PDF viewer without touching anything else was to be able to easily backport the fix to previous versions.
If I am not mistaken the APIs are not allowed to be changed (not even extended) in maintenance versions. I guess it would be possible to make an exception and allow adding a new event class for this. But if an event class is added only from certain maintenance version, how to handle that in the apps? Just by explicitly checking if the class exists before using it? And could anything else break due to that new event?
There was a problem hiding this comment.
However, please note that the reason to implement the check directly in the PDF viewer without touching anything else was to be able to easily backport the fix to previous versions.
@skjnldsv since you objected to cross-app dependency, are you ok with the approach for backportability or have suggestions for further tweaks ?
@danxuliu I think we can go "clean approach" for Nextcloud 26 with the new event on master and keep your code here for Nextcloud <= 25
The PDF viewer is initialized only in public share pages for PDF files, but it was loaded in all public pages (for example, in the public profile page). To reduce the number of unneeded loads now it is loaded only in public share pages instead. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The PDF viewer needs to download the file in order to render it in the client. Due to this, if a public share has a download limit, loading the PDF viewer automatically reduces the number of available downloads by one, even if the user does not download the file manually. To prevent that now the PDF viewer is not loaded in public shares if the share has a download limit. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
danxuliu
force-pushed
the
do-not-load-pdf-viewer-if-public-share-has-a-download-limit
branch
from
7b062d7
to
77f4c0c
Compare
|
I have force pushed to remove a no longer needed import in the first commit. I did not add that as a fixup as it would conflict with the second commit. |
|
What is missing here? |
|
did we explore more potential solutions for solving this and their pros and cons ? some further potential ideas:
|
|
the header could state the app name that is downloading the file, ex: "X-NC-Viewer: files_pdfviewer", might be useful for other things also like debugging or stats in the future the download limit app could simply check if "X-NC-Viewer" is set regardless of value the cool thing is that it still protects against people linking to files from another place like forums or so, because the link doesn't contain headers |
|
seems the header approach won't work as the pdf viewer is rendered in an iframe, so there's no way to inject headers :-/ |
|
seems there's a way: mozilla/pdf.js#11792 (comment) |
|
Doesn't looks like we are using |
I guess that But please note that I have not actually verified it, it is just my guess from the code :-) |
|
Is this still something we should consider? There is not much activity for the last ~1.5 years. |
The PDF viewer needs to download the file in order to render it in the client. Due to this, if a public share has a download limit, loading the PDF viewer automatically reduces the number of available downloads by one, even if the user does not download the file manually. To prevent that now the PDF viewer is not loaded in public shares if the share has a download limit.
In order to accommodate that change, but also to prevent loading the PDF viewer when not needed, the PDF viewer is now loaded only in public share pages instead of in all public pages (which also matches with the existing JavaScript behaviour, as it was initialized only in public shares for PDF files and ignored in the other public pages).
How to test
Result with this pull request
The number of remaining downloads has not changed; the PDF viewer was not loaded in the public share page.
Result without this pull request
The number of remaining downloads was reduced by one, because the PDF viewer was loaded in the public share page and therefore the file was implicitly downloaded.