Skip to content

Commit

Permalink
Merge pull request #44967 from nextcloud/perf/sharing-events
Browse files Browse the repository at this point in the history
perf(files_sharing): Move events to listener classes and registration instead of boot
  • Loading branch information
susnux committed Jun 10, 2024
2 parents 9d0200c + 0ca5c74 commit 21a0009
Show file tree
Hide file tree
Showing 6 changed files with 176 additions and 88 deletions.
2 changes: 2 additions & 0 deletions apps/files_sharing/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions apps/files_sharing/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
77 changes: 13 additions & 64 deletions apps/files_sharing/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,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;
Expand All @@ -28,7 +30,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;
Expand All @@ -39,13 +40,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;
Expand Down Expand Up @@ -85,12 +84,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();

Expand All @@ -105,12 +114,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');
});
Expand All @@ -136,58 +139,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);
}
}
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2019, Roeland Jago Douma <[email protected]>
*
* @author John Molakvoæ <[email protected]>
*
* @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 <http://www.gnu.org/licenses/>.
*
*/

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<BeforeDirectFileDownloadEvent|Event>
*/
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.');
}
}
}
}
76 changes: 76 additions & 0 deletions apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2019, Roeland Jago Douma <[email protected]>
*
* @author John Molakvoæ <[email protected]>
*
* @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 <http://www.gnu.org/licenses/>.
*
*/

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<BeforeZipCreatedEvent|Event>
*/
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);
}
}
}
42 changes: 18 additions & 24 deletions apps/files_sharing/tests/ApplicationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,58 +5,39 @@
*/
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;

/** @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 {
Expand Down Expand Up @@ -117,7 +98,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());
}
Expand Down Expand Up @@ -195,7 +180,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);
Expand All @@ -206,7 +196,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());
Expand Down

0 comments on commit 21a0009

Please sign in to comment.