IAM_NOOBAA | IAM Create account #9242
Conversation
naveenpaul1
requested review from
dannyzaken,
jackyalbo,
liranmauda and
shirady
WalkthroughAdds IAM-related handling and account lifecycle refactors: new AccountSpaceNB and NBAccountSDK wired per-request, centralized account logic in a new account_util, schema and config updates for IAM and metrics, and a new IAM constant. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Endpoint
participant NBAccountSDK
participant AccountSpaceNB
participant AccountUtil
participant Store
Note over Endpoint,NBAccountSDK: init attaches per-request account_sdk
Client->>Endpoint: IAM user request
Endpoint->>NBAccountSDK: forward request
NBAccountSDK->>AccountSpaceNB: dispatch IAM operation
AccountSpaceNB->>AccountUtil: validate & perform action
AccountUtil->>Store: read/write accounts/roles/keys
Store-->>AccountUtil: result
AccountUtil-->>AccountSpaceNB: outcome
AccountSpaceNB-->>NBAccountSDK: response
NBAccountSDK-->>Endpoint: response
Endpoint-->>Client: result
sequenceDiagram
autonumber
participant API as account_server
participant AccountUtil
participant Store
rect rgb(245,250,255)
Note right of API: Refactored account flows delegated to AccountUtil
API->>AccountUtil: create_account(req)
AccountUtil->>Store: persist account/roles/keys
Store-->>AccountUtil: ok
AccountUtil-->>API: token + access_keys
end
API->>AccountUtil: generate_account_keys(req)
AccountUtil->>Store: update encrypted keys
Store-->>AccountUtil: ok
AccountUtil-->>API: decrypted keys
API->>AccountUtil: delete_user(req, target)
AccountUtil->>Store: remove account/roles
Store-->>AccountUtil: ok
AccountUtil-->>API: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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: 9
🧹 Nitpick comments (12)
config.js (2)
1025-1025: Default‑enabling IAM on 7444 may unintentionally expose a new port; mirror STS gatingPreviously IAM was disabled by default (-1). Consider disabling unless explicitly enabled, and mirror the STS NC_NSFS_NO_DB_ENV guard.
Apply:
-config.ENDPOINT_SSL_IAM_PORT = Number(process.env.ENDPOINT_SSL_IAM_PORT) || 7444; +config.ENDPOINT_SSL_IAM_PORT = Number(process.env.ENDPOINT_SSL_IAM_PORT) + || (process.env.NC_NSFS_NO_DB_ENV === 'true' ? -1 : 7444);
1029-1031: ALLOW_HTTP_METRICS/ALLOW_HTTPS_METRICS are not enforcedThese flags are defined but not used when starting metrics servers. Gate the ports in endpoint.js.
Add in endpoint.js (inside main):
-const http_metrics_port = options.http_metrics_port || config.EP_METRICS_SERVER_PORT; -const https_metrics_port = options.https_metrics_port || config.EP_METRICS_SERVER_SSL_PORT; +const http_metrics_port = config.ALLOW_HTTP_METRICS + ? (options.http_metrics_port || config.EP_METRICS_SERVER_PORT) + : 0; +const https_metrics_port = config.ALLOW_HTTPS_METRICS + ? (options.https_metrics_port || config.EP_METRICS_SERVER_SSL_PORT) + : 0;src/endpoint/iam/iam_constants.js (1)
61-61: Naming nit: prefer singular or a clearer intentIAM_SPLIT_CHARACTERS holds a single ':'; consider renaming to IAM_SPLIT_CHARACTER or IAM_ARN_DELIM for clarity.
Also applies to: 74-74
src/endpoint/endpoint.js (1)
97-105: Add account_sdk to EndpointRequest typedefJSDoc does not include account_sdk.
Apply:
* @typedef {import('http').IncomingMessage & { * object_sdk?: ObjectSDK; * sts_sdk?: StsSDK; + * account_sdk?: AccountSDK; * virtual_hosts?: readonly string[]; * bucket_logger?: PersistentLogger; * notification_logger?: PersistentLogger; * }} EndpointRequestsrc/sdk/accountspace_nb.js (5)
56-56: Use IAM_SPLIT_CHARACTERS for composing pseudo-emailAvoid inlining ':' to keep consistency with split logic.
- const account_name = new SensitiveString(`${params.username}:${requesting_account.name.unwrap()}`); + const account_name = new SensitiveString( + `${params.username}${IAM_SPLIT_CHARACTERS}${requesting_account.name.unwrap()}` + );
103-112: Use actual account timestamps; remove placeholdersReplace placeholders with real values if available; otherwise omit.
- // TODO: Dates missing : GAP - create_date: new Date(), - password_last_used: new Date(), + create_date: requested_account.last_update ? new Date(requested_account.last_update) : undefined, + password_last_used: undefined, // until trackedPlease confirm
last_updateexists on accounts; if the schema stores acreate_time, use that instead.
167-172: Minor: normalize details object to usepathkeyAlign with
_throw_access_denied_error:- account_util._check_if_requesting_account_is_root_account(action, requesting_account, { username: params.username }); + account_util._check_if_requesting_account_is_root_account(action, requesting_account, { username: params.username, path: params.iam_path });
193-211: List users: prefer owner/is_iam filter over string parsing; fix date field
- Filtering via name split is brittle. Use
is_iamandowner === requesting_account._id.- Use a real timestamp from the account doc.
Apply:
- const requesting_account_iam_users = _.filter(system_store.data.accounts, function(acc) { - if (!acc.name.unwrap().includes(IAM_SPLIT_CHARACTERS)) { - return false; - } - return acc.name.unwrap().split(IAM_SPLIT_CHARACTERS)[1] === root_name; - }); + const root_id = String(requesting_account._id); + const requesting_account_iam_users = _.filter(system_store.data.accounts, acc => + acc.is_iam === true && acc.owner === root_id + ); @@ - create_date: iam_user.creation_date, - password_last_used: Date.now(), // GAP + create_date: iam_user.last_update ? new Date(iam_user.last_update) : undefined, + password_last_used: undefined,Please confirm
is_iamandownerare present on IAM users; otherwise we can fallback to name-split as a secondary condition.
220-249: Stubs: consider returning NotImplemented consistently with code/messageCurrent pattern is fine. Optionally, include the action name in the message to aid clients, e.g.,
message: 'CreateAccessKey NotImplemented'.src/util/account_util.js (3)
59-70: Avoid hardcoded default_resource from callers
create_accountalready throws if a provideddefault_resourcedoesn't exist. Recommend callers omitdefault_resourceunless validated. No code change here; documenting the expectation.
286-303: validate_assume_role_policy: minor robustnessIf
policy.statementor nested arrays contain non-SensitiveString principals,principal.unwrap()throws. Optionally normalize withprincipal instanceof SensitiveString ? principal.unwrap() : principal.
414-436: _throw_access_denied_error: expects details.path; align call sitesCallers pass
iam_path. Either accept both keys or standardize onpath. You fixed call sites above; alternatively make this tolerant:- user_message = create_arn_for_user(account_id_for_arn, details.username, details.path); + user_message = create_arn_for_user( + account_id_for_arn, + details.username, + details.path ?? details.iam_path + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config.js(1 hunks)src/endpoint/endpoint.js(3 hunks)src/endpoint/iam/iam_constants.js(2 hunks)src/sdk/accountspace_nb.js(1 hunks)src/server/system_services/account_server.js(4 hunks)src/server/system_services/schemas/account_schema.js(1 hunks)src/server/system_services/schemas/role_schema.js(1 hunks)src/server/system_services/schemas/system_schema.js(2 hunks)src/util/account_util.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/sdk/accountspace_nb.js (2)
src/util/account_util.js (21)
_(4-4)require(8-8)require(14-14)require(17-17)require(18-18)SensitiveString(9-9)dbg(7-7)system_store(12-12)IamError(15-15)IamError(312-312)IamError(345-345)IamError(373-373)IamError(410-410)IamError(434-434)req(525-525)account(31-43)account(198-198)account(291-291)account(308-308)account(341-341)account(468-468)src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-16)IAM_DEFAULT_PATH(47-47)IAM_SPLIT_CHARACTERS(61-61)
src/endpoint/endpoint.js (1)
src/sdk/account_sdk.js (1)
BucketSpaceNB(9-9)
src/util/account_util.js (2)
src/server/system_services/account_server.js (31)
_(5-5)require(13-13)require(21-21)require(22-22)require(23-23)Dispatcher(14-14)dbg(12-12)SensitiveString(15-15)cloud_utils(16-16)system_store(17-17)req(118-118)req(382-382)account(53-53)account(101-101)account(120-122)account(144-144)account(192-192)account(315-315)account(678-678)account(942-942)roles(448-448)account_to_delete(402-402)roles_to_delete(414-414)access_keys(156-156)system(197-197)system(1016-1016)params(314-314)params(677-677)params(941-941)status(892-892)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-16)ACCESS_KEY_STATUS_ENUM(34-37)
src/server/system_services/account_server.js (1)
src/sdk/accountspace_nb.js (1)
account_util(6-6)
⏰ 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 (7)
src/server/system_services/schemas/role_schema.js (1)
28-29: Ensure auth/role checks recognize 'iam_user'Adding 'iam_user' is fine. Verify any role-based checks, UI filters, and seeding/migrations include it (e.g., roles_by_system usage, admin/operator checks).
src/server/system_services/schemas/system_schema.js (1)
90-92: LGTM: schema now includes IAM service/apiEnums updated to include 'iam'. Make sure address discovery/registration emits entries for IAM accordingly.
Also applies to: 101-103
src/endpoint/endpoint.js (2)
461-465: Per-request NBAccountSDK wiring: LGTM; ensure consumers use req.account_sdkLooks good. Verify iam_rest and any other handlers read req.account_sdk and not instantiate their own.
75-96: AccountSDK constructor signature matches injected spaces
AccountSDK’s constructor accepts {rpc_client, internal_rpc_client, bucketspace, accountspace}, matching NBAccountSDK’s super call.src/server/system_services/account_server.js (2)
135-136: Delegating key generation: LGTMCentralizing in account_util is cleaner.
38-45: Safe to return only{ token, access_keys }: all existing callers only access those two fields.src/util/account_util.js (1)
142-147: Return field names:create_datevscreation_dateUpstream consumers used
creation_date. This function returnscreate_date. Keep it ascreate_datebut ensure consumers reference it correctly (see fixes in AccountSpaceNB).
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (7)
src/sdk/accountspace_nb.js (6)
52-54: Pass details.path (not details.iam_path) to access-denied builderWrong key breaks error messages/ARNs.
- account_util._check_if_requesting_account_is_root_account(action, requesting_account, - { username: params.username, iam_path: params.iam_path }); + account_util._check_if_requesting_account_is_root_account( + action, + requesting_account, + { username: params.username, path: params.iam_path } + );
58-70: Persist iam_path on created IAM user and remove hardcoded default_resourceAvoids stale path in later reads and brittle resource dependency.
rpc_params: { name: account_name, email: account_name, has_login: false, s3_access: true, allow_bucket_creation: true, owner: requesting_account._id.toString(), is_iam: true, iam_arn: iam_arn, + iam_path: params.iam_path || IAM_DEFAULT_PATH, role: 'iam_user', - // TODO: default_resource remove - default_resource: 'noobaa-default-backing-store', },
80-86: Fix returned iam_path and create_dateUse the created user’s path and the create_account reply field.
- iam_path: requesting_account.iam_path || IAM_DEFAULT_PATH, + iam_path: params.iam_path || IAM_DEFAULT_PATH, username: params.username, user_id: iam_account.id, arn: iam_arn, - create_date: iam_account.creation_date, + create_date: iam_account.create_date,
95-96: Pass details.path (not details.iam_path) to access-denied builderSame issue as in create_user.
- account_util._check_if_requesting_account_is_root_account(action, requesting_account, - { username: params.username, iam_path: params.iam_path }); + account_util._check_if_requesting_account_is_root_account( + action, + requesting_account, + { username: params.username, path: params.iam_path } + );
120-121: Pass details.path (not details.iam_path) to access-denied builderSame fix as above.
- account_util._check_if_requesting_account_is_root_account(action, requesting_account, - { username: params.username, iam_path: params.iam_path }); + account_util._check_if_requesting_account_is_root_account( + action, + requesting_account, + { username: params.username, path: params.iam_path } + );
167-168: Pass details.path (not details.iam_path) to access-denied builderAlign with access-denied message builder expectations.
- account_util._check_if_requesting_account_is_root_account(action, requesting_account, { username: params.username }); + account_util._check_if_requesting_account_is_root_account( + action, + requesting_account, + { username: params.username, path: params.iam_path } + );src/util/account_util.js (1)
114-118: Persist iam_path for IAM usersRequired for get/list/update correctness.
if (req.rpc_params.is_iam) { account.owner = req.rpc_params.owner; account.is_iam = req.rpc_params.is_iam; account.iam_arn = req.rpc_params.iam_arn; + if (req.rpc_params.iam_path) { + account.iam_path = req.rpc_params.iam_path; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/accountspace_nb.js(1 hunks)src/util/account_util.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/accountspace_nb.js (3)
src/util/account_util.js (20)
_(4-4)require(8-8)require(14-14)require(17-17)require(18-18)SensitiveString(9-9)system_store(12-12)IamError(15-15)IamError(312-312)IamError(345-345)IamError(373-373)IamError(410-410)IamError(434-434)req(525-525)account(31-43)account(198-198)account(291-291)account(308-308)account(341-341)account(468-468)src/server/system_services/account_server.js (22)
_(5-5)require(13-13)require(21-21)require(22-22)require(23-23)SensitiveString(15-15)account_util(25-25)account_util(40-40)system_store(17-17)params(314-314)params(677-677)params(941-941)req(118-118)req(382-382)account(53-53)account(101-101)account(120-122)account(144-144)account(192-192)account(315-315)account(678-678)account(942-942)src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-16)IAM_DEFAULT_PATH(47-47)IAM_SPLIT_CHARACTERS(61-61)
src/util/account_util.js (1)
src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-16)ACCESS_KEY_STATUS_ENUM(34-37)
⏰ 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 (1)
src/util/account_util.js (1)
544-555: Export set looks goodPublic helpers align with SDK usage.
| async update_user(params, account_sdk) { | ||
| const action = IAM_ACTIONS.UPDATE_USER; | ||
| const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); | ||
| const account_name = new SensitiveString(`${params.username}:${requesting_account.name.unwrap()}`); | ||
| account_util._check_if_requesting_account_is_root_account(action, requesting_account, | ||
| { username: params.username, iam_path: params.iam_path }); | ||
| account_util._check_if_account_exists(action, params.username, requesting_account); | ||
| const requested_account = system_store.get_account_by_email(account_name); | ||
| let new_iam_path = requested_account.iam_path; | ||
| let new_user_name = requested_account.name.unwrap(); | ||
| const root_account = _.find(system_store.data.accounts, account => account.name.unwrap() === requesting_account.name.unwrap()); | ||
| account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account); | ||
| account_util._check_if_requested_is_owned_by_root_account(action, root_account, requested_account); | ||
| if (params.new_iam_path !== undefined) new_iam_path = params.new_iam_path; | ||
| if (params.new_username !== undefined) new_user_name = params.new_username; | ||
| const iam_arn = iam_utils.create_arn_for_user(root_account._id.toString(), new_user_name, new_iam_path); | ||
| const new_account_name = new SensitiveString(`${params.new_username}:${requesting_account.name.unwrap()}`); | ||
| const updates = { | ||
| name: new_account_name, | ||
| email: new_account_name, | ||
| iam_arn: iam_arn, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
update_user: fix username derivation, prevent duplicates, persist path, await DB write, and return updated values
- new_user_name currently equals "username:root", producing invalid ARN/name.
- No check for duplicate new_username.
- iam_path not persisted in updates.
- Not awaiting make_changes (race/unhandled rejection).
- Response returns stale iam_path/username.
- let new_iam_path = requested_account.iam_path;
- let new_user_name = requested_account.name.unwrap();
+ const current_username = requested_account.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0];
+ let new_iam_path = requested_account.iam_path;
+ let new_user_name = current_username;
@@
- if (params.new_iam_path !== undefined) new_iam_path = params.new_iam_path;
- if (params.new_username !== undefined) new_user_name = params.new_username;
+ if (params.new_iam_path !== undefined) new_iam_path = params.new_iam_path;
+ if (params.new_username !== undefined) new_user_name = params.new_username;
+
+ // Prevent rename to an existing username under the same root
+ if (params.new_username !== undefined && params.new_username !== current_username) {
+ account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account);
+ }
const iam_arn = iam_utils.create_arn_for_user(root_account._id.toString(), new_user_name, new_iam_path);
- const new_account_name = new SensitiveString(`${params.new_username}:${requesting_account.name.unwrap()}`);
+ const new_account_name = new SensitiveString(
+ `${new_user_name}${IAM_SPLIT_CHARACTERS}${requesting_account.name.unwrap()}`
+ );
const updates = {
name: new_account_name,
email: new_account_name,
iam_arn: iam_arn,
+ iam_path: new_iam_path,
};
@@
- system_store.make_changes({
+ await system_store.make_changes({
update: {
accounts: [{
_id: requested_account._id,
$set: _.omitBy(updates, _.isUndefined),
}]
}
});
@@
- return {
- // TODO: IAM path needs to be saved
- iam_path: requested_account.iam_path || IAM_DEFAULT_PATH,
- username: requested_account.name.unwrap(),
- user_id: requested_account._id.toString(),
- arn: iam_arn
- };
+ return {
+ iam_path: new_iam_path || IAM_DEFAULT_PATH,
+ username: new_user_name,
+ user_id: requested_account._id.toString(),
+ arn: iam_arn,
+ };Also verify upstream param mapping (API → SDK): are NewUserName/NewPath mapped to new_username/new_iam_path?
Also applies to: 139-147, 150-156
🏁 Script executed:
#!/bin/bash
# 1. Locate update_user in file
rg -n "update_user" -C2 src/sdk/accountspace_nb.js
# 2. Check for existence of username-existence check
rg -n "check_username_already_exists" -C2 .
# 3. Find IAM_SPLIT_CHARACTERS definition
rg -n "IAM_SPLIT_CHARACTERS" -C2 .
# 4. Verify make_changes usage and async
rg -n "make_changes" -C2 src/sdk/accountspace_nb.js
# 5. Search for new_username param mapping
rg -n "new_username" -C2 .
# 6. Search for new_iam_path param mapping
rg -n "new_iam_path" -C2 .Length of output: 26916
update_user: implement correct rename flow, persist iam_path, await write, and return updated values
- Derive
current_usernameviarequested_account.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0]to avoid embedding root in username - Before applying
params.new_username, callaccount_util._check_username_already_exists(action, { username: params.new_username }, requesting_account)to block duplicates - Add
iam_path: new_iam_pathto theupdatesobject - Prefix
system_store.make_changeswithawaitto ensure the DB write completes - Return
{ iam_path: new_iam_path, username: new_user_name, user_id, arn }instead of stalerequested_accountvalues
Verified that API → SDK mappings for NewUserName/NewPath → new_username/new_iam_path in src/endpoint/iam/ops/iam_update_user.js are correct.
🤖 Prompt for AI Agents
In src/sdk/accountspace_nb.js around lines 116 to 137, the update_user flow
needs to correctly derive the current username, validate and apply a rename,
persist iam_path, await the DB write, and return the updated values: derive
current_username as
requested_account.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0] (so the root
suffix isn't included), before applying params.new_username call
account_util._check_username_already_exists(action, { username:
params.new_username }, requesting_account) to block duplicates, include
iam_path: new_iam_path in the updates object, prefix
system_store.make_changes(...) with await so the write completes, and return the
fresh values { iam_path: new_iam_path, username: new_user_name, user_id, arn }
(ensure user_id and arn are computed/available from the updated account) instead
of returning stale requested_account fields.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/util/account_util.js (3)
18-19: Use IAM_DEFAULT_PATH and iam_path when constructing ARNs; add the missing importImport IAM_DEFAULT_PATH and default to it when path is absent. Also prefer iam_path over requesting_account.path to avoid undefined in ARNs. This fixes incorrect/noisy error messages.
-const { IAM_ACTIONS, ACCESS_KEY_STATUS_ENUM } = require('../endpoint/iam/iam_constants'); +const { IAM_ACTIONS, ACCESS_KEY_STATUS_ENUM, IAM_DEFAULT_PATH } = require('../endpoint/iam/iam_constants');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); + const arn_for_requesting_account = create_arn_for_user( + account_id_for_arn, + requesting_account.name.unwrap(), + requesting_account.iam_path || IAM_DEFAULT_PATH + ); const basic_message = `User: ${arn_for_requesting_account} is not authorized to perform:` + `${full_action_name} on resource: `; let message_with_details; if (entity === 'USER') { let user_message; if (action === IAM_ACTIONS.LIST_ACCESS_KEYS) { user_message = `user ${details.username}`; } else { - user_message = create_arn_for_user(account_id_for_arn, details.username, details.path); + user_message = create_arn_for_user( + account_id_for_arn, + details.username instanceof SensitiveString ? details.username.unwrap() : details.username, + (details.path && details.path.unwrap ? details.path.unwrap() : details.path) || IAM_DEFAULT_PATH + ); } message_with_details = basic_message + `${user_message} because no identity-based policy allows the ${full_action_name} action`; } else { message_with_details = basic_message + `access key ${details.access_key}`; } const { code, http_code, type } = IamError.AccessDeniedException; throw new IamError({ code, message: message_with_details, http_code, type }); }Also applies to: 414-433
114-118: Persist iam_path for IAM users during create_accountCurrently iam_path is never stored; later reads expect requested_account.iam_path.
if (req.rpc_params.is_iam) { account.owner = req.rpc_params.owner; account.is_iam = req.rpc_params.is_iam; account.iam_arn = req.rpc_params.iam_arn; + if (req.rpc_params.iam_path) { + account.iam_path = req.rpc_params.iam_path; + } }
448-456: Avoid undefined account id in ARN owner resolutionProvide a fallback when requested_account is not supplied.
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; } return requesting_account._id; } - return requested_account?._id; + return requested_account ? requested_account._id : requesting_account._id; }
🧹 Nitpick comments (2)
src/util/account_util.js (2)
11-12: Normalize require paths (double slash)Minor cleanup; prevents path inconsistencies.
-const auth_server = require('..//server/common_services/auth_server'); -const system_store = require('..//server/system_services/system_store').get_instance(); +const auth_server = require('../server/common_services/auth_server'); +const system_store = require('../server/system_services/system_store').get_instance();
98-106: PII in logs (emails)Emails are PII; prefer masking or structured logging with redaction.
- Mask emails (e.g., a***@example.com) or omit them at info/alert levels.
- Centralize masking in Dispatcher or a log helper to avoid leaks.
Also applies to: 168-176, 179-187, 232-239
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util/account_util.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/util/account_util.js (2)
src/sdk/accountspace_nb.js (5)
_(4-4)require(12-12)SensitiveString(5-5)system_store(9-9)IamError(11-11)src/endpoint/iam/iam_constants.js (2)
IAM_ACTIONS(5-16)ACCESS_KEY_STATUS_ENUM(34-37)
⏰ 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-package-lock-validation
- GitHub Check: run-jest-unit-tests
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
src/server/system_services/account_server.js (1)
405-405: Verifydelete_userhandles full account cleanup for non-IAM accounts.The function name
delete_usersuggests IAM-specific logic. Ensure it performs complete cleanup (buckets, policies, access keys, roles, etc.) for both IAM and classic accounts, not just role removal.Based on past review feedback.
Run this script to verify the delete_user implementation:
#!/bin/bash # Verify delete_user handles full account cleanup rg -n -A 20 'function delete_user|exports\.delete_user' src/util/account_util.jssrc/sdk/accountspace_nb.js (8)
128-128: Only check for duplicate username if renaming.The duplicate check runs even when
params.new_usernameis undefined, and doesn't verify the new name differs from the current one.Apply this diff:
let new_user_name = requested_account.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0]; - account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account); const root_account = _.find(system_store.data.accounts, account => account.name.unwrap() === requesting_account.name.unwrap()); account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account); account_util._check_if_requested_is_owned_by_root_account(action, root_account, requested_account); if (params.new_iam_path !== undefined) new_iam_path = params.new_iam_path; - if (params.new_username !== undefined) new_user_name = params.new_username; + if (params.new_username !== undefined && params.new_username !== new_user_name) { + new_user_name = params.new_username; + account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account); + }Based on past review feedback.
153-159: Return updated values, not stale or undefined ones.The response uses stale or potentially undefined values. After the fix to line 127,
new_user_namewill be correct. However, ensurenew_iam_pathis defined (it might be undefined ifrequested_account.iam_pathis undefined).Apply this diff to ensure defaults:
return { - // TODO: IAM path needs to be saved iam_path: new_iam_path || IAM_DEFAULT_PATH, username: new_user_name, user_id: requested_account._id.toString(), arn: iam_arn };Based on past review feedback.
70-71: Remove hardcodeddefault_resource.Hardcoding
'noobaa-default-backing-store'will fail if that resource doesn't exist. Letaccount_util.create_accountresolve the default resource automatically.Apply this diff:
iam_path: params.iam_path, role: 'iam_user', - - // TODO: default_resource remove - default_resource: 'noobaa-default-backing-store', },Based on past review feedback.
81-88: Return the created user'siam_path, not the requesting account's.Line 83 returns
requesting_account.iam_path, but the response should reflect the created user's IAM path fromparams.iam_path.Apply this diff:
return { - // TODO : PATH Missing - iam_path: requesting_account.iam_path || IAM_DEFAULT_PATH, + iam_path: params.iam_path || IAM_DEFAULT_PATH, username: params.username, user_id: iam_account.id, arn: iam_arn, create_date: iam_account.create_date, };Based on past review feedback.
111-113: Use persisted creation timestamp and drop placeholderpassword_last_used.
- Line 111:
requested_account.last_updateis the last modification time, not creation time. Accounts should track creation timestamp separately.- Line 113: Returning
new Date()forpassword_last_usedis misleading—it returns "now" instead of the actual last password use time.Apply this diff:
arn: requested_account.iam_arn, - create_date: new Date(requested_account.last_update), - // TODO: Dates missing : GAP - password_last_used: new Date(), + create_date: requested_account.create_date || new Date(requested_account.last_update), + password_last_used: undefined, // Unknown; avoid misleading timestamps };Based on past review feedback.
127-127: Extract username without root suffix.
requested_account.name.unwrap()includes the root account suffix (e.g.,username:root). Extract just the username portion.Apply this diff:
const requested_account = system_store.get_account_by_email(account_name); let new_iam_path = requested_account.iam_path; - let new_user_name = requested_account.name.unwrap(); + let new_user_name = requested_account.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0]; account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account);Based on past review feedback.
136-140: Persistiam_pathin updates.The
updatesobject doesn't includeiam_path, so path changes won't be saved to the database.Apply this diff:
const updates = { name: new_account_name, email: new_account_name, iam_arn: iam_arn, + iam_path: new_iam_path, };Based on past review feedback.
188-218: ImplementPathPrefixfiltering and fix timestamp issues.
- Missing
PathPrefixfiltering: The method ignoresparams.path_prefix(orparams.PathPrefix), so it returns all users regardless of their IAM path.- Line 209: Uses
last_updateinstead of creation timestamp.- Line 211: Returns
Date.now()instead of actual password last-used time.Apply this diff:
async list_users(params, account_sdk) { const action = IAM_ACTIONS.LIST_USERS; - //const requesting_account = account_sdk.requesting_account; const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); account_util._check_if_requesting_account_is_root_account(action, requesting_account, { }); const is_truncated = false; // GAP - no pagination at this point + const path_prefix = params?.path_prefix || params?.PathPrefix; const root_name = requesting_account.name.unwrap(); - // CORE CHANGES PENDING - START const requesting_account_iam_users = _.filter(system_store.data.accounts, function(acc) { if (!acc.name.unwrap().includes(IAM_SPLIT_CHARACTERS)) { return false; } return acc.name.unwrap().split(IAM_SPLIT_CHARACTERS)[1] === root_name; }); let members = _.map(requesting_account_iam_users, function(iam_user) { + if (path_prefix && !(iam_user.iam_path || IAM_DEFAULT_PATH).startsWith(path_prefix)) { + return undefined; + } const member = { user_id: iam_user._id.toString(), iam_path: iam_user.iam_path || IAM_DEFAULT_PATH, username: iam_user.name.unwrap().split(IAM_SPLIT_CHARACTERS)[0], arn: iam_user.iam_arn, - create_date: new Date(iam_user.last_update), - // TODO: Miising password_last_used - password_last_used: Date.now(), // GAP + create_date: iam_user.create_date || new Date(iam_user.last_update), + password_last_used: undefined, // Unknown; avoid misleading timestamps }; return member; }); - // CORE CHANGES PENDING - END + members = members.filter(Boolean); members = members.sort((a, b) => a.username.localeCompare(b.username)); return { members, is_truncated }; }Based on past review feedback.
🧹 Nitpick comments (2)
src/server/system_services/schemas/account_schema.js (2)
31-31: Consider renamingownertoowner_accountfor clarity.The field name
owneris ambiguous. Usingowner_accountmakes it explicit that this references the owning account record.Based on past review feedback.
36-36: Add validation pattern foriam_arn.IAM ARNs follow AWS format (
arn:partition:iam:region:account-id:resource-type/resource-id). Adding a pattern constraint prevents invalid ARN values.Consider adding:
-iam_arn: { type: 'string' }, +iam_arn: { + type: 'string', + pattern: '^arn:[^:]*:iam:[^:]*:[^:]*:(user|role)/.+$' +},Based on past review feedback.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/sdk/accountspace_nb.js(1 hunks)src/server/system_services/account_server.js(4 hunks)src/server/system_services/schemas/account_schema.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/util/account_util.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/sdk/accountspace_nb.js (2)
src/util/account_util.js (21)
_(4-4)require(8-8)require(14-14)require(17-17)require(18-18)SensitiveString(9-9)dbg(7-7)system_store(12-12)IamError(15-15)IamError(312-312)IamError(345-345)IamError(373-373)IamError(410-410)IamError(434-434)req(522-522)account(31-43)account(199-199)account(291-291)account(308-308)account(341-341)account(465-465)src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-16)IAM_DEFAULT_PATH(47-47)IAM_SPLIT_CHARACTERS(61-61)
src/server/system_services/account_server.js (2)
src/sdk/accountspace_nb.js (2)
account_util(6-6)require(12-12)src/util/account_util.js (5)
require(8-8)require(14-14)require(17-17)require(18-18)req(522-522)
⏰ 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/server/system_services/account_server.js (2)
39-46: LGTM: Delegation to account_util centralizes account creation logic.The refactor to delegate account creation to
account_utilimproves code organization and reusability.
136-136: LGTM: Delegation to account_util for key generation.Consistent with the overall refactoring pattern to centralize account operations.
src/sdk/accountspace_nb.js (1)
163-186: LGTM: IAM user deletion flow is correct.The method properly validates ownership and delegates to
account_util.delete_user.
2 participants
Describe the Problem
This PR contains implementation for the following IAM account APIs
NOTE: DB changes added in this PR could change after desig discusion.
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
FYI : Noobaa operator changes are missing, Needs to create IAM service manually for testing
Summary by CodeRabbit
New Features
Changes
Refactor