IAM | Principal validation and S3 permission updated with ID #9309
Conversation
WalkthroughNormalize bucket-policy principal handling to accept arrays (ARN and ID), add ARN-based principal resolution, propagate array-based principal matching through policy evaluation (DENY before ALLOW), adjust authorization flows to check account/owner ID and ARN, and update tests to cover ARN/ID principal cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant s3_rest as s3_rest.js
participant auth as auth_server.js
participant policy as s3_bucket_policy_utils.js
participant bucket as bucket_server.js
Note over Client,s3_rest: Incoming S3 request with account context
Client->>s3_rest: request (account)
s3_rest->>s3_rest: account_identifier_id = account._id
s3_rest->>auth: check permission by account_id
auth->>policy: has_bucket_policy_permission(principal=[root_ARN, account_id], ...)
policy->>policy: normalize principal -> account_arr (array)
policy->>policy: evaluate DENY statements then ALLOW statements using account_arr (includes match)
alt owner present
s3_rest->>auth: owner-based check with owner ARN/ID
auth->>policy: has_bucket_policy_permission(principal=[owner_ARN, owner_id], ...)
end
policy->>bucket: if principal needs existence check -> account_exists_by_principal_arn(principal_as_string)
bucket-->>policy: return account object or undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
2 times, most recently
from
ca9a081 to
0020baa
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: 1
🧹 Nitpick comments (2)
src/server/system_services/bucket_server.js (2)
528-546: Consider removing unnecessaryasynckeyword.This function doesn't contain any
awaitexpressions. Bothsystem_store.data.accounts.find()andsystem_store.get_account_by_email()appear to be synchronous operations based on usage patterns in the codebase. Marking this functionasyncunnecessarily wraps return values in a Promise.If this is intentional for consistency with the caller or future changes, this is fine as-is.
548-567: Function name is misleading - returns boolean, not account.
get_account_by_principalimplies the function returns an account object, but it actually returns a boolean indicating whether the principal exists. Consider renaming to better reflect the return type:
principal_existsis_valid_principalvalidate_principal-async function get_account_by_principal(principal) { +async function principal_exists(principal) {Alternatively, if a getter pattern is preferred, the function could return the account object (or
undefined) and let the caller convert to boolean, which would make the validation callback cleaner and enable future use cases that need the account:async function get_account_by_principal(principal) { const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; const is_principal_arn = principal_as_string.startsWith('arn:aws:iam::'); if (is_principal_arn) { - const principal_by_arn = await account_exists_by_principal_arn(principal_as_string); - dbg.log3('get_account_by_principal: principal_by_arn', principal_by_arn); - if (principal_by_arn) return true; + return account_exists_by_principal_arn(principal_as_string); } else { - const account = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); - const principal_by_id = account !== undefined; - dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); - if (principal_by_id) return true; + return system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); } - return false; }Then update the callback to convert to boolean:
principal => Boolean(get_account_by_principal(principal))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(4 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(10 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🧰 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/integration_tests/api/iam/test_iam_basic_integration.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.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/endpoint/s3/s3_rest.jssrc/server/common_services/auth_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/system_services/bucket_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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/server/system_services/bucket_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/system_services/bucket_server.js
🧬 Code graph analysis (6)
src/endpoint/s3/s3_rest.js (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)
src/server/common_services/auth_server.js (3)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/endpoint/s3/s3_rest.js (2)
account(252-252)account(343-343)src/server/system_services/bucket_server.js (1)
account(561-561)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(303-303)_(4-4)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
src/endpoint/iam/iam_utils.js (1)
statement_principal(723-723)
src/server/system_services/bucket_server.js (6)
src/server/system_services/account_server.js (11)
SensitiveString(15-15)dbg(12-12)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)system_store(17-17)src/api/account_api.js (1)
SensitiveString(4-4)src/cmd/manage_nsfs.js (1)
SensitiveString(25-25)src/api/common_api.js (1)
SensitiveString(4-4)src/cmd/nsfs.js (1)
SensitiveString(38-38)src/sdk/bucketspace_nb.js (1)
dbg(7-7)
⏰ 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 (14)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
76-78: LGTM!The conditional cleanup correctly ensures
folder_deleteonly runs whenis_nc_coretestis true, matching the condition under whichconfig_rootis set in thebeforehook. This prevents attempting to delete an undefined path in containerized deployments.src/server/common_services/auth_server.js (1)
558-566: LGTM!The change to pass
[arn, account._id.toString()]as an array aligns with the updatedhas_bucket_policy_permissionsignature that now accepts an array of account identifiers. This enables policy principal matching against both the ARN and the account ID, supporting the PR's objective of ID-based validation.src/endpoint/s3/s3_rest.js (3)
255-257: LGTM!The simplification to always source
account_identifier_idfromaccount._idis correct. Both NC and containerized deployments now validate bucket policies against the account ID, aligning with the PR's objective of ID-based principal validation.
313-320: LGTM!The name-based permission check is now correctly scoped to NSFS NC deployments only, and restricted to root accounts (
account.owner === undefined). This maintains backward compatibility for NC while preventing IAM users from matching by name.
322-330: LGTM!The ARN-based permission check is now explicitly limited to containerized deployments (
!is_nc_deployment). This correctly separates the authorization paths: NC uses ID + name, while containerized uses ID + ARN.src/endpoint/s3/s3_bucket_policy_utils.js (3)
153-168: LGTM!The JSDoc update and
account_arrnormalization correctly document the function's ability to accept either a string or an array of account identifiers. The normalization patternArray.isArray(account) ? account : [account]ensures consistent array handling downstream.
204-222: LGTM!The refactored
_is_principal_fitfunction correctly usesaccount_arr.includes(principal.unwrap())for matching, allowing a principal to match any identifier in the array (ID, ARN, or name). The wildcard checkprincipal.unwrap() === '*'is evaluated first, maintaining correct anonymous access handling.
239-251: LGTM!The signature changes to
is_statement_fit_of_method_arrayand_is_statements_fitcorrectly propagateaccount_arrthrough the call chain, ensuring consistent array-based principal matching throughout the policy evaluation flow.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (5)
68-70: LGTM!The addition of
user_a_account_idanduser_b_account_idvariables provides a consistent way to reference account IDs across tests, abstracting the difference between NC (_id) and containerized (id) response formats.
127-134: LGTM!The account ID extraction correctly handles the different response formats: NC deployments use
_idwhile containerized deployments useid. The added console logging will aid debugging.
156-157: LGTM!The principal construction correctly differentiates between NC (account name) and containerized (ARN from account ID) deployments, aligning with how each deployment validates principals.
315-362: LGTM!The test cases for principal by account ID correctly use
user_a_account_idin both the policy Sid and Principal AWS fields, properly testing ID-based authorization for both NC and containerized deployments.
482-484: Tests enabled for both deployment types.The removal of
this.skip()guards for NC-only tests (// if (!is_nc_coretest) this.skip();) enables these account-ID-based policy tests to run on both NC and containerized deployments. This aligns with the PR goal of enabling ID-based validation across deployment types.Also applies to: 517-519, 583-585
src/server/system_services/bucket_server.js (1)
572-573: No issues found - async callback handling is correct.The callback is properly awaited by
validate_s3_policyat line 303 ofs3_bucket_policy_utils.js:const account = await get_account_handler(principal);. The Promise returned byget_account_by_principalis correctly handled.
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
from
0020baa to
2a88698
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/endpoint/s3/s3_rest.js (1)
252-332: Public access block is not enforced for ARN-based principal checksIn the containerized branch you call
has_bucket_policy_permissionforaccount_identifier_arnwithpublic_access_block?.restrict_public_bucketsas a bare boolean, sodisallow_public_accessis never set for that path. As a result, statements withPrincipal: "*"can still allow access via the ARN check even whenrestrict_public_bucketsis enabled, weakening the bucket public access block semantics.Mirror the ID-based call and pass an options object so wildcard principals are ignored consistently when public access is restricted:
- // Check bucket policy permission against ARN only for containerized deployments. - // Policy permission is validated by account arn - if (!is_nc_deployment && permission_by_id !== "DENY") { - permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets - ); - dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn); - } + // Check bucket policy permission against ARN only for containerized deployments. + // Policy permission is validated by account arn + if (!is_nc_deployment && permission_by_id !== "DENY") { + permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( + s3_policy, + account_identifier_arn, + method, + arn_path, + req, + { disallow_public_access: public_access_block?.restrict_public_buckets } + ); + dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn); + }src/endpoint/s3/s3_bucket_policy_utils.js (1)
204-222:_is_principal_fitdoes not handle string principals anymore
_is_principal_fitnow unconditionally callsprincipal.unwrap(), but principals in stored policies can be plain strings (e.g.,"*", account IDs, ARNs) as well asSensitiveStringinstances (seevalidate_s3_policyandallows_public_access). For string principals this will throw at runtime and break policy evaluation.You should normalize the principal value to a string in a type-safe way before comparison:
function _is_principal_fit(account_arr, statement, ignore_public_principal = false) { let statement_principal = statement.Principal || statement.NotPrincipal; let principal_fit = false; statement_principal = statement_principal.AWS ? statement_principal.AWS : statement_principal; for (const principal of _.flatten([statement_principal])) { - dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account_arr); - if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { - if (ignore_public_principal && principal.unwrap() === '*' && statement.Principal) { + const principal_val = typeof principal === 'string' ? principal : principal.unwrap(); + dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal_val, account_arr); + if ((principal_val === '*') || account_arr.includes(principal_val)) { + if (ignore_public_principal && principal_val === '*' && statement.Principal) { // Ignore the "fit" if ignore_public_principal is requested continue; } principal_fit = true; break; } } return statement.Principal ? principal_fit : !principal_fit; }This restores compatibility with existing policies/tests that use string principals while still supporting
SensitiveString-wrapped values.
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
397-404:a_principalnow correctly uses user A’s account ID in complex policy testsRecomputing
a_principalwithuser_a_account_idfor the containerized branch fixes the earlier issue where the DENY-by-name statement was unintentionally targeting user B in these conflict-policy tests.
🧹 Nitpick comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
68-70: Account ID test setup is consistent; consider small cleanupsThe introduction of
user_a_account_id/user_b_account_idand using them for ID/ARN-based policies looks consistent with how IDs are used elsewhere, and it gives you a clear separation between NC (_id) and containerized (id) cases.Two minor nits you might consider:
- The
console.log('user_*_account_details', ...)lines are useful for debugging but can be noisy in regular test runs; you may want to drop them once debugging is done.- To avoid ever mixing raw ObjectIDs with strings, you could normalize
user_*_account_idtoString(...)in both branches and then reuse that everywhere (including the ARN construction), instead of sometimes reaching back to*_account_details.id.Also applies to: 127-135, 155-158
src/server/system_services/bucket_server.js (1)
526-567: ARN/ID principal resolution is sound; verify if name-based principals are still neededThe new helpers:
account_exists_by_principal_arn(principal_as_string)correctly:
- Validates the IAM ARN format via
AWS_IAM_ARN_REGEXP.- Resolves
...:rootto an account by_id.- Resolves
...:user/.../usernameto an IAM user account via the${username}:${account_id}email convention.
get_account_by_principal(principal)then:
- Treats values starting with
arn:aws:iam::as ARNs and delegates to the above.- Otherwise looks up
system_store.data.accountsby_id.toString().- Returns a boolean, which
validate_s3_policyuses only for truthiness, so the change in return type is safe.One thing to double-check:
get_account_by_principalintentionally does not handle plain account names/emails. If any bucket policy entry points (especially NC/NSFS ones) still rely onPrincipalbeing an account name rather than ID/ARN, those policies will now be rejected as malformed. If that’s not intended, you may want to extendget_account_by_principalwith a name/email lookup branch for those environments.Also applies to: 569-591
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(4 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(11 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/api/iam/test_iam_basic_integration.js
- src/server/common_services/auth_server.js
🧰 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/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (5)
📓 Common learnings
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.
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-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/endpoint/s3/s3_rest.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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/system_services/bucket_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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (4)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/test/integration_tests/api/s3/test_s3_bucket_policy.js (17)
s3_bucket_policy_utils(15-15)s3_policy(1701-1711)s3_policy(1728-1738)s3_policy(1755-1765)s3_policy(1782-1790)s3_policy(1807-1815)s3_policy(1832-1840)s3_policy(1857-1857)s3_policy(1877-1887)s3_policy(1899-1908)s3_policy(1920-1929)s3_policy(1941-1950)s3_policy(1962-1977)s3_policy(2118-2127)s3_policy(2144-2153)s3_policy(2170-2179)s3_policy(2197-2206)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(303-303)_(4-4)src/server/common_services/auth_server.js (5)
account(104-104)account(189-195)account(318-322)s3_bucket_policy_utils(19-19)_(4-4)
src/endpoint/s3/s3_bucket_policy_utils.js (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (17)
policy(183-192)policy(201-209)policy(218-227)policy(235-249)policy(265-273)policy(288-296)policy(316-324)policy(338-346)policy(411-416)policy(446-451)policy(487-492)policy(521-526)policy(552-557)policy(586-591)policy(617-622)policy(647-656)_(13-13)src/endpoint/iam/iam_utils.js (1)
statement_principal(723-723)
src/server/system_services/bucket_server.js (2)
src/server/system_services/account_server.js (10)
SensitiveString(15-15)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)system_store(17-17)src/api/account_api.js (1)
SensitiveString(4-4)
⏰ 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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
153-189: Account array normalization in policy checks looks correctNormalizing
methodtomethod_arrandaccounttoaccount_arr, then threadingaccount_arrthroughis_statement_fit_of_method_array/_is_statements_fit, matches the new use cases (e.g.,[ARN, account_id]principals) and keeps deny/allow handling consistent.No issues from my side with this structure.
Also applies to: 239-252
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
315-362: ID-based bucket policy tests cover key allow/deny combinationsThe new tests that use
user_a_account_id(principal by account ID, and the various mixed allow/deny-by-ID vs by-name conflict cases) provide good coverage for the updated ID-first matching semantics inauthorize_request_policy. They exercise:
- Pure ID principals (allowing user A, denying B).
- Interactions between ID-based and name-based principals across multiple statements.
- Precedence of DENY over ALLOW when the same identity is referenced by both name and ID.
These look consistent with the intended behavior and complement the name-based tests gated on
is_nc_coretest.Also applies to: 387-395, 406-611
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
from
2a88698 to
93dc942
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/endpoint/s3/s3_rest.js (1)
322-330: Bug: passing boolean instead of options object tohas_bucket_policy_permissionin ARN pathIn the ARN path you call:
permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets );But
has_bucket_policy_permissionnow destructures its last parameter as an object:async function has_bucket_policy_permission(policy, account, method, arn_path, req, { disallow_public_access = false, should_pass_principal = true } = {}) { ... }Passing a boolean will throw at runtime when
public_access_block?.restrict_public_bucketsistrue/false(cannot destructure properties from a boolean), impacting containerized deployments where this flag is set and ARN evaluation is reached.Apply this fix to always pass an options object:
- if (!is_nc_deployment && permission_by_id !== "DENY") { - permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets - ); + if (!is_nc_deployment && permission_by_id !== "DENY") { + permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( + s3_policy, + account_identifier_arn, + method, + arn_path, + req, + { disallow_public_access: public_access_block?.restrict_public_buckets } + ); }
🧹 Nitpick comments (2)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
75-79: Guardingfolder_deletewithis_nc_coretestis correct for non-NC runsTying the cleanup to
is_nc_coretestmatches whereconfig_rootis initialized and avoids deleting with an undefined path in containerized runs. Iffs_utils.folder_deleteis async, considerawaiting it to ensure Mocha waits for cleanup, but that’s optional and consistent with prior behavior.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
127-135: Optional: remove or gate noisy debug logs in setupThe
console.log('user_a_account_details', ...)/console.log('user_b_account_details', ...)lines are useful during development but will spam test output in normal runs. Consider removing them or guarding behind an env flag.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(5 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(11 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🧰 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/integration_tests/api/iam/test_iam_basic_integration.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/endpoint/s3/s3_rest.jssrc/server/common_services/auth_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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/common_services/auth_server.jssrc/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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/common_services/auth_server.jssrc/server/system_services/bucket_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/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/server/common_services/auth_server.jssrc/server/system_services/bucket_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.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (5)
src/test/integration_tests/api/iam/test_iam_basic_integration.js (1)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/endpoint/s3/s3_rest.js (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(303-303)_(4-4)
src/server/common_services/auth_server.js (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)
src/server/system_services/bucket_server.js (3)
src/util/account_util.js (2)
SensitiveString(9-9)system_store(12-12)src/server/system_services/account_server.js (10)
SensitiveString(15-15)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)system_store(17-17)src/api/account_api.js (1)
SensitiveString(4-4)
⏰ 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: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (13)
src/server/common_services/auth_server.js (1)
631-636: Principal array (ARN + ID) wiring inhas_bucket_action_permissionlooks correctUsing
account.owner._id.toString()/account._id.toString()for ARN creation and passing[arn, account._id.toString()]intohas_bucket_policy_permissionmatches the new principal-array semantics ins3_bucket_policy_utilsand system_store account shapes in this file.src/endpoint/s3/s3_rest.js (2)
255-257: ID-based policy checks wiring is consistentDefining
account_identifier_id = account._idand using it as the primary principal for both NC and containerized flows aligns with the move to ID-based authorization and matches how principals are validated elsewhere.
301-321: NC name-based fallback logic matches documented behaviorThe NC path that:
- checks
permission_by_idfirst, then- falls back to
permission_by_nameonly whenaccount.owner === undefinedis consistent with the comment that NC supports principals by account name for backward compatibility while preferring IDs. The allow/deny precedence (
DENYshort‑circuit) remains intact.src/endpoint/s3/s3_bucket_policy_utils.js (4)
153-189: Principal array normalization inhas_bucket_policy_permissionis soundNormalizing
accountintoaccount_arrand threading it through deny/allow evaluation lets callers pass either a single principal or an array (e.g.,[ARN, id]) without changing existing semantics. The implicit-deny vs explicit-ALLOW/DENY logic remains unchanged.
204-222: Principal matching viaaccount_arr.includes()correctly supports multi-identifier principalsUpdating
_is_principal_fitto compareprincipal.unwrap()againstaccount_arrallows a caller to supply multiple identifiers (ID, ARN, etc.) for the same account and still get correct matches, while preserving handling of'*'andignore_public_principal.
239-259: Refactored statement-fit helpers correctly propagateaccount_arrand options
is_statement_fit_of_method_arrayand_is_statements_fitnow consistently passaccount_arrand the{ disallow_public_access, should_pass_principal }options through, keeping IAM inline-policy evaluation (whereshould_pass_principal === false) and bucket-policy principal checks aligned.
374-377: Root ARN construction now uses stringified account IDSwitching
create_arn_for_rootcalls inget_bucket_policy_principal_arntoaccount._id.toString()avoids implicit object interpolation and aligns with other ID-based principal construction in the PR.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (4)
68-70: Account ID derivation for tests matches NC vs containerized layoutsInitializing
user_a_account_id/user_b_account_idfrom_idin NC and from.idin containerized runs aligns with how accounts are represented in each deployment mode and lets the same tests exercise ID-based principals across both.
315-362: ID-based principal tests correctly exercise bucket policy behaviorThe new “principal by account ID” tests for:
put/get bucket policy, andputObjectpermissionscorrectly use
user_a_account_idin bothSidandPrincipal.AWS, and validate that user A is allowed while user B is denied. This matches the intended shift to ID-based principals.
387-411: Rebindinga_principalto ID/ARN keeps complex-policy tests alignedReassigning
a_principalfor the complex “conflict statements” suite usingcreate_arn_for_root(user_a_account_id.toString())on containerized deployments keeps those tests consistent with the rest of the ARN-based principal logic.
2112-2306: New ARN-principal tests provide good negative and positive coverageThe “Bucket policy with ARN principal” describe block adds:
- Multiple negative cases (invalid principal value, malformed ARN, bad account ID, non-existent IAM user ARN) that all assert
MalformedPolicy.- Positive cases for valid account-root ARNs covering both
GetObjectandPutObject, plus a mismatch case ensuring other accounts are denied.All are correctly skipped on NC (
is_nc_coretest), so they focus on containerized behavior where ARN-based principals are expected. This is a solid test addition for the new validation and evaluation paths.src/server/system_services/bucket_server.js (2)
526-546: ARN-based principal resolution helper is consistent with new IAM conventions
account_exists_by_principal_arn()correctly:
- Validates the ARN via
AWS_IAM_ARN_REGEXP.- Distinguishes root vs user ARNs using the suffix and segment 5.
- Resolves root ARNs by account
_id.toString()and IAM-user ARNs by the synthesized${username}:${account_id}email.This matches the introduced IAM-email convention and is an appropriate central place for ARN→account existence checks.
569-591: Wiringget_account_by_principalintovalidate_s3_policyis appropriatePassing
principal => get_account_by_principal(principal)intovalidate_s3_policyensures bucket policies are validated against existing accounts (by ID or ARN, and—after adjustment—by name), and the subsequent public-access-block check remains unchanged.
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
from
93dc942 to
78b78c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
162-189: Fix principal type handling in_is_principal_fitto avoid runtime errorsThe new array-based principal logic is good, but
_is_principal_fitnow assumes everyprincipalhas an.unwrap()method:if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { ... }Elsewhere in this module (e.g.,
validate_s3_policy), principals are explicitly handled as either plain strings orSensitiveString:(typeof principal === 'string') ? principal !== '*' : principal.unwrap() !== '*'With the current implementation, any policy that stores principals as plain strings (very common for JSON-based policies:
"Principal": {"AWS": "..."}or"*") will causeprincipal.unwrapto beundefinedand throw at evaluation time, breaking all authorization paths that hit these statements.You can fix this by normalizing principals to strings and slightly tightening
account_arrhandling:@@ async function has_bucket_policy_permission(policy, account, method, arn_path, req, { disallow_public_access = false, should_pass_principal = true } = {}) { @@ - const method_arr = Array.isArray(method) ? method : [method]; - const account_arr = Array.isArray(account) ? account : [account]; + const method_arr = Array.isArray(method) ? method : [method]; + // Normalize account into an array; allow empty when no principal is supplied + const account_arr = (account === undefined || account === null) + ? [] + : (Array.isArray(account) ? account : [account]); @@ -function _is_principal_fit(account_arr, statement, ignore_public_principal = false) { +function _is_principal_fit(account_arr, statement, ignore_public_principal = false) { @@ - for (const principal of _.flatten([statement_principal])) { - dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account_arr); - if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { - if (ignore_public_principal && principal.unwrap() === '*' && statement.Principal) { + for (const principal of _.flatten([statement_principal])) { + const principal_str = typeof principal === 'string' ? principal : principal.unwrap(); + dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal_str, account_arr); + if ((principal_str === '*') || account_arr.includes(principal_str)) { + if (ignore_public_principal && principal_str === '*' && statement.Principal) { // Ignore the "fit" if ignore_public_principal is requested continue; } @@ - return statement.Principal ? principal_fit : !principal_fit; + return statement.Principal ? principal_fit : !principal_fit; }This keeps the new multi-principal behavior (including
[arn, account_id]arrays) while remaining compatible with both string andSensitiveStringprincipals and avoiding runtime exceptions.Also applies to: 204-222, 239-259, 374-377
♻️ Duplicate comments (1)
src/server/system_services/bucket_server.js (1)
528-567: Re‑add name‑based principal validation for NC buckets inget_account_by_principal
get_account_by_principalnow only accepts principals that are:
- A valid IAM ARN resolvable by
account_exists_by_principal_arn, or- A string equal to some
account._id.toString().Principals specified by account name (e.g.,
"alice") are rejected as invalid, which conflicts with:
- Existing NC behavior/tests such as
should put/get bucket policy - principal by account nameintest_s3_bucket_policy.js(NC-only).- The comment in
s3_rest.authorize_request_policythat NC supports principals by account name for backward compatibility.This is the same regression previously flagged in an earlier review; the issue still exists in the current implementation. To preserve NC compatibility while keeping containerized deployments ID/ARN-centric, you can add a name-based fallback after the ID lookup:
async function get_account_by_principal(principal) { const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; const is_principal_arn = principal_as_string.startsWith('arn:aws:iam::'); if (is_principal_arn) { const principal_by_arn = await account_exists_by_principal_arn(principal_as_string); dbg.log3('get_account_by_principal: principal_by_arn', principal_by_arn); if (principal_by_arn) return true; } else { - const account = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); - const principal_by_id = account !== undefined; - dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); - if (principal_by_id) return true; + // ID-based lookup (containerized & NC) + const account_by_id = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); + const principal_by_id = account_by_id !== undefined; + dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); + if (principal_by_id) return true; + + // Backward-compatible name-based lookup (primarily for NC) + const account_by_name = system_store.data.accounts.find(acc => + acc.name && acc.name.unwrap && acc.name.unwrap() === principal_as_string + ); + const principal_by_name = account_by_name !== undefined; + dbg.log3('get_account_by_principal: principal_by_name', principal_by_name); + if (principal_by_name) return true; } return false; }You may also want to update the function’s docstring to reflect that it returns a boolean “exists” flag rather than an account object.
This keeps containerized flows on ID/ARNs while restoring NC’s documented/supported name-based principals.
Also applies to: 569-575
🧹 Nitpick comments (2)
src/endpoint/s3/s3_rest.js (1)
252-347: Confirmaccount_identifier_idis always a string for policy checksThe new flow (ID → name for NC, ID → ARN for containerized) looks correct and matches the intent to prioritize ID-based evaluation. One nuance:
account_identifier_idis set toaccount._idand then passed tohas_bucket_policy_permission, which ultimately compares principals usingaccount_arr.includes(principal_str).If
req.object_sdk.requesting_account._idis not already a plain string in all deployments (e.g., remains an ObjectId in some paths), ID-based principals in policies will never match and only the name/ARN fallbacks will be effective.To make this robust across NC and containerized, consider normalizing to string at the source:
- const account_identifier_id = account._id; + const account_identifier_id = account._id && account._id.toString();This keeps behavior unchanged when
_idis already a string but protects you if an ObjectId ever reaches this layer.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
68-70: Clarify account ID source and consider trimming debug logsThe introduction of
user_a_account_id/user_b_account_idand their use in principals/denies looks consistent with the move to ID-based principals, but there are a couple of minor cleanups you might consider:
- In containerized mode you now rely on
user_*_account_details.idto equalaccount._id.toString(). If that invariant ever changes, ID-based principal validation inget_account_by_principal(which compares against_id.toString()) will break silently. It may be safer (and clearer) to derive the exported IDs explicitly from_id.toString()when possible.- The
console.log('user_*_account_details', ...)lines are useful when debugging but can be noisy in regular integration runs; consider dropping or gating them if they’re no longer needed.Example tweak:
- user_a_account_id = is_nc_coretest ? user_a_account_details._id : user_a_account_details.id; + user_a_account_id = is_nc_coretest + ? user_a_account_details._id + : user_a_account_details._id.toString(); @@ - user_b_account_id = is_nc_coretest ? user_b_account_details._id : user_b_account_details.id; + user_b_account_id = is_nc_coretest + ? user_b_account_details._id + : user_b_account_details._id.toString(); @@ - console.log('user_a_account_details', user_a_account_details); + // console.log('user_a_account_details', user_a_account_details); @@ - console.log('user_b_account_details', user_b_account_details); + // console.log('user_b_account_details', user_b_account_details);This keeps tests aligned with how principals are validated while reducing noise.
Also applies to: 120-135, 155-158, 387-395
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(5 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/common_services/auth_server.js(1 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(11 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/api/iam/test_iam_basic_integration.js
- src/server/common_services/auth_server.js
🧰 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/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (5)
📓 Common learnings
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.
📚 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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/endpoint/s3/s3_rest.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/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (4)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(303-303)_(4-4)src/server/system_services/bucket_server.js (3)
account(561-561)_(7-7)_(1803-1810)src/util/account_util.js (7)
account(30-42)account(196-196)account(304-304)account(325-325)account(356-356)account(622-622)_(4-4)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
src/endpoint/iam/iam_utils.js (1)
statement_principal(723-723)
⏰ 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
🔇 Additional comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
2112-2307: New ARN principal tests thoroughly exercise validation pathsThe “Bucket policy with ARN principal” suite looks solid:
- Negative cases cover invalid principal values, malformed ARNs, wrong account IDs, and IAM user ARNs, and now explicitly
assert.failwhenputBucketPolicydoes not throw.- Positive cases cover valid root account ARNs for both read and write paths, plus a cross-account negative check (
valid_arn_bwiths3_a.putObject).This provides good end-to-end coverage of the new ARN-based validation in
bucket_server.get_account_by_principalandaccount_exists_by_principal_arn.
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
from
78b78c4 to
19dbd17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
204-222: Make _is_principal_fit robust to string vs SensitiveString principalsHere you unconditionally call
principal.unwrap():for (const principal of _.flatten([statement_principal])) { dbg.log1(..., principal, account_arr); if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { ... } }Elsewhere (
validate_s3_policy,allows_public_access), principals are treated asstring | SensitiveStringand guarded withtypeof principal === 'string' ? ... : principal.unwrap(). With the current code:
- If a policy principal is a plain string (e.g.,
'arn:aws:iam::...:root'or'*'),principal.unwrapwill beundefined, leading toTypeError: principal.unwrap is not a functionat evaluation time instead of a cleanAccessDenied/MalformedPolicy.To align with the rest of the file and avoid runtime errors, extract a string first:
function _is_principal_fit(account_arr, statement, ignore_public_principal = false) { @@ - for (const principal of _.flatten([statement_principal])) { - dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account_arr); - if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { + for (const principal of _.flatten([statement_principal])) { + const principal_str = typeof principal === 'string' ? principal : principal.unwrap(); + dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal_str, account_arr); + if ((principal_str === '*') || account_arr.includes(principal_str)) { - if (ignore_public_principal && principal.unwrap() === '*' && statement.Principal) { + if (ignore_public_principal && principal_str === '*' && statement.Principal) { // Ignore the "fit" if ignore_public_principal is requested continue; }This keeps the new array-based matching while safely handling both raw strings and SensitiveStrings, consistent with
validate_s3_policyandallows_public_access.
🧹 Nitpick comments (4)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
162-189: Clarify return semantics of has_bucket_policy_permission in JSDocThe new
has_bucket_policy_permissionimplementation looks consistent: it normalizesmethodandaccountto arrays, checks DENY statements first, then ALLOW statements, and finally returns'IMPLICIT_DENY'.To aid future readers, consider updating the JSDoc to explicitly document the return type and values, e.g.:
/** * ... * @returns {'ALLOW' | 'DENY' | 'IMPLICIT_DENY'} */This makes it clearer at call sites (like
authorize_request_policyand IAM policy checks) that they must compare against string literals rather than booleans.src/endpoint/s3/s3_rest.js (2)
252-333: Normalize account_identifier_id to a string to avoid ID/principal type mismatchesIn
authorize_request_policyyou now use:const account_identifier_id = account._id; ... permission_by_id = await has_bucket_policy_permission( s3_policy, account_identifier_id, method, arn_path, req, ... );and later compare policy principals via
_is_principal_fit, which does:account_arr.includes(principal_str);where
principal_stris a string derived from the policy document. For containerized flows,account._idmay still be an ObjectId when loaded intorequesting_accountbefore RPC serialization, while principals in bucket policies are always strings. Relying on implicit serialization risks silent mismatches ('id' !== ObjectId('id')).To make this robust and self-documenting, coerce to string up front:
- const account_identifier_id = account._id; + const account_identifier_id = account._id && account._id.toString();(or
String(account._id)).That keeps the new ID-based principal support intact for both NC and containerized deployments, and ensures consistent comparison in
has_bucket_policy_permission.
334-347: Owner-based principal checks for IAM users are correct; consider tiny naming/log nitThe new block:
- Uses
get_owner_account_id(account)to derive the root account ID.- Builds
owner_account_identifier_arnviacreate_arn_for_root(owner_account_id).- Checks permissions with
[owner_account_identifier_arn, owner_account_id].This correctly mirrors the auth_server path and ensures IAM users inherit DENY/ALLOW semantics expressed against their owner root account, which aligns with S3 behavior.
Minor nit: the debug log still refers to
permission_by_arn_owner:dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_owner);You might want to adjust the log string to
permission_by_ownerfor clarity, but functionally the logic looks good.src/server/system_services/bucket_server.js (1)
548-567: Align get_account_by_principal JSDoc and simplify wiring into validate_s3_policyTwo small points in
get_account_by_principal:
- The JSDoc still says “Validate and return account by principal ARN and account id”, but the function now returns a boolean (existence) instead of an account object, which is exactly what
validate_s3_policyexpects. Consider updating the comment to e.g.:/** * Check whether a bucket policy principal refers to an existing account. * @param {SensitiveString | string} principal * @returns {Promise<boolean>} */
- In
put_bucket_policyyou can pass the function directly instead of wrapping it:- await bucket_policy_utils.validate_s3_policy(req.rpc_params.policy, bucket.name, - principal => get_account_by_principal(principal)); + await bucket_policy_utils.validate_s3_policy( + req.rpc_params.policy, + bucket.name, + get_account_by_principal + );Functionally equivalent, slightly clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(5 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(11 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/integration_tests/api/iam/test_iam_basic_integration.js
- src/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧰 Additional context used
🧠 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/server/common_services/auth_server.jssrc/endpoint/s3/s3_rest.jssrc/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/common_services/auth_server.jssrc/endpoint/s3/s3_rest.jssrc/endpoint/s3/s3_bucket_policy_utils.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/common_services/auth_server.jssrc/endpoint/s3/s3_rest.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/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/common_services/auth_server.jssrc/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (4)
src/server/common_services/auth_server.js (3)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/endpoint/s3/s3_rest.js (5)
account(252-252)account(358-358)s3_bucket_policy_utils(10-10)owner_account_id(337-337)owner_account_identifier_arn(338-338)src/endpoint/iam/iam_utils.js (2)
owner_account_id(78-78)owner_account_id(851-851)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(303-303)dbg(5-5)src/endpoint/iam/iam_utils.js (2)
owner_account_id(78-78)owner_account_id(851-851)
src/endpoint/s3/s3_bucket_policy_utils.js (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (17)
policy(183-192)policy(201-209)policy(218-227)policy(235-249)policy(265-273)policy(288-296)policy(316-324)policy(338-346)policy(411-416)policy(446-451)policy(487-492)policy(521-526)policy(552-557)policy(586-591)policy(617-622)policy(647-656)_(13-13)src/endpoint/iam/iam_utils.js (1)
statement_principal(723-723)
src/server/system_services/bucket_server.js (2)
src/server/system_services/account_server.js (10)
SensitiveString(15-15)account(69-69)account(117-117)account(135-137)account(159-159)account(207-207)account(330-330)account(693-693)account(957-957)system_store(17-17)src/api/account_api.js (1)
SensitiveString(4-4)
⏰ 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/endpoint/s3/s3_bucket_policy_utils.js (2)
239-252: Account-array threading through statement evaluation looks correct
is_statement_fit_of_method_arrayand_is_statements_fitnow acceptaccount_arrand forward it into_is_principal_fit, while still honoringshould_pass_principalfor IAM inline policies:
- For bucket policies and anonymous access, principals are evaluated against the provided array (e.g.
[ARN, id]).- For IAM user inline policies (
should_pass_principal === false),principal_fitis forced totrue, so only action/resource/condition are checked, which matches AWS semantics.This change correctly generalizes the principal matching logic without altering existing behavior for IAM policies.
374-377: Root-account ARN now consistently uses stringified account_idUsing
account._id.toString()when building the root-account ARN:create_arn_for_root(account._id.toString());is the right choice and avoids subtle mismatches when policy principals store IDs as strings. This is consistent with the S3 REST path, where IDs are already strings due to RPC serialization.
src/server/system_services/bucket_server.js (1)
528-546: ARN principal resolution logic is sound; ensure AWS_IAM_ARN_REGEXP covers both root and user cases
account_exists_by_principal_arnvalidates principal ARNs and maps:
...:root→ account by_id....:user[/path/]<username>→ account by email${username}:${account_id}.The structure matches your forward
create_arn_for_user/create_arn_for_roothelpers and should correctly resolve both root and IAM-user principals as long asAWS_IAM_ARN_REGEXPaccepts both forms, which it appears intended to do.No functional issues spotted here.
src/server/common_services/auth_server.js (2)
643-660: I'll help you verify the review comment by checking the implementation ofget_owner_account_idand whether it handles both account.owner shapes correctly. Let me start by locating and examining this function.
<function_calls>
rg -n "get_owner_account_id" src --type=js -B 2 -A 10
</function_calls>
<function_calls>
fd -name "iam_utils*" src
</function_calls>
631-637: I'm encountering persistent cloning failures that prevent me from accessing the repository to verify the concerns in the review comment. Since I cannot run shell scripts to:
- Confirm whether
iam_utilsactually exportscreate_arn_for_userandcreate_arn_for_root- Compare the implementations and output semantics with
s3_bucket_policy_utilshelpers- Check for other usages of these functions in the codebase
I need to mark this review comment for manual verification:
Verify that iam_utils exports create_arn_for_user and create_arn_for_root with consistent semantics
The review comment raises two valid structural concerns:
API existence: Confirm that
iam_utilsactually exportscreate_arn_for_user()andcreate_arn_for_root()functions. If these don't exist, the code will fail at runtime.Consistency and duplication: Verify whether
s3_bucket_policy_utils.get_bucket_policy_principal_arn()(already used ins3_rest.js) produces identical output to the current approach. If so, consolidating to use the shared utility would prevent future divergence.A developer should:
- Examine
src/server/common_services/iam_utils.jsto confirm the function exports- Compare implementations with
s3_bucket_policy_utilshelpers- Consider consolidating to a single canonical implementation if feasible
| const is_nc_deployment = Boolean(req.object_sdk.nsfs_config_root); | ||
| const account_identifier_name = is_nc_deployment ? account.name.unwrap() : account.email.unwrap(); | ||
| const account_identifier_id = is_nc_deployment ? account._id : undefined; | ||
| // Both NSFS NC and containerized will validate bucket policy against acccount id. |
There was a problem hiding this comment.
In NC, the account identifier by ID served both accounts and users, as we didn't have the ARN yet.
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_permission_by_id
branch
from
19dbd17 to
dc3f318
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
151-158: Principal construction for NC vs containerized and re-assignment ofa_principalnow match intent
admin_principal,a_principal, andb_principalnow:
- Use plain names (
user_a,user_b) for NC coretest.- Use
create_arn_for_root(<account_id>)for containerized runs.Before the “complex policies (conflict statements)” tests,
a_principalis re-initialized as:
user_afor NC.create_arn_for_root(user_a_account_id.toString())for containerized.This fixes the earlier issue where
a_principalin containerized tests could be built from the wrong account ID, and aligns the deny-by-name / deny-by-ID tests with the intended actor (user_a).Also applies to: 397-404
src/server/system_services/bucket_server.js (1)
548-567:get_account_by_principaldrops name-based validation again; likely breaks NC “principal by account name” policiesCurrent implementation only treats a principal as valid if:
- It’s an IAM ARN resolvable by
account_exists_by_principal_arn, or- It matches some
account._id.toString()(ID-based principal).It no longer checks for principals specified by account name, even though:
- NC-specific tests like
should put/get bucket policy - principal by account name(gated byis_nc_coretest) still construct bucket policies withPrincipal: { AWS: user_a_account_details.name }.s3_rest.authorize_request_policystill has explicit support for NC name principals (viapermission_by_namechecks).With this implementation, a new or updated policy that uses
"alice"(or any account name) as a principal will be rejected as invalid duringvalidate_s3_policy, which appears to reintroduce the regression called out in the earlier review. This risks breaking NC scenarios and those NC-only tests.I recommend restoring a name-based lookup fallback in the non-ARN branch so that NC name principals remain valid alongside ID/ARN principals.
A concrete patch:
async function get_account_by_principal(principal) { const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; const is_principal_arn = principal_as_string.startsWith('arn:aws:iam::'); if (is_principal_arn) { const principal_by_arn = await account_exists_by_principal_arn(principal_as_string); dbg.log3('get_account_by_principal: principal_by_arn', principal_by_arn); if (principal_by_arn) return true; } else { - const account = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); - const principal_by_id = account !== undefined; - dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); - if (principal_by_id) return true; + // ID-based lookup + const account_by_id = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); + const principal_by_id = account_by_id !== undefined; + dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); + if (principal_by_id) return true; + + // Backward-compatible name-based lookup (primarily for NC) + const account_by_name = system_store.data.accounts.find(acc => + acc.name && acc.name.unwrap && acc.name.unwrap() === principal_as_string + ); + const principal_by_name = account_by_name !== undefined; + dbg.log3('get_account_by_principal: principal_by_name', principal_by_name); + if (principal_by_name) return true; } return false; }This keeps containerized behavior focused on IDs/ARNs while maintaining NC compatibility for existing name-based policies and tests.
🧹 Nitpick comments (5)
src/server/common_services/auth_server.js (1)
631-661: Owner-based ARN/ID checks look correct; just ensureget_owner_account_idsemantics match auth_server account shapeLogic here correctly:
- Builds principal array
[arn, account._id.toString()]so bucket policies can match by ARN or ID.- Adds an owner-account check using
const owner_account_id = iam_utils.get_owner_account_id(account);and passes[owner_account_identifier_arn, owner_account_id], while preserving DENY precedence and owner-allow semantics.Given that in
auth_serveraccounts come directly fromsystem_storeandaccount.owneris an ObjectId reference (with._id), please double-check thatiam_utils.get_owner_account_id(account)is implemented to handle this object form (as opposed to the string form used ins3_rest) so both IAM and root accounts resolve consistently. Based on learnings, this is the main cross-context gotcha to verify.If you want to make this more obviously robust, you could add a brief comment near the
get_owner_account_id(account)call noting that this path receives a system_store account whereowneris an ObjectId, to avoid future regressions.src/endpoint/s3/s3_rest.js (1)
252-333: ID/name/ARN principal ordering is sensible; verifyaccount._idis correct ID in all deploymentsThis block now:
- Uses
account_identifier_id = account._idfor both NC and containerized deployments.- Keeps NC-only name-based checking (
permission_by_name) gated byis_nc_deployment && account.owner === undefined.- Restricts ARN-based checking (
permission_by_arn) to non-NC deployments.This preserves:
- Backward compatibility for NC name principals.
- ID-first evaluation with DENY precedence (
permission_by_id === "DENY"short-circuits).- Environment-specific principal types (name vs ARN).
Please confirm that in both NC and containerized flows
req.object_sdk.requesting_account._idindeed matches the identifier used in bucket policy principals (for NC,_id; for containerized, the string account ID you validate inbucket_server). If any flow still relies onaccount.id, you may need to adjust this line or normalize in the account serialization.If you confirm
_idand the validated ID are always the same string, consider updating the nearby comment to make that guarantee explicit.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
68-70: Account ID derivation looks right; consider dropping debug logs
user_a_account_id/user_b_account_idcorrectly select_idfor NC andidfor containerized, matching how principals are formed in policies elsewhere in this file.- The two
console.logcalls foruser_a_account_detailsanduser_b_account_detailsare useful for debugging but will add noise to regular test runs; consider removing them or guarding behind an env flag if you no longer need them.Also applies to: 127-135
2112-2305: ARN principal negative/positive tests look solid; clarify IAM-user-ARN “invalid” commentThe new “Bucket policy with ARN principal” suite:
Negative tests:
- Cover non-ARN values, structurally invalid ARNs, ARNs with non-existent account IDs, and IAM user ARNs that don’t correspond to any account.
- Correctly call
assert.failafterputBucketPolicywhen no error is thrown, ensuring the test fails if validation regresses.Positive tests:
- Verify that policies with valid root ARNs for
user_b_account_idallow the expected operations fors3_band deny fors3_a.Minor nit at Line 2197: the comment
// IAM user ARN formate for account ID(user_b_account_id)has a typo and could more clearly explain why that IAM user ARN is considered invalid (e.g., “no such IAM user exists for this account ID”).You could tweak the comment to something like:
- // IAM user ARN formate for account ID(user_b_account_id) + // Example IAM user ARN for this account ID; no such user exists, so principal must be rejectedsrc/server/system_services/bucket_server.js (1)
528-546: ARN parsing and account lookup are correct; consider minor naming/doc tweaksThe
account_exists_by_principal_arnhelper:
- Correctly:
- Validates the ARN against
AWS_IAM_ARN_REGEXP.- Splits into parts and distinguishes:
- Root ARNs like
arn:aws:iam::<account_id>:root(resolving to account by_id).- User ARNs like
arn:aws:iam::<account_id>:user/.../<user_name>(resolving viaget_account_by_emailwithuser_name:account_id).Minor nits:
- Variable names
root_sufixanduser_sufixare misspelled; renaming toroot_suffix/user_suffixwould improve clarity.- The JSDoc says “Validate and return account by principal ARN” but the function returns either an account object or
undefined; that’s fine, but you might want to explicitly mention the return type in the doc for readability.These are cosmetic and don’t affect behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/endpoint/s3/s3_bucket_policy_utils.js(4 hunks)src/endpoint/s3/s3_rest.js(5 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/bucket_server.js(2 hunks)src/test/integration_tests/api/iam/test_iam_basic_integration.js(1 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(11 hunks)src/util/account_util.js(0 hunks)
💤 Files with no reviewable changes (1)
- src/util/account_util.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/s3/s3_bucket_policy_utils.js
- src/test/integration_tests/api/iam/test_iam_basic_integration.js
🧰 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/integration_tests/api/s3/test_s3_bucket_policy.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/server/common_services/auth_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/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/common_services/auth_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/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/common_services/auth_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/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/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (2)
src/server/common_services/auth_server.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/endpoint/iam/iam_utils.js (3)
result(757-757)owner_account_id(78-78)owner_account_id(851-851)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(303-303)src/endpoint/iam/iam_utils.js (2)
owner_account_id(78-78)owner_account_id(851-851)
⏰ 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 (3)
src/endpoint/s3/s3_rest.js (1)
334-347: Owner-based ARN/ID check for IAM users aligns with bucket-policy semanticsFor IAM users on non-NC deployments, this new block:
- Derives
owner_account_id = get_owner_account_id(account)and a root ARN for the owner.- Checks the bucket policy against
[owner_account_identifier_arn, owner_account_id].- Throws immediately on
permission_by_owner === "DENY"and only treats it as ALLOW in the final return condition.This achieves the intended behavior:
- IAM users inherit bucket access from their owner account when the policy targets the owner by ARN or ID.
- Explicit DENY on the owner also denies the IAM user, preserving AWS-like semantics where DENY overrides ALLOW.
No functional issues here; the change looks good.
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
315-362: ID-based principal tests correctly exercise ALLOW semanticsThe “principal by account ID” tests now:
- Use
Sidstrings interpolated withuser_a_account_idfor clarity.- Set
Principal: { AWS: user_a_account_id }and exercise both PutBucketPolicy and subsequent PutObject flows.These test cases are well-aligned with the new ID-based validation in
bucket_server.put_bucket_policyand S3 authorization, and they complement the NC name-based tests above.src/server/system_services/bucket_server.js (1)
569-590: Async principal validation integration withvalidate_s3_policylooks good
put_bucket_policynow:
- Calls
validate_s3_policywith a principal-check callback:principal => get_account_by_principal(principal).- Enforces
block_public_policyvsallows_public_accessbefore persisting the policy.Assuming
validate_s3_policyis already prepared to await an async validator (or treat a returned Promise correctly), this wiring is appropriate and keeps all principal-format knowledge localized inget_account_by_principal.If not already covered by tests, it’s worth confirming that
validate_s3_policyawaits (orthens) the callback result, rather than assuming a synchronous boolean.
2 participants
Describe the Problem
Explain the Changes
Principal validation
S3 Rest authorize request policy
GAP: IAM User test cases for ARN and user ID test cases are missing
Testing Instructions:
Un integration test
test_s3_bucket_policy.jsuser2_idcould be ID of different account or IAM user ID of IAM user(not belongs to account1)4. Verify the
user2havebucket1access.Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.