Merge pull request #4804 from BookStackApp/oidc_pkce

Add OIDC PKCE functionality

diff --git a/app/Access/Oidc/OidcOAuthProvider.php b/app/Access/Oidc/OidcOAuthProvider.php
index d2dc829b729ef4e342853e29fe2c87e35f236afe..371bfcecb543e6cdb4e8fbccc6aeb91bdc43cf2c 100644 (file)
--- 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 f1e5b25af1490b9fa2c7c5eed7568d3e5e6ab23d..036c9fc47efcf7ac32f0908c57337743d0f005b5 100644 (file)
--- 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 dbf26f1bd302b39009cf776c5673afaa6812379f..345d1dc780b8a1e218b8c57a83340bc97cb7b9ca 100644 (file)
--- 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' => '[email protected]',
+            '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([