Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
536ccf1
feat(encryption): Migrate from hooks to events
come-nc Oct 3, 2024
38f341c
fix(tests): Unregister encryption modules in ViewTest to avoid side e…
come-nc Nov 5, 2024
21233b7
fix(tests): Remove Encryption disabling in ViewTest to avoid side eff…
come-nc Nov 26, 2024
8779ae3
fix(encryption): Fix filesize for part files in Encryption wrapper
come-nc Nov 26, 2024
9bb0721
fix: Fix mtime preservation when moving a directory across storages w…
come-nc Nov 26, 2024
e35a8ed
fix(tests): Disable encryption wrapper when it makes sense
come-nc Nov 26, 2024
14872c8
chore(files_versions): Only mock getSystemValue method to avoid probl…
come-nc Nov 26, 2024
561b590
chore(trashbin): Fix configuration mocking in trashbin tests
come-nc Nov 26, 2024
f6f8343
chore: Update psalm baseline to remove fixed issue
come-nc Nov 26, 2024
367c877
fix(tests): Avoid user login before a private key is setup
come-nc Nov 28, 2024
08bff4c
fix(admin_audit): Survive if file change id after rename (it should not)
come-nc Nov 28, 2024
27599ef
fix(encryption): Fix a PHP error in Encryption Util in specific situa…
come-nc Jan 13, 2025
e6275f8
fix: Preserve file id when moving from object store even if encryptio…
come-nc Jan 13, 2025
2d8f6b3
chore: Assert rename success in versionning tests
come-nc Jan 16, 2025
a79b5de
fix(encryption): Improve Update class and event listenening
come-nc Feb 11, 2025
a86d917
fix(encryption): Only prevent cache deletion if target is not object …
come-nc May 13, 2025
43418ee
fix(tests): Set encryption configuration even earlier so that all use…
come-nc May 13, 2025
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
23 changes: 2 additions & 21 deletions apps/admin_audit/lib/Actions/Files.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
use OC\Files\Node\NonExistingFile;
use OCP\Files\Events\Node\BeforeNodeDeletedEvent;
use OCP\Files\Events\Node\BeforeNodeReadEvent;
use OCP\Files\Events\Node\BeforeNodeRenamedEvent;
use OCP\Files\Events\Node\NodeCopiedEvent;
use OCP\Files\Events\Node\NodeCreatedEvent;
use OCP\Files\Events\Node\NodeRenamedEvent;
Expand All @@ -26,9 +25,6 @@
* @package OCA\AdminAudit\Actions
*/
class Files extends Action {

private array $renamedNodes = [];

/**
* Logs file read actions
*/
Expand All @@ -52,31 +48,16 @@ public function read(BeforeNodeReadEvent $event): void {
);
}

/**
* Logs rename actions of files
*/
public function beforeRename(BeforeNodeRenamedEvent $event): void {
try {
$source = $event->getSource();
$this->renamedNodes[$source->getId()] = $source;
} catch (InvalidPathException|NotFoundException $e) {
Server::get(LoggerInterface::class)->error(
'Exception thrown in file rename: ' . $e->getMessage(), ['app' => 'admin_audit', 'exception' => $e]
);
return;
}
}

/**
* Logs rename actions of files
*/
public function afterRename(NodeRenamedEvent $event): void {
try {
$target = $event->getTarget();
$originalSource = $this->renamedNodes[$target->getId()];
$source = $event->getSource();
$params = [
'newid' => $target->getId(),
'oldpath' => $originalSource->getPath(),
'oldpath' => $source->getPath(),
'newpath' => $target->getPath(),
];
} catch (InvalidPathException|NotFoundException $e) {
Expand Down
8 changes: 0 additions & 8 deletions apps/admin_audit/lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Events\Node\BeforeNodeDeletedEvent;
use OCP\Files\Events\Node\BeforeNodeReadEvent;
use OCP\Files\Events\Node\BeforeNodeRenamedEvent;
use OCP\Files\Events\Node\NodeCopiedEvent;
use OCP\Files\Events\Node\NodeCreatedEvent;
use OCP\Files\Events\Node\NodeRenamedEvent;
Expand Down Expand Up @@ -170,13 +169,6 @@ private function tagHooks(IAuditLogger $logger,
private function fileHooks(IAuditLogger $logger, IEventDispatcher $eventDispatcher): void {
$fileActions = new Files($logger);

$eventDispatcher->addListener(
BeforeNodeRenamedEvent::class,
function (BeforeNodeRenamedEvent $event) use ($fileActions): void {
$fileActions->beforeRename($event);
}
);

$eventDispatcher->addListener(
NodeRenamedEvent::class,
function (NodeRenamedEvent $event) use ($fileActions): void {
Expand Down
11 changes: 10 additions & 1 deletion apps/files_sharing/tests/EncryptedSizePropagationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,22 @@
class EncryptedSizePropagationTest extends SizePropagationTest {
use EncryptionTrait;

protected function setUp(): void {
parent::setUp();
$this->config->setAppValue('encryption', 'useMasterKey', '0');
}

protected function setupUser($name, $password = '') {
$this->createUser($name, $password);
$tmpFolder = Server::get(ITempManager::class)->getTemporaryFolder();
$this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]);
$this->config->setAppValue('encryption', 'useMasterKey', '0');
$this->setupForUser($name, $password);
$this->loginWithEncryption($name);
return new View('/' . $name . '/files');
}

protected function loginHelper($user, $create = false, $password = false) {
$this->setupForUser($user, $password);
parent::loginHelper($user, $create, $password);
}
}
4 changes: 2 additions & 2 deletions apps/files_sharing/tests/TestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ protected function setUp(): void {
Server::get(DisplayNameCache::class)->clear();

//login as user1
self::loginHelper(self::TEST_FILES_SHARING_API_USER1);
$this->loginHelper(self::TEST_FILES_SHARING_API_USER1);

$this->data = 'foobar';
$this->view = new View('/' . self::TEST_FILES_SHARING_API_USER1 . '/files');
Expand Down Expand Up @@ -173,7 +173,7 @@ public static function tearDownAfterClass(): void {
* @param bool $create
* @param bool $password
*/
protected static function loginHelper($user, $create = false, $password = false) {
protected function loginHelper($user, $create = false, $password = false) {
if ($password === false) {
$password = $user;
}
Expand Down
17 changes: 5 additions & 12 deletions apps/files_trashbin/tests/TrashbinTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,11 @@ protected function setUp(): void {

Server::get(IAppManager::class)->enableApp('files_trashbin');
$config = Server::get(IConfig::class);
$mockConfig = $this->createMock(IConfig::class);
$mockConfig
$mockConfig = $this->getMockBuilder(AllConfig::class)
->onlyMethods(['getSystemValue'])
->setConstructorArgs([Server::get(\OC\SystemConfig::class)])
->getMock();
$mockConfig->expects($this->any())
->method('getSystemValue')
->willReturnCallback(static function ($key, $default) use ($config) {
if ($key === 'filesystem_check_changes') {
Expand All @@ -125,16 +128,6 @@ protected function setUp(): void {
return $config->getSystemValue($key, $default);
}
});
$mockConfig
->method('getUserValue')
->willReturnCallback(static function ($userId, $appName, $key, $default = '') use ($config) {
return $config->getUserValue($userId, $appName, $key, $default);
});
$mockConfig
->method('getAppValue')
->willReturnCallback(static function ($appName, $key, $default = '') use ($config) {
return $config->getAppValue($appName, $key, $default);
});
$this->overwriteService(AllConfig::class, $mockConfig);

$this->trashRoot1 = '/' . self::TEST_TRASHBIN_USER1 . '/files_trashbin';
Expand Down
6 changes: 3 additions & 3 deletions apps/files_versions/lib/Listener/FileEventsListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,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 {
Expand Down Expand Up @@ -282,7 +282,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 {
Expand All @@ -301,7 +301,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 {
Expand Down
8 changes: 6 additions & 2 deletions apps/files_versions/tests/VersioningTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@ protected function setUp(): void {
parent::setUp();

$config = Server::get(IConfig::class);
$mockConfig = $this->createMock(IConfig::class);
$mockConfig = $this->getMockBuilder(AllConfig::class)
->onlyMethods(['getSystemValue'])
->setConstructorArgs([Server::get(\OC\SystemConfig::class)])
->getMock();
$mockConfig->expects($this->any())
->method('getSystemValue')
->willReturnCallback(function ($key, $default) use ($config) {
Expand Down Expand Up @@ -427,8 +430,9 @@ public function testMoveFileIntoSharedFolderAsRecipient(): void {
$this->rootView->file_put_contents($v2, 'version2');

// move file into the shared folder as recipient
Filesystem::rename('/test.txt', '/folder1/test.txt');
$success = Filesystem::rename('/test.txt', '/folder1/test.txt');

$this->assertTrue($success);
$this->assertFalse($this->rootView->file_exists($v1));
$this->assertFalse($this->rootView->file_exists($v2));

Expand Down
3 changes: 0 additions & 3 deletions build/psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2013,9 +2013,6 @@
</UndefinedInterfaceMethod>
</file>
<file src="lib/private/Files/Storage/Wrapper/Encryption.php">
<InvalidOperand>
<code><![CDATA[$this->copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, false, $isRename)]]></code>
</InvalidOperand>
<InvalidReturnStatement>
<code><![CDATA[$result]]></code>
</InvalidReturnStatement>
Expand Down
15 changes: 8 additions & 7 deletions lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@
* SPDX-FileCopyrightText: 2013-2016 ownCloud, Inc.
* SPDX-License-Identifier: AGPL-3.0-only
*/
use OC\Encryption\HookManager;

use OC\Profiler\BuiltInProfiler;
use OC\Share20\GroupDeletedListener;
use OC\Share20\Hooks;
use OC\Share20\UserDeletedListener;
use OC\Share20\UserRemovedListener;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Events\BeforeFileSystemSetupEvent;
use OCP\Group\Events\GroupDeletedEvent;
use OCP\Group\Events\UserRemovedEvent;
use OCP\IConfig;
Expand All @@ -22,7 +23,6 @@
use OCP\IUserSession;
use OCP\Security\Bruteforce\IThrottler;
use OCP\Server;
use OCP\Share;
use OCP\Template\ITemplateManager;
use OCP\User\Events\UserChangedEvent;
use OCP\User\Events\UserDeletedEvent;
Expand Down Expand Up @@ -907,15 +907,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));
}
}

Expand Down
2 changes: 1 addition & 1 deletion lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -1539,6 +1539,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',
Expand All @@ -1549,7 +1550,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',
Expand Down
2 changes: 1 addition & 1 deletion lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -1580,6 +1580,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',
Expand All @@ -1590,7 +1591,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',
Expand Down
93 changes: 93 additions & 0 deletions lib/private/Encryption/EncryptionEventListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/

namespace OC\Encryption;

use OC\Files\SetupManager;
use OC\Files\View;
use OCA\Files_Trashbin\Events\NodeRestoredEvent;
use OCP\Encryption\IFile;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\EventDispatcher\IEventListener;
use OCP\Files\Events\Node\NodeRenamedEvent;
use OCP\IUser;
use OCP\IUserSession;
use OCP\Share\Events\ShareCreatedEvent;
use OCP\Share\Events\ShareDeletedEvent;
use Psr\Log\LoggerInterface;

/** @template-implements IEventListener<NodeRenamedEvent|ShareCreatedEvent|ShareDeletedEvent|NodeRestoredEvent> */
class EncryptionEventListener implements IEventListener {
private ?Update $updater = null;

public function __construct(
private IUserSession $userSession,
private SetupManager $setupManager,
private Manager $encryptionManager,
) {
}

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 (!$this->encryptionManager->isEnabled()) {
return;
}
if ($event instanceof NodeRenamedEvent) {
$this->getUpdate()->postRename($event->getSource(), $event->getTarget());
} elseif ($event instanceof ShareCreatedEvent) {
$this->getUpdate()->postShared($event->getShare()->getNode());
} 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()->getNode());
} elseif ($event instanceof NodeRestoredEvent) {
$this->getUpdate()->postRestore($event->getTarget());
}
}

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()),
\OC::$server->getEncryptionManager(),
\OC::$server->get(IFile::class),
\OC::$server->get(LoggerInterface::class),
Comment on lines +81 to +86
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Server::get ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.
But at this point I’m stuck on the failure of https://github.com/nextcloud/server/actions/runs/14195485740/job/39769612916?pr=48560

I do not understand what’s happening.

I pulled ed6e1ec to fix some other tests and it broke this one. I do not understand why it’s failing and why specifically for s3, while it was passing without encryption wrapper.

$uid
);
}

return $this->updater;
}
}
Loading
Loading