feat(policy): DSPX-2754 DynamicValueMapping protos + generated code#3580
Conversation
|
Warning Review limit reached
More reviews will be available in 33 minutes and 31 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 We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 ignored due to path filters (3)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR adds a new dynamic value mapping feature to the policy service. It introduces domain models for runtime entitlement resolution, defines a complete CRUD API service, documents the API in OpenAPI, and provides generated Go Connect-RPC bindings for immediate usability. ChangesDynamic Value Mapping API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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 the protocol-first phase of the dynamic attribute value entitlement feature (DSPX-2754). By introducing new protobuf messages and a dedicated service, it establishes the foundation for resolving entitlement authority dynamically at decision time, moving away from static pre-provisioning. This change is isolated to the protocol layer to facilitate modular go module management and dependency resolution for future consumer implementations. 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. The values shift and flow in time, Dynamic logic, reason's climb. With proto structures clearly set, The policy engine's not done yet. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new DynamicValueMappingService along with its CRUD operations and associated protobuf definitions, such as DynamicValueResolver and DynamicValueOperatorEnum, to support dynamic, definition-level value entitlement. The review feedback highlights several opportunities to improve validation in CreateDynamicValueMappingRequest, including ensuring consistent FQN validation, enforcing mutual exclusivity between namespace fields, and simplifying CEL expressions by leveraging standard validation rules.
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:
|
Mirror the proto validation fix from #3580 (namespace oneof, min_len:1 + uri on FQN fields, direct uuid/uri rules) so the consumer branch stays in sync. Refs: DSPX-2754, DSPX-3498 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:
|
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/policy/dynamicvaluemapping/dynamic_value_mapping.proto`:
- Around line 41-42: The comment on the field namespace_id is
incorrect/misleading; update the comment for the field namespace_id (in the
dynamic value mapping proto) to remove the reference to Attribute Definition ID
and clearly state it is the Namespace ID to filter by (e.g., "Namespace ID to
filter by"), leaving the separate attribute_definition_id field’s own comment
unchanged so generated docs no longer conflate the two.
- Around line 72-75: The schema currently enforces exclusivity via
buf.validate.message.oneof (validation-only) for the fields namespace_id and
namespace_fqn, which doesn't get reflected in generated OpenAPI; change the
proto to make these mutually exclusive at the protobuf level by moving
namespace_id and namespace_fqn into a real oneof (e.g., oneof namespace_selector
{ string namespace_id = X; string namespace_fqn = Y; }) so the generated OpenAPI
includes the exclusivity, and apply the same change for the other occurrence
mentioned (lines ~107-114) where the buf.validate.message.oneof is used; keep
the field names namespace_id and namespace_fqn and ensure tags/types remain
consistent when creating the oneof.
🪄 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: 231eabb5-f6b3-45c3-8567-e28d9b8703af
⛔ Files ignored due to path filters (3)
protocol/go/policy/dynamicvaluemapping/dynamic_value_mapping.pb.gois excluded by!**/*.pb.goprotocol/go/policy/dynamicvaluemapping/dynamic_value_mapping_grpc.pb.gois excluded by!**/*.pb.goprotocol/go/policy/objects.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
docs/grpc/index.htmldocs/openapi/authorization/authorization.openapi.yamldocs/openapi/policy/dynamicvaluemapping/dynamic_value_mapping.openapi.yamldocs/openapi/policy/objects.openapi.yamldocs/openapi/policy/subjectmapping/subject_mapping.openapi.yamlprotocol/go/go.modprotocol/go/policy/dynamicvaluemapping/dynamicvaluemappingconnect/dynamic_value_mapping.connect.goservice/policy/dynamicvaluemapping/dynamic_value_mapping.protoservice/policy/objects.proto
Mirror the doc-comment fix from #3580 to keep the consumer branch in sync. Refs: DSPX-2754, DSPX-3498 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:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/policy/dynamicvaluemapping/dynamic_value_mapping.proto (2)
98-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject requests that set both subject-condition-set inputs.
Right now the contract says
new_subject_condition_setis ignored whenexisting_subject_condition_set_idis also present. That makes create requests lossy for an authorization object: a caller can send a brand-new gate and still create a mapping bound to some preexisting subject condition set. This should be a validation failure, not silent precedence.Suggested fix
message CreateDynamicValueMappingRequest { @@ + option (buf.validate.message).oneof = { + fields: ["existing_subject_condition_set_id", "new_subject_condition_set"] + required: false + }; + // Optional static pre-gate. Reuse an existing SubjectConditionSet (prioritized) ... string existing_subject_condition_set_id = 5 [(buf.validate.field).cel = { @@ - // ... or create a new one (ignored if existing_subject_condition_set_id is provided) + // ... or create a new one policy.subjectmapping.SubjectConditionSetCreate new_subject_condition_set = 6;🤖 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/policy/dynamicvaluemapping/dynamic_value_mapping.proto` around lines 98 - 105, The proto currently allows both existing_subject_condition_set_id and new_subject_condition_set to be supplied but treats new_subject_condition_set as ignored; change the contract to reject requests that set both by making them mutually exclusive: either wrap existing_subject_condition_set_id and new_subject_condition_set in a oneof, or add a CEL validation on the message level that enforces exactly-one (e.g., size(existing_subject_condition_set_id) == 0 && exists(new_subject_condition_set) || size(existing_subject_condition_set_id) > 0 && !exists(new_subject_condition_set) ) so that attempts to provide both existing_subject_condition_set_id and new_subject_condition_set fail validation. Ensure you update any relevant comments and tests that expect the previous precedence behavior.
20-22:⚠️ Potential issue | 🟠 MajorMark DynamicValueMapping CRUD mandatory inputs as
buf.validate.field.required(OpenAPI contract drift)
value_resolveris the only input rendered as required indynamic_value_mapping.openapi.yaml;Get/Update/Delete.idandCreate.actionsare missing from the OpenAPIrequiredlist even though the validators enforce them (UUID validator andrepeated.min_items). Add(buf.validate.field).required = trueso OpenAPI-based clients can’t generate requests that will be rejected server-side.Suggested fix
message GetDynamicValueMappingRequest { // Required - string id = 1 [(buf.validate.field).string.uuid = true]; + string id = 1 [ + (buf.validate.field).required = true, + (buf.validate.field).string.uuid = true + ]; } @@ // Required: actions permitted on a matched value repeated policy.Action actions = 4 [ + (buf.validate.field).required = true, (buf.validate.field).repeated.min_items = 1, (buf.validate.field).cel = { id: "action_name_or_id_not_empty" @@ message UpdateDynamicValueMappingRequest { // Required - string id = 1 [(buf.validate.field).string.uuid = true]; + string id = 1 [ + (buf.validate.field).required = true, + (buf.validate.field).string.uuid = true + ]; } @@ message DeleteDynamicValueMappingRequest { // Required - string id = 1 [(buf.validate.field).string.uuid = true]; + string id = 1 [ + (buf.validate.field).required = true, + (buf.validate.field).string.uuid = true + ]; }🤖 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/policy/dynamicvaluemapping/dynamic_value_mapping.proto` around lines 20 - 22, Add explicit required validators to the ID and actions fields so the OpenAPI output marks them required: update GetDynamicValueMappingRequest.id, UpdateDynamicValueMappingRequest.id, and DeleteDynamicValueMappingRequest.id to include (buf.validate.field).required = true (in addition to the existing UUID validator), and add (buf.validate.field).required = true to CreateDynamicValueMappingRequest.actions (alongside the repeated.min_items validator) so OpenAPI clients cannot omit these mandatory inputs.
🤖 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.
Outside diff comments:
In `@service/policy/dynamicvaluemapping/dynamic_value_mapping.proto`:
- Around line 98-105: The proto currently allows both
existing_subject_condition_set_id and new_subject_condition_set to be supplied
but treats new_subject_condition_set as ignored; change the contract to reject
requests that set both by making them mutually exclusive: either wrap
existing_subject_condition_set_id and new_subject_condition_set in a oneof, or
add a CEL validation on the message level that enforces exactly-one (e.g.,
size(existing_subject_condition_set_id) == 0 &&
exists(new_subject_condition_set) || size(existing_subject_condition_set_id) > 0
&& !exists(new_subject_condition_set) ) so that attempts to provide both
existing_subject_condition_set_id and new_subject_condition_set fail validation.
Ensure you update any relevant comments and tests that expect the previous
precedence behavior.
- Around line 20-22: Add explicit required validators to the ID and actions
fields so the OpenAPI output marks them required: update
GetDynamicValueMappingRequest.id, UpdateDynamicValueMappingRequest.id, and
DeleteDynamicValueMappingRequest.id to include (buf.validate.field).required =
true (in addition to the existing UUID validator), and add
(buf.validate.field).required = true to CreateDynamicValueMappingRequest.actions
(alongside the repeated.min_items validator) so OpenAPI clients cannot omit
these mandatory inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3651a194-9f2e-41b3-9952-4c2eb8cc05eb
⛔ Files ignored due to path filters (1)
protocol/go/policy/dynamicvaluemapping/dynamic_value_mapping.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
docs/grpc/index.htmldocs/openapi/policy/dynamicvaluemapping/dynamic_value_mapping.openapi.yamlservice/policy/dynamicvaluemapping/dynamic_value_mapping.proto
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
b90b728 to
982045a
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
982045a to
6a89798
Compare
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>
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>
Split the all-in-one SubjectMappingOperatorEnum into independent axes: ConditionComparisonOperatorEnum (EQUALS/CONTAINS/STARTS_WITH/ENDS_WITH) and ConditionQuantifierEnum (ANY/ALL/NONE), plus a case_insensitive flag on Condition. The legacy operator field is deprecated and retained so the service layer can normalize old payloads for backward compatibility. Apply the same comparison model to DynamicValueResolver (comparison + case_insensitive) and drop the unreleased DynamicValueOperatorEnum. The dynamic match is inherently existential over the selector-resolved entity values, so it carries no quantifier. This also removes the confusing "inversion" wording raised in review. Proto plus regenerated code and docs only. Normalization, evaluation, and cross-field validation land in the service PR #3568. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
4d263ae to
f35eb6e
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Model Condition.case_insensitive and DynamicValueResolver.case_insensitive as google.protobuf.BoolValue instead of bool, per review, so an explicit false is distinguishable from unset. This leaves room to change the default later without ambiguity. Regenerated protocol/go and docs. 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:
|
|
…appings Bring the consumer in line with the #3335 operator decomposition and the BoolValue case_insensitive change from the protocol PR (#3580): - Evaluation: add a shared compareEntityValue helper (EQUALS/CONTAINS/ STARTS_WITH/ENDS_WITH) used by both the static Condition path and the dynamic resolver. EvaluateCondition normalizes the deprecated operator into comparison + quantifier when unset, preserving existing conditions, and applies ANY/ALL/NONE. The dynamic resolver matches existentially over the entity values and honors case_insensitive (nil = case-sensitive). - Validation: check resolver.comparison instead of the removed operator. - DB: replace the operator column with comparison + case_insensitive in the (unreleased) migration, queries, and sqlc/Go layer. - Tests: update fixtures to comparison; set case_insensitive on the case-mismatch case. Static condition tests keep using the deprecated operator to exercise normalization. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Add the sdkconnect wrapper for the DynamicValueMappingService and register it in the SDK, and bump protocol/go to v0.34.0 (which carries the dynamicvaluemapping package). This is the sdk step of the DSPX-2754 consumer split (protocol/go #3580 released first; service consumer PR #3568 depends on this sdk release). 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>
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>
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>
Proposed Changes
DynamicValueMapping/DynamicValueResolvermessages andDynamicValueOperatorEnumtoobjects.proto, plus a dedicatedDynamicValueMappingServicein a newpolicy.dynamicvaluemappingpackage. Includes regeneratedprotocol/goand OpenAPI/gRPC docs.Why split: the dedicated service introduces a new
protocol/gopackage. The per-modulego mod tidyCI check does not use the go workspace, so consumer modules cannot tidy against a package that is not yet in a releasedprotocol/go. Landing and releasingprotocol/gofirst lets the consumer PR require the new version and pass CI.Checklist
Testing Instructions
cd protocol/go && go build ./... && GOFLAGS=-mod=mod go mod tidy(clean)buf lint serviceRelated
Summary by CodeRabbit
Release Notes
New Features