Skip to content
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.

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

feat(login-flow-v2): Restrict allowed apps by user agent check #50650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions config/config.sample.php
Original file line number Diff line number Diff line change
Expand Up @@ -2438,6 +2438,20 @@
'/^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/
Expand Down
23 changes: 23 additions & 0 deletions core/Controller/ClientFlowLoginV2Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

use OC\Core\Db\LoginFlowV2;
use OC\Core\Exception\LoginFlowV2NotFoundException;
use OC\Core\Exception\LoginFlowV2ClientForbiddenException;
use OC\Core\ResponseDefinitions;
use OC\Core\Service\LoginFlowV2Service;
use OCP\AppFramework\Controller;
Expand Down Expand Up @@ -107,6 +108,8 @@ public function showAuthPickerPage(string $user = '', int $direct = 0): Standalo
$flow = $this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
return $this->loginTokenForbiddenResponse();
} catch (LoginFlowV2ClientForbiddenException $e) {
return $this->loginTokenForbiddenClientResponse();
}

$stateToken = $this->random->generate(
Expand Down Expand Up @@ -150,6 +153,8 @@ public function grantPage(?string $stateToken, int $direct = 0): StandaloneTempl
$flow = $this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
return $this->loginTokenForbiddenResponse();
} catch (LoginFlowV2ClientForbiddenException $e) {
return $this->loginTokenForbiddenClientResponse();
}

/** @var IUser $user */
Expand Down Expand Up @@ -186,6 +191,8 @@ public function apptokenRedirect(?string $stateToken, string $user, string $pass
$this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
return $this->loginTokenForbiddenResponse();
} catch (LoginFlowV2ClientForbiddenException $e) {
return $this->loginTokenForbiddenClientResponse();
}

$loginToken = $this->session->get(self::TOKEN_NAME);
Expand Down Expand Up @@ -231,6 +238,8 @@ public function generateAppPassword(?string $stateToken): Response {
$this->getFlowByLoginToken();
} catch (LoginFlowV2NotFoundException $e) {
return $this->loginTokenForbiddenResponse();
} catch (LoginFlowV2ClientForbiddenException $e) {
return $this->loginTokenForbiddenClientResponse();
}

$loginToken = $this->session->get(self::TOKEN_NAME);
Expand Down Expand Up @@ -331,6 +340,7 @@ private function stateTokenForbiddenResponse(): StandaloneTemplateResponse {
/**
* @return LoginFlowV2
* @throws LoginFlowV2NotFoundException
* @throws LoginFlowV2ClientForbiddenException
*/
private function getFlowByLoginToken(): LoginFlowV2 {
$currentToken = $this->session->get(self::TOKEN_NAME);
Expand All @@ -354,6 +364,19 @@ private function loginTokenForbiddenResponse(): StandaloneTemplateResponse {
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 = '';

Expand Down
12 changes: 12 additions & 0 deletions core/Exception/LoginFlowV2ClientForbiddenException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2025 STRATO GmbH
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\Core\Exception;

class LoginFlowV2ClientForbiddenException extends \Exception {
}
23 changes: 22 additions & 1 deletion core/Service/LoginFlowV2Service.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use OC\Core\Data\LoginFlowV2Tokens;
use OC\Core\Db\LoginFlowV2;
use OC\Core\Db\LoginFlowV2Mapper;
use OC\Core\Exception\LoginFlowV2ClientForbiddenException;
use OC\Core\Exception\LoginFlowV2NotFoundException;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Utility\ITimeFactory;
Expand Down Expand Up @@ -74,13 +75,33 @@ public function poll(string $pollToken): LoginFlowV2Credentials {
* @param string $loginToken
* @return LoginFlowV2
* @throws LoginFlowV2NotFoundException
* @throws LoginFlowV2ClientForbiddenException
*/
public function getByLoginToken(string $loginToken): LoginFlowV2 {
/** @var LoginFlowV2|null $flow */
$flow = null;

try {
return $this->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', []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should be an app or system config value.
But if this should be, as it is now, a system config value then please add it with documentation to config/config.sample.php

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would say system config is good once documented 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


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');
}

/**
Expand Down
69 changes: 69 additions & 0 deletions tests/Core/Controller/ClientFlowLoginV2ControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
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;
Expand Down Expand Up @@ -56,6 +57,12 @@ protected function setUp(): void {
$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,
Expand Down Expand Up @@ -150,6 +157,22 @@ public function testShowAuthPickerInvalidLoginToken(): void {
$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')
Expand Down Expand Up @@ -206,6 +229,29 @@ public function testGrantPageInvalidLoginToken(): void {
$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) {
Expand Down Expand Up @@ -266,6 +312,29 @@ public function testGenerateAppPassworInvalidLoginToken(): void {
$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) {
Expand Down
52 changes: 52 additions & 0 deletions tests/Core/Service/LoginFlowV2ServiceUnitTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use OC\Core\Data\LoginFlowV2Tokens;
use OC\Core\Db\LoginFlowV2;
use OC\Core\Db\LoginFlowV2Mapper;
use OC\Core\Exception\LoginFlowV2ClientForbiddenException;
use OC\Core\Exception\LoginFlowV2NotFoundException;
use OC\Core\Service\LoginFlowV2Service;
use OCP\AppFramework\Db\DoesNotExistException;
Expand Down Expand Up @@ -237,6 +238,57 @@ public function testGetByLoginTokenLoginTokenInvalid(): void {
$this->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
*/
Expand Down
Loading