Fix - list_buckets allowing unauthorized bucket access #9272
Conversation
aayushchouhan09
requested review from
liranmauda,
naveenpaul1,
nimrod-becker and
shirady
WalkthroughReplaces per-bucket inline permission checks in ListBuckets with async ownership-based evaluation; adds ownership helper functions and binds a request-level ownership checker; adds integration tests validating ListBuckets visibility and tightens STS ListBuckets assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BucketService as ListBuckets
participant Auth as AuthServer
Note over BucketService: Old flow — inline per-bucket permission check
Client->>BucketService: list_buckets(req)
BucketService->>Auth: has_s3_bucket_permission(bucket) [sync per-bucket]
Auth-->>BucketService: allow/deny
BucketService-->>Client: filtered list
Note over BucketService: New flow — async ownership-based mapping
Client->>BucketService: list_buckets(req)
rect rgba(150,200,150,0.06)
loop for each bucket (async P.map)
BucketService->>Auth: req.has_bucket_ownership_permission(bucket)
alt bucket.deleting
Auth-->>BucketService: null
else ownership/permission ok
Auth-->>BucketService: bucket
end
end
end
BucketService->>BucketService: remove nulls, sort by name
BucketService-->>Client: filtered list
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)
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 |
aayushchouhan09
force-pushed
the
list-fix
branch
from
70272ee to
ebd6434
Compare
![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/server/common_services/auth_server.js (1)
548-549: Fixbucket_claim_ownercomparison in auth_server.js:549.The schema defines
bucket_claim_owneras an ObjectID (account_schema.js:74), and it's stored as a bucket's _id (account_util.js:81). However, auth_server.js:549 incorrectly accesses.name.unwrap()on it—ObjectIDs don't have a.nameproperty.The comparison should use
bucket._idinstead:const is_owner = (bucket.owner_account.email.unwrap() === account.email.unwrap()) || (account.bucket_claim_owner && account.bucket_claim_owner.equals(bucket._id));Related bug also exists in s3_rest.js:273, which attempts to unwrap and compare the ObjectID to a bucket name string. This should also compare ObjectID to
bucket._id.
🧹 Nitpick comments (1)
src/server/common_services/auth_server.js (1)
541-544: Good defensive validation, but consider log ordering.The input validation correctly prevents NPE issues on lines 539 and 546. However, the debug log at line 539 won't execute when the account is missing, which means some diagnostic context is lost.
Consider moving line 539's log after the validation to ensure it always executes with valid data:
- dbg.log1('has_bucket_action_permission:', bucket.name, account.email, bucket.owner_account.email); - if (!account || !account.email) { dbg.warn('has_bucket_action_permission: account or account.email is missing'); return false; } + + dbg.log1('has_bucket_action_permission:', bucket.name, account.email, bucket.owner_account.email);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/common_services/auth_server.js(2 hunks)src/server/system_services/bucket_server.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/bucket_server.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/server/common_services/auth_server.js (2)
src/server/system_services/account_server.js (8)
account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)src/endpoint/s3/s3_rest.js (2)
account(254-254)is_owner(269-282)
⏰ 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 (1)
src/server/common_services/auth_server.js (1)
565-568: LGTM! Clear policy evaluation logic.The explicit branching correctly implements S3 bucket policy precedence: DENY overrides all, ALLOW grants permission, and ownership is the fallback. This is more readable than the previous implementation.
aayushchouhan09
force-pushed
the
list-fix
branch
3 times, most recently
from
aa3b486 to
550cc14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
164-167: Consider splitting the compound assertion for clearer test output.The logic is correct and validates that the test user can only list owned buckets. However, splitting into two assertions would make test failures more descriptive.
mocha.it('user should list only owned buckets', async function() { const buckets = (await s3_user.send(new ListBucketsCommand())).Buckets.map(b => b.Name); - assert(buckets.includes(user_bucket) && !buckets.includes(admin_bucket)); + assert(buckets.includes(user_bucket), `Expected test user to list owned bucket '${user_bucket}'`); + assert(!buckets.includes(admin_bucket), `Expected test user NOT to list admin bucket '${admin_bucket}'`); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/common_services/auth_server.js(2 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/system_services/bucket_server.js
- src/server/common_services/auth_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/integration_tests/api/s3/test_s3_list_buckets.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (2)
src/server/common_services/auth_server.js (3)
account(103-103)account(188-194)account(317-321)src/server/system_services/bucket_server.js (3)
buckets(1077-1080)buckets(1097-1100)buckets(1318-1318)
⏰ 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 (3)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (3)
130-162: LGTM! Well-structured test setup and teardown.The test suite correctly establishes a permission testing scenario with two accounts and two buckets with different ownership. The cleanup order (buckets before account) is correct and necessary.
169-174: LGTM! Properly validates admin permissions.The test correctly verifies that the admin user can list all buckets, including those owned by other users. The
>= 2check appropriately accounts for buckets created by other test suites.
176-200: LGTM! Comprehensive validation of dynamic policy enforcement.The test properly validates the three-phase policy lifecycle: deny by default, grant via policy, revoke via policy deletion. This directly tests the permission check fix described in the PR objectives.
Principal format
{ AWS: test_user }is correct. Policy evaluation logic insrc/endpoint/s3/s3_bucket_policy_utils.jsmatches principals by comparingprincipal.unwrap() === account, where the account identifier is the authenticated user's email. The test_user email will correctly match during evaluation.
|
@aayushchouhan09 @shirady what about bucketspace_fs ? |
@aayushchouhan09, you can fix this in this PR or in a following PR, whatever helps you - just let me know. I will share as I see what the total fix should contain:
If you want to add something |
aayushchouhan09
force-pushed
the
list-fix
branch
from
550cc14 to
9759849
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
172-175: Admin test correctly validates visibility of all buckets.The test appropriately uses
includes()rather than strict equality, since the admin should see all system buckets (including the 8 buckets from earlier pagination tests). This validates that the fix preserves admin's ability to list all buckets.Optional refinement: Consider verifying the exact count of buckets to catch unexpected side effects:
const buckets = (await s3.send(new ListBucketsCommand())).Buckets.map(b => b.Name); -assert(buckets.includes('bucket-a') && buckets.includes('bucket-b') && buckets.includes('admin-buck')); +assert(buckets.includes('bucket-a') && buckets.includes('bucket-b') && buckets.includes('admin-buck')); +assert.strictEqual(buckets.length, 11, 'Admin should see all 11 buckets (8 from pagination + 3 from permissions)');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/system_services/bucket_server.js
- src/server/common_services/auth_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/integration_tests/api/s3/test_s3_list_buckets.js
🧠 Learnings (2)
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
📚 Learning: 2025-09-30T08:56:55.478Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
🧬 Code graph analysis (1)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (2)
src/server/common_services/auth_server.js (3)
account(103-103)account(188-194)account(317-321)src/server/system_services/bucket_server.js (5)
s3(459-459)s3(1311-1311)buckets(1074-1077)buckets(1094-1097)buckets(1315-1315)
🔇 Additional comments (4)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (4)
135-147: LGTM! Helper function is well-structured.The helper correctly creates test accounts with S3 access and returns properly configured S3 clients. The approach of spreading
s3_client_paramsand overriding only credentials is clean and maintains consistency with the parent client configuration.
149-155: Excellent setup addressing previous feedback.The setup properly creates two separate accounts with their own buckets, plus an admin bucket. This addresses the previous review comment requesting multiple accounts to verify each account sees only its own buckets.
157-163: Proper cleanup sequence.The cleanup correctly deletes buckets before accounts, preventing resource leaks and ensuring test isolation.
165-170: Core authorization test looks correct.This test directly validates the fix by ensuring accounts can only list their owned buckets. The use of
deepStrictEqualensures that accounts don't see unauthorized buckets (including the 8 buckets from earlier pagination tests), which would have been visible before this PR's fix.
aayushchouhan09
force-pushed
the
list-fix
branch
from
9759849 to
bc6baaa
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 (4)
src/sdk/bucketspace_fs.js(3 hunks)src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/common_services/auth_server.js
- src/server/system_services/bucket_server.js
- src/test/integration_tests/api/s3/test_s3_list_buckets.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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/bucketspace_fs.js
🧬 Code graph analysis (1)
src/sdk/bucketspace_fs.js (2)
src/endpoint/s3/s3_rest.js (3)
bucket(347-347)bucket(369-369)account(254-254)src/server/common_services/auth_server.js (3)
owner(594-594)bucket_policy(595-595)bucket_policy(625-625)
⏰ 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 (4)
src/sdk/bucketspace_fs.js (4)
274-276: LGTM! Core fix properly addresses the unauthorized access issue.The synchronous permission check correctly prevents unauthorized bucket visibility in ListBuckets. By checking ownership before the async filesystem validation, this resolves the original bug where async filter predicates were not awaited.
937-951: LGTM! Ownership checks correctly handle both root and IAM accounts.The method properly validates:
- Direct ownership via account ID comparison
- IAM user ownership by checking if the IAM user's root owner matches the bucket owner
Based on learnings,
account.owneris already a string (account ID), so the comparison on line 948 is type-safe.
953-971: LGTM! Synchronous ownership-only check is correct for ListBuckets.The method correctly:
- Uses synchronous checks to avoid the async filter bug
- Limits ListBuckets visibility to system owners and bucket owners
- Excludes bucket policy evaluation (which would require async checks)
The TODO on line 968 appropriately flags future IAM policy support for the
s3:ListAllMyBucketsaction.
973-1012: LGTM! Refactoring improves maintainability and correctness.The changes properly:
- Use new helper methods for cleaner ownership checks
- Return early for system owners (line 977)
- Line 983: Behavior change – now returns owner status when no bucket policy exists (previously would deny). This is correct: bucket owners should have access regardless of policy presence.
- Combine ownership and policy permissions with OR logic (line 1011)
aayushchouhan09
force-pushed
the
list-fix
branch
from
bc6baaa to
a7435d9
Compare
aayushchouhan09
force-pushed
the
list-fix
branch
from
da56748 to
2bf50a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
172-175: Fix minor grammar in test nameThe test title should say “admin should list all the buckets” (singular list), not “lists”.
- mocha.it('admin should lists all the buckets', async function() { + mocha.it('admin should list all the buckets', async function() {src/server/common_services/auth_server.js (2)
500-502: Use thesystemparameter for system-owner check in ListBuckets
has_list_bucket_permissionreceivessystemexplicitly but callsis_system_owner(bucket.system, account). For consistency and to avoid relying on how/ifbucket.systemis populated, it’s safer to use the providedsystemargument:async function has_list_bucket_permission(bucket, account, system) { - // system owner can list all the buckets - if (is_system_owner(bucket.system, account)) return true; + // system owner can list all the buckets + if (is_system_owner(system, account)) return true;
req.has_list_bucket_permissionalready passesreq.system, so this simplifies the ownership check and makes future callers clearer. [duplicate of earlier review note.]Also applies to: 604-607
554-588: Fix IAM ListBuckets evaluation to respect explicit DENY across all IAM policies
has_iam_list_buckets_permissionstill returnstrueon the first policy that yieldsALLOW(after ownership/OBC checks), without scanning remaining IAM policies for a possibleDENY. If a later policy explicitly deniess3:ListAllMyBuckets, that DENY will be ignored, which breaks AWS-style semantics where any matching DENY across all policies must win. Based on learnings.You can preserve DENY precedence and keep ownership/OBC constraints by accumulating ALLOWs and only deciding after scanning all policies:
async function has_iam_list_buckets_permission(bucket, account) { if (!account?.owner) return false; const iam_policies = account.iam_user_policies || []; if (iam_policies.length === 0) return false; - // check each IAM policy for s3:ListAllMyBuckets permission - for (const iam_policy of iam_policies) { - const result = await s3_bucket_policy_utils.has_bucket_policy_permission( - iam_policy.policy_document, - undefined, - 's3:ListAllMyBuckets', - 'arn:aws:s3:::*', - undefined, - { should_pass_principal: false } - ); - - if (result === 'DENY') return false; - - if (result === 'ALLOW') { - const bucket_owner_id = bucket.owner_account?._id; - const account_owner_id = account.owner?._id; - const ownership_match = account_owner_id !== undefined && bucket_owner_id !== undefined && - String(bucket_owner_id) === String(account_owner_id); - - // special case: check if root is OBC account and this is the claimed bucket - const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap(); - const obc_claim_match = root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap(); - - if (ownership_match || obc_claim_match) return true; - } - } - - return false; + const bucket_owner_id = bucket.owner_account?._id; + const account_owner_id = account.owner?._id; + const ownership_match = + account_owner_id !== undefined && + bucket_owner_id !== undefined && + String(bucket_owner_id) === String(account_owner_id); + + // special case: check if root is OBC account and this is the claimed bucket + const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap(); + const obc_claim_match = root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap(); + + let allow_found = false; + // check each IAM policy for s3:ListAllMyBuckets permission + for (const iam_policy of iam_policies) { + const result = await s3_bucket_policy_utils.has_bucket_policy_permission( + iam_policy.policy_document, + undefined, + 's3:ListAllMyBuckets', + 'arn:aws:s3:::*', + undefined, + { should_pass_principal: false } + ); + + if (result === 'DENY') return false; + if (result === 'ALLOW') allow_found = true; + } + + return allow_found && (ownership_match || obc_claim_match); }This ensures:
- Any DENY in any IAM policy immediately blocks access.
- At least one ALLOW is required.
- Ownership/OBC constraints are enforced only after the policy scan.
🧹 Nitpick comments (1)
src/test/integration_tests/api/sts/test_sts.js (1)
573-575: Avoid coupling STS ListBuckets tests to bucket orderingBoth tests assert
buckets1.Buckets[0].Name === alice2_buck, which ties them to the current sort order and to alice2 owning no other lexicographically earlier buckets.To make the tests robust to additional alice2-owned buckets or internal ordering changes, consider asserting by membership instead of index:
- const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); - assert.ok(buckets1.Buckets[0].Name === alice2_buck); + const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); + const names = buckets1.Buckets.map(b => b.Name); + assert.ok(names.includes(alice2_buck));Apply similarly in the second test.
Also applies to: 598-600
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/bucket_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/integration_tests/api/sts/test_sts.jssrc/test/integration_tests/api/s3/test_s3_list_buckets.js
🧠 Learnings (6)
📚 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/common_services/auth_server.js
📚 Learning: 2025-11-19T15:03:42.231Z
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.231Z
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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.js
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
📚 Learning: 2025-09-30T08:56:55.478Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
🧬 Code graph analysis (2)
src/server/common_services/auth_server.js (1)
src/endpoint/s3/s3_rest.js (8)
req(237-243)req(415-415)is_system_owner(268-268)account(255-255)account(339-339)iam_policies(345-345)s3_bucket_policy_utils(10-10)is_owner(271-284)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (3)
src/test/integration_tests/api/sts/test_sts.js (8)
mocha(9-9)account(107-107)coretest(5-5)coretest(76-76)coretest(458-458)coretest(814-814)s3(233-239)assert(10-10)src/server/common_services/auth_server.js (3)
account(104-104)account(189-195)account(318-322)src/server/system_services/bucket_server.js (5)
s3(461-461)s3(1346-1346)buckets(1109-1112)buckets(1129-1132)buckets(1350-1350)
⏰ 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 (3)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
130-170: Solid coverage for per-account and admin ListBuckets behaviorCreating two separate accounts with their own buckets plus an admin bucket, and asserting per-account isolation and admin visibility, exercises exactly the permission model this PR is changing. Setup/teardown look correct and avoid leaking test state.
src/test/integration_tests/api/sts/test_sts.js (1)
460-552: Dedicated alice2 bucket for STS/ListBuckets looks correctDefining
alice2_buck, creating it in thebeforehook with alice2’s S3 client, and deleting it inaftercleanly isolates these STS tests from any bucket-policy behavior onfirst.bucket. This matches the intention that ListBuckets should depend on ownership/IAM, not bucket policy, and the teardown order (bucket before accounts) is correct.src/server/common_services/auth_server.js (1)
527-545: Ownership helpers and unified owner logic improve clarityExtracting
is_system_owner/is_bucket_ownerand using a singleownerflag (direct owner, IAM-root owner, or OBC claim) inhas_bucket_action_permissionmakes the permission model much easier to follow and aligns with the behavior described in s3_rest (system owner, bucket owner, IAM inheritance, OBC). The fallbackreturn ownerwhen there is no bucket policy also keeps non-policy behavior explicit.Also applies to: 640-651, 654-671
aayushchouhan09
force-pushed
the
list-fix
branch
from
2bf50a3 to
1f53a03
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
172-175: Fix test description grammarMinor nit: the test name should use “list” rather than “lists”:
- mocha.it('admin should lists all the buckets', async function() { + mocha.it('admin should list all the buckets', async function() {
🧹 Nitpick comments (2)
src/server/system_services/bucket_server.js (1)
1118-1125: Optional: make continuation token handling resilient to deleted bucketsIf a bucket corresponding to
continuation_tokenis deleted between paginated calls, the current>=+index + 1logic can skip the next bucket in lexicographic order. You can avoid that by finding the first bucket strictly greater than the token and not incrementing the index:- if (continuation_token) { - const index = accessible_bucket_list.findIndex( - bucket => bucket.name.unwrap().localeCompare(continuation_token.unwrap()) >= 0 - ); - if (index !== -1) { - next_index = index + 1; - } - } + if (continuation_token) { + const token = continuation_token.unwrap(); + const index = accessible_bucket_list.findIndex( + bucket => bucket.name.unwrap().localeCompare(token) > 0 + ); + if (index !== -1) { + next_index = index; + } + }This preserves current behavior when the token bucket still exists, while avoiding a possible skip when it was removed mid-iteration.
src/test/integration_tests/api/sts/test_sts.js (1)
460-460: Session token ListBuckets checks now correctly rely on ownershipCreating
alice2_buckand asserting thatlistBuckets()from STS-derived credentials returns that bucket validates the new “account-owned buckets only” behavior without relying on bucket policy onfirst.bucket. The setup/teardown is consistent and keeps the test self-contained.If you want to make the assertions more robust to future additions of alice2-owned buckets, you could assert on the full set of names:
- const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); - assert.ok(buckets1.Buckets[0].Name === alice2_buck); + const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); + const names1 = buckets1.Buckets.map(b => b.Name).sort(); + assert.deepStrictEqual(names1, [alice2_buck]);(and similarly in the second test).
Also applies to: 471-471, 549-552, 573-575, 598-600
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/common_services/auth_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/integration_tests/api/s3/test_s3_list_buckets.jssrc/test/integration_tests/api/sts/test_sts.jssrc/test/integration_tests/nsfs/test_nsfs_integration.js
🧠 Learnings (3)
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
📚 Learning: 2025-09-30T08:56:55.478Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.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/server/system_services/bucket_server.js
🧬 Code graph analysis (3)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (3)
src/test/integration_tests/api/sts/test_sts.js (6)
account(107-107)coretest(5-5)coretest(76-76)coretest(458-458)coretest(814-814)assert(10-10)src/server/common_services/auth_server.js (3)
account(104-104)account(189-195)account(318-322)src/server/system_services/bucket_server.js (3)
buckets(1109-1112)buckets(1129-1132)buckets(1350-1350)
src/test/integration_tests/nsfs/test_nsfs_integration.js (2)
src/server/system_services/bucket_server.js (23)
res(463-464)res(1349-1349)res(1479-1481)res(1949-1952)buckets(1109-1112)buckets(1129-1132)buckets(1350-1350)bucket(350-350)bucket(364-364)bucket(383-383)bucket(401-401)bucket(426-426)bucket(438-438)bucket(453-453)bucket(489-489)bucket(498-498)bucket(511-511)bucket(551-551)bucket(577-577)bucket(596-596)bucket(610-610)bucket(628-628)bucket(642-642)src/server/system_services/account_server.js (11)
res(513-515)res(736-740)accounts(468-480)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)
src/server/system_services/bucket_server.js (1)
src/server/system_services/account_server.js (4)
P(4-4)system_store(17-17)req(134-134)req(397-397)
⏰ 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: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
src/server/system_services/bucket_server.js (1)
1098-1104: Async ListBuckets permission filtering looks correctUsing
P.mapwith an async predicate and returningbucket | nullthen filtering solves the earlier async.filter()issue and avoids races on a shared array, while delegating all auth logic toreq.has_list_bucket_permission. Excludingbucket.deletingup front and sorting byname.unwrap()keeps behavior deterministic and aligned with the new ownership/IAM model.src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
130-175: Good coverage for ListBuckets ownership semanticsThe new
list_buckets permissionssuite cleanly validates that:
- Regular accounts only see buckets they own.
- The admin client can see all buckets (using
includesso it’s robust to other buckets).This aligns well with the updated
list_bucketsauthorization logic and gives solid regression coverage for the bug this PR fixes.src/test/integration_tests/nsfs/test_nsfs_integration.js (2)
252-258: NSFS ListBuckets tests now validate ownership-only semanticsThe updated tests across the NSFS suites consistently reflect the new behavior:
bucket-wrong-uid/bucket-correct-uidcases verify that each NSFS account only sees buckets it owns, regardless of filesystem permissions.- The “uid & gid mismatch/match” list-bucket tests in
list objects - namespace_fsconfirm that:
- Accounts with no owned buckets list nothing.
- Accounts see their own S3-created buckets but not others.
- The
nsfs account configurationsandlist buckets - namespace_fssuites now assert per-account bucket visibility instead of relying on bucket policies.These changes give broad coverage across NSFS scenarios and will catch regressions if
list_bucketsever starts honoring bucket policies again.Also applies to: 525-532, 1073-1077, 1117-1122, 1124-1128, 1137-1142, 1306-1307, 1326-1327, 1345-1346, 1399-1400, 1418-1419, 1669-1676
390-401: Scoped GetBucketAcl permission on first.bucket is appropriateGranting
s3:GetBucketAclonfirst_bucketspecifically to theaccount_s3_correct_uidprincipal before callinggetBucketAclaligns with AWS-style semantics and explains the previously failing test. The policy scope is minimal (onlyGetBucketAclon that bucket), which keeps the test focused on ACL retrieval rather than broader access.
aayushchouhan09
force-pushed
the
list-fix
branch
from
1f53a03 to
98a4af7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
1069-1071: Teardown correctly deletesfirst_bucketpolicySwitching from putting an empty policy to
delete_bucket_policy({ name: first_bucket })is the correct cleanup and avoids invalid-policy errors in teardown.src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
172-175: Fix grammar in test name
'admin should lists all the buckets'should be'admin should list all the buckets'to keep test names clear and consistent.- mocha.it('admin should lists all the buckets', async function() { + mocha.it('admin should list all the buckets', async function() {src/server/common_services/auth_server.js (1)
527-588: Fix IAM ListBuckets evaluation and use the providedsystemparameterTwo related issues in the new ListBuckets helpers:
- IAM policy evaluation can ignore a later DENY
has_iam_list_buckets_permissionreturnstrueon the first policy that yieldsALLOW(and passes ownership/OBC checks):for (const iam_policy of iam_policies) { const result = await ...; if (result === 'DENY') return false; if (result === 'ALLOW') { // ownership_match / obc_claim_match if (ownership_match || obc_claim_match) return true; } }If an ALLOW policy appears earlier in
account.iam_user_policiesthan a conflicting DENY for the same action, the function will allow access without ever seeing the DENY. That breaks the usual “DENY overrides ALLOW across policies” expectation and could re‑expose unauthorized bucket listing in some configurations.You can preserve DENY precedence and still enforce ownership/OBC constraints by scanning all policies, short‑circuiting only on DENY, and deferring the final allow decision to after the loop:
async function has_iam_list_buckets_permission(bucket, account) { if (!account?.owner) return false; - const iam_policies = account.iam_user_policies || []; - if (iam_policies.length === 0) return false; - - // check each IAM policy for s3:ListAllMyBuckets permission - for (const iam_policy of iam_policies) { - const result = await s3_bucket_policy_utils.has_bucket_policy_permission( - iam_policy.policy_document, - undefined, - 's3:ListAllMyBuckets', - 'arn:aws:s3:::*', - undefined, - { should_pass_principal: false } - ); - - if (result === 'DENY') return false; - - if (result === 'ALLOW') { - const bucket_owner_id = bucket.owner_account?._id; - const account_owner_id = account.owner?._id; - const ownership_match = account_owner_id !== undefined && bucket_owner_id !== undefined && - String(bucket_owner_id) === String(account_owner_id); - - // special case: check if root is OBC account and this is the claimed bucket - const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap(); - const obc_claim_match = root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap(); - - if (ownership_match || obc_claim_match) return true; - } - } - - return false; + const iam_policies = account.iam_user_policies || []; + if (iam_policies.length === 0) return false; + + const bucket_owner_id = bucket.owner_account?._id; + const account_owner_id = account.owner?._id; + const ownership_match = + account_owner_id !== undefined && + bucket_owner_id !== undefined && + String(bucket_owner_id) === String(account_owner_id); + + // special case: check if root is OBC account and this is the claimed bucket + const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap(); + const obc_claim_match = + root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap(); + + let allow_found = false; + for (const iam_policy of iam_policies) { + const result = await s3_bucket_policy_utils.has_bucket_policy_permission( + iam_policy.policy_document, + undefined, + 's3:ListAllMyBuckets', + 'arn:aws:s3:::*', + undefined, + { should_pass_principal: false } + ); + + if (result === 'DENY') return false; + if (result === 'ALLOW') allow_found = true; + } + + if (!allow_found) return false; + return ownership_match || obc_claim_match; }This way a single DENY in any policy always wins, while ALLOW still requires both a matching policy and correct ownership/OBC.
- Use
systemparameter instead ofbucket.system
has_list_bucket_permissionreceivessystembut calls:if (is_system_owner(bucket.system, account)) return true;Relying on
bucket.systemrather than the providedsystemcan fail when the caller fully populatessystemon the request but the bucket object has no or stalesystemreference.Prefer using the explicit parameter:
async function has_list_bucket_permission(bucket, account, system) { - // system owner can list all the buckets - if (is_system_owner(bucket.system, account)) return true; + // system owner can list all the buckets + if (is_system_owner(system, account)) return true;Together these changes tighten ListBuckets authorization and avoid surprising behavior when multiple IAM policies are attached or when
bucket.systemis missing/stale.Also applies to: 604-619
🧹 Nitpick comments (2)
src/test/integration_tests/api/sts/test_sts.js (1)
573-575: Make ListBuckets assertions resilient to additional bucketsBoth tests assert
buckets1.Buckets[0].Name === alice2_buck. This will fail if the account ever legitimately owns more than one bucket andalice2_buckis not first.Consider asserting on the set of bucket names instead:
- const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); - assert.ok(buckets1.Buckets[0].Name === alice2_buck); + const buckets1 = await temp_s3_with_session_token.listBuckets().promise(); + const names = buckets1.Buckets.map(b => b.Name); + assert.deepStrictEqual(names, [alice2_buck]);This still guarantees the correct visibility while remaining robust to future changes in bucket ordering or additional buckets.
Also applies to: 598-600
src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
390-401: Verify NC principal format for GetBucketAcl bucket policyGranting explicit
s3:GetBucketAclonfirst_buckettoaccount_s3_correct_uidis the right way to fix this ACL test.For the NC path you currently use:
account_principal = 'account_s3_correct_uid';Elsewhere in this file, NC principals are usually emails (e.g.,
generate_s3_policyand the policy evaluation path indeed interpret'account_s3_correct_uid'correctly in NC; if they key off email, you may need'[email protected]'here for the NC branch.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/bucket_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/integration_tests/api/sts/test_sts.jssrc/test/integration_tests/nsfs/test_nsfs_integration.jssrc/test/integration_tests/api/s3/test_s3_list_buckets.js
🧠 Learnings (6)
📚 Learning: 2025-11-19T15:03:42.231Z
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.231Z
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/common_services/auth_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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.jssrc/test/integration_tests/nsfs/test_nsfs_integration.js
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
📚 Learning: 2025-09-30T08:56:55.478Z
Learnt from: Neon-White
Repo: noobaa/noobaa-core PR: 9229
File: .github/workflows/ibm-nightly-provision-dispatcher.yaml:13-13
Timestamp: 2025-09-30T08:56:55.478Z
Learning: In the noobaa-core repository, PR #9229 (nightly IBM VM provision dispatcher) has a dependency on `.github/ibm-warp-runner-config.yaml` which is provided in PR #9230, requiring PR #9230 to be merged first.
Applied to files:
src/test/integration_tests/api/s3/test_s3_list_buckets.js
🧬 Code graph analysis (3)
src/server/common_services/auth_server.js (2)
src/endpoint/s3/s3_rest.js (6)
req(237-243)req(415-415)account(255-255)account(339-339)s3_bucket_policy_utils(10-10)is_owner(271-284)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
src/test/integration_tests/nsfs/test_nsfs_integration.js (3)
src/server/system_services/bucket_server.js (23)
res(463-464)res(1349-1349)res(1479-1481)res(1949-1952)buckets(1109-1112)buckets(1129-1132)buckets(1350-1350)bucket(350-350)bucket(364-364)bucket(383-383)bucket(401-401)bucket(426-426)bucket(438-438)bucket(453-453)bucket(489-489)bucket(498-498)bucket(511-511)bucket(551-551)bucket(577-577)bucket(596-596)bucket(610-610)bucket(628-628)bucket(642-642)src/server/system_services/account_server.js (12)
res(513-515)res(736-740)iam_utils(26-26)accounts(468-480)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (3)
src/test/integration_tests/nsfs/test_nsfs_integration.js (8)
account(262-262)account(506-506)account(1051-1053)account(1624-1625)account(1671-1671)coretest(41-41)coretest(42-42)buckets(1673-1673)src/server/common_services/auth_server.js (3)
account(104-104)account(189-195)account(318-322)src/server/system_services/bucket_server.js (5)
s3(461-461)s3(1346-1346)buckets(1109-1112)buckets(1129-1132)buckets(1350-1350)
⏰ 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 (8)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
130-171: Good isolated coverage for per-account ListBuckets permissionsThe new
list_buckets permissionssuite cleanly creates per-account clients and buckets, verifies that each account only sees its own bucket, and that admin sees all; setup/teardown look symmetric and should not leak buckets or accounts into other tests.src/test/integration_tests/api/sts/test_sts.js (1)
460-471: Isolating ListBuckets from bucket policy by using an owned bucket looks goodCreating
alice2_buckowned byalice2and cleaning it up in thebefore/afterhooks decouples these STS tests fromfirst.bucketpolicy side effects and aligns them with ownership-based ListBuckets behavior.Also applies to: 549-552
src/test/integration_tests/nsfs/test_nsfs_integration.js (4)
251-258: Using ephemeral buckets for uid/gid ListBuckets tests is soundCreating
bucket-wrong-uid/bucket-correct-uid, asserting their presence viabucket_in_list, and deleting them in the same test keeps these uid/gid ListBuckets checks self-contained and avoids polluting other suites.Also applies to: 525-532
1073-1077: NSFS ListBuckets tests now correctly assert ownership-based visibilityThe new/updated tests around uid/gid mismatch/match validate that:
- Accounts without owned buckets see none.
- Accounts list only the buckets they own (via S3 or CLI).
- Root-style account can see its own bucket among others.
This matches the new behavior where ListBuckets ignores bucket policy and filters by ownership.
Also applies to: 1117-1122, 1124-1128, 1137-1142
1306-1307: Updatedbucket_in_listexpectations align with policy-independent ListBucketsAdjusting these assertions so accounts only expect to see their own buckets (and not
bucket_name1/first_bucket/second.bucketdespite permissive bucket policies) is consistent with the new design where bucket policies do not affect ListBuckets results.Also applies to: 1326-1327, 1345-1346, 1399-1400, 1418-1419
1669-1676: Strong coverage that each NSFS account lists only its own bucketThe looped test over
accountsinlist buckets - namespace_fsthat asserts exactly one bucket per account and that it equalsaccount.bucketnicely validates per-account isolation for both S3-created and CLI-created buckets.src/server/common_services/auth_server.js (2)
500-502: Request helper for ListBuckets permissions is wired correctlyAdding
req.has_list_bucket_permission(bucket)that forwards tohas_list_bucket_permission(bucket, req.account, req.system)provides a clean, reusable hook for bucket_server and keeps auth logic centralized in this module.
640-651: Bucket action permissions: clarified ownership and IAM/OBC inheritanceThe refactor in
has_bucket_action_permissionthat:
- Uses
is_system_ownerfor the system-owner shortcut,- Delegates direct ownership to
is_bucket_owner,- Treats IAM users as owners when their root account owns the bucket, and
- Treats bucket-claim (OBC) owners as owners,
and then returns
ownerwhen there is no bucket policy:const owner = is_owner || has_iam_access || has_obc_access; ... if (!bucket_policy) return owner; ... return owner || result === 'ALLOW';matches the intended semantics: bucket owners (including IAM/OBC inheritance) get implicit access unless explicitly denied by policy, and non-owners rely solely on the policy result.
The use of
_idon bothbucket.owner_accountandaccount.owneris consistent with how these objects are loaded fromsystem_store.datain this module. Based on learnings.Also applies to: 654-654, 668-671
aayushchouhan09
force-pushed
the
list-fix
branch
from
98a4af7 to
1119012
Compare
| assert.ok(res.Buckets.length >= 1); | ||
| assert.ok(res.Buckets.some(b => b.Name === s3_root_b_name)); |
There was a problem hiding this comment.
If you can add a comment here, it can help
aayushchouhan09
force-pushed
the
list-fix
branch
from
1119012 to
bdb207f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/integration_tests/api/s3/test_s3_list_buckets.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/sts/test_sts.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/common_services/auth_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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.jssrc/server/system_services/bucket_server.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: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/server/system_services/bucket_server.js (1)
1099-1105: LGTM! Properly handles async permission checks.The refactored approach using
P.mapwith an async permission predicate correctly awaits all bucket permission checks before filtering. This addresses the previous race condition concern whereArray.prototype.filter()with async predicates would not await promises, causing permission checks to be bypassed.src/test/integration_tests/api/sts/test_sts.js (1)
460-460: LGTM! Test updates align with ownership-based ListBuckets behavior.The changes correctly reflect the new ListBuckets implementation:
- Creating
alice2_buckowned by alice2 ensures ListBuckets returns expected buckets based on ownership (not bucket policy)- Specific bucket name assertions (
buckets1.Buckets[0].Name === alice2_buck) are more precise than generic length checks- The comment at line 550 correctly notes that bucket policy is unrelated to ListBuckets operation, which is AWS-compliant behavior
Also applies to: 470-471, 548-551, 574-574, 599-599
src/server/common_services/auth_server.js (1)
605-635: LGTM! Properly refactored to use ownership helpers.The refactored permission logic correctly:
- Uses
is_system_ownerfor system-level access checks- Combines direct ownership and OBC ownership into
has_owner_access- Properly handles IAM user access via root account ownership comparison (lines 616-617 correctly use
account.owner._idas an ID reference)- Returns
has_owner_access || result === 'ALLOW'when bucket policy exists, maintaining the precedence where owners always have access regardless of policy
aayushchouhan09
force-pushed
the
list-fix
branch
from
bdb207f to
ea65377
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/server/common_services/auth_server.js (1)
580-584: Add null check after loading the owner account.If
account.owner._idis invalid or the owner account has been deleted,system_store.data.get_by_id()may returnundefined. Passingundefinedtois_bucket_ownerwill cause a runtime error when it tries to access properties.Apply this diff:
// special case: iam user can list the buckets of their owner if (account.owner) { const account_owner = system_store.data.get_by_id(account.owner._id); - if (is_bucket_owner(bucket, account_owner)) return true; + if (account_owner && is_bucket_owner(bucket, account_owner)) return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/integration_tests/api/s3/test_s3_list_buckets.js
- src/server/system_services/bucket_server.js
- src/test/integration_tests/api/sts/test_sts.js
🧰 Additional context used
🧠 Learnings (5)
📚 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/common_services/auth_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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.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/server/common_services/auth_server.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-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/server/common_services/auth_server.js (1)
500-502: LGTM!The request helper binding follows the established pattern in this file and correctly binds the account from the request context.
aayushchouhan09
force-pushed
the
list-fix
branch
from
ea65377 to
b612a41
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/server/common_services/auth_server.js (3)
537-545: Add defensive null/undefined checks to prevent runtime errors.The function accesses nested properties without guards. If
bucket.owner_account,bucket.owner_account.email, oraccount.emailis undefined, this will throw a TypeError at runtime.Apply this diff to add defensive checks:
function is_bucket_owner(bucket, account) { + if (!bucket?.owner_account?.email || !account?.email) return false; return bucket.owner_account.email.unwrap() === account.email.unwrap(); }
547-555: Add defensive check for bucket.name to prevent runtime errors.The function guards
account.bucket_claim_ownerbut notbucket.name. Ifbucket.nameis undefined, theunwrap()call will throw a TypeError at runtime.Apply this diff:
function is_bucket_claim_owner(bucket, account) { + if (!account?.bucket_claim_owner?.name || !bucket?.name) return false; - return account.bucket_claim_owner && account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap(); + return account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap(); }
527-535: Add defensive null/undefined checks to prevent runtime errors.The function directly accesses nested properties without guards. If
bucket.system,bucket.system.owner,bucket.system.owner.email, oraccount.emailis undefined, this will throw a TypeError at runtime.Apply this diff to add defensive checks:
function is_system_owner(bucket, account) { + if (!bucket?.system?.owner?.email || !account?.email) return false; return bucket.system.owner.email.unwrap() === account.email.unwrap(); }
🧹 Nitpick comments (1)
src/server/common_services/auth_server.js (1)
581-584: Consider adding defensive check for account.owner._id.While the code correctly loads the owner account from system_store (resolving the previous critical issue), it assumes
account.owner._idexists. Although based on learningsaccount.ownershould always have an._idproperty in this module, adding a defensive check would prevent potential runtime errors if the data is corrupted.Apply this diff for more defensive handling:
// special case: iam user can list the buckets of their owner if (account.owner) { - const account_owner = system_store.data.get_by_id(account.owner._id); + const owner_id = account.owner._id || account.owner.id; + if (!owner_id) return false; + const account_owner = system_store.data.get_by_id(owner_id); if (account_owner && is_bucket_owner(bucket, account_owner)) return true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/bucket_server.js
🧰 Additional context used
🧠 Learnings (5)
📚 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/common_services/auth_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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.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/server/common_services/auth_server.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-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (2)
src/server/common_services/auth_server.js (2)
500-502: LGTM!The request helper correctly wraps the ownership permission check with the request's account. The function name is properly spelled (previous "owership" typo has been resolved).
605-639: LGTM!The function correctly:
- Uses proper parameters for
is_system_owner(bucket, account)(previously flagged issue resolved)- Checks ownership through the new
has_owner_accessvariable- Safely handles IAM users by checking
account.owner !== undefinedbefore accessing nested properties- Implements correct logic: owners have access regardless of bucket policy, and non-owners must have bucket policy ALLOW
|
|
||
| // special case: iam user can list the buckets of their owner | ||
| if (account.owner) { | ||
| const account_owner = system_store.data.get_by_id(account.owner._id); |
There was a problem hiding this comment.
I would try to avoid the system_store call and reuse what we have, this is a reference that might help:
const is_iam_and_same_root_account_owner = account.owner !== undefined &&
account.owner._id.toString() === bucket.owner_account._id.toString();
There was a problem hiding this comment.
We will create a separate PR for this fix, added a comment to add this fix later. Thanks
aayushchouhan09
force-pushed
the
list-fix
branch
from
b612a41 to
f6e31c9
Compare
| // system owner can list all the buckets | ||
| if (is_system_owner(bucket, account)) return true; |
There was a problem hiding this comment.
@aayushchouhan09
I think that for the command noobaa bucket list in the CLI that you mentioned in the call, you need an additional condition.
In the help of the CLI:
List NooBaa buckets
Otherwise, it will list only the buckets under the operator account - which are the OBC buckets that were created with the command noobaa obc create <obc-name>.
cc: @naveenpaul1
There was a problem hiding this comment.
Added a fix for it. Thanks!
|
@aayushchouhan09, you can also link this issue #8931 in your PR as you also fixed this bug. |
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
list-fix
branch
from
f6e31c9 to
0e5ffbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/common_services/auth_server.js (1)
575-592: Function is async but performs no async operations.The
has_bucket_ownership_permissionfunction is marked asasyncand returnsPromise<boolean>per the JSDoc, but it doesn't useawaitanywhere. This is likely intentional for future IAM user handling (per the TODO on lines 588-590), but currently adds unnecessary Promise wrapping overhead.Consider making this a synchronous function until IAM user async handling is actually implemented:
-async function has_bucket_ownership_permission(bucket, account, role) { +function has_bucket_ownership_permission(bucket, account, role) {And update the JSDoc return type to
boolean. This can be changed back to async when IAM user handling is added.Alternatively, if keeping it async for consistency with other permission functions, add a comment explaining why.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/server/common_services/auth_server.js(4 hunks)src/server/system_services/bucket_server.js(1 hunks)src/test/integration_tests/api/s3/test_s3_list_buckets.js(1 hunks)src/test/integration_tests/api/sts/test_sts.js(5 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/integration_tests/api/sts/test_sts.js
- src/server/system_services/bucket_server.js
- src/test/integration_tests/api/s3/test_s3_list_buckets.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/nsfs/test_nsfs_integration.js
🧠 Learnings (5)
📚 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/test/integration_tests/nsfs/test_nsfs_integration.jssrc/server/common_services/auth_server.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/server/common_services/auth_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/common_services/auth_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/common_services/auth_server.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/server/common_services/auth_server.js
🧬 Code graph analysis (1)
src/server/common_services/auth_server.js (2)
src/endpoint/s3/s3_rest.js (4)
req(232-239)req(431-431)account(252-252)account(341-341)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
⏰ 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 (10)
src/test/integration_tests/nsfs/test_nsfs_integration.js (6)
251-257: LGTM! Test correctly validates ownership-based bucket listing.The test now creates a bucket owned by the account, verifies it appears in the list, and cleans up afterward. This aligns with the new ownership-based list_buckets behavior where accounts only see buckets they own.
525-531: LGTM! Consistent ownership-based test pattern.Same pattern as the wrong-uid test - creates owned bucket, verifies visibility, cleans up. The test properly validates that accounts with correct uid/gid see only their owned buckets.
1070-1124: LGTM! Test descriptions and assertions now reflect ownership semantics.The renamed tests clearly communicate the expected behavior:
- "should list nothing initially" - account has no owned buckets yet
- "account1 lists owned bucket" - after creating s3_b_name, account1 sees it
- "account2 lists nothing" - account2 has no owned buckets
This properly tests the fix for the unauthorized bucket access issue.
1134-1138: LGTM! Correctly verifies owned bucket visibility.The assertion
res.Buckets.some(b => b.Name === s3_root_b_name)properly checks that account4 sees the bucket it created, allowing for potential visibility of other owned buckets.
1666-1672: LGTM! Test verifies strict ownership-based bucket visibility.The updated test correctly validates that each account sees exactly one bucket - the one it owns. The
assert.deepStrictEqual(buckets, [account.bucket])assertion provides strong verification of the ownership-based list_buckets fix.
1303-1303: No issues found — assertion correctly validates ownership-based bucket filtering.The verification confirms that
account1created onlyregular_bucket_name[0](line 1290), whilebucket_name1,non_nsfs_bucket1, andnon_nsfs_bucket2were created by system code and admin operations. Thebucket_in_listfunction (lines 2014-2019) correctly validates that all expected buckets are present and all non-owned buckets are absent from the response. The test assertion at line 1303 properly reflects ownership-based filtering.src/server/common_services/auth_server.js (4)
499-502: LGTM! Request-level ownership check binding.The binding correctly delegates to
has_bucket_ownership_permissionwith the account and role from the request context.
527-558: LGTM! Well-guarded ownership helper functions.The helper functions (
is_system_owner,is_bucket_owner,is_bucket_claim_owner) have proper null checks that prevent runtime errors from undefined nested properties. This addresses the concerns raised in previous reviews about defensive checks.
613-643: LGTM! has_bucket_action_permission correctly refactored to use ownership helpers.The updates properly:
- Use
is_system_owner(bucket, account)for system owner check- Combine direct ownership and OBC checks into
has_owner_access- Return ownership-based access when no bucket policy exists
- Incorporate ownership into the final permission decision when bucket policy exists
The logic change from
result === 'ALLOW'tohas_owner_access || result === 'ALLOW'on line 643 correctly ensures bucket owners retain access even if not explicitly allowed by policy (while still respecting explicit DENY on line 641).
588-590: I need to gather more context about the function implementation and the PR scope.Based on my verification, I can now provide a clear assessment. Let me review the evidence:
The TODO is properly tracked – Line 568 documents the expected behavior in JSDoc, and line 589 explicitly marks it as "TODO: handle iam user"
Safe defaults are in place – Line 591 returns
false, which denies access to IAM users attempting to list buckets. This is a secure default that prevents unauthorized access rather than creating a gap.Active development context – NooBaa GitHub releases show ongoing IAM work, including PR #8661 "Allow IAM User to Create Bucket," indicating IAM permissions are being systematically implemented.
No security gap – Since the function returns
falsefor IAM users (not implemented), IAM users cannot list buckets until the TODO is addressed, which is the correct behavior until this feature is complete.Rewrite the TODO comment to clarify the deferred implementation
This TODO is properly tracked and doesn't introduce a security gap. The safe default (denying access) is in place. The deferred implementation is part of ongoing NooBaa IAM permission work and should be addressed in a follow-up, not necessarily in this PR unless the PR explicitly targets IAM bucket listing permissions.
4 participants
Describe the Problem
The
list_buckets()function was using.filter()with an async function, which doesn't wait for promises to resolve. This caused all buckets to pass through the filter ingnoring the actual permissions and allowing users to see all buckets in the system instead of only their authorized buckets.Explain the Changes
Note : Previously, we were allowing bucket policy to interfere with the list bucket operations which is not aws complaint.
Issues: Fixed #xxx / Gap #xxx
(Gaps)
3. Gap: Handle IAM user (small fix)
4. Related PR (gap): #9301
Testing Instructions:
(admin test)
(owner test)
5. Create an account (in case of OBC create OBC bucket and it will create OBC account)
6. Create bucket for the account (not for obc case)
7. Create S3 client using account credentials.
8. Try to list the buckets:
aws s3 ls--> It should list only the authorized buckets for the account.(NooBaa cli)
9. Use noobaa cli to list buckets
noobaa list buckets--> it should list all the bucketsSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.