From 47388e1cfe049265050614f55744adcd77ee8052 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 15 May 2018 10:24:46 +0200 Subject: [PATCH 1/4] Make the Token Auth code strict In preparation for #9441 Signed-off-by: Roeland Jago Douma --- .../Authentication/Token/DefaultToken.php | 81 ++++++++----------- .../Token/DefaultTokenMapper.php | 18 ++--- .../Token/DefaultTokenProvider.php | 35 ++++---- .../Authentication/Token/IProvider.php | 27 ++++--- lib/private/Authentication/Token/IToken.php | 23 +++--- 5 files changed, 94 insertions(+), 90 deletions(-) diff --git a/lib/private/Authentication/Token/DefaultToken.php b/lib/private/Authentication/Token/DefaultToken.php index e06803d0bfcf4..8ddd9b00cf2cf 100644 --- a/lib/private/Authentication/Token/DefaultToken.php +++ b/lib/private/Authentication/Token/DefaultToken.php @@ -1,4 +1,5 @@ addType('uid', 'string'); + $this->addType('loginName', 'string'); + $this->addType('password', 'string'); + $this->addType('name', 'string'); + $this->addType('token', 'string'); $this->addType('type', 'int'); + $this->addType('remember', 'int'); $this->addType('lastActivity', 'int'); $this->addType('lastCheck', 'int'); + $this->addType('scope', 'string'); } - public function getId() { + public function getId(): int { return $this->id; } - public function getUID() { + public function getUID(): string { return $this->uid; } @@ -112,7 +100,7 @@ public function getUID() { * * @return string */ - public function getLoginName() { + public function getLoginName(): string { return parent::getLoginName(); } @@ -121,7 +109,7 @@ public function getLoginName() { * * @return string */ - public function getPassword() { + public function getPassword(): string { return parent::getPassword(); } @@ -140,7 +128,7 @@ public function jsonSerialize() { * * @return int */ - public function getLastCheck() { + public function getLastCheck(): int { return parent::getLastCheck(); } @@ -148,16 +136,17 @@ public function getLastCheck() { * Get the timestamp of the last password check * * @param int $time + * @return int */ - public function setLastCheck($time) { + public function setLastCheck(int $time): int { return parent::setLastCheck($time); } - public function getScope() { + public function getScope(): string { return parent::getScope(); } - public function getScopeAsArray() { + public function getScopeAsArray(): array { $scope = json_decode($this->getScope(), true); if (!$scope) { return [ @@ -167,11 +156,7 @@ public function getScopeAsArray() { return $scope; } - public function setScope($scope) { - if (is_array($scope)) { - parent::setScope(json_encode($scope)); - } else { - parent::setScope((string)$scope); - } + public function setScope(array $scope) { + parent::setScope(json_encode($scope)); } } diff --git a/lib/private/Authentication/Token/DefaultTokenMapper.php b/lib/private/Authentication/Token/DefaultTokenMapper.php index 55494d7237057..285b043c2c19c 100644 --- a/lib/private/Authentication/Token/DefaultTokenMapper.php +++ b/lib/private/Authentication/Token/DefaultTokenMapper.php @@ -1,4 +1,5 @@ db->getQueryBuilder(); $qb->delete('authtoken') @@ -59,7 +59,7 @@ public function invalidate($token) { * @param int $olderThan * @param int $remember */ - public function invalidateOld($olderThan, $remember = IToken::DO_NOT_REMEMBER) { + public function invalidateOld(int $olderThan, int $remember = IToken::DO_NOT_REMEMBER) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->delete('authtoken') @@ -76,7 +76,7 @@ public function invalidateOld($olderThan, $remember = IToken::DO_NOT_REMEMBER) { * @throws DoesNotExistException * @return DefaultToken */ - public function getToken($token) { + public function getToken(string $token): DefaultToken { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'remember', 'token', 'last_activity', 'last_check', 'scope') @@ -95,11 +95,11 @@ public function getToken($token) { /** * Get the token for $id * - * @param string $id + * @param int $id * @throws DoesNotExistException * @return DefaultToken */ - public function getTokenById($id) { + public function getTokenById(int $id): DefaultToken { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check', 'scope') @@ -124,7 +124,7 @@ public function getTokenById($id) { * @param IUser $user * @return DefaultToken[] */ - public function getTokenByUser(IUser $user) { + public function getTokenByUser(IUser $user): array { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'remember', 'token', 'last_activity', 'last_check', 'scope') @@ -146,7 +146,7 @@ public function getTokenByUser(IUser $user) { * @param IUser $user * @param int $id */ - public function deleteById(IUser $user, $id) { + public function deleteById(IUser $user, int $id) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->delete('authtoken') @@ -160,7 +160,7 @@ public function deleteById(IUser $user, $id) { * * @param string $name */ - public function deleteByName($name) { + public function deleteByName(string $name) { $qb = $this->db->getQueryBuilder(); $qb->delete('authtoken') ->where($qb->expr()->eq('name', $qb->createNamedParameter($name), IQueryBuilder::PARAM_STR)); diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php index 36a8b1d5464fa..b1d3d227aef2c 100644 --- a/lib/private/Authentication/Token/DefaultTokenProvider.php +++ b/lib/private/Authentication/Token/DefaultTokenProvider.php @@ -1,4 +1,5 @@ @@ -85,7 +86,13 @@ public function __construct(DefaultTokenMapper $mapper, * @param int $remember whether the session token should be used for remember-me * @return IToken */ - public function generateToken($token, $uid, $loginName, $password, $name, $type = IToken::TEMPORARY_TOKEN, $remember = IToken::DO_NOT_REMEMBER) { + public function generateToken(string $token, + string $uid, + string $loginName, + $password, + string $name, + int $type = IToken::TEMPORARY_TOKEN, + int $remember = IToken::DO_NOT_REMEMBER): IToken { $dbToken = new DefaultToken(); $dbToken->setUid($uid); $dbToken->setLoginName($loginName); @@ -145,7 +152,7 @@ public function updateTokenActivity(IToken $token) { * @param IUser $user * @return IToken[] */ - public function getTokenByUser(IUser $user) { + public function getTokenByUser(IUser $user): array { return $this->mapper->getTokenByUser($user); } @@ -154,9 +161,9 @@ public function getTokenByUser(IUser $user) { * * @param string $tokenId * @throws InvalidTokenException - * @return DefaultToken + * @return IToken */ - public function getToken($tokenId) { + public function getToken(string $tokenId): IToken { try { return $this->mapper->getToken($this->hashToken($tokenId)); } catch (DoesNotExistException $ex) { @@ -169,9 +176,9 @@ public function getToken($tokenId) { * * @param string $tokenId * @throws InvalidTokenException - * @return DefaultToken + * @return IToken */ - public function getTokenById($tokenId) { + public function getTokenById(string $tokenId): IToken { try { return $this->mapper->getTokenById($tokenId); } catch (DoesNotExistException $ex) { @@ -184,7 +191,7 @@ public function getTokenById($tokenId) { * @param string $sessionId * @throws InvalidTokenException */ - public function renewSessionToken($oldSessionId, $sessionId) { + public function renewSessionToken(string $oldSessionId, string $sessionId) { $token = $this->getToken($oldSessionId); $newToken = new DefaultToken(); @@ -210,7 +217,7 @@ public function renewSessionToken($oldSessionId, $sessionId) { * @throws PasswordlessTokenException * @return string */ - public function getPassword(IToken $savedToken, $tokenId) { + public function getPassword(IToken $savedToken, string $tokenId): string { $password = $savedToken->getPassword(); if (is_null($password)) { throw new PasswordlessTokenException(); @@ -226,7 +233,7 @@ public function getPassword(IToken $savedToken, $tokenId) { * @param string $password * @throws InvalidTokenException */ - public function setPassword(IToken $token, $tokenId, $password) { + public function setPassword(IToken $token, string $tokenId, string $password) { if (!($token instanceof DefaultToken)) { throw new InvalidTokenException(); } @@ -240,7 +247,7 @@ public function setPassword(IToken $token, $tokenId, $password) { * * @param string $token */ - public function invalidateToken($token) { + public function invalidateToken(string $token) { $this->mapper->invalidate($this->hashToken($token)); } @@ -250,7 +257,7 @@ public function invalidateToken($token) { * @param IUser $user * @param int $id */ - public function invalidateTokenById(IUser $user, $id) { + public function invalidateTokenById(IUser $user, int $id) { $this->mapper->deleteById($user, $id); } @@ -270,7 +277,7 @@ public function invalidateOldTokens() { * @param string $token * @return string */ - private function hashToken($token) { + private function hashToken(string $token) { $secret = $this->config->getSystemValue('secret'); return hash('sha512', $token . $secret); } @@ -284,7 +291,7 @@ private function hashToken($token) { * @param string $token * @return string encrypted password */ - private function encryptPassword($password, $token) { + private function encryptPassword(string $password, string $token): string { $secret = $this->config->getSystemValue('secret'); return $this->crypto->encrypt($password, $token . $secret); } @@ -299,7 +306,7 @@ private function encryptPassword($password, $token) { * @throws InvalidTokenException * @return string the decrypted key */ - private function decryptPassword($password, $token) { + private function decryptPassword(string $password, string $token): string { $secret = $this->config->getSystemValue('secret'); try { return $this->crypto->decrypt($password, $token . $secret); diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index e1cc8182ff095..1928fd3213663 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -1,4 +1,5 @@ Date: Tue, 15 May 2018 10:56:40 +0200 Subject: [PATCH 2/4] Fix tests Signed-off-by: Roeland Jago Douma --- .../Authentication/Token/DefaultToken.php | 32 +++++++++++++------ .../Token/DefaultTokenProvider.php | 4 +-- .../Authentication/Token/IProvider.php | 4 +-- lib/private/Authentication/Token/IToken.php | 6 ++-- .../Token/DefaultTokenProviderTest.php | 8 ++--- .../Authentication/Token/DefaultTokenTest.php | 8 ----- 6 files changed, 32 insertions(+), 30 deletions(-) diff --git a/lib/private/Authentication/Token/DefaultToken.php b/lib/private/Authentication/Token/DefaultToken.php index 8ddd9b00cf2cf..ad4ece0c46348 100644 --- a/lib/private/Authentication/Token/DefaultToken.php +++ b/lib/private/Authentication/Token/DefaultToken.php @@ -31,14 +31,12 @@ * @method void setUid(string $uid); * @method void setLoginName(string $loginname) * @method void setPassword(string $password) - * @method string getName() * @method void setName(string $name) * @method void setToken(string $token) * @method string getToken() * @method void setType(int $type) * @method int getType() * @method void setRemember(int $remember) - * @method int getRemember() * @method void setLastActivity(int $lastactivity) * @method int getLastActivity() */ @@ -107,9 +105,9 @@ public function getLoginName(): string { /** * Get the (encrypted) login password * - * @return string + * @return string|null */ - public function getPassword(): string { + public function getPassword() { return parent::getPassword(); } @@ -136,14 +134,18 @@ public function getLastCheck(): int { * Get the timestamp of the last password check * * @param int $time - * @return int */ - public function setLastCheck(int $time): int { - return parent::setLastCheck($time); + public function setLastCheck(int $time) { + parent::setLastCheck($time); } public function getScope(): string { - return parent::getScope(); + $scope = parent::getScope(); + if ($scope === null) { + return ''; + } + + return $scope; } public function getScopeAsArray(): array { @@ -156,7 +158,17 @@ public function getScopeAsArray(): array { return $scope; } - public function setScope(array $scope) { - parent::setScope(json_encode($scope)); + public function setScope(array $scope = null) { + if ($scope !== null) { + parent::setScope(json_encode($scope)); + } + } + + public function getName(): string { + return parent::getName(); + } + + public function getRemember(): int { + return parent::getRemember(); } } diff --git a/lib/private/Authentication/Token/DefaultTokenProvider.php b/lib/private/Authentication/Token/DefaultTokenProvider.php index b1d3d227aef2c..747fb8ef6ea42 100644 --- a/lib/private/Authentication/Token/DefaultTokenProvider.php +++ b/lib/private/Authentication/Token/DefaultTokenProvider.php @@ -174,11 +174,11 @@ public function getToken(string $tokenId): IToken { /** * Get a token by token id * - * @param string $tokenId + * @param int $tokenId * @throws InvalidTokenException * @return IToken */ - public function getTokenById(string $tokenId): IToken { + public function getTokenById(int $tokenId): IToken { try { return $this->mapper->getTokenById($tokenId); } catch (DoesNotExistException $ex) { diff --git a/lib/private/Authentication/Token/IProvider.php b/lib/private/Authentication/Token/IProvider.php index 1928fd3213663..9b9048b1635a0 100644 --- a/lib/private/Authentication/Token/IProvider.php +++ b/lib/private/Authentication/Token/IProvider.php @@ -65,11 +65,11 @@ public function getToken(string $tokenId): IToken; /** * Get a token by token id * - * @param string $tokenId + * @param int $tokenId * @throws InvalidTokenException * @return IToken */ - public function getTokenById(string $tokenId): IToken; + public function getTokenById(int $tokenId): IToken; /** * Duplicate an existing session token diff --git a/lib/private/Authentication/Token/IToken.php b/lib/private/Authentication/Token/IToken.php index 07f72d3767092..eff525c8d65b1 100644 --- a/lib/private/Authentication/Token/IToken.php +++ b/lib/private/Authentication/Token/IToken.php @@ -57,9 +57,9 @@ public function getLoginName(): string; /** * Get the (encrypted) login password * - * @return string + * @return string|null */ - public function getPassword(): string; + public function getPassword(); /** * Get the timestamp of the last password check @@ -94,7 +94,7 @@ public function getScopeAsArray(): array; * * @param array $scope */ - public function setScope(array $scope); + public function setScope(array $scope = null); public function getName(): string; diff --git a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php index 08c74961c0d34..a2128e0fd4c6d 100644 --- a/tests/lib/Authentication/Token/DefaultTokenProviderTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenProviderTest.php @@ -24,10 +24,10 @@ use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Token\DefaultToken; +use OC\Authentication\Token\DefaultTokenMapper; use OC\Authentication\Token\DefaultTokenProvider; use OC\Authentication\Token\IToken; use OCP\AppFramework\Db\DoesNotExistException; -use OCP\AppFramework\Db\Mapper; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; use OCP\ILogger; @@ -39,7 +39,7 @@ class DefaultTokenProviderTest extends TestCase { /** @var DefaultTokenProvider|\PHPUnit_Framework_MockObject_MockObject */ private $tokenProvider; - /** @var Mapper|\PHPUnit_Framework_MockObject_MockObject */ + /** @var DefaultTokenMapper|\PHPUnit_Framework_MockObject_MockObject */ private $mapper; /** @var ICrypto|\PHPUnit_Framework_MockObject_MockObject */ private $crypto; @@ -55,9 +55,7 @@ class DefaultTokenProviderTest extends TestCase { protected function setUp() { parent::setUp(); - $this->mapper = $this->getMockBuilder('\OC\Authentication\Token\DefaultTokenMapper') - ->disableOriginalConstructor() - ->getMock(); + $this->mapper = $this->createMock(DefaultTokenMapper::class); $this->crypto = $this->createMock(ICrypto::class); $this->config = $this->createMock(IConfig::class); $this->logger = $this->createMock(ILogger::class); diff --git a/tests/lib/Authentication/Token/DefaultTokenTest.php b/tests/lib/Authentication/Token/DefaultTokenTest.php index f00c32ccaf5cd..76b976586a987 100644 --- a/tests/lib/Authentication/Token/DefaultTokenTest.php +++ b/tests/lib/Authentication/Token/DefaultTokenTest.php @@ -33,14 +33,6 @@ public function testSetScopeAsArray() { $this->assertEquals($scope, $token->getScopeAsArray()); } - public function testSetScopeAsString() { - $scope = ['filesystem' => false]; - $token = new DefaultToken(); - $token->setScope(json_encode($scope)); - $this->assertEquals(json_encode($scope), $token->getScope()); - $this->assertEquals($scope, $token->getScopeAsArray()); - } - public function testDefaultScope() { $scope = ['filesystem' => true]; $token = new DefaultToken(); From 991d9b5c3acb21b65aed9c52c4b18c47ff1951e2 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 15 May 2018 11:32:25 +0200 Subject: [PATCH 3/4] Fix session tests Signed-off-by: Roeland Jago Douma --- tests/lib/User/SessionTest.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 9a5a45c46c5c5..24677b57dd67d 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -581,6 +581,8 @@ public function testRememberLoginValidToken() { $tokenObject->expects($this->once()) ->method('getLoginName') ->willReturn('foobar'); + $tokenObject->method('getId') + ->willReturn(42); $this->tokenProvider->expects($this->once()) ->method('getToken') ->with($sessionId) @@ -593,15 +595,22 @@ public function testRememberLoginValidToken() { ->method('setMagicInCookie'); $user->expects($this->once()) ->method('updateLastLoginTimestamp'); - $session->expects($this->once()) + $setUID = false; + $session ->method('set') - ->with('user_id', 'foo'); + ->will($this->returnCallback(function ($k, $v) use (&$setUID) { + if ($k === 'user_id' && $v === 'foo') { + $setUID = true; + } + })); $userSession->expects($this->once()) ->method('setLoginName') ->willReturn('foobar'); $granted = $userSession->loginWithCookie('foo', $token, $oldSessionId); + $this->assertTrue($setUID); + $this->assertTrue($granted); } From 4ea2daf04dd4df47fe1bdfcae3e87d0122d26fce Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 15 May 2018 11:41:27 +0200 Subject: [PATCH 4/4] Refix scope Signed-off-by: Roeland Jago Douma --- lib/private/Authentication/Token/DefaultToken.php | 6 ++++-- lib/private/Authentication/Token/IToken.php | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/Authentication/Token/DefaultToken.php b/lib/private/Authentication/Token/DefaultToken.php index ad4ece0c46348..e2753ba979c2b 100644 --- a/lib/private/Authentication/Token/DefaultToken.php +++ b/lib/private/Authentication/Token/DefaultToken.php @@ -158,9 +158,11 @@ public function getScopeAsArray(): array { return $scope; } - public function setScope(array $scope = null) { - if ($scope !== null) { + public function setScope($scope) { + if (\is_array($scope)) { parent::setScope(json_encode($scope)); + } else { + parent::setScope((string)$scope); } } diff --git a/lib/private/Authentication/Token/IToken.php b/lib/private/Authentication/Token/IToken.php index eff525c8d65b1..b40f55fb6ca64 100644 --- a/lib/private/Authentication/Token/IToken.php +++ b/lib/private/Authentication/Token/IToken.php @@ -94,7 +94,7 @@ public function getScopeAsArray(): array; * * @param array $scope */ - public function setScope(array $scope = null); + public function setScope($scope); public function getName(): string;