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.

diff --git a/app/Access/Oidc/OidcIdToken.php b/app/Access/Oidc/OidcIdToken.php
index 5a395022a4e3049d91fb0759f50a591d9ca5f432..b1da998a525969675631b32f92a3eb65c286954b 100644 (file)
--- a/app/Access/Oidc/OidcIdToken.php
+++ b/app/Access/Oidc/OidcIdToken.php
@@ -2,7 +2,7 @@
 
 namespace BookStack\Access\Oidc;
 
 
 namespace BookStack\Access\Oidc;
 

-class OidcIdToken
+class OidcIdToken implements ProvidesClaims

 {
     protected array $header;
     protected array $payload;
 {
     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.
     /**
      * 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;
     }
     {
         return $this->payload[$claim] ?? null;
     }

diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php
index 5a73484c1ee1ccb411bcdee1eb9b35f728c34431..fba6dc9a8f34bc5189a2dbce0b3b19c13dc17868 100644 (file)
--- 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) {
         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'));
         }
         if (empty($userDetails->email)) {
             throw new OidcException(trans('errors.oidc_no_email_address'));
         }


@@ -252,6 +227,41 @@ class OidcService
         return $user;
     }
 
         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.
      */
     /**
      * Get the OIDC config from the application.
      */

diff --git a/app/Access/Oidc/OidcUserDetails.php b/app/Access/Oidc/OidcUserDetails.php
index 1fb40ddc23e625ff5297d2d1a471d74f3371d4c2..172bc9ceb44600f5f3b3f39619d729e200a9fe93 100644 (file)
--- a/app/Access/Oidc/OidcUserDetails.php
+++ b/app/Access/Oidc/OidcUserDetails.php
@@ -30,23 +30,19 @@ class OidcUserDetails
     /**
      * Populate user details from OidcIdToken data.
      */
     /**
      * Populate user details from OidcIdToken data.
      */

-    public static function fromToken(
-        OidcIdToken $token,
+    public function populate(
+        ProvidesClaims $claims,

         string $idClaim,
         string $displayNameClaims,
         string $groupsClaim,
         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);
 
     {
         $displayNameClaimParts = explode('|', $displayNameClaims);
 


@@ -65,7 +61,7 @@ class OidcUserDetails
         return implode(' ', $displayName);
     }
 
         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 [];
     {
         if (empty($groupsClaim)) {
             return [];

diff --git a/app/Access/Oidc/OidcUserinfoResponse.php b/app/Access/Oidc/OidcUserinfoResponse.php
new file mode 100644 (file)
index 0000000..7c77604
--- /dev/null
+++ b/app/Access/Oidc/OidcUserinfoResponse.php
@@ -0,0 +1,54 @@
+<?php
+
+namespace BookStack\Access\Oidc;
+
+use Psr\Http\Message\ResponseInterface;
+
+class OidcUserinfoResponse implements ProvidesClaims
+{
+    protected array $claims = [];
+
+    public function __construct(ResponseInterface $response)
+    {
+        if ($response->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 (file)
index 0000000..a3cf516
--- /dev/null
+++ b/app/Access/Oidc/ProvidesClaims.php
@@ -0,0 +1,17 @@
+<?php
+
+namespace BookStack\Access\Oidc;
+
+interface ProvidesClaims
+{
+    /**
+     * Fetch a specific claim.
+     * Returns null if it is null or does not exist.
+     */
+    public function getClaim(string $claim): mixed;
+
+    /**
+     * Get all contained claims.
+     */
+    public function getAllClaims(): array;
+}