IAM | Bucket policy Principal upgrade script #9267
Conversation
WalkthroughAdds a debug log in S3 authorize_request_policy, a new 5.21.0 upgrade script that converts bucket policy Principals to ARN format, and integration tests that exercise the ARN upgrade using SensitiveString-wrapped principals and pool/account mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Upgrade as upgrade_bucket_policy_principal
participant Store as system_store
participant DB as Database
Note over Test,Upgrade: Trigger upgrade script
Test->>Upgrade: run({ dbg, system_store, system_server })
Upgrade->>DB: query buckets with s3_policy
DB-->>Upgrade: buckets[]
loop per bucket
Upgrade->>Upgrade: _create_arn(principals, system_store) => ARN(s)
Upgrade->>Upgrade: rewrite Statement.Principal -> ARN form
end
alt any buckets changed
Upgrade->>Store: make_changes(changed_buckets)
Store->>DB: persist updates
DB-->>Store: ack
Store-->>Upgrade: success
Upgrade-->>Test: complete (updated)
else no changes
Upgrade-->>Test: complete (no-op)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
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: 6
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-277: Consider cleaning up the IAM account created during the test.The test creates an IAM account at line 248 but does not clean it up in the
mocha.afterhook. This could lead to leftover test data accumulating in the system store across test runs.Consider adding cleanup logic in the
mocha.afterhook:mocha.after(async function() { await s3.deleteBucket({ Bucket: BKT }); await s3.deleteBucket({ Bucket: BKT1 }); // Clean up IAM account if it exists try { await rpc_client.account.delete_account({ email: 'iam_username' }); } catch (err) { // Ignore if account doesn't exist } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 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/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
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.
📚 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/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (2)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (12)
require(9-9)require(10-10)_(5-5)dbg(18-18)system_store(13-13)account(239-239)buckets(29-32)buckets(41-44)bucket(95-95)bucket(230-230)new_policy(109-109)new_policy(264-264)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(14-14)new_policy(39-39)
⏰ 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/internal/test_upgrade_scripts.js (3)
7-7: LGTM!The addition of
POOL_LISTto the destructuring is consistent with its usage in line 246 of the new test case.
12-12: LGTM!The
SensitiveStringimport is correctly used to wrap sensitive principal values in the test policy at lines 212 and 220.
15-15: LGTM!The upgrade script import is properly used at line 260 to test the ARN-based principal upgrade functionality.
| function _create_arn(dbg, principals, system_store) { | ||
| if (!principals.AWS) return; | ||
| const principal_arns = []; | ||
| for (const principal of principals.AWS) { | ||
| if (principal === '*') continue; | ||
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | ||
| if (!account) { | ||
| dbg.log0(`Could not found the account with email: ${principal}`); | ||
| continue; | ||
| } | ||
| let arn; | ||
| if (account.owner) { | ||
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | ||
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | ||
| } else { | ||
| arn = `aws:arn:${account._id.toString()}:root`; | ||
| } | ||
| principal_arns.push(arn); | ||
| } | ||
| return { AWS: principal_arns }; | ||
| } |
There was a problem hiding this comment.
Critical: Wildcard principals are lost during upgrade.
The function skips wildcard ('*') principals at line 13 but never adds them to principal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.
Apply this diff to preserve wildcard principals:
function _create_arn(dbg, principals, system_store) {
if (!principals.AWS) return;
const principal_arns = [];
for (const principal of principals.AWS) {
- if (principal === '*') continue;
+ if (principal === '*') {
+ principal_arns.push('*');
+ continue;
+ }
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
if (!account) {
dbg.log0(`Could not found the account with email: ${principal}`);
continue;
}📝 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.
| function _create_arn(dbg, principals, system_store) { | |
| if (!principals.AWS) return; | |
| const principal_arns = []; | |
| for (const principal of principals.AWS) { | |
| if (principal === '*') continue; | |
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | |
| if (!account) { | |
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| continue; | |
| } | |
| let arn; | |
| if (account.owner) { | |
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | |
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | |
| } else { | |
| arn = `aws:arn:${account._id.toString()}:root`; | |
| } | |
| principal_arns.push(arn); | |
| } | |
| return { AWS: principal_arns }; | |
| } | |
| function _create_arn(dbg, principals, system_store) { | |
| if (!principals.AWS) return; | |
| const principal_arns = []; | |
| for (const principal of principals.AWS) { | |
| if (principal === '*') { | |
| principal_arns.push('*'); | |
| continue; | |
| } | |
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | |
| if (!account) { | |
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| continue; | |
| } | |
| let arn; | |
| if (account.owner) { | |
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | |
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | |
| } else { | |
| arn = `aws:arn:${account._id.toString()}:root`; | |
| } | |
| principal_arns.push(arn); | |
| } | |
| return { AWS: principal_arns }; | |
| } |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
lines 9 to 29, the upgrade currently skips wildcard principals ('*') and thus
drops anonymous access from upgraded policies; modify the loop to preserve
wildcards by adding '*' to principal_arns instead of continuing (e.g., when
principal === '*' push '*' into principal_arns), keeping other logic for
resolving account ARNs unchanged so the returned object can include both
resolved ARNs and the wildcard.
| if (principal === '*') continue; | ||
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | ||
| if (!account) { | ||
| dbg.log0(`Could not found the account with email: ${principal}`); |
There was a problem hiding this comment.
Avoid logging sensitive principal information.
Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.
Apply this diff:
- dbg.log0(`Could not found the account with email: ${principal}`);
+ dbg.log0(`Could not find account for principal in bucket policy`);📝 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.
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| dbg.log0(`Could not find account for principal in bucket policy`); |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
line 16, the code currently logs the full principal email; change the log to
avoid printing sensitive principal information by either redacting or
hashing/truncating the principal before logging (e.g., replace the local-part
with asterisks or log only a UUID/hash of the principal), and update the message
to something like "Could not find the account for principal: [REDACTED]" while
preserving enough context for debugging; implement a small helper or inline
masking (do not log the raw principal) and use that in the processLogger/dbg
call.
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
Outdated
Show resolved
Hide resolved
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
60824f7 to
a597682
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/endpoint/s3/s3_rest.js (1)
244-245: Remove debug console.log or convert to proper logging.This console.log statement should be removed before merging, or converted to use the
dbgmodule for consistency with the rest of the codebase. Additionally, be cautious about loggingbucket_ownerandowner_accountas they may contain sensitive information.src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (5)
9-29: Critical: Wildcard principals are lost during upgrade.The function skips wildcard (
'*') principals at line 13 but never adds them toprincipal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.
9-10: Preserve original principals when AWS principals are absent.The function returns
undefinedwhenprincipals.AWSdoesn't exist, which causes thePrincipalfield to becomeundefinedin the reconstructed statement. This corrupts the policy if a statement contains only non-AWS principals (e.g.,Service,Federated, orCanonicalUser).
16-16: Avoid logging sensitive principal information.Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.
38-46: Essential: Statement fields may be lost during upgrade.The upgrade script only copies
Sid,Effect,Action,Resource, andPrincipalfields from each statement. This will drop other important fields such asCondition,NotPrincipal,NotAction, andNotResourceif they exist in the original policy, breaking the policy semantics.
70-70: Fix typo in description.The word "formate" should be "format".
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-276: Consider cleaning up the IAM account in the after hook.The test creates an IAM account at line 248 but doesn't clean it up in the
mocha.afterhook (lines 278-281). While this may be acceptable for integration tests that reset the environment, consider adding cleanup for consistency with other test resources.Apply this diff to add IAM account cleanup:
mocha.after(async function() { + // Clean up IAM account if it exists + const iam_username = 'iam_username'; + try { + await rpc_client.account.delete_account({ email: iam_username }); + } catch (err) { + // Account may not exist if test didn't run + dbg.log0('IAM account cleanup skipped:', err.message); + } await s3.deleteBucket({ Bucket: BKT }); await s3.deleteBucket({ Bucket: BKT1 }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 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/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
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.
📚 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/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (2)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (12)
require(9-9)require(10-10)_(5-5)dbg(18-18)system_store(13-13)account(239-239)buckets(29-32)buckets(41-44)bucket(95-95)bucket(230-230)new_policy(109-109)new_policy(264-264)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(14-14)new_policy(39-39)
⏰ 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 (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (3)
7-7: LGTM!The addition of
POOL_LISTto the imports is appropriate and used in the new test case at line 246.
12-12: LGTM!The
SensitiveStringimport is correctly added and used to wrap sensitive principal values in the test policy at lines 212 and 220.
15-15: LGTM!The upgrade script import is appropriately added and invoked at line 260 to test the ARN-based principal upgrade functionality.
liranmauda
left a comment
liranmauda
left a comment
There was a problem hiding this comment.
We should first fix the new bucket policies to follow the same ARN format when creating bucket policy, and only then merge this PR.
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
Outdated
Show resolved
Hide resolved
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
a597682 to
ea937e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
12-12: Critical: Wildcard principals are still being dropped.Line 12 skips wildcard (
'*') principals but never adds them toprincipal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.Apply this diff to preserve wildcard principals:
- if (principal === '*') continue; + if (principal === '*') { + principal_arns.push('*'); + continue; + }
15-15: Avoid logging sensitive principal information.Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.
Apply this diff:
- dbg.log0(`Could not find the account with email: ${principal}`); + dbg.log0(`Could not find account for principal in bucket policy`);src/test/integration_tests/internal/test_upgrade_scripts.js (1)
274-275: Critical: Test assertions use incorrect ARN format.These assertions expect
aws:arn:...format, but the upgrade script implementation usesarn:aws:iam::...format. This test will fail. See the comment on the upgrade script file for details on the ARN format inconsistency.Ensure the expected ARN format matches the upgrade script implementation:
- assert.strictEqual(new_policy.Statement[0].Principal.AWS[0], `aws:arn:${account._id.toString()}:root`); - assert.strictEqual(new_policy.Statement[1].Principal.AWS[0], `aws:arn:${iam_account._id.toString()}:user/${iam_account.email.unwrap()}`); + assert.strictEqual(new_policy.Statement[0].Principal.AWS[0], `arn:aws:iam::${account._id.toString()}:root`); + assert.strictEqual(new_policy.Statement[1].Principal.AWS[0], `arn:aws:iam::${iam_account._id.toString()}:user/${iam_account.iam_path || '/'}${iam_account.email.unwrap()}`);
🧹 Nitpick comments (2)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
37-37: Optional: Simplify redundant check.The check
bucket.s3_policy.Statement !== undefinedis somewhat redundant since line 36 ensuresbucket.s3_policyexists, and a valid S3 policy should always have aStatementarray. Consider simplifying to!bucket.s3_policy.Statement || !bucket.s3_policy.Statement.length.Apply this diff if you want to make the intent clearer:
- if (bucket.s3_policy.Statement !== undefined) { + if (bucket.s3_policy.Statement && bucket.s3_policy.Statement.length > 0) {src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-276: Recommended: Add test coverage for edge cases.The test focuses on the happy path of email-to-ARN conversion but doesn't cover critical edge cases that were flagged in past reviews:
- Wildcard principals (
'*') - ensure they're preserved after upgrade- Non-AWS principals (e.g.,
Service,Federated) - ensure they're not dropped- Missing accounts - verify graceful handling when a principal email doesn't match any account
Additionally, the IAM account created at line 248 should be cleaned up in the
afterhook to avoid test pollution.Consider adding additional test cases or extending this test to cover:
// Test with wildcard principal const policy_with_wildcard = { Version: '2012-10-17', Statement: [{ Effect: 'Allow', Principal: { AWS: ['*'] }, Action: ['s3:GetObject'], Resource: ['arn:aws:s3:::*'] }] }; // Test with non-AWS principal const policy_with_service = { Version: '2012-10-17', Statement: [{ Effect: 'Allow', Principal: { Service: 's3.amazonaws.com' }, Action: ['s3:PutObject'], Resource: ['arn:aws:s3:::*'] }] };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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/internal/test_upgrade_scripts.js
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.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/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(13-13)new_policy(38-38)
⏰ 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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
🔇 Additional comments (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
7-7: LGTM: Necessary imports for the new test.The imports of
POOL_LIST,SensitiveString, andupgrade_bucket_policy_principalare all properly used in the new test case.Also applies to: 12-12, 15-15
246-246: No action required — original review comment is incorrect.POOL_LIST is defined as a constant array in coretest.js and properly exported. POOL_LIST[1] is statically initialized at module load time with
{ name: 'pool-with-1-host', host_count: 1 }, independent of any test environment. Thesetup()call at line 8 uses POOL_LIST[1] to create a pool instance, but does not define POOL_LIST[1] itself. The code at line 246 correctly accessesPOOL_LIST[1].namein non-NC_CORETEST environments without any dependency or initialization issues.Likely an incorrect or invalid review comment.
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
21-23: The original review comment is incorrect; the implementation is actually correct.The implementation correctly uses the standard AWS Principal ARN format for IAM resources: AWS Principal ARNs for IAM resources follow the format
arn:aws:iam::123456789012:rootfor accounts andarn:aws:iam::123456789012:user/usernamefor users. The code produces exactly this format.However, the test expects a non-standard format (
aws:arn:...), which will cause test failures. The issue lies in the test assertions at lines 274-275 ofsrc/test/integration_tests/internal/test_upgrade_scripts.js, not in the implementation itself. The test should be updated to expect the correct ARN format that matches AWS standards and what the upgrade script correctly generates.Likely an incorrect or invalid review comment.
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
Outdated
Show resolved
Hide resolved
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
2 times, most recently
from
7c63ad0 to
a1e7473
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
6-25: Fix principal conversion to handle undefined, preserve wildcards & non-AWS principals, and avoid logging PIIThis helper has several correctness and safety issues:
- If
principalsisundefinedor has noAWSfield,principals.AWSwill throw or you returnundefined, causingPrincipalto be wiped inrun()for statements that only have non-AWS principals.- Wildcard principals (
'*'orAWS: "*") are skipped and never re-added, breaking anonymous-access policies.- Logging the raw
principal(likely a SensitiveString-wrapped email) leaks PII into logs.- Returning only
{ AWS: [...] }drops any other principal types (e.g.,Service,Federated,CanonicalUser) present on the original object.Recommend guarding for missing principals, preserving existing non-AWS principal fields, preserving wildcards, and redacting the log:
-function _create_arn(dbg, principals, system_store) { - if (!principals.AWS) return; - const principal_arns = []; - for (const principal of principals.AWS) { - if (principal === '*') continue; - const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); - if (!account) { - dbg.log0(`Could not find the account with email: ${principal}`); - continue; - } - let arn; - if (account.owner) { - const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; - arn = `arn:aws:iam::${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; - } else { - arn = `arn:aws:iam::${account._id.toString()}:root`; - } - principal_arns.push(arn); - } - return { AWS: principal_arns }; -} +function _create_arn(dbg, principals, system_store) { + // If there are no AWS principals, leave the object unchanged so other + // principal types (Service/Federated/CanonicalUser) are preserved. + if (!principals || !principals.AWS || principals.AWS.length === 0) return principals; + + const principal_arns = []; + for (const principal of principals.AWS) { + // Preserve wildcard/anonymous access + if (principal === '*') { + principal_arns.push('*'); + continue; + } + + // Support both SensitiveString and plain string principals + const principal_value = typeof principal.unwrap === 'function' ? principal.unwrap() : principal; + + const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal_value); + if (!account) { + dbg.log0('Could not find account for principal in bucket policy'); + continue; + } + + let arn; + if (account.owner) { + const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; + arn = `arn:aws:iam::${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; + } else { + arn = `arn:aws:iam::${account._id.toString()}:root`; + } + principal_arns.push(arn); + } + + // Preserve any non-AWS principal fields that were present + return { ...principals, AWS: principal_arns }; +}
🧹 Nitpick comments (2)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
7-16: Good reuse of coretest utilities and SensitiveString for policy setupImporting
rpc_client,POOL_LIST, andSensitiveString, and wiring inupgrade_bucket_policy_principalgives the upgrade script solid integration-test coverage as required for changes undersrc/test/**/*.*.Optional: for consistency, you could also use the destructured
POOL_LISTincoretest.setup({ pools_to_create: [coretest.POOL_LIST[1]] });, but this is purely cosmetic.
204-276: 5.21.0 upgrade test correctly validates owner/IAM ARN mapping; consider extending coverage laterThis test does a good job of:
- Seeding an old-style policy with SensitiveString-wrapped principals for the main account and an IAM-like account.
- Running the 5.21.0 upgrade and asserting:
- Policy structure (Version, Statement length, key fields) is preserved.
- Principals are converted to the expected root and IAM
arn:aws:iam::...forms using actual_idvalues.This satisfies the guideline that changes under
src/upgrade/...are covered by tests.For a follow-up (not blocking), consider adding one more statement case to this test (or a separate test) that covers:
- A wildcard principal (
'*'orPrincipal: { AWS: '*' }), and/or- A statement that has non-AWS principals only,
so you can lock in the expected behavior for those edge cases as
_create_arnevolves.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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/internal/test_upgrade_scripts.js
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/test/integration_tests/internal/test_upgrade_scripts.jssrc/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.jssrc/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.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/test/integration_tests/internal/test_upgrade_scripts.jssrc/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js
🧬 Code graph analysis (2)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(11-11)new_policy(36-36)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (9)
require(9-9)require(10-10)dbg(18-18)system_store(13-13)account(239-239)buckets(29-32)buckets(41-44)bucket(95-95)bucket(230-230)
⏰ 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 (1)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
28-59: run() flow looks good, but depends on_create_arnsemantics being safeThe iteration over buckets and batched
system_store.make_changesare reasonable; once_create_arnis made robust (no PII logging, preserves non-AWS/wildcard principals, and handles missing principals safely), this function should behave correctly and be side‑effect scoped to buckets that actually haves3_policy.If you expect this upgrade to be re-runnable, consider verifying that
_create_arntreats already-ARN principals as idempotent (e.g., preservesarn:aws:iam::...strings unchanged) so a second run does not corrupt policies.
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
a1e7473 to
6eb1749
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/endpoint/s3/s3_rest.js (1)
246-246: Confirm that logging bucket_owner/owner_account here is acceptable and properly redactedUsing
dbg.log2instead ofconsole.loglooks good and matches existing logging patterns, but this still logsbucket_ownerandowner_account, which may include emails or other sensitive identifiers. Please confirm that:
debug_moduleandSensitiveStringensure these values are redacted/obscured in logs, or- If not, consider narrowing this to non-sensitive fields (e.g., IDs only) or removing it once the upgrade is stable.
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-276: Align expected ARNs with the shared helper and IAM account ID semanticsThe new test covers the 5.21.0 bucket-policy‑principal upgrade well: it seeds an old policy with SensitiveString principals, creates an IAM‑style account, runs the upgrade, and asserts that principals are converted to ARNs.
Two points worth tightening:
Reuse the ARN helper instead of hard‑coding string templates
The expectations:
assert.strictEqual(new_policy.Statement[0].Principal.AWS[0], `arn:aws:iam::${account._id.toString()}:root`); assert.strictEqual(new_policy.Statement[1].Principal.AWS[0], `arn:aws:iam::${iam_account._id.toString()}:user/${iam_account.email.unwrap()}`);duplicate the ARN‑formatting logic that already exists in
s3_bucket_policy_utils.get_bucket_policy_principal_arn(account). To avoid drift between the upgrade script, the S3 authorization path, and this test, consider computing expectations like:const s3_bucket_policy_utils = require('../../../endpoint/s3/s3_bucket_policy_utils'); const owner_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account); const iam_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(iam_account); assert.strictEqual(new_policy.Statement[0].Principal.AWS[0], owner_arn); assert.strictEqual(new_policy.Statement[1].Principal.AWS[0], iam_arn);This keeps the test aligned if ARN construction changes (e.g., new partition, path, or encoding rules).
Verify which ID should be used for IAM ARNs
In the second assertion you use
iam_account._id.toString()as the account ID segment of the ARN. However, IAM utilities elsewhere (e.g., those that consumeaccount.owneras a string ID) may treat the root account’s ID as the canonical account ID for IAM users. If that’s the case, the IAM ARN should likely embedaccount._id(the owner) rather thaniam_account._id.Using
get_bucket_policy_principal_arn(iam_account)for the expected value will both:
- Match whatever the project currently considers the canonical IAM ARN form, and
- Prevent this test from silently diverging from the authorization logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.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/internal/test_upgrade_scripts.js
🧠 Learnings (8)
📓 Common learnings
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.
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/s3/s3_rest.js
📚 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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (2)
src/endpoint/s3/s3_rest.js (2)
src/endpoint/endpoint.js (1)
dbg(11-11)src/endpoint/s3/s3_utils.js (1)
dbg(7-7)
src/test/integration_tests/internal/test_upgrade_scripts.js (3)
src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js (2)
SensitiveString(6-6)account(74-74)src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(11-11)new_policy(36-36)
⏰ 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 (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
7-16: Top‑level imports and coretest destructuring look consistentDestructuring
rpc_client,POOL_LISTfromcoretest, addingSensitiveString, and wiringupgrade_bucket_policy_principalinto the tests all line up with how they’re used later in the file. No issues here.
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
6eb1749 to
6041066
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-276: Consider adding pre-upgrade validation for better test coverage.The test validates the post-upgrade ARN format correctly, but doesn't verify the pre-upgrade state or intermediate account relationships. Adding these checks would make the test more robust and easier to debug.
Consider adding these validations:
- Verify the old policy format before upgrade:
// After line 238, before line 239 const bucket_before_upgrade = system_store.data.buckets.find(b => b.name.unwrap() === BKT); assert.strictEqual(bucket_before_upgrade.s3_policy.Statement[0].Principal.AWS[0].unwrap(), EMAIL); assert.strictEqual(bucket_before_upgrade.s3_policy.Statement[1].Principal.AWS[0].unwrap(), iam_username);
- Verify IAM account ownership after linking:
// After line 258, before line 260 const linked_account = system_store.data.accounts.find(acc => acc.email.unwrap() === iam_username); assert.strictEqual(linked_account.owner.toString(), account._id.toString(), 'IAM account should be owned by parent account');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/s3/s3_rest.js
- src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.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/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
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.
📚 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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(11-11)new_policy(36-36)
⏰ 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 (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
7-7: LGTM! Imports are appropriate.The added imports (
POOL_LIST,SensitiveString, andupgrade_bucket_policy_principal) are all used correctly in the new test case.Also applies to: 12-12, 15-15
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
6041066 to
35a7abb
Compare
naveenpaul1
changed the title
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
35a7abb to
3c1fcc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
25-25: Use a unique identifier for the IAM username to prevent test conflicts.The hardcoded
iam_usernameconstant can cause conflicts if tests run multiple times or in parallel. While cleanup has been added in theafterhook, using a unique identifier would make the test more robust.As noted in a previous review, consider generating a unique username:
-const iam_username = 'iam_username'; +const iam_username = `iam_username_${Date.now()}`;Also applies to: 241-248, 281-284
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
281-284: Consider adding error handling for cleanup resilience.While the cleanup correctly deletes the IAM account, it lacks error handling for cases where the test fails before account creation. Making the cleanup more resilient would prevent after-hook failures from masking the actual test failure.
Apply this diff to add graceful error handling:
mocha.after(async function() { await s3.deleteBucket({ Bucket: BKT }); await s3.deleteBucket({ Bucket: BKT1 }); - const iam_acc = { - email: iam_username, - }; - await rpc_client.account.delete_account(iam_acc); + try { + const iam_acc = { + email: iam_username, + }; + await rpc_client.account.delete_account(iam_acc); + } catch (err) { + // Ignore if account wasn't created or already deleted + dbg.log0('IAM account cleanup skipped:', err.message); + } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(3 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/s3/s3_rest.js
- src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.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/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
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.
📚 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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.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/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
src/test/integration_tests/internal/test_upgrade_scripts.js (4)
7-7: LGTM!The import additions are appropriate for the new test case and are all utilized correctly.
Also applies to: 12-12, 15-15
239-258: IAM account setup looks correct, but verify pool reference.The logic correctly creates an IAM account and links it to the main account via the
ownerfield (line 255), which sets up the proper relationship for testing ARN conversion.However, line 246 references
POOL_LIST[1].namein the non-NC_CORETEST path. While line 8 creates this pool, explicitly verifying the pool exists would make the test more robust.Consider adding an assertion before line 246:
const nsr = 's3_bucket_policy_nsr'; // Ensure pool exists when not in NC_CORETEST mode if (!process.env.NC_CORETEST && !POOL_LIST[1]) { throw new Error('POOL_LIST[1] is required for IAM account creation'); } const iam_acc = { name: iam_username, email: iam_username, has_login: false, s3_access: true, default_resource: process.env.NC_CORETEST ? nsr : POOL_LIST[1].name, };
260-276: LGTM!The upgrade verification thoroughly tests:
- Structural integrity of the upgraded policy (Statement count, Version, Sid, Effect, Actions, Resource)
- ARN format conversion for both account root (line 274) and IAM user (line 275)
- The ARN formats follow the standard AWS IAM ARN structure
The assertions correctly verify that the upgrade script transforms Principal.AWS values from email format to ARN format while preserving all other policy properties.
206-226: No issues with SensitiveString usage in test data.The test correctly uses
new SensitiveString(EMAIL)andnew SensitiveString(iam_username)for Principal.AWS values. The codebase explicitly converts all Principal values to SensitiveString instances before storing bucket policies. This is confirmed by the production code insrc/sdk/bucketspace_fs.js(lines 157-165), which wraps both Principal.AWS arrays and scalar principals in SensitiveString during policy processing.
2 participants
Describe the Problem
Update the bucket policy Principal with ARN, not its a email but we will be updating it with ARN, Existing bucket policy also will be replaced by new ARN,
Explain the Changes
GAP
Will handle in next PR
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
test_upgrade_scripts.jstest and make suretest upgrade bucket policy to ARN version 5.21.0is sucess.Summary by CodeRabbit
Improvements
Testing
✏️ Tip: You can customize this high-level summary in your review settings.