Mirror
/
seaweedfs
mirror of https://github.com/seaweedfs/seaweedfs.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

refactor to test

pull/7519/head
chrislu 2 months ago
parent
b72df84361
commit
fec3088fb1
2 changed files with 87 additions and 151 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 49
      weed/s3api/s3api_bucket_handlers.go
  2. 189
      weed/s3api/s3api_bucket_handlers_test.go

49
weed/s3api/s3api_bucket_handlers.go
View File

@ -70,18 +70,8 @@ func (s3a *S3ApiServer) ListBucketsHandler(w http.ResponseWriter, r *http.Reques
for _, entry := range entries {
if entry.IsDirectory {
// Check ownership: only show buckets owned by this user (unless admin)
if identity != nil && !identity.isAdmin() {
// Use the authenticated identity value directly
authenticatedIdentityId := identity.Name
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != authenticatedIdentityId {
continue
}
if !isBucketVisibleToIdentity(entry, identity) {
continue
}
// Check permissions for each bucket
@ -119,6 +109,41 @@ func (s3a *S3ApiServer) ListBucketsHandler(w http.ResponseWriter, r *http.Reques
writeSuccessResponseXML(w, r, response)
}
// isBucketVisibleToIdentity checks if a bucket entry should be visible to the given identity
// based on ownership rules. Returns true if the bucket should be visible, false otherwise.
//
// Visibility rules:
// - Unauthenticated requests (identity == nil): all buckets visible
// - Admin users: all buckets visible
// - Non-admin users: only buckets they own (matching identity.Name) are visible
// - Buckets without owner metadata are hidden from non-admin users
func isBucketVisibleToIdentity(entry *filer_pb.Entry, identity *Identity) bool {
if !entry.IsDirectory {
return false
}
// Unauthenticated or admin users bypass ownership check
if identity == nil || identity.isAdmin() {
return true
}
// Non-admin users: check ownership
// Use the authenticated identity value directly (cannot be spoofed)
authenticatedIdentityId := identity.Name
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != authenticatedIdentityId {
return false
}
return true
}
func (s3a *S3ApiServer) PutBucketHandler(w http.ResponseWriter, r *http.Request) {
// collect parameters

189
weed/s3api/s3api_bucket_handlers_test.go
View File

@ -380,27 +380,15 @@ func TestListBucketsOwnershipFiltering(t *testing.T) {
entries = append(entries, entry)
}
// Filter entries using the same logic as ListBucketsHandler
// Filter entries using the actual production code
var filteredBuckets []string
for _, entry := range entries {
if entry.IsDirectory {
// Apply ownership filtering logic (lines 67-77 from s3api_bucket_handlers.go)
shouldSkip := false
if tc.requestIdentityId != "" && !tc.requestIsAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != tc.requestIdentityId {
shouldSkip = true
}
}
if !shouldSkip {
filteredBuckets = append(filteredBuckets, entry.Name)
}
var identity *Identity
if tc.requestIdentityId != "" {
identity = mockIdentity(tc.requestIdentityId, tc.requestIsAdmin)
}
if isBucketVisibleToIdentity(entry, identity) {
filteredBuckets = append(filteredBuckets, entry.Name)
}
}
@ -418,6 +406,23 @@ type testBucket struct {
nilExtended bool
}
// mockIdentity creates a mock Identity for testing bucket visibility
func mockIdentity(name string, isAdmin bool) *Identity {
identity := &Identity{
Name: name,
}
if isAdmin {
identity.Credentials = []*Credential{
{
AccessKey: "admin-key",
SecretKey: "admin-secret",
},
}
identity.Actions = []Action{ACTION_ADMIN}
}
return identity
}
// TestListBucketsOwnershipEdgeCases tests edge cases in ownership filtering
func TestListBucketsOwnershipEdgeCases(t *testing.T) {
t.Run("malformed owner id with special characters", func(t *testing.T) {
@ -432,20 +437,12 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := "user@domain.com"
isAdmin := false
identity := mockIdentity("user@domain.com", false)
// Should match exactly even with special characters
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
}
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.False(t, shouldSkip, "Should match owner ID with special characters exactly")
assert.True(t, isVisible, "Should match owner ID with special characters exactly")
})
t.Run("owner id with unicode characters", func(t *testing.T) {
@ -461,19 +458,11 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := unicodeOwnerId
isAdmin := false
identity := mockIdentity(unicodeOwnerId, false)
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
}
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.False(t, shouldSkip, "Should handle unicode owner IDs correctly")
assert.True(t, isVisible, "Should handle unicode owner IDs correctly")
})
t.Run("owner id with binary data", func(t *testing.T) {
@ -488,20 +477,12 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := "normaluser"
isAdmin := false
identity := mockIdentity("normaluser", false)
// Should not panic when converting binary data to string
assert.NotPanics(t, func() {
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
}
assert.True(t, shouldSkip, "Binary owner ID should not match normal user")
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.False(t, isVisible, "Binary owner ID should not match normal user")
})
})
@ -517,23 +498,11 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := "user1"
isAdmin := false
identity := mockIdentity("user1", false)
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.True(t, shouldSkip, "Empty owner ID should be treated as unowned (hidden from non-admins)")
assert.False(t, isVisible, "Empty owner ID should be treated as unowned (hidden from non-admins)")
})
t.Run("nil Extended map safe access", func(t *testing.T) {
@ -546,24 +515,12 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := "user1"
isAdmin := false
identity := mockIdentity("user1", false)
// Should not panic with nil Extended map
assert.NotPanics(t, func() {
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
assert.True(t, shouldSkip, "Nil Extended (no owner) should be hidden from non-admins")
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.False(t, isVisible, "Nil Extended (no owner) should be hidden from non-admins")
})
})
@ -580,20 +537,12 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) {
},
}
requestIdentityId := longOwnerId
isAdmin := false
identity := mockIdentity(longOwnerId, false)
// Should handle very long owner IDs without panic
assert.NotPanics(t, func() {
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
}
assert.False(t, shouldSkip, "Long owner ID should match correctly")
isVisible := isBucketVisibleToIdentity(entry, identity)
assert.True(t, isVisible, "Long owner ID should match correctly")
})
})
}
@ -623,25 +572,12 @@ func TestListBucketsOwnershipWithPermissions(t *testing.T) {
},
}
requestIdentityId := "user1"
isAdmin := false
identity := mockIdentity("user1", false)
// First pass: ownership filtering
var afterOwnershipFilter []*filer_pb.Entry
for _, entry := range entries {
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
if !shouldSkip {
if isBucketVisibleToIdentity(entry, identity) {
afterOwnershipFilter = append(afterOwnershipFilter, entry)
}
}
@ -674,25 +610,12 @@ func TestListBucketsOwnershipWithPermissions(t *testing.T) {
},
}
requestIdentityId := "admin-user"
isAdmin := true
identity := mockIdentity("admin-user", true)
// Admin bypasses ownership check
var afterOwnershipFilter []*filer_pb.Entry
for _, entry := range entries {
shouldSkip := false
if requestIdentityId != "" && !isAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != requestIdentityId {
shouldSkip = true
}
}
if !shouldSkip {
if isBucketVisibleToIdentity(entry, identity) {
afterOwnershipFilter = append(afterOwnershipFilter, entry)
}
}
@ -728,25 +651,13 @@ func TestListBucketsOwnershipCaseSensitivity(t *testing.T) {
for _, tc := range testCases {
t.Run(fmt.Sprintf("identity_%s", tc.requestIdentityId), func(t *testing.T) {
isAdmin := false
shouldSkip := false
if tc.requestIdentityId != "" && !isAdmin {
var bucketOwnerId string
if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok {
bucketOwnerId = string(id)
}
// Skip buckets that have no owner or are owned by someone else
if bucketOwnerId == "" || bucketOwnerId != tc.requestIdentityId {
shouldSkip = true
}
}
identity := mockIdentity(tc.requestIdentityId, false)
isVisible := isBucketVisibleToIdentity(entry, identity)
if tc.shouldMatch {
assert.False(t, shouldSkip, "Identity %s should match (case sensitive)", tc.requestIdentityId)
assert.True(t, isVisible, "Identity %s should match (case sensitive)", tc.requestIdentityId)
} else {
assert.True(t, shouldSkip, "Identity %s should not match (case sensitive)", tc.requestIdentityId)
assert.False(t, isVisible, "Identity %s should not match (case sensitive)", tc.requestIdentityId)
}
})
}

Write Preview
Loading…
Cancel
Save