IAM | Remove Unneeded system_store Calls - Part 1
#9330
Conversation
WalkthroughReplaced in-file account lookups with direct use of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-12-04T10:55:08.659ZApplied to files:
📚 Learning: 2025-11-19T15:03:42.260ZApplied to files:
📚 Learning: 2025-11-13T07:56:23.620ZApplied to files:
📚 Learning: 2025-11-12T04:55:42.193ZApplied to files:
📚 Learning: 2025-08-08T13:08:38.361ZApplied to files:
📚 Learning: 2025-08-08T13:12:46.728ZApplied to files:
📚 Learning: 2025-11-18T07:00:17.653ZApplied to files:
🧬 Code graph analysis (1)src/sdk/accountspace_nb.js (2)
⏰ 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)
🔇 Additional comments (5)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/accountspace_nb.js (1)
36-62: Bug risk:default_resource.nameno longer valid after removing system_store lookupNow that
requesting_accountcomes directly fromaccount_sdk.requesting_account(the RPC-serialized account),requesting_account.default_resourceis already a string (pool name) rather than a pool object. Using.namehere will yieldundefinedand either breakCreateUseror cause the server to fall back to an unintended default pool.Consider updating the request construction to pass the name directly:
- const req = { - name: account_name, - email: account_name, - has_login: false, - s3_access: true, - allow_bucket_creation: true, - owner: requesting_account._id.toString(), - iam_path: params.iam_path, - roles: ['admin'], - // TODO: default_resource remove - default_resource: requesting_account.default_resource.name, - }; + const req = { + name: account_name, + email: account_name, + has_login: false, + s3_access: true, + allow_bucket_creation: true, + owner: requesting_account._id.toString(), + iam_path: params.iam_path, + roles: ['admin'], + // `requesting_account.default_resource` is already the pool name from RPC serialization + default_resource: requesting_account.default_resource, + };This keeps the behavior consistent with what
create_accountexpects (default_resourceas a pool name string) while still avoiding any directsystem_storelookup.
🧹 Nitpick comments (2)
src/util/account_util.js (1)
324-332: Returning the account object here is correct and usefulThe added
return account;keeps existing error semantics while allowing callers (e.g., inaccount_server.js) to avoid a secondsystem_storelookup. The implementation here looks good.You may optionally update
validate_and_return_requested_account()to use the returned value instead of callingsystem_store.get_account_by_email()again to reduce duplication and race surface.src/server/system_services/account_server.js (1)
1227-1281: Centralizing IAM user lookup via_check_if_account_existslooks correctThe refactor in:
update_user(Lines 1227–1264),delete_user(Lines 1268–1281),tag_user(Lines 1422–1460),untag_user(Lines 1462–1487), andlist_user_tags(Lines 1489–1515)to use
account_util._check_if_account_exists()and consume its returnedrequested_accountis structurally sound. It:
- Removes duplicate
system_store.get_account_by_email()logic,- Standardizes the “no such entity” error path and messaging, and
- Keeps all ownership and root/IAM distinctions enforced by the existing helper checks.
As a follow-up, you could consider whether some of these flows (especially where you recompute the same
email_wrappedand run the same root/ownership checks) could reusevalidate_and_return_requested_account()to further reduce repetition, but functionally this change is good.Also applies to: 1422-1515
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/accountspace_nb.js(4 hunks)src/server/system_services/account_server.js(5 hunks)src/util/account_util.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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-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_nb.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/util/account_util.jssrc/sdk/accountspace_nb.jssrc/server/system_services/account_server.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.jssrc/server/system_services/account_server.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/server/system_services/account_server.js
🧬 Code graph analysis (1)
src/sdk/accountspace_nb.js (1)
src/server/system_services/account_server.js (19)
requesting_account(1208-1208)requesting_account(1228-1228)requesting_account(1269-1269)requesting_account(1285-1285)requesting_account(1313-1313)requesting_account(1344-1344)requesting_account(1357-1357)requesting_account(1385-1385)requesting_account(1402-1402)requesting_account(1423-1423)requesting_account(1464-1464)requesting_account(1491-1491)requesting_account(1519-1519)requesting_account(1548-1548)requesting_account(1562-1562)requesting_account(1581-1581)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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (1)
src/sdk/accountspace_nb.js (1)
64-161: Consistent use ofaccount_sdk.requesting_accountlooks good; verify call contractsThe rest of the methods now consistently derive
requesting_accountfromaccount_sdk.requesting_accountand pass it into the corresponding RPC calls. That aligns with the PR goal of avoiding extrasystem_storelookups.Please double-check that:
account_sdk.requesting_accountis always populated with the same logical account asreq.accounton the server side for these IAM flows, and- The second argument to
rpc_client.account.*is still treated as an options/context object and not relied on for anything now removed fromsystem_store.End-to-end IAM integration tests (especially around user management and access key APIs) should catch any remaining mismatches.
shirady
force-pushed
the
iam-remove-system-store-calls
branch
from
9d097bd to
5f59a40
Compare
shirady
force-pushed
the
iam-remove-system-store-calls
branch
from
5f59a40 to
c8167ee
Compare
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 (1)
src/sdk/accountspace_nb.js(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
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-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_nb.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.js
🧬 Code graph analysis (1)
src/sdk/accountspace_nb.js (2)
src/server/system_services/account_server.js (27)
requesting_account(1208-1208)requesting_account(1230-1230)requesting_account(1279-1279)requesting_account(1296-1296)requesting_account(1324-1324)requesting_account(1355-1355)requesting_account(1368-1368)requesting_account(1396-1396)requesting_account(1413-1413)requesting_account(1434-1434)requesting_account(1476-1476)requesting_account(1505-1505)requesting_account(1527-1527)requesting_account(1556-1556)requesting_account(1570-1570)requesting_account(1589-1589)account_email_wrapped(1280-1280)account_email_wrapped(1435-1435)account_email_wrapped(1477-1477)account_email_wrapped(1506-1506)account_util(25-25)account_util(53-53)params(328-328)params(691-691)params(955-955)req(134-134)req(396-396)src/util/account_util.js (1)
req(680-680)
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/sdk/accountspace_nb.js (2)
40-41: Clarify the deployment context scope for this verification.The concern about
default_resourceavailability across "all deployment contexts" may be misdirected. According to account management architecture, NSFS/NC deployments use separate account code insrc/manage_nsfs/, whileaccountspace_nb.jsis part of the containerized SDK layer. Verify thatdefault_resourceis present in the containerized account schema that populatesrequesting_accountobjects, which is the only context relevant for this file.
56-56: Verify RPC method signatures accept params without explicit requesting_account.All RPC method calls (get_user, update_user, delete_user, list_users, create_access_key, get_access_key_last_used, update_access_key, delete_access_key, list_access_keys, tag_user, untag_user, list_user_tags, put_user_policy, get_user_policy, delete_user_policy, list_user_policies) now pass only
paramswithout an explicitrequesting_accountparameter.Ensure that all these RPC methods in account_server.js have been updated to infer requesting_account from the RPC context (req.account) and that the RPC method signatures no longer require an explicit requesting_account parameter.
Also applies to: 60-60, 66-66, 72-72, 81-81, 85-85, 89-89, 94-94, 99-99, 107-107, 111-111, 115-115, 123-123, 127-127, 131-131, 135-135
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sdk/accountspace_nb.js (1)
18-22: Constructor fields appear unused; consider cleanup in a follow-up.
this.rpc_client,this.internal_rpc_client, andthis.statsare no longer used anywhere inAccountSpaceNBnow that all calls go throughaccount_sdk.rpc_client. If nothing else in the codebase depends on these properties being present, you could simplify the constructor signature and remove these assignments in a follow-up to reduce noise.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/accountspace_nb.js(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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: 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-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_nb.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.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/sdk/accountspace_nb.js
🧬 Code graph analysis (1)
src/sdk/accountspace_nb.js (2)
src/server/system_services/account_server.js (27)
params(328-328)params(691-691)params(955-955)requesting_account(1208-1208)requesting_account(1230-1230)requesting_account(1279-1279)requesting_account(1296-1296)requesting_account(1324-1324)requesting_account(1355-1355)requesting_account(1368-1368)requesting_account(1396-1396)requesting_account(1413-1413)requesting_account(1434-1434)requesting_account(1476-1476)requesting_account(1505-1505)requesting_account(1527-1527)requesting_account(1556-1556)requesting_account(1570-1570)requesting_account(1589-1589)account_email_wrapped(1280-1280)account_email_wrapped(1435-1435)account_email_wrapped(1477-1477)account_email_wrapped(1506-1506)account_util(25-25)account_util(53-53)req(134-134)req(396-396)src/util/account_util.js (1)
req(680-680)
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/sdk/accountspace_nb.js (2)
55-136: RPC delegations viaaccount_sdk.rpc_client.account.*look consistent.
get_user,update_user,delete_user,list_users, and all access-key/tag/policy methods now delegate directly toaccount_sdk.rpc_client.account.*(params), which keeps signatures simple and removes the extrarequesting_accountplumbing. This looks consistent across all methods and should preserve behavior as long as the server-side handlers rely onreq.accountrather than explicit parameters.
28-43: No changes needed. The code correctly usesrequesting_account._idwithout.toString()because in the containerized deployment context (whereaccountspace_nboperates viaaccount_sdk), the account object is loaded fromsystem_storewhereowneris an ObjectId reference with an._idproperty. The RPC serialization (which converts IDs to strings) occurs during theaccount_sdk.rpc_client.account.create_user(req)call, not before it. This differs from S3 REST flows where accounts come pre-serialized from RPC, but both patterns are correct for their respective contexts.
shirady
force-pushed
the
iam-remove-system-store-calls
branch
from
88827be to
977d2da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/sdk/accountspace_nb.js (2)
78-98: User access key methods: consistent RPC forwarding.These access key methods now cleanly proxy to
account_sdk.rpc_client.account.*with justparams, matching the updated pattern from the user methods. This looks consistent and correct; any redundantreturn awaitpatterns here can be simplified the same way as above if you prefer.
104-114: User tag methods: straightforward delegation.
tag_user,untag_user, andlist_user_tagsnow simply delegate to the account RPC client with no extra context, which is consistent with the rest of this class and the goal of removingsystem_storelookups. No functional issues spotted.
🧹 Nitpick comments (2)
src/sdk/accountspace_nb.js (2)
28-51: create_user:requesting_accountusage and owner/id wiring look correct.Using
account_sdk.requesting_accountinstead of re‑queryingsystem_storeis appropriate here;_idis already a string in this context, so passingrequesting_account._idintoget_account_email_from_usernameand intoowneris correct without.toString(). The response mapping also looks consistent with the returnediam_accountfields.If you want to tighten this further, you could:
- Use
iam_account.iam_pathfrom the RPC response instead of recomputingparams.iam_path || IAM_DEFAULT_PATH, so the returned value always reflects the server’s source of truth.- Optionally default
iam_pathinreqitself (e.g.,iam_path: params.iam_path || IAM_DEFAULT_PATH) to keep request/response aligned.Based on learnings,
requesting_account._idis already a string in this file.
53-71: User methods: pass‑through RPC calls look correct (minor async nit).Forwarding
paramsdirectly toaccount_sdk.rpc_client.account.*and dropping the extrarequesting_accountargument aligns with obtaining the caller context from the RPC layer instead ofsystem_store.The only minor style nit is the redundant
awaitinreturn await ...— you could simplify toreturn account_sdk.rpc_client.account.get_user(params);(and similarly forupdate_user,delete_user,list_users) if you don’t rely on the extra stack frame.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/accountspace_nb.js(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.638Z
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: 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-12-04T10:55:08.638Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.638Z
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/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_nb.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.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/sdk/accountspace_nb.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/sdk/accountspace_nb.js
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/sdk/accountspace_nb.js
🧬 Code graph analysis (1)
src/sdk/accountspace_nb.js (2)
src/server/system_services/account_server.js (27)
params(328-328)params(691-691)params(955-955)requesting_account(1208-1208)requesting_account(1230-1230)requesting_account(1279-1279)requesting_account(1296-1296)requesting_account(1324-1324)requesting_account(1355-1355)requesting_account(1368-1368)requesting_account(1396-1396)requesting_account(1413-1413)requesting_account(1434-1434)requesting_account(1476-1476)requesting_account(1505-1505)requesting_account(1527-1527)requesting_account(1556-1556)requesting_account(1570-1570)requesting_account(1589-1589)account_email_wrapped(1280-1280)account_email_wrapped(1435-1435)account_email_wrapped(1477-1477)account_email_wrapped(1506-1506)account_util(25-25)account_util(53-53)req(134-134)req(396-396)src/util/account_util.js (1)
req(680-680)
⏰ 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 (2)
src/sdk/accountspace_nb.js (2)
24-26: Section renames improve clarity.Renaming the section headers from generic “ACCOUNT” to “USER …”/“USER ACCESS KEY …”/“USER TAGS …”/“POLICY METHODS” better reflects the IAM semantics and is consistent. No issues here.
Also applies to: 74-76, 100-102, 116-118
120-134: User policy methods: delegation pattern matches the rest.Policy-related methods (
put_user_policy,get_user_policy,delete_user_policy,list_user_policies) follow the same simplified delegation approach as the user and access‑key methods. Assuming the RPC signatures were updated to no longer expect arequesting_accountargument, this is correct and consistent.You may want to confirm RPC signatures once across the codebase to ensure no remaining callers still pass
requesting_account, but this file’s usage is coherent with the described change.
shirady
force-pushed
the
iam-remove-system-store-calls
branch
from
977d2da to
ccc5b63
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/sdk/accountspace_nb.js (3)
28-51: create_user: context usage looks correct; consider normalizingiam_pathonceUsing
account_sdk.requesting_accountfor both email generation andowneris consistent with the RPC-serialized account object; no remainingsystem_storedependency here. To avoid any divergence between what is sent and what is returned, you could normalizeiam_pathonce and reuse it:- async create_user(params, account_sdk) { - const requesting_account = account_sdk.requesting_account; - const account_email_wrapped = account_util.get_account_email_from_username(params.username, requesting_account._id); - const req = { + async create_user(params, account_sdk) { + const requesting_account = account_sdk.requesting_account; + const iam_path = params.iam_path || IAM_DEFAULT_PATH; + const account_email_wrapped = account_util.get_account_email_from_username( + params.username, + requesting_account._id + ); + const req = { name: params.username, email: account_email_wrapped, has_login: false, s3_access: true, allow_bucket_creation: true, - owner: requesting_account._id, - iam_path: params.iam_path, + owner: requesting_account._id, + iam_path, roles: ['admin'], }; const iam_account = await account_sdk.rpc_client.account.create_user(req); return { - iam_path: params.iam_path || IAM_DEFAULT_PATH, + iam_path, username: params.username, user_id: iam_account.id, arn: iam_account.arn, create_date: iam_account.create_date, };Based on learnings, using
requesting_account._iddirectly (without.toString()) here is correct because it is already a string from RPC serialization.
53-71: User RPC wrappers are correct; redundantawaitis optional cleanupThese methods now correctly delegate to
account_sdk.rpc_client.account.*(params)without passingrequesting_account, aligning with the refactor away fromsystem_store. Since they don’t add logic around the call, you can simplify slightly by dropping the redundantawait:- async get_user(params, account_sdk) { - return await account_sdk.rpc_client.account.get_user(params); - } + async get_user(params, account_sdk) { + return account_sdk.rpc_client.account.get_user(params); + }(and similarly for
update_user,delete_user,list_users).
78-98: Access-key RPC wrappers match the new API shape; consider DRYing or droppingawaitForwarding
paramsonly intoaccount_sdk.rpc_client.account.*is consistent with the updated server-side RPC contracts. As with the user methods, you can optionally:
- Remove
awaitand justreturn account_sdk.rpc_client.account.<method>(params);, or- Introduce a tiny helper if you want to reduce repetition across these thin wrappers.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/accountspace_nb.js(3 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.638Z
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: 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-12-04T10:55:08.638Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.638Z
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/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_nb.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/accountspace_nb.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/accountspace_nb.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/sdk/accountspace_nb.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/sdk/accountspace_nb.js
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/sdk/accountspace_nb.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 (6)
src/sdk/accountspace_nb.js (6)
24-26: User section header rename improves readabilityRenaming this block to “USER METHODS” matches the method names and clarifies intent; no functional impact.
74-76: User access-key section header rename is consistentThe “USER ACCESS KEY METHODS” header matches the underlying IAM semantics better than the previous “ACCOUNT” wording.
100-102: User tags section header rename is appropriate“USER TAGS METHODS” better reflects that these operate on IAM users rather than top-level accounts.
104-114: Tagging RPC wrappers are correctly updated to useaccount_sdk.rpc_client
tag_user,untag_user, andlist_user_tagsnow cleanly delegate to the account RPC client with justparams; that aligns with removing explicitrequesting_accountpropagation.
116-118: Policy section header remains clear and scoped to user policiesThe “POLICY METHODS” header is fine here and matches the user-policy-focused methods below it.
120-134: User policy RPC wrappers align with the new contract
put_user_policy,get_user_policy,delete_user_policy, andlist_user_policiescorrectly callaccount_sdk.rpc_client.account.*(params)without passingrequesting_account. This matches the rest of the refactor and keeps the client surface consistent.
Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-remove-system-store-calls
branch
from
d6808e3 to
4ddc131
Compare
2 participants
Describe the Problem
Currently, there are many calls to
system_storein functions related to IAM.Explain the Changes
system_store, mainly where it was to get therequesting_accountin the fileaccountspace_nb.js.Issues:
List of GAPs:
create_userinside theaccount_serverso we would have thereq.accountaccess and keep this file with small functionality.account_serverandaccount_utilsas well.Testing Instructions:
I rerun the API tests to make sure it doesn't break.
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.jsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.