feat(policy): Enforce same namespace when actions referenced downstream#3206
Merged
elizabethhealy merged 19 commits intoMar 26, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughRefactors DB action resolution and adds namespace-consistency validation for obligation triggers and registered-resources; updates SQL/queries, DB helpers, and multiple integration tests to use namespace-scoped action lookups and per-run unique namespaces/actions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Test
participant PDB as PolicyDBClient
participant DB as Database
Client->>PDB: CreateObligationTrigger(req: action id/name, attrValue id/fqn, obligation id)
PDB->>PDB: resolveObligationTriggerActionID(action identifier, obligationNs)
alt identifier is id
PDB->>DB: SELECT action WHERE id = $id
else identifier is name
PDB->>DB: SELECT action(s) WHERE name = $name AND namespace = obligationNs
end
DB-->>PDB: action row or not found
PDB->>PDB: validateObligationNamespaceConsistency(obligationNs, attributeValue, actionID)
PDB->>DB: SELECT attribute_value / attribute, SELECT action by id
DB-->>PDB: attribute + action rows or namespace mismatch
PDB->>DB: INSERT INTO obligation_triggers (resolved ActionID, attribute_value id/fqn, obligation_value id)
DB-->>PDB: inserted trigger / error
PDB-->>Client: created ObligationTrigger or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
comp:db
elizabethhealy
changed the title
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 enhances the policy service by introducing support for namespace-scoped actions. It ensures that every new namespace is automatically populated with standard actions like create, read, update, and delete. Furthermore, it refines the action resolution logic across the system, particularly when creating obligation triggers and registered resource action attribute values, to intelligently prioritize actions defined within the same namespace, providing a more granular and context-aware policy enforcement mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize 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 counter productive. 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. Namespaces now align, Actions find their proper home, Order starts to bloom. Footnotes
|
Contributor
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
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
Member
Author
|
@coderabbitai review |
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:
|
c-r33d
reviewed
Mar 26, 2026
c-r33d
reviewed
Mar 26, 2026
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Reporttest-cases-mapping-report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
service/policy/db/obligations.go (1)
715-717:⚠️ Potential issue | 🟠 MajorClassify bad action identifiers before querying.
Line 717 uses
pgtypeUUID(actionID)without the usual.Validguard, and Line 737 still returns a plainerrorwhen bothidandnameare empty. That means malformed IDs are no longer rejected up front like the other ID-based paths in this file, and callers loseerrors.Is(err, db.ErrMissingValue)on the empty-identifier path.🩹 Minimal fix
actionID := action.GetId() if actionID != "" { - got, err := c.queries.getAction(ctx, getActionParams{ID: pgtypeUUID(actionID)}) + parsedActionID := pgtypeUUID(actionID) + if !parsedActionID.Valid { + return "", db.ErrUUIDInvalid + } + got, err := c.queries.getAction(ctx, getActionParams{ID: parsedActionID}) if err != nil { return "", db.WrapIfKnownInvalidQueryErr( errors.Join(db.ErrMissingValue, fmt.Errorf("failed to get action by id [%v]: %w", actionID, err)), ) } @@ actionName := strings.ToLower(action.GetName()) if actionName == "" { - return "", errors.New("action identifier must include either id or name") + return "", errors.Join( + db.ErrMissingValue, + errors.New("action identifier must include either id or name"), + ) }Also applies to: 735-738
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/policy/db/obligations.go` around lines 715 - 717, Validate and classify bad action identifiers before querying: when using actionID (variable actionID) wrap pgtypeUUID(actionID) in a pgtype UUID value and check its .Valid flag before calling c.queries.getAction (the getActionParams should only use a valid pgtypeUUID), returning db.ErrMissingValue for an empty or invalid ID so callers can use errors.Is(err, db.ErrMissingValue); also ensure the branch that currently returns a plain error when both id and name are empty returns db.ErrMissingValue instead and that any malformed UUID produces a clear classified error rather than proceeding to c.queries.getAction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/obligations.go`:
- Around line 796-799: The error message in the obligation trigger namespace
check uses the wrong subsystem label; update the fmt.Errorf in the if branch
that compares actionNsID and targetNsID (where a.ID, actionNsID, targetNsID and
db.ErrNamespaceMismatch are used) so the message refers to the obligation
trigger/action namespace matching the target namespace (e.g., "action [ID]
namespace [..] does not match the specified obligation trigger namespace [..]")
instead of "subject mapping namespace"; leave the error wrapping with
db.ErrNamespaceMismatch intact.
In `@service/policy/db/registered_resources.go`:
- Around line 649-652: The AttributeValueId branch currently just copies
ident.AttributeValueId into attributeValueID and skips resolution when
targetNsID == "", causing unnamespaced (global) IDs to bypass validation; update
the handling in the switch over aav.GetAttributeValueIdentifier() (the
ActionAttributeValue_AttributeValueId case) to resolve or existence-check the ID
via the same GetAttributeValue lookup used for FQNs (respecting targetNsID), and
return a validation error before building the batch if the ID is
missing/malformed or the lookup fails; apply the same change to the other
identical block around the later range (lines handling the second
ActionAttributeValue switch) so both identifier forms (ID and FQN) are validated
consistently prior to batch creation.
---
Duplicate comments:
In `@service/policy/db/obligations.go`:
- Around line 715-717: Validate and classify bad action identifiers before
querying: when using actionID (variable actionID) wrap pgtypeUUID(actionID) in a
pgtype UUID value and check its .Valid flag before calling c.queries.getAction
(the getActionParams should only use a valid pgtypeUUID), returning
db.ErrMissingValue for an empty or invalid ID so callers can use errors.Is(err,
db.ErrMissingValue); also ensure the branch that currently returns a plain error
when both id and name are empty returns db.ErrMissingValue instead and that any
malformed UUID produces a clear classified error rather than proceeding to
c.queries.getAction.
🪄 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: 8b26feec-d55d-41e5-9066-862c336c3197
📒 Files selected for processing (2)
service/policy/db/obligations.goservice/policy/db/registered_resources.go
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: 1
♻️ Duplicate comments (1)
service/policy/db/obligations.go (1)
800-803:⚠️ Potential issue | 🟡 MinorFix the error message label — still says "subject mapping namespace".
This branch validates an obligation trigger, but the error message incorrectly refers to "subject mapping namespace". This will surface in logs/API errors and misdirect debugging.
✏️ Minimal fix
if actionNsID != targetNsID { return errors.Join(db.ErrNamespaceMismatch, - fmt.Errorf("action [%s] namespace [%s] does not match the specified subject mapping namespace [%s]", a.ID, actionNsID, targetNsID)) + fmt.Errorf("action [%s] namespace [%s] does not match the specified obligation namespace [%s]", a.ID, actionNsID, targetNsID)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/policy/db/obligations.go` around lines 800 - 803, The error message in the obligation trigger namespace check erroneously refers to "subject mapping namespace"; update the message in the branch that compares actionNsID and targetNsID (where it returns errors.Join with db.ErrNamespaceMismatch) to accurately state that the action's namespace does not match the obligation/trigger namespace (use a.ID, actionNsID, targetNsID in the message). Keep the same error construction (errors.Join with db.ErrNamespaceMismatch) but replace "subject mapping namespace" with "obligation/trigger namespace" or similar accurate wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/obligations.go`:
- Around line 714-733: The partial namespace check in
resolveObligationTriggerActionID (the if block that inspects actionNamespace and
compares GetId() to obligationNamespaceID) is redundant and incomplete; remove
that entire conditional so namespace validation is handled only by
validateObligationNamespaceConsistency elsewhere, leaving
resolveObligationTriggerActionID to simply fetch the action, hydrate namespace
(but not enforce mismatch here) and return actionID, nil after successful
lookup; reference resolveObligationTriggerActionID,
hydrateNamespaceFromInterface, and validateObligationNamespaceConsistency to
locate the related logic.
---
Duplicate comments:
In `@service/policy/db/obligations.go`:
- Around line 800-803: The error message in the obligation trigger namespace
check erroneously refers to "subject mapping namespace"; update the message in
the branch that compares actionNsID and targetNsID (where it returns errors.Join
with db.ErrNamespaceMismatch) to accurately state that the action's namespace
does not match the obligation/trigger namespace (use a.ID, actionNsID,
targetNsID in the message). Keep the same error construction (errors.Join with
db.ErrNamespaceMismatch) but replace "subject mapping namespace" with
"obligation/trigger namespace" or similar accurate wording.
🪄 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: ed007138-9bdd-4516-9f75-7697edca882b
📒 Files selected for processing (1)
service/policy/db/obligations.go
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
c-r33d
approved these changes
Mar 26, 2026
Contributor
This was referenced Mar 31, 2026
This was referenced Apr 17, 2026
This was referenced May 4, 2026
7 tasks
JBCongdon
pushed a commit
to JBCongdon/platform
that referenced
this pull request
May 24, 2026
🤖 I have created a release *beep* *boop* --- ## [0.14.0](opentdf/platform@service/v0.13.0...service/v0.14.0) (2026-04-21) ### ⚠ BREAKING CHANGES * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ### Features * **authz:** Namespaced policy in decisioning ([opentdf#3226](opentdf#3226)) ([0355934](opentdf@0355934)) * **cli:** migrate otdfctl into platform monorepo ([opentdf#3205](opentdf#3205)) ([5177bec](opentdf@5177bec)) * fix tracing ([opentdf#3242](opentdf#3242)) ([57e5680](opentdf@57e5680)) * **policy:** add GetObligationTrigger RPC ([opentdf#3318](opentdf#3318)) ([d68e39d](opentdf@d68e39d)) * **policy:** add namespace field to Actions proto ([opentdf#3130](opentdf#3130)) ([bedc9b3](opentdf@bedc9b3)) * **policy:** add namespace field to RegisteredResource proto ([opentdf#3110](opentdf#3110)) ([04fd85d](opentdf@04fd85d)) * **policy:** add namespaced actions schema and namespace-aware action queries ([opentdf#3154](opentdf#3154)) ([c0443f1](opentdf@c0443f1)) * **policy:** add sort ListSubjectMappings API ([opentdf#3255](opentdf#3255)) ([9d5d757](opentdf@9d5d757)) * **policy:** Add sort support listregisteredresources api ([opentdf#3312](opentdf#3312)) ([91a3ff3](opentdf@91a3ff3)) * **policy:** add sort support to ListAttributes API ([opentdf#3223](opentdf#3223)) ([ec3312f](opentdf@ec3312f)) * **policy:** add sort support to ListKeyAccessServer ([opentdf#3287](opentdf#3287)) ([7fae2d7](opentdf@7fae2d7)) * **policy:** Add sort support to ListNamespaces API ([opentdf#3192](opentdf#3192)) ([aac86cd](opentdf@aac86cd)) * **policy:** add sort support to listobligations api ([opentdf#3300](opentdf#3300)) ([9221cac](opentdf@9221cac)) * **policy:** add sort support to ListSubjectConditionSets API ([opentdf#3272](opentdf#3272)) ([9010f12](opentdf@9010f12)) * **policy:** add SortField proto and update PageRequest for sort support ([opentdf#3187](opentdf#3187)) ([6cf1862](opentdf@6cf1862)) * **policy:** Enforce same namespace when actions referenced downstream ([opentdf#3206](opentdf#3206)) ([4b5463a](opentdf@4b5463a)) * **policy:** namespace Registered Resources ([opentdf#3111](opentdf#3111)) ([6db1883](opentdf@6db1883)) * **policy:** Namespace subject mappings and condition sets ([opentdf#3172](opentdf#3172)) ([6deed50](opentdf@6deed50)) * **policy:** Namespace subject mappings and subject condition sets. ([opentdf#3143](opentdf#3143)) ([3006780](opentdf@3006780)) * **policy:** optional namespace for RRs ([opentdf#3165](opentdf#3165)) ([8948018](opentdf@8948018)) * **policy:** rollback migration strategy for namespaced actions ([opentdf#3235](opentdf#3235)) ([f7e5e01](opentdf@f7e5e01)) * **policy:** Seed existing namespaces with standard actions ([opentdf#3228](opentdf#3228)) ([12136b0](opentdf@12136b0)) * **policy:** Seed namespaces with standard actions on creation + namespaced actions for obligation triggers ([opentdf#3161](opentdf#3161)) ([984d76b](opentdf@984d76b)) ### Bug Fixes * **ci:** Upgrade toolchain version to 1.25.8 ([opentdf#3116](opentdf#3116)) ([e1b7882](opentdf@e1b7882)) * **core:** do not concat slashes directly in url/file paths ([opentdf#3290](opentdf#3290)) ([114c2a7](opentdf@114c2a7)) * **deps:** bump github.com/jackc/pgx/v5 from 5.7.5 to 5.9.0 in /service ([opentdf#3316](opentdf#3316)) ([017362e](opentdf@017362e)) * **deps:** bump github.com/opentdf/platform/lib/identifier from 0.2.0 to 0.3.0 in /service ([opentdf#3162](opentdf#3162)) ([8bc5dcd](opentdf@8bc5dcd)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.16.0 to 0.17.0 in /service ([opentdf#3125](opentdf#3125)) ([29fec61](opentdf@29fec61)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.17.0 to 0.21.0 in /service ([opentdf#3220](opentdf#3220)) ([e63add2](opentdf@e63add2)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.21.0 to 0.22.0 in /service ([opentdf#3248](opentdf#3248)) ([1ebce73](opentdf@1ebce73)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.22.0 to 0.23.0 in /service ([opentdf#3271](opentdf#3271)) ([3338b8e](opentdf@3338b8e)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.23.0 to 0.24.0 in /service ([opentdf#3321](opentdf#3321)) ([78e6022](opentdf@78e6022)) * **deps:** bump github.com/opentdf/platform/protocol/go from 0.24.0 to 0.25.0 in /service ([opentdf#3333](opentdf#3333)) ([3940bf8](opentdf@3940bf8)) * **deps:** bump github.com/opentdf/platform/sdk from 0.13.0 to 0.16.0 in /service ([opentdf#3356](opentdf#3356)) ([5617077](opentdf@5617077)) * **deps:** bump go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from 1.42.0 to 1.43.0 in /service ([opentdf#3282](opentdf#3282)) ([046374a](opentdf@046374a)) * **deps:** bump go.opentelemetry.io/otel/sdk from 1.42.0 to 1.43.0 in /service ([opentdf#3281](opentdf#3281)) ([56b33f2](opentdf@56b33f2)) * **deps:** bump google.golang.org/grpc from 1.77.0 to 1.79.3 in /service ([opentdf#3176](opentdf#3176)) ([3289502](opentdf@3289502)) * **deps:** remove direct github.com/docker/docker dependency ([opentdf#3229](opentdf#3229)) ([2becb27](opentdf@2becb27)) * **deps:** upgrade testcontainers-go to resolve vulns ([opentdf#3299](opentdf#3299)) ([72c6f9b](opentdf@72c6f9b)) * **ers:** include standard JWT claims in claims mode entity resolution ([opentdf#3196](opentdf#3196)) ([6d50da1](opentdf@6d50da1)) * **ers:** ldap multi-strategy ers ([opentdf#3117](opentdf#3117)) ([d3aaf1a](opentdf@d3aaf1a)) * **policy:** deprecate ListAttributeValues in favor of existing GetAttribute ([opentdf#3108](opentdf#3108)) ([7e17c2d](opentdf@7e17c2d)) * **policy:** make obligation trigger uniqueness client-aware ([opentdf#3114](opentdf#3114)) ([9265bc3](opentdf@9265bc3)) * **policy:** omit empty attribute values from create responses ([opentdf#3193](opentdf#3193)) ([d298378](opentdf@d298378)) * **policy:** only require namespace on GetAction if no id provided ([opentdf#3144](opentdf#3144)) ([10d0c0f](opentdf@10d0c0f)) * **policy:** Optional namespace on actions protos, NamespacedPolicy feature flag ([opentdf#3155](opentdf#3155)) ([c20f039](opentdf@c20f039)) * **policy:** order List* results by created_at ([opentdf#3088](opentdf#3088)) ([ea90ac2](opentdf@ea90ac2)) * **sdk:** normalize issuer URL before OIDC discovery ([opentdf#3261](opentdf#3261)) ([61f98c9](opentdf@61f98c9)) * **sdk:** reclassify KAS 400 errors — distinguish tamper from misconfiguration ([opentdf#3166](opentdf#3166)) ([f04a385](opentdf@f04a385)) * **sdk:** remove testcontainers from consumer dependency graph ([opentdf#3129](opentdf#3129)) ([f17dcdd](opentdf@f17dcdd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
3 tasks
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
Subject Mappings (DB) (Existing Behavior)
Registered Resource Values (DB)
Obligation Triggers / Obligation Values (DB)
Checklist
Testing Instructions
Summary by CodeRabbit
Bug Fixes
Tests
Refactor