IAM | put_bucket_policy Principal validation updated with ARN #9277
Conversation
WalkthroughAdds IAM ARN-aware principal parsing/resolution and ARN constructors; introduces get_account_by_principal to resolve IAM ARNs (root/user) during bucket policy validation; passes requester ARN into bucket policy and authorization checks; extends account info with 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.95)
Client->>S3_Rest: bucket action request
S3_Rest->>Auth: authorize_request_policy(request, account)
Auth->>Auth: compute requester ARN via iam_utils (user or root)
Auth->>BucketSrv: has_bucket_action_permission(bucket, requester_arn, action)
end
rect rgba(240,255,232,0.95)
BucketSrv->>BucketSrv: evaluate bucket policy Principals
alt Principal matches AWS_IAM_ARN_REGEXP
BucketSrv->>BucketSrv: parse ARN (root vs user)
BucketSrv->>Store: resolve account by ARN/id/owner
else non-ARN Principal
BucketSrv->>Store: resolve principal by email/name (existing flow)
end
BucketSrv->>Auth: return allow/deny
Auth->>S3_Rest: allow/deny
S3_Rest->>Client: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
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
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
2 times, most recently
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.
@naveenpaul1, the plan is to add those (principal as account-id and users under the account) in a separate PR? |
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
c25e7af to
46c13f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
209-216: Extract principal computation to a reusable utility function.The logic to compute principals based on deployment type (NC vs. containerized) will be duplicated across multiple test files. Extract this pattern into a shared utility function to improve maintainability.
Consider adding a helper to
test_utils.js:/** * Get the principal identifier for bucket policies based on deployment type * @param {string} email - Account email * @returns {Promise<string>} Principal (account name for NC, ARN for containerized) */ async function get_bucket_policy_principal(email) { if (is_nc_coretest) { return email; } const account_info = await rpc_client.account.read_account({ email }); return iam_utils.create_arn_for_root(account_info._id.toString()); }Then use it as:
const principal = await get_bucket_policy_principal(EMAIL); const generated = generate_s3_policy(principal, first_bucket, ['s3:*']);Based on past review feedback.
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
119-157: Consider extracting principal computation to a shared utility.The setup logic correctly computes principals based on deployment type, but this pattern is duplicated across test files (also in test_nsfs_integration.js).
Extract to a shared helper in
test_utils.jsto reduce duplication and improve maintainability, as suggested in past review feedback.Based on past review feedback.
397-398: Remove redundant reassignment ofa_principal.The variable
a_principalis module-scoped and initialized insetup()at line 155. No code between setup and this test modifies the variable, so this reassignment is unnecessary.Apply this diff:
- // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); const deny_account_by_name_all_s3_actions_statement = { Sid: `Do not allow user ${user_a} any s3 action`, Effect: 'Deny', Principal: { AWS: [a_principal] },Based on past review feedback.
📜 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 (5)
- src/util/string_utils.js
- src/test/integration_tests/api/sts/test_sts.js
- src/server/system_services/account_server.js
- src/sdk/accountspace_nb.js
- src/api/account_api.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.jssrc/test/integration_tests/nsfs/test_nsfs_integration.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.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/server/common_services/auth_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/iam/iam_utils.jssrc/endpoint/s3/s3_rest.jssrc/util/account_util.jssrc/endpoint/s3/s3_bucket_policy_utils.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
📚 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
📚 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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (4)
src/server/common_services/auth_server.js (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(281-281)
src/server/system_services/bucket_server.js (3)
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/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(281-281)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (4)
src/endpoint/s3/s3_rest.js (2)
s3_bucket_policy_utils(10-10)account(253-253)src/server/common_services/auth_server.js (5)
s3_bucket_policy_utils(19-19)require(7-7)account(104-104)account(189-195)account(318-322)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(281-281)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/endpoint/s3/s3_bucket_policy_utils.js (3)
src/endpoint/s3/s3_rest.js (1)
account(253-253)src/util/account_util.js (3)
username(364-364)username(394-395)username(428-429)src/endpoint/iam/iam_utils.js (1)
basic_structure(41-41)
⏰ 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 (6)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
352-355: Fix IAM user ARN generation
account.owneris a populated object in the system store (with an_idfield). Passing that object straight intocreate_arn_for_userstringifies to"[object Object]", yielding an invalid principal ARN and making every policy lookup fail. Normalize the owner to its ID string before constructing the ARN. That keeps root accounts on the existing path while letting user accounts resolve correctly.- const bucket_policy_arn = account.owner ? create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : - create_arn_for_root(account._id); + const owner_id = typeof account.owner === 'object' ? account.owner._id : account.owner; + const iam_user_name = account.name.unwrap().split(':')[0]; + const bucket_policy_arn = owner_id ? + create_arn_for_user(owner_id.toString(), iam_user_name, account.iam_path) : + create_arn_for_root(account._id);⛔ Skipped due to learnings
Learnt from: naveenpaul1 Repo: noobaa/noobaa-core PR: 9277 File: src/endpoint/s3/s3_rest.js:258-261 Timestamp: 2025-11-12T04:55:42.193Z Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
27-27: LGTM - IAM utilities imported for ARN construction.The import is used appropriately at line 214 to create ARN principals for containerized deployments.
src/endpoint/s3/s3_rest.js (2)
256-256: LGTM - ARN computed for authorization.The account ARN is correctly computed for use in bucket policy evaluation.
325-325: LGTM - ARN-based permissions integrated into authorization decision.The final authorization check correctly includes ARN-based permissions alongside ID and name-based permissions, with DENY precedence properly enforced through earlier short-circuits.
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (2)
15-15: LGTM - Necessary imports and shared test data added.The import and module-scoped variables properly support ARN-based principal computation across test cases.
Also applies to: 68-74
204-204: LGTM - Policy statements correctly use computed principals.The bucket policies throughout the test suite now correctly use the computed principal variables (
a_principal,b_principal,admin_principal) that adapt to the deployment type (account names for NC, ARNs for containerized). This ensures tests work consistently across both deployment models.Also applies to: 222-222, 238-238, 244-244, 657-657, 678-678, 684-684, 690-690, 696-696
| 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 result = await s3_bucket_policy_utils.has_bucket_policy_permission( |
There was a problem hiding this comment.
Handle string and object owners when building ARN
In S3 REST flows the requesting account arrives over RPC with account.owner already serialized to a string (see prior learnings). Accessing account.owner._id throws, breaking authorization. Normalize the owner value the same way you do in the policy helper so both string and object forms work.
- 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 owner_id = typeof account.owner === 'object' ? account.owner._id : account.owner;
+ const iam_user_name = account.name.unwrap().split(':')[0];
+ const arn = owner_id ?
+ iam_utils.create_arn_for_user(owner_id.toString(), iam_user_name, account.iam_path) :
+ iam_utils.create_arn_for_root(account._id);Based on learnings
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 result = await s3_bucket_policy_utils.has_bucket_policy_permission( | |
| const owner_id = typeof account.owner === 'object' ? account.owner._id : account.owner; | |
| const iam_user_name = account.name.unwrap().split(':')[0]; | |
| const arn = owner_id ? | |
| iam_utils.create_arn_for_user(owner_id.toString(), iam_user_name, account.iam_path) : | |
| iam_utils.create_arn_for_root(account._id); | |
| const result = await s3_bucket_policy_utils.has_bucket_policy_permission( |
🤖 Prompt for AI Agents
In src/server/common_services/auth_server.js around lines 552 to 554, the code
assumes account.owner is an object and calls account.owner._id, which breaks
when owner is a serialized string; update to normalize owner first (e.g., if
typeof account.owner === 'string' use that string as ownerId, else extract
account.owner._id.toString()), then pass the normalized ownerId into
iam_utils.create_arn_for_user (keeping the existing
account.name.unwrap().split(':')[0] and account.iam_path), and preserve the
existing fallback to iam_utils.create_arn_for_root when account.owner is falsy.
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
46c13f3 to
221b447
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/sdk/accountspace_nb.js (1)
84-102: Fix leftoveraccount_util.get_iam_usernamereference increate_access_keyerror path.The migration to
iam_utils.get_iam_usernameinget_user,update_user,list_users, and the success paths looks good, butcreate_access_keystill uses the removed account_util helper in its catch block:const message_with_details = `Create accesskey failed for the user with name ${account_util.get_iam_username(requested_account.email.unwrap())}.`;Since
account_util.jsno longer exportsget_iam_username, this will become a runtimeTypeErrorif key generation fails. Reuse the IAM utility here as well, e.g.:- 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())}.`;The rest of the
get_iam_usernamecall sites in this file are consistent with the new helper.Also applies to: 104-142, 164-192, 198-227, 229-243
src/api/account_api.js (1)
623-670: Resolve duplicateownerproperty inaccount_infoschema.
owneris defined twice inaccount_info.properties(lines 665-667 and 766-768). Only the last one is effective, which is confusing and triggered the static-analysis warning.Recommend keeping a single
ownerdefinition (e.g., the one grouped with_idandiam_path) and removing the duplicate at the bottom:iam_user_policies: { type: 'array', items: { $ref: 'common_api#/definitions/iam_user_policy', } }, - owner: { - type: 'string' - }Also applies to: 766-768
♻️ Duplicate comments (3)
src/util/string_utils.js (1)
21-21: Tighten IAM ARN regex to enforce 12‑digit account IDs (and consider tests).
AWS_IAM_ARN_REGEXPcurrently uses\w{10,}for the account segment, which accepts non‑digit characters and variable length. To match AWS IAM ARNs more closely and avoid accepting malformed principals, consider switching to exactly 12 digits and, if desired, keeping the root/user branches distinct, e.g.:-const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\w{10,}:(?:root|user\/[\w\-\.\/]+)$/; +const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\d{12}:(?:root|user\/[\w\-\.\/]+)$/;You may also want a small unit test in
test_string_utils.test.jsto lock in the accepted/rejected forms.Also applies to: 175-175
src/test/integration_tests/nsfs/test_nsfs_integration.js (1)
27-27: NSFS test principal selection (EMAIL vs ARN) matches deployment semantics.Using
iam_utils.create_arn_for_root(account_info_admin._id.toString())for containerized runs keeps existing NSFS behavior while exercising the new ARN path where applicable. If you find yourself repeating this pattern in more tests, it could be worth extracting a small helper intest_utils, but the current change is fine as-is.Also applies to: 209-217
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
388-405: Remove redundant reassignment ofa_principalin conflict tests.
a_principalis a module-scoped variable initialized insetup()and not mutated betweenbefore(setup)and this block. The reassignment at lines 397-398 duplicates the original expression and the comment about "Losing this value in-between" is not supported by the code.You can safely drop this reassignment and just use the existing
a_principal:- // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); const deny_account_by_name_all_s3_actions_statement = { Sid: `Do not allow user ${user_a} any s3 action`, Effect: 'Deny', Principal: { AWS: [a_principal] },
🧹 Nitpick comments (4)
src/test/integration_tests/api/sts/test_sts.js (1)
81-84: STS/S3 tests’ ARN principals look correct; consider reusing the helper.Reading
account_info_*and usingarn:aws:iam::${account_info_X._id.toString()}:rootswitches the policies to stable account‑ID–based principals without changing test semantics, and the session-token test already usesiam_utils.create_arn_for_root. For consistency and to avoid string duplication, you could optionally construct all these root ARNs viaiam_utils.create_arn_for_root(account_info_X._id.toString()), but the current logic is functionally fine.Also applies to: 125-129, 138-149, 526-532
src/endpoint/s3/s3_rest.js (1)
258-259: Align ARN permission check with otherhas_bucket_policy_permissioncall sites.The new
account_identifier_arnandpermission_by_arnlogic (includingDENYshort‑circuit and the!account_identifier_idguard for containerized deployments) looks correct and matches the intended “ARN only for containerized” behavior. One thing to double‑check: for ID/name you now pass an options object,has_bucket_policy_permission(s3_policy, account_identifier_id, method, arn_path, req, { disallow_public_access: public_access_block?.restrict_public_buckets });but for ARN you pass the raw boolean:
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
has_bucket_policy_permissionexpects an options bag (as in the anonymous/ IAM calls), consider updating the ARN call for consistency:- 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 - ); + 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 } + );This keeps the public‑access‑block semantics aligned across all three principal forms.
Also applies to: 297-298, 319-329
src/endpoint/iam/iam_utils.js (1)
65-67: Centralizedget_iam_usernamehelper looks good; consider making it defensive.The new
get_iam_username(requested_account_name)that splits onIAM_SPLIT_CHARACTERSmatches the existingusername:account_idconvention and works for the updated call sites. To guard against accidentalundefined/empty input (e.g.,_list_access_keys_from_accountinaccount_utilcurrently passes_returned_username(...)directly), you might optionally make it defensive:-function get_iam_username(requested_account_name) { - return requested_account_name.split(iam_constants.IAM_SPLIT_CHARACTERS)[0]; -} +function get_iam_username(requested_account_name) { + if (!requested_account_name) return requested_account_name; + return requested_account_name.split(iam_constants.IAM_SPLIT_CHARACTERS)[0]; +}This avoids hard failures if a future caller ever passes
undefinedwhile keeping behavior unchanged for valid names.Also applies to: 825-825
src/util/account_util.js (1)
16-18: Centralizing IAM username/ARN handling inaccount_utillooks good; consider a small safety guard.Importing
get_iam_username/create_arn_for_user/get_action_message_titleand using them in_check_if_account_exists,_check_if_requested_is_owned_by_root_account,_throw_error_perform_action_on_another_root_account, and_throw_access_denied_errormakes the IAM error messages and ARNs consistent with the IAM utilities, which is a nice cleanup.The only edge case worth considering is
_list_access_keys_from_account, which calls:username: get_iam_username(_returned_username(requesting_account, account.name, on_itself)),
_returned_usernamecan returnundefinedfor some root/manager flows, leading to a.splitonundefinedinget_iam_username. Either makeget_iam_usernamedefensive (as suggested iniam_utils.js) or guard at this call site:- const members = []; + const members = []; ... - const member = { - username: get_iam_username(_returned_username(requesting_account, account.name, on_itself)), + const raw_username = _returned_username(requesting_account, account.name, on_itself); + const member = { + username: raw_username ? get_iam_username(raw_username) : undefined,This isn’t new behavior, but the refactor is a good opportunity to harden it.
Also applies to: 325-333, 390-400, 426-437, 455-457, 493-495, 563-576
📜 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 (3)
- src/server/system_services/bucket_server.js
- src/server/system_services/account_server.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/sts/test_sts.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/test/integration_tests/nsfs/test_nsfs_integration.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/util/account_util.jssrc/api/account_api.jssrc/sdk/accountspace_nb.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/iam/iam_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/api/account_api.jssrc/endpoint/s3/s3_rest.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/test/integration_tests/nsfs/test_nsfs_integration.js
🧬 Code graph analysis (5)
src/sdk/accountspace_nb.js (8)
src/server/common_services/auth_server.js (1)
iam_utils(18-18)src/endpoint/iam/ops/iam_list_user_policies.js (1)
iam_utils(5-5)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 (3)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)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_bucket_policy_utils.js (3)
src/endpoint/s3/s3_rest.js (2)
account(255-255)account(339-339)src/util/account_util.js (3)
username(364-364)username(394-395)username(428-429)src/endpoint/iam/iam_utils.js (1)
basic_structure(41-41)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)src/endpoint/s3/ops/s3_put_bucket_public_access_block.js (1)
public_access_block(12-12)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/endpoint/s3/s3_rest.js (1)
s3_bucket_policy_utils(10-10)src/server/common_services/auth_server.js (2)
s3_bucket_policy_utils(19-19)require(7-7)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
🪛 Biome (2.1.2)
src/api/account_api.js
[error] 665-667: This property is later overwritten by an object member with the same name.
Overwritten with this property.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property.
(lint/suspicious/noDuplicateObjectKeys)
⏰ 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/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
15-16: Principal setup for NC vs containerized looks consistent.Using
s3_bucket_policy_utils.create_arn_for_rootplusis_nc_coretest ? name : ARNto populateadmin_principal,a_principal, andb_principalgives consistent principals across all tests and aligns containerized behavior with ARN-based validation, while preserving NC-by-name/ID flows where tests explicitly gate onis_nc_coretest.No further changes needed here.
Also applies to: 68-75, 119-157
src/endpoint/s3/s3_bucket_policy_utils.js (1)
370-401: ARN construction and IAM path handling are consistent with AWS formats.
create_arn_for_root,create_arn_for_user,IAM_DEFAULT_PATH, andcheck_iam_path_was_setcorrectly shape ARNs like:
arn:aws:iam::ACCOUNT_ID:rootarn:aws:iam::ACCOUNT_ID:user/usernamearn:aws:iam::ACCOUNT_ID:user/custom/path/usernameand avoid appending the default
'/'path. Exporting these helpers for reuse in S3 policy evaluation and tests is appropriate.Also applies to: 407-408
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
221b447 to
8059fd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
365-405: Remove redundanta_principalreassignment in conflict-policy describeThe reassignment of
a_principalhere duplicates the calculation already done insetup()and nothing in this file mutatesa_principalbetween those points, so the “Losing this value in-between” comment is not backed by the code. You can safely drop the comment and the extra assignment to simplify the test and avoid confusion about hidden state.- // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); const deny_account_by_name_all_s3_actions_statement = { Sid: `Do not allow user ${user_a} any s3 action`, Effect: 'Deny', - Principal: { AWS: [a_principal] }, + Principal: { AWS: [a_principal] }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT_D}/*`] };
🧹 Nitpick comments (6)
src/api/account_api.js (1)
623-667: Optional fields_idandiam_pathlook good and are backward‑compatibleAdding
_idandiam_pathas non‑required string properties onaccount_infoaligns with how account IDs and IAM paths are exposed elsewhere and won’t break existing consumers. If you think confusion around_idbeing a serialized ID vsObjectIdmay recur, consider a brief API doc note, but it’s not required.src/endpoint/iam/iam_utils.js (1)
65-67: Centralizedget_iam_usernamematches existing semanticsThe helper correctly returns the prefix before
IAM_SPLIT_CHARACTERS, matching how usernames are encoded elsewhere. All current call sites pass strings, so the simple split is sufficient. If you want to make this easier to discover later, a short JSDoc (param type, expected format, and return) would help, but not required.src/server/system_services/bucket_server.js (1)
517-544: ARN-basedget_account_by_principalbehaves correctly; consider small defensive tweakThe new
get_account_by_principalcleanly encapsulates ARN-based principal resolution:
- Validates the principal with
AWS_IAM_ARN_REGEXPbefore parsing.- Distinguishes root vs user ARNs using both
endsWith('root')and thearn_parts[5]guard to avoid misclassifyinguser/.../rootusernames.- For user ARNs, derives the IAM username from the last path segment and looks up the account via the
${iam_user_name}:${account_id}email pattern, which matches how IAM accounts are stored.- Wiring into
put_bucket_policyviavalidate_s3_policy(req.rpc_params.policy, bucket.name, principal => get_account_by_principal(principal))matches the expectedget_account_handlershape.Two minor, optional hardening points:
Empty username guard: if an ARN ends with
user/and no username,iam_user_namebecomes an empty string; returning early in that case would avoid a pointlessget_account_by_emailcall:} 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) return; return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`));
- Optional: drop
asyncif you don’t foresee asynchronous lookups here; it’s harmless, but removingasyncwould better reflect current behavior.Overall, the refactor is sound and aligns with the bucket policy validation flow.
Also applies to: 549-550
src/util/account_util.js (1)
16-18: Good consolidation of IAM helpers intoiam_utilsPulling
get_iam_usernamefrom../endpoint/iam/iam_utilsalongsidecreate_arn_for_userandget_action_message_titlekeeps IAM-specific logic in one place and avoids having a duplicate implementation inaccount_util. Existing call sites continue to behave the same with the centralized helper.src/endpoint/s3/s3_rest.js (1)
258-258: ARN-based bucket policy authorization is wired correctly; consider aligning options for clarityThe changes in
authorize_request_policylook solid:
account_identifier_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account)centralizes ARN construction.- ARN permission checks are only executed when
account_identifier_idis falsy (containerized case), so NSFS/NC continues to use ID/name-based principals only.- DENY precedence is enforced consistently — any
"DENY"from ID, name, or ARN immediately throwsAccessDeniedbefore considering ALLOW oris_owner, which matches the earlier review feedback.- The final ALLOW branch correctly considers all three (
permission_by_id,permission_by_name,permission_by_arn) plusis_owner.One minor consistency tweak you might consider: the ARN call passes a bare boolean as the last argument, while other call sites use an options object:
// ID/name: has_bucket_policy_permission(s3_policy, principal, method, arn_path, req, { disallow_public_access: public_access_block?.restrict_public_buckets }); // ARN: has_bucket_policy_permission( s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets );If
has_bucket_policy_permissionnow expects an options object, updating the ARN call to:- 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 - ); + 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 } + );would keep all call sites consistent and avoid surprises if the helper’s signature evolves further. Behavior should remain unchanged today if the helper already supports both forms.
Also applies to: 297-297, 319-329
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
68-75: Shared admin/account info and principal derivation look correct; consider minor cleanupsThe pattern of caching
admin_info,account_info_a/band derivingadmin_principal,a_principal,b_principalwithis_nc_coretest-based branching (names for NC, ARNs for containerized) is consistent with the PR intent and should exercise both modes correctly. The only nits:
- The
console.log('user_*_account_details', ...)lines are likely leftover debugging and could be removed or guarded to keep test output clean.- If you find yourself needing to recompute principals again, consider a small helper (e.g.
build_principals(is_nc_coretest, admin_info, account_info_a, account_info_b)) instead of duplicating expressions.Also applies to: 119-123, 126-133, 150-157
📜 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 (5)
- src/server/common_services/auth_server.js
- src/sdk/accountspace_nb.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/server/system_services/account_server.js
- src/endpoint/s3/s3_bucket_policy_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/api/sts/test_sts.jssrc/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_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/iam/iam_utils.jssrc/util/account_util.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/server/system_services/bucket_server.jssrc/endpoint/s3/s3_rest.jssrc/api/account_api.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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/api/account_api.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
🧬 Code graph analysis (4)
src/test/integration_tests/api/sts/test_sts.js (3)
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/nsfs/test_nsfs_integration.js (1)
account(264-264)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/endpoint/s3/s3_rest.js (3)
s3_bucket_policy_utils(10-10)account(255-255)account(339-339)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/server/system_services/bucket_server.js (2)
src/server/common_services/auth_server.js (1)
system_store(9-9)src/server/system_services/account_server.js (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_rest.js (2)
src/server/common_services/auth_server.js (5)
s3_bucket_policy_utils(19-19)account(104-104)account(189-195)account(318-322)req(32-32)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (10)
src/test/integration_tests/api/sts/test_sts.js (3)
81-84: LGTM! Account info retrieval setup is correct.The addition of account info variables and subsequent reads after account creation properly sets up the test to use ARN-based principals. The account IDs extracted from these objects are used to construct proper IAM ARNs in the bucket policy, aligning with the PR's goal of moving from email-based to ARN-based principal validation.
Also applies to: 126-126, 128-128, 138-138
140-149: LGTM! ARN-based bucket policy construction is correct.The S3 access policy correctly uses the ARN format
arn:aws:iam::${account_id}:rootfor account principals, which aligns with AWS IAM standards and the PR's objective of implementing ARN-based principal validation. The policy properly grants access to all three test accounts (a, b, and c) for comprehensive test coverage.
526-526: LGTM! Session token tests correctly updated to use ARN principals.The changes consistently apply the ARN-based principal format to the session token test suite, matching the pattern used in the STS tests. Reading
account_info_aliceand using its account ID to construct the ARNarn:aws:iam::${account_info_alice._id.toString()}:rootproperly aligns the bucket policy with the new ARN-based validation approach.Also applies to: 531-531
src/endpoint/iam/iam_utils.js (1)
825-825: Exportingget_iam_usernameis consistent with the new usageThe export wiring is correct and allows other modules (e.g.
account_util) to reuse the centralized helper instead of rolling their own.src/util/string_utils.js (1)
175-175: ExportingAWS_IAM_ARN_REGEXPis appropriateThe export is wired correctly so consumers (e.g.
bucket_server.get_account_by_principal) can share a single source of truth for IAM ARN validation; once the regex is updated, they will automatically benefit.src/server/system_services/bucket_server.js (1)
14-44: New imports are correctly scoped
SensitiveStringandstring_utilsare imported only to support principal resolution in this module and are used inget_account_by_principalas expected. No issues here.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (4)
15-15: Importings3_bucket_policy_utilskeeps tests aligned with production ARN logicUsing the same helper as the S3 endpoint to construct principals is a good choice and reduces drift between tests and implementation.
198-213: Usinga_principal/b_principalin early bucket-policy tests is consistent and fulfills ARN-vs-name dualityAll the updated statements that now use
{ AWS: a_principal }or{ AWS: b_principal }correctly route NC runs through account-name principals and containerized runs through ARN principals, while preserving the original allow/deny semantics of these tests (invalid resource, invalid action, basic put/get/list, and simple allow/deny combinations). This is exactly what you need to exercise the new principal validation path without breaking NC coverage.Also applies to: 215-231, 233-260, 649-727, 729-772, 773-834, 836-861
922-1012: Object-attributes policy tests now also cover ARN-based principals where applicableSwitching the principals in the get‑object‑attributes tests to
a_principalensures that, for containerized runs, these flows go through ARN-based principal evaluation while NC keeps using account names. The split between “allowed”, “split across 2 statements”, and “partial permissions” is preserved, so behavior should remain unchanged aside from the principal encoding.Also applies to: 1050-1076, 1078-1115, 1121-1140
1144-1163: Later policy, validation, and versioned-object tests are consistently migrated to principal variablesAcross the “deny bucket owner”, “system owner”,
NotPrincipal/NotResource/NotActionvalidation tests, public-policy status checks, and the versioned-object access test, usinga_principal,b_principal, andadmin_principalkeeps all assertions intact while routing containerized runs through ARN principals and NC through account names/IDs. The array vs scalar usages for AWS principals are also in line with AWS-style policy shapes. This migration looks coherent end-to-end.Also applies to: 1175-1195, 1643-1668, 1670-1699, 1703-1728, 1730-1755, 1757-1782, 1813-1857, 1901-1920, 2024-2065
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
8059fd6 to
a9e8fb7
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 (2)
src/sdk/accountspace_nb.js (2)
215-215: Complete the migration toiam_utils.get_iam_username.This line still uses
account_util.get_iam_username, which is inconsistent with the refactoring goal of centralizing IAM username logic iniam_utils. Additionally, it usesrequested_account.email.unwrap()instead ofrequested_account.name.unwrap(), which differs from the pattern used elsewhere in this file (lines 88, 113, 176, 221).Apply this diff:
- 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.name.unwrap())}.`;
236-241: Line 307 is missing theiam_utils.get_iam_usernametransformation.The codebase shows a consistent pattern: when returning the
usernamefield in response objects,iam_utils.get_iam_usernameis applied (lines 221 and 241). However, line 307 returns_returned_usernamedirectly without applying the transformation. This inconsistency should be fixed by wrapping the result withiam_utils.get_iam_username:username: iam_utils.get_iam_username(account_util._returned_username(requesting_account, requested_account.name.unwrap(), false))
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
397-398: Remove redundant reassignment ofa_principal.The variable
a_principalis already assigned in thesetup()function at line 155 and persists as a module-scoped variable. This reassignment is unnecessary as no code between setup and this point modifies the value. The comment "Losing this value in-between" is not supported by the code.Remove line 398 entirely:
- // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString());
🧹 Nitpick comments (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
357-368: Clarify JSDoc about account parameter structure.The function correctly derives ARN based on whether the account has an owner (IAM user) or not (root account). However, per past review feedback, the JSDoc should document that
account.owneris a string (account ID) from RPC serialization, not an object reference.Apply this diff to enhance the documentation:
/** * Return IAM ARN for account, if account dont have owner - * and User, if the user do have account ower + * and User ARN if the account has an owner * - * @param {Object} account + * @param {Object} account - Account object from RPC (account.owner is string ID if present) * @returns {string} */Based on learnings
📜 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/server/common_services/auth_server.js
- src/server/system_services/bucket_server.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/util/string_utils.js
- src/util/account_util.js
- src/endpoint/iam/iam_utils.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 (5)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/api/account_api.js
📚 Learning: 2025-11-18T07:00:17.636Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.636Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.jssrc/endpoint/s3/s3_bucket_policy_utils.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_rest.jssrc/api/account_api.jssrc/sdk/accountspace_nb.jssrc/endpoint/s3/s3_bucket_policy_utils.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/endpoint/s3/s3_bucket_policy_utils.js
🧬 Code graph analysis (5)
src/test/integration_tests/api/sts/test_sts.js (4)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
account_info_a(69-69)account_info_b(70-70)account(107-111)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)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/s3/test_s3_bucket_policy.js (4)
src/endpoint/s3/s3_rest.js (3)
s3_bucket_policy_utils(10-10)account(255-255)account(339-339)src/server/common_services/auth_server.js (5)
s3_bucket_policy_utils(19-19)require(7-7)account(104-104)account(189-195)account(318-322)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)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 (5)
s3_bucket_policy_utils(19-19)account(104-104)account(189-195)account(318-322)req(32-32)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
src/sdk/accountspace_nb.js (3)
src/util/account_util.js (3)
username(364-364)username(394-395)username(428-429)src/server/common_services/auth_server.js (1)
iam_utils(18-18)src/endpoint/iam/ops/iam_list_user_policies.js (2)
iam_utils(5-5)params(14-18)
src/endpoint/s3/s3_bucket_policy_utils.js (3)
src/endpoint/s3/s3_rest.js (2)
account(255-255)account(339-339)src/util/account_util.js (3)
username(364-364)username(394-395)username(428-429)src/endpoint/iam/iam_utils.js (1)
basic_structure(41-41)
⏰ 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 (21)
src/api/account_api.js (2)
623-623: LGTM! Account ID field added to support ARN-based principals.The addition of the
_idfield toaccount_infoaligns with the PR's objective to enable ARN-based bucket policy principal validation. This field is correctly typed as a string and appropriately optional.
665-667: LGTM! IAM path field added for ARN construction.The
iam_pathfield enables proper ARN construction for IAM users with custom paths, supporting the full ARN formatarn:aws:iam::{account_id}:user{iam_path}{username}.src/endpoint/s3/s3_bucket_policy_utils.js (5)
97-97: LGTM! IAM default path constant defined.The
IAM_DEFAULT_PATHconstant correctly represents AWS IAM's default path value/, which is used to determine when an IAM path was explicitly set versus using the default.
370-377: LGTM! Root account ARN constructor implemented correctly.The function correctly constructs ARNs in the format
arn:aws:iam::{account_id}:rootper AWS IAM ARN specification for root account principals.
379-393: LGTM! IAM user ARN constructor handles path logic correctly.The function properly constructs user ARNs with three cases:
- No username: returns base structure with trailing slash
- Custom IAM path: includes the path between
userand username- Default path: uses simple format
user/{username}This aligns with AWS IAM ARN specifications.
395-401: LGTM! IAM path check helper is concise and correct.The function correctly identifies when a non-default IAM path has been set by checking for both presence and inequality with the default path.
407-408: LGTM! New ARN utilities properly exported.The exports make the ARN construction functions available to authorization flows in s3_rest.js and other modules that need to work with ARN-based principals.
src/test/integration_tests/api/sts/test_sts.js (4)
81-83: LGTM! Account info variables added to support ARN-based principals.These variables store the full account information (including
_id) needed to construct ARN-based principals for the updated policy tests.
126-138: LGTM! Account info retrieved after creation for ARN construction.The test correctly reads account information after creation to obtain the account IDs needed for constructing ARN-based principals in bucket policies.
140-149: LGTM! S3 access policy updated to use ARN-based principals.The policy correctly constructs ARN principals in the format
arn:aws:iam::{account_id}:rootfor each account, aligning with the PR's transition from email-based to ARN-based principal validation.
526-531: LGTM! Session token test updated to use ARN-based principal.The test correctly retrieves account info and constructs an ARN-based principal for the bucket policy, maintaining consistency with the PR's ARN-based validation approach.
src/endpoint/s3/s3_rest.js (4)
258-258: LGTM! ARN-based account identifier constructed correctly.The function call to
get_bucket_policy_principal_arncorrectly derives either a root ARN or user ARN based on the account structure. Based on learnings,account.owneris already a string from RPC serialization, so no additional conversion is needed.Based on learnings
297-297: LGTM! Permission tracking variable for ARN-based checks.The
permission_by_arnvariable is correctly declared to track the result of ARN-based bucket policy permission checks, parallel to the existingpermission_by_idandpermission_by_namevariables.
319-327: LGTM! ARN-based permission check correctly scoped to containerized deployments.The ARN-based permission check is appropriately gated by
!account_identifier_id, ensuring it only runs for containerized deployments (where ID is undefined) as intended per the PR description. The DENY precedence is correctly enforced at line 327.
329-329: LGTM! ARN permission included in ALLOW decision.The condition correctly includes
permission_by_arn === "ALLOW"alongside the existing ID and name permission checks, ensuring ARN-based principals can authorize access in containerized deployments.src/test/integration_tests/api/s3/test_s3_bucket_policy.js (4)
15-15: LGTM! S3 bucket policy utilities imported for ARN construction.The import provides access to
create_arn_for_rootand other ARN construction helpers needed to build ARN-based principals in containerized test scenarios.
68-74: LGTM! Variables added to support dual principal formats.These variables enable the tests to handle both NC coretest (name-based principals) and containerized (ARN-based principals) deployments, improving test coverage across deployment types.
119-127: LGTM! Account info retrieved to support ARN construction.The test correctly reads admin account info and conditionally retrieves full account details (with
_id) for users when not immediately available from account creation, enabling ARN-based principal construction.
150-156: LGTM! Principal variables set based on deployment type.The conditional logic correctly sets principals to either account names (NC coretest) or ARN format (containerized) using the appropriate ARN construction helper. The comment clearly explains the distinction.
src/sdk/accountspace_nb.js (2)
6-6: LGTM!The import addition correctly supports the centralization of IAM username logic.
88-88: LGTM!The migration from
account_util.get_iam_usernametoiam_utils.get_iam_usernamecorrectly centralizes IAM username logic. The usage pattern is consistent across all modified call sites.Also applies to: 113-113, 176-176, 221-221
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
a9e8fb7 to
2f2ca13
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_rest.js (1)
297-329: Fix options argument for ARN-based policy check to honor public-access blockingIn the ARN branch 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 );All other call sites pass an options object (e.g.
{ disallow_public_access: public_access_block?.restrict_public_buckets }), but here a bare boolean/undefined is passed. As a result,has_bucket_policy_permission()will never seedisallow_public_accessfor ARN principals, sorestrict_public_bucketsis effectively ignored in this path.This weakens protections for buckets that block public access when principals are specified by ARN (the main new use case of this PR).
Suggested fix:
- 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); - } + 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, + { disallow_public_access: public_access_block?.restrict_public_buckets } + ); + dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn); + }The existing
if (permission_by_arn === "DENY")check then continues to enforce DENY precedence.
♻️ Duplicate comments (1)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
365-403: Remove redundanta_principalreassignment in complex-policy testsHere:
// Losing this value in-between, assigning it again a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); const deny_account_by_name_all_s3_actions_statement = { Sid: `Do not allow user ${user_a} any s3 action`, Effect: 'Deny', Principal: { AWS: [a_principal] }, ... };
a_principalis a module-scoped variable already initialized insetup()and is never modified elsewhere, so it shouldn’t “lose” its value between tests. This reassignment is redundant and may confuse future readers into thinking there’s a hidden lifecycle issue.You can safely drop the reassignment and rely on the value from
setup(). If you do need per-suite re-init in the future, prefer abefore/beforeEachhook instead.
🧹 Nitpick comments (3)
src/server/system_services/bucket_server.js (1)
517-546: ARN principal resolver is sound; only minor readability nitsThe new
get_account_by_principal()correctly:
- Accepts both
SensitiveStringand plain strings.- Validates against
AWS_IAM_ARN_REGEXP.- Resolves
arn:aws:iam::ACCOUNT_ID:rootto the root account andarn:aws:iam::ACCOUNT_ID:user/.../user_nameto the IAM user via${user_name}:${ACCOUNT_ID}.Given
validate_s3_policy()already treatsundefinedas “invalid principal,” the silentreturnon unsupported ARNs is acceptable. If you want to tighten things later, you could:
- Early-return when
principalis not a string/SensitiveString, and/or- Factor
arn_parts[5]into a localconst resource = arn_parts[5]for readability.No functional issues here; the wiring into
put_bucket_policyviaget_account_by_principallooks good.src/sdk/accountspace_nb.js (1)
84-95: Centralizing IAM username derivation viaiam_utilslooks correctAll updated call sites now route username extraction through
iam_utils.get_iam_username(...), using either the explicitparams.usernameorrequested_account.name.unwrap(). This keeps IAM usernames consistent with how ARNs and principals are computed elsewhere and avoids diverging parsing logic.The remaining
account_util.get_iam_usernameusage in thecreate_access_keyerror message is purely cosmetic; you can optionally switch that toiam_utils.get_iam_usernamelater for full consolidation.Also applies to: 112-121, 175-183, 221-223, 236-242
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (1)
119-133: Principal setup now correctly abstracts NC vs containerized behaviorThe new
admin_info/account_info_*reads and theadmin_principal/a_principal/b_principalconstruction:
- Use plain account names in NC (
is_nc_coretest) to preserve old behavior.- Use
s3_bucket_policy_utils.create_arn_for_root(<account_id>)in containerized mode, matching the ARN format thatget_account_by_principalandget_bucket_policy_principal_arnexpect.That gives all downstream tests a single source of truth for principals and keeps them aligned with the production auth logic. The only small nit is the
console.logdebug prints foraccount_info_*, which you might want to remove or guard with a debug flag to keep test output clean.Also applies to: 150-157
📜 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 (8)
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/endpoint/iam/iam_utils.js
- src/util/string_utils.js
- src/server/common_services/auth_server.js
- src/api/account_api.js
- src/util/account_util.js
- src/server/system_services/account_server.js
- src/endpoint/s3/s3_bucket_policy_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/api/sts/test_sts.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-18T07:00:17.636Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.636Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/server/system_services/bucket_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.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/system_services/bucket_server.js
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/server/system_services/bucket_server.jssrc/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/sdk/accountspace_nb.jssrc/endpoint/s3/s3_rest.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
📚 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/test/integration_tests/api/s3/test_s3_bucket_policy.jssrc/endpoint/s3/s3_rest.js
🧬 Code graph analysis (3)
src/test/integration_tests/api/sts/test_sts.js (5)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)src/endpoint/s3/s3_rest.js (2)
account(255-255)account(339-339)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/nsfs/test_nsfs_integration.js (1)
account(264-264)
src/test/integration_tests/api/s3/test_s3_bucket_policy.js (3)
src/test/utils/coretest/coretest.js (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)src/test/system_tests/test_utils.js (1)
is_nc_coretest(48-48)
src/endpoint/s3/s3_rest.js (3)
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/test/integration_tests/api/s3/test_s3_bucket_policy.js (13)
s3_bucket_policy_utils(15-15)s3_policy(1706-1716)s3_policy(1733-1743)s3_policy(1760-1770)s3_policy(1787-1795)s3_policy(1812-1820)s3_policy(1837-1845)s3_policy(1862-1862)s3_policy(1882-1892)s3_policy(1904-1913)s3_policy(1925-1934)s3_policy(1946-1955)s3_policy(1967-1982)src/endpoint/s3/s3_bucket_policy_utils.js (1)
account(293-293)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (2)
src/test/integration_tests/api/sts/test_sts.js (1)
81-84: STS–S3 integration now aligned with ARN-based principalsReading
account_info_*viaread_account()and constructingarn:aws:iam::${account_id}:rootfor the S3 bucket policies ties the STS tests to the same principal shape used in runtime policy evaluation. This keeps the tests valid for both NC (by account IDs) and containerized (by ARNs) without changing STS assume-role semantics.Looks consistent and correct.
Also applies to: 124-139, 140-149, 526-532
src/endpoint/s3/s3_rest.js (1)
255-259: Good centralization of principal ARN constructionUsing
s3_bucket_policy_utils.get_bucket_policy_principal_arn(account)to deriveaccount_identifier_arnkeeps S3’s authorization logic in sync with the bucket-policy utils and avoids duplicating ARN construction rules here. This also cleanly supports both root and IAM users.
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_bucket_policy_arn_validation
branch
from
2f2ca13 to
4818d5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/util/string_utils.js (1)
21-21: Regex character class doesn't match AWS username validation.The
AWS_IAM_ARN_REGEXPuser path segment[\w\-\.\/]+is more restrictive thanAWS_USERNAME_REGEXP(line 15) which allows[\w+=,.@-]+. This means ARNs generated bycreate_arn_for_userwith usernames containing+,=,,, or@will fail this regex validation inget_account_by_principal.Consider aligning the character classes:
-const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\w{10,}:(?:root|user\/[\w\-\.\/]+)$/; +const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\w{10,}:(?:root|user\/[\w+=,.@\-\.\/]+)$/;This ensures consistency between ARN construction and validation. Note: The account ID pattern
\w{10,}is intentionally relaxed to support internal NooBaa account IDs, which differ from AWS's 12-digit requirement.
🧹 Nitpick comments (3)
src/server/system_services/bucket_server.js (1)
528-547: Consider validating extracted username is non-empty.At line 542,
iam_user_nameis extracted and trimmed, but there's no validation that it's non-empty before passing it toget_account_by_emailat line 543. If the ARN ends with a trailing slash (e.g.,arn:aws:iam::123456789012:user/path/), this could result in looking up an account with an invalid email format.Add a guard:
} else if (arn_sufix && arn_sufix.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) { + return; + } return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`));src/endpoint/s3/s3_bucket_policy_utils.js (2)
386-393: Clarify the incomplete ARN case at line 388.When
username === undefined, the function returns${basic_structure}/which produces an ARN likearn:aws:iam::123456789012:user/. This doesn't match a valid IAM user ARN according to AWS specifications, which require a username after the path.If this is intentional for internal use cases, please add a comment explaining when and why this incomplete ARN format is needed. Otherwise, consider returning an error or a more specific sentinel value:
function create_arn_for_user(account_id, username, iam_path) { const basic_structure = `arn:aws:iam::${account_id}:user`; - if (username === undefined) return `${basic_structure}/`; + if (username === undefined) { + // Return basic structure for internal cases where username is not yet available + return `${basic_structure}/`; + } if (check_iam_path_was_set(iam_path)) { return `${basic_structure}${iam_path}${username}`; } return `${basic_structure}/${username}`; }
364-368: Document the account structure assumption.The function assumes that for IAM users,
account.namefollows the formatusername:account_id(line 365), but this assumption is not documented in the JSDoc. This would help other developers understand the expected account structure.Enhance the JSDoc:
/** * Return IAM ARN for account, if account dont have owner * and User, if the user do have account ower * - * @param {Object} account + * @param {Object} account - Account object with: + * - _id: Account ID + * - owner: (Optional) Owner account ID for IAM users + * - name: SensitiveString in format "username:account_id" for IAM users, or account email for root + * - iam_path: (Optional) IAM path for users * @returns {string} */
📜 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 (9)
- src/server/common_services/auth_server.js
- src/test/integration_tests/api/sts/test_sts.js
- src/test/integration_tests/nsfs/test_nsfs_integration.js
- src/endpoint/iam/iam_utils.js
- src/server/system_services/account_server.js
- src/sdk/accountspace_nb.js
- src/util/account_util.js
- src/endpoint/s3/s3_rest.js
- src/test/integration_tests/api/s3/test_s3_bucket_policy.js
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
📚 Learning: 2025-11-18T07:00:17.636Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.636Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/endpoint/s3/s3_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_bucket_policy_utils.jssrc/server/system_services/bucket_server.jssrc/api/account_api.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/endpoint/s3/s3_bucket_policy_utils.jssrc/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-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/api/account_api.js
🧬 Code graph analysis (2)
src/endpoint/s3/s3_bucket_policy_utils.js (2)
src/endpoint/s3/s3_rest.js (2)
account(255-255)account(339-339)src/endpoint/iam/iam_utils.js (1)
basic_structure(41-41)
src/server/system_services/bucket_server.js (4)
src/api/account_api.js (1)
SensitiveString(4-4)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(293-293)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/api/account_api.js (1)
623-623: LGTM! Schema extensions support ARN-based principal handling.The addition of
_idandiam_pathfields toaccount_infois appropriate and aligns with the PR's objective to enable ARN-based bucket policy principal validation. Both fields are optional, ensuring backward compatibility.Also applies to: 665-667
3 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