fix(ers): coerce attempted_strategies []string to []interface{} for structpb#3645
fix(ers): coerce attempted_strategies []string to []interface{} for structpb#3645damonbanks wants to merge 2 commits into
Conversation
…tructpb
The v2 ResolveEntities handler iterates result.Metadata into a
map[string]interface{} and calls structpb.NewStruct on it. structpb's
NewValue accepts string|float64|bool|nil|map|[]interface{} only - a
raw []string trips 'proto: invalid type: []string' and the successful
entity resolution is silently dropped via 'continue' in the loop.
Downstream authz sees an empty entityRepresentations response, has
nothing to evaluate against subject_condition_sets, and denies the
request.
This blocks any TDF rewrap that depends on multi-strategy ERS to supply
ABAC attributes (the failure mode requires the two-call PDP flow:
CreateEntityChainsFromTokens then ResolveEntities on the resolved
Entity). The first call works because identifier-only Entities don't
trigger struct serialization; the second call serializes Metadata and
hits the type rejection.
Coerce attemptedStrategies to []interface{} at the point of insertion
so all downstream consumers (v1 + v2 ResolveEntities) can serialize it.
Existing unit test updated to assert the new contract.
See virtru-corp/integration-studios#80 for the full upstream-facing
reproduction.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, 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 serialization compatibility issue within the multi-strategy entity resolution service. By coercing the 'attempted_strategies' metadata field to '[]interface{}', the service now correctly serializes metadata into 'structpb', preventing silent failures and downstream authorization errors during the v2 'ResolveEntities' flow. Highlights
New Features🧠 You can now enable Memory (public preview) 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 AssistThe 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
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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. The slice was string, a rigid type, But structpb needed more to be ripe. With interface cast, the data flows, And now the authz correctly shows. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the multi-strategy entity resolution service to coerce the 'attempted_strategies' metadata from a slice of strings to a slice of empty interfaces. This ensures compatibility with structpb.NewValue serialization and prevents resolved entities from being silently dropped. The corresponding unit tests were updated to match this type change. Feedback was provided to rename the loop variable 's' to avoid shadowing the method receiver of the same name.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
…tructpb
Storing attempted_strategies as []string in result.Metadata caused
structpb.NewValue to reject the value (proto: invalid type: []string)
in the v2 ResolveEntities handler. The entity was silently dropped via
continue, returning empty entityRepresentations. For any caller using the
chain-entity path (e.g. JustInTimePDP during KAS rewrap), this produced
PermissionDenied.
Coerce to []interface{} at the point of insertion so all downstream
consumers can serialize it.
Backport of #3645 onto service/v0.15.0.
1 participant
Summary
Service.ResolveEntityinservice/entityresolution/multi-strategy/service.gostoresattempted_strategiesas a raw[]stringinresult.Metadata. The v2ResolveEntitieshandler later iterates that metadata into amap[string]interface{}and callsstructpb.NewStruct.structpb.NewValueacceptsstring | float64 | bool | nil | map | []interface{}only — a raw[]stringis rejected withproto: invalid type: []string, the successful entity resolution is silently dropped viacontinuein the loop, and the response goes back with emptyentityRepresentations.Downstream effect: any caller that exercises the v2
ResolveEntitiespath with multi-strategy ERS gets no resolved entities, no entitlements, and (in the case of KAS rewrap)PermissionDenied.One-line fix: coerce to
[]interface{}at the point of insertion so all downstream consumers (v1 + v2ResolveEntities) can serialize it. The existing unit test (service_test.go:300) asserted the metadata type as[]string— updated to[]interface{}to match the new contract.Reproduction (before this PR)
The failure is only visible in the full v2 PDP flow (
CreateEntityChainsFromTokens→ResolveEntitieson the resolved Entity → flattenAdditionalPropsforsubject_external_selector_valuematching), not in theEntity_Claimsdirect-pass path the existing examples exercise.Minimal config (LDAP provider, two-strategy chain so both ERS calls have a matching strategy):
```yaml
services:
entityresolution:
mode: multi-strategy
failure_strategy: fail-fast
providers:
corporate_ldap:
type: ldap
connection: { host: ldap, port: 3890, use_tls: false, bind_dn: "uid=admin,...", bind_password: "..." }
mapping_strategies:
- name: ldap_lookup_from_jwt # call 1: token claims (preferred_username from Okta/Keycloak)
provider: corporate_ldap
entity_type: subject
conditions: { jwt_claims: [ { claim: preferred_username, operator: exists } ] }
input_mapping: [ { jwt_claim: preferred_username, parameter: username, required: true } ]
ldap_search: &search
base_dn: "ou=people,dc=demo,dc=com"
filter: "(&(objectClass=inetOrgPerson)(mail={username}))"
scope: subtree
attributes: [uid, mail, cn, clearance, nationality, needtoknow]
output_mapping: &out
- { source_attribute: uid, claim_name: username }
- { source_attribute: clearance, claim_name: clearance }
- { source_attribute: nationality, claim_name: nationality, transformation: array }
- { source_attribute: needtoknow, claim_name: needtoknow, transformation: array }
- name: ldap_lookup_from_entity # call 2: claims map derived from resolved Entity (userName camelCase)
provider: corporate_ldap
entity_type: subject
conditions: { jwt_claims: [ { claim: userName, operator: exists } ] }
input_mapping: [ { jwt_claim: userName, parameter: username, required: true } ]
ldap_search:
<<: *search
filter: "(&(objectClass=inetOrgPerson)(uid={username}))"
output_mapping: *out
```
Before this PR: authz returns `PermissionDenied`; dsp logs show `failed to create result struct ... error="proto: invalid type: []string"` for the second `ResolveEntities` call.
After this PR: the same call returns the full claim map in `additionalProps`; PDP flattens and matches against `subject_condition_set` selectors; KAS rewraps the key; `tructl decrypt` succeeds.
Why this wasn't caught earlier
The reference multi-strategy ERS runbook validates resolution only via the Web Admin Entity Resolution tool, which posts a JWT wrapped as `Entity_Claims` and renders `additionalProps` for human inspection. The full v2 PDP flow (used by KAS rewrap) is the only path that re-resolves the resolved Entity from call 1; that's what triggers the `[]string`/`structpb` collision in metadata serialization. There is no encrypt→decrypt round-trip in the runbook, so the bug surfaces only when a real authz decision is requested against multi-strategy-resolved attributes.
End-to-end reproduction artifacts
A full Docker Compose stack (Okta + LLDAP + DSP built against a patched service module) demonstrating both the failure (without this PR) and the fix (with this PR) is at:
virtru-corp/integration-studios#80 → `okta-multi-strategy-ldap/` — see `BUG_REPORT.md` and `README.md` → Known platform limitation.
The integration repo includes:
Adjacent issue (not in this PR)
`service/entityresolution/multi-strategy/output_mapper.go` → `applyTransformation`: the switch supports `array | csv_to_array | ldap_dn_to_cn_array | lowercase | uppercase | trim`. The constant `LDAPAttrValues = "ldap_attribute_values"` (referenced in `transformation/constants_test.go` and named in the multi-strategy README and downstream runbooks) is not wired into the switch — using it errors at runtime with `unknown transformation: ldap_attribute_values`. Easy follow-up: either implement the case or remove from the docs. Happy to open a separate PR.
Testing
Checklist