Skip to content

Commit 1eebeee

Browse files
come-nccome-nc
committed
fix: Reduce the mixups between apptokens and session ids
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
1 parent 5457f4d commit 1eebeee

2 files changed

Lines changed: 58 additions & 22 deletions

File tree

‎lib/private/User/Session.php‎

Lines changed: 41 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -388,8 +388,15 @@ public function logClientIn($user,
388388
}
389389

390390
try {
391-
$isTokenPassword = $this->isTokenPassword($password);
392-
} catch (ExpiredTokenException $e) {
391+
$dbToken = $this->getTokenFromPassword($password);
392+
$isTokenPassword = $dbToken !== null;
393+
if (($dbToken instanceof PublicKeyToken)
394+
&& ($dbToken->getType() !== IToken::PERMANENT_TOKEN)
395+
) {
396+
// Refuse session tokens here, only app tokens are handled
397+
return false;
398+
}
399+
} catch (ExpiredTokenException) {
393400
// Just return on an expired token no need to check further or record a failed login
394401
return false;
395402
}
@@ -410,7 +417,6 @@ public function logClientIn($user,
410417
}
411418

412419
if ($isTokenPassword) {
413-
$dbToken = $this->tokenProvider->getToken($password);
414420
$userFromToken = $this->manager->get($dbToken->getUID());
415421
$isValidEmailLogin = $userFromToken->getEMailAddress() === $user
416422
&& $this->validateTokenLoginName($userFromToken->getEMailAddress(), $dbToken);
@@ -500,6 +506,24 @@ public function isTokenPassword($password) {
500506
}
501507
}
502508

509+
/**
510+
* Check if the given 'password' is actually a device token
511+
*
512+
* @throws ExpiredTokenException
513+
*/
514+
private function getTokenFromPassword(string $password): ?\OCP\Authentication\Token\IToken {
515+
try {
516+
return $this->tokenProvider->getToken($password);
517+
} catch (ExpiredTokenException $e) {
518+
throw $e;
519+
} catch (InvalidTokenException $ex) {
520+
$this->logger->debug('Token is not valid: ' . $ex->getMessage(), [
521+
'exception' => $ex,
522+
]);
523+
return null;
524+
}
525+
}
526+
503527
protected function prepareUserLogin($firstTimeLogin, $refreshCsrfToken = true) {
504528
if ($refreshCsrfToken) {
505529
// TODO: mock/inject/use non-static
@@ -806,32 +830,39 @@ private function validateTokenLoginName(?string $loginName, IToken $token): bool
806830
*/
807831
public function tryTokenLogin(IRequest $request) {
808832
$authHeader = $request->getHeader('Authorization');
833+
$tokenFromCookie = false;
809834
if (str_starts_with($authHeader, 'Bearer ')) {
810835
$token = substr($authHeader, 7);
811836
} elseif ($request->getCookie($this->config->getSystemValueString('instanceid')) !== null) {
812837
// No auth header, let's try session id, but only if this is an existing
813838
// session and the request has a session cookie
814839
try {
815840
$token = $this->session->getId();
841+
$tokenFromCookie = true;
816842
} catch (SessionNotAvailableException $ex) {
817843
return false;
818844
}
819845
} else {
820846
return false;
821847
}
822848

823-
if (!$this->loginWithToken($token)) {
849+
try {
850+
$dbToken = $this->tokenProvider->getToken($token);
851+
} catch (InvalidTokenException $e) {
852+
// Can't really happen but better safe than sorry
824853
return false;
825854
}
826-
if (!$this->validateToken($token)) {
855+
856+
if ($dbToken instanceof PublicKeyToken && $dbToken->getType() === IToken::TEMPORARY_TOKEN && !$tokenFromCookie) {
857+
// Session token but from Bearer header, not allowed
827858
return false;
828859
}
829860

830-
try {
831-
$dbToken = $this->tokenProvider->getToken($token);
832-
} catch (InvalidTokenException $e) {
833-
// Can't really happen but better save than sorry
834-
return true;
861+
if (!$this->loginWithToken($token)) {
862+
return false;
863+
}
864+
if (!$this->validateToken($token)) {
865+
return false;
835866
}
836867

837868
// Set the session variable so we know this is an app password

‎tests/lib/User/SessionTest.php‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -482,16 +482,18 @@ public function testLogClientInWithTokenPassword(): void {
482482
$manager = $this->createMock(Manager::class);
483483
$session = $this->createMock(ISession::class);
484484
$request = $this->createMock(IRequest::class);
485+
$token = $this->createMock(IToken::class);
485486

486487
/** @var Session $userSession */
487488
$userSession = $this->getMockBuilder(Session::class)
488489
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
489-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
490+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
490491
->getMock();
491492

492-
$userSession->expects($this->once())
493-
->method('isTokenPassword')
494-
->willReturn(true);
493+
$this->tokenProvider->expects($this->once())
494+
->method('getToken')
495+
->with('I-AM-AN-APP-PASSWORD')
496+
->willReturn($token);
495497
$userSession->expects($this->once())
496498
->method('login')
497499
->with('john', 'I-AM-AN-APP-PASSWORD')
@@ -1231,16 +1233,18 @@ public function testLogClientInThrottlerUsername(): void {
12311233
$manager = $this->createMock(Manager::class);
12321234
$session = $this->createMock(ISession::class);
12331235
$request = $this->createMock(IRequest::class);
1236+
$token = $this->createMock(IToken::class);
12341237

12351238
/** @var Session $userSession */
12361239
$userSession = $this->getMockBuilder(Session::class)
12371240
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
1238-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
1241+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
12391242
->getMock();
12401243

1241-
$userSession->expects($this->once())
1242-
->method('isTokenPassword')
1243-
->willReturn(true);
1244+
$this->tokenProvider->expects($this->once())
1245+
->method('getToken')
1246+
->with('I-AM-AN-PASSWORD')
1247+
->willReturn($token);
12441248
$userSession->expects($this->once())
12451249
->method('login')
12461250
->with('john', 'I-AM-AN-PASSWORD')
@@ -1281,12 +1285,13 @@ public function testLogClientInThrottlerEmail(): void {
12811285
/** @var Session $userSession */
12821286
$userSession = $this->getMockBuilder(Session::class)
12831287
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
1284-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
1288+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
12851289
->getMock();
12861290

1287-
$userSession->expects($this->once())
1288-
->method('isTokenPassword')
1289-
->willReturn(false);
1291+
$this->tokenProvider->expects($this->once())
1292+
->method('getToken')
1293+
->with('I-AM-AN-PASSWORD')
1294+
->willThrowException(new InvalidTokenException());
12901295
$userSession->expects($this->once())
12911296
->method('login')
12921297
->with('john@foo.bar', 'I-AM-AN-PASSWORD')

0 commit comments

Comments
 (0)