IAM | IAM integration tests #9300
Conversation
WalkthroughNarrowed IAM get_user calls to pass only Changes
Sequence Diagram(s)sequenceDiagram
participant Endpoint
participant AccountSDK
participant RPC_Account
Endpoint->>AccountSDK: get_user({ username: params.username })
AccountSDK->>RPC_Account: account.get_user({ username })
RPC_Account-->>AccountSDK: user data
AccountSDK-->>Endpoint: user data
sequenceDiagram
participant Caller
participant AuthServer
participant DB
Caller->>AuthServer: _authorize_signature_token(access_key)
AuthServer->>DB: fetch account (acc)
DB-->>AuthServer: acc
AuthServer->>AuthServer: check acc.access_keys && acc.access_keys.length > 0
alt access_keys present
AuthServer->>AuthServer: compare acc.access_keys[0] with provided key
AuthServer-->>Caller: validated
else missing/empty
AuthServer-->>Caller: fail validation
end
sequenceDiagram
participant AccountServer
participant AccountUtil
participant RequestingAccount
AccountServer->>AccountUtil: get_owner_account_id(account)
AccountUtil-->>AccountServer: owner_account_id
AccountServer->>RequestingAccount: compare owner_account_id === requesting_account._id.toString()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
naveenpaul1
force-pushed
the
iam_unit_test
branch
3 times, most recently
from
ce5ae4f to
4cd5d91
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: 0
🧹 Nitpick comments (3)
src/sdk/accountspace_nb.js (1)
65-72: Param sanitization inget_userlooks correct; consider avoiding parameter reassignmentConstraining the RPC call to
{ username }is a reasonable way to satisfy the schema when callers pass extra fields. To keep style consistent and avoid param reassignment (which some linters disallow), you could optionally use a new local:- async get_user(params, account_sdk) { - // Added to fix the coretest, ListAttachedUserPoliciesCommand - // calling get_user with more param we allow and its failing RPC schema validation. - params = { - username: params.username, - }; + async get_user(params, account_sdk) { + // ListAttachedUserPoliciesCommand may pass extra params; normalize to RPC schema. + const sanitized_params = { username: params.username }; const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); - return await account_sdk.rpc_client.account.get_user(params, requesting_account); + return await account_sdk.rpc_client.account.get_user(sanitized_params, requesting_account);src/server/common_services/auth_server.js (1)
318-327: Access key length guard in_authorize_signature_tokenis correct; consider aligningcreate_access_key_authAdding
acc.access_keys.length > 0before dereferencingaccess_keys[0]is a good hardening step and prevents crashes when an account has an emptyaccess_keysarray.For consistency and robustness, you may want to mirror this guard in
create_access_key_auth(around Lines 189‑195), e.g.:- const account = _.find(system_store.data.accounts, function(acc) { - if (acc.access_keys) { - return acc.access_keys[0].access_key.unwrap().toString() === access_key.toString(); - } else { - return false; - } - }); + const account = _.find(system_store.data.accounts, function(acc) { + return acc.access_keys && + acc.access_keys.length > 0 && + acc.access_keys[0].access_key.unwrap().toString() === access_key.toString(); + });src/util/account_util.js (1)
725-732: Reusingget_owner_account_idfor all owner‑based ARN logic would improve consistencyUsing
get_owner_account_id(iam_user)inreturn_list_memberfixes the prior assumption thatiam_user.owneris always an object with an._id, and it correctly supports both object and string owners when constructing ARNs. Exportingget_owner_account_idis a good step toward centralizing this normalization.To further reduce subtle “owner shape” bugs across environments, consider also switching
_get_account_owner_id_for_arnto rely onget_owner_account_idfor IAM users/root accounts instead of accessing.owner._iddirectly. That would align all owner-ID derivation with a single helper that already encodes the dual representation from the learnings.Also applies to: 765-765
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/sdk/accountspace_nb.js(1 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/index/index.jssrc/test/utils/coretest/coretest.jssrc/test/utils/index/nc_index.jssrc/test/integration_tests/api/iam/test_iam_basic_integration.js
🧠 Learnings (3)
📚 Learning: 2025-11-19T15:03:42.260Z
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.260Z
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/server/common_services/auth_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/server/system_services/account_server.jssrc/server/common_services/auth_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/server/system_services/account_server.jssrc/util/account_util.js
🧬 Code graph analysis (3)
src/sdk/accountspace_nb.js (2)
src/server/system_services/account_server.js (3)
params(329-329)params(692-692)params(956-956)src/endpoint/iam/ops/iam_list_user_policies.js (1)
params(14-18)
src/server/system_services/account_server.js (1)
src/util/account_util.js (9)
owner_account_id(341-341)owner_account_id(346-346)owner_account_id(726-726)account(30-42)account(196-196)account(304-304)account(325-325)account(366-366)account(632-632)
src/util/account_util.js (1)
src/server/system_services/account_server.js (4)
owner_account_id(1281-1281)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)
🔇 Additional comments (8)
src/test/utils/coretest/coretest.js (1)
63-72: IAM HTTPS test server wiring mirrors STS pattern and looks sound; verify address usage timingThe new IAM endpoint handler, HTTPS server, address computation, teardown, and
get_iam_https_addressexport are all consistent with the existing STS wiring and should integrate cleanly with the test harness.One thing to double‑check:
https_address_iamis only initialized inside the mochabeforehook. If any IAM tests readcoretest.get_iam_https_address()at module load time (top‑level, before the hook runs), they’ll seeundefined. As long as the getter is only used insidebefore/itblocks (or aftercoretest.setup()has registered and executed itsbefore), you’re fine.Also applies to: 148-156, 214-221, 227-238, 294-295, 482-484, 699-714
src/test/utils/index/index.js (1)
112-114: Shared IAM integration test is correctly wired into the main indexRequiring
test_iam_basic_integrationhere makes sense and ensures the new IAM behavior is covered in the standard coretest suite.src/test/utils/index/nc_index.js (1)
27-29: NC test index now correctly reuses the shared IAM integration testSwitching to
../../integration_tests/api/iam/test_iam_basic_integrationaligns NC tests with the shared IAM test suite and matches the new naming; the wiring looks correct.src/server/system_services/account_server.js (1)
1280-1283: Centralizing IAM user owner filtering viaget_owner_account_idlooks correctUsing
account_util.get_owner_account_id(account)here preserves the previous behavior for containerized deployments whereaccount.owneris an object ID, and also gracefully handles potential string owners or missing owners by just excluding such accounts from the IAM user list. This aligns with the mixed owner representations described in the learnings.src/util/account_util.js (1)
333-343:get_owner_account_idcorrectly abstracts owner ID normalization for mixed account shapesThe refactor of
_check_root_account_owns_userto computeroot_account_idonce and compare it againstget_owner_account_id(user_account)is sound. The new helper handles both cases described in the learnings—owneras an object ID reference (fromsystem_store) and as a primitive/string (after RPC serialization)—while_check_root_account_owns_userstill explicitly guards the “no owner” case.Also applies to: 345-353
src/test/integration_tests/api/iam/test_iam_basic_integration.js (3)
10-13: Environment‑aware IAM client setup with proper key unwrapping looks solidImporting
SensitiveString, wiring inrequire_coretest/is_nc_coretest, and normalizingres.access_key/res.secret_keybefore constructing the IAM client ensures this suite runs correctly in both NC and containerized modes while handling both SensitiveString and plain string keys. Usingcoretest.get_iam_https_address()plusshould_run_iam/https_port_iamaligns the client with the dedicated IAM HTTPS server described in the PR.Also applies to: 31-36, 42-42, 47-69
47-62: NC vs containerized setup branching is appropriate and keeps the suite reusableThe
is_nc_coretestbranch cleanly reconfigures NSFS when needed, prepares a fresh buckets path, and provisions an admin NSFS account, while the else branch reuses an existing account for containerized runs. This achieves the PR goal of sharing the same IAM integration tests across both environments with minimal duplication.
205-210: Selective skip ofGetAccessKeyLastUsedin containerized mode is a reasonable interim workaroundDocumenting that the test currently uses an access key not belonging to the requesting account and skipping it only when
!is_nc_coretestavoids false failures in the containerized path (which usesaccount_server.jsfor IAM) while keeping the check active for NC, where a different implementation is used. Once the containerized implementation is aligned with the intended semantics, this guard can be revisited.
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
4cd5d91 to
25db419
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/sdk/accountspace_nb.js(1 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/common_services/auth_server.js
- src/server/system_services/account_server.js
- src/test/utils/index/index.js
- src/util/account_util.js
- src/test/utils/index/nc_index.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/api/iam/test_iam_basic_integration.jssrc/test/utils/coretest/nc_coretest.jssrc/test/utils/coretest/coretest.js
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/test/integration_tests/api/iam/test_iam_basic_integration.js
🧬 Code graph analysis (3)
src/sdk/accountspace_nb.js (1)
src/server/system_services/account_server.js (3)
params(329-329)params(692-692)params(956-956)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (3)
src/server/common_services/auth_server.js (3)
SensitiveString(12-12)_(4-4)access_key(181-181)src/util/account_util.js (3)
SensitiveString(9-9)_(4-4)access_key(500-501)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/test/utils/coretest/coretest.js (1)
src/test/utils/coretest/nc_coretest.js (6)
https_address_iam(55-55)https_port_iam(52-52)https_port(51-51)http_address(53-53)http_port(50-50)https_address(54-54)
⏰ 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 (5)
src/sdk/accountspace_nb.js (1)
66-70: Parameter sanitization correctly fixes the RPC validation issue.The approach is straightforward and effective: by filtering params to only include
username, you prevent RPC schema validation errors whenListAttachedUserPoliciesCommandorListMFADevicesCommandcall this method with extra fields. The fix aligns with the described problem in the past review comments.src/test/utils/coretest/nc_coretest.js (1)
284-289: LGTM! Function rename improves consistency.The rename from
get_iam_https_addresstoget_https_address_iamaligns with the STS naming convention (get_https_address_sts), improving API consistency across the codebase.Also applies to: 557-557
src/test/utils/coretest/coretest.js (1)
69-70: LGTM! IAM HTTPS server integration follows established patterns.The IAM endpoint integration mirrors the existing STS implementation:
- Creates dedicated endpoint handler and server
- Extracts dynamic port assignment
- Provides public getter for the IAM address
- Properly manages lifecycle (setup and teardown)
This ensures consistent behavior across S3, STS, and IAM endpoints in the test environment.
Also applies to: 153-154, 213-219, 229-230, 236-236, 293-294, 481-483, 712-712
src/test/integration_tests/api/iam/test_iam_basic_integration.js (2)
10-10: LGTM! Proper handling of credentials and dynamic endpoint.The changes correctly:
- Import
SensitiveStringandis_nc_coretestutilities- Use dynamic IAM endpoint via
coretest.get_https_address_iam()- Defensively unwrap
SensitiveStringinstances before passing to IAM clientThe unwrapping pattern matches established usage in the codebase (e.g.,
src/util/account_util.jsline 499-500).Also applies to: 13-13, 65-68
49-62: All verification concerns have been addressed.The NSFS restart logic is necessary and correctly implemented:
Setup options comparison: _.isEqual() performs a deep comparison between two values and will detect any property difference in the full setup_options object (forks, debug, should_run_iam, https_port_iam). This is sufficient to identify all relevant configuration changes requiring NSFS restart.
No concurrent test interference: The test suite runs sequentially. The package.json mocha command contains no
--parallelflag, and no mocha configuration enables parallel execution. Tests execute serially from a single entry point, making concurrent restart conflicts impossible.No race condition: The restart sequence includes an intentional
await P.delay(5000)instart_nsfs_process()with an explicit comment: "wait for the process to be ready (else would see ECONNREFUSED issue)". This delay ensures both the old process termination and new process startup/listening are complete before subsequent operations execute.
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
25db419 to
297cf88
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/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
73-75: Guardconfig_rootusage inafterhook
config_rootis only initialized in the NC branch; on containerized runs it staysundefined, sofolder_delete(\${config_root}`)will operate on the literal"undefined"path. Better to no-op whenconfig_root` is unset:- mocha.after(async () => { - fs_utils.folder_delete(`${config_root}`); - }); + mocha.after(async () => { + if (config_root) { + fs_utils.folder_delete(config_root); + } + });
♻️ Duplicate comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
36-71: Harden access key selection inbeforehook (and confirmgenerate_nsfs_accountreturn shape)The shared setup is moving in the right direction, but there are a couple of edge cases to close:
- For the non-NC branch (Line 64),
(await rpc_client.account.read_account({ email: EMAIL })).access_keys[0]will throw ifaccess_keysis missing or empty, causing the entire suite to fail before any test runs. This was flagged earlier and is still unresolved; please add a guard (or explicit assertion) before indexing. For example:- } else { - account_res = (await rpc_client.account.read_account({ email: EMAIL })).access_keys[0]; - } + } else { + const account = await rpc_client.account.read_account({ email: EMAIL }); + if (!account.access_keys || account.access_keys.length === 0) { + throw new Error(`Account ${EMAIL} has no access keys for IAM testing`); + } + account_res = account.access_keys[0]; + }
- For the NC branch,
account_res = await generate_nsfs_account(...)assumes thatgenerate_nsfs_accountreturns an object withaccess_keyandsecret_keyproperties. If it actually returns an account object with anaccess_keysarray (or some other shape),access_key/secret_keywill beundefinedhere and the IAM client will be misconfigured. Please double-check the helper’s return type and, if needed, mirror the non-NC logic (e.g., pickaccess_keys[0]there as well).
🧹 Nitpick comments (1)
src/util/account_util.js (1)
333-353:get_owner_account_idcorrectly unifies owner handling; consider reusing it more broadlyThe new
get_owner_account_id(user_account)and its use in_check_root_account_owns_userandreturn_list_membercorrectly handle both cases whereowneris an object with_id(system_store context) and where it is already a string account ID (RPC-serialized context). This aligns with the previously noted difference betweenauth_serverands3_restowner representations. Based on learnings, this is the right direction.You still have similar owner-ID normalization logic in
_get_account_owner_id_for_arn; as a follow-up refactor (not required for this PR), you could route that throughget_owner_account_idas well to avoid duplicating type handling for owner values.Also applies to: 725-732, 765-765
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/endpoint/iam/ops/iam_list_attached_user_policies.js(1 hunks)src/endpoint/iam/ops/iam_list_groups_for_user.js(1 hunks)src/endpoint/iam/ops/iam_list_mfa_devices.js(1 hunks)src/endpoint/iam/ops/iam_list_service_specific_credentials.js(1 hunks)src/endpoint/iam/ops/iam_list_signing_certificates.js(1 hunks)src/endpoint/iam/ops/iam_list_ssh_public_keys.js(1 hunks)src/sdk/accountspace_nb.js(0 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/sdk/accountspace_nb.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/common_services/auth_server.js
- src/server/system_services/account_server.js
- src/test/utils/coretest/coretest.js
- src/test/utils/index/index.js
- src/test/utils/coretest/nc_coretest.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/index/nc_index.jssrc/test/integration_tests/api/iam/test_iam_basic_integration.js
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/endpoint/iam/ops/iam_list_service_specific_credentials.jssrc/util/account_util.jssrc/endpoint/iam/ops/iam_list_attached_user_policies.jssrc/endpoint/iam/ops/iam_list_groups_for_user.jssrc/endpoint/iam/ops/iam_list_signing_certificates.jssrc/endpoint/iam/ops/iam_list_ssh_public_keys.js
📚 Learning: 2025-11-19T15:03:42.260Z
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.260Z
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/util/account_util.js
🧬 Code graph analysis (7)
src/endpoint/iam/ops/iam_list_service_specific_credentials.js (2)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)src/endpoint/iam/ops/iam_list_user_policies.js (1)
params(14-18)
src/util/account_util.js (1)
src/server/system_services/account_server.js (4)
owner_account_id(1281-1281)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)
src/endpoint/iam/ops/iam_list_attached_user_policies.js (1)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)
src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (4)
src/endpoint/iam/ops/iam_list_signing_certificates.js (2)
SensitiveString(8-8)require(7-7)src/server/system_services/account_server.js (7)
SensitiveString(15-15)require(13-13)require(21-21)require(22-22)require(23-23)require(27-28)_(5-5)src/util/account_util.js (3)
SensitiveString(9-9)_(4-4)access_key(500-501)src/test/system_tests/test_utils.js (2)
is_nc_coretest(48-48)TMP_PATH(23-23)
src/endpoint/iam/ops/iam_list_mfa_devices.js (1)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)
src/endpoint/iam/ops/iam_list_ssh_public_keys.js (1)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)
⏰ 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 (7)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
14-21: Narrowedget_usercall is consistent with params and other IAM ops
paramsonly containsusername, so callingreq.account_sdk.get_user({ username: params.username })is equivalent to the previous call and matches the pattern used in the other IAM list ops in this PR.src/endpoint/iam/ops/iam_list_mfa_devices.js (1)
13-22: Username-onlyget_usercall keeps existing behaviorWhen
usernameis provided,validate_paramsstill sees the fullparamsobject whileget_usernow receives{ username }, which is all it needs; the no-username branch remains unchanged.src/endpoint/iam/ops/iam_list_service_specific_credentials.js (1)
14-22:get_userargument narrowing is safe here
req.account_sdk.get_user({ username: params.username })still enforces user existence while avoiding leaking unrelated params into the SDK call; overall behavior of the endpoint is preserved.src/endpoint/iam/ops/iam_list_ssh_public_keys.js (1)
14-23: Consistentget_user({ username })usage for SSH public keysUsing
{ username: params.username }here aligns with the other IAM ops and keeps the same error/no-error behavior when the user exists or not.src/endpoint/iam/ops/iam_list_attached_user_policies.js (1)
15-25: Minimalget_userparams without changing validation
iam_utils.validate_paramsstill validates the fullparamsobject, whileget_usernow receives only{ username }, which is all it consumes; behavior and error handling for this operation remain the same.src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
15-24: Username-onlyget_userpreserves list-signing-certificates semanticsThe new
req.account_sdk.get_user({ username: params.username })call continues to enforce user existence whenusernameis provided, while the no-username code path is unchanged.src/test/utils/index/nc_index.js (1)
28-28: LGTM!The require path correctly reflects the renamed test file (
test_nc_iam_basic_integration.js→test_iam_basic_integration.js), aligning with the PR's goal to share IAM tests between NC and containerized Noobaa. The removal of the.jsextension is consistent with most other requires in this file.
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
297cf88 to
96e8b18
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/util/account_util.js (1)
333-343: Owner-ID helper correctly unifies object vs string representationsRefactoring
_check_root_account_owns_userto useget_owner_account_id()plus exporting the helper looks good and matches the mixed owner shapes we see (ObjectId with._idfromsystem_storevs plain string from RPC-serialized flows). Thetypeof user_account.owner === 'object'branch withString(user_account.owner._id)should safely cover theauth_server/account_serverside, while theelsebranch preserves the S3/RPC case.Two small nits you may want to consider (non-blocking):
- Clarify via a short JSDoc or comment that
get_owner_account_idis intended for IAM users whereowneris defined; for root accounts it will returnundefined.- If you ever need similar normalization in
_get_account_owner_id_for_arn, reusing this helper there could reduce future drift.Based on learnings, this aligns with the documented difference between
account.ownercoming fromsystem_storevs RPC-serialized contexts.Also applies to: 345-353, 765-765
src/server/common_services/auth_server.js (1)
315-323: Guard onaccess_keys.lengthfixes potential runtime errorTightening the
_authorize_signature_tokenlookup to:return acc.access_keys && acc.access_keys.length > 0 && acc.access_keys[0].access_key.unwrap() === auth_token_obj.access_key;correctly prevents
TypeErrorwhenaccess_keysis an empty array while preserving existing behavior. This is a safe, focused hardening of the authorization path.If you ever decide to support matching against multiple access keys per account here, you could extend this to search all entries in
acc.access_keys, but that’s outside the scope of this change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/endpoint/iam/ops/iam_list_attached_user_policies.js(1 hunks)src/endpoint/iam/ops/iam_list_groups_for_user.js(1 hunks)src/endpoint/iam/ops/iam_list_mfa_devices.js(1 hunks)src/endpoint/iam/ops/iam_list_service_specific_credentials.js(1 hunks)src/endpoint/iam/ops/iam_list_signing_certificates.js(1 hunks)src/endpoint/iam/ops/iam_list_ssh_public_keys.js(1 hunks)src/sdk/accountspace_nb.js(0 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/sdk/accountspace_nb.js
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/utils/index/nc_index.js
- src/endpoint/iam/ops/iam_list_signing_certificates.js
- src/server/system_services/account_server.js
- src/endpoint/iam/ops/iam_list_attached_user_policies.js
- src/test/integration_tests/api/iam/test_iam_basic_integration.js
- src/endpoint/iam/ops/iam_list_groups_for_user.js
- src/endpoint/iam/ops/iam_list_mfa_devices.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/index/index.jssrc/test/utils/coretest/coretest.jssrc/test/utils/coretest/nc_coretest.js
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/endpoint/iam/ops/iam_list_service_specific_credentials.jssrc/util/account_util.jssrc/endpoint/iam/ops/iam_list_ssh_public_keys.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/test/utils/index/index.jssrc/server/common_services/auth_server.js
📚 Learning: 2025-11-19T15:03:42.260Z
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.260Z
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/util/account_util.jssrc/server/common_services/auth_server.js
🧬 Code graph analysis (2)
src/util/account_util.js (1)
src/server/system_services/account_server.js (4)
owner_account_id(1281-1281)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)
src/test/utils/coretest/coretest.js (1)
src/test/utils/coretest/nc_coretest.js (6)
https_address_iam(55-55)https_port_iam(52-52)https_port(51-51)http_address(53-53)http_port(50-50)https_address(54-54)
⏰ 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 (6)
src/endpoint/iam/ops/iam_list_ssh_public_keys.js (1)
22-22: Good refactor – clearer API contract.Narrowing the
get_usercall to pass only{username: params.username}instead of the fullparamsobject improves clarity and avoids passing unnecessary fields likemarkerandmax_items.src/endpoint/iam/ops/iam_list_service_specific_credentials.js (1)
21-21: Good refactor – clearer API contract.Narrowing the
get_usercall to pass only{username: params.username}instead of the fullparamsobject improves clarity and avoids passing the unnecessaryservice_namefield.src/test/utils/index/index.js (1)
112-114: IAM integration test wiring looks correctRequiring
../../integration_tests/api/iam/test_iam_basic_integrationfrom the common test index is consistent with how other integration tests are registered and matches the renamed file path. No issues from my side.src/test/utils/coretest/coretest.js (1)
69-71: IAM HTTPS test server wiring is consistent with existing STS setupThe new IAM endpoint handler, HTTPS server startup/teardown, address computation, and
get_https_address_iam()export all mirror the existing STS pattern and are cleanly integrated into the coretest lifecycle. This should give the IAM integration tests a stable, dedicated HTTPS endpoint without impacting existing tests.Also applies to: 153-155, 213-219, 229-237, 293-295, 481-483, 712-712
src/test/utils/coretest/nc_coretest.js (1)
284-289: NC IAM HTTPS getter aligned with coretest API
get_https_address_iam()correctly exposes the NC IAM HTTPS address and the export matches the containerized coretest naming, allowing the IAM integration test to use a single helper in both environments. Looks good.Also applies to: 557-557
src/util/account_util.js (1)
725-732: ARN construction now robust to owner shape differencesUsing
get_owner_account_id(iam_user)inreturn_list_memberbefore callingcreate_arn_for_useris a nice improvement: it keeps ARN generation correct whetheriam_user.owneris an object with._idor a string owner ID, and avoids sprinkling._id.toString()logic here.Based on learnings, this should work consistently for both containerized (ObjectId owner) and RPC-serialized (string owner) IAM users.
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
96e8b18 to
c8fbb65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (2)
209-213: Clarify skip reason forget access key (last used)testThe current comment and bare
this.skip()make it hard to understand why containerized runs skip this test. Making the intent explicit will help future maintainers and test reports.You could tighten this up as:
- // Test case is wrong, get-access-key-last-used should have - // access key belongs to same account that requesting. - // Skipping for containerized noobaa - if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + // Skip for containerized NooBaa: GetAccessKeyLastUsedCommand requires the + // access key to belong to the requesting account. In NC coretest the admin + // can query other users' keys, but containerized runs cannot. + if (!is_nc_coretest) { + this.skip('requires access key to belong to requesting account'); // eslint-disable-line no-invalid-this + }
48-66: Normalizeaccount_resshape and guard against emptyaccess_keysTwo concerns here:
Shape consistency:
- NC path:
account_res = await generate_nsfs_account(...)- Container path:
account_res = (await rpc_client.account.read_account({ email: EMAIL })).access_keys[0];
Later you readaccount_res.access_key/account_res.secret_key. This assumes both branches produce the same shape (a single access key object, not an account withaccess_keys[]). Please confirmgenerate_nsfs_accountindeed returns an access key record, or normalize both branches to something like a localaccess_key_recordbefore unwrapping.Guarding
access_keys[0](container path):
The container branch still indexesaccess_keys[0]without checking thataccess_keysexists and is non‑empty, so a misconfigured account would throw before tests even start. Consider explicitly asserting that at least one key exists and failing the test with a clear message if not, rather than relying on an unhandled exception.To quickly inspect how
generate_nsfs_accountis used elsewhere and what shape it returns:#!/bin/bash # Show all usages of generate_nsfs_account with surrounding context rg -n "generate_nsfs_account" -C3Also applies to: 68-72
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/endpoint/iam/ops/iam_list_attached_user_policies.js(1 hunks)src/endpoint/iam/ops/iam_list_groups_for_user.js(1 hunks)src/endpoint/iam/ops/iam_list_mfa_devices.js(1 hunks)src/endpoint/iam/ops/iam_list_service_specific_credentials.js(1 hunks)src/endpoint/iam/ops/iam_list_signing_certificates.js(1 hunks)src/endpoint/iam/ops/iam_list_ssh_public_keys.js(1 hunks)src/sdk/accountspace_nb.js(0 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/sdk/accountspace_nb.js
🚧 Files skipped from review as they are similar to previous changes (11)
- src/endpoint/iam/ops/iam_list_signing_certificates.js
- src/test/utils/coretest/nc_coretest.js
- src/util/account_util.js
- src/test/utils/coretest/coretest.js
- src/server/common_services/auth_server.js
- src/test/utils/index/nc_index.js
- src/endpoint/iam/ops/iam_list_mfa_devices.js
- src/endpoint/iam/ops/iam_list_ssh_public_keys.js
- src/endpoint/iam/ops/iam_list_groups_for_user.js
- src/test/utils/index/index.js
- src/server/system_services/account_server.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/api/iam/test_iam_basic_integration.js
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/test/integration_tests/api/iam/test_iam_basic_integration.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/endpoint/iam/ops/iam_list_service_specific_credentials.js
⏰ 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 (3)
src/endpoint/iam/ops/iam_list_attached_user_policies.js (1)
24-24: LGTM! Parameter narrowing improves API clarity.The change to pass only
{ username: params.username }instead of the fullparamsobject makes theget_usercall more explicit about its requirements and avoids passing unnecessary parameters likemarker,max_items, andiam_path_prefix.src/endpoint/iam/ops/iam_list_service_specific_credentials.js (2)
21-21: LGTM! Consistent parameter refinement.The change aligns with the same pattern applied across other IAM operations, passing only the required
usernamefield toget_userinstead of the fullparamsobject containingservice_name.
21-21: No issues found—code is consistent across the codebase.All calls to
account_sdk.get_user()pass an object with ausernameproperty. The reviewed code uses{username: params.username}, which is consistent with other IAM list operations and satisfies the implementation's expectation ofparams.username. The implementation inaccountspace_fs.get_user()accessesparams.usernameand validation confirms this pattern throughout the codebase.
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
c8fbb65 to
8c49947
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/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
75-77: Guard cleanup whenconfig_rootis unset.
config_rootis only initialized whenis_nc_coretestis true, but the cleanup hook always attempts to delete it. In containerized runs, this becomes'undefined', which could delete an unintended folder or cause confusion.Apply this diff to guard the cleanup:
mocha.after(async () => { - fs_utils.folder_delete(`${config_root}`); + if (config_root) { + fs_utils.folder_delete(`${config_root}`); + } });
♻️ Duplicate comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
65-65: Guard against missing or empty access keys.Line 65 accesses
access_keys[0]without verifying the array exists and contains elements. If the account has no access keys, this will throw a runtime error.Apply this diff to add validation:
} else { - account_res = (await rpc_client.account.read_account({ email: EMAIL })).access_keys[0]; + const account = await rpc_client.account.read_account({ email: EMAIL }); + if (!account.access_keys || account.access_keys.length === 0) { + throw new Error(`Account ${EMAIL} has no access keys for IAM testing`); + } + account_res = account.access_keys[0]; }
🧹 Nitpick comments (1)
src/util/account_util.js (1)
345-353: Consider moving helper toiam_utilsfor broader reusability.The
get_owner_account_idhelper correctly handles both object and primitive owner representations. However, a previous review suggested moving this function toiam_utils.jsso it can be reused more broadly across IAM-related code.Based on past review comments...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/endpoint/iam/ops/iam_list_attached_user_policies.js(1 hunks)src/endpoint/iam/ops/iam_list_groups_for_user.js(1 hunks)src/endpoint/iam/ops/iam_list_mfa_devices.js(1 hunks)src/endpoint/iam/ops/iam_list_service_specific_credentials.js(1 hunks)src/endpoint/iam/ops/iam_list_signing_certificates.js(1 hunks)src/endpoint/iam/ops/iam_list_ssh_public_keys.js(1 hunks)src/sdk/accountspace_nb.js(0 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(3 hunks)
💤 Files with no reviewable changes (1)
- src/sdk/accountspace_nb.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/endpoint/iam/ops/iam_list_mfa_devices.js
- src/server/common_services/auth_server.js
- src/endpoint/iam/ops/iam_list_groups_for_user.js
- src/test/utils/index/index.js
- src/test/utils/coretest/coretest.js
- src/test/utils/coretest/nc_coretest.js
- src/endpoint/iam/ops/iam_list_attached_user_policies.js
- src/endpoint/iam/ops/iam_list_service_specific_credentials.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/index/nc_index.jssrc/test/integration_tests/api/iam/test_iam_basic_integration.js
🧠 Learnings (3)
📚 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/util/account_util.jssrc/endpoint/iam/ops/iam_list_signing_certificates.jssrc/server/system_services/account_server.jssrc/endpoint/iam/ops/iam_list_ssh_public_keys.js
📚 Learning: 2025-11-19T15:03:42.260Z
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.260Z
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/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/test/integration_tests/api/iam/test_iam_basic_integration.jssrc/server/system_services/account_server.js
🧬 Code graph analysis (2)
src/util/account_util.js (1)
src/server/system_services/account_server.js (4)
owner_account_id(1281-1281)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)
src/server/system_services/account_server.js (1)
src/util/account_util.js (9)
owner_account_id(341-341)owner_account_id(346-346)owner_account_id(726-726)account(30-42)account(196-196)account(304-304)account(325-325)account(366-366)account(632-632)
⏰ 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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (6)
src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
20-24: Narrowingget_userparams to{ username }looks correct and saferUsing
await req.account_sdk.get_user({ username: params.username });keeps validation on the fullparamsobject while ensuring the SDK call only receives what it actually needs. This reduces coupling to list-specific fields (marker/max_items) and matches the pattern used by the other IAM list ops in this PR. No issues from this change.src/endpoint/iam/ops/iam_list_ssh_public_keys.js (1)
19-23: Consistentget_userusage with username-only payloadAligning this call to
await req.account_sdk.get_user({ username: params.username });is consistent with other IAM list endpoints and with a GET_USER-style API that only requires a username. Validation still receives the fullparamsobject, so input checks are unchanged. Looks good.src/test/utils/index/nc_index.js (1)
28-28: LGTM! Path update aligns with renamed test file.The require path correctly references the renamed IAM integration test file.
src/server/system_services/account_server.js (1)
1281-1283: LGTM! Ownership check now uses the new helper.The refactor to use
get_owner_account_idimproves consistency and handles both object and primitive owner representations. The comparison logic is correct.src/util/account_util.js (1)
725-731: LGTM! Consistent use of the owner account ID helper.The refactor to use
get_owner_account_idimproves consistency when constructing the ARN for IAM users.src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
69-72: LGTM! Proper handling of endpoint and credentials.The dynamic endpoint retrieval and SensitiveString unwrapping logic correctly handles both NC and containerized environments.
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
iam_unit_test
branch
from
8c49947 to
6bdcf23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (2)
40-42: Guard teardown whenconfig_rootis unset to avoid deleting an unintended path.In non‑NC runs
config_rootis never assigned, butmocha.afteralways callsfs_utils.folder_delete(\${config_root}`), which becomes'undefined'and could target an unexpected directory if it exists. Wrap the delete with a truthy check onconfig_root`.mocha.after(async () => { - fs_utils.folder_delete(`${config_root}`); + if (config_root) { + fs_utils.folder_delete(`${config_root}`); + } });Also applies to: 75-77
39-41: Handle emptyaccess_keyswhen bootstrapping IAM credentials in non‑NC runs.For the non‑NC path you assume
access_keys[0]exists:account_res = (await rpc_client.account.read_account({ email: EMAIL })).access_keys[0];If the account has no access keys,
account_resisundefined, and subsequent use ofaccount_res.access_key/account_res.secret_keywill fail or silently build a broken IAM client. Recommend asserting presence or creating an access key explicitly:- } else { - account_res = (await rpc_client.account.read_account({ email: EMAIL })).access_keys[0]; - } + } else { + const account = await rpc_client.account.read_account({ email: EMAIL }); + const keys = account.access_keys || []; + if (!keys.length) { + throw new Error(`Account ${EMAIL} has no access keys for IAM tests; ensure one is created before running this suite`); + } + account_res = keys[0]; + }Also applies to: 65-72
🧹 Nitpick comments (3)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
10-14: Shared NC/containerized setup and IAM client construction look correct.
- Conditional
setup_optionsand NC‑only NSFS setup (config_root, new bucket path, NSFS account) are cleanly gated onis_nc_coretest.- Using
coretest.get_https_address_iam()plusgenerate_iam_client(access_key, secret_key, coretest_endpoint_iam)withSensitiveStringunwrapping provides a consistent IAM client bootstrap across both environments.- Skipping the “get access key (last used)” test for non‑NC via
is_nc_coretestis an acceptable temporary compromise.No additional changes needed around these pieces.
Also applies to: 32-37, 48-63, 68-73, 209-212
src/endpoint/iam/iam_utils.js (1)
819-831:get_owner_account_idcorrectly normalizes owner types across contexts.The helper’s
typeof user_account.owner === 'object' ? String(user_account.owner._id) : user_account.ownermatches the two documented shapes (ObjectIdvs string) used in different code paths and centralizes this logic for reuse. You might optionally add a fast‑path forowner == nullto make misuse on root accounts more explicit, but as currently used for IAM users it’s fine.Based on learnings, this is the right abstraction for consolidating owner handling.
Also applies to: 833-855
src/util/account_util.js (1)
15-16: Centralizing owner‑ID derivation viaget_owner_account_idimproves consistency.Switching
_check_root_account_owns_userandreturn_list_memberto use the sharedget_owner_account_idhelper, and re‑exporting it fromaccount_util, aligns IAM ownership checks and ARN construction across both ObjectId‑backed and string‑backedownerfields. This matches the documented owner representations and avoids future drift between call sites.Based on learnings, this abstraction looks appropriate and safe.
Also applies to: 333-343, 716-723, 756-756
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/iam/ops/iam_list_attached_user_policies.js(1 hunks)src/endpoint/iam/ops/iam_list_groups_for_user.js(1 hunks)src/endpoint/iam/ops/iam_list_mfa_devices.js(1 hunks)src/endpoint/iam/ops/iam_list_service_specific_credentials.js(1 hunks)src/endpoint/iam/ops/iam_list_signing_certificates.js(1 hunks)src/endpoint/iam/ops/iam_list_ssh_public_keys.js(1 hunks)src/sdk/accountspace_nb.js(0 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(3 hunks)src/test/utils/coretest/coretest.js(7 hunks)src/test/utils/coretest/nc_coretest.js(2 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(4 hunks)
💤 Files with no reviewable changes (1)
- src/sdk/accountspace_nb.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/server/common_services/auth_server.js
- src/endpoint/iam/ops/iam_list_service_specific_credentials.js
- src/endpoint/iam/ops/iam_list_ssh_public_keys.js
- src/server/system_services/account_server.js
- src/test/utils/index/index.js
- src/endpoint/iam/ops/iam_list_attached_user_policies.js
- src/endpoint/iam/ops/iam_list_signing_certificates.js
- src/test/utils/coretest/nc_coretest.js
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/utils/coretest/coretest.jssrc/test/integration_tests/api/iam/test_iam_basic_integration.jssrc/test/utils/index/nc_index.js
🧠 Learnings (3)
📚 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/endpoint/iam/iam_utils.jssrc/util/account_util.js
📚 Learning: 2025-11-19T15:03:42.260Z
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.260Z
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/endpoint/iam/iam_utils.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/test/integration_tests/api/iam/test_iam_basic_integration.js
🧬 Code graph analysis (4)
src/endpoint/iam/iam_utils.js (2)
src/server/system_services/account_server.js (1)
owner_account_id(1281-1281)src/util/account_util.js (2)
owner_account_id(341-341)owner_account_id(717-717)
src/test/utils/coretest/coretest.js (1)
src/test/utils/coretest/nc_coretest.js (6)
https_address_iam(55-55)https_port_iam(52-52)https_port(51-51)http_address(53-53)http_port(50-50)https_address(54-54)
src/endpoint/iam/ops/iam_list_mfa_devices.js (3)
src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
params(14-16)src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
params(15-19)src/endpoint/iam/ops/iam_list_user_policies.js (1)
params(14-18)
src/util/account_util.js (2)
src/server/system_services/account_server.js (9)
require(13-13)require(21-21)require(22-22)require(23-23)require(27-28)owner_account_id(1281-1281)iam_path(1224-1224)iam_path(1287-1287)iam_username(1286-1286)src/sdk/accountspace_fs.js (5)
require(9-9)require(11-11)require(12-13)require(17-17)require(19-19)
⏰ 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 (4)
src/endpoint/iam/ops/iam_list_mfa_devices.js (1)
13-22: Narrowedget_userparameters are correct and consistent.Passing only
{ username: params.username }toreq.account_sdk.get_usermatches the GET_USER contract and keeps validation using the fullparamsobject; this is consistent with other IAM list ops.src/endpoint/iam/ops/iam_list_groups_for_user.js (1)
14-21:get_usercall shape matches expected IAM params.Using
req.account_sdk.get_user({ username: params.username })is explicit and consistent with the validation logic and other IAM endpoints.src/test/utils/coretest/coretest.js (1)
63-75: IAM HTTPS test server wiring looks sound and consistent with STS/S3.The added IAM endpoint handler, dynamic HTTPS server startup, address derivation, teardown, and
get_https_address_iamexport all mirror the existing STS pattern and provide a clean test API without introducing obvious lifecycle issues.Also applies to: 147-155, 205-219, 221-237, 259-295, 469-483, 698-713
src/test/utils/index/nc_index.js (1)
27-30: Updated IAM integration test require matches the new shared test file.Switching to
require('../../integration_tests/api/iam/test_iam_basic_integration')correctly aligns NC index wiring with the renamed/shared IAM test.
|
@naveenpaul1 Could you change the title of the PR from “unit test” to “integration tests”? |
2 participants
Describe the Problem
Both NC and containerized Noobaa will share the same IAM unit test.
Explain the Changes
test_nc_iam_basic_integration.jsrenamed totest_iam_basic_integration.jstest_iam_basic_integrationadded to index.jsIssues: Fixed #xxx / Gap #xxx
Testing Instructions:
NC_CORETEST=true ./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.js./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.jsSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.