Added some fixes and updated tests

Signed-off-by: Aayush Chouhan <achouhan@redhat.com>

Author: aayushchouhan09

src/server/common_services/auth_server.js

@@ -495,6 +495,10 @@ function _prepare_auth_request(req) {
     req.has_bucket_action_permission = async function(bucket, action, bucket_path, req_query) {
         return has_bucket_action_permission(bucket, req.account, action, req_query, bucket_path);
     };
+
+    req.has_list_bucket_permission = function(bucket) {
+        return has_list_bucket_permission(bucket, req.account, req.system);
+    };
 }
 
 function _get_auth_info(account, system, authorized_by, role, extra) {
@@ -519,6 +523,51 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
     return response;
 }
 
+/**
+ * is_system_owner checks if the account is the system owner
+ * @param {Record<string, any>} system
+ * @param {Record<string, any>} account
+ * @returns {boolean}
+ */
+function is_system_owner(system, account) {
+    return system?.owner?.email?.unwrap() === account?.email?.unwrap();
+}
+
+/**
+ * is_bucket_owner checks if the account owns the bucket
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @returns {boolean}
+ */
+function is_bucket_owner(bucket, account) {
+    // check direct ownership
+    if (bucket?.owner_account?.email?.unwrap() === account?.email?.unwrap()) return true;
+
+    // check bucket claim ownership (OBC)
+    return account?.bucket_claim_owner?.name?.unwrap() === bucket?.name?.unwrap();
+}
+
+/**
+ * has_list_bucket_permission returns true if the account can list the bucket in ListBuckets operation
+ * this is a synchronous function that only checks system ownership and bucket ownership
+ *
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @param {Record<string, any>} system
+ * @returns {boolean}
+ */
+function has_list_bucket_permission(bucket, account, system) {
+    // system owner can list all the buckets
+    if (is_system_owner(bucket.system, account)) return true;
+
+    // bucket owner can list their buckets only
+    if (is_bucket_owner(bucket, account)) return true;
+
+    // TODO: Add IAM policy support for s3:ListAllMyBuckets action
+
+    return false;
+}
+
 /**
  * has_bucket_action_permission returns true if the requesting account has permission to perform
  * the given action on the given bucket.
@@ -536,20 +585,16 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
  * @returns  {Promise<boolean>} true if the account has permission to perform the action on the bucket
  */
 async function has_bucket_action_permission(bucket, account, action, req_query, bucket_path = "") {
-    if (!account || !account.email) {
-        dbg.warn('has_bucket_action_permission: account or account.email is missing');
-        return false;
-    }
     dbg.log1('has_bucket_action_permission:', bucket.name, account.email, bucket.owner_account.email);
 
-    // If the system owner account wants to access the bucket, allow it
-    if (bucket.system.owner.email.unwrap() === account.email.unwrap()) return true;
+    // system owner can access all buckets
+    if (is_system_owner(bucket.system, account)) return true;
 
-    const is_owner = (bucket.owner_account.email.unwrap() === account.email.unwrap()) ||
-        (account.bucket_claim_owner && account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap());
+    // check bucket ownership
+    const owner = is_bucket_owner(bucket, account);
     const bucket_policy = bucket.s3_policy;
 
-    if (!bucket_policy) return is_owner;
+    if (!bucket_policy) return owner;
     if (!action) {
         throw new Error('has_bucket_action_permission: action is required');
     }
@@ -563,9 +608,8 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     );
 
     if (result === 'DENY') return false;
-    if (result === 'ALLOW') return true;
 
-    return is_owner;
+    return owner || result === 'ALLOW';
 }
 
 /**

src/server/system_services/bucket_server.js

@@ -1062,14 +1062,11 @@ async function list_buckets(req) {
     let continuation_token = req.rpc_params?.continuation_token;
     const max_buckets = req.rpc_params?.max_buckets;
 
-    const accessible_bucket_list = [];
-    await Promise.all(
-        system_store.data.buckets.map(async bucket => {
-            if (bucket.deleting) return;
-            const has_permission = await req.has_s3_bucket_permission(bucket, "s3:ListBucket");
-            if (has_permission) accessible_bucket_list.push(bucket);
-        })
-    );
+    // filter buckets based on ownership
+    const accessible_bucket_list = system_store.data.buckets.filter(bucket => {
+        if (bucket.deleting) return false;
+        return req.has_list_bucket_permission(bucket);
+    });
 
     accessible_bucket_list.sort((a, b) => a.name.unwrap().localeCompare(b.name.unwrap()));
 

src/test/integration_tests/api/s3/test_s3_list_buckets.js

@@ -129,74 +129,49 @@ mocha.describe('s3_ops', function() {
 
     mocha.describe('list_buckets permissions', function() {
         this.timeout(60000);
-        let s3_user;
-        const test_user = 'test-user';
-        const user_bucket = 'user-buck';
-        const admin_bucket = 'admin-buck';
+        let s3_account_a;
+        let s3_account_b;
 
-        mocha.before(async function() {
+        async function create_account_and_client(name) {
             const account = await rpc_client.account.create_account({
-                name: test_user,
-                email: test_user,
-                has_login: false,
-                s3_access: true,
+                name, email: name, has_login: false, s3_access: true,
                 default_resource: coretest.POOL_LIST[0].name
             });
-
-            s3_user = new S3Client({
+            return new S3Client({
                 ...s3_client_params,
                 credentials: {
                     accessKeyId: account.access_keys[0].access_key.unwrap(),
                     secretAccessKey: account.access_keys[0].secret_key.unwrap(),
                 }
             });
+        }
 
-            await s3_user.send(new CreateBucketCommand({ Bucket: user_bucket }));
-            await s3.send(new CreateBucketCommand({ Bucket: admin_bucket }));
+        mocha.before(async function() {
+            s3_account_a = await create_account_and_client('account-a');
+            s3_account_b = await create_account_and_client('account-b');
+            await s3_account_a.send(new CreateBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new CreateBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new CreateBucketCommand({ Bucket: 'admin-buck' }));
         });
 
         mocha.after(async function() {
-            await s3_user.send(new DeleteBucketCommand({ Bucket: user_bucket }));
-            await s3.send(new DeleteBucketCommand({ Bucket: admin_bucket }));
-            await rpc_client.account.delete_account({ email: test_user });
+            await s3_account_a.send(new DeleteBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new DeleteBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new DeleteBucketCommand({ Bucket: 'admin-buck' }));
+            await rpc_client.account.delete_account({ email: 'account-a' });
+            await rpc_client.account.delete_account({ email: 'account-b' });
         });
 
-        mocha.it('user should list only owned buckets', async function() {
-            const buckets = (await s3_user.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
-            assert(buckets.includes(user_bucket) && !buckets.includes(admin_bucket));
+        mocha.it('accounts should list only owned buckets', async function() {
+            const buckets_a = (await s3_account_a.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            const buckets_b = (await s3_account_b.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            assert.deepStrictEqual(buckets_a, ['bucket-a']);
+            assert.deepStrictEqual(buckets_b, ['bucket-b']);
         });
 
-        mocha.it('admin should list all buckets', async function() {
+        mocha.it('admin should lists all the buckets', async function() {
             const buckets = (await s3.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
-            assert(buckets.length >= 2);
-            assert(buckets.includes(user_bucket));
-            assert(buckets.includes(admin_bucket));
-        });
-
-        mocha.it('bucket policy grants list access', async function() {
-            let buckets = (await s3_user.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
-            assert(!buckets.includes(admin_bucket));
-
-            await rpc_client.bucket.put_bucket_policy({
-                name: admin_bucket,
-                policy: {
-                    Version: '2012-10-17',
-                    Statement: [{
-                        Effect: 'Allow',
-                        Principal: { AWS: test_user },
-                        Action: ['s3:ListBucket'],
-                        Resource: [`arn:aws:s3:::${admin_bucket}`]
-                    }]
-                }
-            });
-
-            buckets = (await s3_user.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
-            assert(buckets.includes(admin_bucket));
-
-            await rpc_client.bucket.delete_bucket_policy({ name: admin_bucket });
-
-            buckets = (await s3_user.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
-            assert(!buckets.includes(admin_bucket));
+            assert(buckets.includes('bucket-a') && buckets.includes('bucket-b') && buckets.includes('admin-buck'));
         });
     });