Skip to content

Commit c7b5380

Browse files
sorbaughsorbaugh
authored
Merge pull request #59805 from nextcloud/backport/59804/stable23
[stable23] fix: Reduce the mixups between apptokens and session ids
2 parents a51c509 + 133c438 commit c7b5380

2 files changed

Lines changed: 61 additions & 21 deletions

File tree

‎lib/private/User/Session.php‎

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
use OC\Authentication\Exceptions\PasswordLoginForbiddenException;
4646
use OC\Authentication\Token\IProvider;
4747
use OC\Authentication\Token\IToken;
48+
use OC\Authentication\Token\PublicKeyToken;
4849
use OC\Hooks\Emitter;
4950
use OC\Hooks\PublicEmitter;
5051
use OC_User;
@@ -445,7 +446,14 @@ public function logClientIn($user,
445446
}
446447

447448
try {
448-
$isTokenPassword = $this->isTokenPassword($password);
449+
$dbToken = $this->getTokenFromPassword($password);
450+
$isTokenPassword = $dbToken !== null;
451+
if (($dbToken instanceof PublicKeyToken)
452+
&& ($dbToken->getType() !== IToken::PERMANENT_TOKEN)
453+
) {
454+
// Refuse session tokens here, only app tokens are handled
455+
return false;
456+
}
449457
} catch (ExpiredTokenException $e) {
450458
// Just return on an expired token no need to check further or record a failed login
451459
return false;
@@ -548,6 +556,24 @@ public function isTokenPassword($password) {
548556
}
549557
}
550558

559+
/**
560+
* Check if the given 'password' is actually a device token
561+
*
562+
* @throws ExpiredTokenException
563+
*/
564+
private function getTokenFromPassword(string $password): ?IToken {
565+
try {
566+
return $this->tokenProvider->getToken($password);
567+
} catch (ExpiredTokenException $e) {
568+
throw $e;
569+
} catch (InvalidTokenException $ex) {
570+
$this->logger->debug('Token is not valid: ' . $ex->getMessage(), [
571+
'exception' => $ex,
572+
]);
573+
return null;
574+
}
575+
}
576+
551577
protected function prepareUserLogin($firstTimeLogin, $refreshCsrfToken = true) {
552578
if ($refreshCsrfToken) {
553579
// TODO: mock/inject/use non-static
@@ -829,29 +855,36 @@ private function validateToken($token, $user = null) {
829855
*/
830856
public function tryTokenLogin(IRequest $request) {
831857
$authHeader = $request->getHeader('Authorization');
858+
$tokenFromCookie = false;
832859
if (strpos($authHeader, 'Bearer ') === 0) {
833860
$token = substr($authHeader, 7);
834861
} else {
835862
// No auth header, let's try session id
836863
try {
837864
$token = $this->session->getId();
865+
$tokenFromCookie = true;
838866
} catch (SessionNotAvailableException $ex) {
839867
return false;
840868
}
841869
}
842870

843-
if (!$this->loginWithToken($token)) {
871+
try {
872+
$dbToken = $this->tokenProvider->getToken($token);
873+
} catch (InvalidTokenException $e) {
874+
// Can't really happen but better safe than sorry
844875
return false;
845876
}
846-
if (!$this->validateToken($token)) {
877+
878+
if ($dbToken instanceof PublicKeyToken && $dbToken->getType() === IToken::TEMPORARY_TOKEN && !$tokenFromCookie) {
879+
// Session token but from Bearer header, not allowed
847880
return false;
848881
}
849882

850-
try {
851-
$dbToken = $this->tokenProvider->getToken($token);
852-
} catch (InvalidTokenException $e) {
853-
// Can't really happen but better save than sorry
854-
return true;
883+
if (!$this->loginWithToken($token)) {
884+
return false;
885+
}
886+
if (!$this->validateToken($token)) {
887+
return false;
855888
}
856889

857890
// Remember me tokens are not app_passwords

‎tests/lib/User/SessionTest.php‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<?php
2+
23
/**
34
* Copyright (c) 2013 Robin Appelman <icewind@owncloud.com>
45
* This file is licensed under the Affero General Public License version 3 or
@@ -10,6 +11,7 @@
1011

1112
use OC\AppFramework\Http\Request;
1213
use OC\Authentication\Events\LoginFailed;
14+
use OC\Authentication\Exceptions\InvalidTokenException;
1315
use OC\Authentication\Token\DefaultTokenMapper;
1416
use OC\Authentication\Token\DefaultTokenProvider;
1517
use OC\Authentication\Token\IProvider;
@@ -489,16 +491,18 @@ public function testLogClientInWithTokenPassword() {
489491
$manager = $this->createMock(Manager::class);
490492
$session = $this->createMock(ISession::class);
491493
$request = $this->createMock(IRequest::class);
494+
$token = $this->createMock(IToken::class);
492495

493496
/** @var \OC\User\Session $userSession */
494497
$userSession = $this->getMockBuilder(Session::class)
495498
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
496-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
499+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
497500
->getMock();
498501

499-
$userSession->expects($this->once())
500-
->method('isTokenPassword')
501-
->willReturn(true);
502+
$this->tokenProvider->expects($this->once())
503+
->method('getToken')
504+
->with('I-AM-AN-APP-PASSWORD')
505+
->willReturn($token);
502506
$userSession->expects($this->once())
503507
->method('login')
504508
->with('john', 'I-AM-AN-APP-PASSWORD')
@@ -1032,7 +1036,7 @@ public function testTryTokenLoginWithDisabledUser() {
10321036
->method('getHeader')
10331037
->with('Authorization')
10341038
->willReturn('Bearer xxxxx');
1035-
$this->tokenProvider->expects($this->once())
1039+
$this->tokenProvider->expects($this->atLeastOnce())
10361040
->method('getToken')
10371041
->with('xxxxx')
10381042
->willReturn($token);
@@ -1478,16 +1482,18 @@ public function testLogClientInThrottlerUsername() {
14781482
$manager = $this->createMock(Manager::class);
14791483
$session = $this->createMock(ISession::class);
14801484
$request = $this->createMock(IRequest::class);
1485+
$token = $this->createMock(IToken::class);
14811486

14821487
/** @var Session $userSession */
14831488
$userSession = $this->getMockBuilder(Session::class)
14841489
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
1485-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
1490+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
14861491
->getMock();
14871492

1488-
$userSession->expects($this->once())
1489-
->method('isTokenPassword')
1490-
->willReturn(true);
1493+
$this->tokenProvider->expects($this->once())
1494+
->method('getToken')
1495+
->with('I-AM-AN-PASSWORD')
1496+
->willReturn($token);
14911497
$userSession->expects($this->once())
14921498
->method('login')
14931499
->with('john', 'I-AM-AN-PASSWORD')
@@ -1528,12 +1534,13 @@ public function testLogClientInThrottlerEmail() {
15281534
/** @var Session $userSession */
15291535
$userSession = $this->getMockBuilder(Session::class)
15301536
->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher])
1531-
->setMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser'])
1537+
->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser'])
15321538
->getMock();
15331539

1534-
$userSession->expects($this->once())
1535-
->method('isTokenPassword')
1536-
->willReturn(true);
1540+
$this->tokenProvider->expects($this->once())
1541+
->method('getToken')
1542+
->with('I-AM-AN-PASSWORD')
1543+
->willThrowException(new InvalidTokenException());
15371544
$userSession->expects($this->once())
15381545
->method('login')
15391546
->with('john@foo.bar', 'I-AM-AN-PASSWORD')

0 commit comments

Comments
 (0)