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

Conversation

printminion-co
Copy link
Contributor

@printminion-co printminion-co commented Feb 4, 2025

add config value to config.php:

	"core.login_flow_v2.allowed_user_agents" => [
			'/Custom Foo/i'
	],

or via occ

./occ config:system:set core.login_flow_v2.allowed_user_agents 0  --value '/Custom Foo/i'

Test Allowed client

curl -s -A "Custom Foo Curl Client/1.0" -X POST http://localhost:8080/index.php/login/v2 | jq

click on generated login url.

Test Forbidden client

curl -s -A "Rogue Curl Client/1.0" -X POST http://localhost:8080/index.php/login/v2 | jq

click on generated login url.
observe
Selection_20250204-003

Unitests

  • Run tests with coverage
  • Add test suite phpunit-autotest-core.xml file
phpunit-autotest-core.xml
<?xml version="1.0" encoding="utf-8" ?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
		 bootstrap="bootstrap.php"
		 verbose="true"
		 backupGlobals="false"
		 timeoutForSmallTests="900"
		 timeoutForMediumTests="900"
		 timeoutForLargeTests="900"
		 convertDeprecationsToExceptions="true"
		 xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.6/phpunit.xsd">
	<testsuite name='test LoginFlowV2'>
		<directory suffix=".php">./Core/Controller/ClientFlowLoginV2ControllerTest.php</directory>
		<directory suffix=".php">./Core/Service/LoginFlowV2ServiceUnitTest.php</directory>
	</testsuite>
	<coverage>
		<include>
			<directory suffix=".php">../core/*</directory>
		</include>
		<exclude>
			<directory suffix=".js">../**/</directory>

			<directory suffix=".php">../3rdparty/**/*</directory>
			<directory suffix=".php">../apps/**/*</directory>
			<directory suffix=".php">../apps-custom/**/*</directory>
			<directory suffix=".php">../apps-external/**/*</directory>
			<directory suffix=".php">../custom-npms/**/*</directory>
			<directory suffix=".php">../build</directory>
			<directory suffix=".php">../IONOS/**/*</directory>
			<directory suffix=".php">../lib/composer</directory>
			<directory suffix=".php">../vendor/**/*</directory>
			<directory suffix=".php">../tests</directory>
		</exclude>
	</coverage>
	<listeners>
		<listener class="StartSessionListener" file="startsessionlistener.php" />
	</listeners>
</phpunit>
  • run tests
export XDEBUG_MODE=coverage && phpunit --configuration tests/phpunit-autotest-core.xml --coverage-html coverage
  • observe coverage in ./coverage folder

Checklist

} 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.

@susnux susnux requested a review from come-nc February 4, 2025 17:14
@susnux susnux added this to the Nextcloud 32 milestone Feb 4, 2025
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Not sure either, but apart from that code looks fine.

@come-nc come-nc requested a review from nickvergessen February 6, 2025 16:37
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 <[email protected]>
@printminion-co printminion-co force-pushed the feat/login_flow_v2-user_agents-allow-list branch from b467dcc to 1ce7ca2 Compare February 7, 2025 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants