IAM | User Without IAM User Policy Improve Error Message #9307
Conversation
WalkthroughAdds an IAM helper to compose detailed S3 "not authorized" messages, exports it, integrates it into the S3 IAM authorization flow to produce detailed AccessDenied errors (defaults resource ARN to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant S3 as s3_rest.js
participant IAM as iam_utils.js
Client->>S3: Send S3 request
S3->>S3: authorize_request_iam_policy(req)
alt bucket present
S3->>S3: resource_arn = _get_arn_from_req_path(req)
else bucket missing / cannot derive
S3->>S3: resource_arn = '*'
end
S3->>S3: determine requesting_account (use iam_path)
alt inline policies present
S3->>S3: evaluate inline policies
alt any policy DENY
S3->>IAM: _create_detailed_message_for_iam_user_access_in_s3(requesting_account, method, resource_arn)
IAM-->>S3: detailed message
S3->>S3: _throw_iam_access_denied_error_for_s3_operation(...) -> throw AccessDenied
else no policy matches
S3->>IAM: _create_detailed_message_for_iam_user_access_in_s3(...)
IAM-->>S3: detailed message
S3->>S3: throw AccessDenied with detailed message
end
else no inline policies
alt NC deployment
S3->>S3: return early (no-op)
else
S3->>IAM: _create_detailed_message_for_iam_user_access_in_s3(...)
IAM-->>S3: detailed message
S3->>S3: throw AccessDenied with detailed message
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
shirady
force-pushed
the
iam-user-policy-error-message
branch
from
d1bff60 to
a6ae534
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 (1)
src/endpoint/s3/s3_rest.js (1)
17-18: IAM S3 authorization wiring and detailed AccessDenied look consistentThe new
_throw_iam_access_denied_error_for_s3_operationplus_create_detailed_message_for_access_in_s3integration cleanly upgrades IAM-based S3AccessDeniederrors with action and resource context, and the NC skip (nsfs_config_root) avoids enforcing non-existent IAM policies there.One behavioral aspect to double‑check: for non‑NC deployments, IAM users without matching/any inline IAM policies are denied here before bucket policies are evaluated, so a bucket policy alone cannot grant access. If the goal is to model AWS semantics where resource policies can allow access even when identity policies don’t, you may want to revisit this precedence; if stricter IAM‑first behavior is intentional, the current logic is fine.
Also applies to: 334-381
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/sdk/accountspace_fs.js(1 hunks)src/util/account_util.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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/endpoint/iam/iam_utils.jssrc/util/account_util.jssrc/sdk/accountspace_fs.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (3)
src/endpoint/iam/iam_utils.js (3)
src/endpoint/s3/s3_rest.js (6)
requesting_account(346-346)method(243-243)method(345-345)method(484-484)resource_arn(344-344)message_with_details(378-378)src/endpoint/iam/iam_rest.js (1)
method(229-229)src/util/account_util.js (8)
arn_for_requesting_account(448-449)full_action_name(446-446)message_with_details(328-328)message_with_details(365-365)message_with_details(393-393)message_with_details(429-429)message_with_details(435-435)message_with_details(441-441)
src/util/account_util.js (3)
src/server/system_services/account_server.js (16)
requesting_account(1198-1198)requesting_account(1218-1218)requesting_account(1260-1260)requesting_account(1277-1277)requesting_account(1305-1305)requesting_account(1336-1336)requesting_account(1349-1349)requesting_account(1377-1377)requesting_account(1394-1394)requesting_account(1415-1415)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)src/endpoint/s3/s3_bucket_policy_utils.js (1)
IAM_DEFAULT_PATH(97-97)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
src/sdk/accountspace_fs.js (1)
src/endpoint/s3/s3_rest.js (1)
requesting_account(346-346)
⏰ 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/sdk/accountspace_fs.js (1)
672-695: Access-denied ARN now correctly respects IAM pathUsing
requesting_account.iam_pathwhen buildingarn_for_requesting_accountmakes the error message consistent with IAM path semantics (and with_create_arn_for_account_or_user).create_arn_for_useralready handles missing/default paths, so this is a safe behavioral improvement.src/util/account_util.js (1)
445-467: IAM error ARN construction aligned with iam_pathSwitching to
requesting_account.iam_path || IAM_DEFAULT_PATHwhen callingcreate_arn_for_usermakes the IAM access-denied message’s ARN consistent with the configured IAM path and provides a sane fallback for legacy accounts.src/endpoint/iam/iam_utils.js (1)
69-93: S3 IAM denial message helper is coherent and AWS‑like
_create_detailed_message_for_access_in_s3correctly derives the requester ARN fromrequesting_account.ownerand IAM username, handles both string/array methods (with a special case forListAllMyBuckets), and produces the expected “no identity-based policy allows …” wording. Export wiring looks good, and usingownerdirectly avoids the S3owner._idpitfalls noted previously.Based on learnings, this keeps S3’s
requesting_account.ownerusage consistent without extra.toString()/._id.Also applies to: 866-866
src/endpoint/s3/s3_rest.js (1)
412-421: Service‑level S3 ARNs now handled safelyAdding
if (!req.params.bucket) return;in_get_arn_from_req_pathprevents bogusarn:aws:s3:::undefinedvalues for service‑level operations; combined with the|| '*'fallback inauthorize_request_iam_policy, this yields a sensible*resource while keeping bucket/object ARNs unchanged.
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/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/sdk/accountspace_fs.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sdk/accountspace_fs.js
- src/util/account_util.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/endpoint/iam/iam_utils.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (1)
src/endpoint/iam/iam_utils.js (1)
src/endpoint/s3/s3_rest.js (6)
requesting_account(346-346)method(243-243)method(345-345)method(484-484)resource_arn(344-344)message_with_details(378-378)
⏰ 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 (8)
src/endpoint/iam/iam_utils.js (2)
866-866: LGTM!The export correctly exposes the new helper function for use by S3 authorization code.
69-93: Now I need to search for where_throw_iam_access_denied_error_for_s3_operationis called to understand the full context:Based on my verification of the codebase, I can now generate the final response:
Review logic is partially valid but overstated—no actual bug exists in current code.
The array handling on line 80 is fragile but functional:
Single-element array concern: When arrays are embedded in template literals, they're converted via
.toString()which returns a comma-separated list without brackets. For a single-element array like["s3:GetObject"], this would actually produce"s3:GetObject"(identical to the string), so no message corruption occurs. However, no single-element arrays exist inOP_NAME_TO_ACTION—all array values are exactly 2 elements.Null/undefined concern: The
methodparameter always comes from_get_method_from_req()in s3_rest.js (line 400-410), which either returns a valid value or throws an error. In practice,methodis always defined when this function is called.Current behavior: The code correctly handles the documented special case (get_object_attributes with 2-element arrays), though it relies on implicit Array.toString() coercion as a fallback.
While the code works, it could be more explicit: either add a type guard (
Array.isArray(method) ? method[0] : method) or use explicit type checking instead of relying on array stringification behavior.src/endpoint/s3/s3_rest.js (6)
17-17: LGTM!The import correctly brings in the new helper function for building detailed IAM error messages.
348-352: LGTM!The logic correctly handles the case where IAM users have no inline policies, maintaining backward compatibility for NC deployments while providing detailed error messages for other deployments.
367-367: LGTM!Using the centralized error message builder for DENY decisions improves consistency and provides better user feedback.
373-375: LGTM!The logic correctly handles the case where policies exist but none match, providing a detailed error message to help users understand why access was denied.
377-381: LGTM!The helper function properly encapsulates the logic for throwing detailed IAM access denied errors, promoting code reuse and consistency across authorization paths.
413-413: LGTM!The early return when no bucket is present prevents undefined ARN construction and enables the fallback to
'*'on line 344 for list-all-buckets scenarios.
shirady
force-pushed
the
iam-user-policy-error-message
branch
from
6f23981 to
7fe2570
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/iam/iam_utils.js (1)
69-94: Consider consistent iam_path handling for clarity.The function correctly builds detailed IAM access messages. However, there's a minor inconsistency: Line 80 uses
requesting_account.iam_pathdirectly, whilesrc/util/account_util.js:444usesrequesting_account.iam_path || IAM_DEFAULT_PATH. Althoughcreate_arn_for_userhandles undefinediam_pathinternally, using an explicit fallback would improve consistency across the codebase.Apply this diff for consistency:
function _create_detailed_message_for_iam_user_access_in_s3(requesting_account, method, resource_arn) { const owner_account_id = get_owner_account_id(requesting_account.owner); + const { IAM_DEFAULT_PATH } = require('./iam_constants'); const arn_for_requesting_account = create_arn_for_user(owner_account_id, - get_iam_username(requesting_account.name.unwrap()), requesting_account.iam_path); + get_iam_username(requesting_account.name.unwrap()), requesting_account.iam_path || IAM_DEFAULT_PATH); const full_action_name = Array.isArray(method) && method.length > 1 ? method[1] : method; // special case for get_object_attributes
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/sdk/accountspace_fs.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/accountspace_fs.js
🧰 Additional context used
🧠 Learnings (4)
📚 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/util/account_util.jssrc/endpoint/iam/iam_utils.jssrc/endpoint/s3/s3_rest.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/endpoint/iam/iam_utils.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/endpoint/iam/iam_utils.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/endpoint/iam/iam_utils.js
🧬 Code graph analysis (2)
src/util/account_util.js (3)
src/server/system_services/account_server.js (16)
requesting_account(1198-1198)requesting_account(1218-1218)requesting_account(1260-1260)requesting_account(1277-1277)requesting_account(1305-1305)requesting_account(1336-1336)requesting_account(1349-1349)requesting_account(1377-1377)requesting_account(1394-1394)requesting_account(1415-1415)requesting_account(1457-1457)requesting_account(1486-1486)requesting_account(1516-1516)requesting_account(1545-1545)requesting_account(1559-1559)requesting_account(1578-1578)src/endpoint/s3/s3_bucket_policy_utils.js (1)
IAM_DEFAULT_PATH(97-97)src/endpoint/iam/iam_constants.js (1)
IAM_DEFAULT_PATH(69-69)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/iam/iam_utils.js (17)
require(6-6)require(7-8)require(10-10)message_with_details(87-87)message_with_details(107-108)message_with_details(281-281)message_with_details(506-507)message_with_details(536-536)message_with_details(563-564)message_with_details(570-571)message_with_details(598-599)message_with_details(619-621)message_with_details(652-652)message_with_details(684-685)message_with_details(704-704)message_with_details(719-719)message_with_details(726-726)src/endpoint/iam/iam_rest.js (1)
method(235-235)
⏰ 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 (10)
src/util/account_util.js (1)
444-444: LGTM!The change from
requesting_account.pathtorequesting_account.iam_path || IAM_DEFAULT_PATHcorrectly aligns with account schemas and provides a proper fallback. Based on the PR objectives, this fixes the field name inconsistency.src/endpoint/s3/s3_rest.js (8)
17-17: LGTM!The import of the IAM helper function centralizes error message construction for IAM access denied errors in S3 operations.
335-335: TODO comment aligns with planned refactoring.The PR objectives mention relocating this function as a future refactor. This is acceptable for now.
345-345: LGTM!Defaulting
resource_arnto'*'for the ListAllMyBuckets operation is correct, as this operation doesn't target a specific bucket resource. This aligns with the PR objective to correct the resource_arn for the ListBuckets operation.
348-352: LGTM!The added logging and detailed error message when an IAM user has no inline policies configured improves diagnostics and user experience, directly addressing the PR objectives.
367-370: LGTM!The added logging and use of the detailed error helper for explicit DENY policies improves diagnostics and provides consistent error messaging.
376-378: LGTM!The added logging and detailed error message when no inline policies match the operation improves diagnostics.
380-384: LGTM!The helper function cleanly wraps the detailed message construction and error throwing, providing good separation of concerns.
416-416: LGTM!The early return when bucket is missing prevents constructing an invalid ARN. The caller handles the undefined return by defaulting to
'*'.src/endpoint/iam/iam_utils.js (1)
882-882: LGTM!The export enables the S3 REST module to use this helper for consistent error messaging. The underscore prefix appropriately indicates this is an internal utility shared between related modules.
shirady
force-pushed
the
iam-user-policy-error-message
branch
from
7fe2570 to
6e24c2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/s3/s3_rest.js (1)
380-384: Centralized IAM AccessDenied construction looks good; consider carrying over any extra fields if S3Error defines themUsing
_create_detailed_message_for_iam_user_access_in_s3here and destructuring{ code, http_code }fromS3Error.AccessDeniedkeeps the error type and HTTP status consistent while improving the message, which is exactly what we want.If
S3Error.AccessDeniedcarries any additional fields that other call sites might rely on (e.g.,retryable,description, etc.), you might optionally spread them to avoid subtle differences:-function _throw_iam_access_denied_error_for_s3_operation(requesting_account, method, resource_arn) { - const message_with_details = _create_detailed_message_for_iam_user_access_in_s3(requesting_account, method, resource_arn); - const { code, http_code } = S3Error.AccessDenied; - throw new S3Error({ code, message: message_with_details, http_code}); -} +function _throw_iam_access_denied_error_for_s3_operation(requesting_account, method, resource_arn) { + const message_with_details = + _create_detailed_message_for_iam_user_access_in_s3(requesting_account, method, resource_arn); + const { code, http_code, ...rest } = S3Error.AccessDenied; + throw new S3Error({ code, http_code, ...rest, message: message_with_details }); +}If
code/http_codeare the only meaningful fields, the current implementation is fine as‑is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/endpoint/iam/iam_utils.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/endpoint/s3/s3_rest.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-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/endpoint/s3/s3_rest.js (2)
345-352: IAM behavior when IAM user has no inline policies now hard‑fails before bucket policy – confirm this is desiredWith the new
iam_policies.length === 0branch plus the explicit DENY / “no match” paths all calling_throw_iam_access_denied_error_for_s3_operation, any IAM user in a non‑NC deployment is now denied at the IAM layer unless an inline IAM policy explicitly ALLOWs the requested S3 action. Becauseauthorize_request_iam_policy(req)runs beforeauthorize_request_policy(req), a denial here short‑circuits bucket policy evaluation for IAM users.If existing workflows ever relied on bucket policies granting access to IAM users that lack IAM user policies, this would be a behavior change; if the intent is “IAM users must always have an IAM user policy in containerized deployments, bucket policies are only an additional filter,” this looks consistent, but it’s worth double‑checking and calling out in docs/release notes.
Also applies to: 367-378
416-423: Guarding_get_arn_from_req_pathon missing bucket correctly fixes ListBuckets ARN handlingThe
if (!req.params.bucket) return;guard, combined withconst resource_arn = _get_arn_from_req_path(req) || '*';inauthorize_request_iam_policy, ensures that service‑level operations (like listing all buckets) use'*'instead of generating an invalidarn:aws:s3:::style ARN. This aligns with IAM expectations for ListBuckets/ListAllMyBuckets‑style actions and avoids malformed ARNs.
1. Edit the error message when a user tries to perform an S3 operation to match what it tries to do and have a proper message when executing authorize_request_iam_policy. 2. Add log messages when there is no IAM user policy for better experience. 3. Fix a minor bug when I read the code so I changed the path to be iam_path according to the account schemas in NC and Containerised deployment. 4. Fix the resource_arn in the IAM policy for the list buckets operation. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-user-policy-error-message
branch
from
6e24c2e to
15a0277
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/iam/iam_utils.js (1)
81-81: Consider more explicit array handling.The current logic
Array.isArray(method) && method.length > 1 ? method[1] : methodreturnsmethod(the array itself) when it's an array with length ≤ 1, relying on implicittoString()coercion in string interpolation. While this works for single-element arrays, it's not explicit.Apply this diff for more defensive handling:
- const full_action_name = Array.isArray(method) && method.length > 1 ? method[1] : method; // special case for get_object_attributes + const full_action_name = Array.isArray(method) ? (method[1] ?? method[0]) : method; // special case for get_object_attributes
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/sdk/accountspace_fs.js(1 hunks)src/util/account_util.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/util/account_util.js
- src/endpoint/s3/s3_rest.js
🧰 Additional context used
🧠 Learnings (4)
📚 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/accountspace_fs.jssrc/endpoint/iam/iam_utils.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/endpoint/iam/iam_utils.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/endpoint/iam/iam_utils.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/endpoint/iam/iam_utils.js
🧬 Code graph analysis (1)
src/endpoint/iam/iam_utils.js (4)
src/endpoint/s3/s3_rest.js (5)
method(244-244)method(346-346)method(487-487)resource_arn(345-345)message_with_details(381-381)src/endpoint/iam/iam_rest.js (1)
method(235-235)src/util/account_util.js (9)
owner_account_id(341-341)owner_account_id(717-717)arn_for_requesting_account(443-444)message_with_details(328-328)message_with_details(360-360)message_with_details(388-388)message_with_details(424-424)message_with_details(430-430)message_with_details(436-436)src/server/system_services/account_server.js (1)
owner_account_id(1281-1281)
⏰ 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 (2)
src/endpoint/iam/iam_utils.js (1)
69-94: LGTM - well-structured detailed error message helper.The function correctly:
- Computes the requester ARN using
iam_pathfrom the user account- Handles the special case for
s3:ListAllMyBucketsby omitting the resource portion- Derives action names from method parameter with special handling for arrays
src/sdk/accountspace_fs.js (1)
677-677: LGTM - correct property name.This fixes the bug by using
requesting_account.iam_pathinstead of the non-existentrequesting_account.pathproperty. The change aligns with the rest of the codebase whereiam_pathis consistently used (e.g., lines 141, 181, 234, 599, 757, 925).
2 participants
Describe the Problem
Currently, in Containerised NooBaa deployment, when a user is created under an account (and has access keys), they must have a matching IAM policy to perform any S3 operations.
Explain the Changes
authorize_request_iam_policy.pathto beiam_pathaccording to the account schemas in NC and Containerised deployment.resource_arnin the IAM policy for the list buckets operation.Issues:
List of GAPs:
s3_bucket_policy_utils.jsso the terms will be policy in general and can be used for bucket policy and IAM policy.Testing Instructions:
Note:
nbis an alias that runs the local operator frombuild/_output/bin(alias created bydevenv).kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1kubectl port-forward -n test1 service/s3 12443:443kubectl port-forward -n test1 service/iam 14443:443nb status --show-secrets -n test1and 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'admin-iam iam list-users; echo $?first.bucket):admin-s3 s3 ls; echo $?admin-iam iam create-user --user-name RobertNote: To validate user creation, you can rerun
admin-iam iam list-usersand expect 1 user in the listadmin-iam iam create-access-keys --user-name Robertuser-1-s3):alias user-1-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'user-1-s3 s3 ls s3://first.bucket(should not work - should throwAccessDeniederror - look at the error). Example:policy_allow_list_bucket.json
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Action": [ "s3:ListBucket" ], "Resource": "*" } ] }admin-iam iam put-user-policy --user-name Robert --policy-name policy_allow_list_bucket --policy-document file://policy_allow_list_bucket.jsonuser-1-s3 s3 ls s3://first.bucket(should work - bucket is empty, no error was thrown).Code changes for testing:
src/sdk/object_sdk.jsuses cache expiry of 1 millisecond.const account_cache = new LRUCache({ name: 'AccountCache', - expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS, + expiry_ms: 1, //SDSDNotes:
In step 1 - deploying the system, I used
--use-standalone-dbfor 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
first.bucket).Doc added/updated
Tests added
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.