Skip to content

fix(ers): include standard JWT claims in claims mode entity resolution - #3196

Merged
marythought merged 2 commits into
mainfrom
fix/ers-claims-sub-selector
Mar 23, 2026
Merged

fix(ers): include standard JWT claims in claims mode entity resolution#3196
marythought merged 2 commits into
mainfrom
fix/ers-claims-sub-selector

Conversation

@marythought

@marythought marythought commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • The jwx library's PrivateClaims() method excludes standard registered JWT claims (sub, iss, aud, jti) because they are stored as typed struct fields, not in the private claims map
  • This caused selectors like .sub to silently fail in subject mapping conditions when using claims mode ERS — the claim was simply absent from the entity representation
  • Adds registered string claims (sub, iss, jti, aud) back into the claims map using the token's typed getters
  • Time-based claims (exp, iat, nbf) remain excluded because structpb.NewStruct() cannot serialize time.Time values — this maintains existing behavior
  • Applies the fix to both v1 and v2 claims ERS implementations

Root Cause

// Before: excludes sub, iss, aud, jti, exp, iat, nbf
claims := token.PrivateClaims()

// After: adds back sub, iss, aud, jti (skips time.Time fields)
claims := token.PrivateClaims()
if sub := token.Subject(); sub != "" {
    claims["sub"] = sub
}
// ...

The multi-strategy ERS already uses token.AsMap(ctx) which includes all claims, so it was not affected.

Test plan

  • go test ./service/entityresolution/claims/... -v — all 8 tests pass
  • golangci-lint run ./service/entityresolution/claims/... — no issues
  • New test assertions verify .sub is present and iat is excluded
  • CI checks pass

Fixes #3188

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • JWT-derived claims now expose standard registered fields (subject, issuer, JWT ID, audience) when present, allowing authorization selectors and rules to reference these names directly.
  • Tests
    • Updated tests to verify the registered subject is included and certain time-based claims (e.g., issuance time) are excluded from serialized claims.

PrivateClaims() from the jwx library excludes standard registered JWT
claims (sub, iss, aud, jti) because they are stored as typed fields.
This means selectors like .sub never match in subject mapping
conditions when using claims mode ERS.

Add registered claims back into the claims map using the token's typed
getters, avoiding time.Time fields (exp, iat, nbf) that structpb
cannot serialize.

Fixes #3188

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
@marythought
marythought requested a review from a team as a code owner March 20, 2026 23:19
@coderabbitai

coderabbitai Bot commented Mar 20, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3ebd35ae-e87a-4776-b181-867b70d8a76f

📥 Commits

Reviewing files that changed from the base of the PR and between f5ede36 and 5881fbb.

📒 Files selected for processing (2)
  • service/entityresolution/claims/entity_resolution.go
  • service/entityresolution/claims/v2/entity_resolution.go

📝 Walkthrough

Walkthrough

Augments JWT-derived claim maps in both v1 and v2 Entity Resolution implementations by inserting standard registered JWT fields (sub, iss, jti, aud) when present, affecting the produced protobuf structpb.Struct used for claim-based entity resolution.

Changes

Cohort / File(s) Summary
V1 Claims Processing
service/entityresolution/claims/entity_resolution.go, service/entityresolution/claims/entity_resolution_test.go
getEntitiesFromToken now merges standard registered JWT fields (sub, iss, jti, aud) into the private claims map when non-empty. Tests updated to assert sub is present and iat is excluded in the resulting structpb.Struct.
V2 Claims Processing
service/entityresolution/claims/v2/entity_resolution.go, service/entityresolution/claims/v2/entity_resolution_test.go
Same augmentation applied to v2: registered claims (sub, iss, jti, aud) are inserted into the claims map prior to protobuf conversion. Tests updated to verify sub presence and iat absence.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hopped through claims with nimble feet,
Found sub and friends and made them meet.
Tokens now sing in proper form,
Selectors match—no more storm.
A tiny fix, a quiet feat.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: including standard JWT claims (sub, iss, aud, jti) in the claims mode entity resolution implementation.
Linked Issues check ✅ Passed The PR successfully addresses issue #3188 by adding standard JWT claims (sub, iss, jti, aud) to the claims map in both v1 and v2 ERS implementations, enabling .sub selector matching in claims mode.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the .sub selector issue: augmenting claims map with standard registered JWT fields in both v1 and v2 implementations with corresponding test assertions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ers-claims-sub-selector

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug in the claims-based entity resolution service where standard JWT claims were not being properly included in the claims map. This omission prevented effective subject mapping using common selectors. The changes ensure that essential string-based JWT claims are correctly represented, thereby improving the reliability and functionality of entity resolution without altering the handling of time-based claims.

Highlights

  • Standard JWT Claims Inclusion: The jwx library's PrivateClaims() method, which previously excluded standard registered JWT claims like sub, iss, aud, and jti because they are stored as typed struct fields, now explicitly adds these string-based claims back into the claims map.
  • Subject Mapping Fix: This change resolves an issue where selectors like .sub would silently fail in subject mapping conditions when using claims mode Entity Resolution Service (ERS), as the claim was absent from the entity representation.
  • Time-Based Claims Exclusion Maintained: Time-based claims (exp, iat, nbf) continue to be excluded from the claims map, maintaining existing behavior due to structpb.NewStruct()'s inability to serialize time.Time values.
  • Broad Application: The fix has been applied to both v1 and v2 implementations of the claims ERS to ensure consistent behavior across versions.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.


Claims were missing, a bug, Now 'sub' and 'iss' are snug. Mapping works right, In the code's clear light, No more silent shrug.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses the issue where standard JWT claims were not included in the claims map, which is crucial for subject mapping conditions. The addition of specific tests for both v1 and v2 implementations to verify the presence of 'sub' and exclusion of 'iat' is a good practice, ensuring the fix works as intended and preventing regressions. The changes align with the stated goal of improving entity resolution for JWT claims.

Comment thread service/entityresolution/claims/entity_resolution.go
Comment thread service/entityresolution/claims/v2/entity_resolution.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@service/entityresolution/claims/entity_resolution_test.go`:
- Around line 116-119: The test currently asserts that "sub" is included and
"iat" is excluded from claimsMap; add similar assertions to cover other
re-injected standard claims by asserting claimsMap contains "iss", "jti", and
"aud" with their expected values (use the same claimsMap variable in
entity_resolution_test.go and the same assert style, e.g., assert.Equal for
expected values and assert.NotContains for time-based ones) so the test fully
validates all re-injected claims related to the fix.

In `@service/entityresolution/claims/entity_resolution.go`:
- Around line 113-127: Duplicate logic that reinserts standard JWT claims should
be extracted into a shared helper; create a function AddRegisteredClaims(token
jwt.Token, claims map[string]interface{}) that contains the four conditionals
currently in entity_resolution.go (checks for token.Subject(), token.Issuer(),
token.JwtID(), token.Audience()) and then replace the duplicated blocks in both
the v1 and v2 implementations to call AddRegisteredClaims(token, claims); update
imports/packaging as needed so both implementations can call the shared helper.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0acfd2a6-34e6-4d4c-a2fd-4e39649e3d0f

📥 Commits

Reviewing files that changed from the base of the PR and between 0057ff1 and f5ede36.

📒 Files selected for processing (4)
  • service/entityresolution/claims/entity_resolution.go
  • service/entityresolution/claims/entity_resolution_test.go
  • service/entityresolution/claims/v2/entity_resolution.go
  • service/entityresolution/claims/v2/entity_resolution_test.go

Comment thread service/entityresolution/claims/entity_resolution_test.go
Comment thread service/entityresolution/claims/entity_resolution.go
@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 177.599633ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 81.781819ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 406.43083ms
Throughput 246.04 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 42.015688949s
Average Latency 418.576628ms
Throughput 119.00 requests/second

structpb.NewStruct cannot serialize []string directly. Convert
token.Audience() to []interface{} before adding to claims map.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor
Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 194.360993ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 92.052338ms

Benchmark Statistics

Name № Requests Avg Duration Min Duration Max Duration

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 379.84697ms
Throughput 263.26 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.854583213s
Average Latency 386.665941ms
Throughput 128.68 requests/second

@github-actions

Copy link
Copy Markdown
Contributor

@marythought
marythought added this pull request to the merge queue Mar 23, 2026
Merged via the queue into main with commit 6d50da1 Mar 23, 2026
38 checks passed
@marythought
marythought deleted the fix/ers-claims-sub-selector branch March 23, 2026 14:42
JBCongdon pushed a commit to JBCongdon/platform that referenced this pull request May 24, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.14.0](opentdf/platform@service/v0.13.0...service/v0.14.0)
(2026-04-21)


### ⚠ BREAKING CHANGES

* **sdk:** reclassify KAS 400 errors — distinguish tamper from
misconfiguration
([opentdf#3166](opentdf#3166))
* **policy:** optional namespace for RRs
([opentdf#3165](opentdf#3165))
* **policy:** Namespace subject mappings and subject condition sets.
([opentdf#3143](opentdf#3143))
* **policy:** Optional namespace on actions protos, NamespacedPolicy
feature flag ([opentdf#3155](opentdf#3155))
* **policy:** add namespaced actions schema and namespace-aware action
queries ([opentdf#3154](opentdf#3154))
* **policy:** only require namespace on GetAction if no id provided
([opentdf#3144](opentdf#3144))
* **policy:** add namespace field to Actions proto
([opentdf#3130](opentdf#3130))
* **policy:** namespace Registered Resources
([opentdf#3111](opentdf#3111))
* **policy:** add namespace field to RegisteredResource proto
([opentdf#3110](opentdf#3110))

### Features

* **authz:** Namespaced policy in decisioning
([opentdf#3226](opentdf#3226))
([0355934](opentdf@0355934))
* **cli:** migrate otdfctl into platform monorepo
([opentdf#3205](opentdf#3205))
([5177bec](opentdf@5177bec))
* fix tracing ([opentdf#3242](opentdf#3242))
([57e5680](opentdf@57e5680))
* **policy:** add GetObligationTrigger RPC
([opentdf#3318](opentdf#3318))
([d68e39d](opentdf@d68e39d))
* **policy:** add namespace field to Actions proto
([opentdf#3130](opentdf#3130))
([bedc9b3](opentdf@bedc9b3))
* **policy:** add namespace field to RegisteredResource proto
([opentdf#3110](opentdf#3110))
([04fd85d](opentdf@04fd85d))
* **policy:** add namespaced actions schema and namespace-aware action
queries ([opentdf#3154](opentdf#3154))
([c0443f1](opentdf@c0443f1))
* **policy:** add sort ListSubjectMappings API
([opentdf#3255](opentdf#3255))
([9d5d757](opentdf@9d5d757))
* **policy:** Add sort support listregisteredresources api
([opentdf#3312](opentdf#3312))
([91a3ff3](opentdf@91a3ff3))
* **policy:** add sort support to ListAttributes API
([opentdf#3223](opentdf#3223))
([ec3312f](opentdf@ec3312f))
* **policy:** add sort support to ListKeyAccessServer
([opentdf#3287](opentdf#3287))
([7fae2d7](opentdf@7fae2d7))
* **policy:** Add sort support to ListNamespaces API
([opentdf#3192](opentdf#3192))
([aac86cd](opentdf@aac86cd))
* **policy:** add sort support to listobligations api
([opentdf#3300](opentdf#3300))
([9221cac](opentdf@9221cac))
* **policy:** add sort support to ListSubjectConditionSets API
([opentdf#3272](opentdf#3272))
([9010f12](opentdf@9010f12))
* **policy:** add SortField proto and update PageRequest for sort
support ([opentdf#3187](opentdf#3187))
([6cf1862](opentdf@6cf1862))
* **policy:** Enforce same namespace when actions referenced downstream
([opentdf#3206](opentdf#3206))
([4b5463a](opentdf@4b5463a))
* **policy:** namespace Registered Resources
([opentdf#3111](opentdf#3111))
([6db1883](opentdf@6db1883))
* **policy:** Namespace subject mappings and condition sets
([opentdf#3172](opentdf#3172))
([6deed50](opentdf@6deed50))
* **policy:** Namespace subject mappings and subject condition sets.
([opentdf#3143](opentdf#3143))
([3006780](opentdf@3006780))
* **policy:** optional namespace for RRs
([opentdf#3165](opentdf#3165))
([8948018](opentdf@8948018))
* **policy:** rollback migration strategy for namespaced actions
([opentdf#3235](opentdf#3235))
([f7e5e01](opentdf@f7e5e01))
* **policy:** Seed existing namespaces with standard actions
([opentdf#3228](opentdf#3228))
([12136b0](opentdf@12136b0))
* **policy:** Seed namespaces with standard actions on creation +
namespaced actions for obligation triggers
([opentdf#3161](opentdf#3161))
([984d76b](opentdf@984d76b))


### Bug Fixes

* **ci:** Upgrade toolchain version to 1.25.8
([opentdf#3116](opentdf#3116))
([e1b7882](opentdf@e1b7882))
* **core:** do not concat slashes directly in url/file paths
([opentdf#3290](opentdf#3290))
([114c2a7](opentdf@114c2a7))
* **deps:** bump github.com/jackc/pgx/v5 from 5.7.5 to 5.9.0 in /service
([opentdf#3316](opentdf#3316))
([017362e](opentdf@017362e))
* **deps:** bump github.com/opentdf/platform/lib/identifier from 0.2.0
to 0.3.0 in /service
([opentdf#3162](opentdf#3162))
([8bc5dcd](opentdf@8bc5dcd))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.16.0 to
0.17.0 in /service
([opentdf#3125](opentdf#3125))
([29fec61](opentdf@29fec61))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.17.0 to
0.21.0 in /service
([opentdf#3220](opentdf#3220))
([e63add2](opentdf@e63add2))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.21.0 to
0.22.0 in /service
([opentdf#3248](opentdf#3248))
([1ebce73](opentdf@1ebce73))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.22.0 to
0.23.0 in /service
([opentdf#3271](opentdf#3271))
([3338b8e](opentdf@3338b8e))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.23.0 to
0.24.0 in /service
([opentdf#3321](opentdf#3321))
([78e6022](opentdf@78e6022))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.24.0 to
0.25.0 in /service
([opentdf#3333](opentdf#3333))
([3940bf8](opentdf@3940bf8))
* **deps:** bump github.com/opentdf/platform/sdk from 0.13.0 to 0.16.0
in /service ([opentdf#3356](opentdf#3356))
([5617077](opentdf@5617077))
* **deps:** bump
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from
1.42.0 to 1.43.0 in /service
([opentdf#3282](opentdf#3282))
([046374a](opentdf@046374a))
* **deps:** bump go.opentelemetry.io/otel/sdk from 1.42.0 to 1.43.0 in
/service ([opentdf#3281](opentdf#3281))
([56b33f2](opentdf@56b33f2))
* **deps:** bump google.golang.org/grpc from 1.77.0 to 1.79.3 in
/service ([opentdf#3176](opentdf#3176))
([3289502](opentdf@3289502))
* **deps:** remove direct github.com/docker/docker dependency
([opentdf#3229](opentdf#3229))
([2becb27](opentdf@2becb27))
* **deps:** upgrade testcontainers-go to resolve vulns
([opentdf#3299](opentdf#3299))
([72c6f9b](opentdf@72c6f9b))
* **ers:** include standard JWT claims in claims mode entity resolution
([opentdf#3196](opentdf#3196))
([6d50da1](opentdf@6d50da1))
* **ers:** ldap multi-strategy ers
([opentdf#3117](opentdf#3117))
([d3aaf1a](opentdf@d3aaf1a))
* **policy:** deprecate ListAttributeValues in favor of existing
GetAttribute ([opentdf#3108](opentdf#3108))
([7e17c2d](opentdf@7e17c2d))
* **policy:** make obligation trigger uniqueness client-aware
([opentdf#3114](opentdf#3114))
([9265bc3](opentdf@9265bc3))
* **policy:** omit empty attribute values from create responses
([opentdf#3193](opentdf#3193))
([d298378](opentdf@d298378))
* **policy:** only require namespace on GetAction if no id provided
([opentdf#3144](opentdf#3144))
([10d0c0f](opentdf@10d0c0f))
* **policy:** Optional namespace on actions protos, NamespacedPolicy
feature flag ([opentdf#3155](opentdf#3155))
([c20f039](opentdf@c20f039))
* **policy:** order List* results by created_at
([opentdf#3088](opentdf#3088))
([ea90ac2](opentdf@ea90ac2))
* **sdk:** normalize issuer URL before OIDC discovery
([opentdf#3261](opentdf#3261))
([61f98c9](opentdf@61f98c9))
* **sdk:** reclassify KAS 400 errors — distinguish tamper from
misconfiguration
([opentdf#3166](opentdf#3166))
([f04a385](opentdf@f04a385))
* **sdk:** remove testcontainers from consumer dependency graph
([opentdf#3129](opentdf#3129))
([f17dcdd](opentdf@f17dcdd))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Subject mapping .sub selector doesn't match in claims ERS mode

2 participants