From d1a94f3c9cf9b6ce526e02de15f88f5515d64fdb Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 30 Jan 2025 17:40:15 +0100 Subject: [PATCH] feat(login-flow-v2): Restrict allowed apps by user agent check Enable via: ./occ config:system:set core.login_flow_v2.allowed_user_agents 0 --value '/Custom Foo Client/i' ./occ config:system:set core.login_flow_v2.allowed_user_agents 1 --value '/Custom Bar Client/i' if user agent string is unknown the template with "Access forbidden"-"Please use original client" will be displayed Signed-off-by: Misha M.-Kupriyanov --- config/config.sample.php | 14 ++++ .../ClientFlowLoginV2Controller.php | 23 +++++++ .../LoginFlowV2ClientForbiddenException.php | 12 ++++ core/Service/LoginFlowV2Service.php | 23 ++++++- lib/composer/autoload.php | 5 +- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../ClientFlowLoginV2ControllerTest.php | 69 +++++++++++++++++++ .../Service/LoginFlowV2ServiceUnitTest.php | 53 ++++++++++++++ 9 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 core/Exception/LoginFlowV2ClientForbiddenException.php diff --git a/config/config.sample.php b/config/config.sample.php index ddc0deb79b9..ddd91d84a3e 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2515,6 +2515,20 @@ $CONFIG = [ '/^Microsoft-WebDAV-MiniRedir/', // Windows webdav drive ], +/** + * This option allows you to specify a list of allowed user agents for the Login Flow V2. + * If a user agent is not in this list, it will not be allowed to use the Login Flow V2. + * The user agents are defined using regular expressions. + * + * WARNING: only use this if you know what you are doing + * + * Example: Allow only the Nextcloud Android app to use the Login Flow V2 + * 'core.login_flow_v2.allowed_user_agents' => ['/Nextcloud-android/i'], + * + * Defaults to an empty array. + */ +'core.login_flow_v2.allowed_user_agents' => [], + /** * By default, there is on public pages a link shown that allows users to * learn about the "simple sign up" - see https://nextcloud.com/signup/ diff --git a/core/Controller/ClientFlowLoginV2Controller.php b/core/Controller/ClientFlowLoginV2Controller.php index 1ce43c19932..b4a7622161f 100644 --- a/core/Controller/ClientFlowLoginV2Controller.php +++ b/core/Controller/ClientFlowLoginV2Controller.php @@ -9,6 +9,7 @@ declare(strict_types=1); namespace OC\Core\Controller; use OC\Core\Db\LoginFlowV2; +use OC\Core\Exception\LoginFlowV2ClientForbiddenException; use OC\Core\Exception\LoginFlowV2NotFoundException; use OC\Core\ResponseDefinitions; use OC\Core\Service\LoginFlowV2Service; @@ -109,6 +110,8 @@ class ClientFlowLoginV2Controller extends Controller { $flow = $this->getFlowByLoginToken(); } catch (LoginFlowV2NotFoundException $e) { return $this->loginTokenForbiddenResponse(); + } catch (LoginFlowV2ClientForbiddenException $e) { + return $this->loginTokenForbiddenClientResponse(); } $stateToken = $this->random->generate( @@ -152,6 +155,8 @@ class ClientFlowLoginV2Controller extends Controller { $flow = $this->getFlowByLoginToken(); } catch (LoginFlowV2NotFoundException $e) { return $this->loginTokenForbiddenResponse(); + } catch (LoginFlowV2ClientForbiddenException $e) { + return $this->loginTokenForbiddenClientResponse(); } /** @var IUser $user */ @@ -188,6 +193,8 @@ class ClientFlowLoginV2Controller extends Controller { $this->getFlowByLoginToken(); } catch (LoginFlowV2NotFoundException $e) { return $this->loginTokenForbiddenResponse(); + } catch (LoginFlowV2ClientForbiddenException $e) { + return $this->loginTokenForbiddenClientResponse(); } $loginToken = $this->session->get(self::TOKEN_NAME); @@ -233,6 +240,8 @@ class ClientFlowLoginV2Controller extends Controller { $this->getFlowByLoginToken(); } catch (LoginFlowV2NotFoundException $e) { return $this->loginTokenForbiddenResponse(); + } catch (LoginFlowV2ClientForbiddenException $e) { + return $this->loginTokenForbiddenClientResponse(); } $loginToken = $this->session->get(self::TOKEN_NAME); @@ -333,6 +342,7 @@ class ClientFlowLoginV2Controller extends Controller { /** * @return LoginFlowV2 * @throws LoginFlowV2NotFoundException + * @throws LoginFlowV2ClientForbiddenException */ private function getFlowByLoginToken(): LoginFlowV2 { $currentToken = $this->session->get(self::TOKEN_NAME); @@ -356,6 +366,19 @@ class ClientFlowLoginV2Controller extends Controller { return $response; } + private function loginTokenForbiddenClientResponse(): StandaloneTemplateResponse { + $response = new StandaloneTemplateResponse( + $this->appName, + '403', + [ + 'message' => $this->l10n->t('Please use original client'), + ], + 'guest' + ); + $response->setStatus(Http::STATUS_FORBIDDEN); + return $response; + } + private function getServerPath(): string { $serverPostfix = ''; diff --git a/core/Exception/LoginFlowV2ClientForbiddenException.php b/core/Exception/LoginFlowV2ClientForbiddenException.php new file mode 100644 index 00000000000..e6ac6d9618f --- /dev/null +++ b/core/Exception/LoginFlowV2ClientForbiddenException.php @@ -0,0 +1,12 @@ +mapper->getByLoginToken($loginToken); + $flow = $this->mapper->getByLoginToken($loginToken); } catch (DoesNotExistException $e) { throw new LoginFlowV2NotFoundException('Login token invalid'); } + + $allowedAgents = $this->config->getSystemValue('core.login_flow_v2.allowed_user_agents', []); + + if (empty($allowedAgents)) { + return $flow; + } + + $flowClient = $flow->getClientName(); + + foreach ($allowedAgents as $allowedAgent) { + if (preg_match($allowedAgent, $flowClient) === 1) { + return $flow; + } + } + + throw new LoginFlowV2ClientForbiddenException('Client not allowed'); } /** diff --git a/lib/composer/autoload.php b/lib/composer/autoload.php index 7b1481e876c..b3b39129e7a 100644 --- a/lib/composer/autoload.php +++ b/lib/composer/autoload.php @@ -14,7 +14,10 @@ if (PHP_VERSION_ID < 50600) { echo $err; } } - throw new RuntimeException($err); + trigger_error( + $err, + E_USER_ERROR + ); } require_once __DIR__ . '/composer/autoload_real.php'; diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 1f3d9d3813b..3d36f50e950 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1371,6 +1371,7 @@ return array( 'OC\\Core\\Db\\ProfileConfigMapper' => $baseDir . '/core/Db/ProfileConfigMapper.php', 'OC\\Core\\Events\\BeforePasswordResetEvent' => $baseDir . '/core/Events/BeforePasswordResetEvent.php', 'OC\\Core\\Events\\PasswordResetEvent' => $baseDir . '/core/Events/PasswordResetEvent.php', + 'OC\\Core\\Exception\\LoginFlowV2ClientForbiddenException' => $baseDir . '/core/Exception/LoginFlowV2ClientForbiddenException.php', 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => $baseDir . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => $baseDir . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeMessageLoggedEventListener' => $baseDir . '/core/Listener/BeforeMessageLoggedEventListener.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 8c87d90156d..7085d0b4a44 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1412,6 +1412,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Db\\ProfileConfigMapper' => __DIR__ . '/../../..' . '/core/Db/ProfileConfigMapper.php', 'OC\\Core\\Events\\BeforePasswordResetEvent' => __DIR__ . '/../../..' . '/core/Events/BeforePasswordResetEvent.php', 'OC\\Core\\Events\\PasswordResetEvent' => __DIR__ . '/../../..' . '/core/Events/PasswordResetEvent.php', + 'OC\\Core\\Exception\\LoginFlowV2ClientForbiddenException' => __DIR__ . '/../../..' . '/core/Exception/LoginFlowV2ClientForbiddenException.php', 'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => __DIR__ . '/../../..' . '/core/Exception/LoginFlowV2NotFoundException.php', 'OC\\Core\\Exception\\ResetPasswordException' => __DIR__ . '/../../..' . '/core/Exception/ResetPasswordException.php', 'OC\\Core\\Listener\\BeforeMessageLoggedEventListener' => __DIR__ . '/../../..' . '/core/Listener/BeforeMessageLoggedEventListener.php', diff --git a/tests/Core/Controller/ClientFlowLoginV2ControllerTest.php b/tests/Core/Controller/ClientFlowLoginV2ControllerTest.php index 98c7821791d..81de4cd92da 100644 --- a/tests/Core/Controller/ClientFlowLoginV2ControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginV2ControllerTest.php @@ -11,6 +11,7 @@ namespace Test\Core\Controller; use OC\Core\Controller\ClientFlowLoginV2Controller; use OC\Core\Data\LoginFlowV2Credentials; use OC\Core\Db\LoginFlowV2; +use OC\Core\Exception\LoginFlowV2ClientForbiddenException; use OC\Core\Exception\LoginFlowV2NotFoundException; use OC\Core\Service\LoginFlowV2Service; use OCP\AppFramework\Http; @@ -56,6 +57,12 @@ class ClientFlowLoginV2ControllerTest extends TestCase { $this->random = $this->createMock(ISecureRandom::class); $this->defaults = $this->createMock(Defaults::class); $this->l = $this->createMock(IL10N::class); + $this->l + ->expects($this->any()) + ->method('t') + ->willReturnCallback(function ($text, $parameters = []) { + return vsprintf($text, $parameters); + }); $this->controller = new ClientFlowLoginV2Controller( 'core', $this->request, @@ -150,6 +157,22 @@ class ClientFlowLoginV2ControllerTest extends TestCase { $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); } + public function testShowAuthPickerForbiddenUserClient() { + $this->session->method('get') + ->with('client.flow.v2.login.token') + ->willReturn('loginToken'); + + $this->loginFlowV2Service->method('getByLoginToken') + ->with('loginToken') + ->willThrowException(new LoginFlowV2ClientForbiddenException()); + + $result = $this->controller->showAuthPickerPage(); + + $this->assertInstanceOf(Http\StandaloneTemplateResponse::class, $result); + $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); + $this->assertSame('Please use original client', $result->getParams()['message']); + } + public function testShowAuthPickerValidLoginToken(): void { $this->session->method('get') ->with('client.flow.v2.login.token') @@ -206,6 +229,29 @@ class ClientFlowLoginV2ControllerTest extends TestCase { $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); } + public function testGrantPageForbiddenUserClient() { + $this->session->method('get') + ->willReturnCallback(function ($name) { + if ($name === 'client.flow.v2.state.token') { + return 'stateToken'; + } + if ($name === 'client.flow.v2.login.token') { + return 'loginToken'; + } + return null; + }); + + $this->loginFlowV2Service->method('getByLoginToken') + ->with('loginToken') + ->willThrowException(new LoginFlowV2ClientForbiddenException()); + + $result = $this->controller->grantPage('stateToken'); + + $this->assertInstanceOf(Http\StandaloneTemplateResponse::class, $result); + $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); + $this->assertSame('Please use original client', $result->getParams()['message']); + } + public function testGrantPageValid(): void { $this->session->method('get') ->willReturnCallback(function ($name) { @@ -266,6 +312,29 @@ class ClientFlowLoginV2ControllerTest extends TestCase { $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); } + public function testGenerateAppPasswordForbiddenUserClient() { + $this->session->method('get') + ->willReturnCallback(function ($name) { + if ($name === 'client.flow.v2.state.token') { + return 'stateToken'; + } + if ($name === 'client.flow.v2.login.token') { + return 'loginToken'; + } + return null; + }); + + $this->loginFlowV2Service->method('getByLoginToken') + ->with('loginToken') + ->willThrowException(new LoginFlowV2ClientForbiddenException()); + + $result = $this->controller->generateAppPassword('stateToken'); + + $this->assertInstanceOf(Http\StandaloneTemplateResponse::class, $result); + $this->assertSame(Http::STATUS_FORBIDDEN, $result->getStatus()); + $this->assertSame('Please use original client', $result->getParams()['message']); + } + public function testGenerateAppPassworValid(): void { $this->session->method('get') ->willReturnCallback(function ($name) { diff --git a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php index 1c31efee8f7..8118106c722 100644 --- a/tests/Core/Service/LoginFlowV2ServiceUnitTest.php +++ b/tests/Core/Service/LoginFlowV2ServiceUnitTest.php @@ -1,4 +1,5 @@ subjectUnderTest->getByLoginToken('test_token'); } + public function testGetByLoginTokenClientForbidden() { + $this->expectException(LoginFlowV2ClientForbiddenException::class); + $this->expectExceptionMessage('Client not allowed'); + + $allowedClients = [ + '/Custom Allowed Client/i' + ]; + + $this->config->expects($this->exactly(1)) + ->method('getSystemValue') + ->willReturn($this->returnCallback(function ($key) use ($allowedClients) { + // Note: \OCP\IConfig::getSystemValue returns either an array or string. + return $key == 'core.login_flow_v2.allowed_user_agents' ? $allowedClients : ''; + })); + + $loginFlowV2 = new LoginFlowV2(); + $loginFlowV2->setClientName('Rogue Curl Client/1.0'); + + $this->mapper->expects($this->once()) + ->method('getByLoginToken') + ->willReturn($loginFlowV2); + + $this->subjectUnderTest->getByLoginToken('test_token'); + } + + public function testGetByLoginTokenClientAllowed() { + $allowedClients = [ + '/Foo Allowed Client/i', + '/Custom Allowed Client/i' + ]; + + $loginFlowV2 = new LoginFlowV2(); + $loginFlowV2->setClientName('Custom Allowed Client Curl Client/1.0'); + + $this->config->expects($this->exactly(1)) + ->method('getSystemValue') + ->willReturn($this->returnCallback(function ($key) use ($allowedClients) { + // Note: \OCP\IConfig::getSystemValue returns either an array or string. + return $key == 'core.login_flow_v2.allowed_user_agents' ? $allowedClients : ''; + })); + + $this->mapper->expects($this->once()) + ->method('getByLoginToken') + ->willReturn($loginFlowV2); + + $result = $this->subjectUnderTest->getByLoginToken('test_token'); + + $this->assertTrue($result instanceof LoginFlowV2); + $this->assertEquals('Custom Allowed Client Curl Client/1.0', $result->getClientName()); + } + /* * Tests for startLoginFlow */