From a71c8c60b7b6dc0bc20938029b14a86ab9cc95cd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 16 Apr 2024 18:10:32 +0100 Subject: [PATCH] OIDC: Extracted user detail handling to own OidcUserDetails class Allows a proper defined object instead of an array an extracts related logic out of OidcService. Updated userinfo to only be called if we're missing details. --- app/Access/Oidc/OidcService.php | 123 +++++++--------------------- app/Access/Oidc/OidcUserDetails.php | 83 +++++++++++++++++++ 2 files changed, 114 insertions(+), 92 deletions(-) create mode 100644 app/Access/Oidc/OidcUserDetails.php diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index a7f31e56b..5a73484c1 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -12,7 +12,6 @@ use BookStack\Facades\Theme; use BookStack\Http\HttpRequestService; use BookStack\Theming\ThemeEvents; use BookStack\Users\Models\User; -use Illuminate\Support\Arr; use Illuminate\Support\Facades\Cache; use League\OAuth2\Client\OptionProvider\HttpBasicAuthOptionProvider; use League\OAuth2\Client\Provider\Exception\IdentityProviderException; @@ -159,69 +158,6 @@ class OidcService return array_filter($scopeArr); } - /** - * Calculate the display name. - */ - protected function getUserDisplayName(OidcIdToken $token, string $defaultValue): string - { - $displayNameAttrString = $this->config()['display_name_claims'] ?? ''; - $displayNameAttrs = explode('|', $displayNameAttrString); - - $displayName = []; - foreach ($displayNameAttrs as $dnAttr) { - $dnComponent = $token->getClaim($dnAttr) ?? ''; - if ($dnComponent !== '') { - $displayName[] = $dnComponent; - } - } - - if (count($displayName) == 0) { - $displayName[] = $defaultValue; - } - - return implode(' ', $displayName); - } - - /** - * Extract the assigned groups from the id token. - * - * @return string[] - */ - protected function getUserGroups(OidcIdToken $token): array - { - $groupsAttr = $this->config()['groups_claim']; - if (empty($groupsAttr)) { - return []; - } - - $groupsList = Arr::get($token->getAllClaims(), $groupsAttr); - if (!is_array($groupsList)) { - return []; - } - - return array_values(array_filter($groupsList, function ($val) { - return is_string($val); - })); - } - - /** - * Extract the details of a user from an ID token. - * - * @return array{name: string, email: string, external_id: string, groups: string[]} - */ - protected function getUserDetails(OidcIdToken $token): array - { - $idClaim = $this->config()['external_id_claim']; - $id = $token->getClaim($idClaim); - - return [ - 'external_id' => $id, - 'email' => $token->getClaim('email'), - 'name' => $this->getUserDisplayName($token, $id), - 'groups' => $this->getUserGroups($token), - ]; - } - /** * Processes a received access token for a user. Login the user when * they exist, optionally registering them automatically. @@ -241,26 +177,6 @@ class OidcService session()->put("oidc_id_token", $idTokenText); - // TODO - This should not affect id token validation - // TODO - Should only call if we're missing properties - if (!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); - } - $returnClaims = Theme::dispatch(ThemeEvents::OIDC_ID_TOKEN_PRE_VALIDATE, $idToken->getAllClaims(), [ 'access_token' => $accessToken->getToken(), 'expires_in' => $accessToken->getExpires(), @@ -281,31 +197,54 @@ class OidcService throw new OidcException("ID token validate failed with error: {$exception->getMessage()}"); } - $userDetails = $this->getUserDetails($idToken); - $isLoggedIn = auth()->check(); + $userDetails = OidcUserDetails::fromToken( + $idToken, + $this->config()['external_id_claim'], + $this->config()['display_name_claims'] ?? '', + $this->config()['groups_claim'] ?? '' + ); - if (empty($userDetails['email'])) { + // 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); + } + + if (empty($userDetails->email)) { throw new OidcException(trans('errors.oidc_no_email_address')); } + $isLoggedIn = auth()->check(); if ($isLoggedIn) { throw new OidcException(trans('errors.oidc_already_logged_in')); } try { $user = $this->registrationService->findOrRegister( - $userDetails['name'], - $userDetails['email'], - $userDetails['external_id'] + $userDetails->name, + $userDetails->email, + $userDetails->externalId ); } catch (UserRegistrationException $exception) { throw new OidcException($exception->getMessage()); } if ($this->shouldSyncGroups()) { - $groups = $userDetails['groups']; $detachExisting = $this->config()['remove_from_groups']; - $this->groupService->syncUserWithFoundGroups($user, $groups, $detachExisting); + $this->groupService->syncUserWithFoundGroups($user, $userDetails->groups ?? [], $detachExisting); } $this->loginService->login($user, 'oidc'); diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php new file mode 100644 index 000000000..1fb40ddc2 --- /dev/null +++ b/app/Access/Oidc/OidcUserDetails.php @@ -0,0 +1,83 @@ +externalId) + || empty($this->email) + || empty($this->name) + || ($groupSyncActive && empty($this->groups)); + + return !$hasEmpty; + } + + /** + * Populate user details from OidcIdToken data. + */ + public static function fromToken( + OidcIdToken $token, + 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), + ); + } + + protected static function getUserDisplayName(string $displayNameClaims, OidcIdToken $token, string $defaultValue): string + { + $displayNameClaimParts = explode('|', $displayNameClaims); + + $displayName = []; + foreach ($displayNameClaimParts as $claim) { + $component = $token->getClaim(trim($claim)) ?? ''; + if ($component !== '') { + $displayName[] = $component; + } + } + + if (count($displayName) === 0) { + $displayName[] = $defaultValue; + } + + return implode(' ', $displayName); + } + + protected static function getUserGroups(string $groupsClaim, OidcIdToken $token): array + { + if (empty($groupsClaim)) { + return []; + } + + $groupsList = Arr::get($token->getAllClaims(), $groupsClaim); + if (!is_array($groupsList)) { + return []; + } + + return array_values(array_filter($groupsList, function ($val) { + return is_string($val); + })); + } +}