Fix (NC | NSFS) - list_buckets allowing unauthorized bucket access #9301
Conversation
aayushchouhan09
requested review from
liranmauda,
naveenpaul1,
nimrod-becker and
shirady
WalkthroughList-buckets gating changed to ownership-based checks in src/sdk/bucketspace_fs.js. New public helpers (is_bucket_owner, is_iam_and_same_root_account_owner, has_bucket_ownership_permission) and NSFS access validators were added. has_bucket_action_permission and list_buckets were refactored. Tests updated: removed two ListBuckets policy tests and added/updated bucket-tagging tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BucketSpaceFS
participant Ownership as OwnershipChecker
participant Policy as PolicyEvaluation
participant NSFS as NSFSValidator
rect rgb(245,250,255)
note right of BucketSpaceFS: Ownership-first list_buckets flow
Client->>BucketSpaceFS: list_buckets(request, account)
BucketSpaceFS->>Ownership: has_bucket_ownership_permission(bucket, account)
alt ownership true
Ownership-->>BucketSpaceFS: true
BucketSpaceFS->>NSFS: validate_fs_bucket_access(bucket, object_sdk)
alt nsfs access ok
NSFS-->>BucketSpaceFS: access OK
BucketSpaceFS-->>Client: include bucket
else nsfs denied
NSFS-->>BucketSpaceFS: denied
BucketSpaceFS-->>Client: omit bucket
end
else ownership false
Ownership-->>BucketSpaceFS: false
BucketSpaceFS->>Policy: get_bucket_policy(bucket)
alt policy exists
Policy-->>BucketSpaceFS: policy
BucketSpaceFS->>Policy: evaluate s3:ListBucket principal
alt allowed
BucketSpaceFS-->>Client: include bucket
else denied
BucketSpaceFS-->>Client: omit bucket
end
else no policy
BucketSpaceFS-->>Client: omit bucket
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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
nc-list-fix
branch
from
0c47879 to
d895b76
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
🧹 Nitpick comments (3)
src/sdk/bucketspace_fs.js (3)
263-281: Ownership-only filtering inlist_bucketslooks right; please verify IAM & no‑DB flows and add testsUsing
has_bucket_ownership_permission(bucket, account)here is a good move to decoupleListBucketsfrom bucket policy and avoid non‑AWS cross‑account listing vias3:ListBucket.Two edge cases are worth double‑checking:
- For IAM users (
account.ownerset,_id!=bucket.owner_account.id),has_bucket_ownership_permission()currently returnsfalse(TODO not implemented), soListBucketswill not show buckets owned by their root account at all. If IAM listing should be supported now, this is a behavior change from previous policy‑based listing.- For NSFS no‑DB / anonymous‑style flows where
object_sdk.requesting_accountmay be missing_id, this filter will silently drop all buckets even if FS ACLs would allow access.I recommend:
- Clarifying whether IAM and no‑DB callers are expected to see buckets now, and adjusting
has_bucket_ownership_permission()accordingly (see below), or explicitly documenting this limitation.- Adding tests around
ListBucketsfor: root owner, IAM user of same root, another account with bucket policy, and an account with no buckets.
924-959: Alignhas_bucket_ownership_permissionwith documented IAM behavior and reuse it consistentlyThe helper extraction is good, but there are a couple of mismatches and duplication spots:
The JSDoc says:
- “Root accounts can list buckets they own”
- “IAM users can list their owner buckets”
but the implementation only checks direct ownership via
_idand always returnsfalsefor IAM users.You already implement “IAM user + same root account owner” logic in
has_bucket_action_permissionvia:const is_iam_and_same_root_account_owner = account.owner !== undefined && account.owner === bucket.owner_account.id;
has_bucket_ownership_permissionshould likely use the same rule so thatListBucketsand per‑bucket actions share ownership semantics instead of duplicating them.Given the existing pattern and the prior learning that
account.owneris already a string owner ID in S3 REST flows, no extra.toString()/._idwork is needed when comparing it tobucket.owner_account.id. Based on learnings, …You could tighten this helper as:
async has_bucket_ownership_permission(bucket, account) { - // check direct ownership (root accounts only) - if (this.is_bucket_owner(bucket, account)) return true; - - // special case: iam user can list the buckets of their owner - // TODO: handle iam user - - return false; + // direct ownership (root account owns the bucket) + if (this.is_bucket_owner(bucket, account)) return true; + + // IAM user can list buckets owned by its root account + if (account?.owner && account.owner === bucket?.owner_account?.id) return true; + + return false; }Optionally, you could also have
has_bucket_action_permissioncall this helper for its “no policy” branch to avoid repeating the IAM/root ownership check in two places.Also, the TODO comment above mentions “4 functions” but currently lists 5; might be worth correcting to avoid confusion.
963-1005:has_bucket_action_permissionrefactor to useis_bucket_ownerlooks good; consider reusing ownership helperRefactoring the ownership check into
is_bucket_owner()and dropping the deprecatedsystem_ownerhandling is a nice cleanup and keeps the method focused on bucket owner + policy evaluation.To keep ownership semantics consistent across APIs, you may want to:
- Reuse
has_bucket_ownership_permission()here for the “no bucket policy” case instead of duplicating the ownership vs IAM logic (is_owner || is_iam_and_same_root_account_owner), so any future change to ownership rules is centralized in one place.Functionally, though, the current change maintains the previous behavior (modulo the system_owner deprecation), so it looks safe.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/bucketspace_fs.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/bucketspace_fs.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
aayushchouhan09
force-pushed
the
nc-list-fix
branch
from
d895b76 to
a15ab26
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/sdk/bucketspace_fs.js (2)
938-947: Inconsistent parameter order withis_bucket_owner.This method uses
(account, bucket)order whileis_bucket_owneruses(bucket, account). This inconsistency makes the API confusing and error-prone, especially when both methods are used together (as inhas_bucket_ownership_permissionandhas_bucket_action_permission).Consider aligning the parameter order:
- is_iam_and_same_root_account_owner(account, bucket) { - if (!account?.owner || !bucket?.owner_account?.id) return false; - return account.owner === bucket.owner_account.id; + is_iam_and_same_root_account_owner(bucket, account) { + if (!account?.owner || !bucket?.owner_account?.id) return false; + return account.owner === bucket.owner_account.id; }Then update the call sites at lines 965 and 983:
- if (this.is_iam_and_same_root_account_owner(account, bucket)) return true; + if (this.is_iam_and_same_root_account_owner(bucket, account)) return true;
949-968: Unnecessaryasynckeyword.This method is marked
asyncbut contains noawaitexpressions. The function executes synchronously and wraps the result in a Promise unnecessarily.Consider making this synchronous:
- async has_bucket_ownership_permission(bucket, account) { + has_bucket_ownership_permission(bucket, account) {Note: If you anticipate future async operations (e.g., database lookups for ownership), keeping it async is acceptable for forward compatibility. Update the caller at line 275 accordingly if changed to sync.
src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
25-25: Consider adding unit tests for new ownership helper methods.The PR adds three new methods (
is_bucket_owner,is_iam_and_same_root_account_owner,has_bucket_ownership_permission) but no direct unit tests for them. While the existing integration tests at lines 502-521 cover the behavior throughlist_buckets, dedicated unit tests for these methods would:
- Ensure edge cases are covered (e.g., missing
owner_account.id, missingaccount._id, missingaccount.owner)- Provide clearer regression detection if these methods are modified
- Document the expected behavior more explicitly
Based on coding guidelines, ensure tests are added for the changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/bucketspace_fs.js(3 hunks)src/test/unit_tests/nsfs/test_bucketspace_fs.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/unit_tests/nsfs/test_bucketspace_fs.js
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/bucketspace_fs.js
🧬 Code graph analysis (1)
src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
src/sdk/bucketspace_fs.js (5)
require(15-23)require(24-24)require(25-25)require(27-27)require(30-30)
⏰ 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 (4)
src/sdk/bucketspace_fs.js (3)
927-936: LGTM!The ownership check is well-implemented with proper null safety using optional chaining.
970-983: LGTM!The refactored ownership logic properly replaces the deprecated
system_ownercheck with the newis_bucket_ownermethod. The no-policy case correctly allows access for both direct owners and IAM users of the owner.
274-278: LGTM!The ownership-based gating for
list_bucketscorrectly implements AWS-compliant behavior where only bucket owners (and their IAM users) can see buckets in the listing.src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
488-536: Existing test coverage for ownership-based listing looks adequate.The tests correctly verify:
- IAM accounts can list buckets owned by their root account (lines 502-515)
- Different accounts cannot see each other's buckets (lines 516-521)
These tests align well with the new ownership-based gating logic.
aayushchouhan09
force-pushed
the
nc-list-fix
branch
from
a15ab26 to
0e22c56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sdk/bucketspace_fs.js (2)
933-947: Consider consistent parameter ordering across helper methods.The parameter order differs between
is_bucket_owner(bucket, account)andis_iam_and_same_root_account_owner(account, bucket). For better code maintainability, consider using a consistent order (e.g., alwaysbucket, accountor alwaysaccount, bucket).Example with consistent ordering:
- is_iam_and_same_root_account_owner(account, bucket) { + is_iam_and_same_root_account_owner(bucket, account) { if (!account?.owner || !bucket?.owner_account?.id) return false; return account.owner === bucket.owner_account.id; }Then update the caller at line 965:
- if (this.is_iam_and_same_root_account_owner(account, bucket)) return true; + if (this.is_iam_and_same_root_account_owner(bucket, account)) return true;
960-968: Remove unnecessaryasynckeyword.The
has_bucket_ownership_permissionmethod is markedasyncbut doesn't perform any asynchronous operations. This can be simplified to a synchronous method for a minor performance improvement.Apply this diff:
- async has_bucket_ownership_permission(bucket, account) { + has_bucket_ownership_permission(bucket, account) { // check direct ownership if (this.is_bucket_owner(bucket, account)) return true; // special case: iam user can list the buckets of their owner if (this.is_iam_and_same_root_account_owner(account, bucket)) return true; return false; }Then update the caller at line 275:
- if (!await this.has_bucket_ownership_permission(bucket, account)) return; + if (!this.has_bucket_ownership_permission(bucket, account)) return;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/bucketspace_fs.js(3 hunks)src/test/unit_tests/nsfs/test_bucketspace_fs.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/unit_tests/nsfs/test_bucketspace_fs.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/bucketspace_fs.js
🔇 Additional comments (3)
src/sdk/bucketspace_fs.js (3)
244-244: LGTM: Comment accurately reflects ownership-based filtering.The updated comment correctly describes the new AWS-compliant behavior where list_buckets filters by ownership rather than bucket policy.
274-275: LGTM: Ownership-based filtering makes list_buckets AWS-compliant.This change correctly implements AWS-compliant behavior where the ListBuckets operation is based on ownership rather than bucket policies. Root accounts can list their own buckets, and IAM users can list their root account's buckets, which matches AWS S3 behavior.
970-1013: LGTM: Refactoring improves code clarity while preserving functionality.The refactoring to use
is_bucket_ownerandis_iam_and_same_root_account_ownerhelper methods improves code maintainability. The removal of the deprecatedsystem_ownercheck is appropriate, and the logic correctly handles both ownership-based access and bucket policy-based access.
src/sdk/bucketspace_fs.js
Outdated
| ///////////////// | ||
|
|
||
| // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir() | ||
| // TODO: move the following 5 functions - is_bucket_owner(), is_iam_and_same_root_account_owner(), has_bucket_ownership_permission(), has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir() |
There was a problem hiding this comment.
If the functions are not used anymore, you can remove them.
There was a problem hiding this comment.
These are util functions and all are used, comment is about to move them in future for code reusability
| const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account); | ||
| assert.equal(res.buckets.length, 0); | ||
| }); | ||
| mocha.it('list buckets - different account with bucket policy (principal by name)', async function() { |
There was a problem hiding this comment.
I suggest changing the test instead of removing it.
Change to another action (not list buckets).
I think it was mainly for checking different principals in bucket policy.
There was a problem hiding this comment.
Sure I will edit them to work for different action.
aayushchouhan09
force-pushed
the
nc-list-fix
branch
from
0e22c56 to
43bb00a
Compare
aayushchouhan09
force-pushed
the
nc-list-fix
branch
from
43bb00a to
85fe071
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/sdk/bucketspace_fs.js (1)
924-969: Ownership helpers correctly encode root/IAM semantics; only minor nitsThe split into
is_bucket_owner,is_iam_and_same_root_account_owner, andhas_bucket_ownership_permissioncaptures the intended model:
- Root:
bucket.owner_account.id === account._id.- IAM:
account.owner === bucket.owner_account.id(same root account).This is consistent with how
owner_accountis written innew_bucket_defaultsand with prior context thataccount.owneris already a serialized account ID string, so no extra.toString()is needed. Based on learnings, this comparison style is appropriate.Two small, optional tweaks you might consider later:
has_bucket_ownership_permissionis purely synchronous; you could dropasync/awaitto simplify the call chain if you don’t plan async checks here.- In
has_bucket_action_permission, you could callawait this.has_bucket_ownership_permission(bucket, account)instead of re-implementing the same logic, to keep ownership semantics centralized.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/bucketspace_fs.js(3 hunks)src/test/unit_tests/nsfs/test_bucketspace_fs.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/unit_tests/nsfs/test_bucketspace_fs.js
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/bucketspace_fs.js
🧬 Code graph analysis (1)
src/test/unit_tests/nsfs/test_bucketspace_fs.js (2)
src/test/integration_tests/api/s3/test_s3_list_buckets.js (1)
create_bucket(66-66)src/sdk/config_fs.js (1)
CONFIG_SUBDIRS(42-50)
⏰ 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 (3)
src/sdk/bucketspace_fs.js (2)
239-282: Ownership-based ListBuckets filtering looks correct and AWS-alignedFiltering buckets in
list_bucketsviahas_bucket_ownership_permission()(direct owner or IAM user whoseownermatches the bucket’s root account) matches AWS behavior whereListBucketsonly returns buckets owned by the caller’s account, regardless of bucket policy. The updated doc comment at Line 244 correctly reflects this behavior, and the early ownership filter precedes the existing FS-access check without altering error handling.
970-1013: I'm experiencing persistent repository cloning issues that prevent me from accessing the codebase. However, I can use the retrieved learnings to inform my analysis.Cannot verify bucket object normalization—repository access unavailable
The repository cloning infrastructure is currently unavailable, preventing me from:
- Inspecting the
is_bucket_ownerimplementation to confirm what shape it expects frombucket.owner_account- Scanning all call sites of
has_bucket_action_permissionto verify whether normalized bucket objects are consistently passed- Confirming whether any code paths could pass raw config buckets (with string
owner_account) to this methodThe review comment raises a valid concern about implicit assumptions on bucket shape. The fix depends on verifying:
- Whether
is_bucket_owneractually validates/requires the{ id: ... }structure forowner_account- Whether all callers (especially in S3 REST handlers, bucket operations, etc.) consistently normalize buckets before calling
has_bucket_action_permissionWithout codebase access, I cannot confirm if the concern is theoretical or represents a real bug path.
src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
909-956: Dedicated tagging bucket and policy-principal tests look goodThe tagging suite now:
- Uses a dedicated
test_bucket_taggingbucket with explicitbefore/aftersetup and cleanup, reducing coupling with other tests.- Re-points the basic put/delete tagging tests at this bucket.
- Adds two focused tests that create bucket policies with principals specified by name and by id and then perform
PutBucketTaggingvia a different account’s SDK wrapper, confirming that such policies are accepted and that tagging state round-trips correctly.This gives you test coverage for the principal encodings after the list_buckets behavior change and satisfies the requirement to include tests alongside the permission/ownership changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/sdk/bucketspace_fs.js (1)
924-926: Future refactoring noted.The TODO to extract these utility functions for reuse is reasonable. Consider prioritizing this refactoring if these functions will be needed in other modules.
🧹 Nitpick comments (1)
src/sdk/bucketspace_fs.js (1)
938-947: Consider standardizing parameter order across helper functions.The function logic is correct, but the parameter order
(account, bucket)differs fromis_bucket_owner(bucket, account). Standardizing to a consistent order (e.g., alwaysbucket, account) would improve code maintainability and reduce the chance of bugs when calling these functions.Apply this diff if you choose to align the parameter order:
- is_iam_and_same_root_account_owner(account, bucket) { - if (!account?.owner || !bucket?.owner_account?.id) return false; - return account.owner === bucket.owner_account.id; + is_iam_and_same_root_account_owner(bucket, account) { + if (!account?.owner || !bucket?.owner_account?.id) return false; + return account.owner === bucket.owner_account.id; }And update the call sites at lines 965 and 983 accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sdk/bucketspace_fs.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/bucketspace_fs.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 (5)
src/sdk/bucketspace_fs.js (5)
244-244: LGTM: Comment accurately reflects the new ownership-based filtering.The updated comment correctly describes that buckets are now filtered based on ownership (direct or IAM user's root account ownership) rather than bucket policy access.
274-276: LGTM: Ownership-based filtering is correct and AWS-compliant.The switch from policy-based permission checks to
has_bucket_ownership_permission()aligns with AWS behavior where bucket policies do not affect the ListBuckets operation. Only buckets owned by the requesting account (or their root account for IAM users) are returned.
927-936: LGTM: Direct ownership check is correct.The function properly validates direct bucket ownership with appropriate null safety guards.
949-968: LGTM: Ownership permission logic correctly implements AWS-compliant behavior.The function properly combines direct ownership and IAM user inheritance checks to determine if a bucket should appear in ListBuckets results. The implementation aligns with AWS behavior where:
- Root accounts see buckets they own
- IAM users see buckets owned by their root account
970-1013: LGTM: Refactored permission logic maintains correct behavior.The updates properly:
- Use the new ownership helper functions for consistency
- Remove deprecated system_owner checks
- Maintain correct fallback behavior: ownership grants access when no policy exists, and policy can grant additional access beyond ownership
The logic at line 1012 correctly allows access if the account is the owner OR if the bucket policy explicitly allows the action.
src/sdk/bucketspace_fs.js
Outdated
| ///////////////// | ||
|
|
||
| // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir() | ||
| // TODO: move the following 6 functions - is_bucket_owner(), is_iam_and_same_root_account_owner(), has_bucket_ownership_permission(), has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir() |
There was a problem hiding this comment.
I don't like keeping this updated after each PR, if we don't want to move it right now, let's create a new section // SHARED UTILS - with the original explanation - and keep everything that we think we want to move from this file there
Signed-off-by: Aayush Chouhan <[email protected]>
Signed-off-by: Aayush Chouhan <[email protected]>
Signed-off-by: Aayush Chouhan <[email protected]>
aayushchouhan09
force-pushed
the
nc-list-fix
branch
from
f36aadc to
5a6620d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/sdk/bucketspace_fs.js (1)
1032-1037: Consider extracting shared utilities in a future refactor.The TODO comment indicates these functions could be extracted to a shared utility file for reuse across the codebase. While this would improve maintainability, it's reasonable to defer this refactor given the PR's current scope.
src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
908-957: Consider adding explicit tests for new ownership helper functions.The PR introduces several new access control functions (
is_bucket_owner,is_iam_and_same_root_account_owner,has_bucket_ownership_permission) but doesn't include explicit unit tests for them. While existing tests likelist_buckets - iam accounts(lines 502-515) exercise these functions implicitly, dedicated tests would improve coverage and make the behavior more explicit.Consider adding tests such as:
is_bucket_owner - returns true for direct owneris_bucket_owner - returns false for non-owneris_iam_and_same_root_account_owner - returns true for IAM user of root ownerhas_bucket_ownership_permission - various ownership scenariosAs per coding guidelines: "Ensure that the PR includes tests for the changes."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/sdk/bucketspace_fs.js(7 hunks)src/sdk/nb.d.ts(2 hunks)src/test/unit_tests/nsfs/test_bucketspace_fs.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/unit_tests/nsfs/test_bucketspace_fs.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/sdk/nb.d.ts
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/sdk/nb.d.ts
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/sdk/bucketspace_fs.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/sdk/bucketspace_fs.js
🧬 Code graph analysis (2)
src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
src/sdk/config_fs.js (1)
CONFIG_SUBDIRS(42-50)
src/sdk/bucketspace_fs.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/server/common_services/auth_server.js (2)
bucket_policy(619-619)bucket_policy(673-673)
⏰ 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 (6)
src/sdk/nb.d.ts (1)
66-66: LGTM! Type declarations support ownership-based access control.The new
ownerfield on Account andowner_accounton Bucket properly support the ownership-based access control logic introduced in this PR.Also applies to: 183-183
src/sdk/bucketspace_fs.js (4)
244-282: LGTM! List_buckets correctly implements ownership-based gating.The refactored flow properly checks ownership permissions (line 275) instead of bucket policy permissions, making the behavior AWS-compliant. The additional filesystem access validation (line 277) ensures the account can actually access the underlying storage.
1113-1122: Clarify the conditional check by account name.The condition
if (account.owner === undefined)restricts name-based permission checks to root accounts only, but the comment suggests all accounts should support both ID and name identification.Is this intentional? If IAM users should also be checkable by name, the condition should be removed:
- if (account.owner === undefined) { - permission_by_name = await bucket_policy_utils.has_bucket_policy_permission( - bucket_policy, - account.name.unwrap(), - action, - `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, - undefined, - { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } - ); - } + permission_by_name = await bucket_policy_utils.has_bucket_policy_permission( + bucket_policy, + account.name.unwrap(), + action, + `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, + undefined, + { disallow_public_access: bucket.public_access_block?.restrict_public_buckets } + );
1127-1153: LGTM! Filesystem access validation is properly implemented.The functions correctly validate that:
- The bucket is an NSFS bucket (line 1130)
- The account has valid nsfs_account_config with uid/gid (lines 1140-1141)
- The account can access the bucket's underlying storage path (line 1146)
- Expected access errors are handled gracefully (line 1150)
1056-1059: Based on the learnings provided and the repository access limitations, I can analyze this review comment:Key Evidence from Learnings:
- Learning 1 (PR 9291, auth_server.js):
account.ownerfromsystem_storeis an ObjectID reference with._idproperty- Learning 2 (PR 9277, s3_rest.js):
account.owneris a string due to RPC serialization- The type of
account.ownervaries depending on how the account object is loadedAnalysis:
The review comment raises a valid concern about type consistency. The learnings demonstrate thataccount.ownerhas different types across different contexts in the codebase:
- When loaded from
system_store(auth_server): ObjectID object- When received via RPC (s3_rest): string
In
bucketspace_fs, which is NSFS (filesystem-backed), accounts are loaded from config files. The review suggests bothaccount.ownerandbucket.owner_account.idshould be strings from JSON config, but the comparison lacks explicit type validation.However, without direct access to the codebase, I cannot definitively verify:
- Where exactly accounts are loaded in bucketspace_fs
- Whether account objects are reused across contexts (potentially mixing types)
- If the current code actually fails due to type mismatch
The suggested defensive
String()coercion is valid practice for ensuring safety when type consistency cannot be guaranteed.
The comparison
account.owner === bucket.owner_account.idmay have type consistency issues. Given thataccount.ownercan be an ObjectID or string depending on loading context (per learnings from other parts of the codebase), add defensive type coercion to ensure safe comparison:is_iam_and_same_root_account_owner(account, bucket) { if (!account?.owner || !bucket?.owner_account?.id) return false; - return account.owner === bucket.owner_account.id; + return String(account.owner) === String(bucket.owner_account.id); }Verify that both
account.ownerandbucket.owner_account.idoriginate from the same source (JSON config) and confirm types are consistently strings in this NSFS context.src/test/unit_tests/nsfs/test_bucketspace_fs.js (1)
928-950: LGTM! Tests validate policy-based tagging access.The new tests properly verify that bucket policy permissions enable cross-account tagging operations for both principal formats (by name and by ID). The tests correctly:
- Set up bucket policy granting
s3:PutBucketTaggingpermission- Execute the operation with a different account
- Verify the tagging was applied
3 participants
Describe the Problem
Currently, bucket policy interfere with the list buckets which is not aws complaint.
Explain the Changes
list_buckets()with proper permissions and make it aws complaint.Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.