feat(policy): DSPX-3498 dynamic attribute value entitlement mappings#3568
Open
alkalescent wants to merge 5 commits into
Open
feat(policy): DSPX-3498 dynamic attribute value entitlement mappings#3568alkalescent wants to merge 5 commits into
alkalescent wants to merge 5 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 32 minutes and 19 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (25)
📝 WalkthroughWalkthroughIntroduces ChangesDynamic Value Mapping Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant DynamicValueMappingService
participant PolicyDBClient
participant AuditLogger
Client->>DynamicValueMappingService: CreateDynamicValueMapping(req)
DynamicValueMappingService->>DynamicValueMappingService: validate namespace scoping
DynamicValueMappingService->>PolicyDBClient: CreateDynamicValueMapping(ctx, r)
PolicyDBClient->>PolicyDBClient: coexistence check (no subject mappings on definition)
PolicyDBClient->>PolicyDBClient: validate namespace consistency
PolicyDBClient->>PolicyDBClient: insert dynamic_value_mappings + action IDs
PolicyDBClient-->>DynamicValueMappingService: *policy.DynamicValueMapping
DynamicValueMappingService->>AuditLogger: record create success event
DynamicValueMappingService-->>Client: CreateDynamicValueMappingResponse
sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over JustInTimePDP,PolicyDecisionPoint: PDP initialization with dynamic mappings
end
participant JustInTimePDP
participant EntitlementPolicyRetriever
participant PolicyDecisionPoint
participant EvaluateDynamicValueMappingsWithActions
JustInTimePDP->>EntitlementPolicyRetriever: ListAllDynamicValueMappings(ctx)
EntitlementPolicyRetriever-->>JustInTimePDP: []*DynamicValueMapping (paginated)
JustInTimePDP->>PolicyDecisionPoint: NewPolicyDecisionPointWithDynamicValueMappings(allDynamicValueMappings)
PolicyDecisionPoint->>PolicyDecisionPoint: validate + index mappings by definition FQN
rect rgba(60, 179, 113, 0.5)
Note over PolicyDecisionPoint,EvaluateDynamicValueMappingsWithActions: Decision evaluation
end
PolicyDecisionPoint->>PolicyDecisionPoint: getResourceDecisionableAttributes(dynamicMappingsByDefinitionFQN)
PolicyDecisionPoint->>EvaluateDynamicValueMappingsWithActions: mappingsByDefinitionFQN, decisionableAttrs, entityRep
EvaluateDynamicValueMappingsWithActions->>EvaluateDynamicValueMappingsWithActions: evaluate static gate (SubjectConditionSet)
EvaluateDynamicValueMappingsWithActions->>EvaluateDynamicValueMappingsWithActions: evaluate value resolver (selector + comparison)
EvaluateDynamicValueMappingsWithActions-->>PolicyDecisionPoint: entitled FQNs → actions
PolicyDecisionPoint->>PolicyDecisionPoint: merge into entitledFQNsToActions, apply rules
PolicyDecisionPoint-->>JustInTimePDP: AccessDecision
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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>
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/authorization/v2/cache.go (2)
227-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd dynamic value mappings count to debug log.
The debug log includes counts for all other cached fields (attributes, subject_mappings, registered_resources, obligations) but omits
dynamic_value_mappings_count. This inconsistency reduces observability.📊 Proposed fix to maintain logging consistency
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 227 - 233, The c.logger.DebugContext call in the cache refresh log statement is missing the count of dynamic_value_mappings, while it includes counts for attributes, subject_mappings, registered_resources, and obligations. Add a new slog.Int field to the debug log that captures the length of the dynamic_value_mappings variable with a key name of dynamic_value_mappings_count to maintain consistency and observability across all cached fields.
17-305:⚠️ Potential issue | 🟡 MinorRemove unused variable declarations in cache.go.
golangci-lint reports 2 unused variables that violate the Go code quality standards:
- Line 32:
minRefreshIntervalis declared but never used- Line 33:
maxRefreshIntervalis declared but never usedEither remove these declarations or use them if they are intended for future functionality. Tests pass successfully. Run
gofumpt -w service/authorization/v2/cache.golocally to verify formatting before finalizing.🤖 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 17 - 305, The variables minRefreshInterval and maxRefreshInterval are declared in the package-level var block but are never used anywhere in the code, causing a lint violation. Remove these two variable declarations entirely unless they are intended for future functionality, in which case add a comment explaining their purpose. After making this change, run gofumpt to ensure proper formatting.Source: Coding guidelines
🤖 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/internal/access/v2/pdp.go`:
- Around line 213-224: The current validation only checks if the definitionFQN
exists in allAttributesByDefinitionFQN but does not enforce that the definition
is not a HIERARCHY type. Enhance the validation logic to retrieve the canonical
definition from allAttributesByDefinitionFQN using the definitionFQN key, then
check whether that definition represents a HIERARCHY type. If it is a HIERARCHY
definition, add a warning log and continue to skip indexing the mapping in
dynamicMappingsByDefinitionFQN, similar to how the code currently handles
missing definitions. This ensures that mappings targeting HIERARCHY definitions
are properly rejected regardless of whether AttributeDefinition.Rule is set in
the payload.
In
`@service/internal/subjectmappingbuiltin/dynamic_value_mapping_builtin_test.go`:
- Around line 124-160: The test TestEvaluateDynamicValueMappings_StaticGate only
exercises a single SubjectSet within the SubjectConditionSet, leaving the AND/OR
aggregation logic between multiple SubjectSet items untested and vulnerable to
regression. Create a new test case that includes multiple SubjectSet items in
the SubjectConditionSet (similar to the existing test but with multiple subject
sets), and verify the expected behavior when combining multiple condition sets
with different AND/OR operators. Include test scenarios that validate both
passing and failing cases across the multiple subject set aggregation to ensure
the gate behavior is correctly locked in.
In `@service/policy/db/dynamic_value_mappings.go`:
- Around line 189-211: The update flow for dynamic value mappings is missing
namespace consistency validation that is enforced in the create path. Add a call
to validateDynamicValueMappingNamespaceConsistency in the update method (similar
to the create flow) to ensure that the actionIDs resolved via
resolveSubjectMappingActions and the SubjectConditionSetID being updated are
consistent with the targetNamespaceID. This validation should be applied after
populating the updateParams struct to prevent cross-namespace references from
being introduced through updates.
In `@service/policy/db/queries/dynamic_value_mappings.sql`:
- Around line 199-200: The deleteDynamicValueMapping query has a SQLFluff LT14
formatting violation because the WHERE clause is on the same line as the DELETE
statement. Move the WHERE clause to a new line so that the WHERE keyword starts
on its own line after the DELETE FROM dynamic_value_mappings statement,
following proper SQL formatting standards.
- Around line 171-190: The action_delete and action_insert CTEs are executing
regardless of whether mapping_update successfully found and updated a row, which
can cause foreign-key errors when the mapping doesn't exist. Add a condition to
both action_delete and action_insert CTEs that checks whether the mapping with
the given id actually exists (or was successfully updated) before attempting to
delete or insert actions. This ensures these mutations only proceed when the
parent mapping update is successful, preventing concurrent deletes from turning
into deterministic not-found errors.
In `@service/policy/db/subject_mappings.go`:
- Around line 270-274: The no-coexistence enforcement in the current create
method and its reciprocal check in CreateDynamicValueMapping lack atomicity,
allowing concurrent requests to bypass the
ensureNoDynamicValueMappingCoexistence check and CreateDynamicValueMapping's
corresponding check. Add definition-level serialization (such as a locking
mechanism keyed by definition ID) that protects the entire check-then-insert
operation in both create paths, ensuring that only one concurrent request can
perform the validation and insert for the same definition at a time.
In `@service/policy/dynamicvaluemapping/dynamic_value_mapping.go`:
- Around line 27-37: The `OnConfigUpdate` function writes to `svc.config` and
`svc.dbClient` without synchronization, creating a data race when handlers read
these fields concurrently. Add a sync.RWMutex field to the
DynamicValueMappingService struct to protect these shared fields, then acquire
the write lock before updating `svc.config` and `svc.dbClient` in the
`OnConfigUpdate` function. Ensure all other code paths that read or write to
these fields also use the appropriate locking mechanism (RLock for reads, Lock
for writes).
- Around line 90-106: The PolicyCRUDSuccess audit call in the RunInTx callback
is logged before the transaction commit is confirmed, meaning it will record
success even if the transaction fails during commit. Move the audit success
logging (the s.logger.Audit.PolicyCRUDSuccess call and the auditParams
assignments for ObjectID and Original) outside and after the RunInTx call, so it
only executes after the entire transaction including commit has completed
successfully, specifically after verifying that err is nil from the RunInTx
operation.
---
Outside diff comments:
In `@service/authorization/v2/cache.go`:
- Around line 227-233: The c.logger.DebugContext call in the cache refresh log
statement is missing the count of dynamic_value_mappings, while it includes
counts for attributes, subject_mappings, registered_resources, and obligations.
Add a new slog.Int field to the debug log that captures the length of the
dynamic_value_mappings variable with a key name of dynamic_value_mappings_count
to maintain consistency and observability across all cached fields.
- Around line 17-305: The variables minRefreshInterval and maxRefreshInterval
are declared in the package-level var block but are never used anywhere in the
code, causing a lint violation. Remove these two variable declarations entirely
unless they are intended for future functionality, in which case add a comment
explaining their purpose. After making this change, run gofumpt to ensure proper
formatting.
🪄 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: 56a7d11e-1ec4-421c-ad88-d93b64af9468
⛔ Files ignored due to path filters (1)
service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
service/authorization/v2/cache.goservice/go.modservice/integration/dynamic_value_mappings_test.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/internal/subjectmappingbuiltin/subject_mapping_builtin.goservice/logger/audit/constants.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
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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
alkalescent
changed the title
Contributor
alkalescent
force-pushed
the
DSPX-2754-dynamic-attribute-values-impl
branch
from
91a8596 to
dd6d7e9
Compare
Contributor
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: 2
🤖 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/integration/dynamic_value_mappings_test.go`:
- Around line 181-196: The TestListByDefinition test currently only creates a
single dynamic value mapping and verifies its retrieval, but does not test
pagination semantics. Extend this test to create multiple dynamic value mappings
(enough to span multiple pages), then call ListDynamicValueMappings with
pagination parameters (such as limit or page_token) to validate that page
boundaries are respected and continuation tokens work correctly. This will
ensure the pagination path in the ListDynamicValueMappings method is properly
covered by the test.
In `@service/policy/db/queries/dynamic_value_mappings.sql`:
- Around line 158-197: The updateDynamicValueMapping query at line 158 is
annotated with :execrows but returns a result set using SELECT cnt FROM
update_count. The :execrows annotation causes sqlc to generate code that calls
RowsAffected(), which returns -1 for SELECT statements instead of the actual
count value, breaking the not-found detection logic. Change the query annotation
from :execrows to :one so that sqlc generates code that properly scans and
returns the cnt value from the result set.
🪄 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: efad754f-1ae8-40f5-b279-0cd8de81b6f7
⛔ Files ignored due to path filters (1)
service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
service/authorization/v2/cache.goservice/go.modservice/integration/dynamic_value_mappings_test.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/internal/subjectmappingbuiltin/subject_mapping_builtin.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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/integration/dynamic_value_mappings_test.go`:
- Around line 209-227: The pagination test validates offsets and counts but does
not verify that the actual items in each page are distinct. After collecting the
responses from the first and second ListDynamicValueMappings calls, extract the
IDs from each page's GetDynamicValueMappings() results and add assertions to
ensure there is no overlap between the two sets of IDs. Additionally, verify
that the union of all IDs from both pages equals the total count of 3 to ensure
complete coverage without gaps or duplicates across the paginated results.
🪄 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: 1dcd856b-6591-4975-9f30-950eff2a27a6
📒 Files selected for processing (1)
service/integration/dynamic_value_mappings_test.go
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
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>
alkalescent
force-pushed
the
DSPX-2754-dynamic-attribute-values-impl
branch
from
99f98fc to
fa7dd03
Compare
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.
Important
Depends on #3580 (protocol-first). This PR adds a new
protocol/gopackage, so the per-modulego mod tidyCI check stays red until #3580 merges andprotocol/gois released. Sequence: merge #3580 → releaseprotocol/go→ rebase this branch and bumpsdk/servicego.mod → CI goes green.Proposed Changes
Implements the
DynamicValueMappingpolicy primitive end to end (the model chosen by the DSPX-2754 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 (improving on the reference spike):
DynamicValueOperatorEnum:RESOURCE_VALUE_IN/RESOURCE_VALUE_IN_CONTAINS,DynamicValueResolver) so dynamic operators stay isolated from static subject-mapping operators and the schema honestly expresses "dynamic".SubjectConditionSetpre-gate (normal static semantics, no field overload) to support compound "entity attribute AND resource value" conditions.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
Release Notes
New Features
Policy Enforcement
Performance & Reliability
Validation & Governance
Documentation & Tests