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.
| 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.
| 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.
1 participant
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