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.

… 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>

provokateurin

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

provokateurin · 2026-05-06T06:43:56Z

 				if ($app === 'encryption' && $key === 'recoveryAdminEnabled' && $default === '0') {
 					return '1';
 				}


This should also be migrated to getValueBool

provokateurin · 2026-05-06T06:44:31Z

-      <code><![CDATA[getAppValue]]></code>
      <code><![CDATA[getAppValue]]></code>
      <code><![CDATA[getUserValue]]></code>
-      <code><![CDATA[setAppValue]]></code>
      <code><![CDATA[setUserValue]]></code>


All of these should be migrated

provokateurin · 2026-05-06T06:45:00Z

 		}

-		$originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled');
+		$originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false);


Unnecessary change, since the default is already false.

provokateurin · 2026-05-06T06:45:33Z

 	#[\Override]
 	protected function execute(InputInterface $input, OutputInterface $output): int {
-		if ($this->config->getAppValue('core', 'encryption_enabled', 'no') === 'yes') {
+		$isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false);


Suggested change

$isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false);

$isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled');

Also unnecessary default value.

provokateurin · 2026-05-06T06:46:12Z

 			return 1;
 		}
-		$defaultModule = $this->config->getAppValue('core', 'default_encryption_module');
+		$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module', '');


Suggested change

$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module', '');

$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module');

Already the default.

provokateurin · 2026-05-06T06:46:48Z

-		$enabled = $this->config->getAppValue('core', 'encryption_enabled', 'no');
-		return $enabled === 'yes';
+		try {
+			return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false);


It's better if you inject IAppConfig into this class.

provokateurin · 2026-05-06T06:46:53Z

-		$enabled = $this->config->getAppValue('core', 'encryption_enabled', 'no');
-		return $enabled === 'yes';
+		try {
+			return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false);


Suggested change

return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false);

return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled);

provokateurin · 2026-05-06T06:47:59Z

+	/**
+	 * @group DB
+	 */


Suggested change

/**

* @group DB

*/

#[Group(name: 'DB')]

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 and others added 6 commits May 5, 2026 15:39

fix(encryption): Assume boolValue for commands

a088ddf

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

fix(encryption): Missed explicit typing

e292a0f

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

1bd6790

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

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

	$isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false);
	$isEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled');

	$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module', '');
	$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module');

	return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false);
	return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled);

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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