Skip to content

Conversation

@shirady
Copy link
Contributor

@shirady shirady commented Dec 8, 2025

Describe the Problem

Currently, the GetAccessKeyLastUsed API didn't work when the account run it on the access key ID of their user.

Explain the Changes

  1. Add the needed implementation in get_access_key_last_used.
  2. Add the test that was skipped.
  3. Add additional test case.

Issues:

  1. List of GAPs:
  • We need to add more cases in the IAM test to validate when another account tries to use the API (currently it is done only from the admin account on an IAM user).

Testing Instructions:

Automatic Test

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

NC:

  1. sudo NC_CORETEST=true ./node_modules/.bin/mocha src/test/integration_tests/api/iam/test_iam_basic_integration.js
  2. sudo npx jest test_accountspace_fs.test.js (unit tests)

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. Create a user: admin-iam iam create-user --user-name Robert
    Note: To validate user creation, you can rerun admin-iam iam list-users and expect 1 user in the list
  2. Create access keys: admin-iam iam create-access-keys --user-name Robert
  3. Try to run get-access-key-last-used from the admin on their user (should work): admin-iam iam get-access-key-last-used --access-key-id <access-key-user>.
  4. Create the alias for 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:14443'
  5. Try to run get-access-key-last-used from the user on himself (should work): user-1-iam iam get-access-key-last-used --access-key-id <access-key-user>.
  6. Create another account: nb account create shira-acc01 -n test1 --show-secrets
  7. Create the alias for the account: alias account-1-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key-account> aws --no-verify-ssl --endpoint-url https://localhost:14443'
  8. Try to run get-access-key-last-used from the account on the user (should fail): account-1-iam iam get-access-key-last-used --access-key-id <access-key-user>. (throws NoSuchEntity).

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).

  • Doc added/updated

  • Tests added

Summary by CodeRabbit

  • Bug Fixes

    • Access key validation for GetAccessKeyLastUsed updated to enforce ownership checks and ensure returned last-used details (username, service, region, date) reflect the correct account.
  • Tests

    • Re-enabled GetAccessKeyLastUsed integration test and added a new test suite that creates and tears down IAM users/access keys and verifies valid and invalid key behavior.

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

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

coderabbitai bot commented Dec 8, 2025

Walkthrough

Replaces direct account validation in get_access_key_last_used with a new access-key-based ownership check that resolves the target account from an access key; adds integration tests for GetAccessKeyLastUsed and introduces helper functions for IAM access key setup/teardown.

Changes

Cohort / File(s) Summary
Account server — access key last-used
src/server/system_services/account_server.js
Replaced use of validate_and_return_requested_account and explicit access-key validation with a call to account_util._check_if_iam_user_belongs_to_account_owner_by_access_key, deriving the requested account from req.rpc_params.access_key; response shape unchanged.
Account utility — ownership check
src/util/account_util.js
Added _check_if_iam_user_belongs_to_account_owner_by_access_key(action, requesting_account, access_key_id) which resolves an account via access key, enforces membership/ownership checks, and is exported.
Integration tests — IAM access keys
src/test/integration_tests/api/iam/test_iam_basic_integration.js
Unskipped/expanded GetAccessKeyLastUsed tests; added an IAM Access Keys test suite that creates/deletes an IAM user and access key and validates valid/invalid-key behavior; added file-scoped helpers create_access_key_iam_user and delete_access_key_iam_user (note: helper definitions appear duplicated in the file due to insertion points).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas to focus on:
    • Correctness and security of _check_if_iam_user_belongs_to_account_owner_by_access_key (account resolution, error handling, permission semantics).
    • Interaction between account_server and the new utility call (parameter passing, edge cases for missing/invalid access keys).
    • Test helper correctness and cleanup to avoid leftover IAM users/access keys and duplicated helper definitions in the test file.

Possibly related PRs

Suggested labels

size/L

Suggested reviewers

  • aayushchouhan09
  • liranmauda
  • naveenpaul1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% 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 | Fix GetAccessKeyLastUsed API' accurately describes the main change: fixing the GetAccessKeyLastUsed API implementation in the IAM system.
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.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady force-pushed the iam-fix-get-access-key-last-used branch from 9adb264 to d606be8 Compare December 8, 2025 16:57
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 8, 2025
@shirady shirady merged commit 1f497fa into noobaa:master Dec 9, 2025
18 of 19 checks passed
@shirady shirady deleted the iam-fix-get-access-key-last-used branch December 9, 2025 06:14
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