feat(authz): Namespaced policy in decisioning#3226
Open
elizabethhealy wants to merge 31 commits intomainfrom
Open
feat(authz): Namespaced policy in decisioning#3226elizabethhealy wants to merge 31 commits intomainfrom
elizabethhealy wants to merge 31 commits intomainfrom
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:
📝 WalkthroughWalkthroughAdds a feature-flagged namespaced subject-mapping decision model: config flag, PDP/JIT plumbing, namespaced indexing/filtering, namespace-aware action matching and merging, deduplication/logging for conflicting actions, tests, BDD scenarios, and an ADR documenting semantics and rollout. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService as Authorization Service
participant JIT_PDP as JustInTimePDP
participant PDP
participant Registry as RegisteredResourceIndex
participant SubjectMapping as SubjectMappingsStore
Client->>AuthService: Decision request (action id/name, resource attrs)
AuthService->>JIT_PDP: NewJustInTimePDP(..., allowDirectEntitlements, namespacedPolicy)
AuthService->>JIT_PDP: GetDecision(request)
JIT_PDP->>PDP: Evaluate(resource, request, namespacedPolicy)
PDP->>Registry: Lookup registered-resource values (namespaced & legacy FQNs)
PDP->>SubjectMapping: Filter subject mappings (skip unnamespaced if strict)
PDP->>PDP: isRequestedActionMatch(action, requiredNamespace)
PDP-->>JIT_PDP: Decision (PERMIT/DENY)
JIT_PDP-->>AuthService: Decision
AuthService-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
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 introduces a namespaced policy evaluation model to the Policy Decision Point (PDP). The primary goal is to resolve ambiguities in access decisioning where action names might overlap across different namespaces. By threading a new 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 strict and clear, Policy doubts disappear. Actions matched with care, Security beyond compare. Footnotes
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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 namespaced subject mapping decisioning in the PDP, as outlined in the newly added ADR. It introduces a NamespacedPolicy feature flag to control the enforcement of strict namespace ownership during access evaluation. The core logic includes a centralized action-matching helper and updated evaluation paths for attribute rules. Feedback focuses on the transition to uniform case-insensitive action name matching and suggests more robust logging or error handling when resolving action name collisions during subject mapping evaluation.
Contributor
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 Report |
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
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/pdp.go (2)
297-326:⚠️ Potential issue | 🟠 MajorKeep registered-resource action candidates ID/namespace-aware here too.
This loop still filters on
GetName()and deduplicates by name, but the common matcher now supportsAction.Idand namespace-aware comparisons. Requests that identify the action by ID, or registered-resource values that carry same-name actions in multiple namespaces, can be denied based on iteration order beforegetResourceDecisionever runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/internal/access/v2/pdp.go` around lines 297 - 326, The loop that builds entitledFQNsToActions deduplicates and matches actions only by name (using aav.GetAction().GetName() and slices.ContainsFunc comparing GetName()), which ignores Action.Id and namespaces and can produce wrong ordering/denials; update the matching and deduplication to be ID+namespace-aware: when reading aav.GetAction() (aavAction) and comparing to the Decision Request action (action), compare both Id and namespace (or use a composite key like action.GetId()+":"+action.GetNamespace()) instead of GetName(); likewise replace the slices.ContainsFunc check with a comparison on Id+namespace (or maintain actionsList as a map keyed by that composite key and convert back to []*policy.Action) so entitledFQNsToActions stores unique actions by ID+namespace before calling getResourceDecision (preserve use of entitledFQNsToActions, getResourceDecision, entityRegisteredResourceValue.GetActionAttributeValues, aav.GetAction, action, and p.namespacedPolicy in your changes).
102-157:⚠️ Potential issue | 🟠 MajorDon't mutate caller-owned
Value.SubjectMappingswhile building the PDP index.The builder stores references to policy objects returned from the store/cache and then mutates their
SubjectMappingsfields in place. When usingEntitlementPolicyCache, the cache returns the same slice and object references across builds; mutations persist in the cached data. A prior non-strict build can leave legacy mappings attached to cached values, and a later strict build will evaluate stale mappings that should have been skipped. Clone the value or clearSubjectMappingsbefore populating the lookup to ensure each PDP build operates on independent data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/internal/access/v2/pdp.go` around lines 102 - 157, The code mutates caller-owned policy.Value objects (specifically Value.SubjectMappings) while building the PDP index (see allEntitleableAttributesByValueFQN, mappedValue, and the block that appends to SubjectMappings), which can leak into cached data; fix by cloning the attribute value (or creating a fresh value instance) before assigning or clearing SubjectMappings and before appending mappings so you never modify objects returned from the store/cache; update the branch that handles existing entries and the branch that calls getDefinition to use the cloned value (or reset SubjectMappings) and attach the clone to the new attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue entry to ensure each PDP build has independent SubjectMappings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.md`:
- Around line 30-36: The ADR currently contradicts itself about whether the
request action is unnamespaced: update the decision text so there is a single
normative request-action contract and remove the conflicting statement; choose
one model (either keep "request action remains unnamespaced" and restrict the
precedence list to contextual/derived matching, or adopt the explicit request
shapes `action.id`, `action.name + action.namespace`, `action.name` as the
normative request contract) and rewrite the paragraph around "Request-action
identity precedence" to match that single choice, ensuring all references to
action.id, action.name, and action.namespace in the document are consistent with
the chosen contract.
In `@service/internal/access/v2/evaluate.go`:
- Around line 82-91: The loop that computes requiredNamespaceID for each
regResValue AAV uses accessibleAttributeValues (a filtered map) and leaves
requiredNamespaceID empty when an AAV isn’t present, allowing
isRequestedActionMatch(...) to silently skip instead of denying in strict mode;
fix by resolving the namespace from the unfiltered value→attribute lookup used
elsewhere (the full value->attribute map) when computing requiredNamespaceID for
regResValue.GetActionAttributeValues(), and if that lookup still fails while the
AAV is otherwise a candidate for the requested action and
namespacedPolicy/strict mode is enabled, return a deny (fail-closed) rather than
continuing; update the logic around requiredNamespaceID,
isRequestedActionMatch(action, requiredNamespaceID, aav.GetAction(),
namespacedPolicy), and the strict-mode path accordingly and add a mixed-AAV
strict-mode regression test that ensures a resource with multiple AAVs is denied
when any AAV’s namespace cannot be resolved.
In `@service/internal/access/v2/helpers_test.go`:
- Around line 396-399: The fixture `valueRestricted` is inconsistent because its
SubjectMapping uses `exampleSecretFQN` instead of the intended
`exampleRestrictedFQN`; update the call to `createSimpleSubjectMapping` inside
the `valueRestricted` declaration so the subject FQN argument is
`exampleRestrictedFQN` (not `exampleSecretFQN`) to correctly represent the
restricted fixture and ensure `populateHigherValuesIfHierarchy` tests exercise
the intended hierarchy.
In `@service/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.go`:
- Around line 147-159: The current selection only checks presence of a Namespace
but doesn't prefer the one with an ID (Namespace.Id) when both have namespaces;
update the logic around existingNS/candidateNS so that if both existingHasNS and
candidateHasNS are true you choose the action whose Namespace.GetId() is
non-empty (prefer candidate if it has an ID, otherwise prefer existing if it has
an ID, otherwise fall back to existing); adjust the branch that returns
existing/candidate to inspect Namespace.GetId() before returning to ensure the
action with an actual namespace ID is chosen.
---
Outside diff comments:
In `@service/internal/access/v2/pdp.go`:
- Around line 297-326: The loop that builds entitledFQNsToActions deduplicates
and matches actions only by name (using aav.GetAction().GetName() and
slices.ContainsFunc comparing GetName()), which ignores Action.Id and namespaces
and can produce wrong ordering/denials; update the matching and deduplication to
be ID+namespace-aware: when reading aav.GetAction() (aavAction) and comparing to
the Decision Request action (action), compare both Id and namespace (or use a
composite key like action.GetId()+":"+action.GetNamespace()) instead of
GetName(); likewise replace the slices.ContainsFunc check with a comparison on
Id+namespace (or maintain actionsList as a map keyed by that composite key and
convert back to []*policy.Action) so entitledFQNsToActions stores unique actions
by ID+namespace before calling getResourceDecision (preserve use of
entitledFQNsToActions, getResourceDecision,
entityRegisteredResourceValue.GetActionAttributeValues, aav.GetAction, action,
and p.namespacedPolicy in your changes).
- Around line 102-157: The code mutates caller-owned policy.Value objects
(specifically Value.SubjectMappings) while building the PDP index (see
allEntitleableAttributesByValueFQN, mappedValue, and the block that appends to
SubjectMappings), which can leak into cached data; fix by cloning the attribute
value (or creating a fresh value instance) before assigning or clearing
SubjectMappings and before appending mappings so you never modify objects
returned from the store/cache; update the branch that handles existing entries
and the branch that calls getDefinition to use the cloned value (or reset
SubjectMappings) and attach the clone to the new
attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue entry to ensure each
PDP build has independent SubjectMappings.
🪄 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: 40e86ab6-c264-4bb2-9f3a-45f77facf7bb
📒 Files selected for processing (15)
adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.mddocs/namespaced-subject-mappings-decisioning-plan.mdservice/authorization/v2/authorization.goservice/authorization/v2/config.goservice/internal/access/v2/evaluate.goservice/internal/access/v2/evaluate_test.goservice/internal/access/v2/helpers_test.goservice/internal/access/v2/just_in_time_pdp.goservice/internal/access/v2/pdp.goservice/internal/access/v2/pdp_test.goservice/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.goservice/internal/subjectmappingbuiltin/subject_mapping_builtin_actions_test.gotests-bdd/cukes/resources/platform.namespaced_policy.templatetests-bdd/cukes/steps_subjectmappings.gotests-bdd/features/namespaced-decisioning.feature
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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
jakedoublev
reviewed
Apr 14, 2026
jakedoublev
reviewed
Apr 14, 2026
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
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
jakedoublev
reviewed
Apr 15, 2026
Contributor
There was a problem hiding this comment.
Looks like some otel stuff got removed? 79e3a29
jakedoublev
approved these changes
Apr 15, 2026
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
3 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
This pull request introduces a namespaced policy evaluation model to the Policy Decision Point (PDP). The primary goal is to resolve ambiguities in access decisioning where action names might overlap across different namespaces. By threading a new
NamespacedPolicyconfiguration through the PDP, the system can now enforce strict namespace equality for actions and subject mappings, ensuring a fail-closed and deterministic authorization process suitable for staged migration.Highlights
NamespacedPolicyfeature flag to enforce strict namespace ownership in access decisioning, preventing cross-namespace action matches.NamespacedPolicymode is enabled.EvaluateSubjectMappingsWithActionsto handle and log action name collisions deterministically.Checklist
Testing Instructions
Summary by CodeRabbit