IAM | User ID for principal is not supported

[naveenpaul1]

Describe the Problem

User ID for principal is not supported

Explain the Changes

1. Block put pucket policy if the principal is IAM user ID

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

GAP: Missing integration test for IAM users, Will add in upcoming PRs

• Doc added/updated
• Tests added

Summary by CodeRabbit

• Bug Fixes
  • Improved bucket policy principal resolution: IAM users are no longer incorrectly recognized as valid principals when resolving by account ID. This prevents unintended principal matches during bucket policy evaluation and leads to more accurate access-control decisions and fewer erroneous permission grants.

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

[coderabbitai]

Walkthrough

Modified get_account_by_principal to check if resolved account has an owner property. When principal is not an ARN and the resolved account has an owner (indicating an IAM user), the function logs debug information and returns false, preventing IAM users from being resolved as valid principals by account ID.

Changes

Cohort / File(s)	Change Summary
IAM user principal resolution check src/server/system_services/bucket_server.js	Added owner property check in get_account_by_principal that returns false for IAM-user accounts identified by ID; logs debug message when condition is met

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

• Focus on understanding the business logic behind rejecting IAM users when resolved by account ID
• Verify the debug logging is appropriate and doesn't expose sensitive information

Possibly related PRs

• Added support for iam user in list_buckets() for containerized #9316: Changes logic for treating IAM users as account owners/principals, with overlapping get_account_by_principal modifications
• IAM | No Bucket Policy Authorization for IAM Users #9291: Modifies bucket_server.js and auth logic for IAM user authorization and account/principal handling
• IAM | put_bucket_policy Principal validation updated with ARN #9277: Changes ARN/ID principal resolution logic in the same get_account_by_principal function

Suggested labels

size/S

Suggested reviewers

• aayushchouhan09
• shirady
• jackyalbo

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and specifically indicates the main change: preventing IAM user IDs from being used as valid principals in bucket policies, which aligns with the code modification in get_account_by_principal that now rejects IAM users identified by ID.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

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

---

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 89f64d8 and 774f343.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧠 Learnings (5)

📓 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-19T15:03:42.260Z

Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.

Applied to files:

• src/server/system_services/bucket_server.js

📚 Learning: 2025-11-12T04:55:42.193Z

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

Applied to files:

• src/server/system_services/bucket_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/system_services/bucket_server.js

📚 Learning: 2025-11-13T07:56:23.620Z

Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.

Applied to files:

• src/server/system_services/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: run-jest-unit-tests
• GitHub Check: Build Noobaa Image