Read a claim from UserInfo when the identity token lacks it

enterprise/authentication/authentication.cc

@@ -471,27 +471,37 @@ auto scope_accepts(const sourcemeta::core::JSON &payload,
 // rules themselves are cumulative, which is what lets a policy widen who it
 // admits without also widening what they reach
 auto admits_claims(const sourcemeta::core::JSON &payload,
-                   const sourcemeta::core::JSON &rules) -> bool {
+                   const sourcemeta::core::JSON &rules)
+    -> sourcemeta::one::Authentication::Admission {
+  using Admission = sourcemeta::one::Authentication::Admission;
   if (!rules.is_object()) {
-    return true;
+    return Admission::Admitted;
   }
 
+  // A claim that never arrived is a question somewhere else may still answer,
+  // so it is held apart from one that arrived and fell short. Every rule is
+  // still read, since a rule already refused settles the whole thing whatever
+  // a later question would return
+  auto outcome{Admission::Admitted};
   for (const auto &rule : rules.as_object()) {
-    if (rule.first == "scope") {
-      if (!scope_accepts(payload, rule.second)) {
-        return false;
+    const auto *value{payload.try_at(rule.first)};
+    if (value == nullptr) {
+      if (outcome == Admission::Admitted) {
+        outcome = Admission::Incomplete;
       }
 
       continue;
     }
 
-    const auto *value{payload.try_at(rule.first)};
-    if (value == nullptr || !claim_accepts(rule.second, *value)) {
-      return false;
+    const auto accepted{rule.first == "scope"
+                            ? scope_accepts(payload, rule.second)
+                            : claim_accepts(rule.second, *value)};
+    if (!accepted) {
+      return Admission::Refused;
     }
   }
 
-  return true;
+  return outcome;
 }
 
 // The reference check treats two JWT policies as the same scope only when
@@ -666,22 +676,32 @@ auto interactive_policy(const OIDCPolicyMetadata &decoded)
 // legally carry one, and the domain names a host, so it is compared without
 // regard to case against domains the artifact already holds in lower case
 auto admits_email_domain(const sourcemeta::core::JSON &claims,
-                         const std::span<const std::byte> domains) -> bool {
+                         const std::span<const std::byte> domains)
+    -> sourcemeta::one::Authentication::Admission {
+  using Admission = sourcemeta::one::Authentication::Admission;
   const auto *verified{claims.try_at("email_verified")};
-  if (verified == nullptr || !verified->is_boolean() ||
-      !verified->to_boolean()) {
-    return false;
+  const auto *address{claims.try_at("email")};
+  // Whatever arrived is judged before whatever did not. An address the
+  // provider declines to vouch for is an answer already given, and it stays
+  // given however much its companion might still turn up elsewhere
+  if (verified != nullptr &&
+      (!verified->is_boolean() || !verified->to_boolean())) {
+    return Admission::Refused;
   }
 
-  const auto *address{claims.try_at("email")};
-  if (address == nullptr || !address->is_string()) {
-    return false;
+  if (address != nullptr && !address->is_string()) {
+    return Admission::Refused;
+  }
+
+  // Only absence is worth a second question
+  if (verified == nullptr || address == nullptr) {
+    return Admission::Incomplete;
   }
 
   const auto &value{address->to_string()};
   const auto separator{value.rfind('@')};
   if (separator == std::string::npos || separator + 1 >= value.size()) {
-    return false;
+    return Admission::Refused;
   }
 
   std::string domain{value.substr(separator + 1)};
@@ -691,10 +711,10 @@ auto admits_email_domain(const sourcemeta::core::JSON &claims,
                            [&admitted, &domain](const auto candidate) -> void {
                              admitted = admitted || candidate == domain;
                            })) {
-    return false;
+    return Admission::Refused;
   }
 
-  return admitted;
+  return admitted ? Admission::Admitted : Admission::Refused;
 }
 
 // The reference check treats two interactive policies as the same scope only
@@ -990,7 +1010,8 @@ struct Authentication::Impl {
       return false;
     }
 
-    return admits_claims(token.payload(), claims);
+    return admits_claims(token.payload(), claims) ==
+           Authentication::Admission::Admitted;
   }
 
   [[nodiscard]] auto
@@ -1141,7 +1162,7 @@ struct Authentication::Impl {
 
   [[nodiscard]] auto admits_identity(const std::string_view policy,
                                      const sourcemeta::core::JSON &claims) const
-      -> bool {
+      -> Authentication::Admission {
     const auto *policies{
         static_cast<const AuthenticationPolicyEntry *>(this->policies_)};
     for (std::uint32_t index{0}; index < this->policy_count_; index += 1) {
@@ -1166,19 +1187,28 @@ struct Authentication::Impl {
       std::uint32_t domains{0};
       std::size_t cursor{0};
       if (!read_u32(decoded.email_domains, cursor, domains)) {
-        return false;
+        return Authentication::Admission::Refused;
       }
 
-      if (domains > 0 && !admits_email_domain(claims, decoded.email_domains)) {
-        return false;
+      const auto address{
+          domains > 0 ? admits_email_domain(claims, decoded.email_domains)
+                      : Authentication::Admission::Admitted};
+      if (address == Authentication::Admission::Refused) {
+        return address;
+      }
+
+      const auto rules{admits_claims(claims, this->claims_[index])};
+      if (rules == Authentication::Admission::Refused) {
+        return rules;
       }
 
-      return admits_claims(claims, this->claims_[index]);
+      // Either half wanting more is the whole wanting more
+      return rules == Authentication::Admission::Incomplete ? rules : address;
     }
 
     // A name that no interactive policy answers to could never have minted a
     // session, so nothing it asserts is admitted
-    return false;
+    return Authentication::Admission::Refused;
   }
 
   [[nodiscard]] auto interactive(const std::string_view path,
@@ -1384,6 +1414,10 @@ struct Authentication::Impl {
         resolved.end_session = document.value().end_session_endpoint().value();
       }
 
+      if (document.value().userinfo_endpoint().has_value()) {
+        resolved.userinfo = document.value().userinfo_endpoint().value();
+      }
+
       // RFC 6749 Section 2.3.1 requires every server to accept the client
       // secret in an authorization header and discourages carrying it in the
       // request body, so the body is used only where the header is refused.
@@ -1630,9 +1664,51 @@ auto Authentication::open_session(const std::string_view value) const
   return this->impl_->open_session(value);
 }
 
+// A provider answering twice about one person is two halves of one account,
+// but only one of them is signed. The address pair is carved out of the merge
+// because `email_verified` speaks for the address delivered with it, so the
+// pair is only ever taken from an answer that carried the address, and an
+// assertion left on its own is dropped whichever answer it came from
+auto Authentication::combine_claims(const sourcemeta::core::JSON &token,
+                                    const sourcemeta::core::JSON &extra)
+    -> sourcemeta::core::JSON {
+  if (!token.is_object()) {
+    return extra.is_object() ? extra : token;
+  }
+
+  // Nothing to combine leaves what the token said untouched, since an
+  // assertion it carried alone can vouch for no address but its own
+  if (!extra.is_object()) {
+    return token;
+  }
+
+  auto result{token};
+  const auto token_has_address{token.defines("email")};
+  const auto extra_has_address{extra.defines("email")};
+  if (!token_has_address) {
+    result.erase("email_verified");
+  }
+
+  for (const auto &claim : extra.as_object()) {
+    const auto address_pair{claim.first == "email" ||
+                            claim.first == "email_verified"};
+    // The answer that carried the address carries the assertion about it, so
+    // neither half is taken from an answer holding only one of them
+    if (address_pair && (token_has_address || !extra_has_address)) {
+      continue;
+    }
+
+    if (!result.defines(claim.first)) {
+      result.assign(claim.first, claim.second);
+    }
+  }
+
+  return result;
+}
+
 auto Authentication::admits_identity(const std::string_view policy,
                                      const sourcemeta::core::JSON &claims) const
-    -> bool {
+    -> Authentication::Admission {
   return this->impl_->admits_identity(policy, claims);
 }
 

enterprise/e2e/auth-sso/one.json

@@ -65,6 +65,53 @@
         "acme.test"
       ]
     },
+    {
+      "type": "oidc",
+      "name": "desk",
+      "paths": [
+        "/desk"
+      ],
+      "issuer": "https://keycloak:8443/realms/main",
+      "clientId": "registry",
+      "clientSecret": {
+        "environmentVariable": "ONE_E2E_OIDC_CLIENT_SECRET"
+      },
+      "sessionSecrets": [
+        {
+          "environmentVariable": "ONE_E2E_OIDC_SESSION_SECRET"
+        }
+      ],
+      "claims": {
+        "department": [
+          "platform"
+        ]
+      }
+    },
+    {
+      "type": "oidc",
+      "name": "split",
+      "paths": [
+        "/split"
+      ],
+      "issuer": "https://keycloak:8443/realms/main",
+      "clientId": "registry",
+      "clientSecret": {
+        "environmentVariable": "ONE_E2E_OIDC_CLIENT_SECRET"
+      },
+      "sessionSecrets": [
+        {
+          "environmentVariable": "ONE_E2E_OIDC_SESSION_SECRET"
+        }
+      ],
+      "claims": {
+        "groups": [
+          "platform"
+        ],
+        "department": [
+          "platform"
+        ]
+      }
+    },
     {
       "type": "oidc",
       "name": "cleartext",
@@ -178,6 +225,12 @@
     },
     "corp": {
       "path": "./schemas/corp"
+    },
+    "desk": {
+      "path": "./schemas/desk"
+    },
+    "split": {
+      "path": "./schemas/split"
     }
   }
 }

enterprise/e2e/auth-sso/playwright/claims.spec.js

@@ -69,6 +69,40 @@ test.describe('Admission by the claims a provider asserts', () => {
     await expect(page).toHaveTitle('Sign In');
   });
 
+  test('a claim the provider only answers for at UserInfo still admits', async ({
+    page,
+  }) => {
+    // The department claim is deliberately kept out of the identity token, so
+    // this only passes if the login asks the UserInfo endpoint for it
+    await page.goto('/desk/');
+    await signIn(page, 'desk', 'jane', 'jane-password');
+    await expect(page).toHaveURL(/\/desk\/$/);
+    await expect(page.locator('table a', { hasText: 'ticket' })).toBeVisible();
+  });
+
+  test('a claim answered at UserInfo can still refuse', async ({ page }) => {
+    await page.goto('/desk/');
+    await signIn(page, 'desk', 'bob', 'bob-password');
+    await expect(page.locator('body')).toContainText(
+      'This account is not admitted here',
+    );
+
+    const denied = await page.goto('/desk/');
+    expect(denied.status()).toBe(401);
+    await expect(page).toHaveTitle('Sign In');
+  });
+
+  test('rules split across the token and UserInfo are read together', async ({
+    page,
+  }) => {
+    // The group only reaches the identity token and the department only the
+    // UserInfo endpoint, so judging either answer alone refuses Jane
+    await page.goto('/split/');
+    await signIn(page, 'split', 'jane', 'jane-password');
+    await expect(page).toHaveURL(/\/split\/$/);
+    await expect(page.locator('table a', { hasText: 'record' })).toBeVisible();
+  });
+
   test('a policy naming no rule still admits whoever signs in', async ({
     page,
   }) => {