From 7d7cd32ca72397b635f7be597ad467ca27cffe6e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 17 Apr 2024 18:23:58 +0100 Subject: [PATCH] OIDC Userinfo: Added userinfo data validation, seperated from id token Wrapped userinfo response in its own class for additional handling and validation. Updated userdetails to take abstract claim data, to be populated by either userinfo data or id token data. --- app/Access/Oidc/OidcIdToken.php | 6 +-- app/Access/Oidc/OidcService.php | 64 ++++++++++++++---------- app/Access/Oidc/OidcUserDetails.php | 22 ++++---- app/Access/Oidc/OidcUserinfoResponse.php | 54 ++++++++++++++++++++ app/Access/Oidc/ProvidesClaims.php | 17 +++++++ 5 files changed, 119 insertions(+), 44 deletions(-) create mode 100644 app/Access/Oidc/OidcUserinfoResponse.php create mode 100644 app/Access/Oidc/ProvidesClaims.php diff --git a/app/Access/Oidc/OidcIdToken.php b/app/Access/Oidc/OidcIdToken.php index 5a395022a..b1da998a5 100644 --- a/app/Access/Oidc/OidcIdToken.php +++ b/app/Access/Oidc/OidcIdToken.php @@ -2,7 +2,7 @@ namespace BookStack\Access\Oidc; -class OidcIdToken +class OidcIdToken implements ProvidesClaims { protected array $header; protected array $payload; @@ -71,10 +71,8 @@ class OidcIdToken /** * Fetch a specific claim from this token. * Returns null if it is null or does not exist. - * - * @return mixed|null */ - public function getClaim(string $claim) + public function getClaim(string $claim): mixed { return $this->payload[$claim] ?? null; } diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 5a73484c1..fba6dc9a8 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -194,35 +194,10 @@ class OidcService try { $idToken->validate($settings->clientId); } catch (OidcInvalidTokenException $exception) { - throw new OidcException("ID token validate failed with error: {$exception->getMessage()}"); - } - - $userDetails = OidcUserDetails::fromToken( - $idToken, - $this->config()['external_id_claim'], - $this->config()['display_name_claims'] ?? '', - $this->config()['groups_claim'] ?? '' - ); - - // TODO - This should not affect id token validation - if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { - $provider = $this->getProvider($settings); - $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); - $response = $provider->getParsedResponse($request); - // TODO - Ensure response content-type is "application/json" before using in this way (5.3.2) - // TODO - The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match, the UserInfo Response values MUST NOT be used. (5.3.2) - // TODO - Response validation (5.3.4) - // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. - // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. - // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. - $claims = $idToken->getAllClaims(); - foreach ($response as $key => $value) { - $claims[$key] = $value; - } - // TODO - Should maybe remain separate from IdToken completely - $idToken->replaceClaims($claims); + throw new OidcException("ID token validation failed with error: {$exception->getMessage()}"); } + $userDetails = $this->getUserDetailsFromToken($idToken, $accessToken, $settings); if (empty($userDetails->email)) { throw new OidcException(trans('errors.oidc_no_email_address')); } @@ -252,6 +227,41 @@ class OidcService return $user; } + /** + * @throws OidcException + */ + protected function getUserDetailsFromToken(OidcIdToken $idToken, OidcAccessToken $accessToken, OidcProviderSettings $settings): OidcUserDetails + { + $userDetails = new OidcUserDetails(); + $userDetails->populate( + $idToken, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); + + if (!$userDetails->isFullyPopulated($this->shouldSyncGroups()) && !empty($settings->userinfoEndpoint)) { + $provider = $this->getProvider($settings); + $request = $provider->getAuthenticatedRequest('GET', $settings->userinfoEndpoint, $accessToken->getToken()); + $response = new OidcUserinfoResponse($provider->getResponse($request)); + + try { + $response->validate($idToken->getClaim('sub')); + } catch (OidcInvalidTokenException $exception) { + throw new OidcException("Userinfo endpoint response validation failed with error: {$exception->getMessage()}"); + } + + $userDetails->populate( + $response, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); + } + + return $userDetails; + } + /** * Get the OIDC config from the application. */ diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php index 1fb40ddc2..172bc9ceb 100644 --- a/app/Access/Oidc/OidcUserDetails.php +++ b/app/Access/Oidc/OidcUserDetails.php @@ -30,23 +30,19 @@ class OidcUserDetails /** * Populate user details from OidcIdToken data. */ - public static function fromToken( - OidcIdToken $token, + public function populate( + ProvidesClaims $claims, string $idClaim, string $displayNameClaims, string $groupsClaim, - ): static { - $id = $token->getClaim($idClaim); - - return new self( - externalId: $id, - email: $token->getClaim('email'), - name: static::getUserDisplayName($displayNameClaims, $token, $id), - groups: static::getUserGroups($groupsClaim, $token), - ); + ): void { + $this->externalId = $claims->getClaim($idClaim) ?? $this->externalId; + $this->email = $claims->getClaim('email') ?? $this->email; + $this->name = static::getUserDisplayName($displayNameClaims, $claims, $this->externalId) ?? $this->name; + $this->groups = static::getUserGroups($groupsClaim, $claims) ?? $this->groups; } - protected static function getUserDisplayName(string $displayNameClaims, OidcIdToken $token, string $defaultValue): string + protected static function getUserDisplayName(string $displayNameClaims, ProvidesClaims $token, string $defaultValue): string { $displayNameClaimParts = explode('|', $displayNameClaims); @@ -65,7 +61,7 @@ class OidcUserDetails return implode(' ', $displayName); } - protected static function getUserGroups(string $groupsClaim, OidcIdToken $token): array + protected static function getUserGroups(string $groupsClaim, ProvidesClaims $token): array { if (empty($groupsClaim)) { return []; diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php new file mode 100644 index 000000000..7c7760434 --- /dev/null +++ b/app/Access/Oidc/OidcUserinfoResponse.php @@ -0,0 +1,54 @@ +getHeader('Content-Type')[0] === 'application/json') { + $this->claims = json_decode($response->getBody()->getContents(), true); + } + + // TODO - Support JWTs + // TODO - Response validation (5.3.4): + // TODO - Verify that the OP that responded was the intended OP through a TLS server certificate check, per RFC 6125 [RFC6125]. + // TODO - If the Client has provided a userinfo_encrypted_response_alg parameter during Registration, decrypt the UserInfo Response using the keys specified during Registration. + // TODO - If the response was signed, the Client SHOULD validate the signature according to JWS [JWS]. + } + + /** + * @throws OidcInvalidTokenException + */ + public function validate(string $idTokenSub): bool + { + $sub = $this->getClaim('sub'); + + // Spec: v1.0 5.3.2: The sub (subject) Claim MUST always be returned in the UserInfo Response. + if (!is_string($sub) || empty($sub)) { + throw new OidcInvalidTokenException("No valid subject value found in userinfo data"); + } + + // Spec: v1.0 5.3.2: The sub Claim in the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; + // if they do not match, the UserInfo Response values MUST NOT be used. + if ($idTokenSub !== $sub) { + throw new OidcInvalidTokenException("Subject value provided in the userinfo endpoint does not match the provided ID token value"); + } + + return true; + } + + public function getClaim(string $claim): mixed + { + return $this->claims[$claim] ?? null; + } + + public function getAllClaims(): array + { + return $this->claims; + } +} diff --git a/app/Access/Oidc/ProvidesClaims.php b/app/Access/Oidc/ProvidesClaims.php new file mode 100644 index 000000000..a3cf51655 --- /dev/null +++ b/app/Access/Oidc/ProvidesClaims.php @@ -0,0 +1,17 @@ +