Skip to content

Commit 1015650

Browse files
julien-ncjulien-nc
committed
fix(oauth): wrap token rotation in a transaction, only rotate if the token hasn't been modified since we have read it
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent 8c73ce0 commit 1015650

3 files changed

Lines changed: 219 additions & 22 deletions

File tree

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

Lines changed: 48 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,55 @@ 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+
$tokenRotated = false;
180185

181-
$appToken = $this->tokenProvider->rotate(
182-
$appToken,
183-
$decryptedToken,
184-
$newToken
185-
);
186+
$this->db->beginTransaction();
187+
try {
188+
$appToken = $this->tokenProvider->rotate(
189+
$appToken,
190+
$decryptedToken,
191+
$newToken
192+
);
193+
$tokenRotated = true;
194+
195+
// Expiration is in 1 hour again
196+
$appToken->setExpires($this->time->getTime() + 3600);
197+
$this->tokenProvider->updateToken($appToken);
198+
199+
$updatedRows = $this->accessTokenMapper->rotateToken(
200+
$accessToken->getId(),
201+
$code,
202+
$newCode,
203+
$newEncryptedToken,
204+
$grant_type === 'authorization_code',
205+
);
206+
207+
if ($updatedRows !== 1) {
208+
$this->db->rollBack();
209+
// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
210+
$this->tokenProvider->invalidateToken($newToken);
211+
$response = new JSONResponse([
212+
'error' => 'invalid_request',
213+
], Http::STATUS_BAD_REQUEST);
214+
$response->throttle(['invalid_request' => 'token already redeemed']);
215+
return $response;
216+
}
186217

187-
// Expiration is in 1 hour again
188-
$appToken->setExpires($this->time->getTime() + 3600);
189-
$this->tokenProvider->updateToken($appToken);
218+
$this->db->commit();
219+
} catch (\Throwable $e) {
220+
if ($this->db->inTransaction()) {
221+
$this->db->rollBack();
222+
}
190223

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);
224+
if ($tokenRotated) {
225+
// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
226+
$this->tokenProvider->invalidateToken($newToken);
227+
}
228+
229+
throw $e;
230+
}
201231

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

‎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: 144 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\AppFramework\Http;
2020
use OCP\AppFramework\Http\JSONResponse;
2121
use OCP\AppFramework\Utility\ITimeFactory;
22+
use OCP\IDBConnection;
2223
use OCP\IRequest;
2324
use OCP\Security\Bruteforce\IThrottler;
2425
use OCP\Security\ICrypto;
@@ -52,6 +53,8 @@ class OauthApiControllerTest extends TestCase {
5253
private $logger;
5354
/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
5455
private $timeFactory;
56+
/** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */
57+
private $db;
5558
/** @var OauthApiController */
5659
private $oauthApiController;
5760

@@ -68,6 +71,7 @@ protected function setUp(): void {
6871
$this->throttler = $this->createMock(IThrottler::class);
6972
$this->logger = $this->createMock(LoggerInterface::class);
7073
$this->timeFactory = $this->createMock(ITimeFactory::class);
74+
$this->db = $this->createMock(IDBConnection::class);
7175

7276
$this->oauthApiController = new OauthApiController(
7377
'oauth2',
@@ -80,7 +84,8 @@ protected function setUp(): void {
8084
$this->time,
8185
$this->logger,
8286
$this->throttler,
83-
$this->timeFactory
87+
$this->timeFactory,
88+
$this->db,
8489
);
8590
}
8691

@@ -315,6 +320,7 @@ public function testRefreshTokenInvalidAppToken(): void {
315320

316321
public function testRefreshTokenValidAppToken(): void {
317322
$accessToken = new AccessToken();
323+
$accessToken->setId(21);
318324
$accessToken->setClientId(42);
319325
$accessToken->setTokenId(1337);
320326
$accessToken->setEncryptedToken('encryptedToken');
@@ -366,6 +372,18 @@ public function testRefreshTokenValidAppToken(): void {
366372
$this->time->method('getTime')
367373
->willReturn(1000);
368374

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

381399
$this->accessTokenMapper->expects($this->once())
382-
->method('update')
400+
->method('rotateToken')
383401
->with(
384402
$this->callback(function (AccessToken $token) {
385403
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -411,6 +429,7 @@ public function testRefreshTokenValidAppToken(): void {
411429

412430
public function testRefreshTokenValidAppTokenBasicAuth(): void {
413431
$accessToken = new AccessToken();
432+
$accessToken->setId(21);
414433
$accessToken->setClientId(42);
415434
$accessToken->setTokenId(1337);
416435
$accessToken->setEncryptedToken('encryptedToken');
@@ -462,6 +481,18 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
462481
$this->time->method('getTime')
463482
->willReturn(1000);
464483

484+
$this->db->expects($this->once())
485+
->method('beginTransaction');
486+
487+
$this->db->expects($this->once())
488+
->method('commit');
489+
490+
$this->db->expects($this->never())
491+
->method('rollBack');
492+
493+
$this->tokenProvider->expects($this->never())
494+
->method('invalidateToken');
495+
465496
$this->tokenProvider->expects($this->once())
466497
->method('updateToken')
467498
->with(
@@ -475,7 +506,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
475506
->willReturn('newEncryptedToken');
476507

477508
$this->accessTokenMapper->expects($this->once())
478-
->method('update')
509+
->method('rotateToken')
479510
->with(
480511
$this->callback(function (AccessToken $token) {
481512
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -510,6 +541,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
510541

511542
public function testRefreshTokenExpiredAppToken(): void {
512543
$accessToken = new AccessToken();
544+
$accessToken->setId(21);
513545
$accessToken->setClientId(42);
514546
$accessToken->setTokenId(1337);
515547
$accessToken->setEncryptedToken('encryptedToken');
@@ -561,6 +593,18 @@ public function testRefreshTokenExpiredAppToken(): void {
561593
$this->time->method('getTime')
562594
->willReturn(1000);
563595

596+
$this->db->expects($this->once())
597+
->method('beginTransaction');
598+
599+
$this->db->expects($this->once())
600+
->method('commit');
601+
602+
$this->db->expects($this->never())
603+
->method('rollBack');
604+
605+
$this->tokenProvider->expects($this->never())
606+
->method('invalidateToken');
607+
564608
$this->tokenProvider->expects($this->once())
565609
->method('updateToken')
566610
->with(
@@ -574,7 +618,7 @@ public function testRefreshTokenExpiredAppToken(): void {
574618
->willReturn('newEncryptedToken');
575619

576620
$this->accessTokenMapper->expects($this->once())
577-
->method('update')
621+
->method('rotateToken')
578622
->with(
579623
$this->callback(function (AccessToken $token) {
580624
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -603,4 +647,100 @@ public function testRefreshTokenExpiredAppToken(): void {
603647

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

0 commit comments

Comments
 (0)