From 39707cd2e883518216d8ded7abfeee96941b2bfe Mon Sep 17 00:00:00 2001 From: John Coffey Date: Sun, 27 Sep 2026 18:36:16 -0700 Subject: [PATCH] Legal holds, step 5: held accounts can't be destroyed Destroying a held account removes the login, as offboarding needs, but keeps its data as a deleted account with no expiry, whether or not undelete keeps accounts; its addresses stay reserved and its holds name it from then on. Destroy-now refuses it, and its DestroyAccount task defers itself while it's held or its time hasn't come. Holds placed or released later freeze or free kept accounts in the same settle pass, with 30 days' grace after the last release (LH-8, LH-10). --- crates/common/src/hold.rs | 56 ++++++++++++++++++- crates/features/src/hold/mod.rs | 13 +++++ crates/jmap/src/inbuxa/deleted_account.rs | 38 +++++++++++-- .../src/task_manager/destroy_account.rs | 17 ++++++ tests/src/system/legal_hold.rs | 51 ++++++++++++++++- 5 files changed, 168 insertions(+), 7 deletions(-) diff --git a/crates/common/src/hold.rs b/crates/common/src/hold.rs index c97dd56..cfa7325 100644 --- a/crates/common/src/hold.rs +++ b/crates/common/src/hold.rs @@ -15,7 +15,14 @@ use inbuxa_features::{ hold::{self, HELD_UNTIL, Hold, Keeping, Member, is_held_until}, undelete::records, }; -use registry::schema::{prelude::ObjectType, structs::ArchivedItem}; +use inbuxa_features::undelete::data::{self as undelete_data, KeptAccount}; +use registry::{ + pickle::PickledStream, + schema::{ + prelude::{ObjectInner, ObjectType}, + structs::ArchivedItem, + }, +}; use store::{registry::RegistryQuery, write::now}; use trc::AddContext; use types::id::Id; @@ -29,6 +36,21 @@ const RELEASE_GRACE: u64 = 30 * 86_400; pub struct Settled { pub frozen: usize, pub released: usize, + /// Deleted accounts kept by a hold, or let go by a release (LH-8, LH-10). + pub accounts_frozen: usize, + pub accounts_released: usize, +} + +/// A kept account as it was when deleted, for a hold's scope: its record +/// still names its domain, groups and tenant. +pub fn kept_member(account_id: u32, kept: &KeptAccount) -> Member { + PickledStream::new(&kept.record) + .and_then(|mut stream| ObjectInner::unpickle(ObjectType::Account, &mut stream)) + .and_then(|inner| Member::of(account_id, &inner)) + .unwrap_or(Member { + account: account_id, + ..Default::default() + }) } impl Server { @@ -114,9 +136,41 @@ impl Server { settled.released += 1; } } + + // LH-8, LH-10: deleted accounts kept by undelete follow the holds + // too. Their DestroyAccount task defers itself while they're kept. + let retention = inbuxa_features::undelete::settings::retention(registry) + .await? + .accounts; + for (account_id, mut kept) in undelete_data::kept_accounts(data).await? { + let covered = !hold::covering(data, &kept_member(account_id, &kept)).await?.is_empty(); + let held = is_held_until(kept.kept_until); + let until = if covered && !held { + settled.accounts_frozen += 1; + HELD_UNTIL + } else if !covered && held { + settled.accounts_released += 1; + (kept.deleted_at + retention.unwrap_or(0)).max(now + RELEASE_GRACE) + } else { + continue; + }; + kept.kept_until = until; + let mut batch = store::write::BatchBuilder::new(); + undelete_data::set_kept_account(&mut batch, account_id, &kept)?; + data.write(batch.build_all()) + .await + .caused_by(trc::location!())?; + } Ok(settled) } + /// LH-8: whether a hold covers a deleted account undelete keeps. + pub async fn is_kept_held(&self, account_id: u32, kept: &KeptAccount) -> trc::Result { + Ok(!hold::covering(self.store(), &kept_member(account_id, kept)) + .await? + .is_empty()) + } + /// Every account an active hold covers now. Empty, without looking at /// accounts, when nothing is held. pub async fn held_accounts(&self) -> trc::Result> { diff --git a/crates/features/src/hold/mod.rs b/crates/features/src/hold/mod.rs index 5815647..cdee60f 100644 --- a/crates/features/src/hold/mod.rs +++ b/crates/features/src/hold/mod.rs @@ -484,6 +484,19 @@ pub async fn keep_moved(data: &Store, before: &Member, after: &Member) -> trc::R Ok(()) } +/// LH-8: names `account_id` in every hold that reaches it, so a deleted +/// account, no longer in any domain or tenant, stays held. +pub async fn pin_account(data: &Store, member: &Member) -> trc::Result<()> { + for mut hold in covering(data, member).await? { + if !hold.scope.accounts.contains(&member.account) { + hold.scope.accounts.push(member.account); + hold.scope.accounts.sort_unstable(); + update(data, &hold).await?; + } + } + Ok(()) +} + /// Replaces a hold that `check_update` allowed. pub async fn update(data: &Store, hold: &Hold) -> trc::Result<()> { let mut batch = BatchBuilder::new(); diff --git a/crates/jmap/src/inbuxa/deleted_account.rs b/crates/jmap/src/inbuxa/deleted_account.rs index a635bae..bbfb07c 100644 --- a/crates/jmap/src/inbuxa/deleted_account.rs +++ b/crates/jmap/src/inbuxa/deleted_account.rs @@ -124,13 +124,29 @@ pub async fn reserved( /// of upstream's immediate destruction. Returns the other accounts whose /// access changed, or `None` when nothing is kept. pub async fn keep(server: &Server, id: Id, account: &Account) -> trc::Result>> { - let Some(period) = retention(server.registry()).await?.accounts else { - return Ok(None); - }; let account_id = id.document_id(); - let deleted_at = now(); - let kept_until = deleted_at + period; let inner = ObjectInner::Account(account.clone()); + // inbuxa: LH-8: a held account's data stays, with no expiry, whether or + // not undelete keeps accounts; its holds name it from now on + let member = inbuxa_features::hold::Member::of(account_id, &inner); + let held = match &member { + Some(member) => { + !inbuxa_features::hold::covering(server.store(), member) + .await? + .is_empty() + } + None => false, + }; + let period = retention(server.registry()).await?.accounts; + let deleted_at = now(); + let kept_until = match (held, period) { + (true, _) => inbuxa_features::hold::HELD_UNTIL, + (false, Some(period)) => deleted_at + period, + (false, None) => return Ok(None), + }; + if held && let Some(member) = &member { + inbuxa_features::hold::pin_account(server.store(), member).await?; + } let addresses = addresses_of(server, &inner) .await? .into_iter() @@ -324,6 +340,18 @@ pub async fn set( for id in will_destroy { match data::kept_account(data, id.document_id()).await? { + // inbuxa: LH-8: a held account's data can't be destroyed + Some(kept) + if may_reach(access_token, &kept, Permission::SysAccountDestroy) + && (inbuxa_features::hold::is_held_until(kept.kept_until) + || server.is_kept_held(id.document_id(), &kept).await?) => + { + response.not_destroyed.append( + id, + SetError::forbidden() + .with_description("A legal hold applies to this account, so its data stays."), + ); + } Some(kept) if may_reach(access_token, &kept, Permission::SysAccountDestroy) => { destroy_now(server, id, &kept).await?; response.destroyed.push(id); diff --git a/crates/services/src/task_manager/destroy_account.rs b/crates/services/src/task_manager/destroy_account.rs index ee88695..1d3da5a 100644 --- a/crates/services/src/task_manager/destroy_account.rs +++ b/crates/services/src/task_manager/destroy_account.rs @@ -55,6 +55,23 @@ impl DestroyAccountTask for Server { async fn destroy_account(server: &Server, task: &TaskDestroyAccount) -> trc::Result { let account_id = task.account_id.document_id(); + // inbuxa: LH-8, LH-10: a kept account waits for its time, and a held one + // for its release; "destroy now" clears the kept record first + if let Some(kept) = + inbuxa_features::undelete::data::kept_account(&server.core.storage.data, account_id).await? + { + let now = store::write::now(); + let held = inbuxa_features::hold::is_held_until(kept.kept_until) + || server.is_kept_held(account_id, &kept).await?; + if held || kept.kept_until > now { + let retry = if held { now + 86_400 } else { kept.kept_until }; + return Ok(TaskResult::deferred( + Some(retry), + "The account is still kept: a legal hold applies, or its time hasn't come.", + )); + } + } + // Destroy public keys and masked emails for object in [ObjectType::PublicKey, ObjectType::MaskedEmail] { let mut batch = BatchBuilder::new(); diff --git a/tests/src/system/legal_hold.rs b/tests/src/system/legal_hold.rs index 7e265c8..219f115 100644 --- a/tests/src/system/legal_hold.rs +++ b/tests/src/system/legal_hold.rs @@ -309,7 +309,10 @@ pub async fn test(test: &mut TestServer) { "r": {"name": "Matter 6002", "from": "2020-01-01T00:00:00Z", "to": "2020-12-31T23:59:59Z", "scope": {"accounts": [ranged.id_string()]}}}})) .await; - assert!(response["created"]["w"]["id"].is_string(), "LH-1: {response}"); + let whole_hold = response["created"]["w"]["id"] + .as_str() + .unwrap_or_else(|| panic!("LH-1: {response}")) + .to_string(); assert!(response["created"]["r"]["id"].is_string(), "LH-1: {response}"); let held_client = held.jmap_client().await; @@ -450,6 +453,52 @@ pub async fn test(test: &mut TestServer) { "test 8, LH-10: under 30 days of grace after release: {until}" ); + // Test 8, LH-8: a held account destroyed as a login is kept, data and + // all, with no expiry, although undelete keeps no accounts here + let held_id = held.id_string().to_string(); + admin.destroy_account(held).await; + let kept = |list: Value| { + list["list"] + .as_array() + .and_then(|l| l.iter().find(|a| a["id"] == held_id.as_str()).cloned()) + }; + let (_, list) = admin + .hold_call("inbuxa:DeletedAccount/get", json!({"ids": null})) + .await; + let entry = kept(list.clone()).unwrap_or_else(|| panic!("test 8, LH-8: not kept: {list}")); + assert!( + entry["keptUntil"].as_str().is_some_and(|u| u.starts_with("9999-")), + "test 8, LH-8: kept with an expiry: {entry}" + ); + let (_, response) = admin + .hold_call("inbuxa:DeletedAccount/set", json!({"destroy": [held_id]})) + .await; + assert_eq!( + response["notDestroyed"][held_id.as_str()]["type"], "forbidden", + "test 8, LH-8: destroy-now wasn't refused: {response}" + ); + // Its hold names it now, so no domain or tenant move can drop it + assert!( + admin.hold_get(&whole_hold).await["scope"]["accounts"] + .as_array() + .is_some_and(|a| a.iter().any(|id| id == held_id.as_str())), + "LH-8: the hold doesn't name the deleted account" + ); + // Release: the data is destroyed 30 days later, not before + admin + .hold_set(json!({"reason": "Matter closed", "update": {whole_hold.as_str(): {"released": true}}})) + .await; + let (_, list) = admin + .hold_call("inbuxa:DeletedAccount/get", json!({"ids": null})) + .await; + let entry = kept(list.clone()).unwrap_or_else(|| panic!("test 8, LH-10: gone at release: {list}")); + let until = entry["keptUntil"].as_str().unwrap_or_default().to_string(); + let grace = chrono::Utc::now() + chrono::Duration::days(29); + assert!( + !until.starts_with("9999-") && until > grace.format("%Y-%m-%dT%H:%M:%S").to_string(), + "test 8, LH-10: after release, not 30 days of grace: {until}" + ); + // LH-10: release needs a reason, and a released hold stays, read-only let response = admin .hold_set(json!({"update": {hold_id.as_str(): {"released": true}}}))