-
Notifications
You must be signed in to change notification settings - Fork 6
serviceability: enforce Activated status on suspend handlers #2428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
serviceability: enforce Activated status on suspend handlers #2428
Conversation
d3884fd to
e45c263
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enforces stricter validation for suspend operations across the serviceability program by ensuring only Activated entities can be suspended, aligning all suspend handlers with the pattern already used in link/suspend.rs.
Changes:
- Added status validation checks to six suspend handlers (user, device, contributor, location, exchange, multicastgroup) requiring
Activatedstatus before suspension - Added six new negative tests to verify that attempting to suspend entities in invalid states (Pending or Suspended) correctly fails with
InvalidStatuserror
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| user/suspend.rs | Added status check to enforce Activated status before suspension |
| multicastgroup/suspend.rs | Added status check to enforce Activated status before suspension |
| location/suspend.rs | Added status check to enforce Activated status before suspension |
| exchange/suspend.rs | Added status check to enforce Activated status before suspension |
| device/suspend.rs | Added status check to enforce Activated status before suspension |
| contributor/suspend.rs | Added status check to enforce Activated status before suspension |
| user_tests.rs | Added negative test verifying suspend fails when user is already suspended |
| multicastgroup_test.rs | Added negative test verifying suspend fails when multicastgroup is pending |
| location_test.rs | Added negative test verifying suspend fails when location is already suspended |
| exchange_test.rs | Added negative test verifying suspend fails when exchange is already suspended |
| device_test.rs | Added negative test verifying suspend fails when device is pending |
| contributor_test.rs | Added negative test verifying suspend fails when contributor is already suspended |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@RedaRahmani Thank you very much for your contribution, and sorry for the delay in reviewing it. I wanted to ask if you could rebase onto main and remove the Device and Link cases, we’ve removed the suspended status from those accounts. We also added a requirement to document changes in the changelog. In some specific cases where there’s nothing to declare, the |
e45c263 to
5ff89b4
Compare
|
Hey @juan-malbeclabs thanks for the review. I just rebased, removed the |
|
Hi @RedaRahmani . Could you resolve the conflicts? Also make sure that when you amend you sign your commit, we recently enabled this feature. |
5ff89b4 to
f693694
Compare
|
@RedaRahmani I could run the tests now but they will fail again due to both commits being unsigned. Could you squash them and sign the new one? You will see on Github a "verified" tag next to your commit when this is done correctly. |
5beffb6 to
82e5be1
Compare
|
Hi @martinsander00, Done I squashed the commits and re-pushed with a signed commit. Thanks for the heads-up, appreciate it! |
Looks like you signed the last commit but you didn't squash. If I look at the commits window I still see 2 and only the second one is verified. |
82e5be1 to
0153acc
Compare
|
Hi @martinsander00 , sorry for the confusion earlier. I’ve now squashed everything into one commit and re-pushed with a signed (Verified) commit. |
|
@RedaRahmani looks like everything is good to go. I missed something and that is that your CHANGELOG entry is in the 0.8.1 release section. It should be in the unreleased section. That is my bad for missing it |
fix SetGlobalConfig accounts in exchange_test
0153acc to
f0d9bff
Compare
|
Hi @martinsander00, done ! I moved the CHANGELOG entry from the 0.8.1 section into the Unreleased section. Thanks for catching that! |
|
@RedaRahmani thanks a lot for your help! |
|
Thanks @martinsander00 , really appreciate the review and merge! |
Title
Enforce Activated status before suspend + add negative tests
Summary
This PR makes the suspend logic in the serviceability program a bit stricter and more consistent.
Right now, some *_suspend handlers don’t check the current status of the entity before allowing a suspend. That means it’s possible to try to suspend something that is still Pending or already Suspended, while link/suspend.rs already enforces that only Activated entities can be suspended.
Here I align all suspend handlers with that same rule and add one negative test per entity type to make sure we don’t regress in the future.
Closes : #2221.
What changed
For the following handlers:
location/suspend.rsexchange/suspend.rscontributor/suspend.rsdevice/suspend.rsuser/suspend.rsmulticastgroup/suspend.rsI now:
Deserialize the entity
Check that
entity.status == EntityStatus::ActivatedReturn
DoubleZeroError::InvalidStatusotherwise (with a small debug log under#[cfg(test)])The check follows the same pattern already used in link/suspend.rs:
This doesn’t change behaviour for valid suspends (entities already Activated). It just tightens validation when the status is not valid for a suspend.
I added one negative test per entity type to make the new checks explicit and guarded by tests:
test_suspend_location_from_suspended_failstest_suspend_exchange_from_suspended_failstest_suspend_contributor_from_suspended_failstest_suspend_device_from_pending_failstest_suspend_user_from_suspended_failstest_suspend_multicastgroup_from_pending_failsEach test:
Puts the entity in an invalid state for suspend (Pending or Suspended depending on the case),
Calls the corresponding *_suspend processor,
Asserts that it fails with
DoubleZeroError::InvalidStatus.This way each handler’s status check is covered and future changes will be forced to keep this behaviour.
While working on the tests, I also fixed the account list passed to device activation.
device/activate expects:
deviceglobalstatepayersystem_programThe previous test setup was missing some of these, so it didn’t really mirror the on-chain invocation. I updated the test to provide the full, correct account list.
This doesn’t change behaviour, but it makes the test more realistic and closer to what actually happens at runtime.
Testing
From the smartcontract workspace:
Results:
All 6 new negative tests pass
All existing tests in doublezero-serviceability pass
cargo clippy clean (no new warnings)