Added support for iam user in list_buckets() for containerized #9316
Conversation
WalkthroughExtracted a new helper Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/server/common_services/auth_server.js (1)
560-569: Helper logic looks correct; consider making it slightly more defensive and consistent with other helpersThe implementation correctly captures “IAM user whose root owner matches the bucket owner” and aligns with how
account.ownerandbucket.owner_accountare shaped in this module (both coming fromsystem_storewith an._idfield). Based on learnings, this should be safe here.Two minor nits you may want to address:
- All other
is_*_ownerhelpers are(bucket, account), while this one is(account, bucket). The current call sites are correct, but aligning the parameter order could avoid future confusion.- If there’s any chance this helper gets reused from endpoint code paths where
account.owneris already a string (e.g.,req.object_sdk.requesting_accountins3_rest.js), making it tolerant of both shapes would harden it, e.g.:-function is_iam_and_same_root_account_owner(account, bucket) { - if (!account?.owner || !bucket?.owner_account) return false; - return account.owner._id.toString() === bucket.owner_account._id.toString(); -} +function is_iam_and_same_root_account_owner(account, bucket) { + if (!account || !bucket?.owner_account) return false; + const account_owner_id = + typeof account.owner === 'string' + ? account.owner + : account.owner?._id?.toString(); + const bucket_owner_id = bucket.owner_account?._id?.toString(); + if (!account_owner_id || !bucket_owner_id) return false; + return account_owner_id === bucket_owner_id; +}Not strictly required for this PR but would make the helper safer if reused.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/common_services/auth_server.js(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
📚 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.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
🧬 Code graph analysis (1)
src/server/common_services/auth_server.js (2)
src/endpoint/s3/s3_rest.js (4)
account(252-252)account(355-355)bucket(431-431)bucket(453-453)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
🔇 Additional comments (2)
src/server/common_services/auth_server.js (2)
586-603: Ownership check extension for IAM users in ListBuckets looks goodAdding the IAM/root-owner check into
has_bucket_ownership_permissionaligns with the documented behavior (“IAM users can list their owner buckets”) and reuses the centralized helper. The ordering of checks still prioritizes system owner, operator, direct bucket owner, and OBC owner, so existing semantics remain intact while covering the missing IAM case.
621-636: Refactoring IAM/root-owner logic into the helper maintains behavior when no bucket policyIn the
!bucket_policybranch, replacing the inline IAM/root-owner computation withis_iam_and_same_root_account_owner(account, bucket)keeps the previous behavior while removing duplication withhas_bucket_ownership_permission. Combined withhas_owner_access, this correctly allows:
- direct bucket or OBC owners, and
- IAM users whose root account owns the bucket.
No additional edge cases are introduced by this change.
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
aayushchouhan09
force-pushed
the
iam-usr
branch
from
85c41c8 to
5bd94e1
Compare
2 participants
Describe the Problem
IAM user not able to list buckets of their owner even with the IAM policy applied.
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.