Merge pull request #49670 from nextcloud/feat/allow-oauth-grant-bypass

feat(oauth): Allow to skip grant step for selected applications
2026-06-08 16:26:59 -04:00 · 2025-01-07 16:46:30 +01:00 · 2025-01-07 16:46:30 +01:00 · 1b0cb4a56c
commit 1b0cb4a56c
parent c9757586cd 75f8bb51ed
5 changed files with 123 additions and 59 deletions
--- a/apps/oauth2/lib/Controller/LoginRedirectorController.php
+++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php
@ -8,6 +8,7 @@ declare(strict_types=1);
 */
 namespace OCA\OAuth2\Controller;

+use OC\Core\Controller\ClientFlowLoginController;
 use OCA\OAuth2\Db\ClientMapper;
 use OCA\OAuth2\Exceptions\ClientNotFoundException;
 use OCP\AppFramework\Controller;
@ -18,10 +19,12 @@ use OCP\AppFramework\Http\Attribute\PublicPage;
 use OCP\AppFramework\Http\Attribute\UseSession;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IAppConfig;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
+use OCP\Security\ISecureRandom;

 #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
 class LoginRedirectorController extends Controller {
@ -40,6 +43,8 @@ class LoginRedirectorController extends Controller {
 		private ClientMapper $clientMapper,
 		private ISession $session,
 		private IL10N $l,
+		private ISecureRandom $random,
+		private IAppConfig $appConfig,
 	) {
 		parent::__construct($appName, $request);
 	}
@ -78,12 +83,28 @@ class LoginRedirectorController extends Controller {

 		$this->session->set('oauth.state', $state);

-		$targetUrl = $this->urlGenerator->linkToRouteAbsolute(
-			'core.ClientFlowLogin.showAuthPickerPage',
-			[
-				'clientIdentifier' => $client->getClientIdentifier(),
-			]
-		);
+		if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) {
+			/** @see ClientFlowLoginController::showAuthPickerPage **/
+			$stateToken = $this->random->generate(
+				64,
+				ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_DIGITS
+			);
+			$this->session->set(ClientFlowLoginController::STATE_NAME, $stateToken);
+			$targetUrl = $this->urlGenerator->linkToRouteAbsolute(
+				'core.ClientFlowLogin.grantPage',
+				[
+					'stateToken' => $stateToken,
+					'clientIdentifier' => $client->getClientIdentifier(),
+				]
+			);
+		} else {
+			$targetUrl = $this->urlGenerator->linkToRouteAbsolute(
+				'core.ClientFlowLogin.showAuthPickerPage',
+				[
+					'clientIdentifier' => $client->getClientIdentifier(),
+				]
+			);
+		}
 		return new RedirectResponse($targetUrl);
 	}
 }
--- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php
+++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php
@ -5,34 +5,35 @@
 */
 namespace OCA\OAuth2\Tests\Controller;

+use OC\Core\Controller\ClientFlowLoginController;
 use OCA\OAuth2\Controller\LoginRedirectorController;
 use OCA\OAuth2\Db\Client;
 use OCA\OAuth2\Db\ClientMapper;
 use OCA\OAuth2\Exceptions\ClientNotFoundException;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\IAppConfig;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\IURLGenerator;
+use OCP\Security\ISecureRandom;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;

 /**
 * @group DB
 */
 class LoginRedirectorControllerTest extends TestCase {
-	/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
-	private $request;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
-	private $urlGenerator;
-	/** @var ClientMapper|\PHPUnit\Framework\MockObject\MockObject */
-	private $clientMapper;
-	/** @var ISession|\PHPUnit\Framework\MockObject\MockObject */
-	private $session;
-	/** @var LoginRedirectorController */
-	private $loginRedirectorController;
-	/** @var IL10N */
-	private $l;
+	private IRequest&MockObject $request;
+	private IURLGenerator&MockObject $urlGenerator;
+	private ClientMapper&MockObject $clientMapper;
+	private ISession&MockObject $session;
+	private IL10N&MockObject $l;
+	private ISecureRandom&MockObject $random;
+	private IAppConfig&MockObject $appConfig;
+
+	private LoginRedirectorController $loginRedirectorController;

 	protected function setUp(): void {
 		parent::setUp();
@ -42,6 +43,8 @@ class LoginRedirectorControllerTest extends TestCase {
 		$this->clientMapper = $this->createMock(ClientMapper::class);
 		$this->session = $this->createMock(ISession::class);
 		$this->l = $this->createMock(IL10N::class);
+		$this->random = $this->createMock(ISecureRandom::class);
+		$this->appConfig = $this->createMock(IAppConfig::class);

 		$this->loginRedirectorController = new LoginRedirectorController(
 			'oauth2',
@ -49,7 +52,9 @@ class LoginRedirectorControllerTest extends TestCase {
 			$this->urlGenerator,
 			$this->clientMapper,
 			$this->session,
-			$this->l
+			$this->l,
+			$this->random,
+			$this->appConfig,
 		);
 	}

@ -80,6 +85,53 @@ class LoginRedirectorControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code'));
 	}

+	public function testAuthorizeSkipPicker(): void {
+		$client = new Client();
+		$client->setName('MyClientName');
+		$client->setClientIdentifier('MyClientIdentifier');
+		$this->clientMapper
+			->expects($this->once())
+			->method('getByIdentifier')
+			->with('MyClientId')
+			->willReturn($client);
+		$this->session
+			->expects(static::exactly(2))
+			->method('set')
+			->willReturnCallback(function (string $key, string $value): void {
+				switch ([$key, $value]) {
+					case ['oauth.state', 'MyState']:
+					case [ClientFlowLoginController::STATE_NAME, 'MyStateToken']:
+						/* Expected */
+						break;
+					default:
+						throw new LogicException();
+				}
+			});
+		$this->appConfig
+			->expects(static::once())
+			->method('getValueArray')
+			->with('oauth2', 'skipAuthPickerApplications', [])
+			->willReturn(['MyClientName']);
+		$this->random
+			->expects(static::once())
+			->method('generate')
+			->willReturn('MyStateToken');
+		$this->urlGenerator
+			->expects($this->once())
+			->method('linkToRouteAbsolute')
+			->with(
+				'core.ClientFlowLogin.grantPage',
+				[
+					'stateToken' => 'MyStateToken',
+					'clientIdentifier' => 'MyClientIdentifier',
+				]
+			)
+			->willReturn('https://example.com/?clientIdentifier=foo');
+
+		$expected = new RedirectResponse('https://example.com/?clientIdentifier=foo');
+		$this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code'));
+	}
+
 	public function testAuthorizeWrongResponseType(): void {
 		$client = new Client();
 		$client->setClientIdentifier('MyClientIdentifier');
--- a/core/Controller/ClientFlowLoginController.php
+++ b/core/Controller/ClientFlowLoginController.php
@ -8,7 +8,6 @@ namespace OC\Core\Controller;
 use OC\Authentication\Events\AppPasswordCreatedEvent;
 use OC\Authentication\Exceptions\PasswordlessTokenException;
 use OC\Authentication\Token\IProvider;
-use OC\Authentication\Token\IToken;
 use OCA\OAuth2\Db\AccessToken;
 use OCA\OAuth2\Db\AccessTokenMapper;
 use OCA\OAuth2\Db\ClientMapper;
@ -24,6 +23,7 @@ use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http\StandaloneTemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\Authentication\Exceptions\InvalidTokenException;
+use OCP\Authentication\Token\IToken;
 use OCP\Defaults;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\IL10N;
@ -157,9 +157,11 @@ class ClientFlowLoginController extends Controller {
 	#[NoCSRFRequired]
 	#[UseSession]
 	#[FrontpageRoute(verb: 'GET', url: '/login/flow/grant')]
-	public function grantPage(string $stateToken = '',
+	public function grantPage(
+		string $stateToken = '',
 		string $clientIdentifier = '',
-		int $direct = 0): StandaloneTemplateResponse {
+		int $direct = 0,
+	): Response {
 		if (!$this->isValidToken($stateToken)) {
 			return $this->stateTokenForbiddenResponse();
 		}
@ -203,14 +205,13 @@ class ClientFlowLoginController extends Controller {
 		return $response;
 	}

-	/**
-	 * @return Http\RedirectResponse|Response
-	 */
 	#[NoAdminRequired]
 	#[UseSession]
 	#[FrontpageRoute(verb: 'POST', url: '/login/flow')]
-	public function generateAppPassword(string $stateToken,
-		string $clientIdentifier = '') {
+	public function generateAppPassword(
+		string $stateToken,
+		string $clientIdentifier = '',
+	): Response {
 		if (!$this->isValidToken($stateToken)) {
 			$this->session->remove(self::STATE_NAME);
 			return $this->stateTokenForbiddenResponse();
--- a/tests/Core/Controller/ClientFlowLoginControllerTest.php
+++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php
@ -1,4 +1,7 @@
 <?php
+
+declare(strict_types=1);
+
 /**
 * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
 * SPDX-License-Identifier: AGPL-3.0-or-later
@ -28,38 +31,25 @@ use OCP\IUserSession;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 use OCP\Session\Exceptions\SessionNotAvailableException;
+use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;

 class ClientFlowLoginControllerTest extends TestCase {
-	/** @var IRequest|\PHPUnit\Framework\MockObject\MockObject */
-	private $request;
-	/** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */
-	private $userSession;
-	/** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */
-	private $l10n;
-	/** @var Defaults|\PHPUnit\Framework\MockObject\MockObject */
-	private $defaults;
-	/** @var ISession|\PHPUnit\Framework\MockObject\MockObject */
-	private $session;
-	/** @var IProvider|\PHPUnit\Framework\MockObject\MockObject */
-	private $tokenProvider;
-	/** @var ISecureRandom|\PHPUnit\Framework\MockObject\MockObject */
-	private $random;
-	/** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */
-	private $urlGenerator;
-	/** @var ClientMapper|\PHPUnit\Framework\MockObject\MockObject */
-	private $clientMapper;
-	/** @var AccessTokenMapper|\PHPUnit\Framework\MockObject\MockObject */
-	private $accessTokenMapper;
-	/** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */
-	private $crypto;
-	/** @var IEventDispatcher|\PHPUnit\Framework\MockObject\MockObject */
-	private $eventDispatcher;
-	/** @var ITimeFactory|\PHPUnit\Framework\MockObject\MockObject */
-	private $timeFactory;
+	private IRequest&MockObject $request;
+	private IUserSession&MockObject $userSession;
+	private IL10N&MockObject $l10n;
+	private Defaults&MockObject $defaults;
+	private ISession&MockObject $session;
+	private IProvider&MockObject $tokenProvider;
+	private ISecureRandom&MockObject $random;
+	private IURLGenerator&MockObject $urlGenerator;
+	private ClientMapper&MockObject $clientMapper;
+	private AccessTokenMapper&MockObject $accessTokenMapper;
+	private ICrypto&MockObject $crypto;
+	private IEventDispatcher&MockObject $eventDispatcher;
+	private ITimeFactory&MockObject $timeFactory;

-	/** @var ClientFlowLoginController */
-	private $clientFlowLoginController;
+	private ClientFlowLoginController $clientFlowLoginController;

 	protected function setUp(): void {
 		parent::setUp();
@ -98,7 +88,7 @@ class ClientFlowLoginControllerTest extends TestCase {
 			$this->accessTokenMapper,
 			$this->crypto,
 			$this->eventDispatcher,
-			$this->timeFactory
+			$this->timeFactory,
 		);
 	}

--- a/tests/lib/Config/UserConfigTest.php
+++ b/tests/lib/Config/UserConfigTest.php
@ -746,9 +746,9 @@ class UserConfigTest extends TestCase {
 	public function testSearchValuesByUsers(
 		string $app,
 		string $key,
-		?ValueType $typedAs = null,
-		?array $userIds = null,
-		array $result = [],
+		?ValueType $typedAs,
+		?array $userIds,
+		array $result,
 	): void {
 		$userConfig = $this->generateUserConfig();
 		$this->assertEqualsCanonicalizing(