Skip to content

Commit 368c803

Browse files
AltahrimAltahrim
authored
Merge pull request #59919 from nextcloud/backport/59767/stable30
2 parents ab914ec + 8a18757 commit 368c803

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
@@ -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,52 @@ 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+
$redeemedThrottleReason = $grant_type === 'authorization_code'
183+
? 'authorization_code_already_redeemed'
184+
: 'refresh_token_already_redeemed';
178185

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

185-
// Expiration is in 1 hour again
186-
$appToken->setExpires($this->time->getTime() + 3600);
187-
$this->tokenProvider->updateToken($appToken);
205+
$appToken = $this->tokenProvider->rotate(
206+
$appToken,
207+
$decryptedToken,
208+
$newToken
209+
);
188210

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);
211+
// Expiration is in 1 hour again
212+
$appToken->setExpires($this->time->getTime() + 3600);
213+
$this->tokenProvider->updateToken($appToken);
214+
215+
$this->db->commit();
216+
} catch (\Throwable $e) {
217+
if ($this->db->inTransaction()) {
218+
$this->db->rollBack();
219+
}
220+
// rotate() and updateToken() write the auth token to the cache,
221+
// so if we are past rotate() we must invalidate the new token
222+
$this->tokenProvider->invalidateToken($newToken);
223+
224+
throw $e;
225+
}
199226

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

‎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() {
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,13 +398,14 @@ public function testRefreshTokenValidAppToken() {
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() {
412431

413432
public function testRefreshTokenValidAppTokenBasicAuth() {
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() {
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() {
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() {
511544

512545
public function testRefreshTokenExpiredAppToken() {
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() {
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() {
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() {
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)