IAM | Change bucket owner for IAM user to account and more #9268
Conversation
WalkthroughAccount/user and IAM flows were refactored to use requesting_account._id for name resolution and ARN construction; SensitiveString usage removed; role enum no longer includes 'iam_user'; account schema dropped public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as AccountSpaceNB
participant IAM as iam_utils
participant Util as account_util
participant DB
Note over Client,API: Create user (new flow uses account _id)
Client->>API: create_user(request, username)
API->>Util: get_account_name_from_username(username, requesting_account._id)
API->>IAM: compute_arn(for requesting_account._id, iam_path, username)
API->>DB: persist user (name uses _id-based composition)
DB-->>API: created
API-->>Client: 201 Created
Note over Client,API: Delete user (with pre-check)
Client->>API: delete_user(request, username)
API->>Util: get_account_name_from_username(username, requesting_account._id)
API->>Util: _check_if_user_does_not_have_access_keys_before_deletion(action, account)
alt has access keys
Util-->>API: throws DeleteConflict
API-->>Client: 409 DeleteConflict
else no access keys
API->>DB: delete user
DB-->>API: deleted
API-->>Client: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/accountspace_nb.js (1)
123-145: Use the owning root account ID when rebuilding the user ARN.Here the ARN switches to
requested_account._id, which is the IAM user’s document ID. ARNs should keep the root account ID (or its owner reference) just likecreate_user, otherwise callers get different ARNs for the same user. Please derive the account segment fromrequested_account.owner(falling back to_idwhen no owner exists) before callingcreate_arn_for_user.- const iam_arn = iam_utils.create_arn_for_user(requested_account._id.toString(), user_name, iam_path); + const owner_id = (requested_account.owner && (requested_account.owner._id || requested_account.owner)) || + requested_account._id; + const iam_arn = iam_utils.create_arn_for_user(owner_id.toString(), user_name, iam_path);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sdk/accountspace_nb.js(9 hunks)src/server/system_services/bucket_server.js(1 hunks)src/server/system_services/schemas/account_schema.js(0 hunks)src/server/system_services/schemas/role_schema.js(1 hunks)src/util/account_util.js(5 hunks)
💤 Files with no reviewable changes (1)
- src/server/system_services/schemas/account_schema.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/server/system_services/bucket_server.js
🧬 Code graph analysis (3)
src/server/system_services/bucket_server.js (1)
src/server/system_services/system_server.js (2)
account_id(352-352)bucket(237-242)
src/sdk/accountspace_nb.js (3)
src/server/system_services/account_server.js (7)
account_util(25-25)account_util(41-41)params(315-315)params(678-678)params(942-942)system_store(17-17)_(5-5)src/util/account_util.js (5)
system_store(12-12)username(369-369)username(399-400)username(449-450)_(4-4)src/endpoint/iam/iam_constants.js (2)
IAM_DEFAULT_PATH(55-55)IAM_SPLIT_CHARACTERS(71-71)
src/util/account_util.js (4)
src/server/system_services/schemas/account_schema.js (1)
SensitiveString(4-4)src/api/common_api.js (1)
SensitiveString(4-4)src/server/system_services/account_server.js (6)
SensitiveString(15-15)params(315-315)params(678-678)params(942-942)account_to_delete(403-403)dbg(12-12)src/sdk/accountspace_nb.js (2)
dbg(7-7)IamError(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
| // Buckets created by IAM users are owned by the IAM account the user belongs to. | ||
| let account_id = req.account._id; | ||
| // Only IAM user will have owner. | ||
| if (req.account.owner) { | ||
| account_id = req.account.owner._id; | ||
| } |
There was a problem hiding this comment.
Fix IAM bucket owner lookup when owner is stored as a string.
create_account persists owner as the root account’s ID string, so req.account.owner._id is undefined. That leaves account_id undefined and creates buckets without a valid owner_account. Guard both representations (object vs string) so IAM-created buckets still land under the root owner.
- let account_id = req.account._id;
- // Only IAM user will have owner.
- if (req.account.owner) {
- account_id = req.account.owner._id;
- }
+ let account_id = req.account._id;
+ if (req.account.owner) {
+ const owner_ref = req.account.owner;
+ account_id = owner_ref && (owner_ref._id || owner_ref);
+ }🤖 Prompt for AI Agents
In src/server/system_services/bucket_server.js around lines 244 to 249, the IAM
owner lookup assumes req.account.owner is an object with _id which can be
undefined when owner is stored as a string; update the logic to detect both
cases and set account_id to req.account.owner._id if owner is an object, or to
req.account.owner if it is a string (or fallback to req.account._id) so
IAM-created buckets get the correct owner_account.
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
iam_bucket_owner
branch
from
a333cda to
c11ab71
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sdk/accountspace_nb.js(9 hunks)src/server/system_services/bucket_server.js(1 hunks)src/server/system_services/schemas/account_schema.js(0 hunks)src/server/system_services/schemas/role_schema.js(1 hunks)src/util/account_util.js(5 hunks)
💤 Files with no reviewable changes (1)
- src/server/system_services/schemas/account_schema.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/system_services/bucket_server.js
- src/sdk/accountspace_nb.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/util/account_util.js (3)
src/server/system_services/schemas/account_schema.js (1)
SensitiveString(4-4)src/server/system_services/account_server.js (5)
SensitiveString(15-15)params(315-315)params(678-678)params(942-942)account_to_delete(403-403)src/sdk/accountspace_nb.js (1)
IamError(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (4)
src/util/account_util.js (3)
369-369: LGTM!The call is correctly updated to pass
requesting_account._idmatching the parameter rename inget_account_name_from_username.
674-674: LGTM!The export statement correctly makes the validation function available for use in other modules.
320-322: Verify backward compatibility strategy before deployment.The parameter change from
requesting_account_nametorequesting_account_idcreates a breaking format change in persisted account names. Existing IAM users with formatusername:accountnamewill not be found by lookups usingusername:accountid. The code includes defensive filtering (lines 180-181 in accountspace_nb.js) that silently excludes non-matching accounts, but there is no migration script.Verify:
- Whether existing deployments have IAM users with the old format that require migration
- Whether this is applied to fresh deployments only (in which case no migration is needed)
- Whether the silent filtering of mismatched accounts is intentional isolation or a hidden data access risk
src/server/system_services/schemas/role_schema.js (1)
28-28: Verify database state before finalizing this breaking schema change.The git diff confirms 'iam_user' was removed from the role enum. However, codebase analysis reveals:
- No active code assigns 'iam_user' as a role (all role assignments use 'admin' or 'operator')
- No test fixtures reference it
- No migration files exist in the repository
While code usage appears safe, you must manually verify that no existing database records have 'iam_user' as a role value before merging, since no data migration is present to handle this breaking change.
| function _check_if_user_does_not_have_access_keys_before_deletion(action, account_to_delete) { | ||
| const resource_name = 'access keys'; | ||
| const is_access_keys_exists = account_to_delete.access_keys && account_to_delete.access_keys.length > 0; | ||
| if (is_access_keys_exists) { | ||
| _throw_error_delete_conflict(action, account_to_delete, resource_name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix indentation on line 410.
The validation logic correctly prevents deletion of users with active access keys. However, line 410 has inconsistent indentation.
Apply this diff to fix the indentation:
function _check_if_user_does_not_have_access_keys_before_deletion(action, account_to_delete) {
const resource_name = 'access keys';
- const is_access_keys_exists = account_to_delete.access_keys && account_to_delete.access_keys.length > 0;
- if (is_access_keys_exists) {
- _throw_error_delete_conflict(action, account_to_delete, resource_name);
- }
+ const is_access_keys_exists = account_to_delete.access_keys && account_to_delete.access_keys.length > 0;
+ if (is_access_keys_exists) {
+ _throw_error_delete_conflict(action, account_to_delete, resource_name);
+ }
}🤖 Prompt for AI Agents
In src/util/account_util.js around lines 408 to 414, line 410 has inconsistent
indentation; align the const is_access_keys_exists line with the surrounding
code (same indent as resource_name) so the function body uses consistent
indentation and readability is restored; update indentation only—no logic
changes.
| function _throw_error_delete_conflict(action, account_to_delete, resource_name) { | ||
| dbg.error(`AccountSpaceFS.${action} requested account ` + | ||
| `${account_to_delete.name} ${account_to_delete._id} has ${resource_name}`); | ||
| const message_with_details = `Cannot delete entity, must delete ${resource_name} first.`; | ||
| const { code, http_code, type } = IamError.DeleteConflict; | ||
| throw new IamError({ code, message: message_with_details, http_code, type }); | ||
| } |
There was a problem hiding this comment.
Fix incorrect module name in error message.
The error message on line 434 references AccountSpaceFS but this function is in account_util.js. This appears to be a copy-paste error.
Apply this diff to fix the module name:
function _throw_error_delete_conflict(action, account_to_delete, resource_name) {
- dbg.error(`AccountSpaceFS.${action} requested account ` +
+ dbg.error(`account_util.${action} requested account ` +
`${account_to_delete.name} ${account_to_delete._id} has ${resource_name}`);
const message_with_details = `Cannot delete entity, must delete ${resource_name} first.`;
const { code, http_code, type } = IamError.DeleteConflict;
throw new IamError({ code, message: message_with_details, http_code, type });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _throw_error_delete_conflict(action, account_to_delete, resource_name) { | |
| dbg.error(`AccountSpaceFS.${action} requested account ` + | |
| `${account_to_delete.name} ${account_to_delete._id} has ${resource_name}`); | |
| const message_with_details = `Cannot delete entity, must delete ${resource_name} first.`; | |
| const { code, http_code, type } = IamError.DeleteConflict; | |
| throw new IamError({ code, message: message_with_details, http_code, type }); | |
| } | |
| function _throw_error_delete_conflict(action, account_to_delete, resource_name) { | |
| dbg.error(`account_util.${action} requested account ` + | |
| `${account_to_delete.name} ${account_to_delete._id} has ${resource_name}`); | |
| const message_with_details = `Cannot delete entity, must delete ${resource_name} first.`; | |
| const { code, http_code, type } = IamError.DeleteConflict; | |
| throw new IamError({ code, message: message_with_details, http_code, type }); | |
| } |
🤖 Prompt for AI Agents
In src/util/account_util.js around lines 433 to 439, the dbg.error call
incorrectly references "AccountSpaceFS"; update the module name to
"account_util" (or "AccountUtil" if you prefer PascalCase for module
identifiers) in the logged string so the error message accurately reflects this
file, keeping the rest of the interpolation and formatting unchanged.
1 participant
Describe the Problem
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Bug Fixes
Breaking Changes
Improvements