Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cuppett wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7a1ae9
refactor(encryption): Migrate appconfig keys to typed bool IAppConfig…
cuppett Apr 29, 2026
ee91b97
fix(encryption): Assume boolValue for commands
cuppett Apr 30, 2026
d837de5
fix(encryption): Missed explicit typing
cuppett Apr 30, 2026
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
1 change: 1 addition & 0 deletions apps/encryption/lib/Settings/Admin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public function getForm() {
$crypt,
$this->userSession,
$this->config,
$this->appConfig,
$this->userManager);

// Check if an adminRecovery account is enabled for recovering files after lost pwd
Expand Down
17 changes: 4 additions & 13 deletions apps/encryption/lib/Util.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use OC\Files\View;
use OCA\Encryption\Crypto\Crypt;
use OCP\Files\Storage\IStorage;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserManager;
Expand All @@ -25,6 +26,7 @@ public function __construct(
private Crypt $crypt,
IUserSession $userSession,
private IConfig $config,
private IAppConfig $appConfig,
private IUserManager $userManager,
) {
$this->user = $userSession->isLoggedIn() ? $userSession->getUser() : false;
Expand All @@ -51,13 +53,7 @@ public function isRecoveryEnabledForUser($uid) {
* @return bool
*/
public function shouldEncryptHomeStorage() {
$encryptHomeStorage = $this->config->getAppValue(
'encryption',
'encryptHomeStorage',
'1'
);

return ($encryptHomeStorage === '1');
return $this->appConfig->getValueBool('encryption', 'encryptHomeStorage', true);
}

/**
Expand All @@ -66,12 +62,7 @@ public function shouldEncryptHomeStorage() {
* @param bool $encryptHomeStorage
*/
public function setEncryptHomeStorage($encryptHomeStorage) {
$value = $encryptHomeStorage ? '1' : '0';
$this->config->setAppValue(
'encryption',
'encryptHomeStorage',
$value
);
$this->appConfig->setValueBool('encryption', 'encryptHomeStorage', $encryptHomeStorage);
}

/**
Expand Down
6 changes: 2 additions & 4 deletions apps/encryption/tests/Settings/AdminTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,15 @@ public function testGetForm(): void {
$this->appConfig
->method('getValueBool')
->willReturnMap([
['encryption', 'recoveryAdminEnabled', true]
['encryption', 'recoveryAdminEnabled', true],
['encryption', 'encryptHomeStorage', true, true],
]);
$this->config
->method('getAppValue')
->willReturnCallback(function ($app, $key, $default) {
if ($app === 'encryption' && $key === 'recoveryAdminEnabled' && $default === '0') {
return '1';
}
if ($app === 'encryption' && $key === 'encryptHomeStorage' && $default === '1') {
return '1';
}
return $default;
});

Expand Down
29 changes: 16 additions & 13 deletions apps/encryption/tests/UtilTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OCA\Encryption\Util;
use OCP\Files\Mount\IMountPoint;
use OCP\Files\Storage\IStorage;
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IUser;
use OCP\IUserManager;
Expand All @@ -26,6 +27,7 @@ class UtilTest extends TestCase {
protected Util $instance;
protected static $tempStorage = [];

protected IAppConfig&MockObject $appConfigMock;
protected IConfig&MockObject $configMock;
protected View&MockObject $filesMock;
protected IUserManager&MockObject $userManagerMock;
Expand Down Expand Up @@ -78,6 +80,7 @@ protected function setUp(): void {
->willReturn(true);

$this->configMock = $this->createMock(IConfig::class);
$this->appConfigMock = $this->createMock(IAppConfig::class);

$this->configMock->expects($this->any())
->method('getUserValue')
Expand All @@ -87,7 +90,7 @@ protected function setUp(): void {
->method('setUserValue')
->willReturnCallback([$this, 'setValueTester']);

$this->instance = new Util($this->filesMock, $cryptMock, $userSessionMock, $this->configMock, $this->userManagerMock);
$this->instance = new Util($this->filesMock, $cryptMock, $userSessionMock, $this->configMock, $this->appConfigMock, $this->userManagerMock);
}

/**
Expand Down Expand Up @@ -136,13 +139,13 @@ public static function dataTestIsMasterKeyEnabled(): array {
}

/**
* @param string $returnValue return value from getAppValue()
* @param bool $returnValue return value from getValueBool()
* @param bool $expected
*/
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestShouldEncryptHomeStorage')]
public function testShouldEncryptHomeStorage($returnValue, $expected): void {
$this->configMock->expects($this->once())->method('getAppValue')
->with('encryption', 'encryptHomeStorage', '1')
public function testShouldEncryptHomeStorage(bool $returnValue, bool $expected): void {
$this->appConfigMock->expects($this->once())->method('getValueBool')
->with('encryption', 'encryptHomeStorage', true)
->willReturn($returnValue);

$this->assertSame($expected,
Expand All @@ -151,26 +154,26 @@ public function testShouldEncryptHomeStorage($returnValue, $expected): void {

public static function dataTestShouldEncryptHomeStorage(): array {
return [
['1', true],
['0', false]
[true, true],
[false, false]
];
}

/**
* @param $value
* @param $expected
* @param bool $value
* @param bool $expected
*/
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestSetEncryptHomeStorage')]
public function testSetEncryptHomeStorage($value, $expected): void {
$this->configMock->expects($this->once())->method('setAppValue')
public function testSetEncryptHomeStorage(bool $value, bool $expected): void {
$this->appConfigMock->expects($this->once())->method('setValueBool')
->with('encryption', 'encryptHomeStorage', $expected);
$this->instance->setEncryptHomeStorage($value);
}

public static function dataTestSetEncryptHomeStorage(): array {
return [
[true, '1'],
[false, '0']
[true, true],
[false, false]
];
}

Expand Down
3 changes: 2 additions & 1 deletion apps/provisioning_api/lib/Controller/AppConfigController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ protected function verifyConfigKey(string $app, string $key, string $value) {
throw new \InvalidArgumentException('The given key can not be set');
}

if ($app === 'core' && $key === 'encryption_enabled' && $value !== 'yes') {
if ($app === 'core' && $key === 'encryption_enabled'
&& !in_array(strtolower(trim($value)), ['yes', '1', 'true', 'on'], true)) {
Comment on lines +206 to +207
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)

Copy link
Copy Markdown
Collaborator

@artonge artonge May 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This is a controller, so the value coming in is set by an HTTP API consumer, I think we should not support more values than we did in the past.

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

Is this related or wrong comment?

throw new \InvalidArgumentException('The given key can not be set');
}

Expand Down
6 changes: 6 additions & 0 deletions apps/provisioning_api/tests/Controller/AppConfigControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,10 @@ public static function dataVerifyConfigKey(): array {
['dav', 'public_route', ''],
['files', 'remote_route', ''],
['core', 'encryption_enabled', 'yes'],
['core', 'encryption_enabled', '1'],
['core', 'encryption_enabled', 'true'],
['core', 'encryption_enabled', 'YES'],
['core', 'encryption_enabled', 'on'],
];
}

Expand All @@ -384,6 +388,8 @@ public static function dataVerifyConfigKeyThrows(): array {
['contacts', 'types', ''],
['core', 'encryption_enabled', 'no'],
['core', 'encryption_enabled', ''],
['core', 'encryption_enabled', '0'],
['core', 'encryption_enabled', 'false'],
['core', 'public_files', ''],
['core', 'public_dav', ''],
['core', 'remote_files', ''],
Expand Down
15 changes: 0 additions & 15 deletions build/psalm-baseline.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,10 +1300,8 @@
</file>
<file src="apps/encryption/lib/Util.php">
<DeprecatedMethod>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[getUserValue]]></code>
<code><![CDATA[setAppValue]]></code>
<code><![CDATA[setUserValue]]></code>
</DeprecatedMethod>
<InternalMethod>
Expand Down Expand Up @@ -3023,19 +3021,6 @@
<code><![CDATA[\OC_Util::tearDownFS()]]></code>
</DeprecatedMethod>
</file>
<file src="core/Command/Encryption/Disable.php">
<DeprecatedMethod>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[setAppValue]]></code>
</DeprecatedMethod>
</file>
<file src="core/Command/Encryption/Enable.php">
<DeprecatedMethod>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[getAppValue]]></code>
<code><![CDATA[setAppValue]]></code>
</DeprecatedMethod>
</file>
<file src="core/Command/Encryption/MigrateKeyStorage.php">
<DeprecatedClass>
<code><![CDATA[\OC_Util::setupFS($uid)]]></code>
Expand Down
2 changes: 1 addition & 1 deletion core/Command/Encryption/DecryptAll.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
return 1;
}

$originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled');
$originallyEnabled = $this->appConfig->getValueBool('core', 'encryption_enabled', false);
try {
if ($originallyEnabled) {
$output->write('Disable server side encryption... ');
Expand Down
9 changes: 5 additions & 4 deletions core/Command/Encryption/Disable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
*/
namespace OC\Core\Command\Encryption;

use OCP\IConfig;
use OCP\IAppConfig;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class Disable extends Command {
public function __construct(
protected IConfig $config,
protected IAppConfig $appConfig,
) {
parent::__construct();
}
Expand All @@ -31,10 +31,11 @@ protected function configure() {

#[\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);
if (!$isEnabled) {
$output->writeln('Encryption is already disabled');
} else {
$this->config->setAppValue('core', 'encryption_enabled', 'no');
$this->appConfig->setValueBool('core', 'encryption_enabled', false);
$output->writeln('<info>Encryption disabled</info>');
}
return 0;
Expand Down
11 changes: 6 additions & 5 deletions core/Command/Encryption/Enable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
namespace OC\Core\Command\Encryption;

use OCP\Encryption\IManager;
use OCP\IConfig;
use OCP\IAppConfig;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

class Enable extends Command {
public function __construct(
protected IConfig $config,
protected IAppConfig $appConfig,
protected IManager $encryptionManager,
) {
parent::__construct();
Expand All @@ -33,10 +33,11 @@ protected function configure() {

#[\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);
if ($isEnabled) {
$output->writeln('Encryption is already enabled');
} else {
$this->config->setAppValue('core', 'encryption_enabled', 'yes');
$this->appConfig->setValueBool('core', 'encryption_enabled', true);
$output->writeln('<info>Encryption enabled</info>');
}
$output->writeln('');
Expand All @@ -46,7 +47,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$output->writeln('<error>No encryption module is loaded</error>');
return 1;
}
$defaultModule = $this->config->getAppValue('core', 'default_encryption_module');
$defaultModule = $this->appConfig->getValueString('core', 'default_encryption_module', '');
if ($defaultModule === '') {
$output->writeln('<error>No default module is set</error>');
return 1;
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2094,6 +2094,7 @@
'OC\\Repair\\RepairInvalidShares' => $baseDir . '/lib/private/Repair/RepairInvalidShares.php',
'OC\\Repair\\RepairLogoDimension' => $baseDir . '/lib/private/Repair/RepairLogoDimension.php',
'OC\\Repair\\RepairMimeTypes' => $baseDir . '/lib/private/Repair/RepairMimeTypes.php',
'OC\\Repair\\RetypeEncryptionConfigKeys' => $baseDir . '/lib/private/Repair/RetypeEncryptionConfigKeys.php',
'OC\\RichObjectStrings\\RichTextFormatter' => $baseDir . '/lib/private/RichObjectStrings/RichTextFormatter.php',
'OC\\RichObjectStrings\\Validator' => $baseDir . '/lib/private/RichObjectStrings/Validator.php',
'OC\\Route\\CachingRouter' => $baseDir . '/lib/private/Route/CachingRouter.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2135,6 +2135,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Repair\\RepairInvalidShares' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairInvalidShares.php',
'OC\\Repair\\RepairLogoDimension' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairLogoDimension.php',
'OC\\Repair\\RepairMimeTypes' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairMimeTypes.php',
'OC\\Repair\\RetypeEncryptionConfigKeys' => __DIR__ . '/../../..' . '/lib/private/Repair/RetypeEncryptionConfigKeys.php',
'OC\\RichObjectStrings\\RichTextFormatter' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/RichTextFormatter.php',
'OC\\RichObjectStrings\\Validator' => __DIR__ . '/../../..' . '/lib/private/RichObjectStrings/Validator.php',
'OC\\Route\\CachingRouter' => __DIR__ . '/../../..' . '/lib/private/Route/CachingRouter.php',
Expand Down
15 changes: 13 additions & 2 deletions lib/private/Encryption/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
use OC\ServiceUnavailableException;
use OCP\Encryption\IEncryptionModule;
use OCP\Encryption\IManager;
use OCP\Exceptions\AppConfigTypeConflictException;
use OCP\Files\Mount\IMountPoint;
use OCP\Files\Storage\IStorage;
use OCP\IConfig;
use OCP\IL10N;
use OCP\Server;
use Psr\Log\LoggerInterface;

class Manager implements IManager {
Expand Down Expand Up @@ -48,8 +50,17 @@ public function isEnabled() {
return false;
}

$enabled = $this->config->getAppValue('core', 'encryption_enabled', 'no');
return $enabled === 'yes';
try {
return Server::get(\OCP\IAppConfig::class)->getValueBool('core', 'encryption_enabled', false);
} catch (AppConfigTypeConflictException) {
// Stored as VALUE_STRING from a pre-upgrade installation.
// RetypeEncryptionConfigKeys repair step will fix the type on occ upgrade.
$raw = Server::get(\OCP\IAppConfig::class)->getValueString('core', 'encryption_enabled', 'no');
return in_array(strtolower(trim($raw)), ['1', 'true', 'yes', 'on'], true);
} catch (\Throwable) {
// DB not ready (e.g. oc_appconfig does not yet exist during install).
return false;
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions lib/private/Repair.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
use OC\Repair\RepairInvalidShares;
use OC\Repair\RepairLogoDimension;
use OC\Repair\RepairMimeTypes;
use OC\Repair\RetypeEncryptionConfigKeys;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use OCP\IDBConnection;
Expand Down Expand Up @@ -157,6 +158,7 @@ public function addStep(IRepairStep|string $repairStep, bool $includeExpensive =
*/
public static function getRepairSteps(bool $includeExpensive = false): array {
$repairSteps = [
Server::get(RetypeEncryptionConfigKeys::class),
new Collation(Server::get(IConfig::class), Server::get(LoggerInterface::class), Server::get(IDBConnection::class), false),
Server::get(CleanTags::class),
Server::get(RepairInvalidShares::class),
Expand Down
Loading
Loading