Skip to content

Commit 2f7dc70

Browse files
joshtrichardsjoshtrichards
authored
Merge pull request #59879 from nextcloud/backport/59767/stable33
[stable33] Wrap oauth2 token rotation in a transaction
2 parents 2d7d728 + f84da68 commit 2f7dc70

3 files changed

Lines changed: 223 additions & 37 deletions

File tree

‎apps/oauth2/lib/Controller/OauthApiController.php‎

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
use OCP\Authentication\Exceptions\ExpiredTokenException;
2525
use OCP\Authentication\Exceptions\InvalidTokenException;
2626
use OCP\DB\Exception;
27+
use OCP\IDBConnection;
2728
use OCP\IRequest;
2829
use OCP\Security\Bruteforce\IThrottler;
2930
use OCP\Security\ICrypto;
@@ -47,6 +48,7 @@ public function __construct(
4748
private LoggerInterface $logger,
4849
private IThrottler $throttler,
4950
private ITimeFactory $timeFactory,
51+
private IDBConnection $db,
5052
) {
5153
parent::__construct($appName, $request);
5254
}
@@ -177,27 +179,52 @@ public function getToken(
177179

178180
// Rotate the apptoken (so the old one becomes invalid basically)
179181
$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
182+
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
183+
$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
184+
$redeemedThrottleReason = $grant_type === 'authorization_code'
185+
? 'authorization_code_already_redeemed'
186+
: 'refresh_token_already_redeemed';
180187

181-
$appToken = $this->tokenProvider->rotate(
182-
$appToken,
183-
$decryptedToken,
184-
$newToken
185-
);
188+
$this->db->beginTransaction();
189+
try {
190+
$updatedRows = $this->accessTokenMapper->rotateToken(
191+
$accessToken->getId(),
192+
$code,
193+
$newCode,
194+
$newEncryptedToken,
195+
$grant_type === 'authorization_code',
196+
);
197+
198+
if ($updatedRows !== 1) {
199+
$this->db->rollBack();
200+
$response = new JSONResponse([
201+
'error' => 'invalid_request',
202+
], Http::STATUS_BAD_REQUEST);
203+
$response->throttle(['invalid_request' => $redeemedThrottleReason]);
204+
return $response;
205+
}
186206

187-
// Expiration is in 1 hour again
188-
$appToken->setExpires($this->time->getTime() + 3600);
189-
$this->tokenProvider->updateToken($appToken);
207+
$appToken = $this->tokenProvider->rotate(
208+
$appToken,
209+
$decryptedToken,
210+
$newToken
211+
);
190212

191-
// Generate a new refresh token and encrypt the new apptoken in the DB
192-
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
193-
$accessToken->setHashedCode(hash('sha512', $newCode));
194-
$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
195-
// increase the number of delivered oauth token
196-
// this helps with cleaning up DB access token when authorization code has expired
197-
// and it never delivered any oauth token
198-
$tokenCount = $accessToken->getTokenCount();
199-
$accessToken->setTokenCount($tokenCount + 1);
200-
$this->accessTokenMapper->update($accessToken);
213+
// Expiration is in 1 hour again
214+
$appToken->setExpires($this->time->getTime() + 3600);
215+
$this->tokenProvider->updateToken($appToken);
216+
217+
$this->db->commit();
218+
} catch (\Throwable $e) {
219+
if ($this->db->inTransaction()) {
220+
$this->db->rollBack();
221+
}
222+
// rotate() and updateToken() write the auth token to the cache,
223+
// so if we are past rotate() we must invalidate the new token
224+
$this->tokenProvider->invalidateToken($newToken);
225+
226+
throw $e;
227+
}
201228

202229
$this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]);
203230

‎apps/oauth2/lib/Db/AccessTokenMapper.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,31 @@ public function cleanupExpiredAuthorizationCode(): void {
8282
->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
8383
$qb->executeStatement();
8484
}
85+
86+
/**
87+
* Rotate an access token only if it still matches the caller's previously-read state.
88+
*
89+
* @param int $id
90+
* @param string $oldCode
91+
* @param string $newCode
92+
* @param string $encryptedToken
93+
* @param bool $expectAuthorizationCodeState Require the token to still be unused
94+
* @return int Number of updated rows
95+
*/
96+
public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
97+
$qb = $this->db->getQueryBuilder();
98+
$qb
99+
->update($this->tableName)
100+
->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode)))
101+
->set('encrypted_token', $qb->createNamedParameter($encryptedToken))
102+
->set('token_count', $qb->createFunction('token_count + 1'))
103+
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
104+
->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
105+
106+
if ($expectAuthorizationCodeState) {
107+
$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
108+
}
109+
110+
return $qb->executeStatement();
111+
}
85112
}

‎apps/oauth2/tests/Controller/OauthApiControllerTest.php‎

Lines changed: 151 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCP\AppFramework\Http;
2121
use OCP\AppFramework\Http\JSONResponse;
2222
use OCP\AppFramework\Utility\ITimeFactory;
23+
use OCP\IDBConnection;
2324
use OCP\IRequest;
2425
use OCP\Security\Bruteforce\IThrottler;
2526
use OCP\Security\ICrypto;
@@ -53,6 +54,8 @@ class OauthApiControllerTest extends TestCase {
5354
private $logger;
5455
/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
5556
private $timeFactory;
57+
/** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */
58+
private $db;
5659
/** @var OauthApiController */
5760
private $oauthApiController;
5861

@@ -69,6 +72,7 @@ protected function setUp(): void {
6972
$this->throttler = $this->createMock(IThrottler::class);
7073
$this->logger = $this->createMock(LoggerInterface::class);
7174
$this->timeFactory = $this->createMock(ITimeFactory::class);
75+
$this->db = $this->createMock(IDBConnection::class);
7276

7377
$this->oauthApiController = new OauthApiController(
7478
'oauth2',
@@ -81,7 +85,8 @@ protected function setUp(): void {
8185
$this->time,
8286
$this->logger,
8387
$this->throttler,
84-
$this->timeFactory
88+
$this->timeFactory,
89+
$this->db,
8590
);
8691
}
8792

@@ -316,6 +321,7 @@ public function testRefreshTokenInvalidAppToken(): void {
316321

317322
public function testRefreshTokenValidAppToken(): void {
318323
$accessToken = new AccessToken();
324+
$accessToken->setId(21);
319325
$accessToken->setClientId(42);
320326
$accessToken->setTokenId(1337);
321327
$accessToken->setEncryptedToken('encryptedToken');
@@ -367,6 +373,18 @@ public function testRefreshTokenValidAppToken(): void {
367373
$this->time->method('getTime')
368374
->willReturn(1000);
369375

376+
$this->db->expects($this->once())
377+
->method('beginTransaction');
378+
379+
$this->db->expects($this->once())
380+
->method('commit');
381+
382+
$this->db->expects($this->never())
383+
->method('rollBack');
384+
385+
$this->tokenProvider->expects($this->never())
386+
->method('invalidateToken');
387+
370388
$this->tokenProvider->expects($this->once())
371389
->method('updateToken')
372390
->with(
@@ -380,13 +398,14 @@ public function testRefreshTokenValidAppToken(): void {
380398
->willReturn('newEncryptedToken');
381399

382400
$this->accessTokenMapper->expects($this->once())
383-
->method('update')
401+
->method('rotateToken')
384402
->with(
385-
$this->callback(function (AccessToken $token) {
386-
return $token->getHashedCode() === hash('sha512', 'random128')
387-
&& $token->getEncryptedToken() === 'newEncryptedToken';
388-
})
389-
);
403+
21,
404+
'validrefresh',
405+
'random128',
406+
'newEncryptedToken',
407+
false,
408+
)->willReturn(1);
390409

391410
$expected = new JSONResponse([
392411
'access_token' => 'random72',
@@ -412,6 +431,7 @@ public function testRefreshTokenValidAppToken(): void {
412431

413432
public function testRefreshTokenValidAppTokenBasicAuth(): void {
414433
$accessToken = new AccessToken();
434+
$accessToken->setId(21);
415435
$accessToken->setClientId(42);
416436
$accessToken->setTokenId(1337);
417437
$accessToken->setEncryptedToken('encryptedToken');
@@ -463,6 +483,18 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
463483
$this->time->method('getTime')
464484
->willReturn(1000);
465485

486+
$this->db->expects($this->once())
487+
->method('beginTransaction');
488+
489+
$this->db->expects($this->once())
490+
->method('commit');
491+
492+
$this->db->expects($this->never())
493+
->method('rollBack');
494+
495+
$this->tokenProvider->expects($this->never())
496+
->method('invalidateToken');
497+
466498
$this->tokenProvider->expects($this->once())
467499
->method('updateToken')
468500
->with(
@@ -476,13 +508,14 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
476508
->willReturn('newEncryptedToken');
477509

478510
$this->accessTokenMapper->expects($this->once())
479-
->method('update')
511+
->method('rotateToken')
480512
->with(
481-
$this->callback(function (AccessToken $token) {
482-
return $token->getHashedCode() === hash('sha512', 'random128')
483-
&& $token->getEncryptedToken() === 'newEncryptedToken';
484-
})
485-
);
513+
21,
514+
'validrefresh',
515+
'random128',
516+
'newEncryptedToken',
517+
false,
518+
)->willReturn(1);
486519

487520
$expected = new JSONResponse([
488521
'access_token' => 'random72',
@@ -511,6 +544,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
511544

512545
public function testRefreshTokenExpiredAppToken(): void {
513546
$accessToken = new AccessToken();
547+
$accessToken->setId(21);
514548
$accessToken->setClientId(42);
515549
$accessToken->setTokenId(1337);
516550
$accessToken->setEncryptedToken('encryptedToken');
@@ -562,6 +596,18 @@ public function testRefreshTokenExpiredAppToken(): void {
562596
$this->time->method('getTime')
563597
->willReturn(1000);
564598

599+
$this->db->expects($this->once())
600+
->method('beginTransaction');
601+
602+
$this->db->expects($this->once())
603+
->method('commit');
604+
605+
$this->db->expects($this->never())
606+
->method('rollBack');
607+
608+
$this->tokenProvider->expects($this->never())
609+
->method('invalidateToken');
610+
565611
$this->tokenProvider->expects($this->once())
566612
->method('updateToken')
567613
->with(
@@ -575,13 +621,14 @@ public function testRefreshTokenExpiredAppToken(): void {
575621
->willReturn('newEncryptedToken');
576622

577623
$this->accessTokenMapper->expects($this->once())
578-
->method('update')
624+
->method('rotateToken')
579625
->with(
580-
$this->callback(function (AccessToken $token) {
581-
return $token->getHashedCode() === hash('sha512', 'random128')
582-
&& $token->getEncryptedToken() === 'newEncryptedToken';
583-
})
584-
);
626+
21,
627+
'validrefresh',
628+
'random128',
629+
'newEncryptedToken',
630+
false,
631+
)->willReturn(1);
585632

586633
$expected = new JSONResponse([
587634
'access_token' => 'random72',
@@ -604,4 +651,89 @@ public function testRefreshTokenExpiredAppToken(): void {
604651

605652
$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
606653
}
654+
655+
public function testRefreshTokenRedeemedConcurrently(): void {
656+
$expected = new JSONResponse([
657+
'error' => 'invalid_request',
658+
], Http::STATUS_BAD_REQUEST);
659+
$expected->throttle(['invalid_request' => 'refresh_token_already_redeemed']);
660+
661+
$accessToken = new AccessToken();
662+
$accessToken->setId(21);
663+
$accessToken->setClientId(42);
664+
$accessToken->setTokenId(1337);
665+
$accessToken->setEncryptedToken('encryptedToken');
666+
667+
$this->accessTokenMapper->method('getByCode')
668+
->with('validrefresh')
669+
->willReturn($accessToken);
670+
671+
$client = new Client();
672+
$client->setClientIdentifier('clientId');
673+
$client->setSecret(bin2hex('hashedClientSecret'));
674+
$this->clientMapper->method('getByUid')
675+
->with(42)
676+
->willReturn($client);
677+
678+
$this->crypto
679+
->method('decrypt')
680+
->with('encryptedToken')
681+
->willReturn('decryptedToken');
682+
683+
$this->crypto
684+
->method('calculateHMAC')
685+
->with('clientSecret')
686+
->willReturn('hashedClientSecret');
687+
688+
$appToken = new PublicKeyToken();
689+
$appToken->setUid('userId');
690+
$this->tokenProvider->method('getTokenById')
691+
->with(1337)
692+
->willReturn($appToken);
693+
694+
$this->secureRandom->method('generate')
695+
->willReturnCallback(function ($len) {
696+
return 'random' . $len;
697+
});
698+
699+
$this->tokenProvider->expects($this->never())
700+
->method('rotate');
701+
702+
$this->time->method('getTime')
703+
->willReturn(1000);
704+
705+
$this->tokenProvider->expects($this->never())
706+
->method('updateToken');
707+
708+
$this->crypto->method('encrypt')
709+
->with('random72', 'random128')
710+
->willReturn('newEncryptedToken');
711+
712+
$this->db->expects($this->once())
713+
->method('beginTransaction');
714+
715+
$this->db->expects($this->never())
716+
->method('commit');
717+
718+
$this->db->expects($this->once())
719+
->method('rollBack');
720+
721+
$this->tokenProvider->expects($this->never())
722+
->method('invalidateToken');
723+
724+
$this->accessTokenMapper->expects($this->once())
725+
->method('rotateToken')
726+
->with(
727+
21,
728+
'validrefresh',
729+
'random128',
730+
'newEncryptedToken',
731+
false,
732+
)->willReturn(0);
733+
734+
$this->throttler->expects($this->never())
735+
->method('resetDelay');
736+
737+
$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
738+
}
607739
}

0 commit comments

Comments
 (0)
⚡