Read a claim from UserInfo when the identity token lacks it - #1194
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR improves OIDC admission so that when a required claim is missing from the ID token, admission can be re-evaluated using the provider’s UserInfo response. Changes:
Technical Notes: UserInfo is only queried when required claims are missing and only accepted when its 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| grant.value().access_token, | ||
| identity.value().subject)}; | ||
| if (extra.has_value()) { | ||
| admission = authentication.admits_identity(policy_name, extra.value()); |
There was a problem hiding this comment.
enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_callback_v1.h:266: Re-checking admission against extra.value() alone can drop claims that were present in the ID token but not returned by UserInfo, potentially denying a user who should be admitted. Consider combining the ID token payload with the UserInfo claims before re-evaluating so UserInfo only fills gaps rather than replacing the full claim set.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/authentication/authentication.cc">
<violation number="1" location="enterprise/authentication/authentication.cc:488">
P1: Users whose required claims are split between the identity token and UserInfo are rejected: the second admission check evaluates only the UserInfo object, not the union of both verified responses. Combining the token claims with UserInfo claims before rechecking would preserve claims already present in the identity token.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (!scope_accepts(payload, rule.second)) { | ||
| return false; | ||
| const auto *value{payload.try_at(rule.first)}; | ||
| if (value == nullptr) { |
There was a problem hiding this comment.
P1: Users whose required claims are split between the identity token and UserInfo are rejected: the second admission check evaluates only the UserInfo object, not the union of both verified responses. Combining the token claims with UserInfo claims before rechecking would preserve claims already present in the identity token.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/authentication/authentication.cc, line 488:
<comment>Users whose required claims are split between the identity token and UserInfo are rejected: the second admission check evaluates only the UserInfo object, not the union of both verified responses. Combining the token claims with UserInfo claims before rechecking would preserve claims already present in the identity token.</comment>
<file context>
@@ -471,27 +471,37 @@ auto scope_accepts(const sourcemeta::core::JSON &payload,
- 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;
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: bb2587b | Previous: 87eb5dd | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
326 ms |
312 ms |
1.04 |
Add one schema (100 existing) |
30 ms |
31 ms |
0.97 |
Add one schema (1000 existing) |
93 ms |
103 ms |
0.90 |
Add one schema (10000 existing) |
795 ms |
852 ms |
0.93 |
Update one schema (1 existing) |
22 ms |
22 ms |
1 |
Update one schema (101 existing) |
30 ms |
31 ms |
0.97 |
Update one schema (1001 existing) |
95 ms |
100 ms |
0.95 |
Update one schema (10001 existing) |
787 ms |
844 ms |
0.93 |
Cached rebuild (1 existing) |
8 ms |
8 ms |
1 |
Cached rebuild (101 existing) |
10 ms |
11 ms |
0.91 |
Cached rebuild (1001 existing) |
37 ms |
40 ms |
0.93 |
Cached rebuild (10001 existing) |
315 ms |
352 ms |
0.89 |
Index 100 schemas |
626 ms |
621 ms |
1.01 |
Index 1000 schemas |
1330 ms |
1295 ms |
1.03 |
Index 10000 schemas |
13709 ms |
13479 ms |
1.02 |
Index 10000 schemas (custom meta-schema) |
16543 ms |
16033 ms |
1.03 |
Index 10000 schemas ($ref fan-out) |
16636 ms |
15961 ms |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_callback_v1.h">
<violation number="1" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_callback_v1.h:665">
P2: Merging two independent claim sources lets the identity token's `email_verified` vouch for an `email` that came from a different source (the UserInfo endpoint). In OpenID Connect, `email_verified` is only meaningful for the `email` claim delivered together with it in the same source. After this merge, a token that carries `email_verified: true` but no matching `email` claim causes a UserInfo-supplied `email` to pass `admits_email_domain` as though the provider had verified that specific address — even though it never vouched for it. Consider only combining `email` and `email_verified` when they arrive from the same source, or requiring the token's `email_verified` to be accompanied by the token's own `email` before it is treated as prepending the UserInfo address into the admitted set.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| for (const auto &claim : extra.as_object()) { | ||
| if (!result.defines(claim.first)) { | ||
| result.assign(claim.first, claim.second); |
There was a problem hiding this comment.
P2: Merging two independent claim sources lets the identity token's email_verified vouch for an email that came from a different source (the UserInfo endpoint). In OpenID Connect, email_verified is only meaningful for the email claim delivered together with it in the same source. After this merge, a token that carries email_verified: true but no matching email claim causes a UserInfo-supplied email to pass admits_email_domain as though the provider had verified that specific address — even though it never vouched for it. Consider only combining email and email_verified when they arrive from the same source, or requiring the token's email_verified to be accompanied by the token's own email before it is treated as prepending the UserInfo address into the admitted set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_auth_callback_v1.h, line 665:
<comment>Merging two independent claim sources lets the identity token's `email_verified` vouch for an `email` that came from a different source (the UserInfo endpoint). In OpenID Connect, `email_verified` is only meaningful for the `email` claim delivered together with it in the same source. After this merge, a token that carries `email_verified: true` but no matching `email` claim causes a UserInfo-supplied `email` to pass `admits_email_domain` as though the provider had verified that specific address — even though it never vouched for it. Consider only combining `email` and `email_verified` when they arrive from the same source, or requiring the token's `email_verified` to be accompanied by the token's own `email` before it is treated as prepending the UserInfo address into the admitted set.</comment>
<file context>
@@ -639,6 +640,35 @@ class ActionAuthCallback_v1 : public sourcemeta::one::RouterAction {
+
+ for (const auto &claim : extra.as_object()) {
+ if (!result.defines(claim.first)) {
+ result.assign(claim.first, claim.second);
+ }
+ }
</file context>
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: bb2587b | Previous: 87eb5dd | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
411 ms |
431 ms |
0.95 |
Add one schema (100 existing) |
110 ms |
105 ms |
1.05 |
Add one schema (1000 existing) |
176 ms |
165 ms |
1.07 |
Add one schema (10000 existing) |
894 ms |
829 ms |
1.08 |
Update one schema (1 existing) |
101 ms |
98 ms |
1.03 |
Update one schema (101 existing) |
108 ms |
103 ms |
1.05 |
Update one schema (1001 existing) |
176 ms |
168 ms |
1.05 |
Update one schema (10001 existing) |
904 ms |
852 ms |
1.06 |
Cached rebuild (1 existing) |
10 ms |
9 ms |
1.11 |
Cached rebuild (101 existing) |
13 ms |
12 ms |
1.08 |
Cached rebuild (1001 existing) |
42 ms |
38 ms |
1.11 |
Cached rebuild (10001 existing) |
341 ms |
323 ms |
1.06 |
Index 100 schemas |
695 ms |
701 ms |
0.99 |
Index 1000 schemas |
1470 ms |
1387 ms |
1.06 |
Index 10000 schemas |
14483 ms |
13842 ms |
1.05 |
Index 10000 schemas (custom meta-schema) |
17338 ms |
17288 ms |
1.00 |
Index 10000 schemas ($ref fan-out) |
17387 ms |
17133 ms |
1.01 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/authentication/authentication.cc">
<violation number="1" location="enterprise/authentication/authentication.cc:1671">
P3: The claim-merging and `email_verified` security policy now exists as identical code in the enterprise and community translation units, so a future fix in one edition can silently leave the other edition inconsistent. A shared implementation would keep both builds aligned.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // but only one of them is signed. The address pair is carved out because | ||
| // `email_verified` speaks for the address delivered with it, so the pair is | ||
| // taken whole from whichever answer carried the address | ||
| auto Authentication::combine_claims(const sourcemeta::core::JSON &token, |
There was a problem hiding this comment.
P3: The claim-merging and email_verified security policy now exists as identical code in the enterprise and community translation units, so a future fix in one edition can silently leave the other edition inconsistent. A shared implementation would keep both builds aligned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/authentication/authentication.cc, line 1671:
<comment>The claim-merging and `email_verified` security policy now exists as identical code in the enterprise and community translation units, so a future fix in one edition can silently leave the other edition inconsistent. A shared implementation would keep both builds aligned.</comment>
<file context>
@@ -1664,6 +1664,45 @@ auto Authentication::open_session(const std::string_view value) const
+// but only one of them is signed. The address pair is carved out because
+// `email_verified` speaks for the address delivered with it, so the pair is
+// taken whole from whichever answer carried the address
+auto Authentication::combine_claims(const sourcemeta::core::JSON &token,
+ const sourcemeta::core::JSON &extra)
+ -> sourcemeta::core::JSON {
</file context>
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com