From 9b366c65d40320d30ffd0d0c7e9a728394520bee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 5 Dec 2024 15:00:46 +0100 Subject: [PATCH 1/6] feat(oauth): Allow to skip the grant step for selected applications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Controller/ClientFlowLoginController.php | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 93eec8921fe56..66e049616c092 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -26,6 +26,7 @@ use OCP\Authentication\Exceptions\InvalidTokenException; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; +use OCP\IAppConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -55,6 +56,7 @@ public function __construct( private ICrypto $crypto, private IEventDispatcher $eventDispatcher, private ITimeFactory $timeFactory, + private IAppConfig $appConfig, ) { parent::__construct($appName, $request); } @@ -157,9 +159,11 @@ public function showAuthPickerPage(string $clientIdentifier = '', string $user = #[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(); } @@ -181,6 +185,10 @@ public function grantPage(string $stateToken = '', /** @var IUser $user */ $user = $this->userSession->getUser(); + if (in_array($clientName, $this->appConfig->getValueArray('oauth2', 'autoGrantApplications', []))) { + return $this->generateAppPassword($stateToken, $clientIdentifier); + } + $response = new StandaloneTemplateResponse( $this->appName, 'loginflow/grant', @@ -203,14 +211,13 @@ public function grantPage(string $stateToken = '', 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(); From e7be008dc1ee9ef504448d61606b03897b33b660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 9 Dec 2024 16:54:01 +0100 Subject: [PATCH 2/6] feat(oauth2): Skip page before login as well for authorized applications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../Controller/LoginRedirectorController.php | 33 +++++++++++++++---- core/Controller/ClientFlowLoginController.php | 2 +- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index 77bb252206a18..481e3cdab5368 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -8,6 +8,7 @@ */ 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\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 @@ public function __construct( private ClientMapper $clientMapper, private ISession $session, private IL10N $l, + private ISecureRandom $random, + private IAppConfig $appConfig, ) { parent::__construct($appName, $request); } @@ -78,12 +83,28 @@ public function authorize($client_id, $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', 'autoGrantApplications', []))) { + /* 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); } } diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 66e049616c092..76f447f210108 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -8,7 +8,6 @@ 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\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\IAppConfig; From 99e0867f0aaa81ebeae43deaa8ea389419dd9aaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 9 Dec 2024 17:09:55 +0100 Subject: [PATCH 3/6] chore: Adapt tests to added constructor parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../LoginRedirectorControllerTest.php | 30 ++++++----- .../ClientFlowLoginControllerTest.php | 54 +++++++++---------- tests/lib/Config/UserConfigTest.php | 6 +-- 3 files changed, 44 insertions(+), 46 deletions(-) diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index 3080685457455..a2ebb676b4acf 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -11,28 +11,28 @@ 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 +42,8 @@ protected function setUp(): void { $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 +51,9 @@ protected function setUp(): void { $this->urlGenerator, $this->clientMapper, $this->session, - $this->l + $this->l, + $this->random, + $this->appConfig, ); } diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 7f9f11db7e379..3a7acd3afebb8 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -1,4 +1,7 @@ crypto = $this->createMock(ICrypto::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->clientFlowLoginController = new ClientFlowLoginController( 'core', @@ -98,7 +91,8 @@ protected function setUp(): void { $this->accessTokenMapper, $this->crypto, $this->eventDispatcher, - $this->timeFactory + $this->timeFactory, + $this->appConfig, ); } diff --git a/tests/lib/Config/UserConfigTest.php b/tests/lib/Config/UserConfigTest.php index e727116d9a82e..0f2aed4a28860 100644 --- a/tests/lib/Config/UserConfigTest.php +++ b/tests/lib/Config/UserConfigTest.php @@ -746,9 +746,9 @@ public function providerSearchValuesByUsers(): array { 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( From cca0e8448066719279e8076e4b1e2af57fda0d09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 9 Dec 2024 17:50:38 +0100 Subject: [PATCH 4/6] feat(oauth2): Add a test for skipping auth page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../LoginRedirectorControllerTest.php | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index a2ebb676b4acf..adeb05158ed73 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -5,6 +5,7 @@ */ 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; @@ -84,6 +85,53 @@ public function testAuthorize(): void { $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code')); } + public function testAuthorizeSkipGrant(): 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', 'autoGrantApplications', []) + ->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'); From f52b4c5eb244c1a1f33b68b06189299f16fefad5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 19 Dec 2024 10:48:54 +0100 Subject: [PATCH 5/6] fix: Remove skip of grant page, only skip first step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Controller/ClientFlowLoginController.php | 6 ------ tests/Core/Controller/ClientFlowLoginControllerTest.php | 4 ---- 2 files changed, 10 deletions(-) diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 76f447f210108..8d84ac100fac0 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -26,7 +26,6 @@ use OCP\Authentication\Token\IToken; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; -use OCP\IAppConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -56,7 +55,6 @@ public function __construct( private ICrypto $crypto, private IEventDispatcher $eventDispatcher, private ITimeFactory $timeFactory, - private IAppConfig $appConfig, ) { parent::__construct($appName, $request); } @@ -185,10 +183,6 @@ public function grantPage( /** @var IUser $user */ $user = $this->userSession->getUser(); - if (in_array($clientName, $this->appConfig->getValueArray('oauth2', 'autoGrantApplications', []))) { - return $this->generateAppPassword($stateToken, $clientIdentifier); - } - $response = new StandaloneTemplateResponse( $this->appName, 'loginflow/grant', diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 3a7acd3afebb8..1f4575208b805 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -22,7 +22,6 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\EventDispatcher\IEventDispatcher; -use OCP\IAppConfig; use OCP\IL10N; use OCP\IRequest; use OCP\ISession; @@ -49,7 +48,6 @@ class ClientFlowLoginControllerTest extends TestCase { private ICrypto&MockObject $crypto; private IEventDispatcher&MockObject $eventDispatcher; private ITimeFactory&MockObject $timeFactory; - private IAppConfig&MockObject $appConfig; private ClientFlowLoginController $clientFlowLoginController; @@ -75,7 +73,6 @@ protected function setUp(): void { $this->crypto = $this->createMock(ICrypto::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->appConfig = $this->createMock(IAppConfig::class); $this->clientFlowLoginController = new ClientFlowLoginController( 'core', @@ -92,7 +89,6 @@ protected function setUp(): void { $this->crypto, $this->eventDispatcher, $this->timeFactory, - $this->appConfig, ); } From 75f8bb51ed6ed57a79d3c51c51ee6481db3d42b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 19 Dec 2024 10:49:36 +0100 Subject: [PATCH 6/6] fix: Rename config option to skipAuthPickerApplications to match what it does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/oauth2/lib/Controller/LoginRedirectorController.php | 4 ++-- .../oauth2/tests/Controller/LoginRedirectorControllerTest.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index 481e3cdab5368..db233e7956ee7 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -83,8 +83,8 @@ public function authorize($client_id, $this->session->set('oauth.state', $state); - if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'autoGrantApplications', []))) { - /* See ClientFlowLoginController::showAuthPickerPage */ + 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 diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index adeb05158ed73..afa5aae4f0739 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -85,7 +85,7 @@ public function testAuthorize(): void { $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code')); } - public function testAuthorizeSkipGrant(): void { + public function testAuthorizeSkipPicker(): void { $client = new Client(); $client->setName('MyClientName'); $client->setClientIdentifier('MyClientIdentifier'); @@ -110,7 +110,7 @@ public function testAuthorizeSkipGrant(): void { $this->appConfig ->expects(static::once()) ->method('getValueArray') - ->with('oauth2', 'autoGrantApplications', []) + ->with('oauth2', 'skipAuthPickerApplications', []) ->willReturn(['MyClientName']); $this->random ->expects(static::once())