Merge pull request #8192 from shirady/nsfs-nc-system-owner

shirady · web-flow · commit ad73e9cb3bd4 · 2024-08-08T13:53:35.000+03:00
NC | NSFS | Stop Setting `system_owner` in Bucket Config
diff --git a/docs/NooBaaNonContainerized/CI&Tests.md b/docs/NooBaaNonContainerized/CI&Tests.md
@@ -103,6 +103,7 @@ The following is a list of `NC jest tests` files -
 8. `test_nc_nsfs_bucket_schema_validation.test.js` - Tests NC bucket schema validation.  
 9. `test_nc_nsfs_account_schema_validation.test.js` - Tests NC account schema validation.  
 10. `test_nc_nsfs_new_buckets_path_validation.test.js` - Tests new_buckets_path RW access.  
+11. `test_config_fs.test.js` - Tests ConfigFS methods.
 
 #### nc_index.js File
 * The `nc_index.js` is a file that runs several NC and NSFS mocha related tests.  
diff --git a/docs/NooBaaNonContainerized/GettingStarted.md b/docs/NooBaaNonContainerized/GettingStarted.md
@@ -187,7 +187,7 @@ make_bucket: s3bucket
 #### 3. Check that the bucket configuration file was created successfully -
 ```sh
 sudo cat /etc/noobaa.conf.d/buckets/s3bucket.json
-{"_id":"65cb1efcbec92b33220112d7","name":"s3bucket","owner_account":"65cb1e7c9e6ae40d499c0ae4","system_owner":"account1","bucket_owner":"account1","versioning":"DISABLED","creation_date":"2023-09-26T05:56:16.252Z","path":"/tmp/fs1/s3bucket","should_create_underlying_storage":true}
+{"_id":"65cb1efcbec92b33220112d7","name":"s3bucket","owner_account":"65cb1e7c9e6ae40d499c0ae4","bucket_owner":"account1","versioning":"DISABLED","creation_date":"2023-09-26T05:56:16.252Z","path":"/tmp/fs1/s3bucket","should_create_underlying_storage":true}
 ```
 
 #### 4. Check that the underlying file system bucket directory was created successfully -
diff --git a/src/cmd/manage_nsfs.js b/src/cmd/manage_nsfs.js
@@ -94,7 +94,6 @@ async function fetch_bucket_data(action, user_input) {
         _id: undefined,
         name: user_input.name === undefined ? undefined : String(user_input.name),
         owner_account: undefined,
-        system_owner: user_input.owner, // GAP - needs to be the system_owner (currently it is the account name)
         bucket_owner: user_input.owner,
         tag: undefined, // if we would add the option to tag a bucket using CLI, this should be changed
         versioning: action === ACTIONS.ADD ? 'DISABLED' : undefined,
@@ -596,8 +595,9 @@ async function list_config_files(type, config_path, wide, show_secrets, filters)
     let config_files_list = await P.map_with_concurrency(10, entries, async entry => {
         if (entry.name.endsWith(JSON_SUFFIX)) {
             if (wide || should_filter) {
-                const full_path = path.join(config_path, entry.name);
-                const data = await config_fs.get_config_data(full_path, options);
+                const data = type === TYPES.ACCOUNT ?
+                    await config_fs.get_account_by_name(entry.name, options) :
+                    await config_fs.get_bucket_by_name(entry.name, options);
                 if (!data) return undefined;
                 if (should_filter && !filter_list_item(type, data, filters)) return undefined;
                 // remove secrets on !show_secrets && should filter
diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js
@@ -230,7 +230,7 @@ async function authorize_request_policy(req) {
 
     const account = req.object_sdk.requesting_account;
     const account_identifier = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
-    const is_system_owner = account_identifier === system_owner.unwrap();
+    const is_system_owner = Boolean(system_owner) && system_owner.unwrap() === account_identifier;
 
     // @TODO: System owner as a construct should be removed - Temporary
     if (is_system_owner) return;
diff --git a/src/sdk/bucketspace_fs.js b/src/sdk/bucketspace_fs.js
@@ -121,7 +121,6 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             };
 
             bucket.name = new SensitiveString(bucket.name);
-            bucket.system_owner = new SensitiveString(bucket.system_owner);
             bucket.bucket_owner = new SensitiveString(bucket.bucket_owner);
             bucket.owner_account = {
                 id: bucket.owner_account,
@@ -291,7 +290,6 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             tag: js_utils.default_value(tag, undefined),
             owner_account: account._id,
             creator: account._id,
-            system_owner: new SensitiveString(account.name),
             bucket_owner: new SensitiveString(account.name),
             versioning: config.NSFS_VERSIONING_ENABLED && lock_enabled ? 'ENABLED' : 'DISABLED',
             object_lock_configuration: config.WORM_ENABLED ? {
@@ -675,7 +673,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         const account_identifier = account.name.unwrap();
         dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account_identifier, bucket.bucket_owner.unwrap());
 
-        const is_system_owner = account_identifier === bucket.system_owner.unwrap();
+        const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account_identifier;
 
         // If the system owner account wants to access the bucket, allow it
         if (is_system_owner) return true;
diff --git a/src/sdk/config_fs.js b/src/sdk/config_fs.js
@@ -398,6 +398,7 @@ class ConfigFS {
     async get_bucket_by_name(bucket_name, options = {}) {
         const bucket_path = this.get_bucket_path_by_name(bucket_name);
         const bucket = await this.get_config_data(bucket_path, options);
+        this.adjust_bucket_with_schema_updates(bucket);
         return bucket;
     }
 
@@ -440,6 +441,18 @@ class ConfigFS {
         const bucket_config_path = this.get_bucket_path_by_name(bucket_name);
         await native_fs_utils.delete_config_file(this.fs_context, this.buckets_dir_path, bucket_config_path);
     }
+
+    /**
+     * adjust_bucket_with_schema_updates changes the bucket properties according to the schema
+     * @param {object} bucket
+     */
+    adjust_bucket_with_schema_updates(bucket) {
+        if (!bucket) return;
+        // system_owner is deprecated since version 5.18
+        if (bucket.system_owner !== undefined) {
+            delete bucket.system_owner;
+        }
+    }
 }
 
 // EXPORTS
diff --git a/src/sdk/object_sdk.js b/src/sdk/object_sdk.js
@@ -194,7 +194,7 @@ class ObjectSDK {
         const { bucket } = await bucket_namespace_cache.get_with_cache({ sdk: this, name });
         const policy_info = {
             s3_policy: bucket.s3_policy,
-            system_owner: bucket.system_owner,
+            system_owner: bucket.system_owner, // note that bucketspace_fs currently doesn't return system_owner
             bucket_owner: bucket.bucket_owner,
             owner_account: bucket.owner_account, // in NC NSFS this is the account id that owns the bucket
         };
diff --git a/src/server/system_services/schemas/nsfs_bucket_schema.js b/src/server/system_services/schemas/nsfs_bucket_schema.js
@@ -7,7 +7,6 @@ module.exports = {
     required: [
         '_id',
         'name',
-        'system_owner',
         'bucket_owner',
         'owner_account',
         'versioning',
@@ -30,6 +29,10 @@ module.exports = {
         name: {
             type: 'string',
         },
+        /** 
+         * @deprecated system_owner is kept for backward compatibility,
+         * but will no longer be included in new / updated bucket json.
+         */
         system_owner: {
             type: 'string',
         },
diff --git a/src/test/unit_tests/jest_tests/test_accountspace_fs.test.js b/src/test/unit_tests/jest_tests/test_accountspace_fs.test.js
@@ -1872,7 +1872,6 @@ function _new_bucket_defaults(account, bucket_name, bucket_storage_path) {
         _id: '65a8edc9bc5d5bbf9db71c75',
         name: bucket_name,
         owner_account: account._id,
-        system_owner: new SensitiveString(account.name),
         bucket_owner: new SensitiveString(account.name),
         creation_date: new Date().toISOString(),
         path: bucket_storage_path,
diff --git a/src/test/unit_tests/jest_tests/test_config_fs.test.js b/src/test/unit_tests/jest_tests/test_config_fs.test.js
@@ -0,0 +1,30 @@
+/* Copyright (C) 2024 NooBaa */
+'use strict';
+
+const path = require('path');
+const config = require('../../../../config');
+const { TMP_PATH } = require('../../system_tests/test_utils');
+const { get_process_fs_context } = require('../../../util/native_fs_utils');
+const { ConfigFS } = require('../../../sdk/config_fs');
+
+const tmp_fs_path = path.join(TMP_PATH, 'test_config_fs');
+const config_root = path.join(tmp_fs_path, 'config_root');
+const config_root_backend = config.NSFS_NC_CONFIG_DIR_BACKEND;
+const fs_context = get_process_fs_context(config_root_backend);
+
+const config_fs = new ConfigFS(config_root, config_root_backend, fs_context);
+
+describe('adjust_bucket_with_schema_updates', () => {
+    it('should return undefined on undefined bucket', () => {
+        const bucket = undefined;
+        config_fs.adjust_bucket_with_schema_updates(bucket);
+        expect(bucket).toBeUndefined();
+    });
+
+    it('should return bucket without deprecated properties (system_owner)', () => {
+        const bucket = {name: 'bucket1', system_owner: 'account1-system-owner'};
+        config_fs.adjust_bucket_with_schema_updates(bucket);
+        expect(bucket).toBeDefined();
+        expect(bucket).not.toHaveProperty('system_owner');
+    });
+});
diff --git a/src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_schema_validation.test.js b/src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_schema_validation.test.js
@@ -181,24 +181,6 @@ describe('schema validation NC NSFS bucket', () => {
             assert_validation(bucket_data, reason, message);
         });
 
-        it('bucket without system_owner', () => {
-            const bucket_data = get_bucket_data();
-            delete bucket_data.system_owner;
-            const reason = 'Test should have failed because of missing required property ' +
-                'system_owner';
-            const message = "must have required property 'system_owner'";
-            assert_validation(bucket_data, reason, message);
-        });
-
-        it('bucket with undefined system_owner', () => {
-            const bucket_data = get_bucket_data();
-            bucket_data.system_owner = undefined;
-            const reason = 'Test should have failed because of missing required property ' +
-                'system_owner';
-            const message = "must have required property 'system_owner'";
-            assert_validation(bucket_data, reason, message);
-        });
-
         it('bucket without bucket_owner', () => {
             const bucket_data = get_bucket_data();
             delete bucket_data.bucket_owner;
@@ -463,7 +445,6 @@ describe('schema validation NC NSFS bucket', () => {
 function get_bucket_data() {
     const bucket_name = 'bucket1';
     const id = '65a62e22ceae5e5f1a758aa8';
-    const system_owner = 'account1'; // GAP - currently account name
     const bucket_owner = 'account1'; // account name
     const owner_account = '65b3c68b59ab67b16f98c26e';
     const versioning_disabled = 'DISABLED';
@@ -473,7 +454,6 @@ function get_bucket_data() {
     const bucket_data = {
         _id: id,
         name: bucket_name,
-        system_owner: system_owner,
         bucket_owner: bucket_owner,
         owner_account: owner_account,
         versioning: versioning_disabled,
diff --git a/src/test/unit_tests/test_bucketspace_fs.js b/src/test/unit_tests/test_bucketspace_fs.js
@@ -387,6 +387,38 @@ mocha.describe('bucketspace_fs', function() {
         });
     });
 
+    mocha.describe('read_bucket_sdk_info', function() {
+        mocha.before(async function() {
+            const param = { name: test_bucket};
+            await bucketspace_fs.create_bucket(param, dummy_object_sdk);
+        });
+        mocha.after(async function() {
+            await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket}`);
+            const file_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket);
+            await fs_utils.file_delete(file_path);
+        });
+        mocha.it('read bucket that was created - check non existing deprecated properties', async function() {
+            const param = { name: test_bucket};
+            const bucket = await bucketspace_fs.read_bucket_sdk_info(param);
+            assert.ok(bucket.system_owner === undefined);
+        });
+        mocha.it('read bucket and check modify on read', async function() {
+            // we want to mock a bucket that already had a deprecated property,
+            // we manually add system_owner in the bucket
+            const bucket_config_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket);
+            const data = await fs.promises.readFile(bucket_config_path);
+            let bucket = await JSON.parse(data.toString());
+            assert.ok(bucket.system_owner === undefined);
+            bucket.system_owner = 'non-existing-system-owner';
+            await update_config_file(process_fs_context, CONFIG_SUBDIRS.BUCKETS, bucket_config_path, JSON.stringify(bucket));
+            await fs_utils.file_must_exist(bucket_config_path);
+
+            const param = { name: test_bucket};
+            bucket = await bucketspace_fs.read_bucket_sdk_info(param);
+            assert.ok(bucket.system_owner === undefined);
+        });
+    });
+
     mocha.describe('list_buckets', async function() {
         mocha.before(async function() {
             await create_bucket(test_bucket);
diff --git a/src/test/unit_tests/test_nc_nsfs_cli.js b/src/test/unit_tests/test_nc_nsfs_cli.js
@@ -1126,7 +1126,6 @@ function assert_response(action, type, actual_res, expected_res, show_secrets, w
 
 function assert_bucket(bucket, bucket_options) {
     assert.strictEqual(bucket.name, bucket_options.name);
-    assert.strictEqual(bucket.system_owner, bucket_options.owner);
     assert.strictEqual(bucket.bucket_owner, bucket_options.owner);
     assert.strictEqual(bucket.path, bucket_options.path);
     assert.strictEqual(bucket.should_create_underlying_storage, false);
diff --git a/src/test/unit_tests/test_s3_bucket_policy.js b/src/test/unit_tests/test_s3_bucket_policy.js
@@ -467,8 +467,6 @@ mocha.describe('s3_bucket_policy', function() {
     });
 
     mocha.it('should deny bucket owner access', async function() {
-        // test fails on NC_CORETEST because system_owner === bucket_owner, until this behavior changes skipping the test
-        if (process.env.NC_CORETEST) this.skip(); // eslint-disable-line no-invalid-this
         const policy = {
             Version: '2012-10-17',
             Statement: [{
@@ -500,6 +498,8 @@ mocha.describe('s3_bucket_policy', function() {
     });
 
     mocha.it('should set and delete bucket policy when system owner', async function() {
+    // test fails on NC_CORETEST because we do not set system_owner in buckets
+    if (process.env.NC_CORETEST) this.skip(); // eslint-disable-line no-invalid-this
         const policy = {
             Version: '2012-10-17',
             Statement: [{
