feat(authz): Namespaced policy in decisioning#3226
Open
elizabethhealy wants to merge 16 commits intomainfrom
Open
feat(authz): Namespaced policy in decisioning#3226elizabethhealy wants to merge 16 commits intomainfrom
elizabethhealy wants to merge 16 commits intomainfrom
Conversation
|
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 (2)
📝 WalkthroughWalkthroughAdded a namespaced-policy ADR and implemented a feature-flag-controlled namespaced subject-mapping decisioning flow: config flag, PDP plumbing, namespaced indexing/filtering, namespace-aware action matching and entitlements merging, tests, BDD scenarios, and auxiliary deduplication/logging for conflicting actions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService as Authorization Service (handler)
participant JIT_PDP as JustInTimePDP
participant PDP
participant Registry as RegisteredResourceIndex
participant SubjectMapping as SubjectMappingsStore
Client->>AuthService: Decision request (action name/id, resource attr)
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 keys)
PDP->>SubjectMapping: Filter subject mappings (skip unnamespaced if strict)
PDP->>PDP: isRequestedActionMatch(action, requiredNamespace)
PDP-->>JIT_PDP: Decision (PERMIT/DENY)
JIT_PDP-->>AuthService: Statusified 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 docstrings
🧪 Generate unit tests (beta)
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.
service/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.go
Show resolved
Hide resolved
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
adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.md
Outdated
Show resolved
Hide resolved
service/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.go
Show resolved
Hide resolved
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
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:
|
Member
Author
|
@coderabbitai review |
Member
Author
|
/gemini summary |
Contributor
Summary of ChangesThis 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. Activity
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Line 86: Add debug logging to action-match and rule-evaluation paths: inside
isRequestedActionMatch() and wherever rules are evaluated, emit a debug log that
includes the requested action, the required namespace, the candidate namespace
being tested, and the current rule/value context (e.g., rule id, rule
conditions, evaluated value). Ensure these logs run at debug level and are added
both for successful matches and for mismatches/early exits so decision traces
show why a candidate was accepted or rejected.
In `@service/internal/access/v2/evaluate.go`:
- Around line 500-515: The namespace matching on requestedAction.Namespace is
being applied unconditionally and can deny requests even when namespacedPolicy
is false; to fix, ensure the namespace filter is only applied when
namespacedPolicy is true by moving the entire requestedAction.GetNamespace() /
entitledAction.GetNamespace() comparison block behind the namespacedPolicy guard
or wrapping that block in if namespacedPolicy { ... }; reference the
requestedAction.GetNamespace(), entitledAction.GetNamespace(), and
namespacedPolicy symbols to locate the code to change.
In `@service/internal/access/v2/pdp_test.go`:
- Around line 2731-2734: The test case name is misleading: the case uses action
Id idNamespaceBRead which is not present in the subject mapping (so the denial
is due to a non-existent action id, not a namespace mismatch). Update the test
entry (the "name" field in the table where action: &policy.Action{Id:
idNamespaceBRead, Name: actions.ActionNameRead}) to a clearer label such as
"non-existent action id denies" (or similar) so the test name matches the actual
cause of denial.
🪄 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: 0e47ec27-d04c-444b-89ce-3d4db19b9ed0
📒 Files selected for processing (7)
adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.mdservice/internal/access/v2/evaluate.goservice/internal/access/v2/evaluate_test.goservice/internal/access/v2/helpers_test.goservice/internal/access/v2/pdp.goservice/internal/access/v2/pdp_test.gotests-bdd/features/namespaced-decisioning.feature
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 1, 2026
|
|
||
| // When authorization service can consume cached policy, switch to the other PDP (process based on policy passed in) | ||
| pdp, err := access.NewJustInTimePDP(ctx, as.logger, as.sdk, as.cache, as.config.AllowDirectEntitlements) | ||
| pdp, err := access.NewJustInTimePDP(ctx, as.logger, as.sdk, as.cache, as.config.AllowDirectEntitlements, as.config.NamespacedPolicy) |
Contributor
There was a problem hiding this comment.
Nitpick: could we rename this to a boolean like shouldEnforceNamespacedPolicy or something a tiny bit more readable?
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/internal/access/v2/pdp.go (2)
103-116:⚠️ Potential issue | 🟠 MajorAvoid mutating caller-owned
Value.SubjectMappings.
allEntitleableAttributesByValueFQNstores the originalattr.GetValues()pointers, and Line 146 appends straight into those shared slices. If the same attribute objects are reused for another PDP build — plausible on the cachedNewJustInTimePDPpath — a legacy build can leave unnamespaced mappings attached and a later strict build will still evaluate them even though Lines 130-140 skip them. Repeated constructors also accumulate duplicates. Please clone/reset thepolicy.Values before wiring mappings and add a regression that builds legacy then strict PDPs from the same inputs.Suggested fix
+import "google.golang.org/protobuf/proto" ... for _, value := range attr.GetValues() { + valueCopy := proto.Clone(value).(*policy.Value) + valueCopy.SubjectMappings = nil mapped := &attrs.GetAttributeValuesByFqnsResponse_AttributeAndValue{ - Value: value, + Value: valueCopy, Attribute: attr, } - allEntitleableAttributesByValueFQN[value.GetFqn()] = mapped + allEntitleableAttributesByValueFQN[valueCopy.GetFqn()] = mapped } ... - mappedValue.SubjectMappings = []*policy.SubjectMapping{sm} + mappedValue = proto.Clone(mappedValue).(*policy.Value) + mappedValue.SubjectMappings = []*policy.SubjectMapping{sm}Also applies to: 130-146, 154-159
🤖 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 103 - 116, The code mutates caller-owned Value objects (attr.GetValues()) by reusing their pointers in allEntitleableAttributesByValueFQN and later appending to Value.SubjectMappings; to fix, create deep copies of each policy.Value (or at minimum copy the Value struct and reset/clone its SubjectMappings slice) before storing/inspecting them in allEntitleableAttributesByValueFQN and before any code that appends mappings (refer to allEntitleableAttributesByValueFQN, attr.GetValues(), and Value.SubjectMappings), so the original attribute definitions are never modified; also add a regression test that constructs a legacy PDP then a strict PDP from the same input (exercise NewJustInTimePDP reuse path) to ensure mappings are not accumulated across builds.
314-345:⚠️ Potential issue | 🟠 MajorPreserve ID/namespace action variants for registered-resource subjects.
By the time Line 344 calls
getResourceDecision, this map has already filtered and dedupedentityRegisteredResourceValue.GetActionAttributeValues()onAction.Namealone. That drops same-name actions that differ only by namespace or ID, so strict mode — or any request with an explicit namespace/ID — can incorrectly deny when the kept variant is not the matching one. UseisRequestedActionMatch(..., false)for the prefilter and keep distinctID -> Name+Namespace -> Namevariants until evaluation. Please add a regression with two same-name RR actions on the same value FQN.Suggested fix
aavAction := aav.GetAction() - if action.GetName() != aavAction.GetName() { + if !isRequestedActionMatch(ctx, l, action, "", aavAction, false) { l.DebugContext(ctx, "skipping action not matching Decision Request action", slog.String("action_name", aavAction.GetName())) continue } ... - if !slices.ContainsFunc(actionsList, func(a *policy.Action) bool { - return a.GetName() == aavAction.GetName() - }) { - actionsList = append(actionsList, aavAction) - } + // Preserve distinct ID / namespace variants so downstream evaluation can + // apply the full action-identity precedence. + actionsList = append(actionsList, aavAction)🤖 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 314 - 345, The current prefilter over entityRegisteredResourceValue.GetActionAttributeValues collapses actions by Action.Name into entitledFQNsToActions, losing distinct ID/namespace variants; change the prefilter to call isRequestedActionMatch(..., false) when deciding which ActionAttributeValues to keep, and stop deduping solely by name — preserve distinct variants (e.g., keep actions keyed by their ID or Name+Namespace as well as Name) so getResourceDecision can pick the exact match; update the construction of entitledFQNsToActions to store multiple variants per attrValFQN (not just unique names) and ensure getResourceDecision still consumes the new structure, then add a regression test that creates two registered-resource actions with the same Name but different ID/namespace on the same attribute FQN to verify strict/namespace-specific requests choose the correct variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@service/internal/access/v2/pdp.go`:
- Around line 103-116: The code mutates caller-owned Value objects
(attr.GetValues()) by reusing their pointers in
allEntitleableAttributesByValueFQN and later appending to Value.SubjectMappings;
to fix, create deep copies of each policy.Value (or at minimum copy the Value
struct and reset/clone its SubjectMappings slice) before storing/inspecting them
in allEntitleableAttributesByValueFQN and before any code that appends mappings
(refer to allEntitleableAttributesByValueFQN, attr.GetValues(), and
Value.SubjectMappings), so the original attribute definitions are never
modified; also add a regression test that constructs a legacy PDP then a strict
PDP from the same input (exercise NewJustInTimePDP reuse path) to ensure
mappings are not accumulated across builds.
- Around line 314-345: The current prefilter over
entityRegisteredResourceValue.GetActionAttributeValues collapses actions by
Action.Name into entitledFQNsToActions, losing distinct ID/namespace variants;
change the prefilter to call isRequestedActionMatch(..., false) when deciding
which ActionAttributeValues to keep, and stop deduping solely by name — preserve
distinct variants (e.g., keep actions keyed by their ID or Name+Namespace as
well as Name) so getResourceDecision can pick the exact match; update the
construction of entitledFQNsToActions to store multiple variants per attrValFQN
(not just unique names) and ensure getResourceDecision still consumes the new
structure, then add a regression test that creates two registered-resource
actions with the same Name but different ID/namespace on the same attribute FQN
to verify strict/namespace-specific requests choose the correct variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1045d389-d60a-4d33-94dc-8c2d747a43ac
📒 Files selected for processing (4)
service/internal/access/v2/evaluate.goservice/internal/access/v2/evaluate_test.goservice/internal/access/v2/pdp.goservice/internal/access/v2/pdp_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
elizabethhealy
commented
Apr 2, 2026
| AllowDirectEntitlements bool `mapstructure:"allow_direct_entitlements" json:"allow_direct_entitlements" default:"false"` | ||
|
|
||
| // enforce strict namespaced policy evaluation behavior in access decisioning | ||
| NamespacedPolicy bool `mapstructure:"namespaced_policy" json:"namespaced_policy" default:"false"` |
Member
Author
There was a problem hiding this comment.
this is currently a seperate feature flag under service.authorization.namespaced_policy, should it be seperate? or should we figure out a way to rely on the service.policy.namespaced_policy flag?
maybe namespaced_policy and namespaced_decisioning are seperate features?
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
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