From 21be557e2afe9e3a1e024d9618a377816d73d63a Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Fri, 11 Nov 2022 13:16:14 +0545 Subject: [PATCH 1/7] invalidate existing tokens when deleting an oauth client Signed-off-by: Artur Neumann --- .../lib/Controller/SettingsController.php | 28 ++++++++- .../Controller/SettingsControllerTest.php | 62 +++++++++++++++++-- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php index 046e6d7704188..872288064ddf2 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -30,6 +30,7 @@ */ namespace OCA\OAuth2\Controller; +use OC\Authentication\Token\IProvider as IAuthTokenProvider; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\Client; use OCA\OAuth2\Db\ClientMapper; @@ -38,6 +39,8 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\IL10N; use OCP\IRequest; +use OCP\IUser; +use OCP\IUserManager; use OCP\Security\ISecureRandom; class SettingsController extends Controller { @@ -49,7 +52,12 @@ class SettingsController extends Controller { private $accessTokenMapper; /** @var IL10N */ private $l; - + /** @var IAuthTokenProvider */ + private $tokenProvider; + /** + * @var IUserManager + */ + private $userManager; public const validChars = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; public function __construct(string $appName, @@ -57,13 +65,17 @@ public function __construct(string $appName, ClientMapper $clientMapper, ISecureRandom $secureRandom, AccessTokenMapper $accessTokenMapper, - IL10N $l + IL10N $l, + IAuthTokenProvider $tokenProvider, + IUserManager $userManager ) { parent::__construct($appName, $request); $this->secureRandom = $secureRandom; $this->clientMapper = $clientMapper; $this->accessTokenMapper = $accessTokenMapper; $this->l = $l; + $this->tokenProvider = $tokenProvider; + $this->userManager = $userManager; } public function addClient(string $name, @@ -92,6 +104,18 @@ public function addClient(string $name, public function deleteClient(int $id): JSONResponse { $client = $this->clientMapper->getByUid($id); + + $this->userManager->callForAllUsers(function (IUser $user) use ($client) { + $tokens = $this->tokenProvider->getTokenByUser($user->getUID()); + foreach ($tokens as $token) { + if ($token->getName() === $client->getName()) { + $this->tokenProvider->invalidateTokenById( + $user->getUID(), $token->getId() + ); + } + } + }); + $this->accessTokenMapper->deleteByClientId($id); $this->clientMapper->delete($client); return new JSONResponse([]); diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index 216655190ae58..6be8a4ee4b983 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -26,6 +26,8 @@ */ namespace OCA\OAuth2\Tests\Controller; +use OC\Authentication\Token\IToken; +use OC\Authentication\Token\IProvider as IAuthTokenProvider; use OCA\OAuth2\Controller\SettingsController; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\Client; @@ -34,9 +36,14 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\IL10N; use OCP\IRequest; +use OCP\IUser; +use OCP\IUserManager; use OCP\Security\ISecureRandom; use Test\TestCase; +/** + * @group DB + */ class SettingsControllerTest extends TestCase { /** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */ private $request; @@ -48,6 +55,8 @@ class SettingsControllerTest extends TestCase { private $accessTokenMapper; /** @var SettingsController */ private $settingsController; + /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ + private $l; protected function setUp(): void { parent::setUp(); @@ -56,18 +65,20 @@ protected function setUp(): void { $this->clientMapper = $this->createMock(ClientMapper::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->accessTokenMapper = $this->createMock(AccessTokenMapper::class); - $l = $this->createMock(IL10N::class); - $l->method('t') + $this->l = $this->createMock(IL10N::class); + $this->l->method('t') ->willReturnArgument(0); - $this->settingsController = new SettingsController( 'oauth2', $this->request, $this->clientMapper, $this->secureRandom, $this->accessTokenMapper, - $l + $this->l, + $this->createMock(IAuthTokenProvider::class), + $this->createMock(IUserManager::class) ); + } public function testAddClient() { @@ -113,6 +124,34 @@ public function testAddClient() { } public function testDeleteClient() { + + $userManager = \OC::$server->getUserManager(); + // count other users in the db before adding our own + $count = 0; + $function = function (IUser $user) use (&$count) { + $count++; + }; + $userManager->callForAllUsers($function); + $user1 = $userManager->createUser('test101', 'test101'); + $tokenMocks[0] = $this->getMockBuilder(IToken::class)->getMock(); + $tokenMocks[0]->method('getName')->willReturn('Firefox session'); + $tokenMocks[0]->method('getId')->willReturn(1); + $tokenMocks[1] = $this->getMockBuilder(IToken::class)->getMock(); + $tokenMocks[1]->method('getName')->willReturn('My Client Name'); + $tokenMocks[1]->method('getId')->willReturn(2); + $tokenMocks[2] = $this->getMockBuilder(IToken::class)->getMock(); + $tokenMocks[2]->method('getName')->willReturn('mobile client'); + $tokenMocks[2]->method('getId')->willReturn(3); + + $tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock(); + $tokenProviderMock->method('getTokenByUser')->willReturn($tokenMocks); + + // expect one call per user and make sure the correct tokeId is selected + $tokenProviderMock + ->expects($this->exactly($count + 1)) + ->method('invalidateTokenById') + ->with($this->isType('string'), 2); + $client = new Client(); $client->setId(123); $client->setName('My Client Name'); @@ -132,9 +171,22 @@ public function testDeleteClient() { ->method('delete') ->with($client); - $result = $this->settingsController->deleteClient(123); + $settingsController = new SettingsController( + 'oauth2', + $this->request, + $this->clientMapper, + $this->secureRandom, + $this->accessTokenMapper, + $this->l, + $tokenProviderMock, + $userManager + ); + + $result = $settingsController->deleteClient(123); $this->assertInstanceOf(JSONResponse::class, $result); $this->assertEquals([], $result->getData()); + + $user1->delete(); } public function testInvalidRedirectUri() { From f634badf1218baff90acde501859081e3765d79f Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Mon, 21 Nov 2022 17:28:21 +0545 Subject: [PATCH 2/7] public interface to invalidate tokens of user Signed-off-by: Artur Neumann --- .../lib/Controller/SettingsController.php | 11 +----- lib/private/Authentication/Token/Manager.php | 12 +++++- lib/private/Server.php | 2 + lib/public/Authentication/Token/IProvider.php | 37 +++++++++++++++++++ 4 files changed, 52 insertions(+), 10 deletions(-) create mode 100644 lib/public/Authentication/Token/IProvider.php diff --git a/apps/oauth2/lib/Controller/SettingsController.php b/apps/oauth2/lib/Controller/SettingsController.php index 872288064ddf2..c24308140ecbb 100644 --- a/apps/oauth2/lib/Controller/SettingsController.php +++ b/apps/oauth2/lib/Controller/SettingsController.php @@ -30,7 +30,7 @@ */ namespace OCA\OAuth2\Controller; -use OC\Authentication\Token\IProvider as IAuthTokenProvider; +use OCP\Authentication\Token\IProvider as IAuthTokenProvider; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\Client; use OCA\OAuth2\Db\ClientMapper; @@ -106,14 +106,7 @@ public function deleteClient(int $id): JSONResponse { $client = $this->clientMapper->getByUid($id); $this->userManager->callForAllUsers(function (IUser $user) use ($client) { - $tokens = $this->tokenProvider->getTokenByUser($user->getUID()); - foreach ($tokens as $token) { - if ($token->getName() === $client->getName()) { - $this->tokenProvider->invalidateTokenById( - $user->getUID(), $token->getId() - ); - } - } + $this->tokenProvider->invalidateTokensOfUser($user->getUID(), $client->getName()); }); $this->accessTokenMapper->deleteByClientId($id); diff --git a/lib/private/Authentication/Token/Manager.php b/lib/private/Authentication/Token/Manager.php index 59c7ca714c632..761e799d29835 100644 --- a/lib/private/Authentication/Token/Manager.php +++ b/lib/private/Authentication/Token/Manager.php @@ -32,8 +32,9 @@ use OC\Authentication\Exceptions\InvalidTokenException; use OC\Authentication\Exceptions\PasswordlessTokenException; use OC\Authentication\Exceptions\WipeTokenException; +use OCP\Authentication\Token\IProvider as OCPIProvider; -class Manager implements IProvider { +class Manager implements IProvider, OCPIProvider { /** @var PublicKeyTokenProvider */ private $publicKeyTokenProvider; @@ -239,4 +240,13 @@ public function markPasswordInvalid(IToken $token, string $tokenId) { public function updatePasswords(string $uid, string $password) { $this->publicKeyTokenProvider->updatePasswords($uid, $password); } + + public function invalidateTokensOfUser(string $uid, ?string $clientName) { + $tokens = $this->getTokenByUser($uid); + foreach ($tokens as $token) { + if ($clientName === null || ($token->getName() === $clientName)) { + $this->invalidateTokenById($uid, $token->getId()); + } + } + } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 9a4ee0da19882..f1e9617088662 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -163,6 +163,7 @@ use OCP\Accounts\IAccountManager; use OCP\App\IAppManager; use OCP\Authentication\LoginCredentials\IStore; +use OCP\Authentication\Token\IProvider as OCPIProvider; use OCP\BackgroundJob\IJobList; use OCP\Collaboration\AutoComplete\IManager; use OCP\Collaboration\Reference\IReferenceManager; @@ -550,6 +551,7 @@ public function __construct($webRoot, \OC\Config $config) { }); $this->registerAlias(IStore::class, Store::class); $this->registerAlias(IProvider::class, Authentication\Token\Manager::class); + $this->registerAlias(OCPIProvider::class, Authentication\Token\Manager::class); $this->registerService(\OC\User\Session::class, function (Server $c) { $manager = $c->get(IUserManager::class); diff --git a/lib/public/Authentication/Token/IProvider.php b/lib/public/Authentication/Token/IProvider.php new file mode 100644 index 0000000000000..9000868907e9f --- /dev/null +++ b/lib/public/Authentication/Token/IProvider.php @@ -0,0 +1,37 @@ + + * + * @author Artur Neumann + * + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ +namespace OCP\Authentication\Token; + +interface IProvider { + /** + * invalidates all tokens of a specific user + * if a client name is given only tokens of that client will be invalidated + * + * @param string $uid + * @param string|null $clientName + * @return void + */ + public function invalidateTokensOfUser(string $uid, ?string $clientName); +} From 707e69b203bd63149e9731a76a93049cc0eaf12e Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Tue, 22 Nov 2022 12:15:28 +0545 Subject: [PATCH 3/7] adjust SettingsController tests Signed-off-by: Artur Neumann --- .../Controller/SettingsControllerTest.php | 20 +++++-------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index 6be8a4ee4b983..b11c59fd41492 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -27,7 +27,7 @@ namespace OCA\OAuth2\Tests\Controller; use OC\Authentication\Token\IToken; -use OC\Authentication\Token\IProvider as IAuthTokenProvider; +use OCP\Authentication\Token\IProvider as IAuthTokenProvider; use OCA\OAuth2\Controller\SettingsController; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\Client; @@ -133,24 +133,13 @@ public function testDeleteClient() { }; $userManager->callForAllUsers($function); $user1 = $userManager->createUser('test101', 'test101'); - $tokenMocks[0] = $this->getMockBuilder(IToken::class)->getMock(); - $tokenMocks[0]->method('getName')->willReturn('Firefox session'); - $tokenMocks[0]->method('getId')->willReturn(1); - $tokenMocks[1] = $this->getMockBuilder(IToken::class)->getMock(); - $tokenMocks[1]->method('getName')->willReturn('My Client Name'); - $tokenMocks[1]->method('getId')->willReturn(2); - $tokenMocks[2] = $this->getMockBuilder(IToken::class)->getMock(); - $tokenMocks[2]->method('getName')->willReturn('mobile client'); - $tokenMocks[2]->method('getId')->willReturn(3); - $tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock(); - $tokenProviderMock->method('getTokenByUser')->willReturn($tokenMocks); - // expect one call per user and make sure the correct tokeId is selected + // expect one call per user and ensure the correct client name $tokenProviderMock ->expects($this->exactly($count + 1)) - ->method('invalidateTokenById') - ->with($this->isType('string'), 2); + ->method('invalidateTokensOfUser') + ->with($this->isType('string'), 'My Client Name'); $client = new Client(); $client->setId(123); @@ -168,6 +157,7 @@ public function testDeleteClient() { ->method('deleteByClientId') ->with(123); $this->clientMapper + ->expects($this->once()) ->method('delete') ->with($client); From 37cfccabc144d70b56219bd350200accb9ce4814 Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Tue, 22 Nov 2022 12:28:35 +0545 Subject: [PATCH 4/7] unit tests for Manager::invalidateTokensOfUser Signed-off-by: Artur Neumann --- .../lib/Authentication/Token/ManagerTest.php | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/lib/Authentication/Token/ManagerTest.php b/tests/lib/Authentication/Token/ManagerTest.php index 5f024bb1d43c0..de3e5e1c36202 100644 --- a/tests/lib/Authentication/Token/ManagerTest.php +++ b/tests/lib/Authentication/Token/ManagerTest.php @@ -355,4 +355,48 @@ public function testUpdatePasswords() { $this->manager->updatePasswords('uid', 'pass'); } + + public function testInvalidateTokensOfUserNoClientName() { + $t1 = new PublicKeyToken(); + $t2 = new PublicKeyToken(); + $t1->setId(123); + $t2->setId(456); + + $this->publicKeyTokenProvider + ->expects($this->once()) + ->method('getTokenByUser') + ->with('theUser') + ->willReturn([$t1, $t2]); + $this->publicKeyTokenProvider + ->expects($this->exactly(2)) + ->method('invalidateTokenById') + ->withConsecutive( + ['theUser', 123], + ['theUser', 456], + ); + $this->manager->invalidateTokensOfUser('theUser', null); + } + + public function testInvalidateTokensOfUserClientNameGiven() { + $t1 = new PublicKeyToken(); + $t2 = new PublicKeyToken(); + $t3 = new PublicKeyToken(); + $t1->setId(123); + $t1->setName('Firefox session'); + $t2->setId(456); + $t2->setName('My Client Name'); + $t3->setId(789); + $t3->setName('mobile client'); + + $this->publicKeyTokenProvider + ->expects($this->once()) + ->method('getTokenByUser') + ->with('theUser') + ->willReturn([$t1, $t2, $t3]); + $this->publicKeyTokenProvider + ->expects($this->once()) + ->method('invalidateTokenById') + ->with('theUser', 456); + $this->manager->invalidateTokensOfUser('theUser', 'My Client Name'); + } } From 565fad8d8c884653b35e2d52522cd766f5b3d503 Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Tue, 22 Nov 2022 12:52:29 +0545 Subject: [PATCH 5/7] added @since tag Signed-off-by: Artur Neumann --- lib/public/Authentication/Token/IProvider.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/public/Authentication/Token/IProvider.php b/lib/public/Authentication/Token/IProvider.php index 9000868907e9f..da2e400eb79ec 100644 --- a/lib/public/Authentication/Token/IProvider.php +++ b/lib/public/Authentication/Token/IProvider.php @@ -24,6 +24,9 @@ */ namespace OCP\Authentication\Token; +/** + * @since 24.0.8 + */ interface IProvider { /** * invalidates all tokens of a specific user @@ -31,6 +34,7 @@ interface IProvider { * * @param string $uid * @param string|null $clientName + * @since 24.0.8 * @return void */ public function invalidateTokensOfUser(string $uid, ?string $clientName); From e9f5e796f01413ab70282adb3f1dc4593207ec58 Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Fri, 6 Jan 2023 16:58:51 +0545 Subject: [PATCH 6/7] autoloaderchecker Signed-off-by: Artur Neumann --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 2 files changed, 2 insertions(+) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 1ecd8152c0f37..d21eb18cc5aed 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -96,6 +96,7 @@ 'OCP\\Authentication\\IProvideUserSecretBackend' => $baseDir . '/lib/public/Authentication/IProvideUserSecretBackend.php', 'OCP\\Authentication\\LoginCredentials\\ICredentials' => $baseDir . '/lib/public/Authentication/LoginCredentials/ICredentials.php', 'OCP\\Authentication\\LoginCredentials\\IStore' => $baseDir . '/lib/public/Authentication/LoginCredentials/IStore.php', + 'OCP\\Authentication\\Token\\IProvider' => $baseDir . '/lib/public/Authentication/Token/IProvider.php', 'OCP\\Authentication\\TwoFactorAuth\\ALoginSetupController' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/ALoginSetupController.php', 'OCP\\Authentication\\TwoFactorAuth\\IActivatableAtLogin' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/IActivatableAtLogin.php', 'OCP\\Authentication\\TwoFactorAuth\\IActivatableByAdmin' => $baseDir . '/lib/public/Authentication/TwoFactorAuth/IActivatableByAdmin.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2127835e51483..b4f25f90d7c0c 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -129,6 +129,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Authentication\\IProvideUserSecretBackend' => __DIR__ . '/../../..' . '/lib/public/Authentication/IProvideUserSecretBackend.php', 'OCP\\Authentication\\LoginCredentials\\ICredentials' => __DIR__ . '/../../..' . '/lib/public/Authentication/LoginCredentials/ICredentials.php', 'OCP\\Authentication\\LoginCredentials\\IStore' => __DIR__ . '/../../..' . '/lib/public/Authentication/LoginCredentials/IStore.php', + 'OCP\\Authentication\\Token\\IProvider' => __DIR__ . '/../../..' . '/lib/public/Authentication/Token/IProvider.php', 'OCP\\Authentication\\TwoFactorAuth\\ALoginSetupController' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/ALoginSetupController.php', 'OCP\\Authentication\\TwoFactorAuth\\IActivatableAtLogin' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/IActivatableAtLogin.php', 'OCP\\Authentication\\TwoFactorAuth\\IActivatableByAdmin' => __DIR__ . '/../../..' . '/lib/public/Authentication/TwoFactorAuth/IActivatableByAdmin.php', From e97540b9c61e736e9d541dffedd6064680418bc3 Mon Sep 17 00:00:00 2001 From: Artur Neumann Date: Mon, 9 Jan 2023 12:46:10 +0545 Subject: [PATCH 7/7] move mocks into private variables Signed-off-by: Artur Neumann --- .../oauth2/tests/Controller/SettingsControllerTest.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/oauth2/tests/Controller/SettingsControllerTest.php b/apps/oauth2/tests/Controller/SettingsControllerTest.php index b11c59fd41492..e79d7cbe34ef1 100644 --- a/apps/oauth2/tests/Controller/SettingsControllerTest.php +++ b/apps/oauth2/tests/Controller/SettingsControllerTest.php @@ -53,6 +53,10 @@ class SettingsControllerTest extends TestCase { private $secureRandom; /** @var AccessTokenMapper|\PHPUnit\Framework\MockObject\MockObject */ private $accessTokenMapper; + /** @var IAuthTokenProvider|\PHPUnit\Framework\MockObject\MockObject */ + private $authTokenProvider; + /** @var IUserManager|\PHPUnit\Framework\MockObject\MockObject */ + private $userManager; /** @var SettingsController */ private $settingsController; /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ @@ -65,6 +69,8 @@ protected function setUp(): void { $this->clientMapper = $this->createMock(ClientMapper::class); $this->secureRandom = $this->createMock(ISecureRandom::class); $this->accessTokenMapper = $this->createMock(AccessTokenMapper::class); + $this->authTokenProvider = $this->createMock(IAuthTokenProvider::class); + $this->userManager = $this->createMock(IUserManager::class); $this->l = $this->createMock(IL10N::class); $this->l->method('t') ->willReturnArgument(0); @@ -75,8 +81,8 @@ protected function setUp(): void { $this->secureRandom, $this->accessTokenMapper, $this->l, - $this->createMock(IAuthTokenProvider::class), - $this->createMock(IUserManager::class) + $this->authTokenProvider, + $this->userManager ); }