IAM | Change bucket owner for IAM user to account and more #9268
Conversation
WalkthroughAccount/IAM flows switched from using account name strings to account Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as AccountSpaceNB
participant Util as account_util
participant DB
participant IAM as iam_utils
Note over Client,API: Create user (uses requesting_account._id)
Client->>API: create_user(params)
API->>Util: get_account_name_from_username(username, requesting_account._id)
API->>IAM: compute_arn(owner_id=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: Get/List user (resolve via _id)
Client->>API: get_user(params)
API->>API: validate_and_return_requested_account(params, action, requesting_account)
API->>Util: get_account_name_from_username(username, requested_account._id)
API->>IAM: compute_arn(owner_id=requested_account._id, iam_path, username)
API-->>Client: 200 OK
Note over Client,API: Delete user (pre-check)
Client->>API: delete_user(params)
API->>Util: get_account_name_from_username(username, requesting_account._id)
API->>Util: _check_if_user_does_not_have_access_keys_before_deletion(requested_account, username)
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 warning)
✅ Passed checks (2 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
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.
shirady
left a comment
shirady
left a comment
There was a problem hiding this comment.
Adding a few comment - most of them are bugs that I saw (you can decide what you want to fix in this PR and what later).
naveenpaul1
force-pushed
the
iam_bucket_owner
branch
from
c11ab71 to
55b8caa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/util/account_util.js (1)
547-555: Handle both embedded owner objects and raw IDs in_get_account_owner_id_for_arn
requesting_account.ownercan still be persisted as a plain string/ObjectId. Returningrequesting_account.owner._idleavesundefinedin that case, which flows intocreate_arn_for_userand breaks access-denied errors. Guard both shapes.Apply this diff:
function _get_account_owner_id_for_arn(requesting_account, requested_account) { if (!requesting_account.iam_operate_on_root_account) { if (requesting_account.owner !== undefined) { - return requesting_account.owner._id; + const owner_ref = requesting_account.owner; + return owner_ref && (owner_ref._id || owner_ref); } return requesting_account._id; } - return requested_account?._id; + return requested_account?._id; }src/sdk/accountspace_nb.js (1)
509-520:validate_and_return_requested_accountbuilds usernames with the root account nameAfter switching IAM usernames to
<username>:<rootAccountId>, this helper still callsget_account_name_from_username(params.username, requesting_account.name.unwrap()), so lookups miss every IAM user created under the new scheme. Please passrequesting_account._id.toString()(and keep usingSensitiveString) when composingaccount_email.Apply this diff:
- const account_email = account_util.get_account_name_from_username(params.username, requesting_account.name.unwrap()); + const account_email = account_util.get_account_name_from_username(params.username, requesting_account._id.toString());
♻️ Duplicate comments (1)
src/server/system_services/bucket_server.js (1)
245-249: Owner lookup still breaks whenowneris stored as an ID string
create_accountpersistsowneras either an embedded object or a plain ID string. When it’s the latter,req.account.owner._idisundefined, soaccount_idstays undefined and IAM-created buckets lose their owner linkage. Please guard both shapes (object vs string) before callingnew_bucket_defaults.Apply this diff:
- if (req.account.owner) { - account_id = req.account.owner._id; - } + if (req.account.owner) { + const owner_ref = req.account.owner; + account_id = owner_ref && (owner_ref._id || owner_ref); + }Based on learnings
🧹 Nitpick comments (1)
src/util/account_util.js (1)
408-414: Remove duplicate_check_if_user_does_not_have_access_keys_before_deletiondeclarationThere’s already another function with the same name later in the file (Line 642 onward). Because of hoisting, the later definition overwrites this one, so this new block is dead code and just adds confusion. Please drop the earlier copy or consolidate into a single definition.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sdk/accountspace_nb.js(12 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(7 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 (1)
- src/server/system_services/schemas/role_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 (2)
src/endpoint/iam/iam_constants.js (2)
IAM_DEFAULT_PATH(69-69)IAM_SPLIT_CHARACTERS(86-86)src/util/account_util.js (7)
req(718-718)username(369-369)username(399-400)username(449-450)_(4-4)system_store(12-12)member(590-597)
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/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
⏰ 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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
src/util/account_util.js
Outdated
| 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 }); | ||
| } |
There was a problem hiding this comment.
Log fix is overwritten by the later _throw_error_delete_conflict
This new definition never runs because the old function _throw_error_delete_conflict at Line 499 still exists below and overrides it. The logged module name therefore remains AccountSpaceNB. Please update the final definition (or remove it) instead of introducing an earlier duplicate.
🤖 Prompt for AI Agents
In src/util/account_util.js around lines 433 to 439, you introduced a new
_throw_error_delete_conflict function that is being shadowed by the existing
definition at line ~499 so the intended log/module name change never takes
effect; remove the earlier duplicate (or replace the final definition) so
there's only one _throw_error_delete_conflict implementation, ensuring the final
remaining function contains the corrected dbg.error call and message
construction, and run tests/linters to confirm no other references rely on the
overwritten variant.
| function _throw_access_denied_error(action, requesting_account, details, entity) { | ||
| const full_action_name = get_action_message_title(action); | ||
| const account_id_for_arn = _get_account_owner_id_for_arn(requesting_account); | ||
| const arn_for_requesting_account = create_arn_for_user(account_id_for_arn, | ||
| requesting_account.name.unwrap(), requesting_account.path || IAM_DEFAULT_PATH); | ||
| const account_id_for_arn = _get_account_owner_id_for_arn(requesting_account).toString(); | ||
| const arn_for_requesting_account = create_arn_for_user(account_id_for_arn, get_iam_username(requesting_account.name.unwrap()), | ||
| requesting_account.path || IAM_DEFAULT_PATH); |
There was a problem hiding this comment.
account_id_for_arn can throw when _get_account_owner_id_for_arn returns undefined
After switching _get_account_owner_id_for_arn to pull _id, it now returns undefined whenever requesting_account.owner is stored as a string/ObjectId. Calling .toString() on undefined will throw. Please make _get_account_owner_id_for_arn handle both embedded objects and plain IDs before this call.
🤖 Prompt for AI Agents
In src/util/account_util.js around lines 474-478, the call to
_get_account_owner_id_for_arn can return undefined when requesting_account.owner
is stored as a string/ObjectId; update _get_account_owner_id_for_arn so it
accepts either an embedded owner object (extract and return owner._id), or a
plain id/ObjectId/string (return it as-is), and ensure the function always
returns a value (coerce to string where appropriate) so that callers can safely
call .toString() without throwing.
55b8caa to
b1ba7ba
Compare
naveenpaul1
force-pushed
the
iam_bucket_owner
branch
from
b1ba7ba to
8dfa0f6
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(19 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(4 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 (1)
- src/util/account_util.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 (2)
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/util/account_util.js (15)
require(8-8)require(14-14)require(16-16)require(17-19)req(702-702)username(369-369)username(399-400)username(433-434)_(4-4)account(32-44)account(198-198)account(306-306)account(331-331)account(370-370)account(645-645)src/endpoint/iam/iam_utils.js (4)
require(6-6)require(7-8)require(10-10)_(4-4)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
⏰ 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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (15)
src/server/system_services/bucket_server.js (1)
244-251: LGTM! IAM bucket ownership correctly implemented.The logic properly attributes bucket ownership to the parent account for IAM users while maintaining the requesting account as owner for non-IAM users. The implementation aligns with the PR objective to change bucket owner for IAM users from the user to the account.
Based on the past review discussion and codebase verification,
req.account.owneris consistently stored as an object reference (account_id), so accessingreq.account.owner._idis the correct approach.src/server/system_services/schemas/role_schema.js (1)
26-29: Verify no existing database records use the removed 'iam_user' role value.Code verification complete—no references to
role='iam_user'found in the codebase. The 'iam_user' references present are related to IAM user management infrastructure (functions, variables), not the role enum. However, manually verify that no existing database records haverole='iam_user'before deployment, as removing this enum value is a breaking change for any such records.src/sdk/accountspace_nb.js (13)
63-63: Verify the security implications of assigning 'admin' role to IAM users.The role has been changed from
'iam_user'to'admin', which grants IAM users full administrative privileges. Confirm this is intentional and aligns with the desired IAM permission model, as it significantly elevates IAM user privileges.
85-103: LGTM!The function correctly handles the optional
usernameparameter (line 89) and uses the requesting account's_idfor ARN construction (lines 90-91), aligning with the PR's shift to ID-based account resolution.
105-143: LGTM!The function correctly uses conditional updates for
new_iam_pathandnew_username(lines 118-119) and consistently uses the requesting account's_idfor account name and ARN construction (lines 108, 120-121).
202-231: LGTM!The function correctly uses the new
validate_and_return_requested_accountsignature and properly handles account email and name references.
249-277: LGTM!The function correctly uses the updated
validate_and_return_requested_accountsignature.
279-300: LGTM!The function correctly uses the updated
validate_and_return_requested_accountsignature.
302-312: LGTM!The function correctly uses the updated
validate_and_return_requested_accountsignature.
318-359: LGTM!The function correctly uses the requesting account's
_idfor username resolution (line 321).
361-388: LGTM!The function correctly uses the requesting account's
_idfor username resolution (line 364).
390-419: LGTM!The function correctly uses the requesting account's
_idfor username resolution (line 393).
425-499: LGTM!All policy functions (
put_user_policy,get_user_policy,delete_user_policy,list_user_policies) correctly use the updatedvalidate_and_return_requested_accountsignature.
503-519: LGTM!The helper function's signature change (removing
account_sdkparameter) is correct and consistently applied across all call sites. The function properly uses the requesting account's_idfor account lookup (line 512).
155-155: Access keys must be deleted before user deletion, not just validated.The current code at line 155 calls
_check_if_user_does_not_have_resources_before_deletion, which validates that access keys do not exist and throws an error if they do. However, the PR objectives state "Delete access keys before deleting user," indicating that access keys should be automatically deleted during the user deletion process rather than blocking it. Additionally, line 156 contains a TODO comment about manually deleting inline policies, suggesting this deletion workflow is incomplete. Please either delete access keys automatically before the validation check or clarify the intended deletion strategy.
| const action = IAM_ACTIONS.TAG_USER; | ||
| const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); | ||
| const username = account_util.get_account_name_from_username(params.username, requesting_account.name.unwrap()); | ||
| const username = account_util.get_account_name_from_username(params.username, requesting_account._id.toString()); |
There was a problem hiding this comment.
This is not username but account_name, for example account1 creates user Robert.
username: Robert
account name (as you decide on): Robert:id-of-account1
You can leave it as gap if you want
There was a problem hiding this comment.
I dont think its a account, its IAM user
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_bucket_owner
branch
from
8dfa0f6 to
df2b2a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/account_util.js (1)
531-539: Critical:_get_account_owner_id_for_arnmay return undefined, causing downstream.toString()to throw.Line 534 accesses
requesting_account.owner._id, butownercan be stored as either:
- A plain ObjectId reference (from line 59 in accountspace_nb.js:
owner: requesting_account._id)- A populated object with an
_idpropertyWhen
owneris a plain ObjectId,owner._idevaluates toundefined. Callers like line 460 then call.toString()onundefined, causing a runtime error.Normalize the owner reference to handle both cases:
function _get_account_owner_id_for_arn(requesting_account, requested_account) { if (!requesting_account.iam_operate_on_root_account) { if (requesting_account.owner !== undefined) { - return requesting_account.owner._id; + // Handle both plain ObjectId and populated object + return requesting_account.owner._id || requesting_account.owner; } return requesting_account._id; } return requested_account?._id; }Based on past review comments.
♻️ Duplicate comments (2)
src/sdk/accountspace_nb.js (1)
171-189: Critical: Inconsistent null-safety forownerreference in ARN construction.Line 173 correctly uses optional chaining (
account.owner?._id), but line 182 directly accessesiam_user.owner._idwithout null-safety. Given that:
- Line 59 stores
owneras a plain ObjectId:owner: requesting_account._id- The filter at line 173 suggests
ownermight be undefined or an objectLine 182 will throw a runtime error when
owneris a plain ObjectId (whereowner._idisundefined) or whenownerisundefined.Apply consistent null-safe owner resolution:
let members = _.map(requesting_account_iam_users, function(iam_user) { const iam_username = account_util.get_iam_username(iam_user.name.unwrap()); const iam_path = iam_user.iam_path || IAM_DEFAULT_PATH; + const owner_id = iam_user.owner?._id || iam_user.owner; const member = { user_id: iam_user._id.toString(), iam_path: iam_path, username: iam_username, - arn: iam_utils.create_arn_for_user(iam_user.owner._id.toString(), iam_username, iam_path), + arn: iam_utils.create_arn_for_user(owner_id.toString(), iam_username, iam_path),Based on past review comments.
src/util/account_util.js (1)
321-323: Type inconsistency:requesting_account_idparameter receives ObjectId but name implies string.The parameter name
requesting_account_idsuggests a string, but callers (e.g., line 369) passrequesting_account._id, which is an ObjectId. While the template string will implicitly coerce it via.toString(), this creates ambiguity about the expected type.Consider explicitly converting to string at call sites for clarity:
-function get_account_name_from_username(username, requesting_account_id) { +function get_account_name_from_username(username, requesting_account_id) { return new SensitiveString(`${username}:${requesting_account_id}`); }Then update the caller:
- const username = get_account_name_from_username(params.username, requesting_account._id); + const username = get_account_name_from_username(params.username, requesting_account._id.toString());Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/sdk/accountspace_nb.js(19 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(4 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/schemas/role_schema.js
- src/server/system_services/bucket_server.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/util/account_util.js (5)
src/server/system_services/schemas/account_schema.js (1)
SensitiveString(4-4)src/api/common_api.js (1)
SensitiveString(4-4)src/sdk/accountspace_fs.js (1)
SensitiveString(16-16)src/server/system_services/account_server.js (1)
SensitiveString(15-15)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
src/sdk/accountspace_nb.js (3)
src/util/account_util.js (38)
require(8-8)require(14-14)require(16-16)require(17-19)req(702-702)username(369-369)username(399-400)username(433-434)system_store(12-12)_(4-4)account(32-44)account(198-198)account(306-306)account(331-331)account(370-370)account(645-645)member(574-581)message_with_details(334-334)message_with_details(373-373)message_with_details(402-402)message_with_details(439-439)message_with_details(446-446)message_with_details(453-453)message_with_details(465-465)message_with_details(486-486)message_with_details(497-497)message_with_details(604-604)IamError(15-15)IamError(335-335)IamError(374-374)IamError(403-403)IamError(440-440)IamError(447-447)IamError(454-454)IamError(479-479)IamError(487-487)IamError(498-498)IamError(605-605)src/endpoint/iam/iam_utils.js (14)
require(6-6)require(7-8)require(10-10)_(4-4)message_with_details(76-77)message_with_details(250-250)message_with_details(475-476)message_with_details(505-505)message_with_details(532-533)message_with_details(539-540)IamError(78-78)IamError(251-251)IamError(477-477)IamError(506-506)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
⏰ 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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/sdk/accountspace_nb.js (3)
87-95: LGTM: Correctly handles optional username parameter.Lines 87-95 properly handle the case where
params.usernameis undefined (per AWS IAM GetUser API specification) by falling back torequested_account.name.unwrap(). ARN construction and username derivation follow the new ID-based pattern consistently.
198-227: LGTM: Correctly handles optional username in CreateAccessKey.The function properly delegates username resolution to
validate_and_return_requested_account, which handles the optionalparams.usernameparameter per AWS IAM specifications. Email and username extraction usingget_iam_usernameare consistent and correct.
499-515: LGTM: Signature change correctly implements ID-based account resolution.The updated signature removes the
account_sdkparameter and correctly implements ID-based username resolution. Line 508 properly convertsrequesting_account._idto string when callingget_account_name_from_username, addressing previous concerns about using root account names.
shirady
left a comment
shirady
left a comment
There was a problem hiding this comment.
Approving, there are gaps that can be handed in future PRs
2 participants
Describe the Problem
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Bug Fixes
Breaking Changes
Improvements