IAM | Add Test Cases on IAM API - Part 1 #9345
Conversation
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
WalkthroughCentralizes requested-account resolution by adding validate_and_return_requested_account_with_option_itself and validate_and_return_requested_account in src/util/account_util.js, updates src/server/system_services/account_server.js to use them, and adjusts IAM integration tests to pass explicit IAM clients. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AccountServer
participant AccountUtil
participant SystemStore
Note over AccountUtil,SystemStore: centralized account resolution/validation
Client->>AccountServer: API request (user/access-key operations)
AccountServer->>AccountUtil: validate_and_return_requested_account_with_option_itself(params, action, requesting_account)
alt requester is IAM user operating on itself
AccountUtil-->>AccountServer: resolved requested_account (self path)
else normal resolution
AccountUtil->>SystemStore: fetch account / ownership info
SystemStore-->>AccountUtil: account record / ownership
AccountUtil-->>AccountServer: resolved requested_account
end
AccountServer->>SystemStore: perform user/access-key operation
SystemStore-->>AccountServer: operation result
AccountServer-->>Client: API response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/system_services/account_server.js(9 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(10 hunks)src/util/account_util.js(9 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/integration_tests/api/iam/test_iam_basic_integration.js
🧠 Learnings (9)
📓 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/test/integration_tests/api/iam/test_iam_basic_integration.jssrc/server/system_services/account_server.jssrc/util/account_util.js
📚 Learning: 2025-12-01T13:35:12.469Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9327
File: src/endpoint/iam/ops/iam_get_user.js:32-32
Timestamp: 2025-12-01T13:35:12.469Z
Learning: In AWS IAM GetUser API responses, the Tags field is omitted entirely when a user has no tags. The field only appears in the response after a user has been tagged, and disappears again when all tags are removed. This differs from some AWS APIs that return empty arrays for absent collections.
Applied to files:
src/test/integration_tests/api/iam/test_iam_basic_integration.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/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/server/system_services/account_server.js
📚 Learning: 2025-12-04T10:55:08.659Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.659Z
Learning: In src/sdk/accountspace_nb.js, account_sdk.requesting_account comes from RPC serialization, so requesting_account._id is already a string (not an ObjectId). This differs from system_store.get_account_by_email() which returns accounts with ObjectId._id references. Therefore, no .toString() conversion is needed when using account_sdk.requesting_account._id in accountspace_nb.js.
Applied to files:
src/server/system_services/account_server.jssrc/util/account_util.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/server/system_services/account_server.js
📚 Learning: 2025-08-08T13:13:42.192Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.192Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/server/system_services/account_server.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
src/endpoint/iam/iam_constants.js (1)
ACCESS_KEY_STATUS_ENUM(55-58)
⏰ 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 (11)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (4)
432-440: LGTM! Good verification of tag propagation.This additional verification ensures that tags are properly reflected in the User object returned by GetUser, which is consistent with AWS IAM behavior.
1011-1024: LGTM! Comprehensive test coverage for IAM user access control.These test cases properly verify that IAM users cannot perform user management operations (CreateUser, GetUser, UpdateUser, DeleteUser) on other users, which aligns with the implicit policy model described in the PR objectives where only root accounts can manage users.
Also applies to: 1055-1067, 1144-1157, 1188-1200
1226-1238: LGTM! Proper validation of self-service access key operations.These test cases correctly verify the implicit policy allowing IAM users to manage their own access keys (create, list, update, delete), which aligns with the PR objectives. The tests properly use
iam_user_clientto simulate IAM user actions.Also applies to: 1271-1282, 1302-1327, 1346-1366, 1383-1394
1411-1471: LGTM! Good refactoring for test flexibility.The helper function signature changes to accept an explicit
iam_clientparameter improve testability and make it clear which credentials are being used for each operation. This supports testing both root account and IAM user scenarios.src/util/account_util.js (3)
395-410: LGTM! Clearer documentation and correct logic.The updated comments clearly explain the behavior of
_returned_usernamefor different scenarios (root accounts manager, root account on itself, IAM user). The logic correctly returnsundefinedfor cases where username should not be included.
748-755: LGTM! Good consolidation of validation logic.The
validate_and_return_requested_accountfunction properly centralizes validation for operations on other users, reducing code duplication and making the logic easier to maintain. The function correctly delegates to existing helper functions for each validation step.
723-746: No action required. The behavior of blocking root accounts from using AccessKey APIs without an explicit username parameter is intentional and documented. The "implicit policy" is specifically for IAM users to manage their own resources (lines 723-724), while root accounts must explicitly specify a username. This differs from AWS behavior but is a documented design choice in NooBaa.src/server/system_services/account_server.js (4)
1202-1221: LGTM! Proper consolidation of validation logic.The
get_userfunction correctly uses the newvalidate_and_return_requested_accounthelper, which centralizes the validation logic and makes the code cleaner. The removal of inline validation checks in favor of the consolidated utility function improves maintainability.
1223-1280: LGTM! Consistent validation pattern across user operations.Both
update_useranddelete_usercorrectly adopt the newvalidate_and_return_requested_accounthelper, ensuring consistent validation logic across all user management operations.
1310-1340: LGTM! Proper implementation of implicit policy for access key operations.The access key APIs (
create_access_key,list_access_keys,update_access_key,delete_access_key) correctly usevalidate_and_return_requested_account_with_option_itself, which implements the implicit policy allowing IAM users to manage their own access keys as described in the PR objectives.Also applies to: 1342-1355, 1358-1396, 1415-1445
1342-1355: LGTM! Improved consistency in list response format.The
list_access_keysfunction now returns a properly structured object withmembers,is_truncated, andusernamefields, providing consistency with other list operations. The sorting of members ensures deterministic output for testing and client consumption.
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
432-440: Good coverage ofGetUsertags behaviorVerifying that
GetUserreturns the expectedTagsafter a priorTagUseraligns with AWS IAM semantics (field is only present when tags exist) and complements the earlierListUserTagsassertions. Based on learnings, this is exactly the right place to assert onUser.Tags.
🧹 Nitpick comments (3)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (3)
952-1200: Advanced IAM User API tests thoroughly exercise negative and IAM‑requester flowsThe new advanced tests for Create/Get/Update/DeleteUser:
- Correctly set up per‑user IAM credentials via
create_iam_user+create_access_key_iam_user+generate_iam_client.- Cover non‑existing user cases with
IamError.NoSuchEntityand IAM‑requester attempts withIamError.AccessDeniedException, using the more defensiveerr.Error?.Code || err.Codewhere shape can vary.- Keep state reasonably isolated with dedicated
before/afterhooks per describe block.If you find yourself adding more of these patterns, consider a small helper like
setup_iam_user_with_client(username)returning{iam_user_client, access_key_id}to reduce duplication across the four IAM User API describes, but this is optional.
1206-1394: Tighten after‑hook cleanup around secondary access keysThe advanced AccessKey API tests look good functionally and match the implicit policy (IAM user managing its own access keys). Two minor resilience nits around cleanup:
In the
IAM CreateAccessKey APIdescribe, theafterhook always calls:await delete_access_key_iam_user(iam_account, access_key_id_2, username);If the test fails before
access_key_id_2is assigned, this will issue aDeleteAccessKeywithAccessKeyIdundefined, and the_check_status_code_okin the helper will then blow up in teardown. A small guard avoids noisy follow‑up failures:mocha.after(async () => { await delete_access_key_iam_user(iam_account, access_key_id, username);
await delete_access_key_iam_user(iam_account, access_key_id_2, username);
if (access_key_id_2) {await delete_access_key_iam_user(iam_account, access_key_id_2, username);} await delete_iam_user(iam_account, username);
- In the
IAM DeleteAccessKey APIdescribe, theafterhook only deletesaccess_key_id, relying on the test body to deleteaccess_key_id_2. If the test fails before that delete, the user deletion will fail because of the remaining key. Mirroring the pattern above (guarded delete ofaccess_key_id_2inafter) would make teardown more robust when assertions fail.Neither affects correctness when tests pass, but they will make failures less noisy and cleanup more reliable.
1410-1470: Helper functions generalized to accept an IAM client improve reuseRefactoring
create_iam_user,delete_iam_user,create_access_key_iam_user, anddelete_access_key_iam_userto take an explicitiam_clientparameter is a good move: it lets you reuse them for bothiam_accountand potential future IAM‑user clients while keeping the call sites concise.If you later start using these helpers with non‑user principals, you might consider slightly more generic names (e.g.,
create_iam_user_with_client) to reflect the broader usage, but that’s a pure naming nit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js(10 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/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.
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.659Z
Learning: In src/sdk/accountspace_nb.js, account_sdk.requesting_account comes from RPC serialization, so requesting_account._id is already a string (not an ObjectId). This differs from system_store.get_account_by_email() which returns accounts with ObjectId._id references. Therefore, no .toString() conversion is needed when using account_sdk.requesting_account._id in accountspace_nb.js.
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.
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.
📚 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/test/integration_tests/api/iam/test_iam_basic_integration.js
📚 Learning: 2025-12-01T13:35:12.469Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9327
File: src/endpoint/iam/ops/iam_get_user.js:32-32
Timestamp: 2025-12-01T13:35:12.469Z
Learning: In AWS IAM GetUser API responses, the Tags field is omitted entirely when a user has no tags. The field only appears in the response after a user has been tagged, and disappears again when all tags are removed. This differs from some AWS APIs that return empty arrays for absent collections.
Applied to files:
src/test/integration_tests/api/iam/test_iam_basic_integration.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (3)
src/test/utils/index/index.js (1)
coretest(5-5)src/test/utils/index/nc_index.js (1)
coretest(5-5)src/endpoint/iam/iam_constants.js (1)
ACCESS_KEY_STATUS_ENUM(55-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
13-18: IAM endpoint wiring and IAM client setup look consistentUsing
require_coretest,is_nc_coretest, and cachingcoretest_endpoint_iam = coretest.get_https_address_iam()before constructingiam_accountandiam_user_clientkeeps all IAM traffic pointed at the same endpoint and matches the existing coretest pattern. No issues spotted here.Also applies to: 44-45, 71-72
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util/account_util.js (1)
740-742: Consider improving the error message for root account self-operations.The code correctly blocks root accounts from performing IAM actions on themselves (line 740-742), which aligns with AWS IAM behavior. However, the generic error message
'You do not have permission to perform this action.'doesn't explain why a root account is blocked from self-operations.Consider making the error message more specific to help users understand that root accounts should use different mechanisms (not IAM APIs) to manage their own credentials.
- if (requesting_account.owner === undefined) { - throw new RpcError('NOT_AUTHORIZED', 'You do not have permission to perform this action.'); - } + if (requesting_account.owner === undefined) { + throw new RpcError('NOT_AUTHORIZED', 'Root accounts cannot perform IAM actions on themselves. Use account management APIs instead.'); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util/account_util.js(10 hunks)
🧰 Additional context used
🧠 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.
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.659Z
Learning: In src/sdk/accountspace_nb.js, account_sdk.requesting_account comes from RPC serialization, so requesting_account._id is already a string (not an ObjectId). This differs from system_store.get_account_by_email() which returns accounts with ObjectId._id references. Therefore, no .toString() conversion is needed when using account_sdk.requesting_account._id in accountspace_nb.js.
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.
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.
📚 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.js
📚 Learning: 2025-12-04T10:55:08.659Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.659Z
Learning: In src/sdk/accountspace_nb.js, account_sdk.requesting_account comes from RPC serialization, so requesting_account._id is already a string (not an ObjectId). This differs from system_store.get_account_by_email() which returns accounts with ObjectId._id references. Therefore, no .toString() conversion is needed when using account_sdk.requesting_account._id in accountspace_nb.js.
Applied to files:
src/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/util/account_util.js
🧬 Code graph analysis (1)
src/util/account_util.js (2)
src/server/system_services/account_server.js (52)
requested_account(1206-1206)requested_account(1226-1226)requested_account(1277-1277)requested_account(1313-1314)requested_account(1345-1346)requested_account(1362-1363)requested_account(1402-1403)requested_account(1419-1420)requested_account(1450-1450)requested_account(1484-1484)requested_account(1505-1505)requested_account(1520-1520)requested_account(1548-1548)requested_account(1562-1562)requested_account(1581-1581)action(42-42)action(1204-1204)action(1224-1224)action(1275-1275)action(1283-1283)action(1311-1311)action(1343-1343)action(1359-1359)action(1399-1399)action(1416-1416)action(1448-1448)action(1482-1482)action(1503-1503)action(1517-1517)action(1545-1545)action(1559-1559)requesting_account(1205-1205)requesting_account(1225-1225)requesting_account(1276-1276)requesting_account(1284-1284)requesting_account(1312-1312)requesting_account(1344-1344)requesting_account(1361-1361)requesting_account(1400-1400)requesting_account(1418-1418)requesting_account(1449-1449)requesting_account(1483-1483)requesting_account(1504-1504)requesting_account(1518-1518)requesting_account(1547-1547)requesting_account(1561-1561)requesting_account(1580-1580)params(328-328)params(691-691)params(955-955)req(134-134)req(396-396)src/endpoint/iam/iam_constants.js (2)
IAM_DEFAULT_PATH(69-69)MAX_NUMBER_OF_ACCESS_KEYS(68-68)
⏰ 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 (3)
src/util/account_util.js (3)
536-544: LGTM! Centralized owner ID extraction.The refactoring to use
get_owner_account_id(requesting_account)instead of directly accessingrequesting_account.owner._idimproves consistency. Based on learnings, this handles both ObjectId references (from system_store) and string values (from RPC serialization) correctly.
749-756: LGTM! Well-structured validation function.The new
validate_and_return_requested_accountfunction effectively centralizes the validation workflow for resolving target accounts. The use ofrequesting_account._id.toString()at line 751 correctly handles both ObjectId (from system_store) and string (from RPC serialization) cases.The function provides clear validation steps:
- Verifies requesting account is root
- Resolves target account by username
- Validates target account type and ownership
813-814: LGTM! New validation functions properly exported.Both new validation functions are correctly exported and are being used by
account_server.jsas shown in the relevant code snippets. This completes the refactoring to centralize account validation logic.
naveenpaul1
left a comment
naveenpaul1
left a comment
There was a problem hiding this comment.
Dose have so much changes, Since its only a refector changes, LGTM
2 participants
Describe the Problem
Add more test cases to IAM API, this adds if focuses mainly on:
(1) Root account is the requester, and the IAM user who tries to run upon does not exist.
(2) The IAM user is the requester - should succeed the access keys API (implicit policy).
Note:
In NC, it is (link):
Explain the Changes
iam_user_clientto run the actions from.account_util.jsrename the functionvalidate_and_return_requested_accounttovalidate_and_return_requested_account_with_option_itself, and move the part where it is on another user to the functionvalidate_and_return_requested_account. Therefore, there will be less places to search for the implicit policy.account_serverusevalidate_and_return_requested_account_with_option_itselfon access key API with username param, and the rest of the API where we can usevalidate_and_return_requested_account.Issues:
Testing Instructions:
Automatic Test
Containerized
docker run -p 5432:5432 -e POSTGRESQL_ADMIN_PASSWORD=noobaa quay.io/sclorg/postgresql-15-c9sNOOBAA_LOG_LEVEL=all ./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.jsNC:
sudo NC_CORETEST=true ./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.jssudo npx jest test_accountspace_fs.test.js(unit tests)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.