Skip to content

Do not load PDF viewer if public share has a download limit #654

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Do not load PDF viewer if public share has a download limit #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7ac1d65
Load PDF viewer only in public share pages
danxuliu Sep 6, 2022
77f4c0c
Do not load PDF viewer if public share has a download limit
danxuliu Sep 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
use OCA\Files_PDFViewer\Listeners\LoadPublicViewerListener;
use OCA\Files_PDFViewer\Listeners\LoadViewerListener;

use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent;
use OCA\Viewer\Event\LoadViewer;

use OCP\AppFramework\App;
use OCP\AppFramework\Bootstrap\IBootContext;
use OCP\AppFramework\Bootstrap\IBootstrap;
use OCP\AppFramework\Bootstrap\IRegistrationContext;
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
use OCP\Security\CSP\AddContentSecurityPolicyEvent;

class Application extends App implements IBootstrap {
Expand Down
40 changes: 36 additions & 4 deletions lib/Listeners/LoadPublicViewerListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,54 @@
namespace OCA\Files_PDFViewer\Listeners;

use OCA\Files_PDFViewer\AppInfo\Application;
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
use OCP\AppFramework\Http\TemplateResponse;
use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent;
use OCP\AppFramework\QueryException;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventListener;
use OCP\IServerContainer;
use OCP\Util;

class LoadPublicViewerListener implements IEventListener {

/** @var IServerContainer */
private $serverContainer;

public function __construct(IServerContainer $serverContainer) {
$this->serverContainer = $serverContainer;
}

public function handle(Event $event): void {
if (!$event instanceof BeforeTemplateRenderedEvent) {
return;
}

// Make sure we are on a public page rendering
if ($event->getResponse()->getRenderAs() !== TemplateResponse::RENDER_AS_PUBLIC) {
// If the event has a scope it is not the default share page but, for
// example, the authentication page
if ($event->getScope() !== null) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure no other area uses the Template without a scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

// Do not load the viewer if there is a download limit
if ($this->getDownloadLimit($event->getShare()->getToken()) >= 0) {
return;
}

Util::addScript(Application::APP_ID, 'files_pdfviewer-public');
}

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();
}
Comment on lines +65 to +79
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we need a new event:

we also have one for when a Zip file is getting created

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

}