diff --git a/apps/oauth2/appinfo/info.xml b/apps/oauth2/appinfo/info.xml index 8aff11d4c6824..bd3605c2e5f4a 100644 --- a/apps/oauth2/appinfo/info.xml +++ b/apps/oauth2/appinfo/info.xml @@ -9,7 +9,7 @@ OAuth 2.0 Allows OAuth2 compatible authentication from other web applications. The OAuth2 app allows administrators to configure the built-in authentication workflow to also allow OAuth2 compatible authentication from other web applications. - 1.22.0 + 1.23.0 agpl Lukas Reschke OAuth2 diff --git a/apps/oauth2/composer/composer/autoload_classmap.php b/apps/oauth2/composer/composer/autoload_classmap.php index f5fc5bfe2519b..e2eb96f3c1624 100644 --- a/apps/oauth2/composer/composer/autoload_classmap.php +++ b/apps/oauth2/composer/composer/autoload_classmap.php @@ -25,5 +25,6 @@ 'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => $baseDir . '/../lib/Migration/Version011602Date20230613160650.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => $baseDir . '/../lib/Migration/Version011603Date20230620111039.php', 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => $baseDir . '/../lib/Migration/Version011901Date20240829164356.php', + 'OCA\\OAuth2\\Migration\\Version012300Date20250423141506' => $baseDir . '/../lib/Migration/Version012300Date20250423141506.php', 'OCA\\OAuth2\\Settings\\Admin' => $baseDir . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/composer/composer/autoload_static.php b/apps/oauth2/composer/composer/autoload_static.php index 0ebf5a483bab4..ccc126522d32a 100644 --- a/apps/oauth2/composer/composer/autoload_static.php +++ b/apps/oauth2/composer/composer/autoload_static.php @@ -40,6 +40,7 @@ class ComposerStaticInitOAuth2 'OCA\\OAuth2\\Migration\\Version011602Date20230613160650' => __DIR__ . '/..' . '/../lib/Migration/Version011602Date20230613160650.php', 'OCA\\OAuth2\\Migration\\Version011603Date20230620111039' => __DIR__ . '/..' . '/../lib/Migration/Version011603Date20230620111039.php', 'OCA\\OAuth2\\Migration\\Version011901Date20240829164356' => __DIR__ . '/..' . '/../lib/Migration/Version011901Date20240829164356.php', + 'OCA\\OAuth2\\Migration\\Version012300Date20250423141506' => __DIR__ . '/..' . '/../lib/Migration/Version012300Date20250423141506.php', 'OCA\\OAuth2\\Settings\\Admin' => __DIR__ . '/..' . '/../lib/Settings/Admin.php', ); diff --git a/apps/oauth2/lib/Controller/LoginRedirectorController.php b/apps/oauth2/lib/Controller/LoginRedirectorController.php index b41e3ebdbf891..b4578d28f9795 100644 --- a/apps/oauth2/lib/Controller/LoginRedirectorController.php +++ b/apps/oauth2/lib/Controller/LoginRedirectorController.php @@ -29,6 +29,9 @@ #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] class LoginRedirectorController extends Controller { + private const PKCE_STRING_PATTERN = '/^[A-Za-z0-9._~-]{43,128}$/'; + private const LEGACY_LOCALHOST_REDIRECT_PATTERN = '/^http:\/\/localhost:[0-9]+$/'; + /** * @param string $appName * @param IRequest $request @@ -51,6 +54,41 @@ public function __construct( parent::__construct($appName, $request); } + /** + * @return RedirectResponse + */ + private function buildErrorRedirectResponse( + string $registeredRedirectUri, + string $providedRedirectUri, + string $error, + string $state, + ?string $errorDescription = null, + ): RedirectResponse { + $redirectUri = $registeredRedirectUri; + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); + if ($enableOcClients && $redirectUri === 'http://localhost:*' && preg_match(self::LEGACY_LOCALHOST_REDIRECT_PATTERN, $providedRedirectUri) === 1) { + $redirectUri = $providedRedirectUri; + } + + $fragment = ''; + $fragmentPosition = strpos($redirectUri, '#'); + if ($fragmentPosition !== false) { + $fragment = substr($redirectUri, $fragmentPosition); + $redirectUri = substr($redirectUri, 0, $fragmentPosition); + } + + $params = [ + 'error' => $error, + ]; + if ($errorDescription !== null) { + $params['error_description'] = $errorDescription; + } + $params['state'] = $state; + + $separator = str_contains($redirectUri, '?') ? '&' : '?'; + return new RedirectResponse($redirectUri . $separator . http_build_query($params) . $fragment); + } + /** * Authorize the user * @@ -58,6 +96,8 @@ public function __construct( * @param string $state State of the flow * @param string $response_type Response type for the flow * @param string $redirect_uri URI to redirect to after the flow (is only used for legacy ownCloud clients) + * @param string $code_challenge PKCE code challenge (optional, RFC 7636 format) + * @param string $code_challenge_method PKCE code challenge method. Only "S256" is supported. If omitted, RFC 7636 defaults to "plain", which is rejected. * @return TemplateResponse|RedirectResponse * * 200: Client not found @@ -69,7 +109,9 @@ public function __construct( public function authorize($client_id, $state, $response_type, - string $redirect_uri = ''): TemplateResponse|RedirectResponse { + string $redirect_uri = '', + string $code_challenge = '', + string $code_challenge_method = ''): TemplateResponse|RedirectResponse { try { $client = $this->clientMapper->getByIdentifier($client_id); } catch (ClientNotFoundException $e) { @@ -80,9 +122,22 @@ public function authorize($client_id, } if ($response_type !== 'code') { - //Fail - $url = $client->getRedirectUri() . '?error=unsupported_response_type&state=' . \urlencode($state); - return new RedirectResponse($url); + return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'unsupported_response_type', $state); + } + + if ($code_challenge === '' && $code_challenge_method !== '') { + return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'code_challenge required'); + } + + if ($code_challenge !== '' && preg_match(self::PKCE_STRING_PATTERN, $code_challenge) !== 1) { + return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'Invalid code_challenge'); + } + + $effectiveCodeChallengeMethod = $code_challenge_method === '' && $code_challenge !== '' + ? 'plain' + : $code_challenge_method; + if ($effectiveCodeChallengeMethod !== '' && $effectiveCodeChallengeMethod !== 'S256') { + return $this->buildErrorRedirectResponse($client->getRedirectUri(), $redirect_uri, 'invalid_request', $state, 'Transform algorithm not supported'); } $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); @@ -93,6 +148,8 @@ public function authorize($client_id, } $this->session->set('oauth.state', $state); + $this->session->set('oauth.code_challenge', $code_challenge); + $this->session->set('oauth.code_challenge_method', $effectiveCodeChallengeMethod); if (in_array($client->getName(), $this->appConfig->getValueArray('oauth2', 'skipAuthPickerApplications', []))) { /** @see ClientFlowLoginController::showAuthPickerPage **/ diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index b194674dfacb7..d03879d6d9700 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -35,6 +35,7 @@ class OauthApiController extends Controller { // the authorization code expires after 10 minutes public const AUTHORIZATION_CODE_EXPIRES_AFTER = 10 * 60; + private const PKCE_STRING_PATTERN = '/^[A-Za-z0-9._~-]{43,128}$/'; public function __construct( string $appName, @@ -61,6 +62,7 @@ public function __construct( * @param ?string $refresh_token Refresh token * @param ?string $client_id Client ID * @param ?string $client_secret Client secret + * @param ?string $code_verifier PKCE code verifier in RFC 7636 format (required if code_challenge was provided) * @throws Exception * @return JSONResponse|JSONResponse * @@ -73,6 +75,7 @@ public function __construct( public function getToken( string $grant_type, ?string $code, ?string $refresh_token, ?string $client_id, ?string $client_secret, + ?string $code_verifier = null, ): JSONResponse { // We only handle two types @@ -124,6 +127,44 @@ public function getToken( $response->throttle(['invalid_request' => 'authorization_code_expired', 'expired_since' => $expiredSince]); return $response; } + + // verify PKCE code_verifier if code_challenge was provided + $hashedCodeChallenge = $accessToken->getHashedCodeChallenge(); + if ($hashedCodeChallenge !== null && $hashedCodeChallenge !== '') { + if ($code_verifier === null || $code_verifier === '') { + $response = new JSONResponse([ + 'error' => 'invalid_request', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_request' => 'code_verifier_required']); + return $response; + } + + if (preg_match(self::PKCE_STRING_PATTERN, $code_verifier) !== 1) { + $response = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_grant' => 'invalid_code_verifier']); + return $response; + } + + $codeChallengeMethod = $accessToken->getCodeChallengeMethod(); + if ($codeChallengeMethod !== 'S256') { + $response = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_grant' => 'unsupported_code_challenge_method']); + return $response; + } + + $expectedHash = rtrim(strtr(base64_encode(hash('sha256', $code_verifier, true)), '+/', '-_'), '='); + if (!hash_equals($hashedCodeChallenge, $expectedHash)) { + $response = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $response->throttle(['invalid_grant' => 'pkce_verification_failed']); + return $response; + } + } } try { diff --git a/apps/oauth2/lib/Db/AccessToken.php b/apps/oauth2/lib/Db/AccessToken.php index 2027371892588..bb9f99d19463f 100644 --- a/apps/oauth2/lib/Db/AccessToken.php +++ b/apps/oauth2/lib/Db/AccessToken.php @@ -24,6 +24,10 @@ * @method void setCodeCreatedAt(int $createdAt) * @method int getTokenCount() * @method void setTokenCount(int $tokenCount) + * @method string|null getHashedCodeChallenge() + * @method void setHashedCodeChallenge(?string $hashedCodeChallenge) + * @method string|null getCodeChallengeMethod() + * @method void setCodeChallengeMethod(?string $codeChallengeMethod) */ class AccessToken extends Entity { /** @var int */ @@ -38,14 +42,20 @@ class AccessToken extends Entity { protected $codeCreatedAt; /** @var int */ protected $tokenCount; + /** @var string|null */ + protected $hashedCodeChallenge; + /** @var string|null */ + protected $codeChallengeMethod; public function __construct() { $this->addType('id', Types::INTEGER); $this->addType('tokenId', Types::INTEGER); $this->addType('clientId', Types::INTEGER); - $this->addType('hashedCode', 'string'); - $this->addType('encryptedToken', 'string'); + $this->addType('hashedCode', Types::STRING); + $this->addType('encryptedToken', Types::STRING); $this->addType('codeCreatedAt', Types::INTEGER); $this->addType('tokenCount', Types::INTEGER); + $this->addType('hashedCodeChallenge', Types::STRING); + $this->addType('codeChallengeMethod', Types::STRING); } } diff --git a/apps/oauth2/lib/Migration/Version012300Date20250423141506.php b/apps/oauth2/lib/Migration/Version012300Date20250423141506.php new file mode 100644 index 0000000000000..02ffdf1c06dcd --- /dev/null +++ b/apps/oauth2/lib/Migration/Version012300Date20250423141506.php @@ -0,0 +1,51 @@ +hasTable('oauth2_access_tokens')) { + $table = $schema->getTable('oauth2_access_tokens'); + $dbChanged = false; + if (!$table->hasColumn('hashed_code_challenge')) { + $table->addColumn('hashed_code_challenge', Types::STRING, [ + 'notnull' => false, + 'length' => 128, + ]); + $dbChanged = true; + } + if (!$table->hasColumn('code_challenge_method')) { + $table->addColumn('code_challenge_method', Types::STRING, [ + 'notnull' => false, + 'length' => 10, + ]); + $dbChanged = true; + } + if ($dbChanged) { + return $schema; + } + } + + return null; + } +} diff --git a/apps/oauth2/openapi.json b/apps/oauth2/openapi.json index 0ec1170a7e2bd..c36013c02623f 100644 --- a/apps/oauth2/openapi.json +++ b/apps/oauth2/openapi.json @@ -74,6 +74,24 @@ "type": "string", "default": "" } + }, + { + "name": "code_challenge", + "in": "query", + "description": "PKCE code challenge (optional, RFC 7636 format)", + "schema": { + "type": "string", + "default": "" + } + }, + { + "name": "code_challenge_method", + "in": "query", + "description": "PKCE code challenge method. Only \"S256\" is supported. If omitted, RFC 7636 defaults to \"plain\", which is rejected.", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { @@ -153,6 +171,12 @@ "type": "string", "nullable": true, "description": "Client secret" + }, + "code_verifier": { + "type": "string", + "nullable": true, + "default": null, + "description": "PKCE code verifier in RFC 7636 format (required if code_challenge was provided)" } } } diff --git a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php index 673c95ebcde9a..0e3d1b53608b6 100644 --- a/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php +++ b/apps/oauth2/tests/Controller/LoginRedirectorControllerTest.php @@ -70,9 +70,18 @@ public function testAuthorize(): void { ->with('MyClientId') ->willReturn($client); $this->session - ->expects($this->once()) + ->expects($this->exactly(3)) ->method('set') - ->with('oauth.state', 'MyState'); + ->willReturnCallback(function (string $key, string $value): void { + switch ([$key, $value]) { + case ['oauth.state', 'MyState']: + case ['oauth.code_challenge', '']: + case ['oauth.code_challenge_method', '']: + break; + default: + throw new LogicException(); + } + }); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -104,11 +113,13 @@ public function testAuthorizeSkipPicker(): void { ->with('MyClientId') ->willReturn($client); $this->session - ->expects(static::exactly(2)) + ->expects(static::exactly(4)) ->method('set') ->willReturnCallback(function (string $key, string $value): void { switch ([$key, $value]) { case ['oauth.state', 'MyState']: + case ['oauth.code_challenge', '']: + case ['oauth.code_challenge_method', '']: case [ClientFlowLoginController::STATE_NAME, 'MyStateToken']: /* Expected */ break; @@ -165,7 +176,69 @@ public function testAuthorizeWrongResponseType(): void { $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'wrongcode')); } - public function testAuthorizeWithLegacyOcClient(): void { + public function testAuthorizeWrongResponseTypePreservesExistingQuery(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://foo.bar?hello=world'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->never()) + ->method('set'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + + $expected = new RedirectResponse('http://foo.bar?hello=world&error=unsupported_response_type&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'wrongcode')); + } + + public function testAuthorizeRejectsCodeChallengeMethodWithoutChallenge(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://foo.bar'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->never()) + ->method('set'); + + $expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=code_challenge+required&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', '', 'S256')); + } + + public function testAuthorizeRejectsPkceWithoutMethodBecausePlainIsUnsupported(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://foo.bar'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->never()) + ->method('set'); + + $codeChallenge = str_repeat('a', 43); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + $expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge)); + } + + public function testAuthorizeRejectsPkceWithoutMethodForLegacyOcClientUsingProvidedRedirectUri(): void { $client = new Client(); $client->setClientIdentifier('MyClientIdentifier'); $client->setRedirectUri('http://localhost:*'); @@ -175,9 +248,86 @@ public function testAuthorizeWithLegacyOcClient(): void { ->with('MyClientId') ->willReturn($client); $this->session + ->expects($this->never()) + ->method('set'); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(true); + + $codeChallenge = str_repeat('a', 43); + $expected = new RedirectResponse('http://localhost:30000?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', 'http://localhost:30000', $codeChallenge)); + } + + public function testAuthorizeRejectsUnsupportedCodeChallengeMethod(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://foo.bar'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->never()) + ->method('set'); + + $codeChallenge = str_repeat('a', 43); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + $expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Transform+algorithm+not+supported&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', $codeChallenge, 'plain')); + } + + public function testAuthorizeRejectsInvalidCodeChallengeFormat(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://foo.bar'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->never()) + ->method('set'); + $this->config ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + + $expected = new RedirectResponse('http://foo.bar?error=invalid_request&error_description=Invalid+code_challenge&state=MyState'); + $this->assertEquals($expected, $this->loginRedirectorController->authorize('MyClientId', 'MyState', 'code', '', 'short', 'S256')); + } + + public function testAuthorizeWithLegacyOcClient(): void { + $client = new Client(); + $client->setClientIdentifier('MyClientIdentifier'); + $client->setRedirectUri('http://localhost:*'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientId') + ->willReturn($client); + $this->session + ->expects($this->exactly(3)) ->method('set') - ->with('oauth.state', 'MyState'); + ->willReturnCallback(function (string $key, string $value): void { + switch ([$key, $value]) { + case ['oauth.state', 'MyState']: + case ['oauth.code_challenge', '']: + case ['oauth.code_challenge_method', '']: + break; + default: + throw new LogicException(); + } + }); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -208,9 +358,18 @@ public function testAuthorizeNotForwardingUntrustedURIs(): void { ->with('MyClientId') ->willReturn($client); $this->session - ->expects($this->once()) + ->expects($this->exactly(3)) ->method('set') - ->with('oauth.state', 'MyState'); + ->willReturnCallback(function (string $key, string $value): void { + switch ([$key, $value]) { + case ['oauth.state', 'MyState']: + case ['oauth.code_challenge', '']: + case ['oauth.code_challenge_method', '']: + break; + default: + throw new LogicException(); + } + }); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 5ef86626cde2c..34e92c4a4277e 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -195,6 +195,216 @@ public function testGetTokenClientDoesNotExist(): void { $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, null, null)); } + public function testGetTokenPkceRequiresVerifier(): void { + $expected = new JSONResponse([ + 'error' => 'invalid_request', + ], Http::STATUS_BAD_REQUEST); + $expected->throttle(['invalid_request' => 'code_verifier_required']); + $codeChallenge = str_repeat('a', 43); + + $accessToken = new AccessToken(); + $accessToken->setClientId(42); + $accessToken->setCodeCreatedAt(100); + $accessToken->setHashedCodeChallenge($codeChallenge); + $accessToken->setCodeChallengeMethod('S256'); + + $this->accessTokenMapper->method('getByCode') + ->with('validcode') + ->willReturn($accessToken); + + $dateNow = (new \DateTimeImmutable())->setTimestamp(101); + $this->timeFactory->method('now') + ->willReturn($dateNow); + + $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, null, null)); + } + + public function testGetTokenPkceRejectsInvalidVerifierFormat(): void { + $expected = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $expected->throttle(['invalid_grant' => 'invalid_code_verifier']); + $codeChallenge = str_repeat('a', 43); + + $accessToken = new AccessToken(); + $accessToken->setClientId(42); + $accessToken->setCodeCreatedAt(100); + $accessToken->setHashedCodeChallenge($codeChallenge); + $accessToken->setCodeChallengeMethod('S256'); + + $this->accessTokenMapper->method('getByCode') + ->with('validcode') + ->willReturn($accessToken); + + $dateNow = (new \DateTimeImmutable())->setTimestamp(101); + $this->timeFactory->method('now') + ->willReturn($dateNow); + + $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, null, null, 'short')); + } + + public function testGetTokenPkceS256VerifierMismatch(): void { + $expected = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $expected->throttle(['invalid_grant' => 'pkce_verification_failed']); + + $accessToken = new AccessToken(); + $accessToken->setClientId(42); + $accessToken->setCodeCreatedAt(100); + $accessToken->setHashedCodeChallenge('expected-challenge'); + $accessToken->setCodeChallengeMethod('S256'); + + $this->accessTokenMapper->method('getByCode') + ->with('validcode') + ->willReturn($accessToken); + + $dateNow = (new \DateTimeImmutable())->setTimestamp(101); + $this->timeFactory->method('now') + ->willReturn($dateNow); + + $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, null, null, str_repeat('b', 43))); + } + + public function testGetTokenPkceRejectsUnsupportedStoredMethod(): void { + $expected = new JSONResponse([ + 'error' => 'invalid_grant', + ], Http::STATUS_BAD_REQUEST); + $expected->throttle(['invalid_grant' => 'unsupported_code_challenge_method']); + + $accessToken = new AccessToken(); + $accessToken->setClientId(42); + $accessToken->setCodeCreatedAt(100); + $accessToken->setHashedCodeChallenge(str_repeat('a', 43)); + $accessToken->setCodeChallengeMethod('plain'); + + $this->accessTokenMapper->method('getByCode') + ->with('validcode') + ->willReturn($accessToken); + + $dateNow = (new \DateTimeImmutable())->setTimestamp(101); + $this->timeFactory->method('now') + ->willReturn($dateNow); + + $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, null, null, str_repeat('a', 43))); + } + + public function testGetTokenPkceS256VerifierMatch(): void { + $codeVerifier = str_repeat('a', 43); + $codeChallenge = rtrim(strtr(base64_encode(hash('sha256', $codeVerifier, true)), '+/', '-_'), '='); + + $accessToken = new AccessToken(); + $accessToken->setId(21); + $accessToken->setClientId(42); + $accessToken->setTokenId(1337); + $accessToken->setEncryptedToken('encryptedToken'); + $accessToken->setCodeCreatedAt(100); + $accessToken->setHashedCodeChallenge($codeChallenge); + $accessToken->setCodeChallengeMethod('S256'); + + $this->accessTokenMapper->method('getByCode') + ->with('validcode') + ->willReturn($accessToken); + + $dateNow = (new \DateTimeImmutable())->setTimestamp(101); + $this->timeFactory->method('now') + ->willReturn($dateNow); + + $client = new Client(); + $client->setClientIdentifier('clientId'); + $client->setSecret(bin2hex('hashedClientSecret')); + $this->clientMapper->method('getByUid') + ->with(42) + ->willReturn($client); + + $this->db->expects($this->once()) + ->method('beginTransaction'); + + $this->db->expects($this->once()) + ->method('commit'); + + $this->db->expects($this->never()) + ->method('rollBack'); + + $this->tokenProvider->expects($this->never()) + ->method('invalidateToken'); + + $this->crypto + ->method('decrypt') + ->with('encryptedToken', 'validcode') + ->willReturn('decryptedToken'); + + $this->crypto + ->method('calculateHMAC') + ->with('clientSecret') + ->willReturn('hashedClientSecret'); + + $appToken = new PublicKeyToken(); + $appToken->setUid('userId'); + $this->tokenProvider->method('getTokenById') + ->with(1337) + ->willThrowException(new ExpiredTokenException($appToken)); + + $this->secureRandom->method('generate') + ->willReturnCallback(function ($len) { + return 'random' . $len; + }); + + $this->tokenProvider->expects($this->once()) + ->method('rotate') + ->with( + $appToken, + 'decryptedToken', + 'random72' + )->willReturn($appToken); + + $this->time->method('getTime') + ->willReturn(1000); + + $this->tokenProvider->expects($this->once()) + ->method('updateToken') + ->with( + $this->callback(function (PublicKeyToken $token) { + return $token->getExpires() === 4600; + }) + ); + + $this->crypto->method('encrypt') + ->with('random72', 'random128') + ->willReturn('newEncryptedToken'); + + $this->accessTokenMapper->expects($this->once()) + ->method('rotateToken') + ->with( + 21, + 'validcode', + 'random128', + 'newEncryptedToken', + true, + )->willReturn(1); + + $expected = new JSONResponse([ + 'access_token' => 'random72', + 'token_type' => 'Bearer', + 'expires_in' => 3600, + 'refresh_token' => 'random128', + 'user_id' => 'userId', + ]); + + $this->request->method('getRemoteAddress') + ->willReturn('1.2.3.4'); + + $this->throttler->expects($this->once()) + ->method('resetDelay') + ->with( + '1.2.3.4', + 'login', + ['user' => 'userId'] + ); + + $this->assertEquals($expected, $this->oauthApiController->getToken('authorization_code', 'validcode', null, 'clientId', 'clientSecret', $codeVerifier)); + } + public function testRefreshTokenInvalidRefreshToken(): void { $expected = new JSONResponse([ 'error' => 'invalid_request', @@ -300,7 +510,7 @@ public function testRefreshTokenInvalidAppToken(): void { $this->crypto ->method('decrypt') - ->with('encryptedToken') + ->with('encryptedToken', 'validrefresh') ->willReturn('decryptedToken'); $this->crypto @@ -339,7 +549,7 @@ public function testRefreshTokenValidAppToken(): void { $this->crypto ->method('decrypt') - ->with('encryptedToken') + ->with('encryptedToken', 'validrefresh') ->willReturn('decryptedToken'); $this->crypto @@ -449,7 +659,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { $this->crypto ->method('decrypt') - ->with('encryptedToken') + ->with('encryptedToken', 'validrefresh') ->willReturn('decryptedToken'); $this->crypto @@ -562,7 +772,7 @@ public function testRefreshTokenExpiredAppToken(): void { $this->crypto ->method('decrypt') - ->with('encryptedToken') + ->with('encryptedToken', 'validrefresh') ->willReturn('decryptedToken'); $this->crypto @@ -677,7 +887,7 @@ public function testRefreshTokenRedeemedConcurrently(): void { $this->crypto ->method('decrypt') - ->with('encryptedToken') + ->with('encryptedToken', 'validrefresh') ->willReturn('decryptedToken'); $this->crypto diff --git a/core/Controller/ClientFlowLoginController.php b/core/Controller/ClientFlowLoginController.php index 836bba6d8b734..629ad86c59524 100644 --- a/core/Controller/ClientFlowLoginController.php +++ b/core/Controller/ClientFlowLoginController.php @@ -297,8 +297,19 @@ public function generateAppPassword( $accessToken->setHashedCode(hash('sha512', $code)); $accessToken->setTokenId($generatedToken->getId()); $accessToken->setCodeCreatedAt($this->timeFactory->now()->getTimestamp()); + + $codeChallenge = $this->session->get('oauth.code_challenge'); + $codeChallengeMethod = $this->session->get('oauth.code_challenge_method'); + if ($codeChallenge !== null && $codeChallenge !== '') { + $accessToken->setHashedCodeChallenge($codeChallenge); + $accessToken->setCodeChallengeMethod($codeChallengeMethod); + } + $this->accessTokenMapper->insert($accessToken); + $this->session->remove('oauth.code_challenge'); + $this->session->remove('oauth.code_challenge_method'); + $enableOcClients = $this->config->getSystemValueBool('oauth2.enable_oc_clients', false); $redirectUri = $client->getRedirectUri(); @@ -313,6 +324,13 @@ public function generateAppPassword( $redirectUri = $providedRedirectUri; } + $fragment = ''; + $fragmentPosition = strpos($redirectUri, '#'); + if ($fragmentPosition !== false) { + $fragment = substr($redirectUri, $fragmentPosition); + $redirectUri = substr($redirectUri, 0, $fragmentPosition); + } + if (parse_url($redirectUri, PHP_URL_QUERY)) { $redirectUri .= '&'; } else { @@ -324,6 +342,7 @@ public function generateAppPassword( urlencode($this->session->get('oauth.state')), urlencode($code) ); + $redirectUri .= $fragment; $this->session->remove('oauth.state'); } else { $redirectUri = 'nc://login/server:' . $this->getServerPath() . '&user:' . urlencode($loginName) . '&password:' . urlencode($token); diff --git a/openapi.json b/openapi.json index 94522cde73429..8580d2a89abb7 100644 --- a/openapi.json +++ b/openapi.json @@ -27711,6 +27711,24 @@ "type": "string", "default": "" } + }, + { + "name": "code_challenge", + "in": "query", + "description": "PKCE code challenge (optional, RFC 7636 format)", + "schema": { + "type": "string", + "default": "" + } + }, + { + "name": "code_challenge_method", + "in": "query", + "description": "PKCE code challenge method. Only \"S256\" is supported. If omitted, RFC 7636 defaults to \"plain\", which is rejected.", + "schema": { + "type": "string", + "default": "" + } } ], "responses": { @@ -27790,6 +27808,12 @@ "type": "string", "nullable": true, "description": "Client secret" + }, + "code_verifier": { + "type": "string", + "nullable": true, + "default": null, + "description": "PKCE code verifier in RFC 7636 format (required if code_challenge was provided)" } } } diff --git a/tests/Core/Controller/ClientFlowLoginControllerTest.php b/tests/Core/Controller/ClientFlowLoginControllerTest.php index 39d915e04bce9..3d092c27ec7a9 100644 --- a/tests/Core/Controller/ClientFlowLoginControllerTest.php +++ b/tests/Core/Controller/ClientFlowLoginControllerTest.php @@ -14,6 +14,7 @@ use OC\Authentication\Token\IProvider; use OC\Authentication\Token\IToken; use OC\Core\Controller\ClientFlowLoginController; +use OCA\OAuth2\Db\AccessToken; use OCA\OAuth2\Db\AccessTokenMapper; use OCA\OAuth2\Db\Client; use OCA\OAuth2\Db\ClientMapper; @@ -422,6 +423,8 @@ public function testGeneratePasswordWithPassword(): void { * @testWith * ["https://example.com/redirect.php", "https://example.com/redirect.php?state=MyOauthState&code=MyAccessCode"] * ["https://example.com/redirect.php?hello=world", "https://example.com/redirect.php?hello=world&state=MyOauthState&code=MyAccessCode"] + * ["https://example.com/redirect.php#target", "https://example.com/redirect.php?state=MyOauthState&code=MyAccessCode#target"] + * ["https://example.com/redirect.php?hello=world#target", "https://example.com/redirect.php?hello=world&state=MyOauthState&code=MyAccessCode#target"] * */ public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $redirectUrl): void { @@ -433,6 +436,8 @@ public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $re ]); $calls = [ 'client.flow.state.token', + 'oauth.code_challenge', + 'oauth.code_challenge_method', 'oauth.state', ]; $this->session @@ -505,6 +510,116 @@ public function testGeneratePasswordWithPasswordForOauthClient($redirectUri, $re $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken', 'MyClientIdentifier')); } + public function testGeneratePasswordWithPasswordForOauthClientStoresPkceChallenge(): void { + $codeChallenge = str_repeat('a', 43); + $this->session + ->method('get') + ->willReturnMap([ + ['client.flow.state.token', 'MyStateToken'], + ['oauth.state', 'MyOauthState'], + ['oauth.code_challenge', $codeChallenge], + ['oauth.code_challenge_method', 'S256'], + ]); + $calls = [ + 'client.flow.state.token', + 'oauth.code_challenge', + 'oauth.code_challenge_method', + 'oauth.state', + ]; + $this->session + ->method('remove') + ->willReturnCallback(function ($key) use (&$calls): void { + $expected = array_shift($calls); + $this->assertEquals($expected, $key); + }); + $this->session + ->expects($this->once()) + ->method('getId') + ->willReturn('SessionId'); + $myToken = $this->createMock(IToken::class); + $myToken + ->expects($this->once()) + ->method('getLoginName') + ->willReturn('MyLoginName'); + $this->tokenProvider + ->expects($this->once()) + ->method('getToken') + ->with('SessionId') + ->willReturn($myToken); + $this->tokenProvider + ->expects($this->once()) + ->method('getPassword') + ->with($myToken, 'SessionId') + ->willReturn('MyPassword'); + $this->random + ->method('generate') + ->willReturnMap([ + [72, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS, 'MyGeneratedToken'], + [128, ISecureRandom::CHAR_UPPER . ISecureRandom::CHAR_LOWER . ISecureRandom::CHAR_DIGITS, 'MyAccessCode'], + ]); + $user = $this->createMock(IUser::class); + $user + ->expects($this->once()) + ->method('getUID') + ->willReturn('MyUid'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($user); + $token = $this->createMock(IToken::class); + $token->method('getId')->willReturn(123); + $this->tokenProvider + ->expects($this->once()) + ->method('generateToken') + ->with( + 'MyGeneratedToken', + 'MyUid', + 'MyLoginName', + 'MyPassword', + 'My OAuth client', + IToken::PERMANENT_TOKEN, + IToken::DO_NOT_REMEMBER + ) + ->willReturn($token); + $client = new Client(); + $client->setId(42); + $client->setName('My OAuth client'); + $client->setRedirectUri('https://example.com/redirect.php'); + $this->clientMapper + ->expects($this->once()) + ->method('getByIdentifier') + ->with('MyClientIdentifier') + ->willReturn($client); + $this->crypto + ->expects($this->once()) + ->method('encrypt') + ->with('MyGeneratedToken', 'MyAccessCode') + ->willReturn('EncryptedToken'); + $this->timeFactory + ->expects($this->once()) + ->method('now') + ->willReturn((new \DateTimeImmutable())->setTimestamp(100)); + $this->accessTokenMapper + ->expects($this->once()) + ->method('insert') + ->with($this->callback(function (AccessToken $accessToken) { + return $accessToken->getHashedCodeChallenge() === str_repeat('a', 43) + && $accessToken->getCodeChallengeMethod() === 'S256' + && $accessToken->getHashedCode() === hash('sha512', 'MyAccessCode'); + })); + $this->config + ->expects($this->once()) + ->method('getSystemValueBool') + ->with('oauth2.enable_oc_clients', false) + ->willReturn(false); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped'); + + $expected = new RedirectResponse('https://example.com/redirect.php?state=MyOauthState&code=MyAccessCode'); + $this->assertEquals($expected, $this->clientFlowLoginController->generateAppPassword('MyStateToken', 'MyClientIdentifier')); + } + public function testGeneratePasswordWithoutPassword(): void { $this->session ->expects($this->once())