IAM | put_bucket_policy Principal validation updated with ARN #9277
Conversation
WalkthroughAdds ARN-aware principal validation and ARN-based permission checks across bucket policy evaluation and authorization; exposes account fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant S3_Rest as s3_rest
participant Auth as auth_server
participant BucketSrv as bucket_server
participant Store as system_store
rect rgba(234,245,255,0.9)
Client->>S3_Rest: bucket action request
S3_Rest->>Auth: authorize_request_policy(request, account)
Auth->>Auth: compute requester ARN via iam_utils
Auth->>BucketSrv: has_bucket_action_permission(bucket, requester_arn, action)
end
rect rgba(240,255,232,0.9)
BucketSrv->>BucketSrv: evaluate bucket policy principals
alt principal matches IAM ARN regex
BucketSrv->>BucketSrv: principal_validation_handler(principal)
alt ARN ends with ":root"
BucketSrv->>Store: get_account_by_id(account_id)
else user ARN
BucketSrv->>Store: get_account_by_email(constructed_email)
end
else non-ARN principal
BucketSrv->>Store: existing principal handling (email/other)
end
BucketSrv->>Auth: return allow/deny
Auth->>S3_Rest: allow/deny
S3_Rest->>Client: response
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/system_services/bucket_server.js(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-08-08T13:13:42.192Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.192Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/server/system_services/bucket_server.js
🧬 Code graph analysis (1)
src/server/system_services/bucket_server.js (3)
src/server/system_services/schemas/account_schema.js (1)
SensitiveString(4-4)src/server/system_services/account_server.js (16)
SensitiveString(15-15)require(13-13)require(21-21)require(22-22)require(23-23)system_store(17-17)account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)req(119-119)req(383-383)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(280-280)
⏰ 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 (2)
src/server/system_services/bucket_server.js (2)
14-14: LGTM: SensitiveString import added correctly.The import is necessary for ARN principal validation and follows the correct path used elsewhere in the codebase.
524-541: Original review premise is incorrect; the code correctly uses account._id as the ARN account identifier.The codebase uses MongoDB ObjectID strings (not 12-digit AWS account IDs) as account identifiers in ARN format. The test suite confirms this pattern:
arn:aws:iam::${account_config_file._id}:rootandarn:aws:iam::${owner_account_id}:user....The comparison on line 535 (
account._id.toString() === account_id) is correct and will match properly. The ARN structure validation on line 532 ensuresarn_parts[4]exists safely before extraction.Minor observations:
- Line 537: Using
includes('user')is loose and could match unintended strings; consider more precise position-based validation.- Line 539:
arn_path_parts[arn_path_parts.length - 1]assumes the ARN path has the expected structure; consider defensive validation.Likely an incorrect or invalid review comment.
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 (7)
src/api/account_api.js(2 hunks)src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(3 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(2 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(30 hunks)src/test/integration_tests/api/sts/test_sts.js(4 hunks)
🧰 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.jssrc/test/integration_tests/api/sts/test_sts.js
🧬 Code graph analysis (5)
src/server/system_services/account_server.js (2)
src/util/account_util.js (6)
account(32-44)account(198-198)account(306-306)account(331-331)account(370-370)account(645-645)src/server/system_services/system_server.js (4)
account(592-592)account(778-778)account(799-799)account(1145-1145)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/test/utils/coretest/coretest.js (1)
src/server/common_services/auth_server.js (3)
account(104-104)account(189-195)account(318-322)src/server/system_services/account_server.js (8)
account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)
src/test/integration_tests/api/sts/test_sts.js (2)
src/server/common_services/auth_server.js (3)
account(104-104)account(189-195)account(318-322)src/server/system_services/account_server.js (8)
account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)
src/endpoint/s3/s3_rest.js (2)
src/server/common_services/auth_server.js (6)
iam_utils(18-18)arn(552-553)account(104-104)account(189-195)account(318-322)req(32-32)src/test/integration_tests/api/s3/test_s3_bucket_policy.js (12)
s3_policy(1693-1703)s3_policy(1720-1730)s3_policy(1747-1757)s3_policy(1774-1782)s3_policy(1799-1807)s3_policy(1824-1832)s3_policy(1849-1849)s3_policy(1869-1879)s3_policy(1891-1900)s3_policy(1912-1921)s3_policy(1933-1942)s3_policy(1954-1969)
src/server/common_services/auth_server.js (1)
src/endpoint/s3/s3_rest.js (4)
iam_utils(15-15)arn(258-259)account(255-255)s3_bucket_policy_utils(10-10)
⏰ 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: Build Noobaa Image
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/server/common_services/auth_server.js (1)
552-555: Fix ARN construction: pass owner ID, not owner objectLine 552 currently feeds the whole
account.ownerobject intocreate_arn_for_user, so the resulting ARN becomesarn:aws:iam::[object Object]:user/.... That will never match any policy principal, effectively denying IAM users access. Please pass the owner’s_idstring instead before interpolating.- const arn = account.owner ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_account(account._id); + const arn = account.owner ? + iam_utils.create_arn_for_user(account.owner._id.toString(), account.name.unwrap().split(':')[0], account.iam_path) : + iam_utils.create_arn_for_account(account._id.toString());Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/endpoint/s3/s3_rest.js (1)
258-259: Add defensive handling for username extraction.The code extracts the username by splitting
account.nameon ':' and taking the first part, assuming the formatusername:account_id. If the name format is unexpected or doesn't contain a colon,split(':')[0]will return the entire name, potentially causing incorrect ARN generation.Consider adding validation or a fallback:
- const arn = account.owner ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_account(account._id); + const account_name_unwrapped = account.name.unwrap(); + const username = account.owner ? account_name_unwrapped.split(':')[0] : null; + const arn = account.owner ? + iam_utils.create_arn_for_user(account.owner, username, account.iam_path) : + iam_utils.create_arn_for_account(account._id);Alternatively, verify the name format matches the expected pattern before extraction.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/s3/s3_rest.js(3 hunks)src/server/system_services/bucket_server.js(3 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(1 hunks)src/util/string_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/system_services/bucket_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/nsfs/test_nsfs_integration.js
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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/nsfs/test_nsfs_integration.js
🧬 Code graph analysis (2)
src/endpoint/s3/s3_rest.js (1)
src/server/common_services/auth_server.js (6)
iam_utils(18-18)arn(552-553)account(104-104)account(189-195)account(318-322)req(32-32)
src/test/integration_tests/nsfs/test_nsfs_integration.js (2)
src/test/external_tests/external_tests_utils.js (1)
rpc_client(37-37)src/test/utils/coretest/coretest.js (4)
rpc_client(87-90)rpc_client(227-231)rpc_client(413-413)
⏰ 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 (1)
310-313: LGTM! ARN-based permission check correctly replaces name-based check.The logic now uses the computed
arnfor policy evaluation instead ofaccount_identifier_name, aligning with the PR's shift to ARN-based principal validation. The condition properly gates the name-based check to non-NC mode, preventing redundant checks when ID-based validation is sufficient.src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
208-210: No changes needed—this pattern is consistent with the test suite.The concern about error handling is not supported by the codebase. The script output shows that all integration tests follow the identical pattern of calling
read_account()and directly accessing properties without explicit error checks. This is the accepted convention across 15+ test files, including other instances within the same file. Tests in this suite rely on natural test failure propagation if setup preconditions aren't met.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
364-364: Minor: Unnecessary async onmocha.describe.The
mocha.describecallback doesn't perform async operations directly, so theasynckeyword is unnecessary. While this doesn't cause issues, it's unconventional for Mocha test suites.Note: This pattern also appears at lines 900, 904, 1014, 1700, 1876, and 1990.
Apply this pattern to remove unnecessary async:
- mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', async function() { + mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', function() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/endpoint/s3/s3_rest.js(3 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(31 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(1 hunks)src/util/string_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/util/string_utils.js
- src/endpoint/s3/s3_rest.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 (1)
📓 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.171Z
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.
🧬 Code graph analysis (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/test/utils/coretest/coretest.js (1)
src/server/system_services/account_server.js (5)
account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
⏰ 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 (3)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
67-73: LGTM: Clean variable declarations.The new module-scoped variables appropriately store account information and principal identifiers for use across test cases.
118-156: Well-implemented principal construction logic.The setup correctly handles both NC coretest (account name) and containerized (ARN-based) deployment modes. The conditional reads ensure account details are fetched when not returned by
create_account, and the ARN format matches the expected pattern from the PR objectives.
203-243: Excellent: Consistent principal usage in test policies.The test correctly uses
a_principalandb_principalvariables throughout policy statements, ensuring the tests work correctly in both NC coretest (name-based) and containerized (ARN-based) environments.
| // Losing this value in-between, assigning it again | ||
| a_principal = is_nc_coretest ? user_a : `arn:aws:iam::${account_info_a._id.toString()}:root`; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this reassignment is necessary.
The comment suggests a_principal is "losing this value in-between", but since it's module-scoped and assigned in setup() (line 154), this reassignment appears unnecessary. If there's a genuine issue causing the value to be lost, that root cause should be investigated.
Please verify:
- Is there a specific scenario where
a_principalbecomes undefined between setup and this test block? - If not, consider removing this redundant assignment.
Run this script to check for any other code that might modify these principal variables:
🏁 Script executed:
#!/bin/bash
# Search for assignments to principal variables outside of setup and line 397
rg -n -C3 '\b(a_principal|b_principal|admin_principal)\s*=' --type=jsLength of output: 1850
I need to verify the scope of a_principal and understand the test structure better.Now I need to examine the actual structure of the test file to see how the variable is declared and what scope it's in:
🏁 Script executed:
#!/bin/bash
# Get context around lines 150-160 and 390-400
sed -n '1,30p' src/test/integration_tests/api/s3/test_s3_bucket_policy.js | head -30
echo "---"
sed -n '148,160p' src/test/integration_tests/api/s3/test_s3_bucket_policy.js
echo "---"
sed -n '385,405p' src/test/integration_tests/api/s3/test_s3_bucket_policy.jsLength of output: 313
Let me search for the file and examine its structure:
🏁 Script executed:
#!/bin/bash
# Find the file first
fd -name "test_s3_bucket_policy.js" -type f
# If found, get context around variable declarations and the reassignment
if [ -f "src/test/integration_tests/api/s3/test_s3_bucket_policy.js" ]; then
echo "=== LINES 1-30 (module start) ==="
head -30 src/test/integration_tests/api/s3/test_s3_bucket_policy.js
echo ""
echo "=== LINES 148-165 (setup/initial assignment) ==="
sed -n '148,165p' src/test/integration_tests/api/s3/test_s3_bucket_policy.js
echo ""
echo "=== LINES 380-410 (reassignment context) ==="
sed -n '380,410p' src/test/integration_tests/api/s3/test_s3_bucket_policy.js
fiLength of output: 3817
Remove the redundant reassignment of a_principal at line 397.
Based on the codebase search, a_principal is assigned only twice:
- Line 154 (in setup):
a_principal = is_nc_coretest ? user_a : arn:aws:iam::... - Line 397 (in test block): identical assignment
No code between these lines modifies a_principal, so the reassignment is unnecessary. The comment "Losing this value in-between, assigning it again" is not supported by actual code—the variable persists as a module-scoped value. Remove line 397 entirely, or if the value needs to be re-established for test isolation, the real issue should be investigated and documented instead.
🤖 Prompt for AI Agents
In src/test/integration_tests/api/s3/test_s3_bucket_policy.js around lines 396
to 397, remove the redundant reassignment of a_principal at line 397 — it
duplicates the setup assignment made at line 154 and nothing in between mutates
a_principal; simply delete that line so the test uses the module-scoped value
from setup, or if test isolation is required instead add a clear setup/reset in
the test's before/after hooks (do not reassign inline).
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
9c50668 to
063d384
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server/system_services/bucket_server.js (1)
517-524: Fix comment typos and examplesUse “principal”, “suffix”, and “user_name”; fix ARN examples.
-/* - validate ARN principle +/* + validate ARN principal 1. validate basic ARN, like arn prefix `arn:aws:iam::` - 2. If principal ARN end with `root sufix its a account and get account with id eg : arn:aws:iam::${account_id}:root - 3. if principle ARN contains `user` its a IAM user and get account with username and id - eg : arn:aws:iam::${account_id}:user/${iam_path}/${use_name} + 2. If principal ARN ends with `root` suffix, it's an account; use id, e.g. arn:aws:iam::${account_id}:root + 3. If principal ARN contains `user`, it's an IAM user; use account id and user name, + e.g. arn:aws:iam::${account_id}:user/${iam_path}/${user_name} account email = ${iam_user_name}:${account_id} */src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
396-403: Redundant a_principal reassignment — removeThe module‑scoped a_principal is already set in setup(). This duplicate line isn’t needed.
- // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : `arn:aws:iam::${account_info_a._id.toString()}:root`;
🧹 Nitpick comments (5)
src/endpoint/s3/s3_rest.js (1)
258-259: Safer account ARN constructionCoerce account._id to string to avoid accidental “[object Object]” in ARNs. This is no-op if already a string. Based on learnings.
- const arn = account.owner ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_account(account._id); + const arn = account.owner + ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) + : iam_utils.create_arn_for_account(String(account._id));Please confirm the parameter order for iam_utils.create_arn_for_user(account_id, user_name, iam_path) matches this call.
src/server/system_services/bucket_server.js (2)
525-541: Tighten ARN parsing (resource part, account_id, username)Avoid false positives and enforce minimal structure: validate 12‑digit account_id, match resource exactly ('root') or starting with 'user/', and ensure non‑empty username.
async function principal_validation_handler(principal) { const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; const root_sufix = 'root'; const user_sufix = 'user'; const arn_parts = principal_as_string.split(':'); if (!string_utils.AWS_ARN_REGEXP.test(principal_as_string)) { return; } - const account_id = arn_parts[4]; - if (principal_as_string.endsWith(root_sufix)) { + const account_id = arn_parts[4]; + const resource = arn_parts[5] || ''; + // enforce standard 12-digit AWS-style account id + if (!/^\d{12}$/.test(account_id)) return; + if (resource === root_sufix) { return system_store.data.accounts.find(account => account._id.toString() === account_id); - } else if (arn_parts[5] && arn_parts[5].startsWith(user_sufix)) { - const arn_path_parts = principal_as_string.split('/'); - const iam_user_name = arn_path_parts[arn_path_parts.length - 1].trim(); + } else if (resource.startsWith(user_sufix + '/')) { + const iam_user_name = resource.split('/').pop().trim(); + if (!iam_user_name) return; return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`)); - } + } else { + // unsupported IAM resource type in principal + return; + } }
543-547: Put policy now validates via ARN resolver — document the breaking changeEmail principals are no longer accepted here; ensure the migration in PR #9267 lands before release and add a short note in upgrade docs.
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
125-133: Remove noisy console logs from test setupThey add noise and aren’t assertions.
- console.log('user_a_account_details', account_info_a); ... - console.log('user_b_account_details', account_info_b);
149-156: Consider adding IAM user ARN coverageCurrent tests exercise account root ARNs; add at least one policy using arn:aws:iam::${account_id}:user/${iam_path}/${user_name} to validate the user branch of principal parsing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/api/account_api.js(2 hunks)src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(4 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(2 hunks)src/server/system_services/bucket_server.js(3 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(31 hunks)src/test/integration_tests/api/sts/test_sts.js(4 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(1 hunks)src/util/string_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/util/string_utils.js
- src/server/system_services/account_server.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/server/common_services/auth_server.js
- src/endpoint/iam/iam_utils.js
- src/test/integration_tests/api/sts/test_sts.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)
📓 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.171Z
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-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/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
📚 Learning: 2025-11-12T04:55:42.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/api/account_api.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (3)
src/server/system_services/bucket_server.js (5)
src/server/common_services/auth_server.js (3)
SensitiveString(12-12)system_store(9-9)account(104-104)src/server/system_services/account_server.js (16)
SensitiveString(15-15)require(13-13)require(21-21)require(22-22)require(23-23)system_store(17-17)account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)req(119-119)req(383-383)src/server/system_services/system_server.js (20)
require(19-19)require(20-20)require(22-22)require(45-45)system_store(32-32)system_store(1198-1198)account(592-592)account(778-778)account(799-799)account(1145-1145)req(288-288)req(344-349)req(825-825)req(893-893)req(968-968)req(1060-1060)req(1241-1241)req(1322-1322)req(1369-1369)bucket(237-242)src/manage_nsfs/manage_nsfs_validations.js (2)
string_utils(9-9)bucket_policy_utils(12-12)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(280-280)
src/endpoint/s3/s3_rest.js (1)
src/server/common_services/auth_server.js (6)
iam_utils(18-18)arn(552-553)account(104-104)account(189-195)account(318-322)req(32-32)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
src/test/utils/coretest/coretest.js (1)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
⏰ 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/api/account_api.js (1)
623-623: Surface area change: account_info now exposes _id, owner, iam_pathLooks good. Please update API docs and confirm responses never send null for these strings (omit when absent), or allow nulls in schema if needed.
Also applies to: 665-670
naveenpaul1
requested review from
aayushchouhan09,
jackyalbo,
shirady and
tangledbytes
shirady
left a comment
shirady
left a comment
There was a problem hiding this comment.
Adding a few comments on the code -
When there is a bucket policy, there are cases that I'm not sure are handled:
- In containerized principal as account-id (I saw only ARN, maybe I missed it).
- User access according to its owner's access
| type: 'object', | ||
| required: ['name', 'email', 'has_s3_access'], | ||
| properties: { | ||
| _id: { type: 'string'}, |
There was a problem hiding this comment.
I think it would be confusing that in containerized the account_info will have _id as string.
I thought maybe to change the property name (account_id), but I'm not sure.
We can ask someone from the team.
There was a problem hiding this comment.
why its confusing? We are returning id and account._id should explain itself.
There was a problem hiding this comment.
Why do you need to change something in STS?
There was a problem hiding this comment.
test_sts.js have test cases that test put_bucket_policy(). for that test case creating S3 bucket access policy. Here I am changing the Principal.
| account.email = user_a; | ||
| user_a_account_details = await rpc_client.account.create_account(account); | ||
| console.log('user_a_account_details', user_a_account_details); | ||
| account_info_a = user_a_account_details._id ? user_a_account_details : await rpc_client.account.read_account({ email: user_a }); |
There was a problem hiding this comment.
Why do you need to check that it has _id (user_a_account_details._id) ?
According to the API, it will not have.
Maybe you discussed it and you can share - why not add it to the reply API?
There was a problem hiding this comment.
This test is common for both NSFS NC and containerized Noobaa, in NSFC NC create_account() always respond back with ID.
containerized deployment we dont have account-id based principal validation, before it was base on email
Out of scope for this PR, This is only dealing with bucket policy principal change from email to ARN for new bucket policies. |
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
063d384 to
722f219
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/accountspace_nb.js (1)
210-221: Fix missed migration to iam_utils.get_iam_username (runtime error on error path)Line 215 still calls the removed
account_util.get_iam_usernamefunction in the error message, causing a runtime failure when the error path is triggered.- const message_with_details = `Create accesskey failed for the user with name ${account_util.get_iam_username(requested_account.email.unwrap())}.`; + const message_with_details = `Create accesskey failed for the user with name ${iam_utils.get_iam_username(requested_account.email.unwrap())}.`;
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
397-398: Remove redundant reassignment (already flagged) and fix critical bug if kept.This reassignment was already flagged in a previous review as unnecessary since
a_principalis module-scoped and nothing modifies it between setup and this test block.Additionally, line 398 has the same critical bug as line 155: missing parentheses on
.toStringmethod call.If this line is kept for any reason, apply this fix:
- a_principal = is_nc_coretest ? user_a : iam_utils.create_arn_for_root(account_info_a._id.toString); + a_principal = is_nc_coretest ? user_a : iam_utils.create_arn_for_root(account_info_a._id.toString());However, the recommended action is to remove lines 397-398 entirely as suggested in the previous review.
🧹 Nitpick comments (4)
src/endpoint/s3/s3_rest.js (1)
307-326: Tighten condition and remove duplicate log
- The name-check gate can be simplified; the left side is always true after the DENY short‑circuit.
permission_by_arnis logged twice.Apply:
- if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) { + if (account.owner === undefined) { permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission( s3_policy, account_identifier_name, method, arn_path, req, public_access_block?.restrict_public_buckets ); dbg.log3('authorize_request_policy: permission_by_name', permission_by_name); } if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied); // Containerized deployment always will have account_identifier_id undefined // Policy permission is validated by account arn if (!account_identifier_id) { 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); } - dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn); if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied);src/server/common_services/auth_server.js (1)
552-557: Reuse principal ARN helper and normalize owner/_id typesAvoid duplicating ARN assembly and handle both string/object owner forms.
Apply:
- const arn = account.owner ? iam_utils.create_arn_for_user(account.owner._id.toString(), account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_root(account._id); + const principal_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account);And pass
principal_arnbelow.This centralizes format quirks (owner as string vs object,
_idstringification).Please confirm
account.owneris always an object in this context; if not, the current. _id.toString()can throw.src/endpoint/s3/s3_bucket_policy_utils.js (1)
345-349: Normalize owner/_id to support both RPC and system_store shapesHarden
get_bucket_policy_principal_arnto accept owner as string or object and_idas object id.-function get_bucket_policy_principal_arn(account) { - const bucket_policy_arn = account.owner ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_root(account._id); - return bucket_policy_arn; -} +function get_bucket_policy_principal_arn(account) { + const owner_id = (account.owner && typeof account.owner === 'object') ? String(account.owner._id) : account.owner; + if (owner_id) { + return iam_utils.create_arn_for_user(owner_id, account.name.unwrap().split(':')[0], account.iam_path); + } + return iam_utils.create_arn_for_root(String(account._id)); +}src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
365-365: Remove unnecessaryasynckeyword from describe block.Mocha describe blocks are synchronous and don't benefit from the
asynckeyword. The tests inside can be async, but the describe block itself shouldn't be.Apply this diff:
- mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', async function() { + mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', function() {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/api/account_api.js(2 hunks)src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(3 hunks)src/sdk/accountspace_nb.js(5 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(2 hunks)src/server/system_services/bucket_server.js(3 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(32 hunks)src/test/integration_tests/api/sts/test_sts.js(4 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(2 hunks)src/util/account_util.js(1 hunks)src/util/string_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/util/string_utils.js
- src/server/system_services/bucket_server.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/server/system_services/account_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/sts/test_sts.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (4)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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/sdk/accountspace_nb.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/api/account_api.jssrc/endpoint/iam/iam_utils.jssrc/util/account_util.jssrc/endpoint/s3/s3_rest.js
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/server/common_services/auth_server.js
📚 Learning: 2025-11-13T07:56:23.606Z
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.606Z
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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (4)
src/sdk/accountspace_nb.js (11)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
iam_utils(8-8)src/server/common_services/auth_server.js (1)
iam_utils(18-18)src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
iam_utils(23-23)src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
iam_utils(27-27)src/endpoint/iam/ops/iam_list_user_policies.js (2)
iam_utils(5-5)params(14-18)src/endpoint/iam/ops/iam_list_user_tags.js (1)
iam_utils(5-5)src/endpoint/iam/ops/iam_tag_user.js (1)
iam_utils(5-5)src/endpoint/iam/ops/iam_untag_user.js (1)
iam_utils(5-5)src/endpoint/iam/ops/iam_delete_user_policy.js (1)
iam_utils(5-5)src/endpoint/iam/ops/iam_get_user_policy.js (1)
iam_utils(5-5)src/endpoint/iam/ops/iam_put_user_policy.js (1)
iam_utils(5-5)
src/test/integration_tests/api/sts/test_sts.js (2)
src/server/system_services/account_server.js (8)
account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)src/test/integration_tests/nsfs/test_nsfs_integration.js (2)
account(264-264)account(497-497)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/endpoint/iam/iam_utils.js (3)
require(6-6)require(7-8)require(10-10)src/test/utils/coretest/coretest.js (1)
src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/endpoint/s3/s3_rest.js (2)
src/server/common_services/auth_server.js (6)
s3_bucket_policy_utils(19-19)account(104-104)account(189-195)account(318-322)req(32-32)is_owner(544-545)src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(281-281)dbg(5-5)
⏰ 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 (9)
src/endpoint/s3/s3_rest.js (1)
256-258: LGTM: principal ARN derivation via utilityUsing
s3_bucket_policy_utils.get_bucket_policy_principal_arn(account)avoids duplicating ARN logic here. Based on learnings.Please confirm ARN evaluation is intentionally gated to containerized deployments only (i.e.,
!account_identifier_id) per PR scope.src/endpoint/iam/iam_utils.js (2)
65-67: LGTM: centralized IAM username parsingConsolidates username extraction in one place; matches existing conventions.
819-826: Export addition is consistent
exports.get_iam_username = get_iam_username;properly exposes the helper.src/api/account_api.js (1)
623-671: Public API shape changed: add docs and verify consumers
account_infonow includes_id,owner,iam_pathas strings. Please:
- Document these fields and note deployment differences (containerized vs NSFS) if any.
- Verify FE/SDK callers don’t expect
_id/ownerto be objects.Would you like a quick repo scan for usages that assume non-string
_id/owner?src/util/account_util.js (1)
16-19: LGTM: switch to iam_utils.get_iam_usernameImport alignment looks good.
src/sdk/accountspace_nb.js (1)
171-183: LGTM: list_users uses iam_utils.get_iam_usernameUsername extraction now consistent with the new utility.
src/test/integration_tests/api/sts/test_sts.js (1)
140-149: LGTM: tests updated to ARN principalsPolicies now use
arn:aws:iam::...:rootprincipals derived from account IDs; matches new validation path.Also applies to: 527-532
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
23-23: LGTM! Clean imports and variable declarations.The addition of
iam_utilsimport and the module-scoped variables for account info and principals properly support the ARN-based principal handling.Also applies to: 68-74
180-2116: LGTM! Consistent principal variable usage throughout tests.The systematic replacement of hardcoded principals with
a_principal,b_principal, andadmin_principalvariables is well-executed and maintains test correctness. The tests now properly support both NC coretest (name-based principals) and containerized deployments (ARN-based principals).
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
722f219 to
4015988
Compare
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
4015988 to
c25e7af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/server/system_services/bucket_server.js (1)
527-543: Strengthen validation for account_id and username extraction.The
AWS_IAM_ARN_REGEXPregex insrc/util/string_utils.js:21validates ARN format but has a critical gap: it uses\w{10,}for the account ID field instead of\d{12}, allowing non-numeric values and incorrect lengths. This means invalid account IDs likeaaaaaaaaaa(10 letters) or12345678901(11 digits) would pass the regex check.Combined with no additional validation in
principal_validation_handler, the function accepts and processes improperly formatted ARNs:
Line 535:
account_idextracted fromarn_parts[4]is not validated for:
- Non-empty value
- Exactly 12 digits (AWS account ID format)
Line 540: After
.trim(),iam_user_namecould be empty, creating invalid email format:${account_id}.Lines 536-542: Unsupported ARN types (role, federated users) silently return
undefinedwith no distinction from invalid ARNs.Apply these fixes:
async function principal_validation_handler(principal) { const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; const root_sufix = 'root'; const user_sufix = 'user'; const arn_parts = principal_as_string.split(':'); if (!string_utils.AWS_IAM_ARN_REGEXP.test(principal_as_string)) { return; } const account_id = arn_parts[4]; + // Validate account_id is a 12-digit number + if (!account_id || !/^\d{12}$/.test(account_id)) { + dbg.warn('principal_validation_handler: Invalid account_id in ARN', principal_as_string); + return; + } if (principal_as_string.endsWith(root_sufix)) { return system_store.data.accounts.find(account => account._id.toString() === account_id); } else if (arn_parts[5] && arn_parts[5].startsWith(user_sufix)) { const arn_path_parts = principal_as_string.split('/'); const iam_user_name = arn_path_parts[arn_path_parts.length - 1].trim(); + if (!iam_user_name) { + dbg.warn('principal_validation_handler: Empty username in ARN', principal_as_string); + return; + } return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`)); + } else { + dbg.warn('principal_validation_handler: Unsupported ARN type', principal_as_string); + return; } }
🧹 Nitpick comments (5)
src/endpoint/s3/s3_rest.js (1)
253-325: ARN-based permission check is correct; consider minor cleanupThe new
permission_by_arnflow correctly:
- Runs only when
account_identifier_idis falsy (containerized path), as intended.- Enforces DENY precedence before aggregating ALLOW with id/name and
is_owner.Two small cleanups you might consider:
- Compute
account_identifier_arnonly inside theif (!account_identifier_id)block to avoid doing ARN work for NSFS accounts that never use it.- Drop one of the two
dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn);calls to avoid duplicate logging.These are non-functional refactors and can be deferred.
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
68-75: Principal abstraction for NC vs containerized looks good; consider a small test helperThe
admin_info/account_info_*+admin_principal/a_principal/b_principalsetup cleanly separates:
- NC coretest: principals are account name/email.
- Containerized: principals are IAM-style root ARNs via
iam_utils.create_arn_for_root(...).This aligns the tests with the new ARN-based principal handling without duplicating policy construction logic per environment.
Given the same “if
is_nc_coretestthen name/email else ARN” pattern now appears here and intest_nsfs_integration.js, you might want to extract a tiny helper intest_utils(e.g.get_root_principal_for_env(account_info, fallbackNameOrEmail)) and reuse it across test suites to keep IAM/ARN details in one place. This is optional but would make future policy/principal changes easier to propagate.Also applies to: 119-157
src/util/string_utils.js (1)
21-21: IAM ARN regex works but is more permissive than AWS account format
AWS_IAM_ARN_REGEXPcorrectly distinguishes between...:rootand...:user/...and the rename makes its scope clear, but the account-id fragment:/^arn:aws:iam::\w{10,}:(?:root|user\/[\w\-\.\/]+)$/still allows:
- Non‑digit characters (because
\wincludes letters and_).- Variable length IDs (10+ chars) instead of the usual 12-digit AWS account ID.
If you want closer alignment to IAM ARNs, you could tighten that part to something like
\d{12}and simplify the character class in the user path. For example:const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\d{12}:(?:root|user\/[\w\-./]+)$/;This remains backwards-compatible for standard IAM ARNs while rejecting obviously invalid principals. Optional, not blocking.
Also applies to: 168-175
src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
27-31: NSFS test now correctly uses ARN principal for containerized; consider sharing helperIn
list buckets without uid, gidyou now derive the principal as:
iam_utils.create_arn_for_root(account_info_admin._id.toString())for containerized.That keeps NSFS semantics unchanged while exercising ARN-based principals for containerized deployments, which is exactly what this PR is trying to validate.
Given
test_s3_bucket_policy.jsuses the sameis_nc_coretest ? identifier : create_arn_for_root(...)pattern, it might be worth adding a small helper intest_utilsto centralize “root principal for current env” construction and reuse it here and in the S3 bucket-policy tests. This would keep tests in sync if ARN formation ever changes.Also applies to: 206-217
src/server/system_services/bucket_server.js (1)
517-526: Clean up diff artifact and enhance JSDoc.The documentation has been improved from previous reviews, but there are minor issues:
- Line 521: The
+prefix appears to be a leftover diff marker that should be removed.- JSDoc completeness: While the
@paramtag is present (addressing shirady's request), consider adding@returnsdocumentation to clarify that the function returns an account object when a valid ARN is found, orundefinedotherwise.Apply this diff:
-/** +/** validate ARN principal 1. validate basic ARN, like arn prefix `arn:aws:iam::` 2. If principal ARN ends with `root` suffix, it's an account and get account with id eg: arn:aws:iam::${account_id}:root - 3. if principal ARN contains `user`, it's an IAM user and get account with username and id + 3. if principal ARN contains `user`, it's an IAM user and get account with username and id eg: arn:aws:iam::${account_id}:user/${iam_path}/${user_name} account email = ${iam_user_name}:${account_id} @param {SensitiveString | String} principal Bucket policy principal + @returns {Object|undefined} Account object if valid ARN is found, undefined otherwise */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/api/account_api.js(2 hunks)src/endpoint/iam/iam_utils.js(2 hunks)src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(3 hunks)src/sdk/accountspace_nb.js(5 hunks)src/server/common_services/auth_server.js(2 hunks)src/server/system_services/account_server.js(2 hunks)src/server/system_services/bucket_server.js(3 hunks)src/test/integration_tests/api/s3/test_s3_bucket_policy.js(32 hunks)src/test/integration_tests/api/sts/test_sts.js(4 hunks)src/test/integration_tests/nsfs/test_nsfs_integration.js(2 hunks)src/util/account_util.js(1 hunks)src/util/string_utils.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/integration_tests/api/sts/test_sts.js
- src/server/system_services/account_server.js
- src/server/common_services/auth_server.js
- src/util/account_util.js
- src/api/account_api.js
- src/sdk/accountspace_nb.js
- src/endpoint/iam/iam_utils.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/nsfs/test_nsfs_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_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
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_bucket_policy_utils.jssrc/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.js
📚 Learning: 2025-11-13T07:56:23.606Z
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.606Z
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/nsfs/test_nsfs_integration.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.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
📚 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/server/system_services/bucket_server.js
📚 Learning: 2025-08-08T13:13:42.192Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.192Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/server/system_services/bucket_server.js
🧬 Code graph analysis (3)
src/test/integration_tests/nsfs/test_nsfs_integration.js (2)
src/endpoint/iam/iam_utils.js (3)
require(6-6)require(7-8)require(10-10)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
iam_utils(8-8)src/endpoint/iam/iam_utils.js (3)
require(6-6)require(7-8)require(10-10)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/server/system_services/bucket_server.js (3)
src/server/common_services/auth_server.js (2)
SensitiveString(12-12)system_store(9-9)src/server/system_services/account_server.js (16)
SensitiveString(15-15)require(13-13)require(21-21)require(22-22)require(23-23)system_store(17-17)account(54-54)account(102-102)account(121-123)account(145-145)account(193-193)account(316-316)account(679-679)account(943-943)req(119-119)req(383-383)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(281-281)
⏰ 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 (6)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
372-405: Complex-policy tests now exercise ARN principals tooThe complex policy suite that builds
deny_account_by_name_all_s3_actions_statementusinga_principalensures:
- NC:
a_principalis the account name, so tests continue to validate name-based principals as before.- Containerized:
a_principalis the root ARN, so the same tests now also cover DENY/ALLOW conflict resolution for ARN principals.That’s a nice way to validate the DENY precedence and wildcard
"*"behavior across both environments without duplicating test logic.src/server/system_services/bucket_server.js (2)
14-14: LGTM!The new imports are necessary for ARN validation and sensitive string handling in the
principal_validation_handlerfunction.Also applies to: 43-43
545-567: LGTM on the integration!The integration of
principal_validation_handleras a callback tovalidate_s3_policyis correctly implemented. The arrow function wrapper appropriately passes each principal for validation.src/endpoint/s3/s3_bucket_policy_utils.js (3)
8-8: LGTM!The new import is necessary for ARN construction in the
get_bucket_policy_principal_arnfunction.
355-355: LGTM!The export correctly exposes the new function to external modules.
345-349: The .toString() concern in the review comment is incorrect for this context.When
get_bucket_policy_principal_arnis called froms3_rest.js:256withreq.object_sdk.requesting_account, the account object is already RPC-serialized. According to confirmed learnings, account.owner is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. Similarly,account._idis also stringified during RPC serialization (as seen in account_server.js). The code correctly uses bothaccount.ownerandaccount._idwithout additional.toString()calls.The JSDoc and validation suggestions have merit as general code quality improvements, but they are optional rather than critical. The current implementation functions correctly for its intended use case.
Likely an incorrect or invalid review comment.
2 participants
Describe the Problem
Explain the Changes
Principal validation
arn:aws:iam::root sufix its a account and get account with id eg:aws:arn:${account_id}:root`userits a IAM user and get account with username and ideg :
aws:arn:${account_id}:user/${iam_path}/${use_name}account email = ${iam_user_name}:${account_id}
S3 Rest authorize request policy
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Summary by CodeRabbit
New Features
Tests