feat(encryption): Migrate from hooks to events #48560

come-nc · 2024-10-03T14:10:31Z

Summary

This is not half as clean as I would’ve liked, because original code is convoluted.
The owner thing in the event listener is ugly and probably incorrect but it was there before in HookManager class.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

come-nc · 2024-11-26T17:05:22Z

Getting there, only one more failure. For some reason it’s not the same one locally and in the CI 🤷

Signed-off-by: Côme Chilliet <[email protected]>

…ffects It was clearing the hooks with the same results before Signed-off-by: Côme Chilliet <[email protected]>

…ects Adapt tests a bit to make them pass with Encryption wrapper registered Signed-off-by: Côme Chilliet <[email protected]>

Signed-off-by: Côme Chilliet <[email protected]>

…ith encryption registered Signed-off-by: Côme Chilliet <[email protected]>

Signed-off-by: Côme Chilliet <[email protected]>

…ems in files_versions tests Signed-off-by: Côme Chilliet <[email protected]>

Signed-off-by: Côme Chilliet <[email protected]>

…tions Signed-off-by: Côme Chilliet <[email protected]>

…n wrapper is present Signed-off-by: Côme Chilliet <[email protected]>

Signed-off-by: Côme Chilliet <[email protected]>

lib/private/Files/Storage/Wrapper/Encryption.php

@@ -521,15 +522,31 @@
 		// - remove $this->copyBetweenStorage

 		if (!$sourceStorage->isDeletable($sourceInternalPath)) {
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath not deletable " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

@@ -521,15 +522,31 @@
 		// - remove $this->copyBetweenStorage

 		if (!$sourceStorage->isDeletable($sourceInternalPath)) {
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath not deletable " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 			return false;
 		}

 		$result = $this->copyBetweenStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, true);
+		echo "[DEBUG] $sourceInternalPath $targetInternalPath copy:$result " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 			return false;
 		}

 		$result = $this->copyBetweenStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime, true);
+		echo "[DEBUG] $sourceInternalPath $targetInternalPath copy:$result " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

-				$result = $sourceStorage->rmdir($sourceInternalPath);
-			} else {
-				$result = $sourceStorage->unlink($sourceInternalPath);
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath copy success " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

-				$result = $sourceStorage->rmdir($sourceInternalPath);
-			} else {
-				$result = $sourceStorage->unlink($sourceInternalPath);
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath copy success " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

@@ -609,10 +626,12 @@
 		bool $preserveMtime,
 		bool $isRename,
 	): bool {
+		echo "[DEBUG] $sourceInternalPath $targetInternalPath $isRename " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

@@ -609,10 +626,12 @@
 		bool $preserveMtime,
 		bool $isRename,
 	): bool {
+		echo "[DEBUG] $sourceInternalPath $targetInternalPath $isRename " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		// for versions we have nothing to do, because versions should always use the
 		// key from the original file. Just create a 1:1 copy and done
 		if ($this->isVersion($targetInternalPath) ||
 			$this->isVersion($sourceInternalPath)) {
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath $isRename " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		// for versions we have nothing to do, because versions should always use the
 		// key from the original file. Just create a 1:1 copy and done
 		if ($this->isVersion($targetInternalPath) ||
 			$this->isVersion($sourceInternalPath)) {
+			echo "[DEBUG] $sourceInternalPath $targetInternalPath $isRename " . __FILE__ . ':' . __LINE__ . "\n";


Signed-off-by: Côme Chilliet <[email protected]>

lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -176,13 +176,17 @@
 	}

 	public function unlink(string $path): bool {
+		echo "[DEBUG] unlink($path) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -176,13 +176,17 @@
 	}

 	public function unlink(string $path): bool {
+		echo "[DEBUG] unlink($path) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

@@ -168,8 +169,11 @@
 	}

 	public function unlink(string $path): bool {
+		echo "[DEBUG] unlink($path) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

@@ -168,8 +169,11 @@
 	}

 	public function unlink(string $path): bool {
+		echo "[DEBUG] unlink($path) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		$fullPath = $this->getFullPath($path);
+		echo "[DEBUG] unlink($fullPath) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		$fullPath = $this->getFullPath($path);
+		echo "[DEBUG] unlink($fullPath) " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		if ($this->util->isExcluded($fullPath)) {
+			echo "[DEBUG] excluded $fullPath " . __FILE__ . ':' . __LINE__ . "\n";


lib/private/Files/Storage/Wrapper/Encryption.php

 		if ($this->util->isExcluded($fullPath)) {
+			echo "[DEBUG] excluded $fullPath " . __FILE__ . ':' . __LINE__ . "\n";


icewind1991 · 2025-01-21T15:07:42Z

lib/private/Encryption/Update.php

 	 */
-	public function postShared($params) {
+	public function postShared(string $nodeType, int $nodeId): void {


since you're touching the params anyway, might as well pass a FileInfo or Node here to save having to query the filecache again in the body.

(same for the other post... methods below)

come-nc added the 3. to review Waiting for reviews label Oct 3, 2024

come-nc added this to the Nextcloud 31 milestone Oct 3, 2024

come-nc self-assigned this Oct 3, 2024

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch 2 times, most recently from 7b456c7 to fc22188 Compare November 5, 2024 15:53

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch from 1d2e3f4 to f392a55 Compare December 16, 2024 14:45

blizzz mentioned this pull request Jan 8, 2025

31.0.0 beta 2 #50095

Merged

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch from efb5812 to 4a3def9 Compare January 13, 2025 08:58

skjnldsv mentioned this pull request Jan 14, 2025

31.0.0 beta 3 #50178

Merged

come-nc added 14 commits January 16, 2025 18:05

feat(encryption): Migrate from hooks to events

dde9da8

Signed-off-by: Côme Chilliet <[email protected]>

fix(tests): Unregister encryption modules in ViewTest to avoid side e…

4b76d88

…ffects It was clearing the hooks with the same results before Signed-off-by: Côme Chilliet <[email protected]>

fix(tests): Remove Encryption disabling in ViewTest to avoid side eff…

1a08f9f

…ects Adapt tests a bit to make them pass with Encryption wrapper registered Signed-off-by: Côme Chilliet <[email protected]>

fix(encryption): Fix filesize for part files in Encryption wrapper

cb1899e

Signed-off-by: Côme Chilliet <[email protected]>

fix: Fix mtime preservation when moving a directory across storages w…

be8579c

…ith encryption registered Signed-off-by: Côme Chilliet <[email protected]>

fix(tests): Disable encryption wrapper when it makes sense

ea251a9

Signed-off-by: Côme Chilliet <[email protected]>

chore(files_versions): Only mock getSystemValue method to avoid probl…

a357a16

…ems in files_versions tests Signed-off-by: Côme Chilliet <[email protected]>

chore(trashbin): Fix configuration mocking in trashbin tests

da9365e

Signed-off-by: Côme Chilliet <[email protected]>

chore: Update psalm baseline to remove fixed issue

d21ae82

Signed-off-by: Côme Chilliet <[email protected]>

fix(tests): Avoid user login before a private key is setup

deb3e83

Signed-off-by: Côme Chilliet <[email protected]>

fix(admin_audit): Survive if file change id after rename (it should not)

bde65a3

Signed-off-by: Côme Chilliet <[email protected]>

fix(encryption): Fix a PHP error in Encryption Util in specific situa…

8cdbd91

…tions Signed-off-by: Côme Chilliet <[email protected]>

fix: Preserve file id when moving from object store even if encryptio…

185ef8b

…n wrapper is present Signed-off-by: Côme Chilliet <[email protected]>

chore: Assert rename success in versionning tests

14b8770

Signed-off-by: Côme Chilliet <[email protected]>

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch from 314010a to 14b8770 Compare January 16, 2025 17:05

skjnldsv mentioned this pull request Jan 16, 2025

31.0.0 beta 4 #50216

Merged

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch from d47a006 to bfbc787 Compare January 20, 2025 09:40

come-nc added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jan 20, 2025

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

chore: WIP - debug for CI

f299305

Signed-off-by: Côme Chilliet <[email protected]>

come-nc force-pushed the fix/migrate-encryption-away-from-hooks branch from bfbc787 to f299305 Compare January 20, 2025 10:24

github-advanced-security bot found potential problems Jan 20, 2025

View reviewed changes

Altahrim mentioned this pull request Jan 21, 2025

31.0.0 beta 5 #50285

Merged

icewind1991 reviewed Jan 21, 2025

View reviewed changes

Altahrim mentioned this pull request Jan 23, 2025

31.0.0 RC1 #50329

Merged

come-nc modified the milestones: Nextcloud 31, Nextcloud 32 Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(encryption): Migrate from hooks to events #48560

feat(encryption): Migrate from hooks to events #48560

come-nc commented Oct 3, 2024

come-nc commented Nov 26, 2024

icewind1991 Jan 21, 2025

		$fullPath = $this->getFullPath($path);
		echo "[DEBUG] unlink($fullPath) " . __FILE__ . ':' . __LINE__ . "\n";

		if ($this->util->isExcluded($fullPath)) {
		echo "[DEBUG] excluded $fullPath " . __FILE__ . ':' . __LINE__ . "\n";

feat(encryption): Migrate from hooks to events #48560

Are you sure you want to change the base?

feat(encryption): Migrate from hooks to events #48560

Conversation

come-nc commented Oct 3, 2024

Summary

Checklist

come-nc commented Nov 26, 2024

icewind1991 Jan 21, 2025

Choose a reason for hiding this comment