Skip to content

fix(performance): Only search for auth tokens when the provided login… #36621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nickvergessen merged 3 commits into from
Feb 10, 2023
Merged

fix(performance): Only search for auth tokens when the provided login… #36621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03a585a
fix(performance): Only search for auth tokens when the provided login…
nickvergessen Feb 8, 2023
7a85a15
fix(authentication): Check minimum length when creating app tokens
nickvergessen Feb 8, 2023
a81d8ec
Fix unit tests
nickvergessen Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion lib/private/Authentication/Token/PublicKeyTokenProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
use Psr\Log\LoggerInterface;

class PublicKeyTokenProvider implements IProvider {
public const TOKEN_MIN_LENGTH = 22;

use TTransactional;

/** @var PublicKeyTokenMapper */
Expand Down Expand Up @@ -98,6 +100,12 @@ public function generateToken(string $token,
string $name,
int $type = IToken::TEMPORARY_TOKEN,
int $remember = IToken::DO_NOT_REMEMBER): IToken {
if (strlen($token) < self::TOKEN_MIN_LENGTH) {
$exception = new InvalidTokenException('Token is too short, minimum of ' . self::TOKEN_MIN_LENGTH . ' characters is required, ' . strlen($token) . ' characters given');
$this->logger->error('Invalid token provided when generating new token', ['exception' => $exception]);
throw $exception;
}

if (mb_strlen($name) > 128) {
$name = mb_substr($name, 0, 120) . '…';
}
Expand All @@ -112,6 +120,27 @@ public function generateToken(string $token,
}

public function getToken(string $tokenId): IToken {
/**
* Token length: 72
* @see \OC\Core\Controller\ClientFlowLoginController::generateAppPassword
* @see \OC\Core\Controller\AppPasswordController::getAppPassword
* @see \OC\Core\Command\User\AddAppPassword::execute
* @see \OC\Core\Service\LoginFlowV2Service::flowDone
* @see \OCA\Talk\MatterbridgeManager::generatePassword
* @see \OCA\Preferred_Providers\Controller\PasswordController::generateAppPassword
* @see \OCA\GlobalSiteSelector\TokenHandler::generateAppPassword
*
* Token length: 22-256 - https://www.php.net/manual/en/session.configuration.php#ini.session.sid-length
* @see \OC\User\Session::createSessionToken
*
* Token length: 29
* @see \OCA\Settings\Controller\AuthSettingsController::generateRandomDeviceToken
* @see \OCA\Registration\Service\RegistrationService::generateAppPassword
*/
if (strlen($tokenId) < self::TOKEN_MIN_LENGTH) {
throw new InvalidTokenException('Token is too short for a generated token, should be the password during basic auth');
}

$tokenHash = $this->hashToken($tokenId);

if (isset($this->cache[$tokenHash])) {
Expand All @@ -122,7 +151,7 @@ public function getToken(string $tokenId): IToken {
$token = $this->cache[$tokenHash];
} else {
try {
$token = $this->mapper->getToken($this->hashToken($tokenId));
$token = $this->mapper->getToken($tokenHash);
$this->cache[$token->getToken()] = $token;
} catch (DoesNotExistException $ex) {
try {
Expand Down
89 changes: 42 additions & 47 deletions tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ protected function setUp(): void {
}

public function testGenerateToken() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand All @@ -115,7 +115,7 @@ public function testGenerateToken() {
}

public function testGenerateTokenNoPassword(): void {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand All @@ -138,7 +138,7 @@ public function testGenerateTokenNoPassword(): void {
}

public function testGenerateTokenLongPassword() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = '';
Expand All @@ -157,7 +157,7 @@ public function testGenerateTokenLongPassword() {
}

public function testGenerateTokenInvalidName() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand Down Expand Up @@ -222,7 +222,7 @@ public function testGetTokenByUser() {
}

public function testGetPassword() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand Down Expand Up @@ -253,7 +253,7 @@ public function testGetPasswordPasswordLessToken() {
public function testGetPasswordInvalidToken() {
$this->expectException(\OC\Authentication\Exceptions\InvalidTokenException::class);

$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand All @@ -270,7 +270,7 @@ public function testGetPasswordInvalidToken() {
}

public function testSetPassword() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand All @@ -291,13 +291,13 @@ public function testSetPassword() {
$this->mapper->expects($this->once())
->method('update')
->with($this->callback(function ($token) use ($newpass) {
return $newpass === $this->tokenProvider->getPassword($token, 'token');
return $newpass === $this->tokenProvider->getPassword($token, 'tokentokentokentokentoken');
}));


$this->tokenProvider->setPassword($actual, $token, $newpass);

$this->assertSame($newpass, $this->tokenProvider->getPassword($actual, 'token'));
$this->assertSame($newpass, $this->tokenProvider->getPassword($actual, 'tokentokentokentokentoken'));
}


Expand All @@ -312,12 +312,12 @@ public function testSetPasswordInvalidToken() {
}

public function testInvalidateToken() {
$this->mapper->expects($this->at(0))
->method('invalidate')
->with(hash('sha512', 'token7'.'1f4h9s'));
$this->mapper->expects($this->at(1))
$this->mapper->expects($this->exactly(2))
->method('invalidate')
->with(hash('sha512', 'token7'));
->withConsecutive(
[hash('sha512', 'token7'.'1f4h9s')],
[hash('sha512', 'token7')]
);

$this->tokenProvider->invalidateToken('token7');
}
Expand Down Expand Up @@ -352,7 +352,7 @@ public function testInvalidateOldTokens() {
}

public function testRenewSessionTokenWithoutPassword() {
$token = 'oldId';
$token = 'oldIdtokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = null;
Expand All @@ -364,7 +364,7 @@ public function testRenewSessionTokenWithoutPassword() {
$this->mapper
->expects($this->once())
->method('getToken')
->with(hash('sha512', 'oldId' . '1f4h9s'))
->with(hash('sha512', 'oldIdtokentokentokentoken' . '1f4h9s'))
->willReturn($oldToken);
$this->mapper
->expects($this->once())
Expand All @@ -384,11 +384,11 @@ public function testRenewSessionTokenWithoutPassword() {
return $token === $oldToken;
}));

$this->tokenProvider->renewSessionToken('oldId', 'newId');
$this->tokenProvider->renewSessionToken('oldIdtokentokentokentoken', 'newIdtokentokentokentoken');
}

public function testRenewSessionTokenWithPassword(): void {
$token = 'oldId';
$token = 'oldIdtokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'password';
Expand All @@ -404,7 +404,7 @@ public function testRenewSessionTokenWithPassword(): void {
$this->mapper
->expects($this->once())
->method('getToken')
->with(hash('sha512', 'oldId' . '1f4h9s'))
->with(hash('sha512', 'oldIdtokentokentokentoken' . '1f4h9s'))
->willReturn($oldToken);
$this->mapper
->expects($this->once())
Expand All @@ -416,7 +416,7 @@ public function testRenewSessionTokenWithPassword(): void {
$token->getType() === IToken::DO_NOT_REMEMBER &&
$token->getLastActivity() === $this->time &&
$token->getPassword() !== null &&
$this->tokenProvider->getPassword($token, 'newId') === 'password';
$this->tokenProvider->getPassword($token, 'newIdtokentokentokentoken') === 'password';
}));
$this->mapper
->expects($this->once())
Expand All @@ -425,7 +425,7 @@ public function testRenewSessionTokenWithPassword(): void {
return $token === $oldToken;
}));

$this->tokenProvider->renewSessionToken('oldId', 'newId');
$this->tokenProvider->renewSessionToken('oldIdtokentokentokentoken', 'newIdtokentokentokentoken');
}

public function testGetToken(): void {
Expand All @@ -438,37 +438,32 @@ public function testGetToken(): void {
$this->mapper->method('getToken')
->with(
$this->callback(function (string $token) {
return hash('sha512', 'unhashedToken'.'1f4h9s') === $token;
return hash('sha512', 'unhashedTokentokentokentokentoken'.'1f4h9s') === $token;
})
)->willReturn($token);

$this->assertSame($token, $this->tokenProvider->getToken('unhashedToken'));
$this->assertSame($token, $this->tokenProvider->getToken('unhashedTokentokentokentokentoken'));
}

public function testGetInvalidToken() {
$this->expectException(InvalidTokenException::class);

$this->mapper->expects($this->at(0))
->method('getToken')
->with(
$this->callback(function (string $token): bool {
return hash('sha512', 'unhashedToken'.'1f4h9s') === $token;
})
)->willThrowException(new DoesNotExistException('nope'));

$this->mapper->expects($this->at(1))
$this->mapper->expects($this->exactly(2))
->method('getToken')
->with(
$this->callback(function (string $token): bool {
return hash('sha512', 'unhashedToken') === $token;
})
->withConsecutive(
[$this->callback(function (string $token): bool {
return hash('sha512', 'unhashedTokentokentokentokentoken'.'1f4h9s') === $token;
})],
[$this->callback(function (string $token): bool {
return hash('sha512', 'unhashedTokentokentokentokentoken') === $token;
})]
)->willThrowException(new DoesNotExistException('nope'));

$this->tokenProvider->getToken('unhashedToken');
$this->tokenProvider->getToken('unhashedTokentokentokentokentoken');
}

public function testGetExpiredToken() {
$token = 'token';
$token = 'tokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'passme';
Expand All @@ -481,12 +476,12 @@ public function testGetExpiredToken() {
$this->mapper->method('getToken')
->with(
$this->callback(function (string $token) {
return hash('sha512', 'token'.'1f4h9s') === $token;
return hash('sha512', 'tokentokentokentokentoken'.'1f4h9s') === $token;
})
)->willReturn($actual);

try {
$this->tokenProvider->getToken('token');
$this->tokenProvider->getToken('tokentokentokentokentoken');
$this->fail();
} catch (ExpiredTokenException $e) {
$this->assertSame($actual, $e->getToken());
Expand Down Expand Up @@ -533,7 +528,7 @@ public function testGetExpiredTokenById() {
}

public function testRotate() {
$token = 'oldtoken';
$token = 'oldtokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = 'password';
Expand All @@ -546,13 +541,13 @@ public function testRotate() {
]);
$actual = $this->tokenProvider->generateToken($token, $uid, $user, $password, $name, $type, IToken::DO_NOT_REMEMBER);

$new = $this->tokenProvider->rotate($actual, 'oldtoken', 'newtoken');
$new = $this->tokenProvider->rotate($actual, 'oldtokentokentokentokentoken', 'newtokentokentokentokentoken');

$this->assertSame('password', $this->tokenProvider->getPassword($new, 'newtoken'));
$this->assertSame('password', $this->tokenProvider->getPassword($new, 'newtokentokentokentokentoken'));
}

public function testRotateNoPassword() {
$token = 'oldtoken';
$token = 'oldtokentokentokentokentoken';
$uid = 'user';
$user = 'User';
$password = null;
Expand All @@ -563,7 +558,7 @@ public function testRotateNoPassword() {

$oldPrivate = $actual->getPrivateKey();

$new = $this->tokenProvider->rotate($actual, 'oldtoken', 'newtoken');
$new = $this->tokenProvider->rotate($actual, 'oldtokentokentokentokentoken', 'newtokentokentokentokentoken');

$newPrivate = $new->getPrivateKey();

Expand Down Expand Up @@ -595,15 +590,15 @@ public function testMarkPasswordInvalid() {
public function testUpdatePasswords() {
$uid = 'myUID';
$token1 = $this->tokenProvider->generateToken(
'foo',
'foobetokentokentokentoken',
$uid,
$uid,
'bar',
'random1',
IToken::PERMANENT_TOKEN,
IToken::REMEMBER);
$token2 = $this->tokenProvider->generateToken(
'foobar',
'foobartokentokentokentoken',
$uid,
$uid,
'bar',
Expand Down