diff --git a/app/Access/Oidc/OidcOAuthProvider.php b/app/Access/Oidc/OidcOAuthProvider.php index d2dc829b7..371bfcecb 100644 --- a/app/Access/Oidc/OidcOAuthProvider.php +++ b/app/Access/Oidc/OidcOAuthProvider.php @@ -83,15 +83,9 @@ class OidcOAuthProvider extends AbstractProvider /** * Checks a provider response for errors. - * - * @param ResponseInterface $response - * @param array|string $data Parsed response data - * * @throws IdentityProviderException - * - * @return void */ - protected function checkResponse(ResponseInterface $response, $data) + protected function checkResponse(ResponseInterface $response, $data): void { if ($response->getStatusCode() >= 400 || isset($data['error'])) { throw new IdentityProviderException( @@ -105,13 +99,8 @@ class OidcOAuthProvider extends AbstractProvider /** * Generates a resource owner object from a successful resource owner * details request. - * - * @param array $response - * @param AccessToken $token - * - * @return ResourceOwnerInterface */ - protected function createResourceOwner(array $response, AccessToken $token) + protected function createResourceOwner(array $response, AccessToken $token): ResourceOwnerInterface { return new GenericResourceOwner($response, ''); } @@ -121,14 +110,18 @@ class OidcOAuthProvider extends AbstractProvider * * The grant that was used to fetch the response can be used to provide * additional context. - * - * @param array $response - * @param AbstractGrant $grant - * - * @return OidcAccessToken */ - protected function createAccessToken(array $response, AbstractGrant $grant) + protected function createAccessToken(array $response, AbstractGrant $grant): OidcAccessToken { return new OidcAccessToken($response); } + + /** + * Get the method used for PKCE code verifier hashing, which is passed + * in the "code_challenge_method" parameter in the authorization request. + */ + protected function getPkceMethod(): string + { + return static::PKCE_METHOD_S256; + } } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index f1e5b25af..036c9fc47 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -33,6 +33,8 @@ class OidcService /** * Initiate an authorization flow. + * Provides back an authorize redirect URL, in addition to other + * details which may be required for the auth flow. * * @throws OidcException * @@ -42,8 +44,12 @@ class OidcService { $settings = $this->getProviderSettings(); $provider = $this->getProvider($settings); + + $url = $provider->getAuthorizationUrl(); + session()->put('oidc_pkce_code', $provider->getPkceCode() ?? ''); + return [ - 'url' => $provider->getAuthorizationUrl(), + 'url' => $url, 'state' => $provider->getState(), ]; } @@ -63,6 +69,10 @@ class OidcService $settings = $this->getProviderSettings(); $provider = $this->getProvider($settings); + // Set PKCE code flashed at login + $pkceCode = session()->pull('oidc_pkce_code', ''); + $provider->setPkceCode($pkceCode); + // Try to exchange authorization code for access token $accessToken = $provider->getAccessToken('authorization_code', [ 'code' => $authorizationCode, diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index dbf26f1bd..345d1dc78 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -655,6 +655,34 @@ class OidcTest extends TestCase ]); } + public function test_pkce_used_on_authorize_and_access() + { + // Start auth + $resp = $this->post('/oidc/login'); + $state = session()->get('oidc_state'); + + $pkceCode = session()->get('oidc_pkce_code'); + $this->assertGreaterThan(30, strlen($pkceCode)); + + $expectedCodeChallenge = trim(strtr(base64_encode(hash('sha256', $pkceCode, true)), '+/', '-_'), '='); + $redirect = $resp->headers->get('Location'); + $redirectParams = []; + parse_str(parse_url($redirect, PHP_URL_QUERY), $redirectParams); + $this->assertEquals($expectedCodeChallenge, $redirectParams['code_challenge']); + $this->assertEquals('S256', $redirectParams['code_challenge_method']); + + $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([ + 'email' => 'benny@example.com', + 'sub' => 'benny1010101', + ])]); + + $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state); + $tokenRequest = $transactions->latestRequest(); + $bodyParams = []; + parse_str($tokenRequest->getBody(), $bodyParams); + $this->assertEquals($pkceCode, $bodyParams['code_verifier']); + } + protected function withAutodiscovery() { config()->set([