NC | bucket owner removal

Signed-off-by: Romy <[email protected]>

Author: romayalon

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","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","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 -

src/cmd/manage_nsfs.js

@@ -21,7 +21,7 @@ const noobaa_cli_diagnose = require('../manage_nsfs/diagnose');
 const nsfs_schema_utils = require('../manage_nsfs/nsfs_schema_utils');
 const { print_usage } = require('../manage_nsfs/manage_nsfs_help_utils');
 const { TYPES, ACTIONS, LIST_ACCOUNT_FILTERS, LIST_BUCKET_FILTERS, GLACIER_ACTIONS } = require('../manage_nsfs/manage_nsfs_constants');
-const { throw_cli_error, write_stdout_response, get_boolean_or_string_value, has_access_keys, set_debug_level } = require('../manage_nsfs/manage_nsfs_cli_utils');
+const { throw_cli_error, get_bucket_owner_account, write_stdout_response, get_boolean_or_string_value, has_access_keys, set_debug_level } = require('../manage_nsfs/manage_nsfs_cli_utils');
 const manage_nsfs_validations = require('../manage_nsfs/manage_nsfs_validations');
 const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
 
@@ -96,8 +96,6 @@ async function fetch_bucket_data(action, user_input) {
         // added undefined values to keep the order the properties when printing the data object
         _id: undefined,
         name: user_input.name === undefined ? undefined : String(user_input.name),
-        owner_account: undefined,
-        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,
         creation_date: action === ACTIONS.ADD ? new Date().toISOString() : undefined,
@@ -126,6 +124,13 @@ async function fetch_bucket_data(action, user_input) {
         data = await fetch_existing_bucket_data(data);
     }
 
+    //if we're updating the owner, needs to override owner in file with the owner from user input.
+    //if we're adding a bucket, need to set its owner id field
+    if ((action === ACTIONS.UPDATE && user_input.owner) || (action === ACTIONS.ADD)) {
+        const account = await get_bucket_owner_account(config_fs, user_input.owner);
+        data.owner_account = account?._id;
+    }
+
     // override values
     // fs_backend deletion specified with empty string '' (but it is not part of the schema)
     data.fs_backend = data.fs_backend || undefined;
@@ -167,6 +172,8 @@ async function get_bucket_status(data) {
 
     try {
         const config_data = await config_fs.get_bucket_by_name(data.name);
+        const account_data = await config_fs.get_identity_by_id(config_data.owner_account);
+        config_data.bucket_owner = account_data.name;
         write_stdout_response(ManageCLIResponse.BucketStatus, config_data);
     } catch (err) {
         const err_code = err.code === 'EACCES' ? ManageCLIError.AccessDenied : ManageCLIError.NoSuchBucket;

src/cmd/nsfs.js

@@ -207,8 +207,6 @@ class NsfsObjectSDK extends ObjectSDK {
                     Principal: [new SensitiveString('*')],
                 }]
             },
-            system_owner: new SensitiveString('nsfs'),
-            bucket_owner: new SensitiveString('nsfs'),
             owner_account: new SensitiveString('nsfs-id'), // temp
         };
     }

src/manage_nsfs/manage_nsfs_cli_utils.js

@@ -38,14 +38,17 @@ function write_stdout_response(response_code, detail, event_arg) {
  * otherwise it would throw an error
  * @param {import('../sdk/config_fs').ConfigFS} config_fs
  * @param {string} bucket_owner
+ * @param {string} [owner_account_id]
  */
-async function get_bucket_owner_account(config_fs, bucket_owner) {
+async function get_bucket_owner_account(config_fs, bucket_owner, owner_account_id) {
     try {
-        const account = await config_fs.get_account_by_name(bucket_owner);
+        const account = bucket_owner ?
+            await config_fs.get_account_by_name(bucket_owner) :
+            await config_fs.get_identity_by_id(owner_account_id);
         return account;
     } catch (err) {
         if (err.code === 'ENOENT') {
-            const detail_msg = `bucket owner ${bucket_owner} does not exists`;
+            const detail_msg = `bucket owner name = ${bucket_owner} id=${owner_account_id} does not exists`;
             throw_cli_error(ManageCLIError.BucketSetForbiddenBucketOwnerNotExists, detail_msg, {bucket_owner: bucket_owner});
         }
         throw err;

src/manage_nsfs/manage_nsfs_logging.js

@@ -39,8 +39,8 @@ async function export_bucket_logging(shared_config_fs) {
  */
 async function get_bucket_owner_keys(log_bucket_name) {
     const log_bucket_config_data = await config_fs.get_bucket_by_name(log_bucket_name);
-    const log_bucket_owner = log_bucket_config_data.bucket_owner;
-    const owner_config_data = await config_fs.get_account_by_name(log_bucket_owner, { show_secrets: true, decrypt_secret_key: true });
+    const log_bucket_owner = log_bucket_config_data.owner_account;
+    const owner_config_data = await config_fs.get_identities_by_id(log_bucket_owner, { show_secrets: true, decrypt_secret_key: true });
     return owner_config_data.access_keys;
 }
 

src/manage_nsfs/manage_nsfs_validations.js

@@ -316,7 +316,7 @@ async function validate_bucket_args(config_fs, data, action) {
     if (action === ACTIONS.ADD || action === ACTIONS.UPDATE) {
         if (action === ACTIONS.ADD) native_fs_utils.validate_bucket_creation({ name: data.name });
         if ((action === ACTIONS.UPDATE) && (data.new_name !== undefined)) native_fs_utils.validate_bucket_creation({ name: data.new_name });
-        if ((action === ACTIONS.ADD) && (data.bucket_owner === undefined)) throw_cli_error(ManageCLIError.MissingBucketOwnerFlag);
+        if ((action === ACTIONS.ADD) && (data.owner_account === undefined)) throw_cli_error(ManageCLIError.MissingBucketOwnerFlag);
         if (!data.path) throw_cli_error(ManageCLIError.MissingBucketPathFlag);
         // fs_backend='' used for deletion of the fs_backend property
         if (data.fs_backend !== undefined && !['GPFS', 'CEPH_FS', 'NFSv4'].includes(data.fs_backend)) {
@@ -328,30 +328,33 @@ async function validate_bucket_args(config_fs, data, action) {
         if (!exists) {
             throw_cli_error(ManageCLIError.InvalidStoragePath, data.path);
         }
-        const account = await get_bucket_owner_account(config_fs, data.bucket_owner);
-        const account_fs_context = await native_fs_utils.get_fs_context(account.nsfs_account_config, data.fs_backend);
+
+        // bucket owner validations 
+        const owner_account = await get_bucket_owner_account(config_fs, undefined, data.owner_account);
+        const account_fs_context = await native_fs_utils.get_fs_context(owner_account.nsfs_account_config,
+            owner_account.nsfs_account_config.fs_backend);
         if (!config.NC_DISABLE_ACCESS_CHECK) {
             const accessible = await native_fs_utils.is_dir_rw_accessible(account_fs_context, data.path);
             if (!accessible) {
                 throw_cli_error(ManageCLIError.InaccessibleStoragePath, data.path);
             }
         }
         if (action === ACTIONS.ADD) {
-            if (!account.allow_bucket_creation) {
-                const detail_msg = `${data.bucket_owner} account not allowed to create new buckets. ` +
+            if (!owner_account.allow_bucket_creation) {
+                const detail_msg = `${owner_account.name} account not allowed to create new buckets. ` +
                 `Please make sure to have a valid new_buckets_path and enable the flag allow_bucket_creation`;
                 throw_cli_error(ManageCLIError.BucketCreationNotAllowed, detail_msg);
+            }
         }
-            data.owner_account = account._id; // TODO move this assignment to better place
-        }
-        if (account.owner) {
-            const detail_msg = `account ${data.bucket_owner} is IAM account`;
-            throw_cli_error(ManageCLIError.BucketSetForbiddenBucketOwnerIsIAMAccount, detail_msg, {bucket_owner: data.bucket_owner});
+        if (owner_account.owner) {
+            const detail_msg = `account ${owner_account.name} is IAM account`;
+            throw_cli_error(ManageCLIError.BucketSetForbiddenBucketOwnerIsIAMAccount, detail_msg,
+                { bucket_owner: owner_account.name });
         }
         if (data.s3_policy) {
             try {
                 await bucket_policy_utils.validate_s3_policy(data.s3_policy, data.name,
-                    async principal => config_fs.is_account_exists_by_name(principal, account.owner)
+                    async principal => config_fs.is_account_exists_by_name(principal, owner_account.owner)
                 );
             } catch (err) {
                 dbg.error('validate_bucket_args invalid bucket policy err:', err);
@@ -440,7 +443,7 @@ async function validate_account_args(config_fs, data, action, is_flag_iam_operat
  * @param {object} data
  */
 async function validate_account_resources_before_deletion(config_fs, data) {
-    await validate_account_not_owns_buckets(config_fs, data.name);
+    await validate_account_not_owns_buckets(config_fs, data);
     // If it is root account (not owned by other account) then we check that it doesn't owns IAM accounts
     if (data.owner === undefined) {
         await check_if_root_account_does_not_have_IAM_users(config_fs, data, ACTIONS.DELETE);
@@ -476,14 +479,14 @@ function _validate_access_keys(access_key, secret_key) {
  * validate_delete_account will check if the account has at least one bucket
  * in case it finds one, it would throw an error
  * @param {import('../sdk/config_fs').ConfigFS} config_fs
- * @param {string} account_name
+ * @param {Object} account_data
  */
-async function validate_account_not_owns_buckets(config_fs, account_name) {
+async function validate_account_not_owns_buckets(config_fs, account_data) {
     const bucket_names = await config_fs.list_buckets();
     await P.map_with_concurrency(10, bucket_names, async bucket_name => {
         const data = await config_fs.get_bucket_by_name(bucket_name, { silent_if_missing: true });
-        if (data && data.bucket_owner === account_name) {
-            const detail_msg = `Account ${account_name} has bucket ${data.name}`;
+        if (data && data.owner_account === account_data._id) {
+            const detail_msg = `Account ${account_data.name} has bucket ${data.name}`;
             throw_cli_error(ManageCLIError.AccountDeleteForbiddenHasBuckets, detail_msg);
         }
         return data;

src/sdk/accountspace_fs.js

@@ -672,7 +672,7 @@ class AccountSpaceFS {
         const bucket_names = await this.config_fs.list_buckets();
         await P.map_with_concurrency(10, bucket_names, async bucket_name => {
             const bucket_data = await this.config_fs.get_bucket_by_name(bucket_name, { silent_if_missing: true});
-            if (bucket_data && bucket_data.bucket_owner === account_to_delete.name) {
+            if (bucket_data && bucket_data.owner_account === account_to_delete._id) {
                 this._throw_error_delete_conflict(action, account_to_delete, resource_name);
             }
             return bucket_data;

src/sdk/bucketspace_fs.js

@@ -17,6 +17,7 @@ const nsfs_schema_utils = require('../manage_nsfs/nsfs_schema_utils');
 const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
 const { ConfigFS, JSON_SUFFIX } = require('./config_fs');
 const mongo_utils = require('../util/mongo_utils');
+const { TYPES } = require('../manage_nsfs/manage_nsfs_constants');
 
 const KeysSemaphore = require('../util/keys_semaphore');
 const { get_umasked_mode, validate_bucket_creation, get_bucket_tmpdir_full_path, folder_delete,
@@ -121,11 +122,22 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             };
 
             bucket.name = new SensitiveString(bucket.name);
-            bucket.bucket_owner = new SensitiveString(bucket.bucket_owner);
+            // Backward compatibility will take longer without bucket owner name
+            const account_config = await this.config_fs.get_identity_by_id(
+                bucket.owner_account,
+                TYPES.ACCOUNT,
+                { silent_if_missing: true }
+            );
+
+            if (!account_config) {
+                dbg.warn(`Bucket Owner does not exist ${bucket.owner_account}`);
+            }
+            bucket.bucket_owner = new SensitiveString(account_config?.name);
             bucket.owner_account = {
                 id: bucket.owner_account,
                 email: bucket.bucket_owner
             };
+
             if (bucket.s3_policy) {
                 for (const [s_index, statement] of bucket.s3_policy.Statement.entries()) {
                     const statement_principal = statement.Principal || statement.NotPrincipal;
@@ -286,7 +298,6 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             tag: js_utils.default_value(tag, undefined),
             owner_account: account._id,
             creator: account._id,
-            bucket_owner: new SensitiveString(account.name),
             versioning: config.NSFS_VERSIONING_ENABLED && lock_enabled ? 'ENABLED' : 'DISABLED',
             object_lock_configuration: config.WORM_ENABLED ? {
                 object_lock_enabled: lock_enabled ? 'Enabled' : 'Disabled',
@@ -667,7 +678,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
     // so they can be re-used
     async has_bucket_action_permission(bucket, account, action, bucket_path = "") {
         const account_identifier = account.name.unwrap();
-        dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account_identifier, bucket.bucket_owner.unwrap());
+        dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account_identifier, bucket.owner_account);
 
         const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account_identifier;
 

src/sdk/config_fs.js

@@ -787,6 +787,10 @@ class ConfigFS {
         if (bucket.system_owner !== undefined) {
             delete bucket.system_owner;
         }
+        // bucket_owner is deprecated since version 5.18
+        if (bucket.bucket_owner !== undefined) {
+            delete bucket.bucket_owner;
+        }
     }
 
     /**

src/server/system_services/schemas/nsfs_bucket_schema.js

@@ -7,7 +7,6 @@ module.exports = {
     required: [
         '_id',
         'name',
-        'bucket_owner',
         'owner_account',
         'versioning',
         'path',
@@ -36,7 +35,10 @@ module.exports = {
         system_owner: {
             type: 'string',
         },
-        // bucket_owner is the account name
+        /** 
+         * @deprecated bucket_owner is kept for backward compatibility,
+         * but will no longer be included in new / updated bucket json.
+         */
         bucket_owner: {
             type: 'string',
         },

src/test/unit_tests/jest_tests/test_accountspace_fs.test.js

@@ -1861,7 +1861,6 @@ function _new_bucket_defaults(account, bucket_name, bucket_storage_path) {
         _id: '65a8edc9bc5d5bbf9db71c75',
         name: bucket_name,
         owner_account: account._id,
-        bucket_owner: new SensitiveString(account.name),
         creation_date: new Date().toISOString(),
         path: bucket_storage_path,
         should_create_underlying_storage: true,

src/test/unit_tests/jest_tests/test_config_fs.test.js

@@ -22,9 +22,10 @@ describe('adjust_bucket_with_schema_updates', () => {
     });
 
     it('should return bucket without deprecated properties (system_owner)', () => {
-        const bucket = {name: 'bucket1', system_owner: 'account1-system-owner'};
+        const bucket = {name: 'bucket1', system_owner: 'account1-system-owner', bucket_owner: 'account1-bucket_owner'};
         config_fs.adjust_bucket_with_schema_updates(bucket);
         expect(bucket).toBeDefined();
         expect(bucket).not.toHaveProperty('system_owner');
+        expect(bucket).not.toHaveProperty('bucket_owner');
     });
 });

src/test/unit_tests/jest_tests/test_nc_nsfs_bucket_cli.test.js

@@ -129,7 +129,7 @@ describe('manage nsfs cli bucket flow', () => {
             await set_path_permissions_and_owner(bucket_options.path, account_defaults, 0o700);
             await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
             const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
-            assert_bucket(bucket, bucket_options);
+            await assert_bucket(bucket, bucket_options, config_fs);
         });
 
         it('cli create bucket - account can not access path  NC_DISABLE_ACCESS_CHECK = true - should succeed', async () => {
@@ -256,7 +256,7 @@ describe('manage nsfs cli bucket flow', () => {
             await exec_manage_cli(type, action, command_flags);
             // compare the details
             const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
-            assert_bucket(bucket, bucket_options);
+            await assert_bucket(bucket, bucket_options, config_fs);
         });
 
         it('cli create bucket using from_file with optional options (fs_backend)', async () => {
@@ -271,7 +271,7 @@ describe('manage nsfs cli bucket flow', () => {
             await exec_manage_cli(type, action, command_flags);
             // compare the details
             const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
-            assert_bucket(bucket, bucket_options);
+            await assert_bucket(bucket, bucket_options, config_fs);
             expect(bucket.fs_backend).toEqual(bucket_options.fs_backend);
         });
 
@@ -287,7 +287,7 @@ describe('manage nsfs cli bucket flow', () => {
             await exec_manage_cli(type, action, command_flags);
             // compare the details
             const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
-            assert_bucket(bucket, bucket_options);
+            await assert_bucket(bucket, bucket_options, config_fs);
             expect(bucket.bucket_policy).toEqual(bucket_options.s3_policy);
         });
 
@@ -511,7 +511,8 @@ describe('manage nsfs cli bucket flow', () => {
             await set_path_permissions_and_owner(bucket_defaults.path, account_defaults2, 0o700);
             await exec_manage_cli(TYPES.BUCKET, action, bucket_options);
             const bucket = await config_fs.get_bucket_by_name(bucket_defaults.name);
-            expect(bucket.bucket_owner).toBe(account_defaults2.name);
+            const owner_data = await config_fs.get_identity_by_id(bucket.owner_account);
+            expect(owner_data.name).toBe(account_defaults2.name);
         });
 
         it('should fail - cli bucket update - without identifier', async () => {
@@ -878,9 +879,11 @@ async function create_json_file(path_to_dir, file_name, data) {
  * assert_bucket will verify the fields of the buckets (only required fields)
  * @param {object} bucket
  * @param {object} bucket_options
+ * @param {import('../../../sdk/config_fs').ConfigFS} config_fs
  */
-function assert_bucket(bucket, bucket_options) {
+async function assert_bucket(bucket, bucket_options, config_fs) {
     expect(bucket.name).toEqual(bucket_options.name);
-    expect(bucket.bucket_owner).toEqual(bucket_options.owner);
+    const owner_data = await config_fs.get_identity_by_id(bucket.owner_account);
+    expect(owner_data.name).toBe(bucket_options.owner);
     expect(bucket.path).toEqual(bucket_options.path);
 }