IAM | Remove Unneeded system_store Calls - Part 2

[shirady]

Describe the Problem

Currently, there are many calls to system_store in functions related to IAM.
Note: Part 1 was in PR #9330.

Explain the Changes

1. Reduce the calls to system_store, mainly where it was to get the requested_account and the use of the function _check_if_account_exists.

Issues:

List of GAPs:
I think we need to move the create_user inside the account_server so we would have the req.account access and keep this file with small functionality.
Planned to work on the files: account_utils as well.
There is code duplication that I found; probably we can reduce more in the future.

Testing Instructions:

Containerized

1. Use the guide "Run Tests From coretest Locally" (link) for running the core tests.
2. Run first tab: docker run -p 5432:5432 -e POSTGRESQL_ADMIN_PASSWORD=noobaa quay.io/sclorg/postgresql-15-c9s
3. Run second tab: NOOBAA_LOG_LEVEL=all ./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.js

• Doc added/updated
• Tests added

Summary by CodeRabbit

• Refactor
  • Centralized account resolution used across user management actions (update, delete, tag/untag, list tags).
  • Account validation helper now returns the resolved account object for downstream use.
  • Validation and error flows for account operations now follow the centralized helper, improving consistency of responses.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

This PR centralizes account resolution by routing IAM handlers to account_util helpers. _check_if_account_exists() now returns the found account object; validate_and_return_requested_account() uses that return. Multiple account handlers in the account server were updated to use these helpers instead of direct system_store email lookups.

Changes

Cohort / File(s)	Summary
Utility Functions Refactoring src/util/account_util.js	_check_if_account_exists() now returns the found account object; validate_and_return_requested_account() assigns requested_account from _check_if_account_exists() instead of performing a separate system_store lookup.
Account Server Handler Updates src/server/system_services/account_server.js	Updated handlers (update_user, delete_user, tag_user, untag_user, list_user_tags) to resolve target accounts via account_util.validate_and_return_requested_account() or _check_if_account_exists() instead of direct system_store.get_account_by_email() calls; update_user now uses req.account as requesting_account.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify _check_if_account_exists() return is properly handled (null/undefined checks) across callers.
• Confirm error/messages and authorization behavior remain consistent after replacing direct system_store lookups.
• Inspect update_user change to req.account for multi-tenant or privilege implications.

Possibly related PRs

• Added a fix for authentication of root and iam user account #9331 — touches validate_and_return_requested_account() and _check_if_account_exists() with similar account resolution changes.
• IAM | Account Server - Block Account Deletion in Case the Account Has Users #9321 — modifies account deletion flow; related to delete_user logic updates.
• IAM | Use the account server flow #9289 — refactors account resolution helpers and server handlers overlapping these files.

Suggested reviewers

• aayushchouhan09
• naveenpaul1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically summarizes the main objective: removing unnecessary system_store calls in IAM code as a continuation of prior work (Part 2).

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1f5f320 and 01cc125.

📒 Files selected for processing (2)

• src/server/system_services/account_server.js (5 hunks)
• src/util/account_util.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/server/system_services/account_server.js

🧰 Additional context used

🧠 Learnings (3)

📚 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

📚 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/util/account_util.js

🧬 Code graph analysis (1)

src/util/account_util.js (1)

src/server/system_services/account_server.js (42)

• account (69-69)
• account (117-117)
• account (136-136)
• account (158-158)
• account (206-206)
• account (329-329)
• account (692-692)
• account (956-956)
• requested_account (1209-1209)
• requested_account (1235-1235)
• requested_account (1281-1281)
• requested_account (1323-1323)
• requested_account (1354-1354)
• requested_account (1367-1367)
• requested_account (1396-1397)
• requested_account (1414-1414)
• requested_account (1434-1434)
• requested_account (1468-1468)
• requested_account (1489-1489)
• requested_account (1504-1504)
• requested_account (1532-1532)
• requested_account (1546-1546)
• requested_account (1565-1565)
• action (42-42)
• action (1207-1207)
• action (1229-1229)
• action (1277-1277)
• action (1293-1293)
• action (1321-1321)
• action (1352-1352)
• action (1364-1364)
• action (1393-1393)
• action (1410-1410)
• action (1432-1432)
• action (1466-1466)
• action (1487-1487)
• action (1501-1501)
• action (1529-1529)
• action (1543-1543)
• 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: run-jest-unit-tests
• GitHub Check: Build Noobaa Image
• GitHub Check: run-package-lock-validation

🔇 Additional comments (2)

src/util/account_util.js (2)

325-333: Returning the account from _check_if_account_exists is a safe contract extension

Having _check_if_account_exists return the account object on success avoids duplicate store lookups and keeps existing callers (that ignore the return) behaving the same, since the success path was already guaranteed and the new value is truthy. This aligns with other places that consume system_store-backed account objects, so types for _id/owner remain consistent. Based on learnings, this is compatible with how account_server/auth_server load accounts from system_store.

---

722-747: Good reuse of _check_if_account_exists in validate_and_return_requested_account

Using _check_if_account_exists’s return value here removes the redundant get_account_by_email call while preserving the same error behavior and follow-up checks (_check_if_requested_account_is_root_account_or_IAM_user, _check_if_requested_is_owned_by_root_account). The account_email construction and ownership checks still operate on system_store account objects as before, so IAM semantics for self vs other-user operations remain unchanged.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}