diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index 77bb252206a18..db233e7956ee7 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', '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); } } diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index 3080685457455..afa5aae4f0739 100644 --- 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 @@ 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 +52,9 @@ protected function setUp(): void { $this->urlGenerator, $this->clientMapper, $this->session, - $this->l + $this->l, + $this->random, + $this->appConfig, ); } @@ -80,6 +85,53 @@ public function testAuthorize(): void { $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'); diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 93eec8921fe56..8d84ac100fac0 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\IL10N; @@ -157,9 +157,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(); } @@ -203,14 +205,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(); diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 7f9f11db7e379..1f4575208b805 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -1,4 +1,7 @@ accessTokenMapper, $this->crypto, $this->eventDispatcher, - $this->timeFactory + $this->timeFactory, ); } 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(