Skip to content

Commit d5666bf

Browse files
julien-ncbackportbot[bot]
julien-nc
authored and
backportbot[bot]
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 9f4fca5 commit d5666bf

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
@@ -23,6 +23,7 @@
2323
use OCP\Authentication\Exceptions\ExpiredTokenException;
2424
use OCP\Authentication\Exceptions\InvalidTokenException;
2525
use OCP\DB\Exception;
26+
use OCP\IDBConnection;
2627
use OCP\IRequest;
2728
use OCP\Security\Bruteforce\IThrottler;
2829
use OCP\Security\ICrypto;
@@ -45,6 +46,7 @@ public function __construct(
4546
private LoggerInterface $logger,
4647
private IThrottler $throttler,
4748
private ITimeFactory $timeFactory,
49+
private IDBConnection $db,
4850
) {
4951
parent::__construct($appName, $request);
5052
}
@@ -175,27 +177,55 @@ public function getToken(
175177

176178
// Rotate the apptoken (so the old one becomes invalid basically)
177179
$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
180+
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
181+
$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
182+
$tokenRotated = false;
178183

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

185-
// Expiration is in 1 hour again
186-
$appToken->setExpires($this->time->getTime() + 3600);
187-
$this->tokenProvider->updateToken($appToken);
216+
$this->db->commit();
217+
} catch (\Throwable $e) {
218+
if ($this->db->inTransaction()) {
219+
$this->db->rollBack();
220+
}
188221

189-
// Generate a new refresh token and encrypt the new apptoken in the DB
190-
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
191-
$accessToken->setHashedCode(hash('sha512', $newCode));
192-
$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
193-
// increase the number of delivered oauth token
194-
// this helps with cleaning up DB access token when authorization code has expired
195-
// and it never delivered any oauth token
196-
$tokenCount = $accessToken->getTokenCount();
197-
$accessToken->setTokenCount($tokenCount + 1);
198-
$this->accessTokenMapper->update($accessToken);
222+
if ($tokenRotated) {
223+
// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
224+
$this->tokenProvider->invalidateToken($newToken);
225+
}
226+
227+
throw $e;
228+
}
199229

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

‎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
@@ -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() {
316321

317322
public function testRefreshTokenValidAppToken() {
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() {
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,7 +398,7 @@ public function testRefreshTokenValidAppToken() {
380398
->willReturn('newEncryptedToken');
381399

382400
$this->accessTokenMapper->expects($this->once())
383-
->method('update')
401+
->method('rotateToken')
384402
->with(
385403
$this->callback(function (AccessToken $token) {
386404
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -412,6 +430,7 @@ public function testRefreshTokenValidAppToken() {
412430

413431
public function testRefreshTokenValidAppTokenBasicAuth() {
414432
$accessToken = new AccessToken();
433+
$accessToken->setId(21);
415434
$accessToken->setClientId(42);
416435
$accessToken->setTokenId(1337);
417436
$accessToken->setEncryptedToken('encryptedToken');
@@ -463,6 +482,18 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
463482
$this->time->method('getTime')
464483
->willReturn(1000);
465484

485+
$this->db->expects($this->once())
486+
->method('beginTransaction');
487+
488+
$this->db->expects($this->once())
489+
->method('commit');
490+
491+
$this->db->expects($this->never())
492+
->method('rollBack');
493+
494+
$this->tokenProvider->expects($this->never())
495+
->method('invalidateToken');
496+
466497
$this->tokenProvider->expects($this->once())
467498
->method('updateToken')
468499
->with(
@@ -476,7 +507,7 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
476507
->willReturn('newEncryptedToken');
477508

478509
$this->accessTokenMapper->expects($this->once())
479-
->method('update')
510+
->method('rotateToken')
480511
->with(
481512
$this->callback(function (AccessToken $token) {
482513
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -511,6 +542,7 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
511542

512543
public function testRefreshTokenExpiredAppToken() {
513544
$accessToken = new AccessToken();
545+
$accessToken->setId(21);
514546
$accessToken->setClientId(42);
515547
$accessToken->setTokenId(1337);
516548
$accessToken->setEncryptedToken('encryptedToken');
@@ -562,6 +594,18 @@ public function testRefreshTokenExpiredAppToken() {
562594
$this->time->method('getTime')
563595
->willReturn(1000);
564596

597+
$this->db->expects($this->once())
598+
->method('beginTransaction');
599+
600+
$this->db->expects($this->once())
601+
->method('commit');
602+
603+
$this->db->expects($this->never())
604+
->method('rollBack');
605+
606+
$this->tokenProvider->expects($this->never())
607+
->method('invalidateToken');
608+
565609
$this->tokenProvider->expects($this->once())
566610
->method('updateToken')
567611
->with(
@@ -575,7 +619,7 @@ public function testRefreshTokenExpiredAppToken() {
575619
->willReturn('newEncryptedToken');
576620

577621
$this->accessTokenMapper->expects($this->once())
578-
->method('update')
622+
->method('rotateToken')
579623
->with(
580624
$this->callback(function (AccessToken $token) {
581625
return $token->getHashedCode() === hash('sha512', 'random128') &&
@@ -604,4 +648,100 @@ public function testRefreshTokenExpiredAppToken() {
604648

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

0 commit comments

Comments
 (0)