refactor(encryption): Migrate appconfig keys to typed bool IAppConfig with repair step by cuppett · Pull Request #60002 · nextcloud/server

cuppett · 2026-04-29T23:58:51Z

Summary

Switch all encryption config reads/writes from deprecated string-typed IConfig to bool-typed IAppConfig (getValueBool/setValueBool). Adds RetypeEncryptionConfigKeys repair step to retype existing string values to bool on upgrade. Includes lazy IAppConfig resolution in Manager and AppConfigTypeConflictException fallbacks throughout for safety during the upgrade window.

Spawned from fix(encryption): Refactor EncryptionWrapper, fix zero-byte size, add S3 tests #57279
Resolves: [Bug]: de-crypting then re-encrypting (SSE) triggers conflict between new type (mixed) and old type (boolean) #58778

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)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

The content of this PR was partly or fully generated using AI

cuppett

I don't know the convention when trying to refactor the legacy values out. Happy to simplify to only the minimal set or wait until next version for another PR. Just LMK.

cuppett · 2026-05-01T13:49:05Z

The psalm things (DAV) seem to be coming from the master branch (not related to this PR). Do I need to fix that or can it be overridden to merge?

artonge

Looks good, thanks for splitting :)

The psalm error is indeed unrelated.

provokateurin

Thanks for the PR. I think it's not complete yet and a little bit more work to do.

provokateurin

Some of the tests for the config getters are a bit questionable now, because they just mirror the implementation exactly.

provokateurin · 2026-05-07T07:16:30Z

-			->willReturnCallback([$this, 'setValueTester']);
+		$this->appConfigMock->expects($this->any())
+			->method('setValueBool')
+			->willReturnCallback(function (string $app, string $key, bool $value): bool {
+				self::$tempStorage[$key] = $value;
+				return true;


Either you keep calling setValueTester or remove that method (and same for the other helper methods).

provokateurin · 2026-05-07T07:16:53Z

-			->willReturnCallback([$this, 'setValueTester']);
+		$this->userConfigMock->expects($this->any())
+			->method('setValueBool')
+			->willReturnCallback(function (string $userId, string $app, string $key, bool $value): bool {
+				self::$tempStorage[$key] = $value;
+				return true;


provokateurin · 2026-05-07T07:18:27Z

+			// DB not ready (e.g. oc_appconfig does not yet exist during install).
+			return false;


Could this have any negative side effect? @artonge

… with repair step Switch all encryption config reads/writes from deprecated string-typed IConfig to bool-typed IAppConfig (getValueBool/setValueBool). Adds RetypeEncryptionConfigKeys repair step to retype existing string values to bool on upgrade. Includes lazy IAppConfig resolution in Manager and AppConfigTypeConflictException fallbacks throughout for safety during the upgrade window. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

Signed-off-by: Stephen Cuppett <steve@cuppett.com>

@artonge

Apply suggestion from @artonge Co-authored-by: Louis <louis@chmn.me> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

Co-authored-by: Louis <louis@chmn.me> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

The IAppConfig API converts stored values to bool on read (getValueBool) and re-stamps the type on write (setValueBool), so legacy string-typed encryption config keys migrate lazily without an explicit repair step. Per PR review feedback, drop the repair step, its test, and the related AppConfigTypeConflictException fallback in Encryption\Manager::isEnabled that only existed to bridge the now-unneeded migration window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

… to match strict 'yes'-only validation The verifyConfigKey check on core.encryption_enabled was reverted to master's strict $value !== 'yes' in 626fadd per review feedback, but the test data providers still asserted the broader truthy set (1/true/YES/on). Drop those entries so the tests match the controller. This is validation, not storage — IAppConfig::setValueBool's broader input handling is unrelated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

Update to annotation from docblock comment Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

…AdminTest The production code already uses getValueBool for this key; the IConfig mock branch was unreachable. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

…yped APIs Replace IConfig::{get,set}UserValue for the per-user 'recoveryEnabled' key with IUserConfig::{getValueBool,setValueBool}, and IConfig::getAppValue for 'useMasterKey' with IAppConfig::getValueBool. IConfig is removed from Util and Recovery constructors entirely. Clears the DeprecatedMethod psalm-baseline entries for apps/encryption/lib/Util.php and the string-typed recoveryAdminEnabled calls that were still in Recovery.php. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

cuppett requested a review from a team as a code owner April 29, 2026 23:58

cuppett requested review from Altahrim, ArtificialOwl, leftybournes and salmart-dev and removed request for a team April 29, 2026 23:58

cuppett added this to the Nextcloud 34 milestone Apr 29, 2026

cuppett added 3. to review Waiting for reviews feature: encryption (server-side) labels Apr 29, 2026

cuppett mentioned this pull request Apr 30, 2026

fix(encryption): Refactor EncryptionWrapper with HomeMountPoint support #60001

Open

6 tasks

cuppett commented Apr 30, 2026

View reviewed changes

Comment thread core/Command/Encryption/Disable.php Outdated

CarlSchwan reviewed Apr 30, 2026

View reviewed changes

Comment thread core/Command/Encryption/DecryptAll.php Outdated

CarlSchwan reviewed Apr 30, 2026

View reviewed changes

Comment thread core/Command/Encryption/Disable.php Outdated

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 864ad6a to f3679b4 Compare April 30, 2026 11:10

cuppett requested a review from CarlSchwan April 30, 2026 11:11

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from f3679b4 to 39b6d31 Compare April 30, 2026 15:45

artonge reviewed Apr 30, 2026

View reviewed changes

Comment thread apps/encryption/lib/Util.php Outdated

Comment thread apps/provisioning_api/lib/Controller/AppConfigController.php Outdated

Comment thread lib/private/Repair/RetypeEncryptionConfigKeys.php Outdated

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from a6fff8f to e4f6ece Compare April 30, 2026 23:13

cuppett requested a review from provokateurin as a code owner April 30, 2026 23:13

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch 4 times, most recently from e5291af to 07461ee Compare May 1, 2026 12:24

cuppett requested a review from artonge May 1, 2026 13:50

artonge approved these changes May 1, 2026

View reviewed changes

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 6e52b67 to c4baa76 Compare May 1, 2026 17:05

artonge force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from c4baa76 to ef7b789 Compare May 5, 2026 14:10

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from ef7b789 to 9f0c402 Compare May 5, 2026 16:32

cuppett enabled auto-merge May 5, 2026 17:03

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 9f0c402 to 1db5b05 Compare May 5, 2026 19:39

provokateurin requested changes May 6, 2026

View reviewed changes

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch 4 times, most recently from ced9bf8 to ec6d206 Compare May 6, 2026 23:38

cuppett requested a review from provokateurin May 7, 2026 00:09

provokateurin reviewed May 7, 2026

View reviewed changes

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 5e7dd3b to 8a2e278 Compare May 7, 2026 11:29

cuppett and others added 10 commits May 7, 2026 09:17

fix(encryption): Assume boolValue for commands

cb9e167

Signed-off-by: Stephen Cuppett <steve@cuppett.com>

fix(encryption): Missed explicit typing

cd6d0d9

Apply suggestion from @artonge Co-authored-by: Louis <louis@chmn.me> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

fix(encryption): Simplify evaluation in AppConfigController

5240705

Co-authored-by: Louis <louis@chmn.me> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

fix(tests): Update tests/lib/Encryption/ManagerTest.php

1dc1cf4

Update to annotation from docblock comment Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

fix(encryption): No need for default values to be explicitly specified.

6a8a1af

Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Stephen Cuppett <steve@cuppett.com>

cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 8a2e278 to ce3532d Compare May 7, 2026 13:17

		// DB not ready (e.g. oc_appconfig does not yet exist during install).
		return false;

Uh oh!

Conversation

cuppett commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

AI (if applicable)

Uh oh!

cuppett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cuppett commented May 1, 2026

Uh oh!

artonge left a comment

Choose a reason for hiding this comment

Uh oh!

provokateurin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

provokateurin left a comment

Choose a reason for hiding this comment

Uh oh!

provokateurin May 7, 2026

Choose a reason for hiding this comment

Uh oh!

provokateurin May 7, 2026

Choose a reason for hiding this comment

Uh oh!

provokateurin May 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cuppett commented Apr 29, 2026 •

edited

Loading