IAM | IAM user integration test for containerized deployments#9339
Conversation
WalkthroughAdds a new integration test verifying IAM user interactions with S3 bucket policies (various allow/deny and principal scenarios) and registers it in the test runner. Also changes account owner extraction in get_account_info to use account_util.get_owner_account_id. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
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: 2
🧹 Nitpick comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (2)
35-35: Unused bucketBKT_Cis created but never used.
BKT_Cis defined at line 35 and created at line 129, but no test case uses it. This is either dead code or indicates a missing test scenario.Either remove the unused bucket or add a test case that utilizes it:
-const BKT_C = 'iam-bucket-policy-ops-2';And in setup:
- await s3_owner.createBucket({ Bucket: BKT_C });Also applies to: 129-129
255-256: Comment step numbering is inconsistent.The comment says "5. Try to put object..." but there's no step 4 in this test case. The previous steps are numbered 1, 2, and 3.
- // 5. Try to put object to bucket with IAM user s3 client, Should not fail + // 4. Try to put object to bucket with IAM user s3 client, Should not fail
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.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/s3/test_s3_bucket_policy_iam_user.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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (3)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
1-68: LGTM!The imports, constants, and shared state declarations are appropriate for this integration test setup. Setting
OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS = 1ensures fresh account data is fetched during tests.
158-224: LGTM!The test case is well-structured, covering the complete flow: IAM user creation, bucket policy setup, access key generation, inline policy attachment, and S3 operation verification. Good practice to clean up the test object at the end.
20-29: LGTM!The
assert_throws_asynchelper is well-implemented for asserting expected exceptions from async operations. It properly re-throws unexpected errors.
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
afd0e81 to
52746f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (2)
152-156: Incomplete cleanup leaves test artifacts.The
afterhook only deletesBKT, but setup creates three buckets (BKT,BKT_B,BKT_C) and two user accounts. This can cause issues in subsequent test runs.As noted in the previous review, extend cleanup to remove all test resources.
360-370: Missingawaitprevents test from validating the expected behavior.The
s3_user_b.putObject()call is not awaited, so the test completes without verifying the operation failure. Additionally, AWS SDK v3 useserr.nameinstead oferr.code.As noted in the previous review, replace the try-catch with the
assert_throws_asynchelper to properly await and verify the AccessDenied error.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js(1 hunks)src/test/utils/index/index.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/s3/test_s3_bucket_policy_iam_user.jssrc/test/utils/index/index.js
🧬 Code graph analysis (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
src/test/utils/index/index.js (1)
coretest(5-5)src/test/utils/coretest/coretest.js (1)
POOL_LIST(80-88)src/test/system_tests/test_utils.js (2)
TMP_PATH(23-23)is_nc_coretest(48-48)
src/test/utils/index/index.js (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (4)
require(7-7)require(14-14)require(15-15)require(31-31)
⏰ 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 (8)
src/test/utils/index/index.js (1)
114-114: LGTM!The new test module is correctly loaded after
test_iam_basic_integration, ensuring proper test execution order for IAM-related integration tests.src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (7)
70-148: LGTM!The setup function properly initializes test accounts, buckets, and IAM clients. The conditional NC_CORETEST logic appropriately configures NSFS accounts when needed.
158-224: LGTM!The test correctly validates IAM user access with bucket policies, including user creation, access key provisioning, inline policy application, and S3 operations.
226-271: LGTM!The test correctly validates IAM user access to buckets owned by their owner account, with proper credential setup and operation verification.
304-320: LGTM!The test correctly validates that using IAM user ID instead of ARN in bucket policy is properly rejected with an appropriate error message.
423-425: LGTM!The helper function correctly validates successful HTTP responses with a clear assertion.
5-6: Verify config reset and test isolation mechanisms in test setup/teardown.Setting
config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS = 1mutates a shared config object. Confirm that:
- The config value is reset to its original state after the test completes
- This modification doesn't affect concurrent tests or subsequent test runs
- There's intentional test isolation handling (e.g., in
require_coretest()or afterEach hooks)Without access to the test setup code, the actual impact on test isolation cannot be determined.
159-159: Verify that skipping all tests in NC_CORETEST mode is intentional.All six test cases in this suite skip when
is_nc_coretestis true (lines 159, 227, 274, 305, 323, 374). This means IAM user integration tests never run in NC_CORETEST mode, despite the setup function including NC_CORETEST-specific configuration logic.Confirm whether:
- NC_CORETEST mode doesn't support IAM user features yet, or
- The tests should be adjusted to run in NC_CORETEST mode
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
52746f9 to
6348d64
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (5)
20-29: AWS SDK v3 useserr.name, noterr.messagefor error types.The helper checks
err.messageagainst'Access Denied', but AWS SDK v3 error objects useerr.name(e.g.,'AccessDenied'). This causes the assertion to incorrectly pass or fail.Apply this diff:
-async function assert_throws_async(promise, expected_message = 'Access Denied') { +async function assert_throws_async(promise, expected_error_name = 'AccessDenied') { try { await promise; - assert.fail('Test was suppose to fail on ' + expected_message); + assert.fail('Test was supposed to fail with ' + expected_error_name); } catch (err) { - if (err.message !== expected_message) { + if (err.name !== expected_error_name) { throw err; } } }
150-151: Missingmocha.afterhook for cleanup.The test setup creates buckets (
BKT,BKT_B,BKT_C) and IAM users but cleanup is embedded in the last test case. If any test fails before that, or tests run in isolation, resources will leak. Add a properafterhook for reliable cleanup.mocha.describe('IAM S3_bucket_policy', async function() { mocha.before(setup); + + mocha.after(async function() { + this.timeout(60000); + try { await s3_owner.deleteBucket({ Bucket: BKT }); } catch (_) { /* ignore */ } + try { await s3_owner.deleteBucket({ Bucket: BKT_C }); } catch (_) { /* ignore */ } + try { await s3_b.deleteBucket({ Bucket: BKT_B }); } catch (_) { /* ignore */ } + try { await rpc_client.account.delete_account({ email: user_a }); } catch (_) { /* ignore */ } + try { await rpc_client.account.delete_account({ email: user_b }); } catch (_) { /* ignore */ } + });
291-291: Typo: "denaied" should be "denied".- // If there is bucket policy, owner account bucket access is denaied for user. + // If there is bucket policy, owner account bucket access is denied for user.
354-365: Missingawaitcauses test to always pass without verifying behavior.The
s3_user_b.putObject()call is not awaited, so the try/catch never catches anything. The test completes immediately. Also, AWS SDK v3 useserr.name, noterr.code.Use the existing helper for consistency:
// IAM policy deny all the access - try { - s3_user_b.putObject({ - Body: BODY, - Bucket: BKT_B, - Key: KEY, - }); - } catch (err) { - if (err.code !== 'AccessDenied') { - throw err; - } - } + await assert_throws_async(s3_user_b.putObject({ + Body: BODY, + Bucket: BKT_B, + Key: KEY, + }));
405-405: Misleading comment.The comment says "IAM policy deny all the access" but the test actually sets IAM policy to Allow (line 373) and validates that bucket policy Deny overrides it.
- // IAM policy deny all the access + // Bucket policy Deny overrides IAM policy Allow
🧹 Nitpick comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (1)
411-414: UnnecessarydeleteObjectcall for an object that was never created.The test expects
putObjectto fail (line 406-410), soKEYwas never created inBKT_B. ThisdeleteObjectis misleading and unnecessary.- await s3_b.deleteObject({ - Bucket: BKT_B, - Key: KEY, - });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js(1 hunks)src/test/utils/index/index.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/index.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
src/test/utils/index/index.js (1)
coretest(5-5)src/test/utils/coretest/coretest.js (1)
POOL_LIST(80-88)src/test/system_tests/test_utils.js (2)
TMP_PATH(23-23)is_nc_coretest(48-48)
⏰ 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 (6)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (5)
70-148: Setup function looks complete and well-structured.The setup correctly initializes S3 clients, IAM clients, creates test accounts and buckets, and handles both NC_CORETEST and standard modes appropriately.
153-219: Test case correctly validates IAM user access via bucket policy.The test properly creates an IAM user, attaches a bucket policy, generates access keys, assigns an inline policy, and verifies S3 operations work as expected.
221-266: Test case correctly validates IAM user access to owner's bucket.The test verifies that an IAM user with appropriate inline policy can access buckets owned by their parent account.
299-315: Test correctly validates that IAM User ID is not supported in bucket policy principal.Good validation of error handling for unsupported principal format.
427-433: Helper function is clean and reusable.src/test/utils/index/index.js (1)
112-114: Test module correctly registered in the test index.The new integration test is appropriately placed under the "Running with IAM port" section, following the existing
test_iam_basic_integrationmodule.
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
6348d64 to
8637578
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
20-29: AWS SDK v3 useserr.namefor error types, noterr.message.The function compares
err.messagewhich contains descriptive text (e.g., "Access Denied"), but for reliable error checking with AWS SDK v3, you should compare againsterr.name(e.g.,'AccessDenied'). This can lead to false positives when unexpected errors have similar messages.-async function assert_throws_async(promise, expected_message = 'Access Denied') { +async function assert_throws_async(promise, expected_error_name = 'AccessDenied') { try { await promise; - assert.fail('Test was supposed to fail with ' + expected_message); + assert.fail('Test was supposed to fail with ' + expected_error_name); } catch (err) { - if (err.message !== expected_message) { + if (err.name !== expected_error_name) { throw err; } } }
153-203: Incomplete cleanup: NooBaa accounts and buckets not deleted inafterhook.The
afterhook properly cleans up IAM resources but leaves the NooBaa accounts (user_a,user_b) and relies on the last test case to delete buckets. This is fragile - if tests run in isolation, skip, or fail before the bucket cleanup code, resources remain.Move bucket deletion here and add account cleanup:
const response_delete_b = await iam_account_b.send(command_delete_b); _check_status_code_ok(response_delete_b); + + // Clean up buckets + await s3_owner.deleteBucket({ Bucket: BKT }).catch(() => {}); + await s3_owner.deleteBucket({ Bucket: BKT_C }).catch(() => {}); + await s3_b.deleteBucket({ Bucket: BKT_B }).catch(() => {}); + + // Clean up NooBaa accounts + await rpc_client.account.delete_account({ email: user_a }); + await rpc_client.account.delete_account({ email: user_b }); });
407-417: Critical:err.Codeis incorrect for AWS SDK v3; useerr.nameinstead.AWS SDK v3 stores the error type in
err.name, noterr.Code. The current code will never match'AccessDenied'becauseerr.Codeisundefined, causing the test to incorrectly fail or pass based on error behavior. Consider using theassert_throws_asynchelper for consistency.- try { - await s3_user_b.putObject({ - Body: BODY, - Bucket: BKT_B, - Key: KEY, - }); - } catch (err) { - if (err.Code !== 'AccessDenied') { - throw err; - } - } + await assert_throws_async(s3_user_b.putObject({ + Body: BODY, + Bucket: BKT_B, + Key: KEY, + }));
🧹 Nitpick comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (1)
463-476: Bucket cleanup should be inafterhook, anddeleteObjectis unnecessary here.The
deleteObjectcall on lines 463-466 attempts to delete a key that was never created (theputObjectwas denied). This is harmless but confusing. Additionally, bucket cleanup in the last test is fragile - consider moving all cleanup to theafterhook as noted above.- await s3_b.deleteObject({ - Bucket: BKT_B, - Key: KEY, - }); - await s3_owner.deleteBucket({ - Bucket: BKT, - }); - await s3_owner.deleteBucket({ - Bucket: BKT_C, - }); - await s3_b.deleteBucket({ - Bucket: BKT_B, - }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js(1 hunks)src/test/utils/index/index.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/index.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
src/test/utils/index/index.js (1)
coretest(5-5)src/test/utils/coretest/coretest.js (1)
POOL_LIST(80-88)src/test/system_tests/test_utils.js (1)
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-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (6)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (5)
1-18: LGTM!The imports and configuration setup are appropriate for the integration test. Using AWS SDK v3 clients and coretest infrastructure aligns with the project patterns.
31-69: LGTM!Constants and IAM command imports are well-organized. The policy documents are correctly structured JSON strings.
71-149: LGTM!The setup function properly initializes S3 credentials, creates accounts, buckets, and IAM clients. The conditional handling for
NC_CORETESTis appropriate.
205-367: LGTM!Tests 1-4 cover important IAM/bucket policy scenarios: basic bucket policy access, owner account bucket access, bucket policy denial, and invalid principal handling. The test structure and assertions are appropriate.
479-485: LGTM!The helper function
_check_status_code_okis well-documented with JSDoc and correctly validates the HTTP status code.src/test/utils/index/index.js (1)
113-114: LGTM!The new integration test is correctly registered in the test loader under the "Running with IAM port" section, ensuring it will be executed as part of the IAM integration test suite.
liranmauda
left a comment
liranmauda
left a comment
There was a problem hiding this comment.
Why are those new tests written with mocha?
all new tests should be written using jest.
rewrite the tests with jest
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
8637578 to
11c697b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (2)
20-29: AWS SDK v3 useserr.namefor error types, noterr.message.The helper checks
err.messagebut AWS SDK v3 sets error types onerr.name(e.g.,'AccessDenied'). This can lead to false positives when unexpected errors occur.Apply this diff:
-async function assert_throws_async(promise, expected_message = 'Access Denied') { +async function assert_throws_async(promise, expected_error_name = 'AccessDenied') { try { await promise; - assert.fail('Test was supposed to fail with ' + expected_message); + assert.fail('Test was supposed to fail with ' + expected_error_name); } catch (err) { - if (err.message !== expected_message) { + if (err.name !== expected_error_name) { throw err; } } }Based on past review comments.
409-420: Useerr.nameinstead oferr.Codefor AWS SDK v3 error checking.Line 417 checks
err.Code, but AWS SDK v3 useserr.namefor error types. This will not correctly validate the AccessDenied error.Apply this diff:
try { await s3_user_b.putObject({ Body: BODY, Bucket: BKT_B, Key: KEY, }); + assert.fail('Expected AccessDenied error'); } catch (err) { - if (err.Code !== 'AccessDenied') { + if (err.name !== 'AccessDenied') { throw err; } }Better yet, use the
assert_throws_asynchelper once it's fixed to checkerr.name.
🧹 Nitpick comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (2)
153-206: Move bucket cleanup to theafterhook for guaranteed execution.The buckets are deleted at the end of the last test (lines 470-478) rather than in the
afterhook. If the last test fails before reaching the cleanup code, the buckets will remain and may interfere with subsequent test runs.Move the bucket deletion to the
afterhook:mocha.after(async function() { this.timeout(60000); + // Clean up buckets + try { + await s3_owner.deleteBucket({ Bucket: BKT }); + } catch (err) { /* ignore if already deleted */ } + try { + await s3_owner.deleteBucket({ Bucket: BKT_C }); + } catch (err) { /* ignore if already deleted */ } + try { + await s3_b.deleteBucket({ Bucket: BKT_B }); + } catch (err) { /* ignore if already deleted */ } + // Clean up accountsThen remove the bucket deletion from lines 470-478.
466-469: Consider defensive cleanup for object deletion.The
deleteObjectcalls will fail if the object wasn't created (e.g., if a previous assertion failed). Consider wrapping in try-catch blocks to make cleanup more resilient.Example pattern:
try { await s3_b.deleteObject({ Bucket: BKT_B, Key: KEY, }); } catch (err) { // Ignore if object doesn't exist }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js(1 hunks)src/test/utils/index/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/utils/index/index.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/s3/test_s3_bucket_policy_iam_user.js
🧠 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-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-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
📚 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
🧬 Code graph analysis (2)
src/server/system_services/account_server.js (2)
src/sdk/accountspace_nb.js (1)
account_util(4-4)src/util/account_util.js (6)
account(30-42)account(196-196)account(304-304)account(326-326)account(358-358)account(647-647)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
src/test/utils/index/index.js (1)
coretest(5-5)src/test/utils/coretest/coretest.js (1)
POOL_LIST(80-88)src/test/system_tests/test_utils.js (2)
TMP_PATH(23-23)is_nc_coretest(48-48)
⏰ 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 (1)
src/server/system_services/account_server.js (1)
1014-1016: Duplicate owner assignment - line 1073 overwrites line 1015.The
info.ownerfield is set twice: once at line 1015 usingaccount_util.get_owner_account_id(account), and again at line 1073 usingaccount.owner._id.toString(). The second assignment overwrites the first, making the utility function call at line 1015 ineffective.Apply this diff to remove the duplicate:
if (account.iam_user_policies) { info.iam_user_policies = account.iam_user_policies; } - if (account.owner) { - info.owner = account.owner._id.toString(); - } return info;Also applies to: 1072-1074
⛔ Skipped due to learnings
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.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.
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
86fc7e9 to
e03a12d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (3)
155-206: Centralize bucket cleanup inafter()instead of relying on the last test.Bucket cleanup is currently done only at the end of the last test (
test_s3_bucket_policy_iam_user.jslines 470–478). If that test is skipped or fails before reaching the cleanup, bucketsBKT,BKT_B, andBKT_C(and their policies) are left behind. Theafterhook, meanwhile, cleans IAM users and accounts but doesn’t touch any buckets.For more predictable teardown and to decouple behavior from test order, consider moving the bucket deletions (and any required object deletions) into the
after()hook and making them best-effort (ignoreNoSuchBucket/NoSuchKey):mocha.after(async function() { this.timeout(60000); // eslint-disable-line no-invalid-this - // Clean up accounts + // Clean up buckets (best effort) + await s3_owner.deleteBucket({ Bucket: BKT }).catch(() => null); + await s3_owner.deleteBucket({ Bucket: BKT_C }).catch(() => null); + await s3_account_b.deleteBucket({ Bucket: BKT_B }).catch(() => null); + + // Clean up accounts @@ - await rpc_client.account.delete_account({ email: user_a }); - await rpc_client.account.delete_account({ email: user_b }); + await rpc_client.account.delete_account({ email: user_a }).catch(() => null); + await rpc_client.account.delete_account({ email: user_b }).catch(() => null); }); @@ - await s3_owner.deleteBucket({ - Bucket: BKT, - }); - await s3_owner.deleteBucket({ - Bucket: BKT_C, - }); - await s3_account_b.deleteBucket({ - Bucket: BKT_B, - });This keeps the tests focused on behavior while teardown consistently cleans all artifacts.
Also applies to: 470-478
20-29: Use AWS errorname(code) inassert_throws_asyncinstead ofmessage, and align the invalid-principal test with S3’s error code.AWS SDK v3 surfaces the service error code via
err.name(for example,AccessDenied,NoSuchBucket,MalformedPolicy), whileerr.messageis human-readable text that can vary and is not ideal for asserting behavior. (aws.amazon.com) For the “invalid principal in policy” case, S3 returns an error code ofMalformedPolicywith a message like “Invalid principal in policy”. (stackoverflow.com)Right now
assert_throws_asynccompareserr.messageto'Access Denied', and the bucket-policy-invalid-principal test passes'Invalid principal in policy'as the expected value. This can:
- Treat unrelated failures that happen to reuse that message as successes.
- Break if AWS adjusts wording while keeping the same error code.
Consider changing the helper to assert on
err.nameand updating the invalid-principal test to useMalformedPolicy:-async function assert_throws_async(promise, expected_message = 'Access Denied') { +async function assert_throws_async(promise, expected_error_name = 'AccessDenied') { try { await promise; - assert.fail('Test was supposed to fail with ' + expected_message); + assert.fail('Test was supposed to fail with ' + expected_error_name); } catch (err) { - if (err.message !== expected_message) { + if (err.name !== expected_error_name) { throw err; } } }And for the invalid-principal policy test:
- await assert_throws_async(s3_owner.putBucketPolicy({ - Bucket: BKT, - Policy: JSON.stringify(s3_policy) - }), 'Invalid principal in policy'); + await assert_throws_async(s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }), 'MalformedPolicy');This keeps the tests anchored to stable error codes rather than fragile message strings.
Also applies to: 366-369
372-421: Use the shared async-throw helper anderr.namefor the “IAM policy deny” test instead oferr.Code.In the “Should fail: IAM policy deny all the access for IAM user” test, the negative assertion is implemented via a manual try/catch that checks
err.Code === 'AccessDenied'. For AWS SDK v3, the service error code is exposed aserr.name(for example,AccessDenied), not asCodeorcode. (aws.amazon.com) Relying onerr.Coderisks rethrowing the expectedAccessDeniederror or silently accepting a different error shape.You already have a shared
assert_throws_asynchelper; once it’s updated to compareerr.name, this test can be simplified and made more robust:- // IAM policy deny all the access - try { - await s3_user_b.putObject({ - Body: BODY, - Bucket: BKT_B, - Key: KEY, - }); - } catch (err) { - if (err.Code !== 'AccessDenied') { - throw err; - } - } + // IAM policy Deny should block access despite bucket policy Allow + await assert_throws_async(s3_user_b.putObject({ + Body: BODY, + Bucket: BKT_B, + Key: KEY, + }), 'AccessDenied');This keeps all negative S3/IAM assertions consistent and avoids depending on nonstandard error fields.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/system_services/account_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js(1 hunks)src/test/utils/index/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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/utils/index/index.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js
🧬 Code graph analysis (1)
src/test/utils/index/index.js (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy_iam_user.js (4)
require(7-7)require(14-14)require(15-15)require(31-32)
⏰ 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/test/utils/index/index.js (1)
112-114: New IAM/S3 bucket policy integration test is correctly wired into the loader.Requiring
../../integration_tests/api/s3/test_s3_bucket_policy_iam_userhere ensures the new IAM user–bucket policy scenarios run with the rest of the integration suite, aligning with the guideline that changes are backed by tests undersrc/test/**. No issues with this addition.
|
@naveenpaul1 could you update the PR title? |
naveenpaul1
changed the title
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_user_integration_test
branch
from
e03a12d to
8192e3d
Compare
3 participants
Describe the Problem
Added integration test for IAM user and policies
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
GAP: This test contains only containarized related test case. Will add NC in upcoming PR
Summary by CodeRabbit
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.