From a77b14559068370d6c7b9735e575547b431d7566 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 17 Dec 2025 00:09:13 -0800 Subject: [PATCH] fix: ListBuckets returns empty for users with bucket-specific permissions (#7799) * fix: ListBuckets returns empty for users with bucket-specific permissions (#7796) The ListBucketsHandler was using sequential AND logic where ownership check happened before permission check. If a user had 'List:bucketname' permission but didn't own the bucket (different AmzIdentityId or missing owner metadata), the bucket was filtered out before the permission check could run. Changed to OR logic: a bucket is now visible if the user owns it OR has explicit permission to list it. This allows users with bucket-specific permissions like 'List:geoserver' to see buckets they have access to, even if they don't own them. Changes: - Modified ListBucketsHandler to check both ownership and permission, including bucket if either check passes - Renamed isBucketVisibleToIdentity to isBucketOwnedByIdentity for clarity - Added comprehensive tests in TestListBucketsIssue7796 Fixes #7796 * address review comments: optimize permission check and add integration test - Skip permission check if user is already the owner (performance optimization) - Add integration test that simulates the complete handler filtering logic to verify the combination of ownership OR permission check works correctly * add visibility assertions to each sub-test for self-contained verification Each sub-test now verifies the final outcome using isOwner || canList logic, making tests more robust and independently verifiable. --- weed/s3api/s3api_bucket_handlers.go | 60 ++--- weed/s3api/s3api_bucket_handlers_test.go | 267 +++++++++++++++++++++++ 2 files changed, 302 insertions(+), 25 deletions(-) diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 5ff155890..d7f5aa7b8 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -73,28 +73,35 @@ func (s3a *S3ApiServer) ListBucketsHandler(w http.ResponseWriter, r *http.Reques var listBuckets ListAllMyBucketsList for _, entry := range entries { if entry.IsDirectory { - // Check ownership: only show buckets owned by this user (unless admin) - if !isBucketVisibleToIdentity(entry, identity) { + // Unauthenticated users should not see any buckets + if identity == nil { continue } - // Check permissions for each bucket - if identity != nil { + // Check if bucket should be visible to this identity + // A bucket is visible if the user owns it OR has explicit permission to list it + isOwner := isBucketOwnedByIdentity(entry, identity) + + // Skip permission check if user is already the owner (optimization) + if !isOwner { + hasPermission := false + // Check permissions for each bucket // For JWT-authenticated users, use IAM authorization sessionToken := r.Header.Get("X-SeaweedFS-Session-Token") if s3a.iam.iamIntegration != nil && sessionToken != "" { // Use IAM authorization for JWT users errCode := s3a.iam.authorizeWithIAM(r, identity, s3_constants.ACTION_LIST, entry.Name, "") - if errCode != s3err.ErrNone { - continue - } + hasPermission = (errCode == s3err.ErrNone) } else { // Use legacy authorization for non-JWT users - if !identity.canDo(s3_constants.ACTION_LIST, entry.Name, "") { - continue - } + hasPermission = identity.canDo(s3_constants.ACTION_LIST, entry.Name, "") + } + + if !hasPermission { + continue } } + listBuckets.Bucket = append(listBuckets.Bucket, ListAllMyBucketsEntry{ Name: entry.Name, CreationDate: time.Unix(entry.Attributes.Crtime, 0).UTC(), @@ -113,40 +120,35 @@ 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. +// isBucketOwnedByIdentity checks if a bucket entry is owned by the given identity. +// Returns true if the identity owns the bucket, false otherwise. // -// Visibility rules: -// - Unauthenticated requests (identity == nil): no 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 { +// Ownership rules: +// - Admin users: considered owners of all buckets +// - Non-admin users: own buckets where AmzIdentityId matches identity.Name +// - Buckets without owner metadata are not owned by anyone (except admins) +func isBucketOwnedByIdentity(entry *filer_pb.Entry, identity *Identity) bool { if !entry.IsDirectory { return false } - // Unauthenticated users should not see any buckets (standard S3 behavior) if identity == nil { return false } - // Admin users bypass ownership check + // Admin users are considered owners of all buckets if identity.isAdmin() { return true } - // Non-admin users with no name cannot own or see buckets. + // Non-admin users with no name cannot own buckets. // This prevents misconfigured identities from matching buckets with empty owner IDs. if identity.Name == "" { return false } - // Non-admin users: check ownership - // Use the authenticated identity value directly (cannot be spoofed) + // Check ownership via AmzIdentityId metadata id, ok := entry.Extended[s3_constants.AmzIdentityId] - // Skip buckets that are not owned by the current user. - // Buckets without an owner are also skipped. if !ok || string(id) != identity.Name { return false } @@ -154,6 +156,14 @@ func isBucketVisibleToIdentity(entry *filer_pb.Entry, identity *Identity) bool { return true } +// isBucketVisibleToIdentity is kept for backward compatibility with tests. +// It checks if a bucket should be visible based on ownership only. +// Deprecated: Use isBucketOwnedByIdentity instead. The ListBucketsHandler +// now uses OR logic: a bucket is visible if user owns it OR has List permission. +func isBucketVisibleToIdentity(entry *filer_pb.Entry, identity *Identity) bool { + return isBucketOwnedByIdentity(entry, identity) +} + func (s3a *S3ApiServer) PutBucketHandler(w http.ResponseWriter, r *http.Request) { // collect parameters diff --git a/weed/s3api/s3api_bucket_handlers_test.go b/weed/s3api/s3api_bucket_handlers_test.go index c2870b15e..977dc6926 100644 --- a/weed/s3api/s3api_bucket_handlers_test.go +++ b/weed/s3api/s3api_bucket_handlers_test.go @@ -776,3 +776,270 @@ func TestListBucketsIssue7647(t *testing.T) { assert.False(t, isVisible, "Non-admin should not see buckets without owner metadata") }) } + +// TestListBucketsIssue7796 reproduces and verifies the fix for issue #7796 +// where a user with bucket-specific List permission (e.g., "List:geoserver") +// couldn't see buckets they have access to but don't own +func TestListBucketsIssue7796(t *testing.T) { + t.Run("user with bucket-specific List permission can see bucket they don't own", func(t *testing.T) { + // Simulate the exact scenario from issue #7796: + // User "geoserver" with ["List:geoserver", "Read:geoserver", "Write:geoserver", ...] permissions + // But the bucket "geoserver" was created by a different user (e.g., admin) + + geoserverIdentity := &Identity{ + Name: "geoserver", + Credentials: []*Credential{ + { + AccessKey: "geoserver", + SecretKey: "secret", + }, + }, + Actions: []Action{ + Action("List:geoserver"), + Action("Read:geoserver"), + Action("Write:geoserver"), + Action("Admin:geoserver"), + Action("List:geoserver-ttl"), + Action("Read:geoserver-ttl"), + Action("Write:geoserver-ttl"), + }, + } + + // Bucket was created by admin, not by geoserver user + geoserverBucket := &filer_pb.Entry{ + Name: "geoserver", + IsDirectory: true, + Extended: map[string][]byte{ + s3_constants.AmzIdentityId: []byte("admin"), // Different owner + }, + Attributes: &filer_pb.FuseAttributes{ + Crtime: time.Now().Unix(), + Mtime: time.Now().Unix(), + }, + } + + // Test ownership check - should return false (not owned by geoserver) + isOwner := isBucketOwnedByIdentity(geoserverBucket, geoserverIdentity) + assert.False(t, isOwner, "geoserver user should not be owner of bucket created by admin") + + // Test permission check - should return true (has List:geoserver permission) + canList := geoserverIdentity.canDo(s3_constants.ACTION_LIST, "geoserver", "") + assert.True(t, canList, "geoserver user with List:geoserver should be able to list geoserver bucket") + + // Verify the combined visibility logic: ownership OR permission + isVisible := isOwner || canList + assert.True(t, isVisible, "Bucket should be visible due to permission (even though not owner)") + }) + + t.Run("user with bucket-specific permission sees bucket without owner metadata", func(t *testing.T) { + // Bucket exists but has no owner metadata (legacy bucket or created before ownership tracking) + + geoserverIdentity := &Identity{ + Name: "geoserver", + Actions: []Action{ + Action("List:geoserver"), + Action("Read:geoserver"), + }, + } + + bucketWithoutOwner := &filer_pb.Entry{ + Name: "geoserver", + IsDirectory: true, + Extended: map[string][]byte{}, // No owner metadata + Attributes: &filer_pb.FuseAttributes{ + Crtime: time.Now().Unix(), + }, + } + + // Not owner (no owner metadata) + isOwner := isBucketOwnedByIdentity(bucketWithoutOwner, geoserverIdentity) + assert.False(t, isOwner, "No owner metadata means not owned") + + // But has permission + canList := geoserverIdentity.canDo(s3_constants.ACTION_LIST, "geoserver", "") + assert.True(t, canList, "Has explicit List:geoserver permission") + + // Verify the combined visibility logic: ownership OR permission + isVisible := isOwner || canList + assert.True(t, isVisible, "Bucket should be visible due to permission (even without owner metadata)") + }) + + t.Run("user cannot see bucket they neither own nor have permission for", func(t *testing.T) { + // User has no ownership and no permission for the bucket + + geoserverIdentity := &Identity{ + Name: "geoserver", + Actions: []Action{ + Action("List:geoserver"), + Action("Read:geoserver"), + }, + } + + otherBucket := &filer_pb.Entry{ + Name: "otherbucket", + IsDirectory: true, + Extended: map[string][]byte{ + s3_constants.AmzIdentityId: []byte("admin"), + }, + Attributes: &filer_pb.FuseAttributes{ + Crtime: time.Now().Unix(), + }, + } + + // Not owner + isOwner := isBucketOwnedByIdentity(otherBucket, geoserverIdentity) + assert.False(t, isOwner, "geoserver doesn't own otherbucket") + + // No permission for this bucket + canList := geoserverIdentity.canDo(s3_constants.ACTION_LIST, "otherbucket", "") + assert.False(t, canList, "geoserver has no List permission for otherbucket") + + // Verify the combined visibility logic: ownership OR permission + isVisible := isOwner || canList + assert.False(t, isVisible, "Bucket should NOT be visible (neither owner nor has permission)") + }) + + t.Run("user with wildcard permission sees matching buckets", func(t *testing.T) { + // User has "List:geo*" permission - should see any bucket starting with "geo" + + geoIdentity := &Identity{ + Name: "geouser", + Actions: []Action{ + Action("List:geo*"), + Action("Read:geo*"), + }, + } + + geoBucket := &filer_pb.Entry{ + Name: "geoserver", + IsDirectory: true, + Extended: map[string][]byte{ + s3_constants.AmzIdentityId: []byte("admin"), + }, + Attributes: &filer_pb.FuseAttributes{ + Crtime: time.Now().Unix(), + }, + } + + geoTTLBucket := &filer_pb.Entry{ + Name: "geoserver-ttl", + IsDirectory: true, + Extended: map[string][]byte{ + s3_constants.AmzIdentityId: []byte("admin"), + }, + Attributes: &filer_pb.FuseAttributes{ + Crtime: time.Now().Unix(), + }, + } + + // Not owner of either bucket + isOwnerGeo := isBucketOwnedByIdentity(geoBucket, geoIdentity) + isOwnerGeoTTL := isBucketOwnedByIdentity(geoTTLBucket, geoIdentity) + assert.False(t, isOwnerGeo) + assert.False(t, isOwnerGeoTTL) + + // But has permission via wildcard + canListGeo := geoIdentity.canDo(s3_constants.ACTION_LIST, "geoserver", "") + canListGeoTTL := geoIdentity.canDo(s3_constants.ACTION_LIST, "geoserver-ttl", "") + assert.True(t, canListGeo) + assert.True(t, canListGeoTTL) + + // Verify the combined visibility logic for matching buckets + assert.True(t, isOwnerGeo || canListGeo, "geoserver bucket should be visible via wildcard permission") + assert.True(t, isOwnerGeoTTL || canListGeoTTL, "geoserver-ttl bucket should be visible via wildcard permission") + + // Should NOT have permission for unrelated buckets + canListOther := geoIdentity.canDo(s3_constants.ACTION_LIST, "otherbucket", "") + assert.False(t, canListOther, "No permission for otherbucket") + assert.False(t, false || canListOther, "otherbucket should NOT be visible (no ownership, no permission)") + }) + + t.Run("integration test: complete handler filtering logic", func(t *testing.T) { + // This test simulates the complete filtering logic as used in ListBucketsHandler + // to verify that the combination of ownership OR permission check works correctly + + // User "geoserver" with bucket-specific permissions (same as issue #7796) + geoserverIdentity := &Identity{ + Name: "geoserver", + Credentials: []*Credential{ + {AccessKey: "geoserver", SecretKey: "secret"}, + }, + Actions: []Action{ + Action("List:geoserver"), + Action("Read:geoserver"), + Action("Write:geoserver"), + Action("Admin:geoserver"), + Action("List:geoserver-ttl"), + Action("Read:geoserver-ttl"), + Action("Write:geoserver-ttl"), + }, + } + + // Create test buckets with various ownership scenarios + buckets := []*filer_pb.Entry{ + { + // Bucket owned by admin but geoserver has permission + Name: "geoserver", + IsDirectory: true, + Extended: map[string][]byte{s3_constants.AmzIdentityId: []byte("admin")}, + Attributes: &filer_pb.FuseAttributes{Crtime: time.Now().Unix()}, + }, + { + // Bucket with no owner but geoserver has permission + Name: "geoserver-ttl", + IsDirectory: true, + Extended: map[string][]byte{}, + Attributes: &filer_pb.FuseAttributes{Crtime: time.Now().Unix()}, + }, + { + // Bucket owned by geoserver (should be visible via ownership) + Name: "geoserver-owned", + IsDirectory: true, + Extended: map[string][]byte{s3_constants.AmzIdentityId: []byte("geoserver")}, + Attributes: &filer_pb.FuseAttributes{Crtime: time.Now().Unix()}, + }, + { + // Bucket owned by someone else, no permission for geoserver + Name: "otherbucket", + IsDirectory: true, + Extended: map[string][]byte{s3_constants.AmzIdentityId: []byte("otheruser")}, + Attributes: &filer_pb.FuseAttributes{Crtime: time.Now().Unix()}, + }, + } + + // Simulate the exact filtering logic from ListBucketsHandler + var visibleBuckets []string + for _, entry := range buckets { + if !entry.IsDirectory { + continue + } + + // Check ownership + isOwner := isBucketOwnedByIdentity(entry, geoserverIdentity) + + // Skip permission check if user is already the owner (optimization) + if !isOwner { + // Check permission + hasPermission := geoserverIdentity.canDo(s3_constants.ACTION_LIST, entry.Name, "") + if !hasPermission { + continue + } + } + + visibleBuckets = append(visibleBuckets, entry.Name) + } + + // Expected: geoserver should see: + // - "geoserver" (has List:geoserver permission, even though owned by admin) + // - "geoserver-ttl" (has List:geoserver-ttl permission, even though no owner) + // - "geoserver-owned" (owns this bucket) + // NOT "otherbucket" (neither owns nor has permission) + expectedBuckets := []string{"geoserver", "geoserver-ttl", "geoserver-owned"} + assert.ElementsMatch(t, expectedBuckets, visibleBuckets, + "geoserver should see buckets they own OR have permission for") + + // Verify "otherbucket" is NOT in the list + assert.NotContains(t, visibleBuckets, "otherbucket", + "geoserver should NOT see buckets they neither own nor have permission for") + }) +}