From c76d12d1ded72b5bf74a51fdb1647f87bb935edc Mon Sep 17 00:00:00 2001 From: "Luke T. Shumaker" Date: Fri, 15 Dec 2023 10:58:20 -0700 Subject: [PATCH] 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 --- .env.example.complete | 1 + app/Access/Oidc/OidcProviderSettings.php | 7 ++++- app/Access/Oidc/OidcService.php | 12 ++++++++ app/Config/oidc.php | 1 + tests/Auth/OidcTest.php | 38 ++++++++++++++++++++++-- 5 files changed, 56 insertions(+), 3 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index e8520a24cae..1242968182a 100644 --- 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_USERINFO_ENDPOINT=null 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 bea6a523e77..49ccab6f0de 100644 --- 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 $userinfoEndpoint; /** * @var string[]|array[] @@ -128,6 +129,10 @@ protected function loadSettingsFromIssuerDiscovery(ClientInterface $httpClient): $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); @@ -177,7 +182,7 @@ protected function loadKeysFromUri(string $uri, ClientInterface $httpClient): ar */ 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; diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index f1e5b25af14..24495799195 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -85,6 +85,7 @@ protected function getProviderSettings(): OidcProviderSettings '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 @@ -228,6 +229,17 @@ protected function processAccessTokenCallback(OidcAccessToken $accessToken, Oidc 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(), diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 7f8f40d4190..8b5470931d0 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -35,6 +35,7 @@ // 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. diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index dbf26f1bd30..f47a201005d 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -668,11 +668,44 @@ protected function withAutodiscovery() 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->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 @@ -684,6 +717,7 @@ protected function getAutoDiscoveryResponse($responseOverrides = []): Response ], 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',