feat(policy): dynamic attribute value entitlement mappings#3568
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces definition-scoped dynamic value mappings across policy storage, CRUD APIs, authorization decisioning, caching, configuration, consistency enforcement, and test/documentation coverage. ChangesDynamic value mapping rollout
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthorizationService
participant EntitlementPolicyCache
participant JustInTimePDP
participant PolicyDecisionPoint
participant EvaluateDynamicValueMappingsWithActions
Client->>AuthorizationService: GetDecision request
AuthorizationService->>EntitlementPolicyCache: retrieve dynamic mappings
AuthorizationService->>JustInTimePDP: construct with feature flag
JustInTimePDP->>PolicyDecisionPoint: WithDynamicValueMappings
PolicyDecisionPoint->>PolicyDecisionPoint: build decisionable attributes
PolicyDecisionPoint->>EvaluateDynamicValueMappingsWithActions: evaluate entity and resource values
EvaluateDynamicValueMappingsWithActions-->>PolicyDecisionPoint: entitled actions by value FQN
PolicyDecisionPoint-->>AuthorizationService: decision and entitlements
AuthorizationService-->>Client: response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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 implements a new policy primitive, DefinitionValueEntitlementMapping, which enables dynamic entitlement mappings at the attribute definition level. This allows for more flexible and scalable authorization by comparing resource values against entity attributes at decision time, rather than relying on pre-provisioned static mappings. The change includes a full end-to-end implementation, including protocol definitions, service logic, database persistence, and integration into the existing policy decision point. 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. Ignored Files
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. Dynamic rules now take the stage, No longer static on the page. From definition, values flow, To let the right permissions grow. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements Definition Value Entitlement Mappings (DSPX-2754), which move entitlement authority from concrete attribute values to the attribute definition level. The changes span SDK client generation, caching, PDP evaluation, database storage, and a new gRPC service. The review feedback is highly constructive and identifies several critical issues that should be addressed: a potential data race hazard in the gRPC service, possible nil pointer dereferences in both the PDP evaluator and the database client, a bug in error handling that could return nil values, incorrect type formatting in cache errors, and a redundant database index in the migration script.
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.
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:
|
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:
|
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:
|
Protocol-first half of the dynamic attribute value entitlement work: adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), plus regenerated protocol/go and OpenAPI/gRPC docs. No service implementation here. This lands and releases protocol/go first so the consumer PR (#3568) can require the new version and pass per-module 'go mod tidy'. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Protocol-first half of the dynamic attribute value entitlement work: adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), plus regenerated protocol/go and OpenAPI/gRPC docs. No service implementation here. This lands and releases protocol/go first so the consumer PR (#3568) can require the new version and pass per-module 'go mod tidy'. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Protocol-first half of the dynamic attribute value entitlement work: adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), plus regenerated protocol/go and OpenAPI/gRPC docs. No service implementation here. This lands and releases protocol/go first so the consumer PR (#3568) can require the new version and pass per-module 'go mod tidy'. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), with regenerated protocol/go and OpenAPI/gRPC docs. Protocol-first half of DSPX-2754; the consumer implementation (service/sdk/db/PDP) is PR #3568. Includes review fixes: namespace oneof + min_len:1/uri + direct validation rules on CreateDynamicValueMappingRequest; per-field List filter comments; and a namespace_fqn filter (namespace_id|namespace_fqn oneof) on ListDynamicValueMappingsRequest. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Protocol-first half of the dynamic attribute value entitlement work: adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), plus regenerated protocol/go and OpenAPI/gRPC docs. No service implementation here. This lands and releases protocol/go first so the consumer PR (#3568) can require the new version and pass per-module 'go mod tidy'. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Adds the DynamicValueMapping / DynamicValueResolver messages and DynamicValueOperatorEnum to objects.proto, and a dedicated DynamicValueMappingService (policy.dynamicvaluemapping), with regenerated protocol/go and OpenAPI/gRPC docs. Protocol-first half of DSPX-2754; the consumer implementation (service/sdk/db/PDP) is PR #3568. Includes review fixes: namespace oneof + min_len:1/uri + direct validation rules on CreateDynamicValueMappingRequest; per-field List filter comments; and a namespace_fqn filter (namespace_id|namespace_fqn oneof) on ListDynamicValueMappingsRequest. Refs: DSPX-2754, DSPX-3498 Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
The dynamic value mapping indexing loop skipped mappings whose attribute definition was absent from the entitleable set. The DVM table has a foreign key to attribute_definitions (ON DELETE CASCADE), so a mapping cannot reference a missing definition; the guard is unreachable. Remove it and read the HIERARCHY defense-in-depth rule directly from the canonical map (nil-safe on a miss). Signed-off-by: Krish Suchak <suchak.krish@gmail.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:
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/internal/access/v2/helpers_test.go (1)
1131-1207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a case for the new
hasDynamicMappingbranch.None of the three subtests exercise
dynamicMappingsByDefinitionFQNpopulated withallowDirectEntitlements=false— the new branch added inhelpers.go(lines 258-280) that admits synthetic values purely via a dynamic mapping. Integration coverage exists viapdp_dynamic_test.go, but a focused unit case here would pin down the new admission logic directly.✅ Suggested additional subtest
t.Run("dynamic mapping present, direct entitlements disabled, synthetic value admitted", func(t *testing.T) { resources := []*authz.Resource{ { Resource: &authz.Resource_AttributeValues_{ AttributeValues: &authz.Resource_AttributeValues{ Fqns: []string{attrSyntheticValueFQN}, }, }, }, } dynamicMappings := subjectmappingbuiltin.DynamicValueMappingsByDefinitionFQN{ attrFQN: {{AttributeDefinition: attr}}, } decisionableAttrs, err := getResourceDecisionableAttributes(ctx, logger, nil, nil, entitleableAttributesByDefinitionFQN, dynamicMappings, resources, false, // direct entitlements disabled ) require.NoError(t, err) require.Contains(t, decisionableAttrs, attrSyntheticValueFQN) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/internal/access/v2/helpers_test.go` around lines 1131 - 1207, The unit tests in getResourceDecisionableAttributes are missing coverage for the new hasDynamicMapping path. Add a focused subtest in helpers_test.go that passes dynamicMappingsByDefinitionFQN with a matching definition FQN, uses allowDirectEntitlements=false, and verifies the synthetic value is accepted without error. Use getResourceDecisionableAttributes, entitleableAttributesByDefinitionFQN, and subjectmappingbuiltin.DynamicValueMappingsByDefinitionFQN to locate the branch and assert the returned decisionableAttrs contains the synthetic FQN.Source: Path instructions
service/authorization/v2/cache.go (1)
238-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRefresh log omits dynamic value mappings count.
The debug log after a successful refresh reports counts for attributes, subject mappings, registered resources, and obligations, but not for the newly-added dynamic value mappings, making it harder to observe this experimental feature's cache state.
🔧 Proposed fix
c.logger.DebugContext(ctx, "refreshed EntitlementPolicyCache", slog.Int("attributes_count", len(attributes)), slog.Int("subject_mappings_count", len(subjectMappings)), + slog.Int("dynamic_value_mappings_count", len(dynamicValueMappings)), slog.Int("registered_resources_count", len(registeredResources)), slog.Int("obligations_count", len(obligations)), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@service/authorization/v2/cache.go` around lines 238 - 244, The refresh debug log in EntitlementPolicyCache is missing the new dynamic value mappings count. Update the c.logger.DebugContext call in the refresh path to include a count field for dynamic value mappings alongside attributes, subject mappings, registered resources, and obligations, using the same local variable that holds the dynamic mappings so the cache state is fully observable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@service/authorization/v2/cache_test.go`:
- Line 24: The tests for NewEntitlementPolicyCache only cover the
allowDynamicValueMappings=false path, so add a case that creates the cache with
true and exercises the dynamic mapping branch. Update cache_test.go to hit the
ListAllDynamicValueMappings accessor and verify both the cache miss and cache
hit behavior for the dynamic-value-mapping path.
In `@service/integration/dynamic_value_mappings_test.go`:
- Around line 86-95: Add a sibling test alongside TestRejectsHierarchyDefinition
to cover validateDynamicValueResolverOperator rejecting
SUBJECT_MAPPING_OPERATOR_ENUM_NOT_IN. Use DynamicValueMappingsSuite,
createDefinition, s.resolver, and CreateDynamicValueMapping to construct a
request with an ANY_OF attribute and assert db.ErrEnumValueInvalid, matching the
existing hierarchy rejection style.
- Around line 175-180: The delete verification in the dynamic value mappings
test only checks for any error, which can hide regressions in
GetDynamicValueMapping. Update the assertion after DeleteDynamicValueMapping in
dynamic_value_mappings_test.go to require the specific db.ErrNotFound sentinel,
matching the stricter pattern already used in this test file for other policy
errors. Keep the check tied to the existing
s.db.PolicyClient.GetDynamicValueMapping call so it verifies the mapping is
truly gone.
In `@service/internal/subjectmappingbuiltin/dynamic_value_mapping_builtin.go`:
- Around line 29-79: EvaluateDynamicValueMappingsWithActions currently appends
the same mapping actions once per matching entity in
entityRepresentation.GetAdditionalProps(), which can duplicate actions for a
single valueFQN when multiple entities satisfy the same mapping. Update the
logic so each mapping/valueFQN contributes actions at most once across all
entities, using an existential check/break after the first match rather than
appending per entity. While refactoring, keep the work centered in
EvaluateDynamicValueMappingsWithActions and avoid recomputing
resourceValueSegment inside the per-entity loop since it depends only on the
valueFQN and attribute value.
---
Outside diff comments:
In `@service/authorization/v2/cache.go`:
- Around line 238-244: The refresh debug log in EntitlementPolicyCache is
missing the new dynamic value mappings count. Update the c.logger.DebugContext
call in the refresh path to include a count field for dynamic value mappings
alongside attributes, subject mappings, registered resources, and obligations,
using the same local variable that holds the dynamic mappings so the cache state
is fully observable.
In `@service/internal/access/v2/helpers_test.go`:
- Around line 1131-1207: The unit tests in getResourceDecisionableAttributes are
missing coverage for the new hasDynamicMapping path. Add a focused subtest in
helpers_test.go that passes dynamicMappingsByDefinitionFQN with a matching
definition FQN, uses allowDirectEntitlements=false, and verifies the synthetic
value is accepted without error. Use getResourceDecisionableAttributes,
entitleableAttributesByDefinitionFQN, and
subjectmappingbuiltin.DynamicValueMappingsByDefinitionFQN to locate the branch
and assert the returned decisionableAttrs contains the synthetic FQN.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a4b35fb-436d-41a5-b2b2-d8e6df7510cf
📒 Files selected for processing (28)
service/authorization/v2/authorization.goservice/authorization/v2/cache.goservice/authorization/v2/cache_test.goservice/authorization/v2/config.goservice/integration/dynamic_value_mappings_test.goservice/internal/access/v2/evaluate.goservice/internal/access/v2/helpers.goservice/internal/access/v2/helpers_test.goservice/internal/access/v2/just_in_time_pdp.goservice/internal/access/v2/pdp.goservice/internal/access/v2/pdp_dynamic_test.goservice/internal/access/v2/policy_store.goservice/internal/access/v2/validators.goservice/internal/subjectmappingbuiltin/dynamic_value_mapping_builtin.goservice/internal/subjectmappingbuiltin/dynamic_value_mapping_builtin_test.goservice/logger/audit/constants.goservice/pkg/server/services_test.goservice/policy/adr/0005-dynamic-attribute-value-entitlements-spike.mdservice/policy/db/attributes.goservice/policy/db/dynamic_value_mappings.goservice/policy/db/dynamic_value_mappings.sql.goservice/policy/db/migrations/20260618000000_add_dynamic_value_mappings.sqlservice/policy/db/models.goservice/policy/db/queries/dynamic_value_mappings.sqlservice/policy/db/subject_mappings.goservice/policy/db/utils.goservice/policy/dynamicvaluemapping/dynamic_value_mapping.goservice/policy/policy.go
💤 Files with no reviewable changes (12)
- service/policy/db/attributes.go
- service/pkg/server/services_test.go
- service/policy/db/subject_mappings.go
- service/policy/db/utils.go
- service/policy/policy.go
- service/policy/db/dynamic_value_mappings.sql.go
- service/policy/adr/0005-dynamic-attribute-value-entitlements-spike.md
- service/policy/db/models.go
- service/policy/dynamicvaluemapping/dynamic_value_mapping.go
- service/policy/db/dynamic_value_mappings.go
- service/policy/db/migrations/20260618000000_add_dynamic_value_mappings.sql
- service/policy/db/queries/dynamic_value_mappings.sql
- EvaluateDynamicValueMappingsWithActions now flattens entities once and matches each mapping existentially across all entities, appending its actions at most once per value FQN. Previously multiple matching entities (e.g. person + client) duplicated the entitlement, and the resource value segment was recomputed per entity. - Add integration coverage for NOT_IN operator rejection (ErrEnumValueInvalid). - Assert ErrNotFound after dynamic value mapping delete. - Add authz cache coverage for the enabled dynamic-value-mappings path. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
- Forbid attribute values under a definition with a dynamic value mapping from being added to a registered resource's action attribute values, mirroring the value-level subject-mapping coexistence rule (shared definitionHasDynamicValueMapping helper; enforced in createRegisteredResourceActionAttributeValues). - Name the dynamic value resolver operator errors so the PDP/authz layer can errors.Is them and log malformed policy. - Remove dead not-found check in GetDynamicValueMapping (:one already returns it). - Gate the authz cache Set on allow_dynamic_value_mappings so a disabled feature does not cache an empty slice. - Use slog.Any for error logging in the DVM service. - Document subject-set AND / mapping OR semantics and the IN_CONTAINS substring over-match caution in ADR 0005 and code comments. - Add integration coverage for the registered-resource coexistence guard. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
…tion doc, ERD) Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Proposed Changes
Implements the
DynamicValueMappingpolicy primitive end to end (the model chosen by the spike / ADR 0005). It raises entitlement authority from a concreteAttributeValueto theAttributeDefinition: a single mapping entitles dynamically-requested values by comparing the requested resource value segment against the entity representation at decision time, instead of pre-provisioning a value + subject mapping per discrete ID (MRNs, account IDs, etc.).Design highlights:
DynamicValueResolverreusesSubjectMappingOperatorEnum(IN= the resource segment equals any resolved entity value;IN_CONTAINS= substring).NOT_INis rejected because dynamic resolution is existential over the resolved values. This keeps the resolver and subject-mapping vocabularies aligned and avoids a parallel operator enum. Follows the operator revert in feat(policy)!: undo subject mapping operator decomposition #3685; the earlier decomposedcomparison/quantifier/case_insensitivemodel was undone.SubjectConditionSetpre-gate (normal static semantics, no field overload) to support compound "entity attribute AND resource value" conditions.HIERARCHYrejected; matching is exact and case-sensitive.allow_dynamic_value_mappingsconfig; dynamic mappings are loaded (including in the authz cache), evaluated in decisioning, and gated in the cache refresh only when enabled.ANY_OF/ALL_OFrule layer as static values (manifest stays a list of attribute FQNs).Wiring: proto + generated code + SDK client; DB migration + sqlc + CRUD; dedicated
DynamicValueMappingService; decision-time evaluator; PDP load/merge with synthetic-value support; and authz cache. Replaces the throwaway spike package and ports its tests.GetEntitlementsintentionally does not enumerate dynamic values (high-cardinality, resource-scoped).Checklist
Testing Instructions
cd service && go build ./...cd service && go test ./internal/subjectmappingbuiltin/... ./internal/access/v2/... ./authorization/v2/...cd service && golangci-lint run ./internal/subjectmappingbuiltin/... ./internal/access/v2/ ./authorization/v2/ ./policy/db/ ./policy/dynamicvaluemapping/Related
service/policy/adr/0005-dynamic-attribute-value-entitlements-spike.mdSummary by CodeRabbit
allow_dynamic_value_mappingsflag to enable/disable dynamic mapping evaluation and related direct entitlement synthesis.