diff --git a/lib/Service/SecretService.php b/lib/Service/SecretService.php index bd261c9c..c96a1c6d 100644 --- a/lib/Service/SecretService.php +++ b/lib/Service/SecretService.php @@ -13,6 +13,7 @@ use OCP\IUserManager; use OCP\PreConditionNotMetException; use OCP\Security\ICrypto; +use Psr\Log\LoggerInterface; /** * Service to make requests to GitHub v3 (JSON) API @@ -23,6 +24,7 @@ public function __construct( private IConfig $config, private IUserManager $userManager, private ICrypto $crypto, + private LoggerInterface $logger, ) { } @@ -45,15 +47,11 @@ public function setEncryptedUserValue(string $userId, string $key, string $value /** * @param string $userId * @param string $key - * @return string - * @throws Exception + * @return string the decrypted value, or an empty string if it cannot be decrypted */ public function getEncryptedUserValue(string $userId, string $key): string { $storedValue = $this->config->getUserValue($userId, Application::APP_ID, $key); - if ($storedValue === '') { - return ''; - } - return $this->crypto->decrypt($storedValue); + return $this->decryptOrDiscard($storedValue, $key, $userId); } /** @@ -72,15 +70,37 @@ public function setEncryptedAppValue(string $key, string $value): void { /** * @param string $key - * @return string - * @throws Exception + * @return string the decrypted value, or an empty string if it cannot be decrypted */ public function getEncryptedAppValue(string $key): string { $storedValue = $this->config->getAppValue(Application::APP_ID, $key); + return $this->decryptOrDiscard($storedValue, $key, null); + } + + /** + * Decrypt a stored secret, treating one that cannot be decrypted as unset. + * + * A value that is not valid ciphertext — stored as plaintext, or encrypted under + * a secret that has since changed — makes ICrypto::decrypt() throw. Both settings + * classes read secrets in getForm(), and the settings controller renders every + * app's section, so letting that escape returns 500 for the whole "Connected + * accounts" page: not just ours, but every installed integration's. + */ + private function decryptOrDiscard(string $storedValue, string $key, ?string $userId): string { if ($storedValue === '') { return ''; } - return $this->crypto->decrypt($storedValue); + + try { + return $this->crypto->decrypt($storedValue); + } catch (Exception $e) { + $this->logger->warning('Could not decrypt the stored "' . $key . '" value, treating it as unset', [ + 'exception' => $e, + 'userId' => $userId, + 'app' => Application::APP_ID, + ]); + return ''; + } } /** @@ -92,8 +112,7 @@ public function getEncryptedAppValue(string $key): string { * * @param string|null $userId * @param bool $endpointUsesDefaultToken - * @return string - * @throws Exception + * @return string the access token, or an empty string if there is none usable */ public function getAccessToken(?string $userId, bool $endpointUsesDefaultToken = false): string { // use user access token in priority diff --git a/tests/unit/Service/SecretServiceTest.php b/tests/unit/Service/SecretServiceTest.php new file mode 100644 index 00000000..a84f045d --- /dev/null +++ b/tests/unit/Service/SecretServiceTest.php @@ -0,0 +1,106 @@ +config = $this->createMock(IConfig::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->secretService = new SecretService( + $this->config, $this->userManager, $this->crypto, $this->logger + ); + } + + public function testUserValueIsDecrypted(): void { + $this->config->method('getUserValue')->willReturn('ciphertext'); + $this->crypto->expects($this->once())->method('decrypt')->with('ciphertext')->willReturn('plain'); + + $this->assertSame('plain', $this->secretService->getEncryptedUserValue('alice', 'token')); + } + + public function testAppValueIsDecrypted(): void { + $this->config->method('getAppValue')->willReturn('ciphertext'); + $this->crypto->expects($this->once())->method('decrypt')->with('ciphertext')->willReturn('plain'); + + $this->assertSame('plain', $this->secretService->getEncryptedAppValue('client_id')); + } + + public function testUnsetValuesAreNotDecrypted(): void { + $this->config->method('getUserValue')->willReturn(''); + $this->config->method('getAppValue')->willReturn(''); + $this->crypto->expects($this->never())->method('decrypt'); + + $this->assertSame('', $this->secretService->getEncryptedUserValue('alice', 'token')); + $this->assertSame('', $this->secretService->getEncryptedAppValue('client_id')); + } + + /** + * A stored value that is not valid ciphertext must not escape as an exception. + * Both settings classes read secrets in getForm(), and the settings controller + * renders every app's section, so throwing here returns 500 for the entire + * "Connected accounts" page rather than just this app's part of it. + */ + public function testUndecryptableUserValueIsTreatedAsUnset(): void { + $this->config->method('getUserValue')->willReturn('not-actually-ciphertext'); + $this->crypto->method('decrypt') + ->willThrowException(new Exception('Authenticated ciphertext could not be decoded.')); + $this->logger->expects($this->once())->method('warning'); + + $this->assertSame('', $this->secretService->getEncryptedUserValue('alice', 'token')); + } + + public function testUndecryptableAppValueIsTreatedAsUnset(): void { + $this->config->method('getAppValue')->willReturn('not-actually-ciphertext'); + $this->crypto->method('decrypt') + ->willThrowException(new Exception('Authenticated ciphertext could not be decoded.')); + $this->logger->expects($this->once())->method('warning'); + + $this->assertSame('', $this->secretService->getEncryptedAppValue('client_id')); + } + + /** + * getAccessToken() builds on both getters, so an undecryptable token must leave + * callers with "no token" rather than an exception surfacing in a controller. + */ + public function testAccessTokenIsEmptyWhenTheStoredTokenCannotBeDecrypted(): void { + $this->config->method('getUserValue')->willReturn('not-actually-ciphertext'); + $this->config->method('getAppValue')->willReturn(''); + $this->crypto->method('decrypt') + ->willThrowException(new Exception('Authenticated ciphertext could not be decoded.')); + + $this->assertSame('', $this->secretService->getAccessToken('alice')); + } + + public function testSettingAnEmptyValueDoesNotEncrypt(): void { + $this->crypto->expects($this->never())->method('encrypt'); + $this->config->expects($this->once())->method('setUserValue') + ->with('alice', Application::APP_ID, 'token', ''); + + $this->secretService->setEncryptedUserValue('alice', 'token', ''); + } +}