From fc221884d51c6a5a4dd849a9f4b40390d7dc182b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 3 Oct 2024 16:07:02 +0200 Subject: [PATCH] feat(encryption): Migrate from hooks to events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../lib/Listener/FileEventsListener.php | 6 +- lib/base.php | 14 +-- lib/composer/composer/autoload_classmap.php | 2 +- lib/composer/composer/autoload_static.php | 2 +- .../Encryption/EncryptionEventListener.php | 92 +++++++++++++++++++ lib/private/Encryption/EncryptionWrapper.php | 1 - lib/private/Encryption/HookManager.php | 75 --------------- lib/private/Encryption/Update.php | 45 +++------ tests/lib/Encryption/UpdateTest.php | 49 +++------- 9 files changed, 133 insertions(+), 153 deletions(-) create mode 100644 lib/private/Encryption/EncryptionEventListener.php delete mode 100644 lib/private/Encryption/HookManager.php diff --git a/apps/files_versions/lib/Listener/FileEventsListener.php b/apps/files_versions/lib/Listener/FileEventsListener.php index c078f4bc34755..cd4db886b7898 100644 --- a/apps/files_versions/lib/Listener/FileEventsListener.php +++ b/apps/files_versions/lib/Listener/FileEventsListener.php @@ -241,7 +241,7 @@ public function post_write_hook(Node $node): void { /** * Erase versions of deleted file * - * This function is connected to the delete signal of OC_Filesystem + * This function is connected to the NodeDeletedEvent event * cleanup the versions directory if the actual file gets deleted */ public function remove_hook(Node $node): void { @@ -273,7 +273,7 @@ public function pre_remove_hook(Node $node): void { /** * rename/move versions of renamed/moved files * - * This function is connected to the rename signal of OC_Filesystem and adjust the name and location + * This function is connected to the NodeRenamedEvent event and adjust the name and location * of the stored versions along the actual file */ public function rename_hook(Node $source, Node $target): void { @@ -292,7 +292,7 @@ public function rename_hook(Node $source, Node $target): void { /** * copy versions of copied files * - * This function is connected to the copy signal of OC_Filesystem and copies the + * This function is connected to the NodeCopiedEvent event and copies the * the stored versions to the new location */ public function copy_hook(Node $source, Node $target): void { diff --git a/lib/base.php b/lib/base.php index 5bab5d9f3f699..ae7d73b73175c 100644 --- a/lib/base.php +++ b/lib/base.php @@ -6,9 +6,9 @@ * SPDX-FileCopyrightText: 2013-2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ -use OC\Encryption\HookManager; use OC\Share20\Hooks; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Files\Events\BeforeFileSystemSetupEvent; use OCP\Group\Events\UserRemovedEvent; use OCP\ILogger; use OCP\IRequest; @@ -16,7 +16,6 @@ use OCP\IUserSession; use OCP\Security\Bruteforce\IThrottler; use OCP\Server; -use OCP\Share; use OCP\User\Events\UserChangedEvent; use Psr\Log\LoggerInterface; use Symfony\Component\Routing\Exception\MethodNotAllowedException; @@ -860,15 +859,16 @@ public static function registerCleanupHooks(\OC\SystemConfig $systemConfig): voi } private static function registerEncryptionWrapperAndHooks(): void { + /** @var \OC\Encryption\Manager */ $manager = Server::get(\OCP\Encryption\IManager::class); - \OCP\Util::connectHook('OC_Filesystem', 'preSetup', $manager, 'setupStorage'); + Server::get(IEventDispatcher::class)->addListener( + BeforeFileSystemSetupEvent::class, + $manager->setupStorage(...), + ); $enabled = $manager->isEnabled(); if ($enabled) { - \OCP\Util::connectHook(Share::class, 'post_shared', HookManager::class, 'postShared'); - \OCP\Util::connectHook(Share::class, 'post_unshare', HookManager::class, 'postUnshared'); - \OCP\Util::connectHook('OC_Filesystem', 'post_rename', HookManager::class, 'postRename'); - \OCP\Util::connectHook('\OCA\Files_Trashbin\Trashbin', 'post_restore', HookManager::class, 'postRestore'); + \OC\Encryption\EncryptionEventListener::register(Server::get(IEventDispatcher::class)); } } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index ef9af33414f13..f22a5c58f0bb5 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1463,6 +1463,7 @@ 'OC\\DirectEditing\\Token' => $baseDir . '/lib/private/DirectEditing/Token.php', 'OC\\EmojiHelper' => $baseDir . '/lib/private/EmojiHelper.php', 'OC\\Encryption\\DecryptAll' => $baseDir . '/lib/private/Encryption/DecryptAll.php', + 'OC\\Encryption\\EncryptionEventListener' => $baseDir . '/lib/private/Encryption/EncryptionEventListener.php', 'OC\\Encryption\\EncryptionWrapper' => $baseDir . '/lib/private/Encryption/EncryptionWrapper.php', 'OC\\Encryption\\Exceptions\\DecryptionFailedException' => $baseDir . '/lib/private/Encryption/Exceptions/DecryptionFailedException.php', 'OC\\Encryption\\Exceptions\\EmptyEncryptionDataException' => $baseDir . '/lib/private/Encryption/Exceptions/EmptyEncryptionDataException.php', @@ -1473,7 +1474,6 @@ 'OC\\Encryption\\Exceptions\\ModuleDoesNotExistsException' => $baseDir . '/lib/private/Encryption/Exceptions/ModuleDoesNotExistsException.php', 'OC\\Encryption\\Exceptions\\UnknownCipherException' => $baseDir . '/lib/private/Encryption/Exceptions/UnknownCipherException.php', 'OC\\Encryption\\File' => $baseDir . '/lib/private/Encryption/File.php', - 'OC\\Encryption\\HookManager' => $baseDir . '/lib/private/Encryption/HookManager.php', 'OC\\Encryption\\Keys\\Storage' => $baseDir . '/lib/private/Encryption/Keys/Storage.php', 'OC\\Encryption\\Manager' => $baseDir . '/lib/private/Encryption/Manager.php', 'OC\\Encryption\\Update' => $baseDir . '/lib/private/Encryption/Update.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 99f8afb61f1ac..da735af9a284a 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1496,6 +1496,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\DirectEditing\\Token' => __DIR__ . '/../../..' . '/lib/private/DirectEditing/Token.php', 'OC\\EmojiHelper' => __DIR__ . '/../../..' . '/lib/private/EmojiHelper.php', 'OC\\Encryption\\DecryptAll' => __DIR__ . '/../../..' . '/lib/private/Encryption/DecryptAll.php', + 'OC\\Encryption\\EncryptionEventListener' => __DIR__ . '/../../..' . '/lib/private/Encryption/EncryptionEventListener.php', 'OC\\Encryption\\EncryptionWrapper' => __DIR__ . '/../../..' . '/lib/private/Encryption/EncryptionWrapper.php', 'OC\\Encryption\\Exceptions\\DecryptionFailedException' => __DIR__ . '/../../..' . '/lib/private/Encryption/Exceptions/DecryptionFailedException.php', 'OC\\Encryption\\Exceptions\\EmptyEncryptionDataException' => __DIR__ . '/../../..' . '/lib/private/Encryption/Exceptions/EmptyEncryptionDataException.php', @@ -1506,7 +1507,6 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Encryption\\Exceptions\\ModuleDoesNotExistsException' => __DIR__ . '/../../..' . '/lib/private/Encryption/Exceptions/ModuleDoesNotExistsException.php', 'OC\\Encryption\\Exceptions\\UnknownCipherException' => __DIR__ . '/../../..' . '/lib/private/Encryption/Exceptions/UnknownCipherException.php', 'OC\\Encryption\\File' => __DIR__ . '/../../..' . '/lib/private/Encryption/File.php', - 'OC\\Encryption\\HookManager' => __DIR__ . '/../../..' . '/lib/private/Encryption/HookManager.php', 'OC\\Encryption\\Keys\\Storage' => __DIR__ . '/../../..' . '/lib/private/Encryption/Keys/Storage.php', 'OC\\Encryption\\Manager' => __DIR__ . '/../../..' . '/lib/private/Encryption/Manager.php', 'OC\\Encryption\\Update' => __DIR__ . '/../../..' . '/lib/private/Encryption/Update.php', diff --git a/lib/private/Encryption/EncryptionEventListener.php b/lib/private/Encryption/EncryptionEventListener.php new file mode 100644 index 0000000000000..a22661e9f7503 --- /dev/null +++ b/lib/private/Encryption/EncryptionEventListener.php @@ -0,0 +1,92 @@ + */ +class EncryptionEventListener implements IEventListener { + private ?Update $updater = null; + + public function __construct( + private IUserSession $userSession, + private SetupManager $setupManager, + ) { + } + + public static function register(IEventDispatcher $dispatcher): void { + $dispatcher->addServiceListener(NodeRenamedEvent::class, static::class); + $dispatcher->addServiceListener(ShareCreatedEvent::class, static::class); + $dispatcher->addServiceListener(ShareDeletedEvent::class, static::class); + $dispatcher->addServiceListener(NodeRestoredEvent::class, static::class); + } + + public function handle(Event $event): void { + if ($event instanceof NodeRenamedEvent) { + $this->getUpdate()->postRename($event->getSource() instanceof Folder, $event->getSource()->getPath(), $event->getTarget()->getPath()); + } elseif ($event instanceof ShareCreatedEvent) { + $this->getUpdate()->postShared($event->getShare()->getNodeType(), $event->getShare()->getNodeId()); + } elseif ($event instanceof ShareDeletedEvent) { + // In case the unsharing happens in a background job, we don't have + // a session and we load instead the user from the UserManager + $owner = $event->getShare()->getNode()->getOwner(); + $this->getUpdate($owner)->postUnshared($event->getShare()->getNodeType(), $event->getShare()->getNodeId()); + } elseif ($event instanceof NodeRestoredEvent) { + $this->getUpdate()->postRestore($event->getTarget() instanceof Folder, $event->getTarget()->getPath()); + } + } + + private function getUpdate(?IUser $owner = null): Update { + if (is_null($this->updater)) { + $user = $this->userSession->getUser(); + if (!$user && ($owner !== null)) { + $user = $owner; + } + if (!$user) { + throw new \Exception('Inconsistent data, File unshared, but owner not found. Should not happen'); + } + + $uid = $user->getUID(); + + if (!$this->setupManager->isSetupComplete($user)) { + $this->setupManager->setupForUser($user); + } + + $this->updater = new Update( + new Util( + new View(), + \OC::$server->getUserManager(), + \OC::$server->getGroupManager(), + \OC::$server->getConfig()), + Filesystem::getMountManager(), + \OC::$server->getEncryptionManager(), + \OC::$server->get(IFile::class), + \OC::$server->get(LoggerInterface::class), + $uid + ); + } + + return $this->updater; + } +} diff --git a/lib/private/Encryption/EncryptionWrapper.php b/lib/private/Encryption/EncryptionWrapper.php index 7f355b603d620..6a19c1cccaa96 100644 --- a/lib/private/Encryption/EncryptionWrapper.php +++ b/lib/private/Encryption/EncryptionWrapper.php @@ -76,7 +76,6 @@ public function wrapStorage(string $mountPoint, IStorage $storage, IMountPoint $ \OC::$server->getConfig() ); $update = new Update( - new View(), $util, Filesystem::getMountManager(), $this->manager, diff --git a/lib/private/Encryption/HookManager.php b/lib/private/Encryption/HookManager.php deleted file mode 100644 index 39e7edabb9561..0000000000000 --- a/lib/private/Encryption/HookManager.php +++ /dev/null @@ -1,75 +0,0 @@ -postShared($params); - } - public static function postUnshared($params): void { - // In case the unsharing happens in a background job, we don't have - // a session and we load instead the user from the UserManager - $path = Filesystem::getPath($params['fileSource']); - $owner = Filesystem::getOwner($path); - self::getUpdate($owner)->postUnshared($params); - } - - public static function postRename($params): void { - self::getUpdate()->postRename($params); - } - - public static function postRestore($params): void { - self::getUpdate()->postRestore($params); - } - - private static function getUpdate(?string $owner = null): Update { - if (is_null(self::$updater)) { - $user = \OC::$server->getUserSession()->getUser(); - if (!$user && $owner) { - $user = \OC::$server->getUserManager()->get($owner); - } - if (!$user) { - throw new \Exception('Inconsistent data, File unshared, but owner not found. Should not happen'); - } - - $uid = ''; - if ($user) { - $uid = $user->getUID(); - } - - $setupManager = \OC::$server->get(SetupManager::class); - if (!$setupManager->isSetupComplete($user)) { - $setupManager->setupForUser($user); - } - - self::$updater = new Update( - new View(), - new Util( - new View(), - \OC::$server->getUserManager(), - \OC::$server->getGroupManager(), - \OC::$server->getConfig()), - Filesystem::getMountManager(), - \OC::$server->getEncryptionManager(), - \OC::$server->get(IFile::class), - \OC::$server->get(LoggerInterface::class), - $uid - ); - } - - return self::$updater; - } -} diff --git a/lib/private/Encryption/Update.php b/lib/private/Encryption/Update.php index 0b27d63c19a2a..d3f9c0ebef7cd 100644 --- a/lib/private/Encryption/Update.php +++ b/lib/private/Encryption/Update.php @@ -18,9 +18,6 @@ * update encrypted files, e.g. because a file was shared */ class Update { - /** @var View */ - protected $view; - /** @var Util */ protected $util; @@ -43,7 +40,6 @@ class Update { * @param string $uid */ public function __construct( - View $view, Util $util, Mount\Manager $mountManager, Manager $encryptionManager, @@ -51,7 +47,6 @@ public function __construct( LoggerInterface $logger, $uid, ) { - $this->view = $view; $this->util = $util; $this->mountManager = $mountManager; $this->encryptionManager = $encryptionManager; @@ -62,32 +57,28 @@ public function __construct( /** * hook after file was shared - * - * @param array $params */ - public function postShared($params) { + public function postShared(string $nodeType, int $nodeId): void { if ($this->encryptionManager->isEnabled()) { - if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { - $path = Filesystem::getPath($params['fileSource']); + if ($nodeType === 'file' || $nodeType === 'folder') { + $path = Filesystem::getPath($nodeId); [$owner, $ownerPath] = $this->getOwnerPath($path); $absPath = '/' . $owner . '/files/' . $ownerPath; - $this->update($absPath); + $this->update($nodeType === 'folder', $absPath); } } } /** * hook after file was unshared - * - * @param array $params */ - public function postUnshared($params) { + public function postUnshared(string $nodeType, int $nodeId): void { if ($this->encryptionManager->isEnabled()) { - if ($params['itemType'] === 'file' || $params['itemType'] === 'folder') { - $path = Filesystem::getPath($params['fileSource']); + if ($nodeType === 'file' || $nodeType === 'folder') { + $path = Filesystem::getPath($nodeId); [$owner, $ownerPath] = $this->getOwnerPath($path); $absPath = '/' . $owner . '/files/' . $ownerPath; - $this->update($absPath); + $this->update($nodeType === 'folder', $absPath); } } } @@ -95,32 +86,26 @@ public function postUnshared($params) { /** * inform encryption module that a file was restored from the trash bin, * e.g. to update the encryption keys - * - * @param array $params */ - public function postRestore($params) { + public function postRestore(bool $directory, string $filePath): void { if ($this->encryptionManager->isEnabled()) { - $path = Filesystem::normalizePath('/' . $this->uid . '/files/' . $params['filePath']); - $this->update($path); + $path = Filesystem::normalizePath('/' . $this->uid . '/files/' . $filePath); + $this->update($directory, $path); } } /** * inform encryption module that a file was renamed, * e.g. to update the encryption keys - * - * @param array $params */ - public function postRename($params) { - $source = $params['oldpath']; - $target = $params['newpath']; + public function postRename(bool $directory, string $source, string $target): void { if ( $this->encryptionManager->isEnabled() && dirname($source) !== dirname($target) ) { [$owner, $ownerPath] = $this->getOwnerPath($target); $absPath = '/' . $owner . '/files/' . $ownerPath; - $this->update($absPath); + $this->update($directory, $absPath); } } @@ -149,7 +134,7 @@ protected function getOwnerPath($path) { * @param string $path relative to data/ * @throws Exceptions\ModuleDoesNotExistsException */ - public function update($path) { + public function update(bool $directory, string $path): void { $encryptionModule = $this->encryptionManager->getEncryptionModule(); // if the encryption module doesn't encrypt the files on a per-user basis @@ -159,7 +144,7 @@ public function update($path) { } // if a folder was shared, get a list of all (sub-)folders - if ($this->view->is_dir($path)) { + if ($directory) { $allFiles = $this->util->getAllFiles($path); } else { $allFiles = [$path]; diff --git a/tests/lib/Encryption/UpdateTest.php b/tests/lib/Encryption/UpdateTest.php index 2627e18601d26..d54aeab35ddf3 100644 --- a/tests/lib/Encryption/UpdateTest.php +++ b/tests/lib/Encryption/UpdateTest.php @@ -13,36 +13,21 @@ use OC\Files\Mount\Manager; use OC\Files\View; use OCP\Encryption\IEncryptionModule; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; class UpdateTest extends TestCase { - /** @var \OC\Encryption\Update */ - private $update; + private Update $update; - /** @var string */ - private $uid; - - /** @var \OC\Files\View | \PHPUnit\Framework\MockObject\MockObject */ - private $view; - - /** @var Util | \PHPUnit\Framework\MockObject\MockObject */ - private $util; - - /** @var \OC\Files\Mount\Manager | \PHPUnit\Framework\MockObject\MockObject */ - private $mountManager; - - /** @var \OC\Encryption\Manager | \PHPUnit\Framework\MockObject\MockObject */ - private $encryptionManager; - - /** @var \OCP\Encryption\IEncryptionModule | \PHPUnit\Framework\MockObject\MockObject */ - private $encryptionModule; - - /** @var \OC\Encryption\File | \PHPUnit\Framework\MockObject\MockObject */ - private $fileHelper; - - /** @var \PHPUnit\Framework\MockObject\MockObject|LoggerInterface */ - private $logger; + private string $uid; + private View&MockObject $view; + private Util&MockObject $util; + private Manager&MockObject $mountManager; + private \OC\Encryption\Manager&MockObject $encryptionManager; + private IEncryptionModule&MockObject $encryptionModule; + private File&MockObject $fileHelper; + private LoggerInterface&MockObject $logger; protected function setUp(): void { parent::setUp(); @@ -58,7 +43,6 @@ protected function setUp(): void { $this->uid = 'testUser1'; $this->update = new Update( - $this->view, $this->util, $this->mountManager, $this->encryptionManager, @@ -80,10 +64,6 @@ public function testUpdate($path, $isDir, $allFiles, $numberOfFiles): void { ->method('getEncryptionModule') ->willReturn($this->encryptionModule); - $this->view->expects($this->once()) - ->method('is_dir') - ->willReturn($isDir); - if ($isDir) { $this->util->expects($this->once()) ->method('getAllFiles') @@ -98,7 +78,7 @@ public function testUpdate($path, $isDir, $allFiles, $numberOfFiles): void { ->method('update') ->willReturn(true); - $this->update->update($path); + $this->update->update($isDir, $path); } /** @@ -143,7 +123,7 @@ public function testPostRename($source, $target, $encryptionEnabled): void { $updateMock->expects($this->once())->method('update'); } - $updateMock->postRename(['oldpath' => $source, 'newpath' => $target]); + $updateMock->postRename(false, $source, $target); } /** @@ -181,7 +161,7 @@ public function testPostRestore($encryptionEnabled): void { $updateMock->expects($this->never())->method('update'); } - $updateMock->postRestore(['filePath' => '/folder/test.txt']); + $updateMock->postRestore(false, '/folder/test.txt'); } /** @@ -200,13 +180,12 @@ public function dataTestPostRestore() { * create mock of the update method * * @param array $methods methods which should be set - * @return \OC\Encryption\Update | \PHPUnit\Framework\MockObject\MockObject + * @return \OC\Encryption\Update | MockObject */ protected function getUpdateMock($methods) { return $this->getMockBuilder('\OC\Encryption\Update') ->setConstructorArgs( [ - $this->view, $this->util, $this->mountManager, $this->encryptionManager,