From 0ca5c7487af25886e02e6b52fed9873450b15965 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Sat, 20 Apr 2024 20:07:55 +0200 Subject: [PATCH] perf(files_sharing): Move events to listener classes and registration instead of boot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + .../files_sharing/lib/AppInfo/Application.php | 77 ++++--------------- .../BeforeDirectFileDownloadListener.php | 65 ++++++++++++++++ .../lib/Listener/BeforeZipCreatedListener.php | 76 ++++++++++++++++++ apps/files_sharing/tests/ApplicationTest.php | 42 +++++----- 6 files changed, 176 insertions(+), 88 deletions(-) create mode 100644 apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php create mode 100644 apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php diff --git a/apps/files_sharing/composer/composer/autoload_classmap.php b/apps/files_sharing/composer/composer/autoload_classmap.php index 142e13679178e..e1abddb3a6450 100644 --- a/apps/files_sharing/composer/composer/autoload_classmap.php +++ b/apps/files_sharing/composer/composer/autoload_classmap.php @@ -56,6 +56,8 @@ 'OCA\\Files_Sharing\\Hooks' => $baseDir . '/../lib/Hooks.php', 'OCA\\Files_Sharing\\ISharedMountPoint' => $baseDir . '/../lib/ISharedMountPoint.php', 'OCA\\Files_Sharing\\ISharedStorage' => $baseDir . '/../lib/ISharedStorage.php', + 'OCA\\Files_Sharing\\Listener\\BeforeDirectFileDownloadListener' => $baseDir . '/../lib/Listener/BeforeDirectFileDownloadListener.php', + 'OCA\\Files_Sharing\\Listener\\BeforeZipCreatedListener' => $baseDir . '/../lib/Listener/BeforeZipCreatedListener.php', 'OCA\\Files_Sharing\\Listener\\LoadAdditionalListener' => $baseDir . '/../lib/Listener/LoadAdditionalListener.php', 'OCA\\Files_Sharing\\Listener\\LoadSidebarListener' => $baseDir . '/../lib/Listener/LoadSidebarListener.php', 'OCA\\Files_Sharing\\Listener\\ShareInteractionListener' => $baseDir . '/../lib/Listener/ShareInteractionListener.php', diff --git a/apps/files_sharing/composer/composer/autoload_static.php b/apps/files_sharing/composer/composer/autoload_static.php index 087c7d4cd86ac..5d2fb3bac2a47 100644 --- a/apps/files_sharing/composer/composer/autoload_static.php +++ b/apps/files_sharing/composer/composer/autoload_static.php @@ -71,6 +71,8 @@ class ComposerStaticInitFiles_Sharing 'OCA\\Files_Sharing\\Hooks' => __DIR__ . '/..' . '/../lib/Hooks.php', 'OCA\\Files_Sharing\\ISharedMountPoint' => __DIR__ . '/..' . '/../lib/ISharedMountPoint.php', 'OCA\\Files_Sharing\\ISharedStorage' => __DIR__ . '/..' . '/../lib/ISharedStorage.php', + 'OCA\\Files_Sharing\\Listener\\BeforeDirectFileDownloadListener' => __DIR__ . '/..' . '/../lib/Listener/BeforeDirectFileDownloadListener.php', + 'OCA\\Files_Sharing\\Listener\\BeforeZipCreatedListener' => __DIR__ . '/..' . '/../lib/Listener/BeforeZipCreatedListener.php', 'OCA\\Files_Sharing\\Listener\\LoadAdditionalListener' => __DIR__ . '/..' . '/../lib/Listener/LoadAdditionalListener.php', 'OCA\\Files_Sharing\\Listener\\LoadSidebarListener' => __DIR__ . '/..' . '/../lib/Listener/LoadSidebarListener.php', 'OCA\\Files_Sharing\\Listener\\ShareInteractionListener' => __DIR__ . '/..' . '/../lib/Listener/ShareInteractionListener.php', diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index fc305e58dbe45..b01c9b4472d0e 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -38,6 +38,8 @@ use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\External\MountProvider as ExternalMountProvider; use OCA\Files_Sharing\Helper; +use OCA\Files_Sharing\Listener\BeforeDirectFileDownloadListener; +use OCA\Files_Sharing\Listener\BeforeZipCreatedListener; use OCA\Files_Sharing\Listener\LoadAdditionalListener; use OCA\Files_Sharing\Listener\LoadSidebarListener; use OCA\Files_Sharing\Listener\ShareInteractionListener; @@ -51,7 +53,6 @@ use OCA\Files_Sharing\Notification\Notifier; use OCA\Files_Sharing\ShareBackend\File; use OCA\Files_Sharing\ShareBackend\Folder; -use OCA\Files_Sharing\ViewOnly; use OCP\AppFramework\App; use OCP\AppFramework\Bootstrap\IBootContext; use OCP\AppFramework\Bootstrap\IBootstrap; @@ -62,13 +63,11 @@ use OCP\Files\Config\IMountProviderCollection; use OCP\Files\Events\BeforeDirectFileDownloadEvent; use OCP\Files\Events\BeforeZipCreatedEvent; -use OCP\Files\IRootFolder; use OCP\Group\Events\GroupChangedEvent; use OCP\Group\Events\GroupDeletedEvent; use OCP\Group\Events\UserAddedEvent; use OCP\IDBConnection; use OCP\IGroup; -use OCP\IUserSession; use OCP\Share\Events\ShareCreatedEvent; use OCP\User\Events\UserChangedEvent; use OCP\User\Events\UserDeletedEvent; @@ -108,12 +107,22 @@ function () use ($c) { $context->registerEventListener(UserDeletedEvent::class, DisplayNameCache::class); $context->registerEventListener(GroupChangedEvent::class, GroupDisplayNameCache::class); $context->registerEventListener(GroupDeletedEvent::class, GroupDisplayNameCache::class); + + // sidebar and files scripts + $context->registerEventListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class); + $context->registerEventListener(LoadSidebar::class, LoadSidebarListener::class); + $context->registerEventListener(ShareCreatedEvent::class, ShareInteractionListener::class); + $context->registerEventListener(ShareCreatedEvent::class, UserShareAcceptanceListener::class); + $context->registerEventListener(UserAddedEvent::class, UserAddedToGroupListener::class); + + // Handle download events for view only checks + $context->registerEventListener(BeforeZipCreatedEvent::class, BeforeZipCreatedListener::class); + $context->registerEventListener(BeforeDirectFileDownloadEvent::class, BeforeDirectFileDownloadListener::class); } public function boot(IBootContext $context): void { $context->injectFn([$this, 'registerMountProviders']); $context->injectFn([$this, 'registerEventsScripts']); - $context->injectFn([$this, 'registerDownloadEvents']); Helper::registerHooks(); @@ -128,12 +137,6 @@ public function registerMountProviders(IMountProviderCollection $mountProviderCo } public function registerEventsScripts(IEventDispatcher $dispatcher): void { - // sidebar and files scripts - $dispatcher->addServiceListener(LoadAdditionalScriptsEvent::class, LoadAdditionalListener::class); - $dispatcher->addServiceListener(LoadSidebar::class, LoadSidebarListener::class); - $dispatcher->addServiceListener(ShareCreatedEvent::class, ShareInteractionListener::class); - $dispatcher->addServiceListener(ShareCreatedEvent::class, UserShareAcceptanceListener::class); - $dispatcher->addServiceListener(UserAddedEvent::class, UserAddedToGroupListener::class); $dispatcher->addListener(ResourcesLoadAdditionalScriptsEvent::class, function () { \OCP\Util::addScript('files_sharing', 'collaboration'); }); @@ -159,58 +162,4 @@ public function registerEventsScripts(IEventDispatcher $dispatcher): void { $listener->userAddedToGroup($event); }); } - - public function registerDownloadEvents( - IEventDispatcher $dispatcher, - IUserSession $userSession, - IRootFolder $rootFolder - ): void { - - $dispatcher->addListener( - BeforeDirectFileDownloadEvent::class, - function (BeforeDirectFileDownloadEvent $event) use ($userSession, $rootFolder): void { - $pathsToCheck = [$event->getPath()]; - // Check only for user/group shares. Don't restrict e.g. share links - $user = $userSession->getUser(); - if ($user) { - $viewOnlyHandler = new ViewOnly( - $rootFolder->getUserFolder($user->getUID()) - ); - if (!$viewOnlyHandler->check($pathsToCheck)) { - $event->setSuccessful(false); - $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); - } - } - } - ); - - $dispatcher->addListener( - BeforeZipCreatedEvent::class, - function (BeforeZipCreatedEvent $event) use ($userSession, $rootFolder): void { - $dir = $event->getDirectory(); - $files = $event->getFiles(); - - $pathsToCheck = []; - foreach ($files as $file) { - $pathsToCheck[] = $dir . '/' . $file; - } - - // Check only for user/group shares. Don't restrict e.g. share links - $user = $userSession->getUser(); - if ($user) { - $viewOnlyHandler = new ViewOnly( - $rootFolder->getUserFolder($user->getUID()) - ); - if (!$viewOnlyHandler->check($pathsToCheck)) { - $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); - $event->setSuccessful(false); - } else { - $event->setSuccessful(true); - } - } else { - $event->setSuccessful(true); - } - } - ); - } } diff --git a/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php new file mode 100644 index 0000000000000..578f2bee8d899 --- /dev/null +++ b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php @@ -0,0 +1,65 @@ + + * + * @author John Molakvoæ + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Files_Sharing\Listener; + +use OCA\Files_Sharing\ViewOnly; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\Files\Events\BeforeDirectFileDownloadEvent; +use OCP\Files\IRootFolder; +use OCP\IUserSession; + +/** + * @template-implements IEventListener + */ +class BeforeDirectFileDownloadListener implements IEventListener { + + public function __construct( + private IUserSession $userSession, + private IRootFolder $rootFolder, + ) { + } + + public function handle(Event $event): void { + if (!($event instanceof BeforeDirectFileDownloadEvent)) { + return; + } + + $pathsToCheck = [$event->getPath()]; + // Check only for user/group shares. Don't restrict e.g. share links + $user = $this->userSession->getUser(); + if ($user) { + $viewOnlyHandler = new ViewOnly( + $this->rootFolder->getUserFolder($user->getUID()) + ); + if (!$viewOnlyHandler->check($pathsToCheck)) { + $event->setSuccessful(false); + $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); + } + } + } +} diff --git a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php new file mode 100644 index 0000000000000..c4f12963867ac --- /dev/null +++ b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php @@ -0,0 +1,76 @@ + + * + * @author John Molakvoæ + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Files_Sharing\Listener; + +use OCA\Files_Sharing\ViewOnly; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\Files\Events\BeforeZipCreatedEvent; +use OCP\Files\IRootFolder; +use OCP\IUserSession; + +/** + * @template-implements IEventListener + */ +class BeforeZipCreatedListener implements IEventListener { + + public function __construct( + private IUserSession $userSession, + private IRootFolder $rootFolder, + ) { + } + + public function handle(Event $event): void { + if (!($event instanceof BeforeZipCreatedEvent)) { + return; + } + + $dir = $event->getDirectory(); + $files = $event->getFiles(); + + $pathsToCheck = []; + foreach ($files as $file) { + $pathsToCheck[] = $dir . '/' . $file; + } + + // Check only for user/group shares. Don't restrict e.g. share links + $user = $this->userSession->getUser(); + if ($user) { + $viewOnlyHandler = new ViewOnly( + $this->rootFolder->getUserFolder($user->getUID()) + ); + if (!$viewOnlyHandler->check($pathsToCheck)) { + $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); + $event->setSuccessful(false); + } else { + $event->setSuccessful(true); + } + } else { + $event->setSuccessful(true); + } + } +} diff --git a/apps/files_sharing/tests/ApplicationTest.php b/apps/files_sharing/tests/ApplicationTest.php index 270a56504f97b..826adeecf5795 100644 --- a/apps/files_sharing/tests/ApplicationTest.php +++ b/apps/files_sharing/tests/ApplicationTest.php @@ -22,29 +22,24 @@ */ namespace OCA\Files_Sharing\Tests; -use OC\EventDispatcher\EventDispatcher; -use OC\Share20\Manager; use OCA\Files_Sharing\AppInfo\Application; +use OCA\Files_Sharing\Listener\BeforeDirectFileDownloadListener; +use OCA\Files_Sharing\Listener\BeforeZipCreatedListener; use OCA\Files_Sharing\SharedStorage; -use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\BeforeDirectFileDownloadEvent; use OCP\Files\Events\BeforeZipCreatedEvent; use OCP\Files\File; use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Storage\IStorage; -use OCP\IServerContainer; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IAttributes; use OCP\Share\IShare; -use Psr\Log\LoggerInterface; -use Symfony\Component\EventDispatcher\EventDispatcher as SymfonyDispatcher; use Test\TestCase; class ApplicationTest extends TestCase { private Application $application; - private IEventDispatcher $eventDispatcher; /** @var IUserSession */ private $userSession; @@ -52,28 +47,14 @@ class ApplicationTest extends TestCase { /** @var IRootFolder */ private $rootFolder; - /** @var Manager */ - private $manager; protected function setUp(): void { parent::setUp(); $this->application = new Application([]); - $symfonyDispatcher = new SymfonyDispatcher(); - $this->eventDispatcher = new EventDispatcher( - $symfonyDispatcher, - $this->createMock(IServerContainer::class), - $this->createMock(LoggerInterface::class) - ); $this->userSession = $this->createMock(IUserSession::class); $this->rootFolder = $this->createMock(IRootFolder::class); - - $this->application->registerDownloadEvents( - $this->eventDispatcher, - $this->userSession, - $this->rootFolder - ); } public function providesDataForCanGet(): array { @@ -134,7 +115,11 @@ public function testCheckDirectCanBeDownloaded(string $path, Folder $userFolder, // Simulate direct download of file $event = new BeforeDirectFileDownloadEvent($path); - $this->eventDispatcher->dispatchTyped($event); + $listener = new BeforeDirectFileDownloadListener( + $this->userSession, + $this->rootFolder + ); + $listener->handle($event); $this->assertEquals($run, $event->isSuccessful()); } @@ -212,7 +197,12 @@ public function testCheckZipCanBeDownloaded(string $dir, array $files, Folder $u // Simulate zip download of folder folder $event = new BeforeZipCreatedEvent($dir, $files); - $this->eventDispatcher->dispatchTyped($event); + $listener = new BeforeZipCreatedListener( + $this->userSession, + $this->rootFolder + ); + $listener->handle($event); + $this->assertEquals($run, $event->isSuccessful()); $this->assertEquals($run, $event->getErrorMessage() === null); @@ -223,7 +213,11 @@ public function testCheckFileUserNotFound(): void { // Simulate zip download of folder folder $event = new BeforeZipCreatedEvent('/test', ['test.txt']); - $this->eventDispatcher->dispatchTyped($event); + $listener = new BeforeZipCreatedListener( + $this->userSession, + $this->rootFolder + ); + $listener->handle($event); // It should run as this would restrict e.g. share links otherwise $this->assertTrue($event->isSuccessful());