diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml index 716e9ecd362..6cd8f4cae8d 100644 --- a/apps/oauth2/appinfo/info.xml +++ b/apps/oauth2/appinfo/info.xml @@ -5,7 +5,7 @@ OAuth 2.0 Allows OAuth2 compatible authentication from other web applications. The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications. - 1.15.2 + 1.15.3 agpl Lukas Reschke OAuth2 diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index b2f24b59c77..6714e3e081a 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -22,5 +22,6 @@ return array( 'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => $baseDir . '/../lib/Migration/Version010402Date20190107124745.php', 'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => $baseDir . '/../lib/Migration/Version011601Date20230522143227.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => $baseDir . '/../lib/Migration/Version011603Date20230620111039.php', + 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => $baseDir . '/../lib/Migration/Version011901Date20240829164356.php', 'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php index ad4983efbb9..d1ddd62f2b7 100644 --- a/apps/oauth2/composer/composer/autoload_static.php +++ b/apps/oauth2/composer/composer/autoload_static.php @@ -37,6 +37,7 @@ class ComposerStaticInitOAuth2 'OCA\\OAuth2\\Migration\\Version010402Date20190107124745' => __DIR__ . '/..' . '/../lib/Migration/Version010402Date20190107124745.php', 'OCA\\OAuth2\\Migration\\Version011601Date20230522143227' => __DIR__ . '/..' . '/../lib/Migration/Version011601Date20230522143227.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => __DIR__ . '/..' . '/../lib/Migration/Version011603Date20230620111039.php', + 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => __DIR__ . '/..' . '/../lib/Migration/Version011901Date20240829164356.php', 'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index dea8bd5c83d..fec19f8f741 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -149,7 +149,8 @@ class OauthApiController extends Controller { } try { - $storedClientSecret = $this->crypto->decrypt($client->getSecret()); + $storedClientSecretHash = $client->getSecret(); + $clientSecretHash = bin2hex($this->crypto->calculateHMAC($client_secret)); } catch (\Exception $e) { $this->logger->error('OAuth client secret decryption error', ['exception' => $e]); // we don't throttle here because it might not be a bruteforce attack @@ -158,7 +159,7 @@ class OauthApiController extends Controller { ], Http::STATUS_BAD_REQUEST); } // The client id and secret must match. Else we don't provide an access token! - if ($client->getClientIdentifier() !== $client_id || $storedClientSecret !== $client_secret) { + if ($client->getClientIdentifier() !== $client_id || $storedClientSecretHash !== $clientSecretHash) { $response = 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 3dcda337917..c1c397ae121 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -72,8 +72,8 @@ class SettingsController extends Controller { $client->setName($name); $client->setRedirectUri($redirectUri); $secret = $this->secureRandom->generate(64, self::validChars); - $encryptedSecret = $this->crypto->encrypt($secret); - $client->setSecret($encryptedSecret); + $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + $client->setSecret($hashedSecret); $client->setClientIdentifier($this->secureRandom->generate(64, self::validChars)); $client = $this->clientMapper->insert($client); diff --git a/apps/oauth2/lib/Migration/Version011901Date20240829164356.php b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php new file mode 100644 index 00000000000..9ce9ff371cb --- /dev/null +++ b/apps/oauth2/lib/Migration/Version011901Date20240829164356.php @@ -0,0 +1,49 @@ +connection->getQueryBuilder(); + $qbUpdate->update('oauth2_clients') + ->set('secret', $qbUpdate->createParameter('updateSecret')) + ->where( + $qbUpdate->expr()->eq('id', $qbUpdate->createParameter('updateId')) + ); + + $qbSelect = $this->connection->getQueryBuilder(); + $qbSelect->select('id', 'secret') + ->from('oauth2_clients'); + $req = $qbSelect->executeQuery(); + while ($row = $req->fetch()) { + $id = $row['id']; + $storedEncryptedSecret = $row['secret']; + $secret = $this->crypto->decrypt($storedEncryptedSecret); + $hashedSecret = bin2hex($this->crypto->calculateHMAC($secret)); + $qbUpdate->setParameter('updateSecret', $hashedSecret, IQueryBuilder::PARAM_STR); + $qbUpdate->setParameter('updateId', $id, IQueryBuilder::PARAM_INT); + $qbUpdate->executeStatement(); + } + $req->closeCursor(); + } +} diff --git a/apps/oauth2/lib/Settings/Admin.php b/apps/oauth2/lib/Settings/Admin.php index 7b297116a77..e142d131d65 100644 --- a/apps/oauth2/lib/Settings/Admin.php +++ b/apps/oauth2/lib/Settings/Admin.php @@ -29,9 +29,9 @@ namespace OCA\OAuth2\Settings; use OCA\OAuth2\Db\ClientMapper; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; +use OCP\IURLGenerator; use OCP\Security\ICrypto; use OCP\Settings\ISettings; -use OCP\IURLGenerator; use Psr\Log\LoggerInterface; class Admin implements ISettings { @@ -40,7 +40,6 @@ class Admin implements ISettings { private IInitialState $initialState, private ClientMapper $clientMapper, private IURLGenerator $urlGenerator, - private ICrypto $crypto, private LoggerInterface $logger, ) { } @@ -51,13 +50,12 @@ class Admin implements ISettings { foreach ($clients as $client) { try { - $secret = $this->crypto->decrypt($client->getSecret()); $result[] = [ 'id' => $client->getId(), 'name' => $client->getName(), 'redirectUri' => $client->getRedirectUri(), 'clientId' => $client->getClientIdentifier(), - 'clientSecret' => $secret, + 'clientSecret' => '', ]; } catch (\Exception $e) { $this->logger->error('[Settings] OAuth client secret decryption error', ['exception' => $e]); diff --git a/apps/oauth2/src/App.vue b/apps/oauth2/src/App.vue index fc154204c8d..498e9b3637b 100644 --- a/apps/oauth2/src/App.vue +++ b/apps/oauth2/src/App.vue @@ -39,6 +39,10 @@ @delete="deleteClient" /> + + {{ t('oauth2', 'Make sure you store the secret key, it cannot be recovered.') }} +

{{ t('oauth2', 'Add client') }}

@@ -68,6 +72,7 @@ import { generateUrl } from '@nextcloud/router' import { getCapabilities } from '@nextcloud/capabilities' import NcSettingsSection from '@nextcloud/vue/dist/Components/NcSettingsSection.js' import NcButton from '@nextcloud/vue/dist/Components/NcButton.js' +import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js' import { loadState } from '@nextcloud/initial-state' export default { @@ -76,6 +81,7 @@ export default { OAuthItem, NcSettingsSection, NcButton, + NcNoteCard, }, props: { clients: { @@ -92,6 +98,7 @@ export default { error: false, }, oauthDocLink: loadState('oauth2', 'oauth2-doc-link'), + showSecretWarning: false, } }, computed: { @@ -119,6 +126,7 @@ export default { ).then(response => { // eslint-disable-next-line vue/no-mutating-props this.clients.push(response.data) + this.showSecretWarning = true this.newClient.name = '' this.newClient.redirectUri = '' diff --git a/apps/oauth2/src/components/OAuthItem.vue b/apps/oauth2/src/components/OAuthItem.vue index a759af56906..aaf2f2ecd92 100644 --- a/apps/oauth2/src/components/OAuthItem.vue +++ b/apps/oauth2/src/components/OAuthItem.vue @@ -37,7 +37,7 @@ {{ t('oauth2', 'Secret') }} - {{ renderedSecret }} + {{ renderedSecret }} diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 6faef7c88f3..f3ffbfe7991 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -265,9 +265,20 @@ class OauthApiControllerTest extends TestCase { ->with('validrefresh') ->willReturn($accessToken); + $this->crypto + ->method('calculateHMAC') + ->with($this->callback(function (string $text) { + return $text === 'clientSecret' || $text === 'invalidClientSecret'; + })) + ->willReturnCallback(function (string $text) { + return $text === 'clientSecret' + ? 'hashedClientSecret' + : 'hashedInvalidClientSecret'; + }); + $client = new Client(); $client->setClientIdentifier('clientId'); - $client->setSecret('clientSecret'); + $client->setSecret('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); @@ -292,21 +303,20 @@ class OauthApiControllerTest extends TestCase { $client = new Client(); $client->setClientIdentifier('clientId'); - $client->setSecret('encryptedClientSecret'); + $client->setSecret('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); $this->crypto ->method('decrypt') - ->with($this->callback(function (string $text) { - return $text === 'encryptedClientSecret' || $text === 'encryptedToken'; - })) - ->willReturnCallback(function (string $text) { - return $text === 'encryptedClientSecret' - ? 'clientSecret' - : ($text === 'encryptedToken' ? 'decryptedToken' : ''); - }); + ->with('encryptedToken') + ->willReturn('decryptedToken'); + + $this->crypto + ->method('calculateHMAC') + ->with('clientSecret') + ->willReturn('hashedClientSecret'); $this->tokenProvider->method('getTokenById') ->with(1337) @@ -331,21 +341,20 @@ class OauthApiControllerTest extends TestCase { $client = new Client(); $client->setClientIdentifier('clientId'); - $client->setSecret('encryptedClientSecret'); + $client->setSecret('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); $this->crypto ->method('decrypt') - ->with($this->callback(function (string $text) { - return $text === 'encryptedClientSecret' || $text === 'encryptedToken'; - })) - ->willReturnCallback(function (string $text) { - return $text === 'encryptedClientSecret' - ? 'clientSecret' - : ($text === 'encryptedToken' ? 'decryptedToken' : ''); - }); + ->with('encryptedToken') + ->willReturn('decryptedToken'); + + $this->crypto + ->method('calculateHMAC') + ->with('clientSecret') + ->willReturn('hashedClientSecret'); $appToken = new PublicKeyToken(); $appToken->setUid('userId'); @@ -428,21 +437,20 @@ class OauthApiControllerTest extends TestCase { $client = new Client(); $client->setClientIdentifier('clientId'); - $client->setSecret('encryptedClientSecret'); + $client->setSecret('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); $this->crypto ->method('decrypt') - ->with($this->callback(function (string $text) { - return $text === 'encryptedClientSecret' || $text === 'encryptedToken'; - })) - ->willReturnCallback(function (string $text) { - return $text === 'encryptedClientSecret' - ? 'clientSecret' - : ($text === 'encryptedToken' ? 'decryptedToken' : ''); - }); + ->with('encryptedToken') + ->willReturn('decryptedToken'); + + $this->crypto + ->method('calculateHMAC') + ->with('clientSecret') + ->willReturn('hashedClientSecret'); $appToken = new PublicKeyToken(); $appToken->setUid('userId'); @@ -528,21 +536,20 @@ class OauthApiControllerTest extends TestCase { $client = new Client(); $client->setClientIdentifier('clientId'); - $client->setSecret('encryptedClientSecret'); + $client->setSecret('hashedClientSecret'); $this->clientMapper->method('getByUid') ->with(42) ->willReturn($client); $this->crypto ->method('decrypt') - ->with($this->callback(function (string $text) { - return $text === 'encryptedClientSecret' || $text === 'encryptedToken'; - })) - ->willReturnCallback(function (string $text) { - return $text === 'encryptedClientSecret' - ? 'clientSecret' - : ($text === 'encryptedToken' ? 'decryptedToken' : ''); - }); + ->with('encryptedToken') + ->willReturn('decryptedToken'); + + $this->crypto + ->method('calculateHMAC') + ->with('clientSecret') + ->willReturn('hashedClientSecret'); $appToken = new PublicKeyToken(); $appToken->setUid('userId'); diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index 817747599b7..191ee8de53b 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -103,13 +103,13 @@ class SettingsControllerTest extends TestCase { $this->crypto ->expects($this->once()) - ->method('encrypt') - ->willReturn('MyEncryptedSecret'); + ->method('calculateHMAC') + ->willReturn('MyHashedSecret'); $client = new Client(); $client->setName('My Client Name'); $client->setRedirectUri('https://example.com/'); - $client->setSecret('MySecret'); + $client->setSecret('MyHashedSecret'); $client->setClientIdentifier('MyClientIdentifier'); $this->clientMapper @@ -118,7 +118,7 @@ class SettingsControllerTest extends TestCase { ->with($this->callback(function (Client $c) { return $c->getName() === 'My Client Name' && $c->getRedirectUri() === 'https://example.com/' && - $c->getSecret() === 'MyEncryptedSecret' && + $c->getSecret() === 'MyHashedSecret' && $c->getClientIdentifier() === 'MyClientIdentifier'; }))->willReturnCallback(function (Client $c) { $c->setId(42); @@ -161,7 +161,7 @@ class SettingsControllerTest extends TestCase { $client->setId(123); $client->setName('My Client Name'); $client->setRedirectUri('https://example.com/'); - $client->setSecret('MySecret'); + $client->setSecret('MyHashedSecret'); $client->setClientIdentifier('MyClientIdentifier'); $this->clientMapper diff --git a/apps/oauth2/tests/Settings/AdminTest.php b/apps/oauth2/tests/Settings/AdminTest.php index fb19a9fc6d1..55f8920b202 100644 --- a/apps/oauth2/tests/Settings/AdminTest.php +++ b/apps/oauth2/tests/Settings/AdminTest.php @@ -28,7 +28,6 @@ use OCA\OAuth2\Settings\Admin; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; use OCP\IURLGenerator; -use OCP\Security\ICrypto; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -38,7 +37,7 @@ class AdminTest extends TestCase { /** @var Admin|MockObject */ private $admin; - /** @var IInitialStateService|MockObject */ + /** @var IInitialState|MockObject */ private $initialState; /** @var ClientMapper|MockObject */ @@ -54,7 +53,6 @@ class AdminTest extends TestCase { $this->initialState, $this->clientMapper, $this->createMock(IURLGenerator::class), - $this->createMock(ICrypto::class), $this->createMock(LoggerInterface::class) ); }