Skip to content

Commit 507c0a7

Browse files
cuppettcuppett
authored
Merge pull request #60002 from cuppett/cuppett/migrate-appconfig-keys-to-bool
refactor(encryption): Migrate appconfig keys to typed bool IAppConfig with repair step
2 parents 39d2dc2 + b86fa0d commit 507c0a7

18 files changed

Lines changed: 171 additions & 254 deletions

File tree

‎apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionMasterKeyUploadTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
namespace OCA\DAV\Tests\unit\Connector\Sabre\RequestTest;
1111

1212
use OC\Files\View;
13-
use OCP\IConfig;
13+
use OCP\IAppConfig;
1414
use OCP\ITempManager;
1515
use OCP\Server;
1616
use Test\Traits\EncryptionTrait;
@@ -30,7 +30,7 @@ protected function setupUser($name, $password): View {
3030
$tmpFolder = Server::get(ITempManager::class)->getTemporaryFolder();
3131
$this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]);
3232
// we use the master key
33-
Server::get(IConfig::class)->setAppValue('encryption', 'useMasterKey', '1');
33+
Server::get(IAppConfig::class)->setValueBool('encryption', 'useMasterKey', true);
3434
$this->setupForUser($name, $password);
3535
$this->loginWithEncryption($name);
3636
return new View('/' . $name . '/files');

‎apps/dav/tests/unit/Connector/Sabre/RequestTest/EncryptionUploadTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
namespace OCA\DAV\Tests\unit\Connector\Sabre\RequestTest;
1111

1212
use OC\Files\View;
13-
use OCP\IConfig;
13+
use OCP\IAppConfig;
1414
use OCP\ITempManager;
1515
use OCP\Server;
1616
use Test\Traits\EncryptionTrait;
@@ -30,7 +30,7 @@ protected function setupUser($name, $password): View {
3030
$tmpFolder = Server::get(ITempManager::class)->getTemporaryFolder();
3131
$this->registerMount($name, '\OC\Files\Storage\Local', '/' . $name, ['datadir' => $tmpFolder]);
3232
// we use per-user keys
33-
Server::get(IConfig::class)->setAppValue('encryption', 'useMasterKey', '0');
33+
Server::get(IAppConfig::class)->setValueBool('encryption', 'useMasterKey', false);
3434
$this->setupForUser($name, $password);
3535
$this->loginWithEncryption($name);
3636
return new View('/' . $name . '/files');

‎apps/encryption/lib/Recovery.php‎

Lines changed: 11 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,9 @@
1010

1111
use OC\Files\View;
1212
use OCA\Encryption\Crypto\Crypt;
13+
use OCP\Config\IUserConfig;
1314
use OCP\Encryption\IFile;
14-
use OCP\IConfig;
15+
use OCP\IAppConfig;
1516
use OCP\IUser;
1617
use OCP\IUserSession;
1718
use OCP\PreConditionNotMetException;
@@ -23,18 +24,16 @@ public function __construct(
2324
IUserSession $userSession,
2425
protected Crypt $crypt,
2526
private KeyManager $keyManager,
26-
private IConfig $config,
27+
private IAppConfig $appConfig,
28+
private IUserConfig $userConfig,
2729
private IFile $file,
2830
private View $view,
2931
) {
3032
$this->user = ($userSession->isLoggedIn()) ? $userSession->getUser() : null;
3133
}
3234

3335
public function enableAdminRecovery(string $password): bool {
34-
$appConfig = $this->config;
35-
$keyManager = $this->keyManager;
36-
37-
if (!$keyManager->recoveryKeyExists()) {
36+
if (!$this->keyManager->recoveryKeyExists()) {
3837
$keyPair = $this->crypt->createKeyPair();
3938
if (!is_array($keyPair)) {
4039
return false;
@@ -43,8 +42,8 @@ public function enableAdminRecovery(string $password): bool {
4342
$this->keyManager->setRecoveryKey($password, $keyPair);
4443
}
4544

46-
if ($keyManager->checkRecoveryPassword($password)) {
47-
$appConfig->setAppValue('encryption', 'recoveryAdminEnabled', '1');
45+
if ($this->keyManager->checkRecoveryPassword($password)) {
46+
$this->appConfig->setValueBool('encryption', 'recoveryAdminEnabled', true);
4847
return true;
4948
}
5049

@@ -74,7 +73,7 @@ public function disableAdminRecovery(string $recoveryPassword): bool {
7473

7574
if ($keyManager->checkRecoveryPassword($recoveryPassword)) {
7675
// Set recoveryAdmin as disabled
77-
$this->config->setAppValue('encryption', 'recoveryAdminEnabled', '0');
76+
$this->appConfig->setValueBool('encryption', 'recoveryAdminEnabled', false);
7877
return true;
7978
}
8079
return false;
@@ -87,29 +86,19 @@ public function disableAdminRecovery(string $recoveryPassword): bool {
8786
*/
8887
public function isRecoveryEnabledForUser(string $user = ''): bool {
8988
$uid = $user === '' ? $this->user->getUID() : $user;
90-
$recoveryMode = $this->config->getUserValue($uid,
91-
'encryption',
92-
'recoveryEnabled',
93-
'0');
94-
95-
return ($recoveryMode === '1');
89+
return $this->userConfig->getValueBool($uid, 'encryption', 'recoveryEnabled');
9690
}
9791

9892
/**
9993
* check if recovery is key is enabled by the administrator
10094
*/
10195
public function isRecoveryKeyEnabled(): bool {
102-
$enabled = $this->config->getAppValue('encryption', 'recoveryAdminEnabled', '0');
103-
104-
return ($enabled === '1');
96+
return $this->appConfig->getValueBool('encryption', 'recoveryAdminEnabled');
10597
}
10698

10799
public function setRecoveryForUser(bool $value): bool {
108100
try {
109-
$this->config->setUserValue($this->user->getUID(),
110-
'encryption',
111-
'recoveryEnabled',
112-
$value ? '1' : '0');
101+
$this->userConfig->setValueBool($this->user->getUID(), 'encryption', 'recoveryEnabled', $value);
113102

114103
if ($value) {
115104
$this->addRecoveryKeys('/' . $this->user->getUID() . '/files/');

‎apps/encryption/lib/Settings/Admin.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCA\Encryption\Util;
1717
use OCP\AppFramework\Http\TemplateResponse;
1818
use OCP\AppFramework\Services\IInitialState;
19+
use OCP\Config\IUserConfig;
1920
use OCP\IAppConfig;
2021
use OCP\IConfig;
2122
use OCP\IL10N;
@@ -35,6 +36,7 @@ public function __construct(
3536
private ISession $session,
3637
private IInitialState $initialState,
3738
private IAppConfig $appConfig,
39+
private IUserConfig $userConfig,
3840
) {
3941
}
4042

@@ -53,7 +55,8 @@ public function getForm() {
5355
new View(),
5456
$crypt,
5557
$this->userSession,
56-
$this->config,
58+
$this->appConfig,
59+
$this->userConfig,
5760
$this->userManager);
5861

5962
// Check if an adminRecovery account is enabled for recovering files after lost pwd

‎apps/encryption/lib/Util.php‎

Lines changed: 10 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@
1111
use OC\Files\Storage\Storage;
1212
use OC\Files\View;
1313
use OCA\Encryption\Crypto\Crypt;
14+
use OCP\Config\IUserConfig;
1415
use OCP\Files\Storage\IStorage;
15-
use OCP\IConfig;
16+
use OCP\IAppConfig;
1617
use OCP\IUser;
1718
use OCP\IUserManager;
1819
use OCP\IUserSession;
@@ -25,7 +26,8 @@ public function __construct(
2526
private View $files,
2627
private Crypt $crypt,
2728
IUserSession $userSession,
28-
private IConfig $config,
29+
private IAppConfig $appConfig,
30+
private IUserConfig $userConfig,
2931
private IUserManager $userManager,
3032
) {
3133
$this->user = $userSession->isLoggedIn() ? $userSession->getUser() : false;
@@ -38,12 +40,7 @@ public function __construct(
3840
* @return bool
3941
*/
4042
public function isRecoveryEnabledForUser($uid) {
41-
$recoveryMode = $this->config->getUserValue($uid,
42-
'encryption',
43-
'recoveryEnabled',
44-
'0');
45-
46-
return ($recoveryMode === '1');
43+
return $this->userConfig->getValueBool($uid, 'encryption', 'recoveryEnabled');
4744
}
4845

4946
/**
@@ -52,45 +49,28 @@ public function isRecoveryEnabledForUser($uid) {
5249
* @return bool
5350
*/
5451
public function shouldEncryptHomeStorage() {
55-
$encryptHomeStorage = $this->config->getAppValue(
56-
'encryption',
57-
'encryptHomeStorage',
58-
'1'
59-
);
60-
61-
return ($encryptHomeStorage === '1');
52+
return $this->appConfig->getValueBool('encryption', 'encryptHomeStorage', true);
6253
}
6354

6455
/**
6556
* set the home storage encryption on/off
6657
*
6758
* @param bool $encryptHomeStorage
6859
*/
69-
public function setEncryptHomeStorage($encryptHomeStorage) {
70-
$value = $encryptHomeStorage ? '1' : '0';
71-
$this->config->setAppValue(
72-
'encryption',
73-
'encryptHomeStorage',
74-
$value
75-
);
60+
public function setEncryptHomeStorage(bool $encryptHomeStorage) {
61+
$this->appConfig->setValueBool('encryption', 'encryptHomeStorage', $encryptHomeStorage);
7662
}
7763

7864
/**
7965
* check if master key is enabled
8066
*/
8167
public function isMasterKeyEnabled(): bool {
82-
$userMasterKey = $this->config->getAppValue('encryption', 'useMasterKey', '1');
83-
return ($userMasterKey === '1');
68+
return $this->appConfig->getValueBool('encryption', 'useMasterKey', true);
8469
}
8570

8671
public function setRecoveryForUser(bool $enabled): bool {
87-
$value = $enabled ? '1' : '0';
88-
8972
try {
90-
$this->config->setUserValue($this->user->getUID(),
91-
'encryption',
92-
'recoveryEnabled',
93-
$value);
73+
$this->userConfig->setValueBool($this->user->getUID(), 'encryption', 'recoveryEnabled', $enabled);
9474
return true;
9575
} catch (PreConditionNotMetException $e) {
9676
return false;

‎apps/encryption/tests/RecoveryTest.php‎

Lines changed: 26 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@
1414
use OCA\Encryption\Crypto\Crypt;
1515
use OCA\Encryption\KeyManager;
1616
use OCA\Encryption\Recovery;
17+
use OCP\Config\IUserConfig;
1718
use OCP\Encryption\IFile;
18-
use OCP\IConfig;
19+
use OCP\IAppConfig;
1920
use OCP\IUser;
2021
use OCP\IUserSession;
2122
use PHPUnit\Framework\MockObject\MockObject;
@@ -29,7 +30,8 @@ class RecoveryTest extends TestCase {
2930
private IUserSession&MockObject $userSessionMock;
3031
private IUser&MockObject $user;
3132
private KeyManager&MockObject $keyManagerMock;
32-
private IConfig&MockObject $configMock;
33+
private IAppConfig&MockObject $appConfigMock;
34+
private IUserConfig&MockObject $userConfigMock;
3335
private Crypt&MockObject $cryptMock;
3436

3537
private Recovery $instance;
@@ -56,7 +58,7 @@ public function testEnableAdminRecoverySuccessful(): void {
5658

5759
$this->assertTrue($this->instance->enableAdminRecovery('password'));
5860
$this->assertArrayHasKey('recoveryAdminEnabled', self::$tempStorage);
59-
$this->assertEquals(1, self::$tempStorage['recoveryAdminEnabled']);
61+
$this->assertTrue(self::$tempStorage['recoveryAdminEnabled']);
6062

6163
$this->assertTrue($this->instance->enableAdminRecovery('password'));
6264
}
@@ -83,7 +85,7 @@ public function testEnableAdminRecoveryCouldNotCheckPassword(): void {
8385

8486
$this->assertTrue($this->instance->enableAdminRecovery('password'));
8587
$this->assertArrayHasKey('recoveryAdminEnabled', self::$tempStorage);
86-
$this->assertEquals(1, self::$tempStorage['recoveryAdminEnabled']);
88+
$this->assertTrue(self::$tempStorage['recoveryAdminEnabled']);
8789

8890
$this->assertFalse($this->instance->enableAdminRecovery('password'));
8991
}
@@ -140,23 +142,23 @@ public function testDisableAdminRecovery(): void {
140142

141143
$this->assertArrayHasKey('recoveryAdminEnabled', self::$tempStorage);
142144
$this->assertTrue($this->instance->disableAdminRecovery('password'));
143-
$this->assertEquals(0, self::$tempStorage['recoveryAdminEnabled']);
145+
$this->assertFalse(self::$tempStorage['recoveryAdminEnabled']);
144146

145147
$this->assertFalse($this->instance->disableAdminRecovery('password'));
146148
}
147149

148150
public function testIsRecoveryEnabledForUser(): void {
149-
$this->configMock->expects($this->exactly(2))
150-
->method('getUserValue')
151-
->willReturnOnConsecutiveCalls('1', '0');
151+
$this->userConfigMock->expects($this->exactly(2))
152+
->method('getValueBool')
153+
->willReturnOnConsecutiveCalls(true, false);
152154

153155
$this->assertTrue($this->instance->isRecoveryEnabledForUser());
154156
$this->assertFalse($this->instance->isRecoveryEnabledForUser('admin'));
155157
}
156158

157159
public function testIsRecoveryKeyEnabled(): void {
158160
$this->assertFalse($this->instance->isRecoveryKeyEnabled());
159-
self::$tempStorage['recoveryAdminEnabled'] = '1';
161+
self::$tempStorage['recoveryAdminEnabled'] = true;
160162
$this->assertTrue($this->instance->isRecoveryKeyEnabled());
161163
}
162164

@@ -238,51 +240,31 @@ protected function setUp(): void {
238240

239241
$this->cryptMock = $this->getMockBuilder(Crypt::class)->disableOriginalConstructor()->getMock();
240242
$this->keyManagerMock = $this->getMockBuilder(KeyManager::class)->disableOriginalConstructor()->getMock();
241-
$this->configMock = $this->createMock(IConfig::class);
243+
$this->appConfigMock = $this->createMock(IAppConfig::class);
244+
$this->userConfigMock = $this->createMock(IUserConfig::class);
242245
$this->fileMock = $this->createMock(IFile::class);
243246
$this->viewMock = $this->createMock(View::class);
244247

245-
$this->configMock->expects($this->any())
246-
->method('setAppValue')
247-
->willReturnCallback([$this, 'setValueTester']);
248+
$this->appConfigMock->expects($this->any())
249+
->method('setValueBool')
250+
->willReturnCallback(function (string $app, string $key, bool $value): bool {
251+
self::$tempStorage[$key] = $value;
252+
return true;
253+
});
248254

249-
$this->configMock->expects($this->any())
250-
->method('getAppValue')
251-
->willReturnCallback([$this, 'getValueTester']);
255+
$this->appConfigMock->expects($this->any())
256+
->method('getValueBool')
257+
->willReturnCallback(function (string $app, string $key, bool $default = false): bool {
258+
return self::$tempStorage[$key] ?? $default;
259+
});
252260

253261
$this->instance = new Recovery($this->userSessionMock,
254262
$this->cryptMock,
255263
$this->keyManagerMock,
256-
$this->configMock,
264+
$this->appConfigMock,
265+
$this->userConfigMock,
257266
$this->fileMock,
258267
$this->viewMock);
259268
}
260269

261-
/**
262-
* @param $app
263-
* @param $key
264-
* @param $value
265-
*/
266-
public function setValueTester($app, $key, $value) {
267-
self::$tempStorage[$key] = $value;
268-
}
269-
270-
/**
271-
* @param $key
272-
*/
273-
public function removeValueTester($key) {
274-
unset(self::$tempStorage[$key]);
275-
}
276-
277-
/**
278-
* @param $app
279-
* @param $key
280-
* @return mixed
281-
*/
282-
public function getValueTester($app, $key) {
283-
if (!empty(self::$tempStorage[$key])) {
284-
return self::$tempStorage[$key];
285-
}
286-
return null;
287-
}
288270
}

0 commit comments

Comments
 (0)