Skip to content

Conversation

@shirady
Copy link
Contributor

@shirady shirady commented Dec 9, 2025

Describe the Problem

Currently, we have events for user creation and user deletion (with an issue).
In this PR, we are adding the events so we will have events on:

  1. Create user (reusing account creation)
  2. Delete user(reusing account deletion)
  3. Update user
  4. Add access key (reusing account generate credentials)
  5. Delete access key
  6. Update access key

Explain the Changes

  1. Fix the arguments that were passed to delete_account in delete_user.
  2. Add events for: Update user, Delete access key and Update access key.

Note: Since there are some cases that use the same function for account and IAM user:

  • create user - reusing account creation
  • delete user - reusing account deletion
  • add access key - reusing account generate credentials)
    I wanted to use the same style, and hence, the events are with a prefix of 'accountand not IAM user. The assumption is that it's all saved in theactivitylogs, and can filter by account`, which is the ID (can be user or account).

Issues:

  1. none

Testing Instructions:

Manual Test

  1. Build the images and install NooBaa system on Rancher Desktop (see guide).
    Note: nb is an alias that runs the local operator from build/_output/bin (alias created by devenv).
  2. Wait for the default backing store pod to be in state Ready before starting the tests: kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1
  3. I'm using port-forward (in a different tab):
  • S3 kubectl port-forward -n test1 service/s3 12443:443
  • IAM kubectl port-forward -n test1 service/iam 14443:443
  1. Create the alias for the admin - first, need to get the credentials: nb status --show-secrets -n test1 and then:
    alias admin-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'
  2. Check the connection to the endpoint:
  • try to list the users (should be an empty list): admin-iam iam list-users; echo $?
  1. Start running actions that create events, for example:
  • Create a user: admin-iam iam create-user --user-name Robert
  • Create access key: admin-iam iam create-access-key --user-name Robert
  • Update user: admin-iam iam update-user --user-name Robert --new-path /div/sub/
  • Create additional access key: admin-iam iam create-access-key --user-name Robert
  1. Create the alias for the the user: alias user-1-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:1414443'
  2. Continue running actions that create events, for example:
  • Update the access key from the user: user-1-iam iam update-access-key --access-key-id <access-key-id> --status Inactive
  • Delete access key: user-1-iam iam delete-access-key --access-key-id <access-key-id>
  1. In the DB you will see the following (I copied the line of desc only):

“desc”: “robert:693824bf2b88180021f0b950 was created by admin@noobaa.io
“desc”: “Credentials for robert:693824bf2b88180021f0b950 were regenerated by admin@noobaa.io”,
“desc”: “robert:693824bf2b88180021f0b950 was updated by admin@noobaa.io”,
“desc”: “Credentials for robert:693824bf2b88180021f0b950 were regenerated by admin@noobaa.io”,
“desc”: “Credentials for robert:693824bf2b88180021f0b950 were updated by robert:693824bf2b88180021f0b950",
“desc”: “Credentials for robert:693824bf2b88180021f0b950 were deleted by robert:693824bf2b88180021f0b950",

Notes:

  • In step 1 - deploying the system, I used --use-standalone-db for simplicity (fewer steps for the system in Ready status).

  • I used the admin account, but for IAM operations, there is no difference between a created account and an admin account. I tested with the admin only because it saves steps (I have the admin account upon system creation).

  • To see the full table use: select jsonb_pretty(data) from activitylogs; from the DB pod.

  • Doc added/updated

  • Tests added

Summary by CodeRabbit

  • New Features

    • Added system-scoped activity logging for account updates, credential updates, and credential deletions, capturing system context and actor details.
    • Added a helper to resolve system context used by event logging.
  • Refactor

    • Simplified the user delete flow and aligned IAM-related logging with other account operations for consistent activity records.

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

@shirady shirady self-assigned this Dec 9, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Walkthrough

Added a helper to derive a system id for activity events and emitted system-scoped activity events for account/user and credential operations; refactored delete_user to call delete_account(req, requested_account) directly and aligned IAM-path logging with the standard flows.

Changes

Cohort / File(s) Summary
Activity logging & delete flow refactor
src/server/system_services/account_server.js
Emit activity events (account.update, account.update_credentials, account.delete_credentials) after user and credential operations; derive system using the new helper; refactor delete_user to call delete_account(req, requested_account) directly; add IAM-path logging to match non-IAM flow.
System ID helper
src/util/account_util.js
Add and export get_system_id_for_events(req) which returns a system id from new_system_parameters.new_system_id (via system_store.parse_system_store_id) or falls back to req.system._id.
Manifest
package.json
Touched in diff (no functional details provided).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify activity event payload shapes and field names (system, actor, account, description) match event schema/consumers.
  • Confirm get_system_id_for_events handles both IAM and non-IAM request shapes and missing fields safely.
  • Ensure delete_userdelete_account(req, requested_account) refactor preserves authorization, validation, and side effects.

Suggested reviewers

  • aayushchouhan09
  • naveenpaul1
  • liranmauda

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
Title check ✅ Passed The title 'IAM | Add Events' directly describes the main change—adding IAM-related activity event emissions to the account server for user and access key operations.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 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 1a4d22d and a19f47a.

📒 Files selected for processing (1)
  • src/server/system_services/account_server.js (4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-19T15:03:42.260Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.

Applied to files:

  • src/server/system_services/account_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.

Applied to files:

  • src/server/system_services/account_server.js
📚 Learning: 2025-12-04T10:55:08.659Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9330
File: src/sdk/accountspace_nb.js:30-30
Timestamp: 2025-12-04T10:55:08.659Z
Learning: In src/sdk/accountspace_nb.js, account_sdk.requesting_account comes from RPC serialization, so requesting_account._id is already a string (not an ObjectId). This differs from system_store.get_account_by_email() which returns accounts with ObjectId._id references. Therefore, no .toString() conversion is needed when using account_sdk.requesting_account._id in accountspace_nb.js.

Applied to files:

  • src/server/system_services/account_server.js
🧬 Code graph analysis (1)
src/server/system_services/account_server.js (3)
src/util/account_util.js (4)
  • sys_id (54-56)
  • req (704-704)
  • system_store (12-12)
  • Dispatcher (5-5)
src/server/object_services/object_server.js (1)
  • Dispatcher (23-23)
src/server/notifications/event_server.js (1)
  • Dispatcher (11-11)
⏰ 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/server/system_services/account_server.js (1)

1287-1298: delete_user now delegates correctly to account_util.delete_account

The new call return account_util.delete_account(req, requested_account); matches the existing usage in delete_account(req) (which calls account_util.delete_account(req, account_to_delete);), so this looks like the right fix for wiring IAM user deletion through the shared deletion path (and its associated events/cleanup).

If you want extra assurance, verify that account_util.delete_account:

  • Emits the intended account deletion activity event for IAM users as well as root accounts.
  • Does not rely on req.rpc_params.email matching the deleted account, since here you pass the IAM account object directly.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/util/account_util.js (1)

776-781: LGTM! Good refactor to centralize system ID derivation for events.

The helper function correctly derives the system ID by preferring new_system_parameters.new_system_id and falling back to req.system._id, which matches the pattern used elsewhere (e.g., lines 54-56 in create_account).

Consider adding JSDoc documentation to describe the function's purpose and parameters:

+/**
+ * Derives the system ID for activity event logging.
+ * @param {Object} req - The request object
+ * @returns {string|undefined} The system ID, or undefined if unavailable
+ */
 function get_system_id_for_events(req) {
     const sys_id = req.rpc_params.new_system_parameters ?
     system_store.parse_system_store_id(req.rpc_params.new_system_parameters.new_system_id) :
     req.system && req.system._id;
     return sys_id;
 }

Additionally, you could refactor the similar logic in create_account (lines 54-56) to use this helper:

-    const sys_id = req.rpc_params.new_system_parameters ?
-        system_store.parse_system_store_id(req.rpc_params.new_system_parameters.new_system_id) :
-        req.system._id;
+    const sys_id = get_system_id_for_events(req);
📜 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 5055ac9 and 906b072.

📒 Files selected for processing (2)
  • src/server/system_services/account_server.js (4 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-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.

Applied to files:

  • src/util/account_util.js
🧬 Code graph analysis (1)
src/util/account_util.js (1)
src/server/system_services/account_server.js (5)
  • req (134-134)
  • req (396-396)
  • sys_id (1264-1264)
  • sys_id (1394-1394)
  • sys_id (1443-1443)
⏰ 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/util/account_util.js (1)

810-810: LGTM! Export statement is correct.

The export follows the existing pattern and enables the helper function to be used in account_server.js for event logging.

@shirady shirady merged commit 2617b49 into noobaa:master Dec 10, 2025
18 checks passed
@shirady shirady deleted the iam-add-events branch December 10, 2025 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants