Skip to content

Commit 4aca516

Browse files
AltahrimAltahrim
authored
Merge pull request #59920 from nextcloud/backport/59767/stable29
2 parents 6ac7f73 + 7967f21 commit 4aca516

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
@@ -40,6 +40,7 @@
4040
use OCP\Authentication\Exceptions\ExpiredTokenException;
4141
use OCP\Authentication\Exceptions\InvalidTokenException;
4242
use OCP\DB\Exception;
43+
use OCP\IDBConnection;
4344
use OCP\IRequest;
4445
use OCP\Security\Bruteforce\IThrottler;
4546
use OCP\Security\ICrypto;
@@ -62,6 +63,7 @@ public function __construct(
6263
private LoggerInterface $logger,
6364
private IThrottler $throttler,
6465
private ITimeFactory $timeFactory,
66+
private IDBConnection $db,
6567
) {
6668
parent::__construct($appName, $request);
6769
}
@@ -193,27 +195,52 @@ public function getToken(
193195

194196
// Rotate the apptoken (so the old one becomes invalid basically)
195197
$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
198+
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
199+
$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
200+
$redeemedThrottleReason = $grant_type === 'authorization_code'
201+
? 'authorization_code_already_redeemed'
202+
: 'refresh_token_already_redeemed';
196203

197-
$appToken = $this->tokenProvider->rotate(
198-
$appToken,
199-
$decryptedToken,
200-
$newToken
201-
);
204+
$this->db->beginTransaction();
205+
try {
206+
$updatedRows = $this->accessTokenMapper->rotateToken(
207+
$accessToken->getId(),
208+
$code,
209+
$newCode,
210+
$newEncryptedToken,
211+
$grant_type === 'authorization_code',
212+
);
213+
214+
if ($updatedRows !== 1) {
215+
$this->db->rollBack();
216+
$response = new JSONResponse([
217+
'error' => 'invalid_request',
218+
], Http::STATUS_BAD_REQUEST);
219+
$response->throttle(['invalid_request' => $redeemedThrottleReason]);
220+
return $response;
221+
}
202222

203-
// Expiration is in 1 hour again
204-
$appToken->setExpires($this->time->getTime() + 3600);
205-
$this->tokenProvider->updateToken($appToken);
223+
$appToken = $this->tokenProvider->rotate(
224+
$appToken,
225+
$decryptedToken,
226+
$newToken
227+
);
206228

207-
// Generate a new refresh token and encrypt the new apptoken in the DB
208-
$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
209-
$accessToken->setHashedCode(hash('sha512', $newCode));
210-
$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
211-
// increase the number of delivered oauth token
212-
// this helps with cleaning up DB access token when authorization code has expired
213-
// and it never delivered any oauth token
214-
$tokenCount = $accessToken->getTokenCount();
215-
$accessToken->setTokenCount($tokenCount + 1);
216-
$this->accessTokenMapper->update($accessToken);
229+
// Expiration is in 1 hour again
230+
$appToken->setExpires($this->time->getTime() + 3600);
231+
$this->tokenProvider->updateToken($appToken);
232+
233+
$this->db->commit();
234+
} catch (\Throwable $e) {
235+
if ($this->db->inTransaction()) {
236+
$this->db->rollBack();
237+
}
238+
// rotate() and updateToken() write the auth token to the cache,
239+
// so if we are past rotate() we must invalidate the new token
240+
$this->tokenProvider->invalidateToken($newToken);
241+
242+
throw $e;
243+
}
217244

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

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,4 +102,31 @@ public function cleanupExpiredAuthorizationCode(): void {
102102
->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
103103
$qb->executeStatement();
104104
}
105+
106+
/**
107+
* Rotate an access token only if it still matches the caller's previously-read state.
108+
*
109+
* @param int $id
110+
* @param string $oldCode
111+
* @param string $newCode
112+
* @param string $encryptedToken
113+
* @param bool $expectAuthorizationCodeState Require the token to still be unused
114+
* @return int Number of updated rows
115+
*/
116+
public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
117+
$qb = $this->db->getQueryBuilder();
118+
$qb
119+
->update($this->tableName)
120+
->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode)))
121+
->set('encrypted_token', $qb->createNamedParameter($encryptedToken))
122+
->set('token_count', $qb->createFunction('token_count + 1'))
123+
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
124+
->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
125+
126+
if ($expectAuthorizationCodeState) {
127+
$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
128+
}
129+
130+
return $qb->executeStatement();
131+
}
105132
}

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

Lines changed: 151 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
use OCP\AppFramework\Http;
4040
use OCP\AppFramework\Http\JSONResponse;
4141
use OCP\AppFramework\Utility\ITimeFactory;
42+
use OCP\IDBConnection;
4243
use OCP\IRequest;
4344
use OCP\Security\Bruteforce\IThrottler;
4445
use OCP\Security\ICrypto;
@@ -72,6 +73,8 @@ class OauthApiControllerTest extends TestCase {
7273
private $logger;
7374
/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
7475
private $timeFactory;
76+
/** @var IDBConnection|\PHPUnit\Framework\MockObject\MockObject */
77+
private $db;
7578
/** @var OauthApiController */
7679
private $oauthApiController;
7780

@@ -88,6 +91,7 @@ protected function setUp(): void {
8891
$this->throttler = $this->createMock(IThrottler::class);
8992
$this->logger = $this->createMock(LoggerInterface::class);
9093
$this->timeFactory = $this->createMock(ITimeFactory::class);
94+
$this->db = $this->createMock(IDBConnection::class);
9195

9296
$this->oauthApiController = new OauthApiController(
9397
'oauth2',
@@ -100,7 +104,8 @@ protected function setUp(): void {
100104
$this->time,
101105
$this->logger,
102106
$this->throttler,
103-
$this->timeFactory
107+
$this->timeFactory,
108+
$this->db,
104109
);
105110
}
106111

@@ -335,6 +340,7 @@ public function testRefreshTokenInvalidAppToken() {
335340

336341
public function testRefreshTokenValidAppToken() {
337342
$accessToken = new AccessToken();
343+
$accessToken->setId(21);
338344
$accessToken->setClientId(42);
339345
$accessToken->setTokenId(1337);
340346
$accessToken->setEncryptedToken('encryptedToken');
@@ -386,6 +392,18 @@ public function testRefreshTokenValidAppToken() {
386392
$this->time->method('getTime')
387393
->willReturn(1000);
388394

395+
$this->db->expects($this->once())
396+
->method('beginTransaction');
397+
398+
$this->db->expects($this->once())
399+
->method('commit');
400+
401+
$this->db->expects($this->never())
402+
->method('rollBack');
403+
404+
$this->tokenProvider->expects($this->never())
405+
->method('invalidateToken');
406+
389407
$this->tokenProvider->expects($this->once())
390408
->method('updateToken')
391409
->with(
@@ -399,13 +417,14 @@ public function testRefreshTokenValidAppToken() {
399417
->willReturn('newEncryptedToken');
400418

401419
$this->accessTokenMapper->expects($this->once())
402-
->method('update')
420+
->method('rotateToken')
403421
->with(
404-
$this->callback(function (AccessToken $token) {
405-
return $token->getHashedCode() === hash('sha512', 'random128') &&
406-
$token->getEncryptedToken() === 'newEncryptedToken';
407-
})
408-
);
422+
21,
423+
'validrefresh',
424+
'random128',
425+
'newEncryptedToken',
426+
false,
427+
)->willReturn(1);
409428

410429
$expected = new JSONResponse([
411430
'access_token' => 'random72',
@@ -431,6 +450,7 @@ public function testRefreshTokenValidAppToken() {
431450

432451
public function testRefreshTokenValidAppTokenBasicAuth() {
433452
$accessToken = new AccessToken();
453+
$accessToken->setId(21);
434454
$accessToken->setClientId(42);
435455
$accessToken->setTokenId(1337);
436456
$accessToken->setEncryptedToken('encryptedToken');
@@ -482,6 +502,18 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
482502
$this->time->method('getTime')
483503
->willReturn(1000);
484504

505+
$this->db->expects($this->once())
506+
->method('beginTransaction');
507+
508+
$this->db->expects($this->once())
509+
->method('commit');
510+
511+
$this->db->expects($this->never())
512+
->method('rollBack');
513+
514+
$this->tokenProvider->expects($this->never())
515+
->method('invalidateToken');
516+
485517
$this->tokenProvider->expects($this->once())
486518
->method('updateToken')
487519
->with(
@@ -495,13 +527,14 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
495527
->willReturn('newEncryptedToken');
496528

497529
$this->accessTokenMapper->expects($this->once())
498-
->method('update')
530+
->method('rotateToken')
499531
->with(
500-
$this->callback(function (AccessToken $token) {
501-
return $token->getHashedCode() === hash('sha512', 'random128') &&
502-
$token->getEncryptedToken() === 'newEncryptedToken';
503-
})
504-
);
532+
21,
533+
'validrefresh',
534+
'random128',
535+
'newEncryptedToken',
536+
false,
537+
)->willReturn(1);
505538

506539
$expected = new JSONResponse([
507540
'access_token' => 'random72',
@@ -530,6 +563,7 @@ public function testRefreshTokenValidAppTokenBasicAuth() {
530563

531564
public function testRefreshTokenExpiredAppToken() {
532565
$accessToken = new AccessToken();
566+
$accessToken->setId(21);
533567
$accessToken->setClientId(42);
534568
$accessToken->setTokenId(1337);
535569
$accessToken->setEncryptedToken('encryptedToken');
@@ -581,6 +615,18 @@ public function testRefreshTokenExpiredAppToken() {
581615
$this->time->method('getTime')
582616
->willReturn(1000);
583617

618+
$this->db->expects($this->once())
619+
->method('beginTransaction');
620+
621+
$this->db->expects($this->once())
622+
->method('commit');
623+
624+
$this->db->expects($this->never())
625+
->method('rollBack');
626+
627+
$this->tokenProvider->expects($this->never())
628+
->method('invalidateToken');
629+
584630
$this->tokenProvider->expects($this->once())
585631
->method('updateToken')
586632
->with(
@@ -594,13 +640,14 @@ public function testRefreshTokenExpiredAppToken() {
594640
->willReturn('newEncryptedToken');
595641

596642
$this->accessTokenMapper->expects($this->once())
597-
->method('update')
643+
->method('rotateToken')
598644
->with(
599-
$this->callback(function (AccessToken $token) {
600-
return $token->getHashedCode() === hash('sha512', 'random128') &&
601-
$token->getEncryptedToken() === 'newEncryptedToken';
602-
})
603-
);
645+
21,
646+
'validrefresh',
647+
'random128',
648+
'newEncryptedToken',
649+
false,
650+
)->willReturn(1);
604651

605652
$expected = new JSONResponse([
606653
'access_token' => 'random72',
@@ -623,4 +670,89 @@ public function testRefreshTokenExpiredAppToken() {
623670

624671
$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
625672
}
673+
674+
public function testRefreshTokenRedeemedConcurrently(): void {
675+
$expected = new JSONResponse([
676+
'error' => 'invalid_request',
677+
], Http::STATUS_BAD_REQUEST);
678+
$expected->throttle(['invalid_request' => 'refresh_token_already_redeemed']);
679+
680+
$accessToken = new AccessToken();
681+
$accessToken->setId(21);
682+
$accessToken->setClientId(42);
683+
$accessToken->setTokenId(1337);
684+
$accessToken->setEncryptedToken('encryptedToken');
685+
686+
$this->accessTokenMapper->method('getByCode')
687+
->with('validrefresh')
688+
->willReturn($accessToken);
689+
690+
$client = new Client();
691+
$client->setClientIdentifier('clientId');
692+
$client->setSecret(bin2hex('hashedClientSecret'));
693+
$this->clientMapper->method('getByUid')
694+
->with(42)
695+
->willReturn($client);
696+
697+
$this->crypto
698+
->method('decrypt')
699+
->with('encryptedToken')
700+
->willReturn('decryptedToken');
701+
702+
$this->crypto
703+
->method('calculateHMAC')
704+
->with('clientSecret')
705+
->willReturn('hashedClientSecret');
706+
707+
$appToken = new PublicKeyToken();
708+
$appToken->setUid('userId');
709+
$this->tokenProvider->method('getTokenById')
710+
->with(1337)
711+
->willReturn($appToken);
712+
713+
$this->secureRandom->method('generate')
714+
->willReturnCallback(function ($len) {
715+
return 'random' . $len;
716+
});
717+
718+
$this->tokenProvider->expects($this->never())
719+
->method('rotate');
720+
721+
$this->time->method('getTime')
722+
->willReturn(1000);
723+
724+
$this->tokenProvider->expects($this->never())
725+
->method('updateToken');
726+
727+
$this->crypto->method('encrypt')
728+
->with('random72', 'random128')
729+
->willReturn('newEncryptedToken');
730+
731+
$this->db->expects($this->once())
732+
->method('beginTransaction');
733+
734+
$this->db->expects($this->never())
735+
->method('commit');
736+
737+
$this->db->expects($this->once())
738+
->method('rollBack');
739+
740+
$this->tokenProvider->expects($this->never())
741+
->method('invalidateToken');
742+
743+
$this->accessTokenMapper->expects($this->once())
744+
->method('rotateToken')
745+
->with(
746+
21,
747+
'validrefresh',
748+
'random128',
749+
'newEncryptedToken',
750+
false,
751+
)->willReturn(0);
752+
753+
$this->throttler->expects($this->never())
754+
->method('resetDelay');
755+
756+
$this->assertEquals($expected, $this->oauthApiController->getToken('refresh_token', null, 'validrefresh', 'clientId', 'clientSecret'));
757+
}
626758
}

0 commit comments

Comments
 (0)