Oidc: Properly query the UserInfo Endpoint

BooksStack's OIDC Client requests the 'profile' and 'email' scope values
in order to have access to the 'name', 'email', and other claims.  It
looks for these claims in the ID Token that is returned along with the
Access Token.

However, the OIDC-core specification section 5.4 [1] only requires that
the Provider include those claims in the ID Token *if* an Access Token is
not also issued.  If an Access Token is issued, the Provider can leave out
those claims from the ID Token, and the Client is supposed to obtain them
by submitting the Access Token to the UserInfo Endpoint.

So I suppose it's just good luck that the OIDC Providers that BookStack
has been tested with just so happen to also stick those claims in the ID
Token even though they don't have to.  But others (in particular:
https://login.infomaniak.com) don't do so, and require fetching the
UserInfo Endpoint.)

A workaround is currently possible by having the user write a theme with a
ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook that fetches the UserInfo
Endpoint.  This workaround isn't great, for a few reasons:
 1. Asking the user to implement core parts of the OIDC protocol is silly.
 2. The user either needs to re-fetch the .well-known/openid-configuration
    file to discover the endpoint (adding yet another round-trip to each
    login) or hard-code the endpoint, which is fragile.
 3. The hook doesn't receive the HTTP client configuration.

So, have BookStack's OidcService fetch the UserInfo Endpoint and inject
those claims into the ID Token, if a UserInfo Endpoint is defined.
Two points about this:
 - Injecting them into the ID Token's claims is the most obvious approach
   given the current code structure; though I'm not sure it is the best
   approach, perhaps it should instead fetch the user info in
   processAuthorizationResponse() and pass that as an argument to
   processAccessTokenCallback() which would then need a bit of
   restructuring.  But this made sense because it's also how the
   ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE hook works.
 - OIDC *requires* that a UserInfo Endpoint exists, so why bother with
   that "if a UserInfo Endpoint is defined" bit?  Simply out of an
   abundance of caution that there's an existing BookStack user that is
   relying on it not fetching the UserInfo Endpoint in order to work with
   a non-compliant OIDC Provider.

[1]: https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims

diff --git a/.env.example.complete b/.env.example.complete
index e8520a24caec0801b745537701bf4e66b5762924..1242968182a0e22b9a8d9b8fa20b60ddda9ec3df 100644 (file)
--- a/.env.example.complete
+++ b/.env.example.complete
@@ -267,6 +267,7 @@ OIDC_ISSUER_DISCOVER=false
 OIDC_PUBLIC_KEY=null
 OIDC_AUTH_ENDPOINT=null
 OIDC_TOKEN_ENDPOINT=null
 OIDC_PUBLIC_KEY=null
 OIDC_AUTH_ENDPOINT=null
 OIDC_TOKEN_ENDPOINT=null

+OIDC_USERINFO_ENDPOINT=null

 OIDC_ADDITIONAL_SCOPES=null
 OIDC_DUMP_USER_DETAILS=false
 OIDC_USER_TO_GROUPS=false
 OIDC_ADDITIONAL_SCOPES=null
 OIDC_DUMP_USER_DETAILS=false
 OIDC_USER_TO_GROUPS=false

diff --git a/app/Access/Oidc/OidcProviderSettings.php b/app/Access/Oidc/OidcProviderSettings.php
index bea6a523e773987612aefae83aeb903ee0d1ae93..49ccab6f0deec10dd10239aafac32d83bb4496e6 100644 (file)
--- a/app/Access/Oidc/OidcProviderSettings.php
+++ b/app/Access/Oidc/OidcProviderSettings.php
@@ -22,6 +22,7 @@ class OidcProviderSettings
     public ?string $authorizationEndpoint;
     public ?string $tokenEndpoint;
     public ?string $endSessionEndpoint;
     public ?string $authorizationEndpoint;
     public ?string $tokenEndpoint;
     public ?string $endSessionEndpoint;

+    public ?string $userinfoEndpoint;

 
     /**
      * @var string[]|array[]
 
     /**
      * @var string[]|array[]


@@ -128,6 +129,10 @@ class OidcProviderSettings
             $discoveredSettings['tokenEndpoint'] = $result['token_endpoint'];
         }
 
             $discoveredSettings['tokenEndpoint'] = $result['token_endpoint'];
         }
 

+        if (!empty($result['userinfo_endpoint'])) {
+            $discoveredSettings['userinfoEndpoint'] = $result['userinfo_endpoint'];
+        }
+

         if (!empty($result['jwks_uri'])) {
             $keys = $this->loadKeysFromUri($result['jwks_uri'], $httpClient);
             $discoveredSettings['keys'] = $this->filterKeys($keys);
         if (!empty($result['jwks_uri'])) {
             $keys = $this->loadKeysFromUri($result['jwks_uri'], $httpClient);
             $discoveredSettings['keys'] = $this->filterKeys($keys);


@@ -177,7 +182,7 @@ class OidcProviderSettings
      */
     public function arrayForProvider(): array
     {
      */
     public function arrayForProvider(): array
     {

-        $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint'];
+        $settingKeys = ['clientId', 'clientSecret', 'redirectUri', 'authorizationEndpoint', 'tokenEndpoint', 'userinfoEndpoint'];

         $settings = [];
         foreach ($settingKeys as $setting) {
             $settings[$setting] = $this->$setting;
         $settings = [];
         foreach ($settingKeys as $setting) {
             $settings[$setting] = $this->$setting;

diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index f1e5b25af1490b9fa2c7c5eed7568d3e5e6ab23d..244957991953510aa4f647c31d7f829b143c621e 100644 (file)
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -85,6 +85,7 @@ class OidcService
             'authorizationEndpoint' => $config['authorization_endpoint'],
             'tokenEndpoint'         => $config['token_endpoint'],
             'endSessionEndpoint'    => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null,
             'authorizationEndpoint' => $config['authorization_endpoint'],
             'tokenEndpoint'         => $config['token_endpoint'],
             'endSessionEndpoint'    => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null,

+            'userinfoEndpoint'      => $config['userinfo_endpoint'],

         ]);
 
         // Use keys if configured
         ]);
 
         // Use keys if configured


@@ -228,6 +229,17 @@ class OidcService
 
         session()->put("oidc_id_token", $idTokenText);
 
 
         session()->put("oidc_id_token", $idTokenText);
 

+        if (!empty($settings->userinfoEndpoint)) {
+            $provider = $this->getProvider($settings);
+            $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken());
+            $response = $provider->getParsedResponse($request);
+            $claims = $idToken->getAllClaims();
+            foreach ($response as $key => $value) {
+                $claims[$key] = $value;
+            }
+            $idToken->replaceClaims($claims);
+        }
+

         $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [
             'access_token' => $accessToken->getToken(),
             'expires_in' => $accessToken->getExpires(),
         $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [
             'access_token' => $accessToken->getToken(),
             'expires_in' => $accessToken->getExpires(),

diff --git a/app/Config/oidc.php b/app/Config/oidc.php
index 7f8f40d419090af498907ee4bb2b8c59dee6e926..8b5470931d08379431c8f3b420f02aa7356c87b1 100644 (file)
--- a/app/Config/oidc.php
+++ b/app/Config/oidc.php
@@ -35,6 +35,7 @@ return [
     // OAuth2 endpoints.
     'authorization_endpoint' => env('OIDC_AUTH_ENDPOINT', null),
     'token_endpoint'         => env('OIDC_TOKEN_ENDPOINT', null),
     // OAuth2 endpoints.
     'authorization_endpoint' => env('OIDC_AUTH_ENDPOINT', null),
     'token_endpoint'         => env('OIDC_TOKEN_ENDPOINT', null),

+    'userinfo_endpoint'      => env('OIDC_USERINFO_ENDPOINT', null),

 
     // OIDC RP-Initiated Logout endpoint URL.
     // A false value force-disables RP-Initiated Logout.
 
     // OIDC RP-Initiated Logout endpoint URL.
     // A false value force-disables RP-Initiated Logout.

diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index dbf26f1bd302b39009cf776c5673afaa6812379f..f47a201005df627a170f7429cef883229cd9b599 100644 (file)
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -668,11 +668,44 @@ class OidcTest extends TestCase
 
     protected function runLogin($claimOverrides = []): TestResponse
     {
 
     protected function runLogin($claimOverrides = []): TestResponse
     {

+        // These two variables should perhaps be arguments instead of
+        // assuming that they're tied to whether discovery is enabled,
+        // but that's how the tests are written for now.
+        $claimsInIdToken = !config('oidc.discover');
+        $tokenEndpoint = config('oidc.discover')
+                       ? OidcJwtHelper::defaultIssuer() . '/oidc/token'
+                       : 'https://oidc.local/token';
+

         $this->post('/oidc/login');
         $state = session()->get('oidc_state');
         $this->post('/oidc/login');
         $state = session()->get('oidc_state');

-        $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides)]);

 
 

-        return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);
+        $providerResponses = [$this->getMockAuthorizationResponse($claimsInIdToken ? $claimOverrides : [])];
+        if (!$claimsInIdToken) {
+            $providerResponses[] = new Response(200, [
+                'Content-Type'  => 'application/json',
+                'Cache-Control' => 'no-cache, no-store',
+                'Pragma'        => 'no-cache',
+            ], json_encode($claimOverrides));
+        }
+
+        $transactions = $this->mockHttpClient($providerResponses);
+
+        $response = $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);
+
+        if (auth()->check()) {
+            $this->assertEquals($claimsInIdToken ? 1 : 2, $transactions->requestCount());
+            $tokenRequest = $transactions->requestAt(0);
+            $this->assertEquals($tokenEndpoint, (string) $tokenRequest->getUri());
+            $this->assertEquals('POST', $tokenRequest->getMethod());
+            if (!$claimsInIdToken) {
+                $userinfoRequest = $transactions->requestAt(1);
+                $this->assertEquals(OidcJwtHelper::defaultIssuer() . '/oidc/userinfo', (string) $userinfoRequest->getUri());
+                $this->assertEquals('GET', $userinfoRequest->getMethod());
+                $this->assertEquals('Bearer abc123', $userinfoRequest->getHeader('Authorization')[0]);
+            }
+        }
+
+        return $response;

     }
 
     protected function getAutoDiscoveryResponse($responseOverrides = []): Response
     }
 
     protected function getAutoDiscoveryResponse($responseOverrides = []): Response


@@ -684,6 +717,7 @@ class OidcTest extends TestCase
         ], json_encode(array_merge([
             'token_endpoint'         => OidcJwtHelper::defaultIssuer() . '/oidc/token',
             'authorization_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/authorize',
         ], json_encode(array_merge([
             'token_endpoint'         => OidcJwtHelper::defaultIssuer() . '/oidc/token',
             'authorization_endpoint' => OidcJwtHelper::defaultIssuer() . '/oidc/authorize',

+            'userinfo_endpoint'      => OidcJwtHelper::defaultIssuer() . '/oidc/userinfo',

             'jwks_uri'               => OidcJwtHelper::defaultIssuer() . '/oidc/keys',
             'issuer'                 => OidcJwtHelper::defaultIssuer(),
             'end_session_endpoint'   => OidcJwtHelper::defaultIssuer() . '/oidc/logout',
             'jwks_uri'               => OidcJwtHelper::defaultIssuer() . '/oidc/keys',
             'issuer'                 => OidcJwtHelper::defaultIssuer(),
             'end_session_endpoint'   => OidcJwtHelper::defaultIssuer() . '/oidc/logout',