feat(authz): Namespaced policy in decisioning#3226
feat(authz): Namespaced policy in decisioning#3226elizabethhealy wants to merge 21 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 |
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
|
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}
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
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![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
X-Test Failure Report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
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/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
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
service/authorization/v2/config.go
Outdated
| 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"` |
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?
There was a problem hiding this comment.
Good question. In my mind, they are different settings (require administration of the policy constructs with namespaces, drive entitlement resolution via namespaces), but one feature: namespaced policy. Are there any global/temporary config locations for cross-cutting features like this, or temporary features?
There was a problem hiding this comment.
Also, I'm wondering if to us they're separate features because of service boundaries and intent, but to customers/admins they might really more like key management (crossing KAS and policy) where the real goal is to rely on namespaces everywhere or to not. 🤔
There was a problem hiding this comment.
ya my thought is from a consumer standpoint, theyre probably the same. but then maybe thats something we do in the charts at deployment time? from a dev standpoint i like the separate flags, i think we had something similar with key_managment? like a top level key_managment flag and one within services.kas
|
|
||
| The normative request-action contract supports three request shapes. During evaluation, matching is always applied per namespace context (derived from the rule/value being evaluated), not globally. | ||
|
|
||
| Request-action identity precedence is explicit: |
There was a problem hiding this comment.
Confirming, this is the GetDecisionRequest? If so, the action name has historically been required at the protovalidation level and the id ignored.
There was a problem hiding this comment.
correct. with namespaced decisioning, because we can have multiple actions with the same name, we now allow id/namesapce to specify a namespaced action and use a stricter order to avoid ambiguity:
- action.id first (if provided)
- otherwise action.name
- and if action.namespace is provided, it further scopes the match
so same proto shape, but evaluation is now namespace-aware and deterministic when the action id is sent. ill update the ADR with some clarity on the proto requirements.
adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.md
Outdated
Show resolved
Hide resolved
service/authorization/v2/config.go
Outdated
| 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"` |
There was a problem hiding this comment.
Good question. In my mind, they are different settings (require administration of the policy constructs with namespaces, drive entitlement resolution via namespaces), but one feature: namespaced policy. Are there any global/temporary config locations for cross-cutting features like this, or temporary features?
service/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.go
Outdated
Show resolved
Hide resolved
|
|
||
| if namespacedPolicy { | ||
| ns := sm.GetNamespace() | ||
| if ns == nil || (ns.GetId() == "" && ns.GetFqn() == "") { |
There was a problem hiding this comment.
Just validating - potentially skipping some Subject Mappings here is safe because a user would only be granted less entitlements, therefore failing closed instead of open, right?
There was a problem hiding this comment.
Yep exactly. When namespacedPolicy is enabled, skipping unnamespaced SMs is fail-closed behavior: we only reduce the set of candidate entitlements, which can only keep decision the same or move PERMIT -> DENY, never DENY -> PERMIT.
So this cannot grant extra access; it can only be stricter.
|
|
||
| namespaceName := namespaceNameFromPolicyNamespace(rr.GetNamespace()) | ||
|
|
||
| fullyQualifiedValue := identifier.FullyQualifiedRegisteredResourceValue{ |
There was a problem hiding this comment.
How should these states be handled?
- Auth service flag requires namespacing all the policy. Unnamespaced Registered Resources still exist. Request comes in for
https://reg_res/example_rr_name/value/value. - Auth service flag requires namespacing all the policy. All Registered Resources have been moved into namespaces, like
https://example.com/reg_res/example_rr_name/value/value. Request comes in forhttps://reg_res/example_rr_name/value/value. - Auth service flag is off and therefore does not require namespacing all the policy. A Registered Resource is namespaced, like
https://example.com/reg_res/example_rr_name/value/value. Request comes in forhttps://reg_res/example_rr_name/value/value.
Maybe we need an expected behavior matrix to match against the implementation/tests?
There was a problem hiding this comment.
- in that scenario we don’t reject the RR just because it’s unnamespaced. (do you think we should?)
Flow is: resolve the RR value FQN, evaluate each RR AAV, derive required namespace from the AAV’s attribute value context, in strict mode (EnforceNamespacedEntitlements=true), only allow AAV actions whose namespace matches that required namespace (otherwise fail/deny).
So enforcement is at the AAV/action matching layer, not an upfront “RR must be namespaced” check.
1A: EnforceNamespacedEntitlements=true + namespaced RR FQN request, flow is: resolve RR value by the namespaced RR FQN, evaluate RR AAVs for requested action, derive required namespace from each AAV’s attribute-value context, enforce strict namespace match for action evaluation per AAV, return permit/deny from those namespace-scoped results.
And since RR create/update already enforces AAV namespace consistency with the RR namespace, this path stays aligned.
- assuming they've done the migration/pruning (and the old un-namespaced RRs no longer exist in the db)
Flow: request uses legacy/global RR FQN (https://reg_res/...), all RR data is now namespaced-only, lookup for legacy FQN won’t resolve a registered resource value (bc it doesnt exist), decision returns deny / not-found path (fail-closed), not permit fallback.
2A: if they havent pruned the un-namespaced RRs, then this behaves like 1.
- If https://reg_res/example_rr_name/value/value does not exist in the db (ie migration/pruning has taken place) then fail/deny. If it does still exist in the db (no pruning yet) -- then we match the existing behavior -- PDP resolves RR value using legacy/global FQN key (https://reg_res/...) if present. AAV action matching is name-based legacy behavior (no strict namespace requirement). Rules evaluate as before; decision can permit/deny based on entitlement/rule match.
So: legacy RR requests continue to work in non-strict mode, assuming legacy RR entries exist in memory.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 62-66: The Consequences bullets are stylistically repetitive —
three items start with "🟩 **Good**, because..."; reword at least two of these
to vary sentence openings while preserving meaning (for example start one with
"Provides ..." or "Enables ..." and another with "Allows ..." or "Ensures ..."),
and optionally adjust the negative bullets for parallelism (e.g., change "🟥
**Bad**, because..." to "However, ..." or "Drawback: ...") so the section reads
less repetitive without changing technical content; update the lines containing
the "🟩 **Good**, because" and "🟥 **Bad**, because" bullets accordingly.
In `@service/internal/access/v2/pdp.go`:
- Around line 203-222: The function namespaceNameFromPolicyNamespace silently
ignores parse errors; update it to emit a trace-level log when identifier.Parse
returns an error so failures are observable during rollout: in
namespaceNameFromPolicyNamespace, after calling
identifier.Parse[*identifier.FullyQualifiedAttribute](ns.GetFqn()), if err !=
nil call the appropriate trace logger (e.g., process or package logger
accessible at call site) to log the error and the failing FQN, then continue
returning "" to preserve fail-closed behavior; ensure you reference
namespaceNameFromPolicyNamespace and identifier.Parse in your change.
🪄 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: b0ebccf2-5ff7-4e45-894c-7302717eebf8
📒 Files selected for processing (8)
adr/decisions/2026-03-30-namespaced-subject-mappings-decisioning.mdservice/authorization/v2/authorization.goservice/authorization/v2/config.goservice/internal/access/v2/evaluate.goservice/internal/access/v2/pdp.goservice/internal/subjectmappingbuiltin/subject_mapping_builtin_actions.goservice/internal/subjectmappingbuiltin/subject_mapping_builtin_actions_test.gotests-bdd/cukes/resources/platform.namespaced_policy.template
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
3 participants
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