Added fixes and some test fixes

Signed-off-by: Aayush Chouhan <[email protected]>

Author: aayushchouhan09

src/sdk/bucketspace_fs.js

@@ -274,8 +274,12 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
 
             if (!this.has_list_bucket_permission(bucket, account)) return;
 
-            const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);
-            if (!fs_accessible) return;
+            // only for nsfs buckets, also check filesystem access
+            const is_nsfs_bucket = object_sdk.is_nsfs_bucket(bucket.namespace);
+            if (is_nsfs_bucket) {
+                const fs_accessible = await this._has_access_to_nsfs_dir(bucket.namespace, object_sdk);
+                if (!fs_accessible) return;
+            }
             return bucket;
         });
         return { buckets: buckets.filter(bucket => bucket) };
@@ -942,7 +946,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
 
     /**
      * 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
+     * only checks bucket ownership (aws-compliant: bucket policies don't affect ListBuckets)
      *
      * @param {Record<string, any>} bucket
      * @param {Record<string, any>} account
@@ -952,7 +956,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         // bucket owner can list their buckets only
         if (this.is_bucket_owner(bucket, account)) return true;
 
-        // TODO: Add IAM policy support for s3:ListAllMyBuckets action
+        // TODO: Add IAM policy support for user's s3:ListAllMyBuckets action
 
         return false;
     }

src/server/common_services/auth_server.js

@@ -564,7 +564,7 @@ function has_list_bucket_permission(bucket, account, system) {
     // bucket owner can list their buckets only
     if (is_bucket_owner(bucket, account)) return true;
 
-    // TODO: Add IAM policy support for s3:ListAllMyBuckets action
+    // TODO: Add IAM policy support for user's s3:ListAllMyBuckets action
 
     return false;
 }

src/test/integration_tests/api/sts/test_sts.js

@@ -457,6 +457,7 @@ function verify_session_token(session_token, access_key, secret_key, assumed_rol
 mocha.describe('Session token tests', function() {
     const { rpc_client } = coretest;
     const alice2 = 'alice2';
+    const alice2_buck = 'alice2-test-bucket';
     const bob2 = 'bob2';
     const charlie2 = 'charlie2';
     const accounts = [{ email: alice2 }, { email: bob2 }, { email: charlie2 }];
@@ -466,6 +467,8 @@ mocha.describe('Session token tests', function() {
     mocha.after(async function() {
         const self = this; // eslint-disable-line no-invalid-this
         self.timeout(60000);
+
+        await accounts[0].s3.deleteBucket({ Bucket: alice2_buck }).promise();
         for (const account of accounts) {
             await rpc_client.account.delete_account({ email: account.email });
         }
@@ -542,6 +545,10 @@ mocha.describe('Session token tests', function() {
             name: 'first.bucket',
             policy: s3accesspolicy,
         });
+
+        // create a bucket owned by alice2 for ListBuckets to work
+        // Note: bucket policy alone is not sufficient for ListBuckets operation
+        await accounts[0].s3.createBucket({ Bucket: alice2_buck }).promise();
     });
 
     mocha.it('user b assume role of user a - default expiry - list s3 - should be allowed', async function() {
@@ -564,7 +571,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - valid expiry via durationSeconds - list s3 - should be allowed', async function() {
@@ -589,7 +596,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - invalid expiry via durationSeconds - should be rejected', async function() {

src/test/integration_tests/nsfs/test_nsfs_integration.js

@@ -249,14 +249,12 @@ mocha.describe('bucket operations - namespace_fs', function() {
         s3_wrong_uid = new S3(s3_creds);
     });
     mocha.it('list buckets with wrong uid, gid', async function() {
-        // Give s3_wrong_uid access to the required buckets
-        const s3_policy = generate_s3_policy('*', first_bucket, ['s3:*']);
-        await rpc_client.bucket.put_bucket_policy({ name: first_bucket, policy: s3_policy.policy });
-
+        await s3_wrong_uid.createBucket({ Bucket: 'bucket-wrong-uid' });
         const res = await s3_wrong_uid.listBuckets({});
         console.log(inspect(res));
-        const list_ok = bucket_in_list([first_bucket], [bucket_name], res.Buckets);
+        const list_ok = bucket_in_list(['bucket-wrong-uid'], [first_bucket, bucket_name], res.Buckets);
         assert.ok(list_ok);
+        await s3_wrong_uid.deleteBucket({ Bucket: 'bucket-wrong-uid' });
     });
     mocha.it('update account', async function() {
         this.timeout(600000); // eslint-disable-line no-invalid-this
@@ -514,13 +512,12 @@ mocha.describe('bucket operations - namespace_fs', function() {
         }
     });
     mocha.it('list buckets with uid, gid', async function() {
-        // Give s3_correct_uid access to the required buckets
-        const generated = generate_s3_policy('*', bucket_name, ['s3:*']);
-        await rpc_client.bucket.put_bucket_policy({ name: generated.params.bucket, policy: generated.policy });
+        await s3_correct_uid.createBucket({ Bucket: 'bucket-correct-uid' });
         const res = await s3_correct_uid.listBuckets({});
         console.log(inspect(res));
-        const list_ok = bucket_in_list([bucket_name], [], res.Buckets);
+        const list_ok = bucket_in_list(['bucket-correct-uid'], [bucket_name], res.Buckets);
         assert.ok(list_ok);
+        await s3_correct_uid.deleteBucket({ Bucket: 'bucket-correct-uid' });
     });
     mocha.it('list buckets with dn', async function() {
         // Give s3_correct_uid access to the required buckets
@@ -1059,11 +1056,10 @@ mocha.describe('list objects - namespace_fs', async function() {
         await fs_utils.folder_delete(tmp_fs_root_ls1);
     });
 
-    mocha.it('list buckets - uid & gid mismatch - should fail', async function() {
+    mocha.it('list buckets - uid & gid mismatch - should list nothing initially', async function() {
         const { s3_client } = accounts.account1;
         const res = await s3_client.listBuckets({});
-        assert.equal(res.Buckets.length, 1);
-        assert.equal(res.Buckets[0].Name, first_bucket);
+        assert.equal(res.Buckets.length, 0);
     });
 
     mocha.it('put object 1 account_with_access - uid & gid mismatch - should fail', async function() {
@@ -1104,18 +1100,17 @@ mocha.describe('list objects - namespace_fs', async function() {
         }
     });
 
-    mocha.it('list buckets - uid & gid mismatch - account1', async function() {
+    mocha.it('list buckets - uid & gid mismatch - account1 lists owned bucket', async function() {
         const { s3_client } = accounts.account1;
         const res = await s3_client.listBuckets({});
-        assert.equal(res.Buckets.length, 2);
-        assert.equal(res.Buckets.filter(bucket => bucket.Name === s3_b_name || bucket.Name === first_bucket).length, 2);
+        assert.equal(res.Buckets.length, 1);
+        assert.equal(res.Buckets[0].Name, s3_b_name);
     });
 
-    mocha.it('list buckets - uid & gid mismatch - account2', async function() {
+    mocha.it('list buckets - uid & gid mismatch - account2 lists nothing', async function() {
         const { s3_client } = accounts.account2;
         const res = await s3_client.listBuckets({});
-        assert.equal(res.Buckets.length, 1);
-        assert.equal(res.Buckets[0].Name, first_bucket);
+        assert.equal(res.Buckets.length, 0);
     });
 
     mocha.it('create s3 bucket by root', async function() {
@@ -1125,10 +1120,11 @@ mocha.describe('list objects - namespace_fs', async function() {
         await rpc_client.bucket.put_bucket_policy({ name: s3_root_b_name, policy: s3_policy.policy });
     });
 
-    mocha.it('list buckets - uid & gid match - account with permission', async function() {
+    mocha.it('list buckets - uid & gid match - account lists owned bucket', async function() {
         const { s3_client } = accounts.account4;
         const res = await s3_client.listBuckets({});
-        assert.equal(res.Buckets.length, 4);
+        assert.equal(res.Buckets.length, 1);
+        assert.equal(res.Buckets[0].Name, s3_root_b_name);
     });
 
     mocha.it('change mode of /bucket123/ to 0o777: ', async function() {
@@ -1665,92 +1661,14 @@ mocha.describe('list buckets - namespace_fs', async function() {
         await fs_utils.folder_delete(tmp_fs_root_ls);
     });
 
-    mocha.it('list buckets - each account can list only its own bucket - no bucket policies applied ', async function() {
+    mocha.it('list buckets - each account can list only its own bucket', async function() {
         for (const account of Object.values(accounts)) {
             const { s3_client } = account;
             const res = await s3_client.listBuckets({});
             const buckets = res.Buckets.map(bucket => bucket.Name).sort();
-            assert.equal(buckets.length, 2);
-            assert.deepStrictEqual(buckets, [account.bucket, first_bucket]);
-        }
-    });
-
-    mocha.it('account1 - all accounts are allowed to list bucket1', async function() {
-        this.timeout(50000); // eslint-disable-line no-invalid-this
-        // allow all accounts to list bucket1
-        const public_bucket = accounts.account1.bucket;
-        const bucket_policy = generate_s3_policy('*', public_bucket, ['s3:ListBucket']);
-        await rpc_client.bucket.put_bucket_policy({
-            name: public_bucket,
-            policy: bucket_policy.policy,
-        });
-        // check account2 and account3 can list bucket1
-        // account2/account3 should be able to list -
-        // 1. first.bucket
-        // 2. buckets owned by the account (account2 can list bucket2 / account3 can list bucket3)
-        // 3. bucket1 (by the given bucket policy)
-        // account4 can not list bucket1 because of missing fs access permissions (unmatching uid/gid)
-        // account4 can list -
-        // 1. first.bucket
-        // 2. bucket4 - owned by the account
-        for (const account_name of ['account2', 'account3', 'account4']) {
-            const account = accounts[account_name];
-            const { s3_client, bucket } = account;
-            const res = await s3_client.listBuckets({});
-            const buckets = res.Buckets.map(bucket_info => bucket_info.Name).sort();
-            if (account_name === 'account4') {
-                assert.equal(buckets.length, 2);
-                assert.deepStrictEqual(buckets, [bucket, first_bucket]);
-            } else {
-                assert.equal(buckets.length, 3);
-                assert.deepStrictEqual(buckets, [accounts.account1.bucket, bucket, first_bucket]);
-            }
+            assert.equal(buckets.length, 1);
+            assert.deepStrictEqual(buckets, [account.bucket]);
         }
-
-        // delete bucket policy
-        await rpc_client.bucket.put_bucket_policy({
-            name: public_bucket,
-            policy: '',
-        });
-    });
-
-    mocha.it('account2 - set allow only account1 list bucket2, account1/account2 can list bucket2 but account3 cant', async function() {
-        this.timeout(50000); // eslint-disable-line no-invalid-this
-        const bucket2 = accounts.account2.bucket;
-        const account_name = 'account1';
-        // on NC the account identifier is account name, and on containerized it's the account's email
-        // allow bucket2 to be listed by account1
-        const account1_principal = is_nc_coretest ? account_name : `${account_name}@noobaa.com`;
-        const bucket_policy = generate_s3_policy(account1_principal, bucket2, ['s3:ListBucket']);
-        await rpc_client.bucket.put_bucket_policy({
-            name: bucket2,
-            policy: bucket_policy.policy,
-        });
-
-        // account1 can list -
-        // 1. bucket1
-        // 2. bucket2 (due to the policy)
-        // 3. first.bucket
-        let s3_client = accounts.account1.s3_client;
-        let res = await s3_client.listBuckets({});
-        let buckets = res.Buckets.map(bucket => bucket.Name).sort();
-        assert.equal(buckets.length, 3);
-        assert.deepStrictEqual(buckets, [accounts.account1.bucket, accounts.account2.bucket, first_bucket]);
-
-        // account3 can list -
-        // 1. bucket3
-        // 2. first.bucket
-        s3_client = accounts.account3.s3_client;
-        res = await s3_client.listBuckets({});
-        buckets = res.Buckets.map(bucket => bucket.Name).sort();
-        assert.equal(buckets.length, 2);
-        assert.deepStrictEqual(buckets, [accounts.account3.bucket, first_bucket]);
-
-        // delete bucket policy
-        await rpc_client.bucket.put_bucket_policy({
-            name: bucket2,
-            policy: '',
-        });
     });
 });
 

src/test/unit_tests/nsfs/test_bucketspace_fs.js

@@ -22,7 +22,7 @@ const nb_native = require('../../../util/nb_native');
 const SensitiveString = require('../../../util/sensitive_string');
 const NamespaceFS = require('../../../sdk/namespace_fs');
 const BucketSpaceFS = require('../../../sdk/bucketspace_fs');
-const { TMP_PATH, generate_s3_policy } = require('../../system_tests/test_utils');
+const { TMP_PATH } = require('../../system_tests/test_utils');
 const { CONFIG_SUBDIRS, JSON_SUFFIX } = require('../../../sdk/config_fs');
 const nc_mkm = require('../../../manage_nsfs/nc_master_key_manager').get_instance();
 
@@ -519,27 +519,10 @@ mocha.describe('bucketspace_fs', function() {
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
             assert.equal(res.buckets.length, 0);
         });
-        mocha.it('list buckets - different account with bucket policy (principal by name)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4.name, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
-            const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
-            const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
-        });
-        mocha.it('list buckets - different account with bucket policy (principal by id)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4._id, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
+        mocha.it('list buckets - different account cannot list buckets they do not own', async function() {
             const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
+            assert.equal(res.buckets.length, 0);
         });
         mocha.afterEach(async function() {
             await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket}`);