144 lines
6.4 KiB
Diff
144 lines
6.4 KiB
Diff
From e4915155b1bd4d9f133eb0b25ba6c9ab846b19a4 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 replace the claims in the ID Token with
|
|
the claims from the UserInfo Endpoint, if a UserInfo Endpoint is defined.
|
|
Two points about this:
|
|
- Replacing the ID Token's claims is the most obvious approach given the
|
|
current code structure and is safe is safe because the ID Token claims
|
|
are a subset of the UserInfo claims; 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.
|
|
|
|
TODO: Write tests
|
|
|
|
[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 | 8 ++++++++
|
|
app/Config/oidc.php | 1 +
|
|
4 files changed, 16 insertions(+), 1 deletion(-)
|
|
|
|
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..eed26167 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,13 @@ class OidcService
|
|
$settings->keys,
|
|
);
|
|
|
|
+ if (!empty($settings->userinfoEndpoint)) {
|
|
+ $provider = $this->getProvider($settings);
|
|
+ $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken());
|
|
+ $response = $provider->getParsedResponse($request);
|
|
+ $idToken->replaceClaims($response);
|
|
+ }
|
|
+
|
|
$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.
|
|
--
|
|
2.43.0
|
|
|