IAM | CreateUser, UpdateUser - UserName, NewUserName Check
#9323
Conversation
WalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Layer
participant Server as Account Server
participant SDK as SDK Layer
participant Util as Account Util
participant Store as System Store
Client->>API: create_user(username, email, ...)
API->>Server: create_account(rpc_params...)
Server->>Util: _check_username_already_exists(action, email_wrapped, username)
Util->>Store: find account by email (case-insensitive)
alt Email-owned account found
Util-->>Server: throw ENTITY_ALREADY_EXISTS
else No email match
Server->>SDK: create_user(..., username)
SDK->>Store: _check_if_account_exists_under_the_owner(owner, username)
alt Username conflict under owner (case-insensitive)
SDK-->>Server: throw ENTITY_ALREADY_EXISTS
else No conflict
SDK-->>Server: user created
end
end
Server-->>API: success/error
API-->>Client: success/error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/api/account_api.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(1 hunks)src/server/system_services/account_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_advanced_integration.js(1 hunks)src/test/utils/index/index.js(1 hunks)src/test/utils/index/nc_index.js(1 hunks)src/util/account_util.js(1 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/nc_index.jssrc/test/utils/index/index.jssrc/test/integration_tests/api/iam/test_iam_advanced_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/util/account_util.jssrc/sdk/accountspace_fs.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_fs.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (5)
src/sdk/accountspace_nb.js (1)
src/server/system_services/account_server.js (3)
params(328-328)params(691-691)params(955-955)
src/util/account_util.js (2)
src/sdk/accountspace_nb.js (1)
system_store(5-5)src/test/utils/coretest/coretest.js (1)
system_store(49-49)
src/test/utils/index/nc_index.js (2)
src/sdk/accountspace_nb.js (1)
require(6-6)src/util/account_util.js (4)
require(8-8)require(14-14)require(15-15)require(16-17)
src/sdk/accountspace_fs.js (4)
src/server/system_services/account_server.js (2)
owner_account_id(1184-1184)owner_account_id(1292-1292)src/util/account_util.js (2)
owner_account_id(341-341)owner_account_id(723-723)src/endpoint/s3/s3_rest.js (1)
owner_account_id(337-337)src/endpoint/iam/iam_utils.js (2)
owner_account_id(78-78)owner_account_id(851-851)
src/server/system_services/account_server.js (1)
src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
⏰ 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 (9)
src/test/utils/index/index.js (1)
113-114: LGTM!The new IAM advanced integration test is correctly added alongside the existing basic integration test.
src/sdk/accountspace_nb.js (1)
41-53: LGTM!The
usernamefield is correctly added to the request object, aligning with the updated API schema.src/api/account_api.js (1)
679-681: LGTM!The
usernameparameter is correctly added to thecreate_userAPI schema as an optional string field.src/test/utils/index/nc_index.js (1)
28-29: LGTM!The NC test bootstrap correctly includes the new advanced IAM integration test alongside the basic test.
src/test/integration_tests/api/iam/test_iam_advanced_integration.js (2)
30-74: Good test setup with comprehensive case-insensitive username testing.The test suite correctly validates case-insensitive username uniqueness for both CreateUser and UpdateUser operations. The setup/teardown logic properly handles test isolation.
209-215: LGTM!Simple and effective helper function for validating HTTP 200 responses.
src/util/account_util.js (1)
355-369: Case-insensitive username check implementation looks correct.The new
_check_if_account_exists_by_emailhelper properly performs case-insensitive email comparison by usingtoLowerCase()on both sides of the comparison. The null-safety withaccounts || []is a good defensive pattern.src/sdk/accountspace_fs.js (1)
796-807: Verify case-insensitive handling in account name lookups.The code converts username to lowercase before checking existence via
is_account_exists_by_name(), but the implementation details of that method and how account names are stored cannot be confirmed. Ensure this method performs case-insensitive comparison internally or that account names are consistently stored in lowercase. Additionally, verify that other account lookups (e.g.,_check_if_account_config_file_exists) use the same case-insensitive approach to avoid inconsistency.src/server/system_services/account_server.js (1)
42-48: IAM create_user path now cleanly uses explicit username and case‑insensitive uniquenessUsing
req.rpc_params.usernamefor both the root‑account IAM check and ARN construction, while delegating the case‑insensitive uniqueness check toaccount_util._check_username_already_exists, looks correct and aligns with the new API contract. No additional changes needed here.
src/test/integration_tests/api/iam/test_iam_advanced_integration.js
Outdated
Show resolved
Hide resolved
src/test/integration_tests/api/iam/test_iam_advanced_integration.js
Outdated
Show resolved
Hide resolved
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 (2)
src/server/system_services/account_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_advanced_integration.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/integration_tests/api/iam/test_iam_advanced_integration.js
🧰 Additional context used
🧠 Learnings (4)
📚 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.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-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.js
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/server/system_services/account_server.js
🧬 Code graph analysis (1)
src/server/system_services/account_server.js (1)
src/endpoint/iam/iam_constants.js (1)
IAM_ACTIONS(5-23)
⏰ 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 (1)
src/server/system_services/account_server.js (1)
1237-1244: Past issue resolved: guarded computation looks correct.The code now correctly guards
email_new_usernamecomputation and the uniqueness check behind theis_username_updatecondition. This addresses the concern from the previous review about callingget_account_name_from_usernamewithundefinedwhen no username change is requested.
shirady
force-pushed
the
iam-user-name-check
branch
from
bbd49fe to
e837a5d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/integration_tests/api/iam/test_iam_advanced_integration.js (1)
145-165: Consider adding a test for updating to a different user's name.The current UpdateUser tests validate case variants of the same user. For more comprehensive coverage, consider adding a test that creates two users and attempts to update one user's name to match the other user's name.
Example test structure:
mocha.it('update a user to another existing user name should fail', async function() { // Create a second user const secondUser = 'Alice'; await iam_account.send(new CreateUserCommand({ UserName: secondUser })); try { // Try to update first user to second user's name const command = new UpdateUserCommand({ UserName: username, NewUserName: secondUser, }); await iam_account.send(command); assert.fail('update user to existing username - should throw an error'); } catch (err) { assert.equal(err.Error.Code, IamError.EntityAlreadyExists.code); } finally { await iam_account.send(new DeleteUserCommand({ UserName: secondUser })); } });
📜 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_advanced_integration.js(1 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_advanced_integration.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/iam/test_iam_advanced_integration.js (4)
src/sdk/accountspace_nb.js (1)
require(6-6)src/test/utils/index/nc_index.js (1)
coretest(5-5)src/test/utils/index/index.js (1)
coretest(5-5)src/test/system_tests/test_utils.js (2)
is_nc_coretest(48-48)TMP_PATH(23-23)
⏰ 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 (7)
src/test/integration_tests/api/iam/test_iam_advanced_integration.js (7)
1-28: LGTM! Test setup and imports are well-structured.The imports appropriately pull in necessary utilities, AWS SDK commands, and test infrastructure. The conditional setup for NC vs non-NC environments follows established patterns.
33-68: LGTM! Test account and IAM client setup is thorough.The setup correctly handles both NC and non-NC test environments, properly unwraps sensitive credentials, and establishes the IAM client for subsequent tests.
81-143: Good test coverage for case-insensitive username collision in CreateUser.The tests properly verify that usernames are unique in a case-insensitive manner by checking exact match, lowercase, and uppercase variants. The setup/teardown hooks ensure proper test isolation.
145-205: LGTM! UpdateUser tests correctly validate case-insensitive username handling.The tests verify that:
- Updating to the same username succeeds (line 166-174)
- Updating to case variants of the existing username fails with
EntityAlreadyExists(lines 176-204)The previous review comments about incorrect assertion messages have been addressed.
210-216: LGTM! Helper function is well-documented.The
_check_status_code_okhelper provides a clean abstraction for response validation with appropriate JSDoc type annotation.
110-113: Verify error property access pattern matches SDK response structure.The error access pattern
err.Error.Codeassumes a specific error structure. If the SDK throws a differently-structured error, accessingerr.Error.Codecould throw a TypeError. Verify this pattern is consistent with actual error responses from the IAM SDK in this codebase.
70-74: Missingawaiton async cleanup call.
fs_utils.folder_deleteappears to be an async function based on standard Node.js patterns. Withoutawait, the cleanup may not complete before the test process exits, potentially leaving orphaned test directories.mocha.after(async () => { if (is_nc_coretest) { - fs_utils.folder_delete(`${config_root}`); + await fs_utils.folder_delete(`${config_root}`); } });
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
930-955: Avoid coupling advanced suite to prior describe’s setup
iam_accountis used here but only initialized in thebeforehook of the earlier"IAM basic integration tests - happy path"suite. This makes the advanced tests depend on that suite running first; usingdescribe.onlyon this block (or running suites in isolation) will leaveiam_accountundefined.Consider factoring the IAM client setup into a shared helper or a top-level
beforeso each top-level suite can initializeiam_accountindependently.
📜 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(1 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
⏰ 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 (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
936-999: CreateUser case-insensitive duplicate tests look solidThe three CreateUser tests correctly cover:
- duplicate creation with the exact same username
- duplicate creation with lowercase
- duplicate creation with uppercase
all asserting
IamError.EntityAlreadyExists. This aligns well with the PR goal of enforcing case-insensitive uniqueness on user creation and reuses the existing error-checking pattern in this file.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
1018-1078: UpdateUser tests don't validate collision between distinct users.The UpdateUser tests only create a single user (
username= 'Mateo') and then attempt to update it tousername_lowercaseorusername_uppercase. This tests renaming a user to different casings of its own name, not collision with a different existing user.To properly test case-insensitive uniqueness on update, create a second user with a conflicting name and attempt to rename the first user to that name (in different casings). The test should verify that updating user A to a name already used by user B (in any casing) correctly yields
EntityAlreadyExists.This issue was previously flagged in past review comments with detailed fix suggestions.
Suggested fix:
Create a second user in the test setup:
mocha.describe('IAM UpdateUser API', async function() { + const conflicting_username = `${username}_conflict`; + const conflicting_username_lowercase = conflicting_username.toLowerCase(); + const conflicting_username_uppercase = conflicting_username.toUpperCase(); + mocha.before(async () => { - // create a user - const input = { - UserName: username - }; - const command = new CreateUserCommand(input); - const response = await iam_account.send(command); - _check_status_code_ok(response); + // create two users to test collision on update + for (const name of [username, conflicting_username]) { + const input = { UserName: name }; + const command = new CreateUserCommand(input); + const response = await iam_account.send(command); + _check_status_code_ok(response); + } }); mocha.after(async () => { - // delete a user - const input = { - UserName: username - }; - const command = new DeleteUserCommand(input); - const response = await iam_account.send(command); - _check_status_code_ok(response); + // delete both users + for (const name of [username, conflicting_username]) { + const input = { UserName: name }; + const command = new DeleteUserCommand(input); + const response = await iam_account.send(command); + _check_status_code_ok(response); + } });Then update the test cases to use the conflicting username:
mocha.it('update a user with new username that already exists (lower case) should fail', async function() { try { const input = { UserName: username, - NewUserName: username_lowercase, + NewUserName: conflicting_username_lowercase, }; const command = new UpdateUserCommand(input); await iam_account.send(command);mocha.it('update a user with new username that already exists (upper case) should fail', async function() { try { const input = { UserName: username, - NewUserName: username_uppercase, + NewUserName: conflicting_username_uppercase, }; const command = new UpdateUserCommand(input); await iam_account.send(command);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/unit_tests/nsfs/test_accountspace_fs.test.js(2 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.jssrc/test/unit_tests/nsfs/test_accountspace_fs.test.js
🧠 Learnings (1)
📚 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/unit_tests/nsfs/test_accountspace_fs.test.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (2)
src/util/account_util.js (2)
username(391-392)username(424-425)src/endpoint/iam/iam_utils.js (15)
IamError(109-109)IamError(282-282)IamError(508-508)IamError(537-537)IamError(565-565)IamError(572-572)IamError(600-600)IamError(622-622)IamError(651-651)IamError(686-686)IamError(703-703)IamError(718-718)IamError(725-725)IamError(738-738)IamError(742-742)
⏰ 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
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Outdated
Show resolved
Hide resolved
shirady
force-pushed
the
iam-user-name-check
branch
from
bc65ecf to
f7d41a7
Compare
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)
930-1063: Advanced IAM create tests look good; UpdateUser duplicate-username tests still don’t cover a second conflicting userThe new
IAM advanced integration testsnicely validate CreateUser failing for same, lower-case, and upper-case usernames once a baseline user exists, which directly exercises the new case-insensitive uniqueness checks.In the
IAM UpdateUser APIblock, though, you still only ever create a single user (username), and then try to rename that same user tousername_lowercase/username_uppercase. That verifies your current behavior for “case-only rename” of a single user, but it never exercises the more typical “rename user A to a name already used by user B (in any casing)” conflict.If you want tests that validate the full “username already exists under this owner (case-insensitive)” behavior on update, consider:
- Creating a second, clearly distinct user (e.g.,
${username}_conflict) in thebeforehook.- Using its lower/upper variants as
NewUserNamein the two negative tests.- Deleting both users in the
afterhook.This keeps your existing expectations but actually introduces a real second account as the conflicting target.
🧹 Nitpick comments (2)
src/util/account_util.js (1)
355-369: Case-insensitive email lookup looks correct; consider hardening SensitiveString handlingThe new
_check_if_account_exists_by_emailhelper plus_check_username_already_existsgives you the desired case-insensitive uniqueness for IAM usernames (since the username is encoded into the email), and the control flow is clear.To make this a bit more robust against future callers that might pass plain strings instead of
SensitiveString, you could defensively unwrap both sides only when needed:function _check_if_account_exists_by_email(email_wrapped) { const accounts = system_store.data.accounts || []; - return accounts.find(account => - account.email.unwrap().toLowerCase() === email_wrapped.unwrap().toLowerCase() - ); + return accounts.find(account => { + const acc_email = account.email instanceof SensitiveString ? + account.email.unwrap() : account.email; + const req_email = email_wrapped instanceof SensitiveString ? + email_wrapped.unwrap() : email_wrapped; + return acc_email.toLowerCase() === req_email.toLowerCase(); + }); }If you’re certain everything here is always a
SensitiveString, feel free to treat this as a future-proofing suggestion rather than a blocker.src/sdk/accountspace_fs.js (1)
797-825: Username uniqueness logic is correct; watch for scalability of the owner-scope scanThe updated
_check_username_already_exists+_check_if_account_exists_under_the_ownercorrectly enforce:
- Direct conflicts via
is_account_exists_by_name, and- Case-insensitive conflicts under the relevant owner (IAM users under a root account, or root accounts under a root-accounts manager),
with a single, shared error path in
_throw_error_if_account_already_exists.One thing to keep in mind is that
_check_if_account_exists_under_the_ownercalls_list_config_files_for_usersand effectively walks all users/accounts under that owner on every check. For large deployments this could become a noticeable extra cost on CreateUser/UpdateUser.If this ever shows up in profiles, a follow‑up refactor like adding a case-insensitive lookup helper in
ConfigFS(or short‑circuiting onsome()over a cheaper list of names) would reduce the amount of config I/O here, but this is not a blocker for this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/api/account_api.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(1 hunks)src/server/system_services/account_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/system_services/account_server.js
- src/api/account_api.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 (2)
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_fs.jssrc/sdk/accountspace_nb.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/sdk/accountspace_fs.jssrc/util/account_util.js
🧬 Code graph analysis (4)
src/sdk/accountspace_fs.js (4)
src/server/system_services/account_server.js (2)
owner_account_id(1184-1184)owner_account_id(1297-1297)src/util/account_util.js (3)
owner_account_id(341-341)owner_account_id(723-723)dbg(7-7)src/endpoint/iam/iam_rest.js (2)
dbg(4-4)IamError(5-5)src/sdk/config_fs.js (1)
dbg(11-11)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (4)
src/server/system_services/account_server.js (6)
username(1210-1210)username(1277-1277)username(1398-1398)username(1432-1432)username(1474-1474)username(1503-1503)src/endpoint/iam/ops/iam_list_signing_certificates.js (1)
username(28-28)src/sdk/accountspace_fs.js (1)
IamError(14-14)src/endpoint/iam/iam_utils.js (15)
IamError(109-109)IamError(282-282)IamError(508-508)IamError(537-537)IamError(565-565)IamError(572-572)IamError(600-600)IamError(622-622)IamError(651-651)IamError(686-686)IamError(703-703)IamError(718-718)IamError(725-725)IamError(738-738)IamError(742-742)
src/util/account_util.js (2)
src/server/system_services/account_server.js (10)
account(69-69)account(117-117)account(136-136)account(158-158)account(206-206)account(329-329)account(692-692)account(956-956)accounts(467-479)system_store(17-17)src/sdk/accountspace_nb.js (1)
system_store(5-5)
src/sdk/accountspace_nb.js (1)
src/server/system_services/account_server.js (3)
params(328-328)params(691-691)params(955-955)
⏰ 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/sdk/accountspace_nb.js (1)
41-53: Propagatingusernameinto the create_user RPC looks correctIncluding
username: params.usernamein the request keeps the NB SDK in sync with the server’s new username-aware create_user flow and doesn’t change existing behavior for other fields. No issues from this change alone.
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 (2)
src/sdk/accountspace_fs.js(1 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(2 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 (1)
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_fs.js
🧬 Code graph analysis (1)
src/sdk/accountspace_fs.js (3)
src/util/account_util.js (6)
owner_account_id(341-341)owner_account_id(723-723)username(391-392)username(424-425)message_with_details(328-328)message_with_details(359-359)src/server/system_services/account_server.js (8)
owner_account_id(1184-1184)owner_account_id(1297-1297)username(1210-1210)username(1277-1277)username(1398-1398)username(1432-1432)username(1474-1474)username(1503-1503)src/endpoint/iam/iam_utils.js (16)
owner_account_id(78-78)owner_account_id(851-851)message_with_details(87-87)message_with_details(107-108)message_with_details(281-281)message_with_details(506-507)message_with_details(536-536)message_with_details(563-564)message_with_details(570-571)message_with_details(598-599)message_with_details(619-621)message_with_details(652-652)message_with_details(684-685)message_with_details(704-704)message_with_details(719-719)message_with_details(726-726)
⏰ 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 (5)
src/sdk/accountspace_fs.js (1)
810-815: Good refactoring to centralize error handling.Extracting this helper consolidates the EntityAlreadyExists error logic and ensures consistent error messages. The message format aligns with similar patterns elsewhere in the codebase.
src/test/integration_tests/api/iam/test_iam_basic_integration.js (4)
45-86: LGTM: Test suite restructuring improves organization.The consolidated top-level setup (before/after hooks) and nested describe blocks properly separate basic happy-path tests from advanced collision tests. The IAM client initialization is shared across both suites.
944-994: LGTM: CreateUser collision tests correctly validate case-insensitive uniqueness.The tests create a base user
'Mateo'and correctly verify that attempts to create'Mateo','mateo', or'MATEO'all fail withEntityAlreadyExists. Setup and teardown properly isolate each test run.
996-1048: LGTM: UpdateUser collision tests now correctly validate case-insensitive uniqueness.The tests now create two distinct users (
'Mateo'and'Leonardo'), addressing the previous review concern. Attempting to rename'Leonardo'to'mateo'or'MATEO'correctly fails due to collision with the existing'Mateo'user. The no-op rename test (line 1009) also ensures that updating a user to its own name succeeds.
1066-1087: LGTM: Helper functions reduce duplication in test setup/teardown.Extracting
create_iam_useranddelete_iam_userimproves maintainability by consolidating the common before/after hook logic used across multiple test suites.
shirady
force-pushed
the
iam-user-name-check
branch
2 times, most recently
from
c7e475b to
8603428
Compare
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-user-name-check
branch
from
8603428 to
0dbdccc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sdk/accountspace_fs.js (2)
796-808: Username existence check logic matches desired semantics but assumesparams.usernameis always setThe two-step check (owner-scoped
is_account_exists_by_name+ case-insensitive scan under the same owner) correctly enforces per-owner case-insensitive uniqueness for both root accounts and IAM users, and centralizing the error via_throw_error_if_account_already_existskeeps behavior uniform.This helper, however, implicitly relies on
params.usernamebeing a non-empty string; if a future caller passesparams.usernameasundefinedor'', the follow-up helper will break (seetoLowerCasecall below). Consider adding a short guard here (e.g., validateusernameand fail fast with a clear error, or derive it fromparams.namewhere appropriate) to make misuse fail predictably.
817-824: Defensive check and lightweight alternative for_check_if_account_exists_under_the_ownerFunctionally this correctly reuses
_list_config_files_for_usersso the owner scope is identical to other flows (root manager → all root accounts, root account → IAM users under that account), and the case-insensitive comparison is straightforward.Two small improvements to consider:
Defensive guard on
username
Callingusername.toLowerCase()will throw ifusernameis everundefinedor not a string. A simple early return or explicit validation would make this helper safer if additional callers are added.Avoid re-reading full account configs when only names are needed
Since you only usemember.username, you could instead use the underlying name lists directly (config_fs.list_accounts()/config_fs.list_users_under_account()with a simplesome(name => name.toLowerCase() === username.toLowerCase())) and avoidget_account_or_user_by_namefor every entry. That would reduce I/O for owners with many users, without changing behavior.Both are non-blocking but would harden and streamline this helper. Based on learnings, the current owner scoping is already correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/api/account_api.js(1 hunks)src/sdk/accountspace_fs.js(1 hunks)src/sdk/accountspace_nb.js(1 hunks)src/server/system_services/account_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(4 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/sdk/accountspace_nb.js
- src/util/account_util.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)
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_fs.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/sdk/accountspace_fs.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_fs.jssrc/test/integration_tests/api/iam/test_iam_basic_integration.js
🧬 Code graph analysis (1)
src/sdk/accountspace_fs.js (2)
src/server/system_services/account_server.js (24)
owner_account_id(1184-1184)owner_account_id(1297-1297)requesting_account(1208-1208)requesting_account(1228-1228)requesting_account(1276-1276)requesting_account(1293-1293)requesting_account(1321-1321)requesting_account(1352-1352)requesting_account(1365-1365)requesting_account(1393-1393)requesting_account(1410-1410)requesting_account(1431-1431)requesting_account(1473-1473)requesting_account(1502-1502)requesting_account(1532-1532)requesting_account(1561-1561)requesting_account(1575-1575)requesting_account(1594-1594)username(1210-1210)username(1277-1277)username(1398-1398)username(1432-1432)username(1474-1474)username(1503-1503)src/util/account_util.js (4)
owner_account_id(341-341)owner_account_id(723-723)username(391-392)username(424-425)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ceph-s3-tests / Ceph S3 Tests
🔇 Additional comments (5)
src/api/account_api.js (1)
679-681: CreateUserusernamefield wiring looks consistent with existing IAM schemaAdding
usernameas a simple string property matches how other IAM methods in this API modelusernameand keeps it optional as before; server-side validation can enforce any stricter rules. No issues from the schema side.src/test/integration_tests/api/iam/test_iam_basic_integration.js (4)
45-79: IAM client setup and credential unwrapping look correctThe test setup cleanly handles both NC and containerized modes and unwraps
SensitiveStringfor access/secret keys before constructing the IAM client, avoiding type surprises in the AWS SDK calls. No issues here.
87-933: Broad “happy path” IAM coverage is thorough and consistentThe reorganized “IAM basic integration tests – happy path” block exercises end‑to‑end flows for users, access keys, inline user policies, tags, and a wide range of list* IAM APIs (including both empty and NoSuchEntity cases). The patterns (create → verify via list/get → delete → verify/cleanup) are clear and consistent, and resource lifecycle cleanup is handled via
before/afterhooks. This gives solid regression coverage for existing behavior.
936-1050: Advanced CreateUser/UpdateUser tests correctly validate case-insensitive username uniquenessThe advanced suite now:
- Creates a baseline user (
username) and verifies that:
- Re-creating with the same casing fails with
EntityAlreadyExists.- Creating with lower/upper-case variants also fails, confirming case-insensitive behavior on create.
- For UpdateUser, creates two distinct users (
usernameandusername2), then:
- Confirms a no-op rename (
username2 → username2) succeeds.- Asserts that renaming
username2tousernamein lower/upper-case variants fails withEntityAlreadyExists.This directly exercises the new collision logic for both create and update while still allowing no-op updates, aligning with the stated behavior (and acknowledging the documented case-only rename gap). Cleanup of both users in
afterkeeps the suite idempotent.
1061-1087: Helper functions for IAM user create/delete improve test reuse
create_iam_useranddelete_iam_usercentralize the IAM CreateUser/DeleteUser commands and status checks, reducing duplication in the advanced tests and making the before/after hooks more readable. This is a good test-only abstraction and keeps IAM client usage consistent across suites. As per coding guidelines, these helpers support the added tests for the new username behavior.
shirady
changed the title
CreateUser, UpdateUser - UserName, NewUserName Check
| const is_username_update = req.rpc_params.new_username !== undefined && | ||
| req.rpc_params.new_username !== req.rpc_params.username; | ||
| if (is_username_update) { | ||
| const email_new_username_wrapped = account_util.get_account_name_from_username( |
There was a problem hiding this comment.
email_new_username_wrapped this name kind of confusing
There was a problem hiding this comment.
It is the email field of the new username suggested, and it is wrapped in a Sensitive string.
Do you have another variable name to suggest?
There was a problem hiding this comment.
may be new_email_wrapped or new_username_wrapped?
| } | ||
| } | ||
|
|
||
| function _check_if_account_exists_by_email(email_wrapped) { |
There was a problem hiding this comment.
One other solution we have is save actual customer saving username in user name field in DB and lowercase value in email, and when fetch the username get it from name and for get account we can still use the get_account_by_email, WDYT?
There was a problem hiding this comment.
In this suggestion, we will still need to iterate over the accounts, right?
This suggestion seems to refactor the code base, it can be done, but I'm not sure it is needed as part of this fix.
There was a problem hiding this comment.
There is shouldnt be any iteration, We will be using the get_account_by_email to get the account. And get the account directly, Where do you see the iteration?
2 participants
Describe the Problem
Users name should be unique within the account, they aren’t distinguished by case.
Explain the Changes
usernamein thecreate_userAPI.Issues:
I left a GAP - where a trying to update the username with the same name case case-insensitive , currently it will throw an error, but AWS also throws one (can fix it up in the future):
I created a user with user name shira, and wanted to change its name to SHIRA (there is no SHIRA user under the account).
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
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.