lukeshu
/
umorpha-boxes
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity
umorpha-boxes/modules/0001-Oidc-Properly-query-th...

205 lines
9.5 KiB
Diff
Raw Permalink Blame History

From fa62cc6f36c9d4f0cb64a222cdc273bdfb449b17 Mon Sep 17 00:00:00 2001
From: "Luke T. Shumaker" <lukeshu@umorpha.io>
Date: Fri, 15 Dec 2023 10:58:20 -0700
Subject: [PATCH 1/1] 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 0853bd1f..4ccba3d4 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 fa3f579b..cd0b9239 100644
--- a/app/Access/Oidc/OidcProviderSettings.php
+++ b/app/Access/Oidc/OidcProviderSettings.php
@@ -21,6 +21,7 @@ class OidcProviderSettings
public ?string $redirectUri;
public ?string $authorizationEndpoint;
public ?string $tokenEndpoint;
+ public ?string $userinfoEndpoint;
/**
* @var string[]|array[]
@@ -127,6 +128,10 @@ class OidcProviderSettings
$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);
@@ -172,7 +177,7 @@ class OidcProviderSettings
*/
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 8778cbd9..40d8151a 100644
--- a/app/Access/Oidc/OidcService.php
+++ b/app/Access/Oidc/OidcService.php
@@ -84,6 +84,7 @@ class OidcService
'redirectUri' => url('/oidc/callback'),
'authorizationEndpoint' => $config['authorization_endpoint'],
'tokenEndpoint' => $config['token_endpoint'],
+ 'userinfoEndpoint' => $config['userinfo_endpoint'],
]);
// Use keys if configured
@@ -217,6 +218,17 @@ class OidcService
$settings->keys,
);
+ 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 b28b8a41..d6e4f777 100644
--- 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),
+ 'userinfo_endpoint' => env('OIDC_USERINFO_ENDPOINT', null),
// Add extra scopes, upon those required, to the OIDC authentication request
// Multiple values can be provided comma seperated.
diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php
index 204a3bb5..9fc43fe5 100644
--- a/tests/Auth/OidcTest.php
+++ b/tests/Auth/OidcTest.php
@@ -545,11 +545,44 @@ class OidcTest extends TestCase
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
@@ -561,6 +594,7 @@ class OidcTest extends TestCase
], 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(),
], $responseOverrides)));
--
2.43.0
Reference in New Issue View Git Blame Copy Permalink