IAM | Use the account server flow #9289
Conversation
WalkthroughAdds extensive IAM user/access-key/tag/policy APIs, extends create_account with owner/iam_path and returns id/arn, moves SDK IAM logic to RPC delegation, broadens IAM error mappings, and exposes new account utility helpers for IAM workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as accountspace_nb
participant RPC as RPC Client
participant Server as account_server
participant Util as account_util
rect rgb(210,230,250)
Note over SDK,Server: SDK delegates IAM ops via RPC (new flow)
end
Client->>SDK: get_user(username)
SDK->>RPC: account.get_user(context, { username })
RPC->>Server: get_user(req)
Server->>Util: validate_and_return_requested_account(params, action, requesting_account)
Util-->>Server: validated_account
Server-->>RPC: user_info
RPC-->>SDK: user_info
SDK-->>Client: user_info
sequenceDiagram
participant Client
participant SDK as accountspace_nb
participant RPC as RPC Client
participant Server as account_server
rect rgb(220,245,220)
Note over Client,Server: create_account with owner → IAM user created and ARN returned
end
Client->>SDK: create_account({ owner, iam_path, ... })
SDK->>RPC: account.create_account(context, params)
RPC->>Server: create_account(req)
Server->>Server: derive username, validate perms, create IAM user, generate keys
Server-->>RPC: { id, arn, token, access_keys }
RPC-->>SDK: { id, arn, token, access_keys }
SDK-->>Client: { id, arn, token, access_keys }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus on:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
naveenpaul1
force-pushed
the
iam_account_flow
branch
3 times, most recently
from
20fdd0b to
b355dee
Compare
naveenpaul1
force-pushed
the
iam_account_flow
branch
from
b355dee to
3456f89
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
src/util/account_util.js (2)
107-117: IAM account creation and access-key branching look consistent, but rely on owner type conversion
- Using
req.rpc_params.owneras theis_iamflag increate_access_key_authand ingenerate_account_keyscleanly separates IAM flows (append keys, no initial key on create) from legacy CLI flows (single key, replace).- Setting
account.owner = req.rpc_params.ownerand later readingiam_user.owner._id(e.g., inreturn_list_memberand_get_account_owner_id_for_arn) assumes system_store converts the supplied value into an objectid reference on insert.Please confirm that
system_store.make_changes(for accounts fromaccount_schema.js) normalizesownerstrings into objectid refs. If not, consider parsing/normalizing here, e.g.:- if (req.rpc_params.owner) { - account.owner = req.rpc_params.owner; - account.iam_path = req.rpc_params.iam_path; - } + if (req.rpc_params.owner) { + account.owner = system_store.parse_system_store_id + ? system_store.parse_system_store_id(req.rpc_params.owner) + : req.rpc_params.owner; + account.iam_path = req.rpc_params.iam_path; + }This keeps downstream assumptions (
owner._id) safe.Also applies to: 144-146, 231-237, 254-256
325-332: New IAM-oriented helpers and error mappings look correct and consistentThe refactoring to:
- Map existence/ownership/limit errors via
IamError.*intoRpcError(code, message, http_code), and- Centralize account resolution/authorization in
validate_and_return_requested_accountplusreturn_list_memberis consistent with the new
RPC_ERRORS_TO_IAMmap and should produce stable IAM error codes and messages over RPC.The logic in these helpers matches their usages in
account_server.js(GET_USER, access key APIs, tagging, policies), and the new exports (validate_and_return_requested_account,get_iam_username,return_list_member) form a clean surface.If you find
_returned_usernamepatterns confusing, you might document or renameon_itselfinvalidate_and_return_requested_accountcall sites to make the “root-on-self vs root-on-IAM-user” semantics clearer, but it’s not blocking.Also applies to: 363-371, 397-399, 422-424, 435-437, 442-444, 449-451, 474-476, 482-484, 493-495, 596-602, 715-731, 733-744, 769-771
src/api/account_api.js (2)
44-50: create_account schema changes line up with server behavior
- Adding
ownerandiam_pathparams andid/arnfields in the reply matches the new IAM-awareaccount_server.create_accountflow (root vs IAM user creation).owneras a string andiam_pathas a string are consistent with how the server currently consumes them.Consider documenting in the API docstring that
ownermust be the root account ID and that IAM users created through this path will not get access keys automatically (CreateAccessKey must be used), to avoid misuse from callers outside the IAM REST flow.Also applies to: 93-111, 1243-1245
888-978: Tag schema should match server-side expectationsServer-side
tag_user/untag_user/list_user_tagscode inaccount_server.jsworks withtag.key/tag.valueonrequested_account.tagging, andlist_user_tagsmaps those to{ member: { Key, Value } }in replies.Here:
tag_user/untag_userusecommon_api#/definitions/tagfor input.list_user_tagsreplies reference#/definitions/tag, which you’ve defined withKey/Value.Please double-check that
common_api#/definitions/taguseskey/value(lowercase) to match the server-side implementation. If not, either:
- Normalize case server-side (accept
Key/Valueand map tokey/value), or- Point
tag_user/untag_userat the new#/definitions/tagand adjust server code accordingly.Aligning these avoids subtle JSON shape mismatches between client and server.
Also applies to: 1249-1331
src/endpoint/iam/iam_rest.js (1)
16-33: RPC-to-IAM error mapping is consistent; consider how much oferr.messageyou want to expose
- The expanded
RPC_ERRORS_TO_IAMtable correctly aligns common RPC codes (e.g.,NoSuchEntity,LimitExceeded,DeleteConflict,AccessDeniedException) with their correspondingIamError.*, matching the new RpcError usage inaccount_util/account_server.- For non-IamError failures,
handle_errornow does:const iam_err = ((err instanceof IamError) && err) || new IamError(RPC_ERRORS_TO_IAM[err.rpc_code] || IamError.InternalFailure); iam_err.message = err.message;so the client always sees the underlying RPC/error message.
If there’s any risk of internal details leaking via
err.message, you might want to only overrideiam_err.messagefor known, curatedrpc_codes (e.g., those present inRPC_ERRORS_TO_IAM) and fall back to the default IAM message for unexpected/internal failures.Also applies to: 232-237
src/sdk/accountspace_nb.js (2)
37-63: create_user assumes requesting account always has a default_resourceThe IAM
create_userflow builds:default_resource: requesting_account.default_resource.name,and sends it to
account.create_account. If the requesting root account ever lacksdefault_resource(no S3 access configured), this will throw at.name.If such accounts are possible, consider either:
- Validating this precondition and returning a clear IAM error (e.g., “root account must have S3 access to create IAM users”), or
- Falling back to a system default pool/namespace (similar to the root account create flow) when
default_resourceis missing.If by design all IAM-capable roots always have
default_resource, a short comment here stating that invariant would help future readers.
65-171: RPC delegation pattern for IAM operations looks cleanAll IAM operations (
get_user,update_user,delete_user,list_users, access key, tag, and policy methods) now:
- Resolve
requesting_accountfromsystem_storeby email, and- Delegate directly to
rpc_client.account.<op>(params, requesting_account).This keeps
AccountSpaceNBthin and centralizes logic inaccount_server.If IAM traffic grows, consider caching
requesting_accountper-request inaccount_sdkto avoid repeatedget_account_by_emaillookups, but it’s not critical.src/server/system_services/account_server.js (3)
1213-1253: update_user: avoid username-existence check when not renaming
update_userunconditionally computes and checksnew_username:const new_username = account_util.get_account_name_from_username(req.rpc_params.new_username, requesting_account._id.toString()); account_util._check_username_already_exists(action, new_username, req.rpc_params.new_username);even when
req.rpc_params.new_usernameis not provided and onlynew_iam_pathis being changed. This results in a pointless lookup for something like"undefined:<id>"and confusing log context, though it won’t currently throw.Guard this block so it only runs when a new username is actually supplied:
- const new_username = account_util.get_account_name_from_username(req.rpc_params.new_username, requesting_account._id.toString()); - account_util._check_username_already_exists(action, new_username, req.rpc_params.new_username); + if (req.rpc_params.new_username) { + const new_username = account_util.get_account_name_from_username( + req.rpc_params.new_username, + requesting_account._id.toString() + ); + account_util._check_username_already_exists(action, new_username, req.rpc_params.new_username); + }This keeps logs and checks aligned with the actual intent of the call.
1301-1331: Prefer RpcError over IamError in RPC handlers to align with iam_rest mappingIn a few places, server-side IAM handlers throw
IamErrordirectly, for example:
create_access_keycatch block:const { code, http_code, type } = IamError.InternalFailure; throw new IamError({ code, message: message_with_details, http_code, type });
tag_userwhen exceedingMAX_TAGS:const { code, http_code, type } = IamError.LimitExceeded; throw new IamError({ code, message: message_with_details, http_code, type });Everywhere else in this module and in
account_util, IAM conditions are surfaced asRpcError(code, message, http_code)and then translated to IAM errors iniam_rest.jsusingRPC_ERRORS_TO_IAM[err.rpc_code].For consistency and clearer RPC error handling, consider throwing
RpcErrorhere as well:- const { code, http_code, type } = IamError.InternalFailure; - throw new IamError({ code, message: message_with_details, http_code, type }); + const { code, http_code } = IamError.InternalFailure; + throw new RpcError(code, message_with_details, http_code);and similarly for the
LimitExceededcase. This keeps the error flow “RPC → iam_rest mapping → IAM XML” uniform.Also applies to: 1514-1587
1333-1410: Access key IAM helpers match the new utility functionsThe IAM access key handlers:
- Use
validate_and_return_requested_accountfor scoping/authorization,- Enforce the per-user key limit via
_check_number_of_access_key_array,- Rely on
generate_account_keysandget_non_updating_access_keyfor encryption-safe updates,- Sort list results by access_key, and
- Correctly toggle
deactivatedusing_check_access_key_is_deactivatedand_get_access_key_status.If you later implement pagination, you can reuse the existing sort logic and simply slice
membersaccording tomax_itemsandmarker; the currentis_truncated = falseplaceholders are fine for now.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/account_api.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/sdk/accountspace_nb.js(2 hunks)src/server/system_services/account_server.js(5 hunks)src/util/account_util.js(14 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-19T15:03:42.231Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.231Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/sdk/accountspace_nb.jssrc/server/system_services/account_server.jssrc/util/account_util.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.jssrc/server/system_services/account_server.jssrc/util/account_util.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.jssrc/server/system_services/account_server.jssrc/util/account_util.jssrc/api/account_api.js
📚 Learning: 2025-06-17T12:59:51.543Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (3)
src/endpoint/iam/iam_rest.js (2)
src/endpoint/iam/iam_utils.js (15)
IamError(82-82)IamError(255-255)IamError(481-481)IamError(510-510)IamError(538-538)IamError(545-545)IamError(573-573)IamError(595-595)IamError(624-624)IamError(659-659)IamError(676-676)IamError(691-691)IamError(698-698)IamError(711-711)IamError(715-715)src/sdk/accountspace_fs.js (1)
IamError(14-14)
src/sdk/accountspace_nb.js (2)
src/server/system_services/account_server.js (19)
requesting_account(1196-1196)requesting_account(1216-1216)requesting_account(1258-1258)requesting_account(1275-1275)requesting_account(1303-1303)requesting_account(1335-1335)requesting_account(1348-1348)requesting_account(1376-1376)requesting_account(1393-1393)requesting_account(1414-1414)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)req(136-136)req(399-399)system_store(17-17)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
src/util/account_util.js (2)
src/server/system_services/account_server.js (73)
req(136-136)req(399-399)account(71-71)account(119-119)account(137-139)account(161-161)account(209-209)account(332-332)account(695-695)account(959-959)message_with_details(1319-1319)message_with_details(1436-1436)action(41-41)action(1195-1195)action(1215-1215)action(1257-1257)action(1274-1274)action(1302-1302)action(1334-1334)action(1346-1346)action(1375-1375)action(1391-1391)action(1413-1413)action(1456-1456)action(1485-1485)action(1515-1515)action(1543-1543)action(1557-1557)username(1198-1198)username(1259-1259)username(1381-1381)username(1415-1415)username(1458-1458)username(1487-1487)system_store(17-17)dbg(12-12)params(331-331)params(694-694)params(958-958)requesting_account(1196-1196)requesting_account(1216-1216)requesting_account(1258-1258)requesting_account(1275-1275)requesting_account(1303-1303)requesting_account(1335-1335)requesting_account(1348-1348)requesting_account(1376-1376)requesting_account(1393-1393)requesting_account(1414-1414)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)requested_account(1197-1197)requested_account(1221-1221)requested_account(1262-1262)requested_account(1304-1304)requested_account(1336-1336)requested_account(1349-1349)requested_account(1395-1395)requested_account(1420-1420)requested_account(1463-1463)requested_account(1492-1492)requested_account(1518-1518)requested_account(1546-1546)requested_account(1560-1560)requested_account(1579-1579)iam_path(1222-1222)iam_path(1285-1285)iam_username(1284-1284)src/endpoint/s3/s3_bucket_policy_utils.js (3)
account(293-293)RpcError(7-7)dbg(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (4)
src/api/account_api.js (1)
1266-1316: user_info and user_accesskey_info definitions align with server contractsThe shapes of
user_infoanduser_accesskey_info(user_id, username, arn, iam_path, create_date, password_last_used, access_keys/status/access_key/secret_key) match whataccount_server.get_user,list_users,create_access_key, andlist_access_keysreturn.src/server/system_services/account_server.js (3)
40-56: create_account IAM branch is reasonable; relies on owner normalization
For IAM-style account creation (
rpc_params.ownerpresent), the flow correctly:
- Requires a root requester via
_check_if_requesting_account_is_root_account,- Ensures uniqueness via
_check_username_already_exists, and- Builds
iam_arnwithcreate_arn_for_user(req.account._id.toString(), user_name, iam_path || IAM_DEFAULT_PATH).For root account creation (no owner), it falls back to the existing permission/param validation and returns
{ id, arn: create_arn_for_root(id), token, access_keys }.As with
account_util.create_account, this assumes thataccount.owneris stored as an objectid ref (soowner._idworks everywhere). Please confirm that the value coming fromrpc_params.owneris normalized by system_store to avoid surprises in helpers like_check_root_account_owns_userandreturn_list_member.Also applies to: 998-1021
135-144: Guard on access_keys length in read_account_by_access_key is a good fixThe additional
acc.access_keys && acc.access_keys.length > 0guard before dereferencingacc.access_keys[0]prevents crashes for accounts without keys while preserving previous behavior.
1412-1512: Tagging logic looks sound; ensure it matches the tag schemaThe IAM user tagging helpers:
- Merge existing
requested_account.tagging(array of{ key, value }),- Enforce
MAX_TAGSafter merging,- Support deleting by
tag_keys, and- Return a
list_user_tagspayload shaped as{ tags: [{ member: { Key, Value } }], is_truncated: false }.This is coherent with the IAM design.
Given the earlier schema note in
account_api.js, please just confirm thatreq.rpc_params.tagsindeed arrives as{ key, value }(lowercase) to match this implementation, or adjust input normalization if it comes asKey/Value.
| async function get_user(req) { | ||
|
|
||
| const action = IAM_ACTIONS.GET_USER; | ||
| const requesting_account = req.account; | ||
| const requested_account = account_util.validate_and_return_requested_account(req.rpc_params, action, requesting_account); | ||
| const username = account_util.get_iam_username(req.rpc_params.username || requested_account.name.unwrap()); | ||
| const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), username, | ||
| requested_account.iam_path || IAM_DEFAULT_PATH); | ||
| return { | ||
| user_id: requested_account._id.toString(), | ||
| iam_path: requested_account.iam_path || IAM_DEFAULT_PATH, | ||
| username: username, | ||
| arn: iam_arn, | ||
| // TODO: GAP Need to save created date | ||
| create_date: Date.now(), | ||
| // TODO: Dates missing : GAP | ||
| password_last_used: Date.now(), | ||
| }; |
There was a problem hiding this comment.
get_user builds ARNs based on caller ID, not the user’s owning account
get_user currently does:
const requesting_account = req.account;
const requested_account = account_util.validate_and_return_requested_account(...);
const username = account_util.get_iam_username(req.rpc_params.username || requested_account.name.unwrap());
const iam_arn = iam_utils.create_arn_for_user(
requesting_account._id.toString(),
username,
requested_account.iam_path || IAM_DEFAULT_PATH
);If an IAM user calls GetUser on itself (no username param), requesting_account === requested_account, so the ARN’s account ID comes from the IAM user’s _id, not from its root/owner. When a root account calls GetUser for the same IAM user, the ARN uses the root _id. This makes the ARN unstable and caller-dependent.
To keep ARNs consistent and rooted at the owning account, derive the account ID from the requested account’s owner when present, or from its own _id when it’s a root account. For example:
- const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), username,
- requested_account.iam_path || IAM_DEFAULT_PATH);
+ let owner_id_for_arn;
+ if (requested_account.owner !== undefined) {
+ owner_id_for_arn = typeof requested_account.owner === 'object'
+ ? requested_account.owner._id.toString()
+ : requested_account.owner.toString();
+ } else {
+ owner_id_for_arn = requested_account._id.toString();
+ }
+ const iam_arn = iam_utils.create_arn_for_user(
+ owner_id_for_arn,
+ username,
+ requested_account.iam_path || IAM_DEFAULT_PATH
+ );This way, the same user always has the same ARN regardless of who calls GetUser.
🤖 Prompt for AI Agents
In src/server/system_services/account_server.js around lines 1193 to 1210,
get_user currently builds the ARN using the requesting account's _id which makes
ARNs caller-dependent; change it to derive the accountId for the ARN from the
requested account's owning account when present (e.g. requested_account.owner or
requested_account.owner_id) or fall back to requested_account._id for root
accounts, then pass that owner-derived id (toString()) into
iam_utils.create_arn_for_user along with the existing username and iam_path so
the ARN is stable and always rooted at the user’s owning account.
There was a problem hiding this comment.
The caller should be the owner; otherwise, the validation will fail.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the validate_and_return_requested_account function implementation
echo "=== Searching for validate_and_return_requested_account implementation ==="
ast-grep --pattern 'function validate_and_return_requested_account($$$) { $$$ }'
echo -e "\n=== Also search with const/exports pattern ==="
rg -A 30 'validate_and_return_requested_account.*=.*function' src/util/account_util.js
Length of output: 1699
🏁 Script executed:
#!/bin/bash
# Check what GET_USER action validation does - look for permission checks
echo "=== Checking IAM_ACTIONS.GET_USER validation logic ==="
rg -B 5 -A 15 'IAM_ACTIONS\.GET_USER' src/
echo -e "\n=== Check if IAM users can call GetUser on themselves ==="
rg -B 3 -A 10 '_check_if_requesting_account_is_root_account' src/util/account_util.js
Length of output: 28742
🏁 Script executed:
#!/bin/bash
# Look at how requested_account is determined when username is not provided
echo "=== Check how requested_account is resolved in get_user ==="
rg -B 2 -A 5 'req\.rpc_params\.username \|\|' src/server/system_services/account_server.js
echo -e "\n=== Check the account_util validation function signature and exports ==="
rg 'exports\.validate_and_return_requested_account' src/util/account_util.js
Length of output: 808
@naveenpaul1, Thank you for the clarification! However, looking at the validate_and_return_requested_account implementation, I found that IAM users are actually allowed to call GetUser on themselves without providing a username parameter:
// From src/util/account_util.js lines 715-731
function validate_and_return_requested_account(params, action, requesting_account) {
const on_itself = !params.username;
let requested_account;
if (on_itself) {
// When accesskey API called without specific username, action on the same requesting account.
// So in that case requesting account and requested account is same.
requested_account = requesting_account; // ← IAM user can act on itself!
} else {
_check_if_requesting_account_is_root_account(action, requesting_account, { username: params.username });
// ... validation for cross-account access
}
return requested_account;
}This creates an ARN inconsistency issue:
Scenario 1: IAM user alice (owned by root account root123) calls GetUser without username:
requesting_account= alice (IAM user with_id:alice456)requested_account= alice (same object, line 721)- Current ARN:
arn:aws:iam::alice456:user/alice❌ (uses IAM user's own ID)
Scenario 2: Root account root123 calls GetUser with username=alice:
requesting_account= root123 (root account with_id:root123)requested_account= alice (IAM user owned by root123)- Current ARN:
arn:aws:iam::root123:user/alice✅ (uses owner's ID)
The same user gets different ARNs depending on who calls the API!
Fix: Use the owner's ID (when present) instead of the requesting account's ID:
const owner_id = requested_account.owner
? (typeof requested_account.owner === 'object'
? requested_account.owner._id.toString()
: requested_account.owner.toString())
: requested_account._id.toString();
const iam_arn = iam_utils.create_arn_for_user(owner_id, username, requested_account.iam_path || IAM_DEFAULT_PATH);This ensures the ARN is stable and always rooted at the owning root account. Would you like me to provide a more detailed code change suggestion?
🧬 Code Graph Analysis Results
[src/server/system_services/account_server.js] (relevant snippets summarized)
- is_support_or_admin_or_me(system, account, target_account)
- Purpose: Determine whether the caller has permission to act on the target account based on role ownership or admin/operator privileges.
- Parameters:
- system: the current system object
- account: the caller's account object
- target_account: the account being targeted
- Returns: boolean
- Logic:
- Returns true if caller is a support account, or
- target_account exists and matches the caller (ownership), or
- caller has admin or operator role for the current system
- Notes: Used to gate updates/deletions on accounts. If false, operations should be rejected as unauthorized.
- _verify_can_delete_account(req, account_to_delete)
- Purpose: Validate that a given account can be deleted in the context of the current request.
- Parameters:
- req: the RPC request context (includes system, account, etc.)
- account_to_delete: the account object to be deleted
- Returns: void (throws RpcError on failure)
- Validation steps (high level):
- Ensure account_to_delete exists; otherwise raise NO_SUCH_ACCOUNT
- Disallow deletion of the special “support” account
- Disallow deletion of the system owner account
- Ensure the requester is allowed to delete the target (uses is_support_or_admin_or_me)
- Ensure the account does not own any buckets (cannot delete an account that owns buckets)
- Errors/Exceptions: Throws RpcError with appropriate codes/messages (e.g., UNAUTHORIZED, FORBIDDEN, NO_SUCH_ACCOUNT) when validations fail.
- read_account(req)
- Purpose: Retrieve account information with access-aware visibility.
- Parameters:
- req: the RPC request context
- Behavior:
- Determines target email (from req.rpc_params.email or req.account.email)
- Looks up the account by email; if not found, throws NO_SUCH_ACCOUNT
- Computes is_self (caller is the same account) and is_operator (caller has operator role for the system)
- Returns account info via get_account_info(account, is_self || is_operator)
- Access semantics:
- If caller is the owner (or an operator) certain sensitive fields may be included
- Owner/operator status influences what get_account_info reveals
- read_account_by_access_key(req) (contextual relevance)
- Purpose: Retrieve account info by matching an access key (for authentication-derived lookups).
- Parameters:
- req: the RPC request context
- Returns: account info via get_account_info(account)
- Notes: Used for credential-based lookups; access path should be restricted to authorized callers.
- verify_authorized_account(req)
- Purpose: Quick authorization check for certain operations (used by operators and similar flows).
- Parameters:
- req: the RPC request context
- Returns: boolean (true for operator paths, otherwise false)
- Behavior: Allows operator calls to proceed without a password, otherwise requires standard authorization.
These snippets collectively establish owner-based validation and role-based access checks that relate directly to the stated user comment: “The caller should be the owner; otherwise, the validation will fail.”
🧠 Learnings used
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.231Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
3456f89 to
ffe8131
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/endpoint/iam/iam_rest.js (1)
232-237: Overwriting all IAM error messages with rawerr.messagemay leak internal details
handle_errornow forcesiam_err.message = err.messagefor every error, includingInternalFailure. This can expose internal exception messages and stack-derived text to external IAM clients, which is generally undesirable for security and UX (clients see low‑level errors instead of stable, spec‑aligned messages).Consider limiting message override to:
- known, user‑facing validation / IAM errors (e.g., only when
err instanceof IamErroror whenRPC_ERRORS_TO_IAM[err.rpc_code]is truthy), and- leaving the default canned message for
InternalFailure/ unexpected errors.Alternatively, add a guard to avoid overriding when
erris not a controlled RpcError/IamError instance.src/util/account_util.js (2)
31-43: Persistaccount.owneras a system_store ID, not a bare stringIn
create_accountyou now:const account = { _id: ..., name: ..., email: ..., ... }; ... const account_access_info = create_access_key_auth(req, account, req.rpc_params.owner); ... if (req.rpc_params.owner) { account.owner = req.rpc_params.owner; account.iam_path = req.rpc_params.iam_path; }Given
req.rpc_params.ownercomes from the RPC layer as a string (e.g.,requesting_account._id.toString()), this will storeowneras a string on the account document. Elsewhere, the code assumes an ObjectId-like reference:
_check_root_account_owns_usercomparesrequested_account.owner._id._get_account_owner_id_for_arnusesrequesting_account.owner._id.get_account_infoandreturn_list_memberdoaccount.owner._id.toString().With
owneras a plain string,account.owner?._id.toString()inlist_userswill throw, and ARN helpers will seeundefinedIDs.Please convert the incoming string to a proper system_store ID here, e.g.:
- if (req.rpc_params.owner) { - account.owner = req.rpc_params.owner; - account.iam_path = req.rpc_params.iam_path; - } + if (req.rpc_params.owner) { + // owner is a string account id over RPC; normalize to a system_store ID reference + account.owner = system_store.parse_system_store_id(req.rpc_params.owner); + account.iam_path = req.rpc_params.iam_path; + }This keeps wire types simple while preserving the in‑memory invariants expected by account_server and other utilities. Based on learnings
Also applies to: 107-117
335-351: Fix_check_root_account_owns_userwhenuser_account.owneris not an objectThe else-branch currently does:
if (typeof user_account.owner === 'object') { owner_account_id = String(user_account.owner._id); } else { owner_account_id = user_account.owner._id; }If
user_account.owneris a primitive (e.g., a string ID),user_account.owner._idisundefined, soowner_account_idbecomesundefined. That breaks ownership checks and, combined with the new IAM flows, can cause_check_if_requested_is_owned_by_root_accountto incorrectly fail withNoSuchEntity.Safer logic:
if (typeof user_account.owner === 'object') { owner_account_id = String(user_account.owner._id); -} else { - owner_account_id = user_account.owner._id; -} +} else { + owner_account_id = String(user_account.owner); +}That supports both ObjectId references and plain string IDs and avoids silently returning
undefined.src/server/system_services/account_server.js (1)
40-64: Create-account IAM branch: overall flow is good, but relies onaccount.ownertype being fixed upstreamThe IAM-aware part of
create_account:
- Validates the caller is a root account (
_check_if_requesting_account_is_root_account).- Prevents duplicate IAM usernames via
_check_username_already_exists.- Builds an IAM user ARN with
create_arn_for_user(req.account._id.toString(), ...).- Calls
account_util.create_accountand returns{ id, arn, token, access_keys }, defaulting to root ARN when noowneris specified.That high-level flow is correct. The main caveat is that downstream IAM helpers assume
created_account.owneris an ObjectId-like reference; as noted inaccount_util.create_account, you currently assignownerfrom a string RPC param. Once you normalizeaccount.ownerthere (e.g., viasystem_store.parse_system_store_id), the rest of this logic — including ARN creation and later use inlist_users— will behave as intended.Also applies to: 107-117, 141-146
♻️ Duplicate comments (1)
src/server/system_services/account_server.js (1)
1193-1210:get_userARN still depends on caller account ID (duplicate of earlier concern)
get_userbuilds the ARN with:const iam_arn = iam_utils.create_arn_for_user( requesting_account._id.toString(), username, requested_account.iam_path || IAM_DEFAULT_PATH );Combined with
validate_and_return_requested_account, this means:
- If a root account calls
GetUserfor an IAM user, the ARN uses the root account’s_id(good).- If an IAM user calls
GetUseron itself (nousernameparam),requesting_account === requested_account, so the ARN uses the IAM user’s_id(not the owner’s). At the same time,list_usersbuilds ARNs usingiam_user.owner._id.toString(), so the same user can have two different ARNs depending on who calls.This mismatch makes ARNs caller-dependent and inconsistent with how
list_usersandreturn_list_membertreat the owning account.Even if you expect “the caller to always be the owner” for cross-user calls, the self-call case still produces an IAM-user-ID-based ARN. To stabilize ARNs, consider deriving the account ID from the user’s owning account (or root, when owner is undefined), e.g.:
- const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), username, - requested_account.iam_path || IAM_DEFAULT_PATH); + const owner_id_for_arn = requested_account.owner + ? requested_account.owner._id.toString() + : requested_account._id.toString(); + const iam_arn = iam_utils.create_arn_for_user( + owner_id_for_arn, + username, + requested_account.iam_path || IAM_DEFAULT_PATH + );(or by reusing
_get_account_owner_id_for_arn).This aligns
get_userwithlist_usersand makes ARNs stable per user. Based on learnings
🧹 Nitpick comments (5)
src/api/account_api.js (2)
93-111:create_accountreply shape: server returns extra fields beyond schemaThe reply schema lists
id,arn,token, andaccess_keys, butaccount_server.create_accountalso returnscreate_date(for IAM-created users). That’s fine if clients tolerate additional properties, but if you want full schema alignment you may want to:
- either add
create_dateto the reply schema (withidate: true), or- drop it from the server response until you persist a real creation date.
Right now it’s a transient value (
Date.now()) and not stored anywhere, so clients should not treat it as authoritative.
1285-1299: Consider reusingcommon_api#/definitions/taginlist_user_tagsresponse to eliminate duplicationVerification confirms the concern:
tag_userusescommon_api#/definitions/tag(lowercasekey/value), butlist_user_tagsresponse at line 941 uses a locally-defined#/definitions/tag(uppercaseKey/Value). This creates schema drift between request and response formats for the same entity.Recommend either:
- Reuse the common definition by changing line 941 to
$ref: 'common_api#/definitions/tag', or- If the uppercase casing is intentional for IAM wire format compatibility, rename the local definition to
iam_tagand update line 941 for clarity.src/util/account_util.js (2)
230-257:generate_account_keysIAM branch behavior is correct but slightly bypasses legacy permission loggingThe new IAM-aware branch:
if (req.rpc_params.owner) { account_access_keys.push(access_key_obj); } else { account_access_keys[0] = access_key_obj; } ... if (req.rpc_params.owner) { return decrypted_access_keys; }correctly:
- accumulates multiple keys for IAM users, and
- leaves root/CLI flows with a single rotated key.
Note that when called from
account_server.create_access_key, you pass a minimalaccount_reqwithoutsystem;generate_account_keystherefore skips theis_support_or_admin_or_mecheck and logssystemassystem_store.data.systems[0]. Given IAM access-key RPCs are already guarded byauth: { system: 'admin' }andvalidate_and_return_requested_account, this is acceptable, but if you want consistent audit logs you may want to threadreq.systemthrough that call as well.
733-744:return_list_memberuses placeholder dates; fine for now but should be revisited
return_list_membercurrently sets:create_date: Date.now(), // TODO: GAP Need to save created date password_last_used: Date.now(), // GAPfor every list entry, which doesn’t reflect the actual user creation or last-used times. Since this is explicitly marked as GAP and only affects metadata (not authorization), it’s acceptable for an initial cut, but it would be good to:
- start persisting
creation_dateon IAM user accounts, and- either track or omit
password_last_useduntil you can compute it correctly.For now, consider omitting
password_last_usedentirely to avoid clients misinterpreting the placeholder.src/server/system_services/account_server.js (1)
1301-1331: IAMcreate_access_keyflow matches IAM semantics and uses new IAM-aware key generationThis method:
- Validates the target account via
validate_and_return_requested_account.- Enforces the per-user key quota with
_check_number_of_access_key_array.- Calls
account_util.generate_account_keyswith{ email: requested_account.email, is_iam: true, owner: requesting_account._id.toString() }, which:
- appends a new key to
access_keysinstead of replacing, and- returns decrypted keys only in the IAM branch.
- Shapes the reply into
{ username, access_key, create_date, status: Active, secret_key }.That aligns well with IAM behavior (multiple access keys per user, explicit create API). The only caveat is the same owner-type issue discussed earlier: passing
owneras a string is fine as long as it’s converted back to an ID ingenerate_account_keys/create_account.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/account_api.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/sdk/accountspace_nb.js(2 hunks)src/server/system_services/account_server.js(5 hunks)src/util/account_util.js(14 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-19T15:03:42.231Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.231Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/sdk/accountspace_nb.jssrc/api/account_api.jssrc/util/account_util.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.jssrc/api/account_api.jssrc/util/account_util.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.jssrc/util/account_util.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-18T07:00:17.636Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.636Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/server/system_services/account_server.js
📚 Learning: 2025-06-17T12:59:51.543Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (4)
src/endpoint/iam/iam_rest.js (2)
src/endpoint/iam/iam_utils.js (15)
IamError(82-82)IamError(255-255)IamError(481-481)IamError(510-510)IamError(538-538)IamError(545-545)IamError(573-573)IamError(595-595)IamError(624-624)IamError(659-659)IamError(676-676)IamError(691-691)IamError(698-698)IamError(711-711)IamError(715-715)src/sdk/accountspace_fs.js (1)
IamError(14-14)
src/sdk/accountspace_nb.js (1)
src/server/system_services/account_server.js (27)
require(13-13)require(21-21)require(22-22)require(23-23)require(28-29)requesting_account(1196-1196)requesting_account(1216-1216)requesting_account(1258-1258)requesting_account(1275-1275)requesting_account(1303-1303)requesting_account(1335-1335)requesting_account(1348-1348)requesting_account(1376-1376)requesting_account(1393-1393)requesting_account(1414-1414)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)params(331-331)params(694-694)params(958-958)req(136-136)req(399-399)system_store(17-17)
src/util/account_util.js (3)
src/server/system_services/account_server.js (74)
req(136-136)req(399-399)account(71-71)account(119-119)account(137-139)account(161-161)account(209-209)account(332-332)account(695-695)account(959-959)IamError(27-27)IamError(1320-1320)IamError(1437-1437)action(41-41)action(1195-1195)action(1215-1215)action(1257-1257)action(1274-1274)action(1302-1302)action(1334-1334)action(1346-1346)action(1375-1375)action(1391-1391)action(1413-1413)action(1456-1456)action(1485-1485)action(1515-1515)action(1543-1543)action(1557-1557)username(1198-1198)username(1259-1259)username(1381-1381)username(1415-1415)username(1458-1458)username(1487-1487)system_store(17-17)dbg(12-12)params(331-331)params(694-694)params(958-958)requesting_account(1196-1196)requesting_account(1216-1216)requesting_account(1258-1258)requesting_account(1275-1275)requesting_account(1303-1303)requesting_account(1335-1335)requesting_account(1348-1348)requesting_account(1376-1376)requesting_account(1393-1393)requesting_account(1414-1414)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)requested_account(1197-1197)requested_account(1221-1221)requested_account(1262-1262)requested_account(1304-1304)requested_account(1336-1336)requested_account(1349-1349)requested_account(1395-1395)requested_account(1420-1420)requested_account(1463-1463)requested_account(1492-1492)requested_account(1518-1518)requested_account(1546-1546)requested_account(1560-1560)requested_account(1579-1579)iam_path(1222-1222)iam_path(1285-1285)iam_username(1284-1284)src/endpoint/s3/s3_bucket_policy_utils.js (3)
account(293-293)RpcError(7-7)dbg(5-5)src/endpoint/iam/iam_rest.js (2)
IamError(5-5)dbg(4-4)
src/server/system_services/account_server.js (3)
src/util/account_util.js (33)
require(8-8)require(14-14)require(16-16)require(17-18)IamError(15-15)IamError(330-330)IamError(368-368)IamError(397-397)IamError(422-422)IamError(435-435)IamError(442-442)IamError(449-449)IamError(474-474)IamError(482-482)IamError(493-493)IamError(600-600)req(697-697)access_key(507-508)requested_account(717-717)username(393-394)username(428-429)system_store(12-12)_(4-4)account(31-43)account(197-197)account(305-305)account(326-326)account(364-364)account(640-640)member(569-576)iam_user_policies(632-632)iam_user_policy_index(583-583)iam_user_policy_index(591-592)src/endpoint/iam/iam_utils.js (3)
IamError(82-82)IamError(255-255)IamError(481-481)src/endpoint/iam/iam_constants.js (4)
IAM_ACTIONS(5-23)IAM_DEFAULT_PATH(69-69)ACCESS_KEY_STATUS_ENUM(55-58)MAX_TAGS(67-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
🔇 Additional comments (11)
src/api/account_api.js (1)
627-1064: IAM RPC method shapes look consistent;system: 'admin'auth fixes prior exposureThe new IAM methods (
get_user,update_user,delete_user,list_users, access-key/tag/policy ops) are well-shaped and match the corresponding server replies (e.g.,user_info,user_accesskey_info). Settingauth: { system: 'admin' }for all of them closes the earlier gap where they were anonymously callable at the RPC layer.No further schema issues spotted here.
src/util/account_util.js (3)
325-332: RpcError + IamError refactor for IAM helper errors looks consistentThe helpers now throw
RpcErrorwith IAM-style codes/http codes derived fromIamError.*:
_check_if_account_exists→NoSuchEntity_check_username_already_exists→EntityAlreadyExists_check_if_requested_is_owned_by_root_account/_throw_error_perform_action_on_another_root_account→NoSuchEntity_throw_access_denied_error→AccessDeniedException_throw_error_delete_conflict→DeleteConflict_check_number_of_access_key_array/_check_total_policy_size→LimitExceededThese codes are all present in
RPC_ERRORS_TO_IAM, so IAM REST will reconstruct the correctIamErrorflavor and preserve your detailed messages. The shapes (code,message_with_details,http_code) also align withRpcErrorexpectations.No issues here; this is a solid improvement over throwing raw IamError from deep inside util code.
Also applies to: 363-371, 389-399, 419-437, 439-451, 478-495, 596-602
715-731:validate_and_return_requested_accountis the central IAM authorization gate; semantics look goodThis helper:
- Treats
!params.usernameas “operate on self”, returningrequesting_account.- Otherwise enforces:
- caller is a root account (
_check_if_requesting_account_is_root_account),- target IAM user exists (
_check_if_account_exists),- root-vs-IAM constraints (
_check_if_requested_account_is_root_account_or_IAM_user),- ownership (
_check_if_requested_is_owned_by_root_account).That matches the intended “only root (or its manager) can act on IAM users; IAM users can operate only on themselves” model and centralizes the checks nicely. Just make sure every IAM server method uses this (or an equivalent) when taking a
usernameparameter, which is already true for the new IAM handlers.
769-771: New exports (validate_and_return_requested_account,get_iam_username,return_list_member) are coherentExporting these helpers makes sense given account_server’s new IAM methods:
validate_and_return_requested_accountfor central auth logic,get_iam_usernamefor normalizing names, andreturn_list_memberfor list-users shaping.API surface here looks reasonable; no concern.
src/server/system_services/account_server.js (6)
135-143:read_account_by_access_keyguard on empty access_keys is a solid safety fixThe new predicate:
acc.access_keys && acc.access_keys.length > 0 && acc.access_keys[0].access_key.unwrap() === access_key.unwrap()avoids dereferencing
access_keys[0]when the array is missing or empty. That prevents a potential runtime exception when scanning accounts that don’t yet have keys.Looks good.
1374-1410:delete_access_keylooks correct and reuses shared helpers
delete_access_keycorrectly:
- Validates the caller and target via
validate_and_return_requested_account.- Ensures the key belongs to the target account with
_check_access_key_belongs_to_account.- Uses
get_non_updating_access_keyto produce an encrypted, filtered key list.- Persists via
system_store.make_changes.No obvious edge cases beyond the existing helper behavior; this is consistent and reuses shared logic appropriately.
1514-1587: User policy CRUD and listing leverage shared helpers correctlyFor user policies:
put_user_policy:
- Uses
validate_and_return_requested_account.- Upserts into
requested_account.iam_user_policies.- Enforces total inline policy size via
_check_total_policy_size.
get_user_policy/delete_user_policy:
- Reuse
validate_and_return_requested_accountand_check_user_policy_existsto locate the policy.
list_user_policies:
- Validates and returns sorted policy names with
is_truncated: false.These are clean, consistent, and aligned with how IAM inline policies work (within the current GAPs like missing pagination). No changes needed beyond any future pagination or persisted metadata work.
1590-1627: New IAM exports are wired correctlyAll newly added IAM handlers (
get_user,update_user,delete_user,list_users, access-key/tag/policy methods,get_access_key_last_used) are exported and thus reachable via the expandedaccount_apisurface.Export list is consistent; nothing missing.
1273-1299:list_userswill throw ifaccount.owneris stored as a stringThe filter:
const requesting_account_iam_users = _.filter(system_store.data.accounts, function(account) { // Check IAM user owner is same as requesting_account id return account.owner?._id.toString() === requesting_account._id.toString(); });assumes
account.owneris an object with an_idfield. Ifaccount.owneris ever a plain string (e.g., fromcreate_accountstoringreq.rpc_params.ownerdirectly),account.owner?._idevaluates toundefined, andundefined.toString()throws a TypeError, breakinglist_usersfor those records.Once
account_util.create_accountis fixed to normalizeownerback to an ID object, this will be safe again. If you want extra robustness, you can also defensively handle both shapes:- return account.owner?._id.toString() === requesting_account._id.toString(); + if (!account.owner) return false; + const owner_id = typeof account.owner === 'object' + ? account.owner._id.toString() + : account.owner.toString(); + return owner_id === requesting_account._id.toString();But the primary fix is to ensure the DB invariant that
owneris always an ID reference, not a raw string. Based on learnings⛔ Skipped due to learnings
Learnt from: shirady Repo: noobaa/noobaa-core PR: 9291 File: src/server/common_services/auth_server.js:548-554 Timestamp: 2025-11-19T15:03:42.231Z Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.Learnt from: naveenpaul1 Repo: noobaa/noobaa-core PR: 9277 File: src/endpoint/s3/s3_rest.js:258-261 Timestamp: 2025-11-12T04:55:42.193Z Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.Learnt from: shirady Repo: noobaa/noobaa-core PR: 9281 File: src/server/system_services/account_server.js:1053-1058 Timestamp: 2025-11-13T07:56:23.620Z Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
1412-1512: Request tag shape matches server expectations; code is coherentThe tag definition in
src/api/common_api.js(lines 76-86) specifies properties as lowercasekeyandvalue, which matches exactly how the server code reads them intag_user. The three functions are internally consistent:
tag_usercorrectly readstag.keyandtag.valuefromreq.rpc_params.tagsuntag_usercorrectly filters bytag.keylist_user_tagscorrectly projects the internal schema to the API response schemaNo input normalization is needed.
src/endpoint/iam/iam_rest.js (1)
16-33: Confirm all RpcError codes used by IAM flows are covered byRPC_ERRORS_TO_IAMThe new mapping is IAM-aligned, but any RpcError code not listed here will fall back to
InternalFailure. Given the new helpers inaccount_util.jsand IAM methods inaccount_server.jsnow throw RpcError with IAM-style codes (e.g.,NoSuchEntity,EntityAlreadyExists,LimitExceeded,DeleteConflict), please double‑check that every IAM-related RpcErrorcodeyou emit is either:
- explicitly mapped here, or
- intentionally treated as
InternalFailure.If new codes are added later (e.g., for additional IAM actions), they must be added to this map to preserve correct IAM error semantics.
| owner: { | ||
| type: 'string', | ||
| }, | ||
| iam_path: { | ||
| type: 'string', | ||
| }, |
There was a problem hiding this comment.
Align owner schema with server-side type conversion
create_account now exposes an owner: string param, which is then forwarded into account_util.create_account. On the server, account objects expect owner to be an ObjectId-like reference (with ._id) when stored in system_store.
Please ensure that account_util.create_account always converts this string to a proper system_store ID (e.g., system_store.parse_system_store_id(req.rpc_params.owner)) before persisting, so that downstream helpers that dereference owner._id continue to work without type errors. Based on learnings
🤖 Prompt for AI Agents
In src/api/account_api.js around lines 44 to 49, the owner field is declared as
a string but downstream code expects an ObjectId-like system_store ID; ensure
you convert the string owner to a system_store ID before persisting by calling
system_store.parse_system_store_id(req.rpc_params.owner) (either immediately
before passing params into account_util.create_account or inside
account_util.create_account), so the stored account.owner is an object with the
expected ._id shape and downstream dereferences do not break.
| async create_user(params, account_sdk) { | ||
|
|
||
| const action = IAM_ACTIONS.CREATE_USER; | ||
| 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, | ||
| { username: params.username, path: params.iam_path }); | ||
| account_util._check_username_already_exists(action, params, requesting_account); | ||
| const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), params.username, | ||
| params.iam_path || IAM_DEFAULT_PATH); | ||
| const account_name = account_util.get_account_name_from_username(params.username, requesting_account._id.toString()); | ||
| const req = { | ||
| rpc_params: { | ||
| name: account_name, | ||
| email: account_name, | ||
| has_login: false, | ||
| s3_access: true, | ||
| allow_bucket_creation: true, | ||
| owner: requesting_account._id, | ||
| is_iam: true, | ||
| owner: requesting_account._id.toString(), | ||
| iam_path: params.iam_path, | ||
| role: 'admin', | ||
|
|
||
| roles: ['admin'], | ||
| // TODO: default_resource remove | ||
| default_resource: 'noobaa-default-backing-store', | ||
| }, | ||
| account: requesting_account, | ||
| }; | ||
| const iam_account = await account_util.create_account(req); | ||
|
|
||
| default_resource: requesting_account.default_resource.name, | ||
| }; | ||
| const iam_account = await account_sdk.rpc_client.account.create_account(req, requesting_account); | ||
| // TODO : Clean account cache | ||
| // TODO : Send Event | ||
| const requested_account = system_store.get_account_by_email(account_name); | ||
| return { | ||
| iam_path: requested_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, | ||
| arn: iam_account.arn, | ||
| create_date: iam_account.create_date, | ||
| }; | ||
|
|
||
| } |
There was a problem hiding this comment.
Avoid repeated system_store.get_account_by_email lookups and clarify owner semantics
Every IAM method re‑resolves requesting_account from account_sdk.requesting_account.email, and create_user passes owner: requesting_account._id.toString() into the RPC layer. This works, but:
- You’re doing the same
system_store.get_account_by_email(...)in every method; a tiny helper (or caching onaccount_sdk) would reduce repetition and DB lookups. - On the server side,
account_util.create_accountcurrently assignsaccount.owner = req.rpc_params.ownerdirectly. Since server‑side account objects expectownerto be an ObjectId-like reference (with._id), keepingowneras a bare string in the DB will break places that doaccount.owner._id(for example,return_list_member()andget_account_info()), and will also crashlist_users()when it doesaccount.owner?._id.toString().
I’d recommend:
- keep sending
owneras a string ID over RPC (as you do now), and - in
account_util.create_account, convert that to a system_store ID (e.g., viasystem_store.parse_system_store_id) before assigning toaccount.owner.
That keeps the wire format simple while preserving the in‑memory owner shape expected by account_server/account_util. Based on learnings
🤖 Prompt for AI Agents
In src/sdk/accountspace_nb.js around lines 37-63, avoid repeated
system_store.get_account_by_email lookups by caching the resolved
requesting_account on the account_sdk (e.g., populate
account_sdk.requesting_account once and reuse it) or add a small helper to
return the cached account; and ensure you continue to send owner as a string ID
over RPC but update the server-side account_util.create_account to convert that
string into a system_store ID/object (use system_store.parse_system_store_id or
equivalent) before assigning to account.owner so the in-memory account.owner
retains the expected ObjectId-like shape.
| const SensitiveString = require('../util/sensitive_string'); | ||
| const cloud_utils = require('../util/cloud_utils'); | ||
| const auth_server = require('..//server/common_services/auth_server'); | ||
| const system_store = require('..//server/system_services/system_store').get_instance(); |
There was a problem hiding this comment.
If this file is planned to serve NC and Containerized - why do we have system_store in this file?
There was a problem hiding this comment.
No, planned to serve NC and Containerized, this is only for noobaa accounts and IAM accounts
This wont be share between NSFS NC
There was a problem hiding this comment.
Will push it to next PR, Its not related to account flow
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_account_flow
branch
from
ffe8131 to
8598600
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/account_util.js (1)
106-145: Persisting owner as a raw string breaks server-side owner semanticsIn
create_accountyou now do:const account_access_info = create_access_key_auth(req, account, req.rpc_params.owner); ... if (req.rpc_params.owner) { account.owner = req.rpc_params.owner; account.iam_path = req.rpc_params.iam_path; } ... return { token: auth_server.make_auth_token(auth), access_keys: account_access_info.decrypted_access_keys, id: created_account._id.toString(), create_date: req.rpc_params.owner ? Date.now() : undefined, // GAP };On the server, account objects loaded from
system_storeexpectownerto be an ObjectId-like reference (with._id), not a bare string. This is relied on in several places:
get_account_infoinaccount_server.jsusesaccount.owner._id.toString()to populateinfo.owner._get_account_owner_id_for_arnusesrequesting_account.owner._idto build ARNs._check_root_account_owns_userandreturn_list_memberaccessuser_account.owner._id.If IAM users are created with
account.ownerset to a string here, those sites will either throw (owner._idis undefined) or silently produce wrong ARNs/ownership checks, andlist_users’account.owner?._id.toString() === requesting_account._id.toString()will fail for such users. Based on learnings,account_server.jsonly processes accounts fromaccount_schemawhereowneris an ObjectId-like reference, so this regression is specific to the new IAM flow.You should convert the incoming owner string to a system_store ID before persisting, and keep the in‑memory shape consistent:
async function create_account(req) { - const account_access_info = create_access_key_auth(req, account, req.rpc_params.owner); + const is_iam_user = Boolean(req.rpc_params.owner); + const account_access_info = create_access_key_auth(req, account, is_iam_user); ... - if (req.rpc_params.owner) { - account.owner = req.rpc_params.owner; - account.iam_path = req.rpc_params.iam_path; - } + if (req.rpc_params.owner) { + account.owner = system_store.parse_system_store_id(req.rpc_params.owner); + account.iam_path = req.rpc_params.iam_path || IAM_DEFAULT_PATH; + } ... - return { - token: auth_server.make_auth_token(auth), - access_keys: account_access_info.decrypted_access_keys, - id: created_account._id.toString(), - create_date: req.rpc_params.owner ? Date.now() : undefined, - }; + return { + token: auth_server.make_auth_token(auth), + access_keys: account_access_info.decrypted_access_keys, + id: created_account._id.toString(), + create_date: is_iam_user ? Date.now() : undefined, // GAP: still not persisted + }; }This keeps the wire format (
owneras string ID over RPC) while preserving the ObjectId‑likeownershape insidesystem_store, matching how other server paths behave.
♻️ Duplicate comments (3)
src/sdk/accountspace_nb.js (1)
39-170: Repeated system_store.get_account_by_email lookups in every IAM methodEach IAM wrapper (
create_user,get_user,update_user,delete_user,list_users, access‑key/tag/policy methods) re‑resolvesrequesting_accountfromaccount_sdk.requesting_account.emailviasystem_store.get_account_by_email(...)before making the RPC call.Given
account_sdk.requesting_accountis already the authenticated account, you can:
- either pass it directly to the RPC calls, or
- add a small helper on
AccountSpaceNBto resolve/cache once and reuse.This will cut redundant DB reads and simplify the code, without changing behavior.
src/server/system_services/account_server.js (1)
1195-1212: get_user builds ARNs from caller ID instead of the user’s owning account
get_usercurrently does:const requesting_account = req.account; const requested_account = account_util.validate_and_return_requested_account(req.rpc_params, action, requesting_account); const username = account_util.get_iam_username(req.rpc_params.username || requested_account.name.unwrap()); const iam_arn = iam_utils.create_arn_for_user( requesting_account._id.toString(), username, requested_account.iam_path || IAM_DEFAULT_PATH );If an IAM user calls
GetUseron itself (nousernameparam),requesting_account === requested_account, so the ARN accountId comes from the IAM user’s own_id. When the root owner callsGetUserfor the same user, the ARN accountId comes from the root account. The same logical user can thus have different ARNs depending on who calls the API.To make ARNs stable and rooted at the owning root account, derive the accountId from the requested account’s owner when present, or from its own
_idwhen it’s a root account. For example:- const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), username, - requested_account.iam_path || IAM_DEFAULT_PATH); + let owner_id_for_arn; + if (requested_account.owner !== undefined) { + owner_id_for_arn = typeof requested_account.owner === 'object' + ? requested_account.owner._id.toString() + : requested_account.owner.toString(); + } else { + owner_id_for_arn = requested_account._id.toString(); + } + const iam_arn = iam_utils.create_arn_for_user( + owner_id_for_arn, + username, + requested_account.iam_path || IAM_DEFAULT_PATH + );This keeps the ARN consistent regardless of caller and correctly anchors it to the owning root account.
src/api/account_api.js (1)
645-705: create_user should require owner to avoid accidentally creating root accountsThe
create_userRPC is intended for IAM users, but its params only require['email', 'name']whileowneris optional:required: ['email', 'name'], properties: { ... owner: { type: 'string' }, iam_path: { type: 'string' }, }On the server,
account_server.create_userjust forwards intocreate_account(req), whose IAM branch is triggered solely byreq.rpc_params.owner. Ifowneris omitted, the call will follow the non‑IAM branch and effectively create a regular/root account instead of an IAM user.To keep the API unambiguous, you should either:
- add
ownerto therequiredarray forcreate_user, or- drop the explicit
ownerparam and derive the owner fromreq.accountin the server implementation.
🧹 Nitpick comments (2)
src/api/account_api.js (1)
916-1000: Tag input vs output schema use different tag definitions (intentional, but worth documenting)For
tag_user, thetagsparam usescommon_api#/definitions/tag, whilelist_user_tagsreply wrapsmemberwith the local#/definitions/tag(Key/Value). Server-side,account_server.tag_userstores tags as{ key, value }andlist_user_tagsmaps them to{ Key, Value }in the response.Functionally this works, but the mixed use of
common_apivs localtagmay be confusing for clients. Consider:
- adding a brief comment near the methods stating that input uses lower-case
key/valueand output uses AWS-styleKey/Value, or- standardizing both sides on the same schema if you don’t need the distinction.
src/util/account_util.js (1)
195-256: Using req.rpc_params.owner as the “is_iam” flag in generate_account_keysIn
generate_account_keysyou now distinguish IAM from non‑IAM flows by checkingreq.rpc_params.owner:// IAM create accesskey will have multiple accesskeys. if (req.rpc_params.owner) { account_access_keys.push(access_key_obj); } else { // Noobaa CLI create and generate accesskey commands replace existing accesskey account_access_keys[0] = access_key_obj; } ... if (req.rpc_params.owner) { return decrypted_access_keys; }Functionally this is fine given the new IAM flows always pass
ownerfor IAM users, and non‑IAM callers don’t. To make intent clearer and avoid accidental coupling to the exact type ofowner(string vs falsy), consider normalizing at the top:async function generate_account_keys(req) { + const is_iam_user = Boolean(req.rpc_params.owner); ... - if (req.rpc_params.owner) { + if (is_iam_user) { account_access_keys.push(access_key_obj); } else { account_access_keys[0] = access_key_obj; } ... - if (req.rpc_params.owner) { + if (is_iam_user) { return decrypted_access_keys; }This avoids subtle bugs if
owneris ever changed to a non-string truthy value or refactored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/api/account_api.js(4 hunks)src/endpoint/iam/iam_rest.js(2 hunks)src/sdk/accountspace_nb.js(2 hunks)src/server/system_services/account_server.js(5 hunks)src/util/account_util.js(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/endpoint/iam/iam_rest.js
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-19T15:03:42.231Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.231Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/server/system_services/account_server.jssrc/sdk/accountspace_nb.jssrc/util/account_util.jssrc/api/account_api.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/system_services/account_server.jssrc/sdk/accountspace_nb.jssrc/util/account_util.jssrc/api/account_api.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/server/system_services/account_server.jssrc/sdk/accountspace_nb.jssrc/util/account_util.jssrc/api/account_api.js
📚 Learning: 2025-11-18T07:00:17.636Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.636Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/server/system_services/account_server.js
📚 Learning: 2025-06-17T12:59:51.543Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9042
File: src/util/cloud_utils.js:183-194
Timestamp: 2025-06-17T12:59:51.543Z
Learning: In the set_noobaa_s3_connection function in src/util/cloud_utils.js, internal NooBaa S3 endpoints (filtered by 'api': 's3', 'kind': 'INTERNAL') will always be non-secure, so tls: false is the correct setting despite the conditional https:// scheme logic.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (3)
src/server/system_services/account_server.js (2)
src/util/account_util.js (16)
req(685-685)access_key(496-497)requested_account(705-705)username(390-391)username(423-424)system_store(12-12)_(4-4)account(30-42)account(196-196)account(304-304)account(325-325)account(362-362)account(628-628)iam_user_policies(620-620)iam_user_policy_index(572-572)iam_user_policy_index(580-581)src/endpoint/iam/iam_constants.js (4)
IAM_ACTIONS(5-23)IAM_DEFAULT_PATH(69-69)ACCESS_KEY_STATUS_ENUM(55-58)MAX_TAGS(67-67)
src/sdk/accountspace_nb.js (3)
src/server/system_services/account_server.js (24)
require(13-13)require(21-21)require(22-22)require(23-23)require(27-28)requesting_account(1198-1198)requesting_account(1218-1218)requesting_account(1260-1260)requesting_account(1277-1277)requesting_account(1305-1305)requesting_account(1336-1336)requesting_account(1349-1349)requesting_account(1377-1377)requesting_account(1394-1394)requesting_account(1415-1415)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)req(134-134)req(397-397)system_store(17-17)src/util/account_util.js (6)
require(8-8)require(14-14)require(15-15)require(16-17)req(685-685)system_store(12-12)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
src/util/account_util.js (2)
src/server/system_services/account_server.js (68)
req(134-134)req(397-397)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)action(40-40)action(1197-1197)action(1217-1217)action(1259-1259)action(1276-1276)action(1304-1304)action(1335-1335)action(1347-1347)action(1376-1376)action(1392-1392)action(1414-1414)action(1456-1456)action(1485-1485)action(1515-1515)action(1543-1543)action(1557-1557)username(1200-1200)username(1261-1261)username(1382-1382)username(1416-1416)username(1458-1458)username(1487-1487)params(329-329)params(692-692)params(956-956)requesting_account(1198-1198)requesting_account(1218-1218)requesting_account(1260-1260)requesting_account(1277-1277)requesting_account(1305-1305)requesting_account(1336-1336)requesting_account(1349-1349)requesting_account(1377-1377)requesting_account(1394-1394)requesting_account(1415-1415)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)requested_account(1199-1199)requested_account(1223-1223)requested_account(1264-1264)requested_account(1306-1306)requested_account(1337-1337)requested_account(1350-1350)requested_account(1396-1396)requested_account(1421-1421)requested_account(1463-1463)requested_account(1492-1492)requested_account(1518-1518)requested_account(1546-1546)requested_account(1560-1560)requested_account(1579-1579)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(293-293)RpcError(7-7)
⏰ 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 (2)
src/util/account_util.js (2)
703-719: validate_and_return_requested_account helper looks solid and centralizes IAM account resolutionThe new
validate_and_return_requested_accountcorrectly captures the common pattern:
- When
params.usernameis missing, it treats the action as operating on the requesting account itself.- When
params.usernameis provided, it enforces:
- root‑only operation via
_check_if_requesting_account_is_root_account,- existence of the derived account email,
- that the requested account is a root or IAM user as appropriate, and
- that it is owned by the requesting root via
_check_if_requested_is_owned_by_root_account.This nicely de-duplicates logic across get_user, access‑key, and policy methods. No changes needed here.
721-731: return_list_member assumes iam_user.owner is an ObjectId-like reference
return_list_memberbuilds the ARN using:arn: create_arn_for_user(iam_user.owner._id.toString(), iam_username, iam_path),This relies on
iam_user.ownerbeing an ObjectId-like reference (with._id), which is correct oncecreate_accountstores owner viasystem_store.parse_system_store_idas suggested above. Just be aware that if owner were ever accidentally left as a string again, this would throw. Fixing owner persistence increate_accountshould keep this helper safe.
| async create_user(params, account_sdk) { | ||
|
|
||
| const action = IAM_ACTIONS.CREATE_USER; | ||
| 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, | ||
| { username: params.username, path: params.iam_path }); | ||
| account_util._check_username_already_exists(action, params, requesting_account); | ||
| const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), params.username, | ||
| params.iam_path || IAM_DEFAULT_PATH); | ||
| const account_name = account_util.get_account_name_from_username(params.username, requesting_account._id.toString()); | ||
| const req = { | ||
| rpc_params: { | ||
| name: account_name, | ||
| email: account_name, | ||
| has_login: false, | ||
| s3_access: true, | ||
| allow_bucket_creation: true, | ||
| owner: requesting_account._id, | ||
| is_iam: true, | ||
| owner: requesting_account._id.toString(), | ||
| iam_path: params.iam_path, | ||
| role: 'admin', | ||
|
|
||
| roles: ['admin'], | ||
| // TODO: default_resource remove | ||
| default_resource: 'noobaa-default-backing-store', | ||
| }, | ||
| account: requesting_account, | ||
| }; | ||
| const iam_account = await account_util.create_account(req); | ||
|
|
||
| default_resource: requesting_account.default_resource.name, | ||
| }; | ||
| const iam_account = await account_sdk.rpc_client.account.create_user(req, requesting_account); | ||
| // TODO : Clean account cache | ||
| // TODO : Send Event | ||
| const requested_account = system_store.get_account_by_email(account_name); | ||
| return { | ||
| iam_path: requested_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, | ||
| arn: iam_account.arn, | ||
| create_date: iam_account.create_date, | ||
| }; |
There was a problem hiding this comment.
create_user returns create_date that is never set by the RPC server
create_user forwards to account.create_user and then exposes create_date: iam_account.create_date, but account_server.create_account only returns { id, arn, token, access_keys } and drops the create_date coming from account_util.create_account. This means create_date here will always be undefined.
Either:
- propagate
create_datefromaccount_server.create_accountand include it in the RPC reply, or - drop
create_datefrom this SDK response until the server actually sets it, to avoid a misleading field.
🤖 Prompt for AI Agents
In src/sdk/accountspace_nb.js around lines 37 to 62, the returned object
includes create_date which is never provided by the RPC response; remove the
create_date property from the returned object so the SDK does not expose an
undefined value. Update the return to omit create_date (leave iam_path,
username, user_id, arn) and add a TODO comment noting to reintroduce create_date
once account_server.create_account includes it in its RPC reply.
|
@naveenpaul1 Approving as I understand that would be following PR |
2 participants
Describe the Problem
Use the account server flow
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.