Skip to content

refactor(encryption): Migrate appconfig keys to typed bool IAppConfig with repair step#60002

Open
cuppett wants to merge 3 commits intonextcloud:masterfrom
cuppett:cuppett/migrate-appconfig-keys-to-bool
Open

refactor(encryption): Migrate appconfig keys to typed bool IAppConfig with repair step#60002
cuppett wants to merge 3 commits intonextcloud:masterfrom
cuppett:cuppett/migrate-appconfig-keys-to-bool

Conversation

@cuppett
Copy link
Copy Markdown
Contributor

@cuppett cuppett commented Apr 29, 2026 •
edited
Loading

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.

Checklist

AI (if applicable)

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

@cuppett cuppett requested a review from a team as a code owner April 29, 2026 23:58
@cuppett cuppett requested review from Altahrim, ArtificialOwl, leftybournes and salmart-dev and removed request for a team April 29, 2026 23:58
@cuppett cuppett added this to the Nextcloud 34 milestone Apr 29, 2026
@cuppett cuppett mentioned this pull request Apr 30, 2026
Open
6 tasks
cuppett
cuppett commented Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@cuppett cuppett left a comment

Choose a reason for hiding this comment

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

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.

Comment thread core/Command/Encryption/Disable.php Outdated
CarlSchwan
CarlSchwan reviewed Apr 30, 2026
View reviewed changes
Comment thread core/Command/Encryption/DecryptAll.php Outdated
CarlSchwan
CarlSchwan reviewed Apr 30, 2026
View reviewed changes
Comment thread core/Command/Encryption/Disable.php Outdated
@cuppett cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from 864ad6a to f3679b4 Compare April 30, 2026 11:10
@cuppett cuppett requested a review from CarlSchwan April 30, 2026 11:11
@cuppett cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from f3679b4 to 39b6d31 Compare April 30, 2026 15:45
artonge
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
Comment on lines +206 to +207
if ($app === 'core' && $key === 'encryption_enabled'
&& !in_array(strtolower(trim($value)), ['yes', '1', 'true', 'on'], true)) {
Copy link
Copy Markdown
Collaborator

@artonge artonge Apr 30, 2026

Choose a reason for hiding this comment

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

Suggested change
if ($app === 'core' && $key === 'encryption_enabled'
&& !in_array(strtolower(trim($value)), ['yes', '1', 'true', 'on'], true)) {
if ($app === 'core' && $key === 'encryption_enabled' && $value !== 'yes) {

Copy link
Copy Markdown
Contributor Author

@cuppett cuppett Apr 30, 2026

Choose a reason for hiding this comment

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

Will this be okay if repair is needed or the database on upgrades hasn't been migrated yet?

Copy link
Copy Markdown
Contributor Author

@cuppett cuppett Apr 30, 2026

Choose a reason for hiding this comment

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

In #57279, test and application bootstrapping was dying early on and added this in response from this comment:

#57279 (comment)

Comment thread lib/private/Repair/RetypeEncryptionConfigKeys.php
Copy link
Copy Markdown
Collaborator

@artonge artonge Apr 30, 2026

Choose a reason for hiding this comment

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

Is the repair step really needed? I just checked, and simply using the IAppConfig API seems to be enough.

server/core/Command/Encryption/Enable.php

Line 39 in 585ca98

$this->config->setAppValue('core', 'encryption_enabled', 'yes');

Copy link
Copy Markdown
Contributor Author

@cuppett cuppett Apr 30, 2026

Choose a reason for hiding this comment

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

My hope here is to get it all consistent with using getBoolVal and setBoolVal. That this PR has lots of changes where getBoolVal replaces things like:

		$encryptHomeStorage = $this->config->getAppValue(
			'encryption',
			'encryptHomeStorage',
			'1'
		);

		return ($encryptHomeStorage === '1');

Has me worried it's messy out there. How does this release get everything to a boolVal and the code uses it consistently here forward?

@cuppett cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from a6fff8f to e4f6ece Compare April 30, 2026 23:13
@cuppett cuppett requested a review from provokateurin as a code owner April 30, 2026 23:13
@cuppett cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from e4f6ece to ed8afc2 Compare April 30, 2026 23:14
cuppett and others added 3 commits April 30, 2026 20:39
@cuppett @claude
refactor(encryption): Migrate appconfig keys to typed bool IAppConfig…
f7a1ae9 
… 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>
@cuppett
fix(encryption): Assume boolValue for commands
ee91b97 
Signed-off-by: Stephen Cuppett <steve@cuppett.com>
@cuppett @artonge
fix(encryption): Missed explicit typing
d837de5 
Apply suggestion from @artonge

Co-authored-by: Louis <louis@chmn.me>
Signed-off-by: Stephen Cuppett <steve@cuppett.com>
@cuppett cuppett force-pushed the cuppett/migrate-appconfig-keys-to-bool branch from ed8afc2 to d837de5 Compare May 1, 2026 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge left review comments

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@salmart-dev salmart-dev Awaiting requested review from salmart-dev salmart-dev is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

@provokateurin provokateurin Awaiting requested review from provokateurin

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

3. to review Waiting for reviews feature: encryption (server-side)

Projects

None yet

Milestone

Nextcloud 34

Development

Successfully merging this pull request may close these issues.

[Bug]: de-crypting then re-encrypting (SSE) triggers conflict between new type (mixed) and old type (boolean)

3 participants

@cuppett @artonge @CarlSchwan