A policy reports which resources and values it covers - #254
Conversation
📝 WalkthroughWalkthroughThe change adds policy APIs for extracting condition values and S3 Tables resource patterns. It adds condition-function helpers, action-key lookup, exported result types, and table-driven tests for filtering, allow/deny effects, unconstrained conditions, and resource patterns. ChangesPolicy condition inspection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Policy
participant PolicyStatements
participant Functions
participant ConditionValueSet
Caller->>Policy: ConditionValues(resource, actions, key)
Policy->>PolicyStatements: Filter by resource and action
Policy->>Functions: Extract values for key
Functions-->>Policy: Grouped condition values
Policy->>ConditionValueSet: Separate allow and deny results
ConditionValueSet-->>Caller: Per-action condition values
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@policy/condition_values_test.go`:
- Around line 242-265: The table-driven test around wantAllow currently ignores
unexpected actions in got. Update the validation to compare the complete action
set, or explicitly fail when got contains an action absent from test.wantAllow,
while preserving the existing allow and deny pattern checks for expected
actions.
In `@policy/policy.go`:
- Around line 274-285: Update the loop that builds constrained values from
functions.ValuesByKey(key) so encountering any set-qualified or non-allow-list
condition immediately makes the result unconstrained, rather than skipping it
and retaining values from other functions. Preserve the existing value
aggregation for keys containing only supported allow-list conditions, and add a
test covering mixed operators such as StringEquals with StringNotEquals.
- Around line 209-254: Update the NotResource-only handling in the
policy-building flow to append ResourceARNAll whenever statement.NotResources is
non-empty, regardless of whether its entries are table resources; remove or stop
using excludesTableResource for this decision. Add a test covering a NotResource
containing only a regular S3 ARN and verify the statement produces the wildcard
table-resource candidate.
In `@policy/table-action.go`:
- Around line 482-484: Update TableActionConditionKeys to return an isolated
copy of the condition.KeySet rather than the shared map stored in
tableActionConditionKeyMap. Clone each key into a new map before returning so
callers can mutate their result without changing global state or racing with
other callers.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0922f9a-b8a4-46cf-b2be-b0cda82e198d
📒 Files selected for processing (8)
policy/condition/func.gopolicy/condition/func_test.gopolicy/condition/name.gopolicy/condition/stringfunc_test.gopolicy/condition_values_test.gopolicy/policy.gopolicy/table-action.gopolicy/table-action_test.go
| for action, want := range test.wantAllow { | ||
| entry := got[action] | ||
| if entry == nil { | ||
| t.Fatalf("%s: no entry", action) | ||
| } | ||
| allow := entry.Allow.ToSlice() | ||
| slices.Sort(allow) | ||
| slices.Sort(want) | ||
| if !slices.Equal(allow, want) { | ||
| t.Errorf("%s allow = %v, want %v", action, allow, want) | ||
| } | ||
| } | ||
| for action, entry := range got { | ||
| deny := entry.Deny.ToSlice() | ||
| slices.Sort(deny) | ||
| want := test.wantDeny[action] | ||
| slices.Sort(want) | ||
| if len(deny) == 0 && len(want) == 0 { | ||
| continue | ||
| } | ||
| if !slices.Equal(deny, want) { | ||
| t.Errorf("%s deny = %v, want %v", action, deny, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unexpected action entries.
The test checks only actions in wantAllow. It passes if TableResourcePatterns incorrectly adds an Allow pattern for another action. Compare the complete action set, or fail when got contains an action absent from wantAllow.
As per coding guidelines, table-driven policy tests must cover edge cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policy/condition_values_test.go` around lines 242 - 265, The table-driven
test around wantAllow currently ignores unexpected actions in got. Update the
validation to compare the complete action set, or explicitly fail when got
contains an action absent from test.wantAllow, while preserving the existing
allow and deny pattern checks for expected actions.
Source: Coding guidelines
| // A statement naming only NotResource reaches every resource it does not | ||
| // exclude, so it names them all rather than none. | ||
| if len(statement.Resources) == 0 && excludesTableResource(statement.NotResources) { | ||
| patterns = append(patterns, ResourceARNAll.String()) | ||
| } | ||
| if len(patterns) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| for _, action := range actions { | ||
| if len(statement.Actions) > 0 && !statement.Actions.Match(action) { | ||
| continue | ||
| } | ||
| if statement.NotActions.Match(action) { | ||
| continue | ||
| } | ||
| if byAction == nil { | ||
| byAction = make(map[Action]*ResourcePatternSet, len(actions)) | ||
| } | ||
| entry := byAction[action] | ||
| if entry == nil { | ||
| entry = &ResourcePatternSet{Allow: set.NewStringSet(), Deny: set.NewStringSet()} | ||
| byAction[action] = entry | ||
| } | ||
| target := entry.Allow | ||
| if statement.Effect != Allow { | ||
| target = entry.Deny | ||
| } | ||
| for _, pattern := range patterns { | ||
| target.Add(pattern) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return byAction | ||
| } | ||
|
|
||
| // excludesTableResource reports whether a NotResource set excludes any S3 Tables | ||
| // resource, meaning the statement reaches the remaining ones. | ||
| func excludesTableResource(notResources ResourceSet) bool { | ||
| for resource := range notResources { | ||
| if resource.isTable() { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include table resources for every NotResource-only statement.
If NotResource contains only a regular S3 ARN, it excludes no S3 Tables ARN. The statement still applies to every S3 Tables resource. The current excludesTableResource check drops that statement and hides valid candidates.
Use the presence of NotResource, not its resource type, to report "*". Add a non-table NotResource test case.
Proposed fix
- if len(statement.Resources) == 0 && excludesTableResource(statement.NotResources) {
+ if len(statement.Resources) == 0 && len(statement.NotResources) > 0 {
patterns = append(patterns, ResourceARNAll.String())
}
@@
-// excludesTableResource reports whether a NotResource set excludes any S3 Tables
-// resource, meaning the statement reaches the remaining ones.
-func excludesTableResource(notResources ResourceSet) bool {
- for resource := range notResources {
- if resource.isTable() {
- return true
- }
- }
- return false
-}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policy/policy.go` around lines 209 - 254, Update the NotResource-only
handling in the policy-building flow to append ResourceARNAll whenever
statement.NotResources is non-empty, regardless of whether its entries are table
resources; remove or stop using excludesTableResource for this decision. Add a
test covering a NotResource containing only a regular S3 ARN and verify the
statement produces the wildcard table-resource candidate.
There was a problem hiding this comment.
@rraulinio, thanks for confirming. The finding remains open until the NotResource-only handling includes the wildcard S3 Tables resource candidate.
🐇
You are interacting with an AI system.
| for name, vs := range functions.ValuesByKey(key) { | ||
| // A set qualifier such as "ForAllValues:" holds when the request carries no | ||
| // value for the key, so the listed values are not the reachable set. | ||
| if strings.ContainsRune(name, ':') { | ||
| continue | ||
| } | ||
| if !condition.IsAllowList(name) { | ||
| continue | ||
| } | ||
| constrained = true | ||
| values = append(values, vs...) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Mark mixed condition forms as unconstrained.
The loop ignores an unsupported or set-qualified function and still returns values from another function on the same key. For example, StringEquals plus StringNotEquals returns a partial value set with AllowAll unset. This conflicts with the API contract that an unsupported form must not narrow the result.
Return constrained == false when any condition name for the key is unsupported or qualified. Add a mixed-operator test.
Proposed fix
for name, vs := range functions.ValuesByKey(key) {
- if strings.ContainsRune(name, ':') {
- continue
- }
- if !condition.IsAllowList(name) {
- continue
+ if strings.ContainsRune(name, ':') || !condition.IsAllowList(name) {
+ return nil, false
}
constrained = true
values = append(values, vs...)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for name, vs := range functions.ValuesByKey(key) { | |
| // A set qualifier such as "ForAllValues:" holds when the request carries no | |
| // value for the key, so the listed values are not the reachable set. | |
| if strings.ContainsRune(name, ':') { | |
| continue | |
| } | |
| if !condition.IsAllowList(name) { | |
| continue | |
| } | |
| constrained = true | |
| values = append(values, vs...) | |
| } | |
| for name, vs := range functions.ValuesByKey(key) { | |
| // A set qualifier such as "ForAllValues:" holds when the request carries no | |
| // value for the key, so the listed values are not the reachable set. | |
| if strings.ContainsRune(name, ':') || !condition.IsAllowList(name) { | |
| return nil, false | |
| } | |
| constrained = true | |
| values = append(values, vs...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policy/policy.go` around lines 274 - 285, Update the loop that builds
constrained values from functions.ValuesByKey(key) so encountering any
set-qualified or non-allow-list condition immediately makes the result
unconstrained, rather than skipping it and retaining values from other
functions. Preserve the existing value aggregation for keys containing only
supported allow-list conditions, and add a test covering mixed operators such as
StringEquals with StringNotEquals.
There was a problem hiding this comment.
@rraulinio, thanks for confirming.
🐇
You are interacting with an AI system.
| func TableActionConditionKeys(action TableAction) condition.KeySet { | ||
| return tableActionConditionKeyMap[Action(action)] | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'type KeySet|func \(.*KeySet.*\) (Add|Delete|Remove|Clone)' policy/condition
rg -n -C 3 'TableActionConditionKeys|tableActionConditionKeyMap' policyRepository: minio/pkg
Length of output: 12839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- policy/condition/key.go relevant definitions ---\n'
sed -n '1,220p' policy/condition/key.go | cat -n
printf '\n--- table-action.go relevant helper/map construction ---\n'
sed -n '360,485p' policy/table-action.go | cat -n
printf '\n--- statement.go relevant call site ---\n'
sed -n '280,305p' policy/statement.go | cat -nRepository: minio/pkg
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- policy/condition/key.go relevant definitions ---'
sed -n '1,220p' policy/condition/key.go | cat -n
printf '%s\n' ''
printf '%s\n' '--- table-action.go relevant helper/map construction ---'
sed -n '360,485p' policy/table-action.go | cat -n
printf '%s\n' ''
printf '%s\n' '--- statement.go relevant call site ---'
sed -n '280,305p' policy/statement.go | cat -nRepository: minio/pkg
Length of output: 16207
Return an isolated key set from TableActionConditionKeys.
condition.KeySet is map[Key]struct{} and KeySet.Add mutates the map. This returns the map stored in tableActionConditionKeyMap, so callers can change shared global state and concurrent callers can race. Return an unexported wrapper, clone the map before returning, or otherwise avoid exposing writable state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policy/table-action.go` around lines 482 - 484, Update
TableActionConditionKeys to return an isolated copy of the condition.KeySet
rather than the shared map stored in tableActionConditionKeyMap. Clone each key
into a new map before returning so callers can mutate their result without
changing global state or racing with other callers.
There was a problem hiding this comment.
@rraulinio, acknowledged. TableActionConditionKeys should return an isolated condition.KeySet.
🐇
You are interacting with an AI system.
There was a problem hiding this comment.
Pull request overview
This PR adds introspection APIs to the policy package so callers can determine which S3 Tables resources and condition values a policy “covers” (for pre-filtering candidate resources before doing full authorization checks), alongside supporting condition-operator recognition and related unit tests.
Changes:
- Add
Policy.ConditionValuesto extract per-action allow/deny value sets for a specific condition key on a resource. - Add
Policy.TableResourcePatternsto report per-action S3 Tables resource patterns (including NotResource-only semantics). - Export
TableActionConditionKeysand add condition helpers/tests (Functions.ValuesByKey,condition.IsAllowList, new unit tests).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| policy/table-action.go | Exposes supported condition keys per table action via a new exported helper. |
| policy/table-action_test.go | Updates tests to use the exported helper instead of direct map access. |
| policy/policy.go | Adds policy introspection APIs for condition values and S3 Tables resource patterns. |
| policy/condition/name.go | Adds helper to classify condition operators as allow-list style. |
| policy/condition/func.go | Adds Functions.ValuesByKey to extract literal values by key/operator name. |
| policy/condition/func_test.go | Adds unit tests for the new ValuesByKey behavior. |
| policy/condition/stringfunc_test.go | Formatting-only adjustments from gofmt/goimports. |
| policy/condition_values_test.go | New tests covering ConditionValues and TableResourcePatterns. |
Suppressed comments (1)
policy/policy.go:281
- interpretableValues() can incorrectly narrow the reachable set when the same condition key is constrained by both an allow-list operator (StringEquals/StringLike) and any other operator (e.g. StringNotEquals). The current loop simply ignores non-allow-list operators, but the function comment and intended behavior indicate that the presence of any uninterpretable/qualified operator should make the key unconstrained (to avoid hiding permitted resources).
if strings.ContainsRune(name, ':') {
continue
}
if !condition.IsAllowList(name) {
continue
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if len(statement.Resources) > 0 && !statement.Resources.MatchResource(resource) { | ||
| continue | ||
| } | ||
| if statement.NotResources.MatchResource(resource) { | ||
| continue | ||
| } |
| // ConditionValues reports the values of key that this policy names for each of | ||
| // actions on resource,so callers can compose Allow and Deny | ||
| // across several policies. |
| func TableActionConditionKeys(action TableAction) condition.KeySet { | ||
| return tableActionConditionKeyMap[Action(action)] | ||
| } |
| func TableActionConditionKeys(action TableAction) condition.KeySet { | ||
| return tableActionConditionKeyMap[Action(action)] | ||
| } |
| // A statement naming only NotResource reaches every resource it does not | ||
| // exclude, so it names them all rather than none. | ||
| if len(statement.Resources) == 0 && excludesTableResource(statement.NotResources) { | ||
| patterns = append(patterns, ResourceARNAll.String()) | ||
| } | ||
| if len(patterns) == 0 { | ||
| continue | ||
| } | ||
|
|
||
| for _, action := range actions { | ||
| if len(statement.Actions) > 0 && !statement.Actions.Match(action) { | ||
| continue | ||
| } | ||
| if statement.NotActions.Match(action) { | ||
| continue | ||
| } | ||
| if byAction == nil { | ||
| byAction = make(map[Action]*ResourcePatternSet, len(actions)) | ||
| } | ||
| entry := byAction[action] | ||
| if entry == nil { | ||
| entry = &ResourcePatternSet{Allow: set.NewStringSet(), Deny: set.NewStringSet()} | ||
| byAction[action] = entry | ||
| } | ||
| target := entry.Allow | ||
| if statement.Effect != Allow { | ||
| target = entry.Deny | ||
| } | ||
| for _, pattern := range patterns { | ||
| target.Add(pattern) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return byAction | ||
| } | ||
|
|
||
| // excludesTableResource reports whether a NotResource set excludes any S3 Tables | ||
| // resource, meaning the statement reaches the remaining ones. | ||
| func excludesTableResource(notResources ResourceSet) bool { | ||
| for resource := range notResources { | ||
| if resource.isTable() { | ||
| return true | ||
| } | ||
| } | ||
| return false |
| for name, vs := range functions.ValuesByKey(key) { | ||
| // A set qualifier such as "ForAllValues:" holds when the request carries no | ||
| // value for the key, so the listed values are not the reachable set. | ||
| if strings.ContainsRune(name, ':') { | ||
| continue | ||
| } | ||
| if !condition.IsAllowList(name) { | ||
| continue | ||
| } | ||
| constrained = true | ||
| values = append(values, vs...) | ||
| } |
| patterns := make([]string, 0, len(statement.Resources)) | ||
| for resource := range statement.Resources { | ||
| if resource.isTable() { | ||
| patterns = append(patterns, resource.Pattern) |
There was a problem hiding this comment.
Preserve whether wildcard patterns still require authorization
TableResourcePatterns discards the statement's conditions, and the NotResource path also approximates the result as "*". These approximate wildcards are indistinguishable from an unconditional wildcard grant.
The linked AIStor consumer treats any wildcard as allWarehouses and skips per-warehouse authorization. For example, an Allow s3tables:ListTables on bucket/* restricted by aws:SourceIp still returns bucket/* when the request is outside the permitted CIDR. AIStor then returns every warehouse without checking the failed condition. An Allow with NotResource: bucket/wh2 similarly returns "*" and loses the wh2 exclusion.
Should either preserve whether a wildcard is exact/unconditional, or require the caller to authorize every registry warehouse before taking the unfiltered wildcard path.
| if statement.Effect != Allow { | ||
| target, all = entry.Deny, &entry.DenyAll | ||
| } | ||
| if !constrained { |
There was a problem hiding this comment.
Do not turn an inexact conditional Deny into DenyAll
For a Deny using an unsupported or negated operator, !constrained currently sets DenyAll. The linked consumer removes that action before performing final authorization, which can hide values that are genuinely allowed.
For example, an Allow for namespace admin combined with Deny StringNotEquals admin permits admin under normal policy evaluation, but this method returns DenyAll, so admin is removed from the candidate set.
Unsupported or otherwise inexact Deny conditions must not narrow candidate discovery. They should be ignored for narrowing or explicitly marked as requiring final evaluation, rather than represented as an unconditional deny.
| byAction[action] = entry | ||
| } | ||
| target := entry.Allow | ||
| if statement.Effect != Allow { |
There was a problem hiding this comment.
Do not report conditioned resource Denies as unconditional exclusions
TableResourcePatterns places every Deny resource into ResourcePatternSet.Deny without preserving the statement's conditions. The linked consumer subtracts exact Allow/Deny pattern matches before final authorization.
For example, an unconditional Allow on bucket/wh1 plus a Deny on the same resource only when s3tables:namespace == private still permits access to public namespaces. This method nevertheless returns identical Allow and Deny patterns, causing wh1 to be removed entirely.
Only unconditional Denies can safely be subtracted this way. Conditioned Denies need to remain candidates for final policy evaluation or be represented as inexact.
|
(sorry, somehow only saw the first file - and yes ValuesByKey is fine :D ) |
PR description:
needed for https://github.com/miniohq/aistor/pull/6595
What this does
Adds a new way to ask a policy document a question it couldn't answer
before: instead of just "is this one specific warehouse allowed?", it
before being shown to a user — this is just a faster way to narrow
down the list of candidates first.
(rather than what it does) was being read backwards, which could
incorrectly deny access that should have been allowed.
the request actually provides one) was being misread as a strict
allow-list, which could have hidden resources a user should
actually be able to see.
included in this diff — no behavior changes there.
How this was tested
Summary by CodeRabbit
New Features
Tests