feat(policy): dynamic attribute value entitlement mappings#3568
Open
alkalescent wants to merge 14 commits into
Open
feat(policy): dynamic attribute value entitlement mappings#3568alkalescent wants to merge 14 commits into
alkalescent wants to merge 14 commits into
Conversation
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a new "dynamic value mapping" primitive across the policy stack: a schema/migration, DB CRUD queries and client methods, a Connect RPC service, entity-flatten-based evaluation logic, PDP/JIT-PDP integration, entitlement cache storage, authorization config flag propagation, hierarchy/coexistence constraint enforcement, and integration/unit tests plus an ADR document. ChangesDynamic value mapping rollout
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthorizationService
participant JustInTimePDP
participant PolicyDecisionPoint
participant EvaluateDynamicValueMappingsWithActions
Client->>AuthorizationService: GetDecision request
AuthorizationService->>JustInTimePDP: NewJustInTimePDP(allowDynamicValueMappings)
JustInTimePDP->>JustInTimePDP: ListAllDynamicValueMappings (if enabled)
JustInTimePDP->>PolicyDecisionPoint: NewPolicyDecisionPoint(WithDynamicValueMappings)
PolicyDecisionPoint->>PolicyDecisionPoint: getResourceDecisionableAttributes
PolicyDecisionPoint->>EvaluateDynamicValueMappingsWithActions: evaluate dynamic mappings
EvaluateDynamicValueMappingsWithActions-->>PolicyDecisionPoint: entitled actions by value FQN
PolicyDecisionPoint-->>AuthorizationService: decision + entitlements
AuthorizationService-->>Client: response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
Bot
added
comp:db
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
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
|
Contributor
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
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.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
3 tasks
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
alkalescent
added a commit
that referenced
this pull request
Jun 9, 2026
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>
alkalescent
added a commit
that referenced
this pull request
Jun 10, 2026
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>
alkalescent
added a commit
that referenced
this pull request
Jun 11, 2026
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>
alkalescent
added a commit
that referenced
this pull request
Jun 11, 2026
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>
alkalescent
added a commit
that referenced
this pull request
Jun 15, 2026
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>
alkalescent
added a commit
that referenced
this pull request
Jun 15, 2026
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>
Remove the reserved field numbers/names added for the removed comparison, quantifier, and case_insensitive fields. The decomposed proto surface had no consumers, so reserving the numbers is unnecessary. Regenerated protocol/go. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Service consumer for dynamic value mappings, built on protocol/go v0.34.0 and sdk v0.23.0: - DB: dynamic_value_mappings table + queries + CRUD, no-coexistence guard with value-level subject mappings, comparison + case_insensitive columns. - Service: DynamicValueMappingService, validators, and PDP wiring that loads dynamic mappings via the SDK and evaluates them alongside subject mappings. - Evaluation: shared comparison helper (EQUALS/CONTAINS/STARTS_WITH/ENDS_WITH) used by both static conditions (with quantifier + deprecated-operator normalization) and the dynamic resolver (existential, case_insensitive). protocol/go protos, the dynamicvaluemapping package, and the sdk wrapper landed in #3580 and #3635; this PR bumps to those releases. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
…ing indexing - Bump numExpectedPolicyServices to 11 now that DynamicValueMappingService is registered (fixes the go (service) CI failure). - PDP: skip HIERARCHY attribute definitions when indexing dynamic value mappings, using the canonical definition map (defense in depth vs an unset payload rule). - Add a multi-SubjectSet static-gate test to lock the AND aggregation across subject sets. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
…te value ensureNoDynamicValueMappingCoexistence ran on the CreateSubjectMapping path and returned ErrNotFound for a non-existent attribute value, masking the foreign-key violation that the create insert previously surfaced. Treat a not-found value as "nothing to guard" and return nil so the normal create path reports ErrForeignKeyViolation. The coexistence guard still fires for existing values. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Add TestListByDefinition_Pagination asserting page boundaries (Total and NextOffset) across multiple dynamic value mappings on one definition, matching the subject-mapping list pagination coverage. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
…rlap Strengthen TestListByDefinition_Pagination to verify the paginated pages partition the corpus: page items are distinct, page 2 does not repeat page 1, and the union covers all created mappings exactly once. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
…and ADR Remove DSPX-2754 ticket tokens from code comments and the dynamic_value_mappings table comment (regenerated models.go), and genericize the Virtru repo / Atlassian links in the ADR. Also clarify in the ADR that no-coexistence is between value-level subject mappings and dynamic mappings, not the existence of attribute values (which may exist for obligation triggers, FQN resolution). Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
- Gate the feature behind an experimental allow_dynamic_value_mappings config (default off); the PDP only loads and evaluates dynamic mappings when enabled. - Consolidate the PDP constructors into a single NewPolicyDecisionPoint with a WithDynamicValueMappings functional option (drops the second constructor). - Drop nil checks made redundant by nil-safe proto getters; use a named ErrDynamicValueMappingEvaluation sentinel. - Remove the can't-happen unnamespaced-mapping skip (CRUD enforces namespacing); log a HIERARCHY-referencing dynamic mapping at error level (needs correction). - Wrap UpdateDynamicValueMapping get+update+audit in a transaction, matching UpdateSubjectMapping. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Consume the reverted policy protos that drop the operator decomposition. DynamicValueResolver now carries a SubjectMappingOperatorEnum instead of the removed ConditionComparisonOperatorEnum + case_insensitive, and Condition is evaluated directly on its operator again. - Restore EvaluateCondition to switch on SubjectMappingOperatorEnum (IN, NOT_IN, IN_CONTAINS); drop the normalized comparison/quantifier helpers. - Resolve dynamic value mappings via the resolver operator: IN is existential equality, IN_CONTAINS is substring; NOT_IN and UNSPECIFIED are rejected in the resolver, the access validator, and the DB layer. - Rename the dynamic_value_mappings.operator column (was comparison), drop case_insensitive, and regenerate sqlc. - Update tests to construct resolvers with operators; remove the case-insensitive canonicalization test (case folding is no longer part of resolution). Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
alkalescent
force-pushed
the
DSPX-2754-dynamic-attribute-values-impl
branch
from
c7ba900 to
369b655
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
- Gate the entitlement policy cache's dynamic value mapping fetch behind the allow_dynamic_value_mappings flag so cache readiness no longer depends on the experimental endpoint when the feature is disabled. - Tighten dynamic value mapping negative-path integration tests to assert the specific rejection sentinel (ErrEnumValueInvalid / ErrRestrictViolation). - Update ADR 0005 to the shipped operator model (SubjectMappingOperatorEnum, NOT_IN rejected, exact/case-sensitive matching, optional SubjectConditionSet pre-gate) and drop the removed decomposed-operator description. - Align a stale HIERARCHY rule-change error message with the DynamicValueMapping naming. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
alkalescent
commented
Jul 1, 2026
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>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
New Features
Bug Fixes