IAM_NOOBAA | IAM Create account #9242
Conversation
naveenpaul1
requested review from
dannyzaken,
jackyalbo,
liranmauda and
shirady
WalkthroughAdds IAM-aware account management: new NBAccountSDK and AccountSpaceNB, per-request account_sdk initialization, centralized account_util for account creation/keys/deletion, account-server delegations, schema and constant updates, and IAM-related enum/value additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint
participant Req as Request
participant NBSDK as NBAccountSDK
participant AccountSpace as AccountSpaceNB
participant AccountUtil
participant Store as SystemStore
Client->>Endpoint: API call
Endpoint->>Req: create_init_request_sdk(req)
Req->>NBSDK: new NBAccountSDK(rpc_client, internal_rpc_client)
NBSDK-->>Req: attached account_sdk
Client->>Endpoint: account operation (e.g., create_user)
Endpoint->>AccountSpace: AccountSpaceNB.create_user(params, req.account_sdk)
AccountSpace->>AccountUtil: validate & request create_account(...)
AccountUtil->>Store: read/write accounts, keys, roles
Store-->>AccountUtil: result
AccountUtil-->>AccountSpace: created/updated metadata
AccountSpace-->>Endpoint: formatted response
Endpoint-->>Client: return API result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
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: 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.
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.
config.js
Outdated
| // Remove the NSFS condition when NSFS starts to support STS. | ||
| config.ENDPOINT_SSL_STS_PORT = Number(process.env.ENDPOINT_SSL_STS_PORT) || (process.env.NC_NSFS_NO_DB_ENV === 'true' ? -1 : 7443); | ||
| config.ENDPOINT_SSL_IAM_PORT = Number(process.env.ENDPOINT_SSL_IAM_PORT) || -1; | ||
| config.ENDPOINT_SSL_IAM_PORT = Number(process.env.ENDPOINT_SSL_IAM_PORT) || 7444; |
There was a problem hiding this comment.
In this change it is enabling the port for all deployments - NC case should not be affected.
WDYT? Can we change in other place?
There was a problem hiding this comment.
We can configure it in noobaa-config
There was a problem hiding this comment.
You can do the same as STS (in next PR - after choosing the port number), for example
// Remove the NSFS condition when NSFS starts to support STS.
config. ENDPOINT_SSL_IAM_PORT = Number(process.env. ENDPOINT_SSL_IAM_PORT) || (process.env.NC_NSFS_NO_DB_ENV === 'true' ? -1 : 13443);
| role: { | ||
| type: 'string', | ||
| enum: ['admin', 'user', 'operator'] | ||
| enum: ['admin', 'user', 'operator', 'iam_user'] |
There was a problem hiding this comment.
Why do we need 'iam_user' and also 'user'?
Do you know in which cases do we use 'user'?
There was a problem hiding this comment.
no, default value for current account is admin
There was a problem hiding this comment.
In general I don't mind adding this enum, but I'm not sure exactly where we use this role_schema.
naveenpaul1
force-pushed
the
iam_account
branch
from
5fef7ca to
ddbfc2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
src/server/system_services/account_server.js (1)
405-405: Verify delete_user handles all account types, not just IAM users.The function name
delete_usersuggests IAM-specific deletion, but it's being called fromdelete_accountfor all account types. Ensureaccount_util.delete_userproperly handles:
- Cleanup of roles (already implemented based on code context)
- Cleanup of buckets owned by classic (non-IAM) accounts
- Removal of bucket policies, user policies, and access keys
- Deletion of any other account-scoped resources
If
delete_useronly handles IAM users, consider either (A) extending it to handle all account types or (B) renaming it to reflect its actual scope and creating a separate function for classic account deletion.Run the following script to inspect the delete_user implementation:
#!/bin/bash # Inspect account_util.delete_user to verify it handles all account types rg -n -A 20 "function delete_user|exports.delete_user|delete_user.*=" src/util/account_util.jssrc/sdk/accountspace_nb.js (7)
126-127: Extract username without root suffix for correct default.Line 127 assigns the full account name (including
:root_account_namesuffix) tonew_user_name. This should extract only the username portion usingsplit(IAM_SPLIT_CHARACTERS)[0].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];
81-88: Return the created user's iam_path, not the requester's.Line 83 returns
requesting_account.iam_path(the root account's path), but should return the newly created IAM user's 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, };
136-140: Persist iam_path in the updates object.The
iam_pathfield is not included in theupdatesobject, so path changes won't be saved. Addiam_path: new_iam_pathto persist the updated path.Apply this diff:
const updates = { name: new_account_name, email: new_account_name, iam_arn: iam_arn, + iam_path: new_iam_path, };
97-98: Use consistent parameter name 'path' in access-denied details.Line 98 passes
{ username: params.username, iam_path: params.iam_path }, but_check_if_requesting_account_is_root_accountand the error builder expectpath, notiam_path. This breaks error messages.Apply this diff:
account_util._check_if_requesting_account_is_root_account(action, requesting_account, - { username: params.username, iam_path: params.iam_path }); + { username: params.username, path: params.iam_path });
70-71: Remove hardcoded default_resource and let account creation resolve it.The hardcoded
'noobaa-default-backing-store'may not exist in all deployments and will cause creation failures. Remove this line and allowaccount_util.create_accountto resolve 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', },
188-218: Implement PathPrefix filtering and fix password_last_used.Two issues in list_users:
Missing PathPrefix filter: The method doesn't filter by
params.path_prefix. Users should only be returned if theiriam_pathstarts with the specified prefix.Placeholder timestamp: Line 211 uses
Date.now()forpassword_last_used, which is misleading.Apply this diff:
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; } + if (path_prefix && !(acc.iam_path || IAM_DEFAULT_PATH).startsWith(path_prefix)) { + return false; + } return acc.name.unwrap().split(IAM_SPLIT_CHARACTERS)[1] === root_name; }); let members = _.map(requesting_account_iam_users, function(iam_user) { 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 + // Password last used is not tracked; omit to avoid misleading data + password_last_used: undefined, }; return member; });
111-113: Use persisted timestamps instead of placeholder dates.Lines 111-113 use
new Date()for bothcreate_dateandpassword_last_used, which returns "now" and is misleading. Use persisted timestamps or omit unknown fields.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: new Date(requested_account.last_update || requested_account._id.getTimestamp()), + // Password last used is not tracked; omit or return undefined + password_last_used: requested_account.password_last_used || undefined, };
🧹 Nitpick comments (2)
src/server/system_services/schemas/account_schema.js (1)
30-33: Consider adding validation and uniqueness constraints for IAM fields.The schema additions look good and address previous review concerns by using
objectidfor owner. However, consider these optional enhancements:
- Add a pattern validation for
iam_arnto enforce AWS ARN format (e.g.,^arn:[^:]*:iam:.*$)- Consider a unique index on
iam_arnat the model/database layer to prevent duplicatesThese additions would strengthen data integrity for IAM resources.
src/endpoint/endpoint.js (1)
74-95: LGTM: NBAccountSDK properly wires account and bucket spaces.The class correctly composes BucketSpaceNB and AccountSpaceNB into the AccountSDK. The comment on lines 75-76 about "simple flow was not implemented" is unclear—consider clarifying or removing it before merge.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util/account_util.js
- src/endpoint/iam/iam_constants.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/server/system_services/account_server.js (1)
src/sdk/accountspace_nb.js (1)
account_util(6-6)
src/sdk/accountspace_nb.js (4)
src/server/system_services/account_server.js (21)
_(5-5)require(13-13)require(21-21)require(22-22)require(23-23)account_util(25-25)account_util(41-41)system_store(17-17)params(315-315)params(678-678)params(942-942)req(119-119)req(383-383)account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)src/util/account_util.js (19)
_(4-4)require(8-8)require(14-14)require(17-17)require(18-18)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/endpoint.js (7)
require(39-39)require(40-40)require(42-42)require(43-43)require(45-45)system_store(36-36)AccountSpaceNB(49-49)src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-16)IAM_DEFAULT_PATH(47-47)IAM_SPLIT_CHARACTERS(61-61)
⏰ 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
🔇 Additional comments (5)
src/server/system_services/schemas/role_schema.js (1)
28-28: LGTM: IAM user role type added.The addition of the 'iam_user' role type appropriately extends the role enum to support IAM users alongside existing admin, user, and operator roles.
src/server/system_services/schemas/system_schema.js (1)
90-91: LGTM: IAM service and API enums updated.The additions properly register IAM as both a service type and API type in system addresses, enabling IAM endpoint configuration.
Also applies to: 102-102
src/endpoint/endpoint.js (1)
461-465: LGTM: Account SDK properly initialized per request.The initialization of
req.account_sdkfollows the same pattern asreq.sts_sdkandreq.object_sdk, making IAM account operations available to endpoint handlers.src/server/system_services/account_server.js (1)
39-41: LGTM: Account creation properly delegated to account_util.The refactoring cleanly centralizes account creation logic, maintaining the same interface while improving maintainability.
src/sdk/accountspace_nb.js (1)
224-253: LGTM: Access key methods appropriately stubbed.The access key methods are clearly marked as not implemented and throw appropriate errors. This is acceptable for an initial IAM implementation focusing on user management APIs.
| NEW_USERNAME: 'NewUserName', | ||
| }); | ||
|
|
||
| const IAM_SPLIT_CHARACTERS = ':'; |
There was a problem hiding this comment.
Would be better to add a comment about this variable use.
And if you move the functions related to the account name of IAM user, it can also move to there.
There was a problem hiding this comment.
we need to move the IAM specific method like create user (name:account_name), and get only user name methods also need to move to IAM util
naveenpaul1
force-pushed
the
iam_account
branch
from
ddbfc2a to
2bc8354
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
src/util/account_util.js (3)
462-470: Provide fallback whenrequested_accountis not supplied.Line 469 can return
undefinedwhenrequesting_account.iam_operate_on_root_accountis true butrequested_accountis not provided, leading to "undefined" appearing in ARNs.Apply this diff to add a fallback:
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?._id || requesting_account._id; }
507-511: Fix resource existence check (comparing object to 0 is always false).Line 509 compares
req.system.pools_by_nameto0, which will always be false for an object. This masks "no resources" conditions and can allow account creation to proceed incorrectly.Apply this diff to fix the check:
- if (req.system.pools_by_name === 0) { + if (!req.system.pools_by_name || Object.keys(req.system.pools_by_name).length === 0) { throw new RpcError('No resources in the system - Can\'t create accounts'); }
428-435: Useiam_pathinstead of undefinedpathproperty.Line 432 references
requesting_account.pathwhich is not a defined property on account objects. This should berequesting_account.iam_pathto correctly construct the ARN for error messages.Apply this diff:
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); + requesting_account.name.unwrap(), requesting_account.iam_path || IAM_DEFAULT_PATH);src/sdk/accountspace_nb.js (4)
183-214: Implement PathPrefix filtering in list_users.The
list_usersmethod ignores thePathPrefixparameter, which is a required filter for AWS IAM compatibility. Additionally, lines 205 and 207 return incorrect placeholder dates.Apply this diff to add PathPrefix filtering and fix dates:
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; 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; + const is_owned = acc.name.unwrap().split(IAM_SPLIT_CHARACTERS)[1] === root_name; + if (!is_owned) return false; + + // Apply PathPrefix filter if specified + if (path_prefix) { + const user_path = acc.iam_path || IAM_DEFAULT_PATH; + return user_path.startsWith(path_prefix); + } + return true; }); let members = _.map(requesting_account_iam_users, function(iam_user) { 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, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: GAP Miising password_last_used - password_last_used: Date.now(), // GAP + create_date: undefined, // Unknown until persisted + password_last_used: undefined, }; return member; }); - // CORE CHANGES PENDING - END members = members.sort((a, b) => a.username.localeCompare(b.username));
66-67: Remove hardcoded default_resource or make it configurable.Line 67 hardcodes
default_resource: 'noobaa-default-backing-store', which may not exist in all deployments and can cause account creation to fail. The TODO indicates this should be removed.Either remove the
default_resourcefield to letcreate_accountresolve the default, or make it configurable through params. Consider applying:iam_arn: iam_arn, iam_path: params.iam_path, role: 'iam_user', - - // TODO: default_resource remove - default_resource: 'noobaa-default-backing-store', },
120-127: Only check for duplicate username when actually renaming.Line 123 unconditionally checks if
params.new_usernamealready exists, but this should only run when a rename is actually requested (i.e., whenparams.new_usernameis defined and differs from the current username).Apply this diff:
const requested_account = system_store.get_account_by_email(username); let iam_path = requested_account.iam_path; - let user_name = requested_account.name.unwrap(); - account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account); + const current_username = account_util.get_iam_username(requested_account.name.unwrap()); + let user_name = current_username; + + // Only check for duplicates if renaming + if (params.new_username !== undefined && params.new_username !== current_username) { + account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account); + } + account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account);
105-109: Return persisted or undefined dates instead of "now".Lines 106 and 108 return
new Date()forcreate_dateandpassword_last_used, which incorrectly suggests these events just happened. This misleads callers about when the user was actually created.Apply this diff to use undefined or persisted values:
arn: requested_account.iam_arn, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: Dates missing : GAP - password_last_used: new Date(), + // Return undefined until these fields are persisted + create_date: undefined, + password_last_used: undefined, };Alternatively, if a
create_datefield is being persisted on the account object, userequested_account.create_dateinstead.
🧹 Nitpick comments (3)
src/util/account_util.js (1)
555-567: Consider export naming convention for internal helpers.Several functions prefixed with
_(e.g.,_check_if_requesting_account_is_root_account,_returned_username) are exported, which diverges from the typical convention where_indicates private/internal. This may reduce clarity about the public API surface.Consider either:
- Removing the
_prefix for functions that are part of the public API- Keeping these truly internal and only exporting the primary functions (create_account, delete_account, etc.)
- Documenting why these internal helpers need to be exported (e.g., for testing)
src/sdk/nb_account_sdk.js (1)
16-26: Consider using ES6 property shorthand for cleaner syntax.Lines 21-22 explicitly assign
rpc_client: rpc_clientandinternal_rpc_client: internal_rpc_client, which can be simplified using ES6 shorthand.Apply this diff:
super({ - rpc_client: rpc_client, - internal_rpc_client: internal_rpc_client, + rpc_client, + internal_rpc_client, bucketspace: bucketspace, accountspace: accountspace, });src/sdk/accountspace_nb.js (1)
10-10: Remove commented-out code.Lines 10, 161, and 168 contain commented-out code that should be either implemented or removed to improve code clarity.
Consider removing:
- Line 10:
// const { account_cache } = require('./object_sdk');- Line 161:
//const requesting_account = account_sdk.requesting_account;- Line 168:
//const root_account = system_store.get_account_by_email(requesting_account.email);If these are needed for future implementation, track them in a TODO or issue instead of leaving commented code.
Also applies to: 161-161, 168-168
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/endpoint/endpoint.js(3 hunks)src/endpoint/iam/iam_constants.js(2 hunks)src/sdk/accountspace_nb.js(1 hunks)src/sdk/nb_account_sdk.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)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/server/system_services/schemas/role_schema.js
- src/endpoint/iam/iam_constants.js
- src/endpoint/endpoint.js
- src/server/system_services/schemas/account_schema.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/sdk/nb_account_sdk.js (1)
src/endpoint/endpoint.js (7)
require(40-40)require(41-41)require(43-43)require(44-44)require(46-46)NBAccountSDK(27-27)internal_rpc_client(164-164)
src/sdk/accountspace_nb.js (4)
src/server/system_services/account_server.js (15)
_(5-5)require(13-13)require(21-21)require(22-22)require(23-23)SensitiveString(15-15)account_util(25-25)account_util(41-41)dbg(12-12)system_store(17-17)params(315-315)params(678-678)params(942-942)req(119-119)req(383-383)src/util/account_util.js (18)
_(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(318-318)IamError(358-358)IamError(387-387)IamError(424-424)IamError(448-448)req(536-536)username(353-353)username(383-384)username(417-418)src/sdk/nb_account_sdk.js (1)
AccountSpaceNB(6-6)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 (2)
src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
requesting_account_name(26-27)src/endpoint/iam/iam_constants.js (3)
IAM_SPLIT_CHARACTERS(61-61)IAM_DEFAULT_PATH(47-47)IAM_ACTIONS(5-16)
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-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (3)
src/server/system_services/schemas/system_schema.js (1)
79-103: LGTM! Schema updates support IAM service.The additions of 'iam' to both the
serviceandapienums are consistent with the existing schema structure and align with the PR's goal of introducing IAM support.src/server/system_services/account_server.js (1)
36-47: LGTM! Clean delegation to centralized account utilities.The refactoring to delegate account creation, key generation, and deletion to
account_utilreduces code duplication and centralizes account management logic. The validation flow is preserved and the API surface remains consistent.src/sdk/accountspace_nb.js (1)
15-21: Track outstanding TODOs for production readiness.Multiple TODO comments throughout the file indicate incomplete functionality, including:
- IAM account lifecycle questions (lines 15-21)
- Hardcoded default_resource (lines 66-67)
- Missing account cache invalidation (lines 76-77, 146-147, 179)
- Missing event dispatching (lines 77, 147)
- Missing IAM path persistence and dates (lines 101-102)
- Missing access key and policy cleanup (lines 170-171)
Consider creating tracking issues for these items to ensure they are addressed before the feature reaches production, especially:
- The hardcoded
default_resourcewhich can cause failures- The missing
create_datepersistence which breaks API contracts- The access key/policy cleanup which can leave orphaned data
Also applies to: 66-67, 76-77, 101-102, 146-147, 170-171
naveenpaul1
force-pushed
the
iam_account
branch
from
2bc8354 to
590d6b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/util/account_util.js (1)
507-511: Fix resource existence check - comparing object to number.Line 509 compares
req.system.pools_by_name(an object) to0(a number), which will always be false. This check is meant to catch the "no resources" condition but fails to do so.Apply this diff:
if (!req.rpc_params.new_system_parameters) { - if (req.system.pools_by_name === 0) { + if (!req.system.pools_by_name || Object.keys(req.system.pools_by_name).length === 0) { throw new RpcError('No resources in the system - Can\'t create accounts'); }src/sdk/accountspace_nb.js (4)
112-154: Fix username derivation to avoid "undefined:root_account" bug.Line 128 directly uses
params.new_usernamewhich can be undefined, creating invalid account names like "undefined:root_account_name". The code should use theuser_namevariable that was conditionally updated on line 126.Apply this diff:
if (params.new_iam_path !== undefined) iam_path = params.new_iam_path; if (params.new_username !== undefined) user_name = params.new_username; const iam_arn = iam_utils.create_arn_for_user(requested_account._id.toString(), user_name, iam_path); - const new_account_name = new SensitiveString(`${params.new_username}:${requesting_account.name.unwrap()}`); + const new_account_name = new SensitiveString(`${user_name}${IAM_SPLIT_CHARACTERS}${requesting_account.name.unwrap()}`);
197-209: Fix date inconsistencies and consider PathPrefix filtering.Line 206 uses
Date.now()which returns a number (milliseconds), while line 204 usesnew Date()which returns a Date object. Usenew Date()consistently for both, or better yet, use undefined for password_last_used as suggested in get_user. Additionally, the list_users implementation doesn't filter by params.path_prefix, though past reviews suggest this was addressed - verify if PathPrefix filtering is required.Apply this diff:
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, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: GAP Miising password_last_used - password_last_used: Date.now(), // GAP + // TODO: Add creation_date to schema + create_date: iam_user.creation_date || undefined, + password_last_used: undefined, // Not tracked; avoid false data };
66-67: Remove hardcoded default_resource to avoid failures.The hardcoded 'noobaa-default-backing-store' may not exist in all deployments, causing account creation to fail. As noted in past reviews, let account_util.create_account resolve the default resource automatically, or pass through an explicit resource only if provided by the caller.
Apply this diff:
iam_arn: iam_arn, iam_path: params.iam_path, role: 'iam_user', - - // TODO: default_resource remove - default_resource: 'noobaa-default-backing-store', },
104-107: Replace placeholder dates with actual persisted values or undefined.Returning
new Date()for create_date and password_last_used is misleading as it returns the current time, not when the account was created or password last used. As discussed in past reviews, either use persisted timestamps (e.g., requested_account.create_date if available) or return undefined for password_last_used to avoid returning false data.Apply this diff:
arn: requested_account.iam_arn, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: Dates missing : GAP - password_last_used: new Date(), + // TODO: Add creation_date to schema; until then use undefined + create_date: requested_account.creation_date || undefined, + password_last_used: undefined, // Not tracked; return undefined to avoid false data };
🧹 Nitpick comments (5)
src/endpoint/iam/iam_constants.js (1)
61-61: Add documentation comment for IAM_SPLIT_CHARACTERS.As noted in past reviews, a comment explaining this constant's purpose would improve clarity. The delimiter separates IAM usernames from root account names in composite account identifiers.
Apply this diff:
+// Delimiter used to construct composite account names: format is "username:root_account_name" const IAM_SPLIT_CHARACTERS = ':';src/server/system_services/schemas/account_schema.js (1)
15-35: Consider adding creation_date field for IAM API compliance.As discussed in past reviews, IAM's GetUser and ListUsers APIs require a create_date timestamp. While the team acknowledged the need to address upgrade scripts for existing accounts, adding this field to the schema would complete the IAM implementation. The current workaround uses
new Date()placeholders which return incorrect values.src/endpoint/endpoint.js (1)
437-441: Consider enabling stats collection for account SDK.The stats parameter is commented out, likely because EndpointStatsCollector doesn't yet track account operations. Consider adding account-level metrics (create_user, delete_user, etc.) to the stats collector for observability, similar to object operations.
src/util/account_util.js (2)
142-148: Store create_date in the database rather than generating at return time.Line 146 returns
new Date()for IAM account creation time, but this generates the timestamp at function return rather than using the actual database insertion time. Consider storing creation_date in the account record during insertion (line 120-126) and returning that persisted value, ensuring consistency with the actual creation time.
428-450: Clarify ARN path source in access denied errors.Line 432 uses
requesting_account.path || IAM_DEFAULT_PATH, but the account schema definesiam_path, notpath. Consider usingrequesting_account.iam_path || IAM_DEFAULT_PATHfor consistency with the schema.Apply this diff:
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); + requesting_account.name.unwrap(), requesting_account.iam_path || IAM_DEFAULT_PATH);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/endpoint/endpoint.js(3 hunks)src/endpoint/iam/iam_constants.js(2 hunks)src/sdk/accountspace_nb.js(1 hunks)src/sdk/nb_account_sdk.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)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/system_services/schemas/system_schema.js
- src/sdk/nb_account_sdk.js
- src/server/system_services/schemas/role_schema.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/sdk/accountspace_nb.js (3)
src/util/account_util.js (17)
_(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(318-318)IamError(358-358)IamError(387-387)IamError(424-424)IamError(448-448)req(536-536)username(353-353)username(383-384)username(417-418)src/sdk/nb_account_sdk.js (1)
AccountSpaceNB(6-6)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 (3)
IAM_SPLIT_CHARACTERS(61-61)IAM_DEFAULT_PATH(47-47)IAM_ACTIONS(5-16)
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). (1)
- GitHub Check: ceph-s3-tests / Ceph S3 Tests
🔇 Additional comments (6)
src/endpoint/endpoint.js (1)
27-27: LGTM!The NBAccountSDK import follows the established pattern for SDK integrations and is correctly placed alongside ObjectSDK and StsSDK imports.
src/server/system_services/account_server.js (3)
36-47: LGTM!The refactoring to delegate account creation to account_util is a solid improvement. It centralizes account management logic and reduces duplication, making the codebase more maintainable.
135-137: LGTM!Delegating key generation to account_util maintains consistency with the refactored account creation flow and centralizes credential management logic.
402-406: LGTM!The delegation pattern for delete_account is consistent with other refactored methods. Keeping the pre-flight checks (_verify_can_delete_account) in account_server while delegating the actual deletion maintains good separation of concerns.
src/util/account_util.js (2)
150-190: LGTM!The delete_account implementation properly handles role cleanup and includes comprehensive audit logging for both success and failure cases.
304-310: LGTM!The username helper functions (populate_username and get_iam_username) provide clear, reusable logic for handling composite IAM account names.
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
iam_account
branch
from
590d6b6 to
3d165ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/endpoint/endpoint.js (1)
72-80: Add account_sdk to EndpointRequest typedef.The
account_sdkproperty is initialized on the request at lines 436-440 but missing from the typedef. This breaks type checking and IDE autocomplete.Apply this diff:
/** * @typedef {import('http').IncomingMessage & { * object_sdk?: ObjectSDK; * sts_sdk?: StsSDK; + * account_sdk?: NBAccountSDK; * virtual_hosts?: readonly string[]; * bucket_logger?: PersistentLogger; * notification_logger?: PersistentLogger; * }} EndpointRequest */
♻️ Duplicate comments (5)
src/endpoint/endpoint.js (1)
48-48: Remove redundant newline.This empty line is unnecessary and should be removed for consistency.
src/util/account_util.js (1)
480-554: Fix resource existence check.Line 511 incorrectly compares
req.system.pools_by_name === 0. An object is never equal to 0, causing this check to always pass even when no resources exist.Apply this diff:
if (!req.rpc_params.new_system_parameters) { - if (req.system.pools_by_name === 0) { + if (!req.system.pools_by_name || Object.keys(req.system.pools_by_name).length === 0) { throw new RpcError('No resources in the system - Can\'t create accounts'); }src/sdk/accountspace_nb.js (3)
182-213: Replace placeholder dates in list_users.Lines 204 and 206 return placeholder timestamps. Use persisted values or omit them to avoid misleading data.
Apply this diff:
let members = _.map(requesting_account_iam_users, function(iam_user) { 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, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: GAP Miising password_last_used - password_last_used: Date.now(), // GAP + create_date: iam_user.create_date ? new Date(iam_user.create_date) : undefined, + password_last_used: undefined, // Not tracked yet }; return member; });
112-155: Critical: Fix username construction bug.Line 128 uses
params.new_usernamedirectly, which isundefinedwhen onlynew_iam_pathis updated. This creates invalid account names like"undefined:root". Use theuser_namevariable which is correctly updated at line 126.Apply this diff:
if (params.new_iam_path !== undefined) iam_path = params.new_iam_path; if (params.new_username !== undefined) user_name = params.new_username; const iam_arn = iam_utils.create_arn_for_user(requested_account._id.toString(), user_name, iam_path); - const new_account_name = new SensitiveString(`${params.new_username}:${requesting_account.name.unwrap()}`); + const new_account_name = new SensitiveString(`${user_name}${IAM_SPLIT_CHARACTERS}${requesting_account.name.unwrap()}`); const updates = { name: new_account_name, email: new_account_name,
88-110: Replace placeholder dates with persisted timestamps.Lines 105 and 107 return
new Date()which creates misleading "now" timestamps instead of actual account creation/last-use times. Use persisted fields or omit them.Apply this diff:
const reply = { user_id: requested_account._id.toString(), - // TODO : IAM PATH iam_path: requested_account.iam_path || IAM_DEFAULT_PATH, username: account_util.get_iam_username(requested_account.name.unwrap()), arn: requested_account.iam_arn, - // TODO: GAP Need to save created date - create_date: new Date(), - // TODO: Dates missing : GAP - password_last_used: new Date(), + create_date: requested_account.create_date ? new Date(requested_account.create_date) : undefined, + password_last_used: undefined, // Not tracked yet };
🧹 Nitpick comments (1)
src/sdk/accountspace_nb.js (1)
44-86: Consider removing hardcoded default_resource.Line 67 hardcodes
'noobaa-default-backing-store'which may not exist in all deployments. Letaccount_util.create_accountresolve the default resource instead (as it does for non-IAM accounts).Apply this diff:
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, role: 'iam_user', - - // TODO: default_resource remove - default_resource: 'noobaa-default-backing-store', },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/endpoint/endpoint.js(3 hunks)src/endpoint/iam/iam_constants.js(2 hunks)src/sdk/accountspace_nb.js(1 hunks)src/sdk/nb_account_sdk.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)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/iam/iam_constants.js
- src/server/system_services/schemas/role_schema.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/sdk/nb_account_sdk.js (1)
src/endpoint/endpoint.js (7)
require(40-40)require(41-41)require(43-43)require(44-44)require(46-46)NBAccountSDK(27-27)internal_rpc_client(163-163)
src/util/account_util.js (1)
src/endpoint/iam/iam_constants.js (3)
IAM_SPLIT_CHARACTERS(61-61)IAM_DEFAULT_PATH(47-47)IAM_ACTIONS(5-16)
src/server/system_services/account_server.js (1)
src/sdk/accountspace_nb.js (1)
account_util(6-6)
src/sdk/accountspace_nb.js (3)
src/server/system_services/account_server.js (14)
_(5-5)require(13-13)require(21-21)require(22-22)require(23-23)SensitiveString(15-15)account_util(25-25)account_util(41-41)system_store(17-17)params(315-315)params(678-678)params(942-942)req(119-119)req(383-383)src/util/account_util.js (17)
_(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(320-320)IamError(360-360)IamError(389-389)IamError(426-426)IamError(450-450)req(538-538)username(355-355)username(385-386)username(419-420)src/endpoint/iam/iam_constants.js (3)
IAM_ACTIONS(5-16)IAM_DEFAULT_PATH(47-47)IAM_SPLIT_CHARACTERS(61-61)
🔇 Additional comments (13)
src/endpoint/endpoint.js (2)
27-27: LGTM: NBAccountSDK import.The import follows the existing SDK import pattern and is correctly placed.
436-440: LGTM: Per-request account_sdk initialization.The NBAccountSDK is properly instantiated per request with the necessary RPC clients, following the same pattern as
object_sdkandsts_sdk.src/server/system_services/schemas/system_schema.js (1)
90-91: LGTM: Schema enums updated for IAM service.The additions of
'iam'to both theserviceandapienums are necessary to support IAM service registration and routing.Also applies to: 102-102
src/sdk/nb_account_sdk.js (1)
1-29: LGTM: Clean NBAccountSDK implementation.The class correctly extends AccountSDK and wires NB-specific BucketSpaceNB and AccountSpaceNB implementations. The constructor pattern matches other SDK classes in the codebase.
src/server/system_services/schemas/account_schema.js (1)
30-35: LGTM: IAM-related schema fields added.The new fields (
owner,tagging,iam_arn,iam_path) correctly support IAM account management. Theownerfield properly usesobjectidtype for referential integrity.Note: Past reviews flagged that
iam_arnmust be regenerated wheniam_pathor username changes. Ensure update operations maintain this invariant. Based on learningssrc/server/system_services/account_server.js (3)
25-25: LGTM: Clean delegation to account_util.The refactoring properly delegates account creation logic to
account_utilwhile maintaining the same public API. The validation and creation flow is now centralized.Also applies to: 39-46
136-136: LGTM: Key generation delegated.Proper delegation of key generation to
account_util.generate_account_keys.
405-405: LGTM: Account deletion delegated.Deletion now properly delegates to
account_util.delete_account, addressing past review concerns about semantics.src/sdk/accountspace_nb.js (2)
157-180: LGTM: Delete user implementation.The method properly validates ownership, permissions, and delegates deletion to
account_util.delete_account.
219-248: LGTM: Access key method stubs.The placeholder methods correctly throw
NotImplementederrors and log activity for future implementation tracking.src/util/account_util.js (3)
29-148: LGTM: Comprehensive account creation logic.The
create_accountfunction properly handles IAM and non-IAM accounts, master key creation, role assignment, resource defaults, and audit logging. Lines 114-118 correctly persistowner,iam_arn, andiam_pathfor IAM users.
150-190: LGTM: Account deletion with role cleanup.The function correctly removes both the account and associated roles, with proper audit logging for success and failure cases.
197-242: LGTM: Key generation with IAM support.Properly generates and encrypts access keys, with conditional return for IAM mode and comprehensive audit logging.
3 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
Refactor