IAM | No Bucket Policy Authorization for IAM Users

[shirady]

Describe the Problem

In case there is no bucket policy, we want users under the account to be authorized to perform S3 operations.

Explain the Changes

1. In case there is no bucket policy - permit users under the account for S3 operations.

Issues:

List of GAPs:

1. This code does not handle a case where there is a user under an OBC account, in case we decide to add this layer.
2. The tests are only manual at this point; there is a plan to add automated tests after we have the design change.

Testing Instructions:

1. Build the images and install the NooBaa system on Rancher Desktop (see guide).
   Note: nb is an alias that runs the local operator from build/_output/bin (alias created by devenv).
2. Wait for the default backing store pod to be in state Ready before starting the tests: kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1
3. I'm using port-forward (in a different tab):

• S3 kubectl port-forward -n test1 service/s3 12443:443
• IAM kubectl port-forward -n test1 service/iam 14443:443

4. Create the alias for the admin - first, need to get the credentials: nb status --show-secrets -n test1 and then:
   alias admin-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'
alias admin-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'
5. Check the connection to the endpoint:

• try to list the users (should be an empty list): admin-iam iam list-users; echo $?
• try to list the buckets (should be first.bucket): admin-iam s3 ls; echo $?

6. Create a user: admin-iam iam create-user --user-name Robert
   Note: To validate user creation, you can rerun admin-iam iam list-users and expect 1 user in the list
7. Create access keys: admin-iam iam create-access-keys --user-name Robert
8. Create the alias for the user (like in step 4 with alias user-2-s3).
9. The case with no bucket policy, for example:

• The user can put an object in the bucket of the owner:
  echo 'test_data' | user-2-s3 s3 cp - s3://first.bucket/test_object.txt (should work).
• The user can list objects in the bucket (to check the flow of the RPC call of has_bucket_action_permission function that was created in the auth_server):
  user-1-s3 s3 ls s3://first.bucket

Code changes for testing:

1. To see the account (of a user) in the cache after changes, src/sdk/object_sdk.js uses cache expiry of 1 millisecond.

const account_cache = new LRUCache({
    name: 'AccountCache',
-    expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS,
+   expiry_ms: 1, //SDSD 

Notes:

• In step 1 - deploying the system, I used --use-standalone-db for simplicity (fewer steps for the system in Ready status).

• I used the admin account, but for IAM operations, there is no difference between a created account and an admin account. I tested with the admin only because it saves steps (I have the admin account upon system creation and a bucket).

• Doc added/updated

• Tests added

Summary by CodeRabbit

• New Features

  • Added a public bucket_owner_id field to bucket info and policy responses.

• Improvements

  • Owner info now includes both account id and display name.
  • Authorization expanded so IAM accounts sharing a root owner can access buckets when no policy exists.
  • Ownership checks unified and made aware of containerized vs. non-containerized deployments.

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

[coderabbitai]

Walkthrough

Adds a new bucket_owner_id field to bucket API/schema, SDK reads, and server responses; propagates it to S3 authorization logic; makes owner comparisons NC-deployment-aware; and expands no-policy access to allow IAM accounts whose root owner matches the bucket owner.

Changes

Cohort / File(s)	Summary
API / schema src/api/bucket_api.js	Added bucket_owner_id: { type: 'string' } to definitions.bucket_sdk_info.
Bucket server responses src/server/system_services/bucket_server.js	read_bucket_sdk_info now includes bucket_owner_id derived from bucket.owner_account._id.toString().
SDK / policy info src/sdk/object_sdk.js	read_bucket_sdk_policy_info(name) now returns policy_info including bucket_owner_id (from bucket.bucket_owner_id) and clarifies owner_account shape.
S3 endpoint / auth logic src/endpoint/s3/s3_rest.js	Added bucket_owner_id to policy info extraction; introduced is_nc_deployment and made account identifier selection and ownership comparisons deployment-aware.
Server auth checks src/server/common_services/auth_server.js	has_bucket_action_permission updated: when no bucket policy exists, allow access if requester is the bucket owner OR requester's root owner equals the bucket owner's account.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant S3 as s3_rest.js
    participant SDK as object_sdk.js
    participant Bucket as bucket_server.js
    participant Auth as auth_server.js

    Client->>S3: authorize_request(request)
    S3->>SDK: read_bucket_sdk_policy_info(bucket)
    SDK->>Bucket: read_bucket_sdk_info(bucket)
    Bucket-->>SDK: bucket info (includes bucket_owner_id)
    SDK-->>S3: policy_info (includes bucket_owner_id, owner_account)

    alt Bucket has policy
        S3->>Auth: evaluate policy using owner_account
    else No bucket policy
        S3->>S3: determine is_nc_deployment
        S3->>S3: owner_to_compare = is_nc_deployment ? owner_account.id : bucket_owner_id
        S3->>Auth: has_bucket_action_permission(requester, owner_to_compare, ...)
    end

    Auth->>Auth: if no policy: allow if requester == owner OR requester.root_owner == bucket_owner_root
    Auth-->>S3: permission result
    S3-->>Client: authorized / denied

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review NC-deployment detection and owner-id selection in src/endpoint/s3/s3_rest.js.
• Confirm bucket_owner_id is consistently set/serialized in src/server/system_services/bucket_server.js and consumed by src/sdk/object_sdk.js.
• Verify has_bucket_action_permission changes in src/server/common_services/auth_server.js do not introduce auth bypass for IAM/root-owner edge cases.

Possibly related PRs

• IAM | Change bucket owner for IAM user to account and more #9268 — Related changes to bucket owner representation and propagation (introduces/uses explicit owner id).

Suggested labels

size/M

Suggested reviewers

• aayushchouhan09
• naveenpaul1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'IAM | No Bucket Policy Authorization for IAM Users' directly addresses the main change: enabling IAM users to perform S3 operations when no bucket policy exists.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 418bb3b and decf8cc.

📒 Files selected for processing (5)

• src/api/bucket_api.js (1 hunks)
• src/endpoint/s3/s3_rest.js (2 hunks)
• src/sdk/object_sdk.js (1 hunks)
• src/server/common_services/auth_server.js (1 hunks)
• src/server/system_services/bucket_server.js (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

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

📚 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/api/bucket_api.js
• src/server/common_services/auth_server.js
• src/sdk/object_sdk.js
• src/endpoint/s3/s3_rest.js
• src/server/system_services/bucket_server.js

📚 Learning: 2025-11-18T07:00:17.636Z

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.636Z
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/endpoint/s3/s3_rest.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/server/common_services/auth_server.js (2)

src/endpoint/s3/s3_rest.js (5)

• account (256-256)
• account (340-340)
• bucket (402-402)
• bucket (424-424)
• is_owner (272-285)

src/endpoint/s3/s3_bucket_policy_utils.js (1)

• account (293-293)

src/sdk/object_sdk.js (2)

src/endpoint/s3/s3_rest.js (2)

• bucket (402-402)
• bucket (424-424)

src/server/system_services/bucket_server.js (14)

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

src/server/system_services/bucket_server.js (2)

src/server/object_services/object_server.js (4)

• bucket (470-470)
• bucket (1394-1394)
• bucket (1483-1483)
• bucket (1529-1529)

src/server/system_services/system_server.js (1)

• bucket (237-242)

⏰ 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 (5)

src/sdk/object_sdk.js (1)

221-221: LGTM: Bucket owner ID added to policy info.

The new bucket_owner_id field is correctly added to the policy info payload, aligning with the bucket info response from bucket_server.js (line 736) where it's populated as bucket.owner_account._id.toString().

src/api/bucket_api.js (1)

1210-1212: LGTM: Schema updated for bucket owner ID.

The bucket_owner_id field is appropriately added to the bucket_sdk_info definition as a non-required string field, ensuring backward compatibility while supporting the new IAM authorization flow.

src/server/system_services/bucket_server.js (1)

736-736: LGTM: Bucket owner ID populated correctly.

The bucket_owner_id field is properly set from bucket.owner_account._id.toString(), which is safe since buckets always have an owner_account. This value flows correctly to the SDK layer and authorization logic.

src/endpoint/s3/s3_rest.js (2)

290-291: LGTM: Account owner comparison handles string correctly.

The comparison correctly treats account.owner as a string (line 291), which aligns with the retrieved learning that account.owner from req.object_sdk.requesting_account is already a string (account ID) from RPC serialization.

Based on learnings

---

287-294: Fix type mismatch in owner ID comparison on line 290.

When nsfs_config_root is true, owner_account?.id is an ObjectId (from bucket_server.js line 1521), but it's being compared against account.owner which is a string. This comparison will always fail. Line 290 should call .toString() on the ObjectId:

const owner_id_to_compare = req.object_sdk.nsfs_config_root ? owner_account?.id?.toString() : bucket_owner_id;

The bucket_owner_id path (line 736 of bucket_server.js) is already stringified, so only the nsfs_config_root path requires the conversion.

⛔ Skipped due to learnings

Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.

Learnt from: 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.

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.636Z
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.

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.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/api/bucket_api.js (1)

1192-1212: bucket_owner_id addition looks correct and backward‑compatible

Adding bucket_owner_id as a string on bucket_sdk_info cleanly exposes the owner account ID without breaking existing callers (it’s not in required). Just ensure the producers of bucket_sdk_info consistently set this field wherever IAM / owner‑ID based auth decisions depend on it.

src/server/common_services/auth_server.js (1)

548-554: No‑policy IAM access logic matches intent; consider a slightly safer owner check

The new !bucket_policy branch correctly gives access to:

• the bucket owner / claim owner (is_owner), and
• IAM accounts whose account.owner matches bucket.owner_account.

This aligns with the PR’s goal of allowing IAM users under the bucket’s root account when no bucket policy exists. Based on learnings, using account.owner._id.toString() here is appropriate since account is loaded from system_store and owner is an object ID reference, not a serialized string.

To harden against unexpected shapes, you could defensively guard the comparison:

-    const is_iam_and_same_root_account_owner = account.owner !== undefined &&
-        account.owner._id.toString() === bucket.owner_account._id.toString();
+    const is_iam_and_same_root_account_owner = account.owner &&
+        account.owner._id &&
+        bucket.owner_account &&
+        bucket.owner_account._id &&
+        account.owner._id.toString() === bucket.owner_account._id.toString();

This avoids a possible runtime error if either side is ever nullish while preserving the intended semantics.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa4124b and 640cdd3.

📒 Files selected for processing (5)

• src/api/bucket_api.js (1 hunks)
• src/endpoint/s3/s3_rest.js (3 hunks)
• src/sdk/object_sdk.js (1 hunks)
• src/server/common_services/auth_server.js (1 hunks)
• src/server/system_services/bucket_server.js (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• src/server/system_services/bucket_server.js
• src/endpoint/s3/s3_rest.js
• src/sdk/object_sdk.js

🧰 Additional context used

🧠 Learnings (6)

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

📚 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/api/bucket_api.js
• 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-18T07:00:17.636Z

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.636Z
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

📚 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-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

🧬 Code graph analysis (1)

src/server/common_services/auth_server.js (2)

src/endpoint/s3/s3_rest.js (5)

• account (256-256)
• account (344-344)
• bucket (406-406)
• bucket (428-428)
• is_owner (273-286)

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: run-package-lock-validation
• GitHub Check: run-jest-unit-tests
• GitHub Check: Build Noobaa Image