handle oauth client secret decryption errors

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
nextcloud · May 23, 2023 · 8b842fa · 8b842fa
1 parent a5456d1
commit 8b842fa
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 42 deletions.
diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml
@@ -5,7 +5,7 @@
 	<name>OAuth 2.0</name>
 	<summary>Allows OAuth2 compatible authentication from other web applications.</summary>
 	<description>The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications.</description>
-	<version>1.16.0</version>
+	<version>1.16.1</version>
 	<licence>agpl</licence>
 	<author>Lukas Reschke</author>
 	<namespace>OAuth2</namespace>

diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php
@@ -42,40 +42,23 @@
 use OCP\IRequest;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
+use Psr\Log\LoggerInterface;
 
 class OauthApiController extends Controller {
-	/** @var AccessTokenMapper */
-	private $accessTokenMapper;
-	/** @var ClientMapper */
-	private $clientMapper;
-	/** @var ICrypto */
-	private $crypto;
-	/** @var TokenProvider */
-	private $tokenProvider;
-	/** @var ISecureRandom */
-	private $secureRandom;
-	/** @var ITimeFactory */
-	private $time;
-	/** @var Throttler */
-	private $throttler;
-
-	public function __construct(string $appName,
-								IRequest $request,
-								ICrypto $crypto,
-								AccessTokenMapper $accessTokenMapper,
-								ClientMapper $clientMapper,
-								TokenProvider $tokenProvider,
-								ISecureRandom $secureRandom,
-								ITimeFactory $time,
-								Throttler $throttler) {
+
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private ICrypto $crypto,
+		private AccessTokenMapper $accessTokenMapper,
+		private ClientMapper $clientMapper,
+		private TokenProvider $tokenProvider,
+		private ISecureRandom $secureRandom,
+		private ITimeFactory $time,
+		private LoggerInterface $logger,
+		private Throttler $throttler
+	) {
 		parent::__construct($appName, $request);
-		$this->crypto = $crypto;
-		$this->accessTokenMapper = $accessTokenMapper;
-		$this->clientMapper = $clientMapper;
-		$this->tokenProvider = $tokenProvider;
-		$this->secureRandom = $secureRandom;
-		$this->time = $time;
-		$this->throttler = $throttler;
 	}
 
 	/**
@@ -124,8 +107,15 @@ public function getToken($grant_type, $code, $refresh_token, $client_id, $client
 			$client_secret = $this->request->server['PHP_AUTH_PW'];
 		}
 
+		try {
+			$storedClientSecret = $this->crypto->decrypt($client->getSecret());
+		} catch (\Exception $e) {
+			$this->logger->error('OAuth client secret decryption error', ['exception' => $e]);
+			return new JSONResponse([
+				'error' => 'invalid_client',
+			], Http::STATUS_BAD_REQUEST);
+		}
 		// The client id and secret must match. Else we don't provide an access token!
-		$storedClientSecret = $this->crypto->decrypt($client->getSecret());
 		if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) {
 			return new JSONResponse([
 				'error' => 'invalid_client',

diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php
@@ -32,14 +32,16 @@
 use OCP\Security\ICrypto;
 use OCP\Settings\ISettings;
 use OCP\IURLGenerator;
+use Psr\Log\LoggerInterface;
 
 class Admin implements ISettings {
 
 	public function __construct(
 		private IInitialState $initialState,
 		private ClientMapper $clientMapper,
 		private IURLGenerator $urlGenerator,
-		private ICrypto $crypto
+		private ICrypto $crypto,
+		private LoggerInterface $logger,
 	) {
 	}
 
@@ -48,14 +50,18 @@ public function getForm(): TemplateResponse {
 		$result = [];
 
 		foreach ($clients as $client) {
-			$secret = $this->crypto->decrypt($client->getSecret());
-			$result[] = [
-				'id' => $client->getId(),
-				'name' => $client->getName(),
-				'redirectUri' => $client->getRedirectUri(),
-				'clientId' => $client->getClientIdentifier(),
-				'clientSecret' => $secret,
-			];
+			try {
+				$secret = $this->crypto->decrypt($client->getSecret());
+				$result[] = [
+					'id' => $client->getId(),
+					'name' => $client->getName(),
+					'redirectUri' => $client->getRedirectUri(),
+					'clientId' => $client->getClientIdentifier(),
+					'clientSecret' => $secret,
+				];
+			} catch (\Exception $e) {
+				$this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]);
+			}
 		}
 		$this->initialState->provideInitialState('clients', $result);
 		$this->initialState->provideInitialState('oauth2-doc-link', $this->urlGenerator->linkToDocs('admin-oauth2'));

diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php
@@ -43,6 +43,7 @@
 use OCP\IRequest;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 /* We have to use this to add a property to the mocked request and avoid warnings about dynamic properties on PHP>=8.2 */
@@ -67,6 +68,8 @@ class OauthApiControllerTest extends TestCase {
 	private $time;
 	/** @var Throttler|\PHPUnit\Framework\MockObject\MockObject */
 	private $throttler;
+	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
+	private $logger;
 	/** @var OauthApiController */
 	private $oauthApiController;
 
@@ -81,6 +84,7 @@ protected function setUp(): void {
 		$this->secureRandom = $this->createMock(ISecureRandom::class);
 		$this->time = $this->createMock(ITimeFactory::class);
 		$this->throttler = $this->createMock(Throttler::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->oauthApiController = new OauthApiController(
 			'oauth2',
@@ -91,6 +95,7 @@ protected function setUp(): void {
 			$this->tokenProvider,
 			$this->secureRandom,
 			$this->time,
+			$this->logger,
 			$this->throttler
 		);
 	}