block COMPLIANCE to GOVERNANCE retention mode downgrade - #9823
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe object retention API now blocks active COMPLIANCE mode changes and disallowed date reductions. Unit and S3 integration tests cover rejected and accepted retention transitions. ChangesRetention enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
8e72f20 to
886436b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/server/object_services/object_server.js (1)
366-372: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftPre-existing TOCTOU: read-validate-write without optimistic concurrency control.
Between
find_object_md(line 342) andupdate_object_by_id(line 378), a concurrent request can change the object's retention, making the validation stale. For example, request A reads GOVERNANCE and passes the new COMPLIANCE check, but before A writes, request B upgrades to COMPLIANCE — A then writes GOVERNANCE, effectively downgrading active COMPLIANCE.Consider a conditional update (e.g., check
lock_settingshasn't changed) or a version-based optimistic lock inupdate_object_by_idas a follow-up.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/object_services/object_server.js` around lines 366 - 372, The retention read-validation-write flow around find_object_md and update_object_by_id lacks optimistic concurrency control. Update update_object_by_id usage to condition the write on the retention/lock state or object version read by find_object_md, and handle a failed conditional update as a conflict without overwriting concurrent retention changes.Source: Coding guidelines
src/test/unit_tests/internal/test_object_server.test.js (1)
624-716: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for GOVERNANCE bypass scenarios.
The suite covers COMPLIANCE transitions thoroughly but doesn't exercise the GOVERNANCE-specific bypass path at line 367 of
object_server.js: shortening GOVERNANCE retention withbypass_governance: true+ admin (should succeed) and withoutbypass_governance(should fail with UNAUTHORIZED). These are important existing paths that interact with the refactored validation logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/unit_tests/internal/test_object_server.test.js` around lines 624 - 716, Add tests covering the GOVERNANCE shortening path in object_server.put_object_retention: verify an admin request with bypass_governance: true succeeds and calls update_object_by_id_stub, while the same shortening request without bypass_governance rejects with rpc_code UNAUTHORIZED and does not update. Use active GOVERNANCE retention dates and the existing request/object helpers.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/server/object_services/object_server.js`:
- Around line 366-372: The retention read-validation-write flow around
find_object_md and update_object_by_id lacks optimistic concurrency control.
Update update_object_by_id usage to condition the write on the retention/lock
state or object version read by find_object_md, and handle a failed conditional
update as a conflict without overwriting concurrent retention changes.
In `@src/test/unit_tests/internal/test_object_server.test.js`:
- Around line 624-716: Add tests covering the GOVERNANCE shortening path in
object_server.put_object_retention: verify an admin request with
bypass_governance: true succeeds and calls update_object_by_id_stub, while the
same shortening request without bypass_governance rejects with rpc_code
UNAUTHORIZED and does not update. Use active GOVERNANCE retention dates and the
existing request/object helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e52bbfe-8909-4a1e-a99f-16ff5132d1bd
📒 Files selected for processing (3)
src/server/object_services/object_server.jssrc/test/integration_tests/api/s3/test_s3_worm.jssrc/test/unit_tests/internal/test_object_server.test.js
| // AWS: while COMPLIANCE retention is active, mode cannot change (e.g. to GOVERNANCE). | ||
| // Must run outside the shorten-date check — same/longer dates previously bypassed it. |
There was a problem hiding this comment.
may be personal, but I feel like this comment is not very understandable. what does it mean: "Must run outside the shorten-date check — same/longer dates previously bypassed it."?
I feel like it should be more intuitive to someone that doesn't necessarily understand object lock semantics. something like:
"Cannot change the COMPLIANCE mode when replacing an existing policy with a new one"
There was a problem hiding this comment.
Updated. Changed it to:
"Cannot change COMPLIANCE mode when replacing an existing retention policy (e.g. COMPLIANCE -> GOVERNANCE) while retention is still active."
Also kept a short note that this check is separate from date shortening so a longer retain-until cannot bypass the mode restriction.
| } | ||
| /** | ||
| * | ||
| * put_object_retention |
There was a problem hiding this comment.
I think its worth adding a small comment explaining the function. and generally what it allows and disallows
There was a problem hiding this comment.
Added a short JSDoc on put_object_retention describing what it allows/disallows for COMPLIANCE vs GOVERNANCE while retention is active.
| // Use a date >= current retain-until (extended to +60 above). | ||
| // A shorter date would fail for the wrong reason (cannot shorten COMPLIANCE). | ||
| const longerOrEqualDate = new Date(); | ||
| longerOrEqualDate.setDate(longerOrEqualDate.getDate() + 90); |
There was a problem hiding this comment.
why is this change needed? why does it matters if we are 30 or 90 days in the future?
also its never an equal date
There was a problem hiding this comment.
ahh its because of the previous test. then its a good fix. you should just change the name of the variable
There was a problem hiding this comment.
@kajalpareek-lab could you add a comment about the days so it would be clear for others? (the dependency between the tests).
There was a problem hiding this comment.
@nadavMiz Yes — needed because the previous test extends retain-until to +60 days, so this one must use a later date (+90) so the failure is on COMPLIANCE -> GOVERNANCE mode change, not on shortening.
Renamed the variable to longerRetainUntilDate and added a comment explaining the dependency between the two tests.
There was a problem hiding this comment.
@shirady Added a comment explaining the dependency between the tests: previous test sets +60, so this one uses +90 to assert mode-change denial rather than date shortening.
| }); | ||
| }); | ||
|
|
||
| describe('object_server - put_object_retention', () => { |
There was a problem hiding this comment.
I don't dislike the idea of adding unit tests. but we are already using integration tests for object lock. I think it would be good to have this coverage in test_s3_worm.js. the full path doesn't add much complexity, and I feel like its worth testing. we would also get the added benefit of testing NC as well.
having said that if you still feel like unit tests are needed we should probably move them to their own file for object_lock. no one would expect object lock tests to be here, and I fear they would be missed and potentially recreated elsewhere
There was a problem hiding this comment.
Agreed. Moved the unit tests out of test_object_server.test.js. I’ll also add the COMPLIANCE -> GOVERNANCE / allowed transition coverage to test_s3_worm.js so we exercise the full S3 path (including NC).
| } | ||
|
|
||
| if (!new_retention || new Date(new_retention.retain_until_date) < current_retain_until) { | ||
| if ((current_retention.mode === 'GOVERNANCE' && (!req.rpc_params.bypass_governance || req.role !== 'admin')) || |
There was a problem hiding this comment.
worth adding a TODO that bypass_governance should be allowed to users with IAM permission and not specificly to admin
There was a problem hiding this comment.
@nadavMiz I think the req.role admin is not related to the IAM users.
@kajalpareek-lab I don't think you need to check the req.role again, as it is already checked in the function before:
if (req.role !== 'admin') {
throw new RpcError('UNAUTHORIZED');
}
and anyway I think that you can test the feature of object lock with IAM users vs root users to see you have the expected results according to AWS and if not fix accordingly.
There was a problem hiding this comment.
@nadavMiz Added a TODO that BypassGovernanceRetention should be allowed based on IAM / bucket-policy permission, not specifically the admin role.
There was a problem hiding this comment.
@shirady Removed the redundant admin check in the shorten/bypass path. Admin is already enforced earlier in the function. Agree that IAM-user vs root-user Object Lock coverage is useful; I’d prefer to handle that as a follow-up outside this COMPLIANCE downgrade fix.
| } | ||
|
|
||
| if (!new_retention || new Date(new_retention.retain_until_date) < current_retain_until) { | ||
| if ((current_retention.mode === 'GOVERNANCE' && (!req.rpc_params.bypass_governance || req.role !== 'admin')) || |
There was a problem hiding this comment.
@nadavMiz I think the req.role admin is not related to the IAM users.
@kajalpareek-lab I don't think you need to check the req.role again, as it is already checked in the function before:
if (req.role !== 'admin') {
throw new RpcError('UNAUTHORIZED');
}
and anyway I think that you can test the feature of object lock with IAM users vs root users to see you have the expected results according to AWS and if not fix accordingly.
| // Use a date >= current retain-until (extended to +60 above). | ||
| // A shorter date would fail for the wrong reason (cannot shorten COMPLIANCE). | ||
| const longerOrEqualDate = new Date(); | ||
| longerOrEqualDate.setDate(longerOrEqualDate.getDate() + 90); |
There was a problem hiding this comment.
@kajalpareek-lab could you add a comment about the days so it would be clear for others? (the dependency between the tests).
fadd93c to
2b1e5df
Compare
| current_retention, | ||
| new_retention, | ||
| }); | ||
| throw new RpcError('UNAUTHORIZED'); |
There was a problem hiding this comment.
Could you check the error message in AWS for those cases to see if we have the same error message?
There was a problem hiding this comment.
Checked on a live hosted cluster with this change: PutObjectRetention denials (COMPLIANCE → GOVERNANCE and shortening COMPLIANCE) return AccessDenied / Access Denied, same as other Object Lock denials and what our worm tests already expect for hosted. That matches AWS’s AccessDenied behavior for these cases. I’d leave the message as-is in this PR.
There was a problem hiding this comment.
I meant the error message, not just the error code.
There was a problem hiding this comment.
You’re right — I was looking at the error code only.
Hosted today still returns "Access Denied" for these PutObjectRetention denials (UNAUTHORIZED). Aligning the message to "Access Denied because object protected by object lock." (and mapping via OBJECT_LOCKED / AccessDeniedObjectLocked) is already covered in #9868, including the worm test updates for both hosted and NC.
I’ll leave the message as-is in this PR and rebase onto #9868 after it merges.
There was a problem hiding this comment.
What are this new tests add that we do not cover in src/test/unit_tests/internal/test_object_lock_retention.test.js?
There was a problem hiding this comment.
The unit file keeps the edge cases that are awkward or slow over S3: same-date downgrade (proves mode check isn’t only hit via shorten), longer-date downgrade, expired COMPLIANCE then GOVERNANCE, and isolated put_object_retention without a cluster. Per Nadav’s earlier note we moved them out of test_object_server.test.js into this dedicated file.
There was a problem hiding this comment.
@nadavMiz by using this file it would not be tested in NC, could you explain why not adding the test cases in src/test/integration_tests/api/s3/test_s3_worm.js?
I'm trying to look at the test cases, and I think it is good coverage for both environments:
rejects COMPLIANCE to GOVERNANCE downgrade with same retain daterejects COMPLIANCE to GOVERNANCE downgrade with longer retain date'allows GOVERNANCE to COMPLIANCE upgrade with same retain dateallows extending COMPLIANCE retention while keeping COMPLIANCE modeallows setting GOVERNANCE after COMPLIANCE retention has expiredrejects shortening active COMPLIANCE retention
8ba2527 to
8404ed8
Compare
| current_retention, | ||
| new_retention, | ||
| }); | ||
| throw new RpcError('UNAUTHORIZED'); |
There was a problem hiding this comment.
I meant the error message, not just the error code.
699411a to
0d13158
Compare
|
looks like error message is still wrong. after that is fixed. its fine by me |
@nadavMiz @kajalpareek-lab plans to fix it in another PR - see comment. |
There was a problem hiding this comment.
@nadavMiz by using this file it would not be tested in NC, could you explain why not adding the test cases in src/test/integration_tests/api/s3/test_s3_worm.js?
I'm trying to look at the test cases, and I think it is good coverage for both environments:
rejects COMPLIANCE to GOVERNANCE downgrade with same retain daterejects COMPLIANCE to GOVERNANCE downgrade with longer retain date'allows GOVERNANCE to COMPLIANCE upgrade with same retain dateallows extending COMPLIANCE retention while keeping COMPLIANCE modeallows setting GOVERNANCE after COMPLIANCE retention has expiredrejects shortening active COMPLIANCE retention
80e3ca9 to
1079cc4
Compare
Reject PutObjectRetention mode changes away from COMPLIANCE while retention is still active, matching AWS Object Lock behavior. Retention denials use OBJECT_LOCKED so S3 clients get the Object Lock Access Denied message. Signed-off-by: kajalpareek-lab <pareekkajal97@gmail.com>
a76ac6c to
5475cc6
Compare
3 participants
Summary
Test plan
put_object_retentioncases intest_object_server.test.js(downgrade blocked, upgrade allowed, extend allowed, expired allowed, shorten blocked)test_s3_worm.js—should fail to change compliance mode to governanceSummary by CodeRabbit
Bug Fixes
Tests