Skip to content

Commit 2db1b50

Browse files
oleksandr-ncoleksandr-nc
committed
fix: keep the settings pages working when a stored secret cannot be decrypted
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
1 parent 429276a commit 2db1b50

2 files changed

Lines changed: 136 additions & 11 deletions

File tree

‎lib/Service/SecretService.php‎

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use OCP\IUserManager;
1414
use OCP\PreConditionNotMetException;
1515
use OCP\Security\ICrypto;
16+
use Psr\Log\LoggerInterface;
1617

1718
/**
1819
* Service to make requests to GitHub v3 (JSON) API
@@ -23,6 +24,7 @@ public function __construct(
2324
private IConfig $config,
2425
private IUserManager $userManager,
2526
private ICrypto $crypto,
27+
private LoggerInterface $logger,
2628
) {
2729
}
2830

@@ -45,15 +47,11 @@ public function setEncryptedUserValue(string $userId, string $key, string $value
4547
/**
4648
* @param string $userId
4749
* @param string $key
48-
* @return string
49-
* @throws Exception
50+
* @return string the decrypted value, or an empty string if it cannot be decrypted
5051
*/
5152
public function getEncryptedUserValue(string $userId, string $key): string {
5253
$storedValue = $this->config->getUserValue($userId, Application::APP_ID, $key);
53-
if ($storedValue === '') {
54-
return '';
55-
}
56-
return $this->crypto->decrypt($storedValue);
54+
return $this->decryptOrDiscard($storedValue, $key, $userId);
5755
}
5856

5957
/**
@@ -72,15 +70,37 @@ public function setEncryptedAppValue(string $key, string $value): void {
7270

7371
/**
7472
* @param string $key
75-
* @return string
76-
* @throws Exception
73+
* @return string the decrypted value, or an empty string if it cannot be decrypted
7774
*/
7875
public function getEncryptedAppValue(string $key): string {
7976
$storedValue = $this->config->getAppValue(Application::APP_ID, $key);
77+
return $this->decryptOrDiscard($storedValue, $key, null);
78+
}
79+
80+
/**
81+
* Decrypt a stored secret, treating one that cannot be decrypted as unset.
82+
*
83+
* A value that is not valid ciphertext — stored as plaintext, or encrypted under
84+
* a secret that has since changed — makes ICrypto::decrypt() throw. Both settings
85+
* classes read secrets in getForm(), and the settings controller renders every
86+
* app's section, so letting that escape returns 500 for the whole "Connected
87+
* accounts" page: not just ours, but every installed integration's.
88+
*/
89+
private function decryptOrDiscard(string $storedValue, string $key, ?string $userId): string {
8090
if ($storedValue === '') {
8191
return '';
8292
}
83-
return $this->crypto->decrypt($storedValue);
93+
94+
try {
95+
return $this->crypto->decrypt($storedValue);
96+
} catch (Exception $e) {
97+
$this->logger->warning('Could not decrypt the stored "' . $key . '" value, treating it as unset', [
98+
'exception' => $e,
99+
'userId' => $userId,
100+
'app' => Application::APP_ID,
101+
]);
102+
return '';
103+
}
84104
}
85105

86106
/**
@@ -92,8 +112,7 @@ public function getEncryptedAppValue(string $key): string {
92112
*
93113
* @param string|null $userId
94114
* @param bool $endpointUsesDefaultToken
95-
* @return string
96-
* @throws Exception
115+
* @return string the access token, or an empty string if there is none usable
97116
*/
98117
public function getAccessToken(?string $userId, bool $endpointUsesDefaultToken = false): string {
99118
// use user access token in priority

‎tests/unit/Service/SecretServiceTest.php‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2020 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace OCA\Github\Tests\Unit\Service;
9+
10+
use Exception;
11+
use OCA\Github\AppInfo\Application;
12+
use OCA\Github\Service\SecretService;
13+
use OCP\IConfig;
14+
use OCP\IUserManager;
15+
use OCP\Security\ICrypto;
16+
use PHPUnit\Framework\MockObject\MockObject;
17+
use PHPUnit\Framework\TestCase;
18+
use Psr\Log\LoggerInterface;
19+
20+
class SecretServiceTest extends TestCase {
21+
22+
private IConfig|MockObject $config;
23+
private IUserManager|MockObject $userManager;
24+
private ICrypto|MockObject $crypto;
25+
private LoggerInterface|MockObject $logger;
26+
private SecretService $secretService;
27+
28+
protected function setUp(): void {
29+
parent::setUp();
30+
$this->config = $this->createMock(IConfig::class);
31+
$this->userManager = $this->createMock(IUserManager::class);
32+
$this->crypto = $this->createMock(ICrypto::class);
33+
$this->logger = $this->createMock(LoggerInterface::class);
34+
$this->secretService = new SecretService(
35+
$this->config, $this->userManager, $this->crypto, $this->logger
36+
);
37+
}
38+
39+
public function testUserValueIsDecrypted(): void {
40+
$this->config->method('getUserValue')->willReturn('ciphertext');
41+
$this->crypto->expects($this->once())->method('decrypt')->with('ciphertext')->willReturn('plain');
42+
43+
$this->assertSame('plain', $this->secretService->getEncryptedUserValue('alice', 'token'));
44+
}
45+
46+
public function testAppValueIsDecrypted(): void {
47+
$this->config->method('getAppValue')->willReturn('ciphertext');
48+
$this->crypto->expects($this->once())->method('decrypt')->with('ciphertext')->willReturn('plain');
49+
50+
$this->assertSame('plain', $this->secretService->getEncryptedAppValue('client_id'));
51+
}
52+
53+
public function testUnsetValuesAreNotDecrypted(): void {
54+
$this->config->method('getUserValue')->willReturn('');
55+
$this->config->method('getAppValue')->willReturn('');
56+
$this->crypto->expects($this->never())->method('decrypt');
57+
58+
$this->assertSame('', $this->secretService->getEncryptedUserValue('alice', 'token'));
59+
$this->assertSame('', $this->secretService->getEncryptedAppValue('client_id'));
60+
}
61+
62+
/**
63+
* A stored value that is not valid ciphertext must not escape as an exception.
64+
* Both settings classes read secrets in getForm(), and the settings controller
65+
* renders every app's section, so throwing here returns 500 for the entire
66+
* "Connected accounts" page rather than just this app's part of it.
67+
*/
68+
public function testUndecryptableUserValueIsTreatedAsUnset(): void {
69+
$this->config->method('getUserValue')->willReturn('not-actually-ciphertext');
70+
$this->crypto->method('decrypt')
71+
->willThrowException(new Exception('Authenticated ciphertext could not be decoded.'));
72+
$this->logger->expects($this->once())->method('warning');
73+
74+
$this->assertSame('', $this->secretService->getEncryptedUserValue('alice', 'token'));
75+
}
76+
77+
public function testUndecryptableAppValueIsTreatedAsUnset(): void {
78+
$this->config->method('getAppValue')->willReturn('not-actually-ciphertext');
79+
$this->crypto->method('decrypt')
80+
->willThrowException(new Exception('Authenticated ciphertext could not be decoded.'));
81+
$this->logger->expects($this->once())->method('warning');
82+
83+
$this->assertSame('', $this->secretService->getEncryptedAppValue('client_id'));
84+
}
85+
86+
/**
87+
* getAccessToken() builds on both getters, so an undecryptable token must leave
88+
* callers with "no token" rather than an exception surfacing in a controller.
89+
*/
90+
public function testAccessTokenIsEmptyWhenTheStoredTokenCannotBeDecrypted(): void {
91+
$this->config->method('getUserValue')->willReturn('not-actually-ciphertext');
92+
$this->config->method('getAppValue')->willReturn('');
93+
$this->crypto->method('decrypt')
94+
->willThrowException(new Exception('Authenticated ciphertext could not be decoded.'));
95+
96+
$this->assertSame('', $this->secretService->getAccessToken('alice'));
97+
}
98+
99+
public function testSettingAnEmptyValueDoesNotEncrypt(): void {
100+
$this->crypto->expects($this->never())->method('encrypt');
101+
$this->config->expects($this->once())->method('setUserValue')
102+
->with('alice', Application::APP_ID, 'token', '');
103+
104+
$this->secretService->setEncryptedUserValue('alice', 'token', '');
105+
}
106+
}

0 commit comments

Comments
 (0)