object: clobber bucket policy on modify instead of merging

ModifyBucketPolicy merged the caller's statement into the policy fetched
from the bucket, matching by SID. Besides a missing match flag that
appended a duplicate when a SID matched, this preserved any pre-existing
statements on the bucket and reapplied them on every reconcile.

Overwrite the policy with the provided statements instead, so a managed
bucket always ends up with exactly the intended policy and cannot retain
unexpected statements.

Signed-off-by: Artem Muterko <artem@sopho.tech>
(cherry picked from commit 321911b96c)
This commit is contained in:
Artem Muterko authored and Mergify committed 2026-07-21 20:59:48 +00:00
1 parent 7fa1fa5ac4
commit 5a3eaedbf4
2 files changed
+56 -14

No files matched your search

+3 -14
View File
@@ -194,21 +194,10 @@ func (s *S3Agent) GetBucketPolicy(bucket string) (*BucketPolicy, error) {
return policy, nil
}
// ModifyBucketPolicy new and old statement SIDs and overwrites on a match.
// This allows users to Get, modify, and Replace existing statements as well as
// add new ones.
// ModifyBucketPolicy replaces the policy's statements with the given ones,
// rather than merging them into the existing statements by SID.
func (bp *BucketPolicy) ModifyBucketPolicy(ps ...PolicyStatement) *BucketPolicy {
for _, newP := range ps {
var match bool
for j, oldP := range bp.Statement {
if newP.Sid == oldP.Sid {
bp.Statement[j] = newP
}
}
if !match {
bp.Statement = append(bp.Statement, newP)
}
}
bp.Statement = append([]PolicyStatement{}, ps...)
return bp
}
+53
View File
@@ -0,0 +1,53 @@
/*
Copyright 2026 The Rook Authors. All rights reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package object
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestModifyBucketPolicy(t *testing.T) {
t.Run("does not duplicate an existing SID", func(t *testing.T) {
bp := NewBucketPolicy(*NewPolicyStatement().WithSID("s1").Allows().Actions(GetObject))
bp.ModifyBucketPolicy(*NewPolicyStatement().WithSID("s1").Denies().Actions(PutObject))
assert.Len(t, bp.Statement, 1)
assert.Equal(t, effectDeny, bp.Statement[0].Effect)
})
t.Run("replaces existing statements instead of merging", func(t *testing.T) {
bp := NewBucketPolicy(
*NewPolicyStatement().WithSID("s1").Allows().Actions(GetObject),
*NewPolicyStatement().WithSID("s2").Allows().Actions(GetObject),
)
bp.ModifyBucketPolicy(*NewPolicyStatement().WithSID("s3").Denies().Actions(PutObject))
assert.Len(t, bp.Statement, 1)
assert.Equal(t, "s3", bp.Statement[0].Sid)
})
t.Run("keeps all passed statements in order", func(t *testing.T) {
bp := NewBucketPolicy(*NewPolicyStatement().WithSID("old").Allows().Actions(GetObject))
bp.ModifyBucketPolicy(
*NewPolicyStatement().WithSID("a").Allows().Actions(GetObject),
*NewPolicyStatement().WithSID("b").Denies().Actions(PutObject),
)
assert.Len(t, bp.Statement, 2)
assert.Equal(t, "a", bp.Statement[0].Sid)
assert.Equal(t, "b", bp.Statement[1].Sid)
})
}