diff --git a/weed/s3api/s3api_bucket_handlers.go b/weed/s3api/s3api_bucket_handlers.go index 42e351259..13ea70938 100644 --- a/weed/s3api/s3api_bucket_handlers.go +++ b/weed/s3api/s3api_bucket_handlers.go @@ -66,10 +66,14 @@ func (s3a *S3ApiServer) ListBucketsHandler(w http.ResponseWriter, r *http.Reques if entry.IsDirectory { // Check ownership: only show buckets owned by this user (unless admin) if identity != nil && identityId != "" && !identity.isAdmin() { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != identityId { - continue - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != identityId { + continue } } diff --git a/weed/s3api/s3api_bucket_handlers_test.go b/weed/s3api/s3api_bucket_handlers_test.go index 0b6f3657c..b61408aa2 100644 --- a/weed/s3api/s3api_bucket_handlers_test.go +++ b/weed/s3api/s3api_bucket_handlers_test.go @@ -287,18 +287,18 @@ func TestListBucketsOwnershipFiltering(t *testing.T) { description: "Admin should see all buckets regardless of owner", }, { - name: "buckets without owner are visible to all non-admins", + name: "buckets without owner are hidden from non-admins", buckets: []testBucket{ {name: "owned-bucket", ownerId: "user1"}, {name: "unowned-bucket", ownerId: ""}, // No owner set }, requestIdentityId: "user2", requestIsAdmin: false, - expectedBucketNames: []string{"unowned-bucket"}, - description: "Buckets without owner should be visible to all users (backward compatibility)", + expectedBucketNames: []string{}, + description: "Buckets without owner should be hidden from non-admin users", }, { - name: "empty identityId allows only unowned buckets", + name: "empty identityId skips ownership check", buckets: []testBucket{ {name: "owned-bucket", ownerId: "user1"}, {name: "unowned-bucket", ownerId: ""}, @@ -306,7 +306,7 @@ func TestListBucketsOwnershipFiltering(t *testing.T) { requestIdentityId: "", requestIsAdmin: false, expectedBucketNames: []string{"owned-bucket", "unowned-bucket"}, - description: "When identityId is empty, ownership check is skipped", + description: "When identityId is empty, ownership check is skipped, all buckets visible", }, { name: "admin with empty identityId sees all", @@ -320,15 +320,15 @@ func TestListBucketsOwnershipFiltering(t *testing.T) { description: "Admin should see all buckets even with empty identityId", }, { - name: "buckets with nil Extended metadata", + name: "buckets with nil Extended metadata hidden from non-admins", buckets: []testBucket{ {name: "bucket-no-extended", ownerId: "", nilExtended: true}, {name: "bucket-with-owner", ownerId: "user1"}, }, requestIdentityId: "user1", requestIsAdmin: false, - expectedBucketNames: []string{"bucket-no-extended", "bucket-with-owner"}, - description: "Buckets with nil Extended should be treated as unowned", + expectedBucketNames: []string{"bucket-with-owner"}, + description: "Buckets with nil Extended (no owner) should be hidden from non-admins", }, { name: "user sees only their bucket among many", @@ -343,6 +343,18 @@ func TestListBucketsOwnershipFiltering(t *testing.T) { expectedBucketNames: []string{"bob-bucket"}, description: "User should see only their single bucket among many", }, + { + name: "admin sees buckets without owners", + buckets: []testBucket{ + {name: "owned-bucket", ownerId: "user1"}, + {name: "unowned-bucket", ownerId: ""}, + {name: "no-metadata-bucket", ownerId: "", nilExtended: true}, + }, + requestIdentityId: "admin", + requestIsAdmin: true, + expectedBucketNames: []string{"owned-bucket", "unowned-bucket", "no-metadata-bucket"}, + description: "Admin should see all buckets including those without owners", + }, } for _, tc := range testCases { @@ -372,13 +384,17 @@ func TestListBucketsOwnershipFiltering(t *testing.T) { var filteredBuckets []string for _, entry := range entries { if entry.IsDirectory { - // Apply ownership filtering logic (lines 67-74 from s3api_bucket_handlers.go) + // 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 { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != tc.requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != tc.requestIdentityId { + shouldSkip = true } } @@ -506,14 +522,18 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) { shouldSkip := false if requestIdentityId != "" && !isAdmin { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != requestIdentityId { + shouldSkip = true } } - assert.False(t, shouldSkip, "Empty owner ID should be treated as unowned (visible to all)") + assert.True(t, shouldSkip, "Empty owner ID should be treated as unowned (hidden from non-admins)") }) t.Run("nil Extended map safe access", func(t *testing.T) { @@ -533,13 +553,17 @@ func TestListBucketsOwnershipEdgeCases(t *testing.T) { assert.NotPanics(t, func() { shouldSkip := false if requestIdentityId != "" && !isAdmin { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != requestIdentityId { + shouldSkip = true } } - assert.False(t, shouldSkip, "Nil Extended should be treated as unowned") + assert.True(t, shouldSkip, "Nil Extended (no owner) should be hidden from non-admins") }) }) @@ -607,10 +631,14 @@ func TestListBucketsOwnershipWithPermissions(t *testing.T) { for _, entry := range entries { shouldSkip := false if requestIdentityId != "" && !isAdmin { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != requestIdentityId { + shouldSkip = true } } if !shouldSkip { @@ -654,10 +682,14 @@ func TestListBucketsOwnershipWithPermissions(t *testing.T) { for _, entry := range entries { shouldSkip := false if requestIdentityId != "" && !isAdmin { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != requestIdentityId { + shouldSkip = true } } if !shouldSkip { @@ -700,10 +732,14 @@ func TestListBucketsOwnershipCaseSensitivity(t *testing.T) { shouldSkip := false if tc.requestIdentityId != "" && !isAdmin { + var bucketOwnerId string if id, ok := entry.Extended[s3_constants.AmzIdentityId]; ok { - if bucketOwnerId := string(id); bucketOwnerId != "" && bucketOwnerId != tc.requestIdentityId { - shouldSkip = true - } + bucketOwnerId = string(id) + } + + // Skip buckets that have no owner or are owned by someone else + if bucketOwnerId == "" || bucketOwnerId != tc.requestIdentityId { + shouldSkip = true } }