encrypt oauth2 client secrets

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>

2 files changed, 39 insertions, 62 deletions

diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php
index 910fdc99432..badafd3bb77 100644
--- a/apps/oauth2/lib/Controller/OauthApiController.php
+++ b/apps/oauth2/lib/Controller/OauthApiController.php
@@ -42,40 +42,23 @@ use OCP\AppFramework\Utility\ITimeFactory;
 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,16 @@ class OauthApiController extends Controller {
 			$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!
-		if ($client->getClientIdentifier() !== $client_id || $client->getSecret() !== $client_secret) {
+		if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) {
 			return new JSONResponse([
 				'error' => 'invalid_client',
 			], Http::STATUS_BAD_REQUEST);
diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php
index c24308140ec..3dcda337917 100644
--- a/apps/oauth2/lib/Controller/SettingsController.php
+++ b/apps/oauth2/lib/Controller/SettingsController.php
@@ -41,41 +41,25 @@ use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 
 class SettingsController extends Controller {
-	/** @var ClientMapper */
-	private $clientMapper;
-	/** @var ISecureRandom */
-	private $secureRandom;
-	/** @var AccessTokenMapper  */
-	private $accessTokenMapper;
-	/** @var IL10N */
-	private $l;
-	/** @var IAuthTokenProvider */
-	private $tokenProvider;
-	/**
-	 * @var IUserManager
-	 */
-	private $userManager;
+
 	public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
 
-	public function __construct(string $appName,
-								IRequest $request,
-								ClientMapper $clientMapper,
-								ISecureRandom $secureRandom,
-								AccessTokenMapper $accessTokenMapper,
-								IL10N $l,
-								IAuthTokenProvider $tokenProvider,
-								IUserManager $userManager
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private ClientMapper $clientMapper,
+		private ISecureRandom $secureRandom,
+		private AccessTokenMapper $accessTokenMapper,
+		private IL10N $l,
+		private IAuthTokenProvider $tokenProvider,
+		private IUserManager $userManager,
+		private ICrypto $crypto
 	) {
 		parent::__construct($appName, $request);
-		$this->secureRandom = $secureRandom;
-		$this->clientMapper = $clientMapper;
-		$this->accessTokenMapper = $accessTokenMapper;
-		$this->l = $l;
-		$this->tokenProvider = $tokenProvider;
-		$this->userManager = $userManager;
 	}
 
 	public function addClient(string $name,
@@ -87,7 +71,9 @@ class SettingsController extends Controller {
 		$client = new Client();
 		$client->setName($name);
 		$client->setRedirectUri($redirectUri);
-		$client->setSecret($this->secureRandom->generate(64, self::validChars));
+		$secret = $this->secureRandom->generate(64, self::validChars);
+		$encryptedSecret = $this->crypto->encrypt($secret);
+		$client->setSecret($encryptedSecret);
 		$client->setClientIdentifier($this->secureRandom->generate(64, self::validChars));
 		$client = $this->clientMapper->insert($client);
 
@@ -96,7 +82,7 @@ class SettingsController extends Controller {
 			'name' => $client->getName(),
 			'redirectUri' => $client->getRedirectUri(),
 			'clientId' => $client->getClientIdentifier(),
-			'clientSecret' => $client->getSecret(),
+			'clientSecret' => $secret,
 		];
 
 		return new JSONResponse($result);