diff --git a/crates/persistence/src/backends/s3/keyspace.rs b/crates/persistence/src/backends/s3/keyspace.rs index a571975cc..d11985006 100644 --- a/crates/persistence/src/backends/s3/keyspace.rs +++ b/crates/persistence/src/backends/s3/keyspace.rs @@ -32,13 +32,68 @@ impl S3Keyspace { /// Returns a new keyspace with `tenant_id` appended to the base prefix. /// /// Used in `PrefixPerTenant` mode to scope all keys under a per-tenant - /// directory segment without changing the bucket. + /// directory segment without changing the bucket. This is the *only* place + /// a tenant id becomes a data-key location, reached through the single + /// funnel `S3Backend::tenant_location`, so the two guarantees below are + /// what tenant isolation on S3 rests on. + /// + /// # Guarantees + /// + /// 1. **Injective** — distinct ids yield distinct prefixes. + /// 2. **Prefix-disjoint** — no tenant's *sweep* prefix + /// (`{prefix}/resources/`, `{prefix}/history/`, `{prefix}/bulk/`) is a + /// prefix of another tenant's keys. + /// + /// Both are needed, and neither implies the other. (1) alone would still let + /// a tenant named `acme/resources` nest its whole keyspace inside the prefix + /// `acme` sweeps; (2) alone would still let `/a` and `a` share one prefix. + /// + /// # Why this is not a plain escape of the whole id + /// + /// S3 is the **system of record**, not a derived index: there is no + /// `$reindex`-equivalent that could rebuild objects at a new key. A + /// derivation change does not move objects — it makes the old ones + /// unreachable, and a later `purge_tenant_data` will not reach them either. + /// So the derivation escapes exactly the ids that are *not already safe* and + /// is the **identity** for every id that is: + /// + /// | tenant id | before | after | + /// |---|---|---| + /// | `acme` | `acme` | `acme` (unchanged) | + /// | `acme/research` | `acme/research` | `acme/research` (unchanged) | + /// | `__system__` | `__system__` | `__system__` (unchanged) | + /// | `/acme` | `acme` ⚠ shared | `%2Facme` | + /// | `acme/` | `acme` ⚠ shared | `acme%2F` | + /// | `//acme` | `acme` ⚠ shared | `%2F%2Facme` | + /// | `acme/resources` | `acme/resources` ⚠ inside `acme`'s sweep | `acme%2Fresources` | + /// + /// Every id whose prefix moves is one whose objects are *currently* + /// commingled with another tenant's, or sitting inside another tenant's + /// purge radius. There is no coherent single-tenant dataset at the old + /// location to preserve — relocating it **is** the remediation. Every id a + /// deployment can already be storing safely keeps its exact prefix, so an + /// upgrade moves nothing that was working. See issue #447. + /// + /// # Why the guarantees hold + /// + /// [`is_keyspace_safe`] rejects any id containing a character + /// [`registry_object_id`] escapes (`%`, `\`, space) and any id whose + /// structure is unsafe (an empty segment, or a sweep-root segment after the + /// first) — and every such structural defect implies the id contains `/`. + /// So an escaped prefix always contains `%`, and a safe prefix never can: + /// the two ranges are disjoint, and each mapping is injective on its own + /// domain, giving (1). For (2), an escaped prefix contains no `/` at all and + /// so cannot end in `/{sweep-root}`, while a safe prefix is barred from a + /// non-first sweep-root segment by construction. pub fn with_tenant_prefix(&self, tenant_id: &str) -> Self { - let tenant = tenant_id.trim_matches('/'); + let tenant = tenant_prefix_component(tenant_id); let merged = match &self.base_prefix { Some(base) => format!("{}/{}", base, tenant), - None => tenant.to_string(), + None => tenant, }; + // `new`'s slash trim is a no-op on the tenant component by construction + // (a safe id has no empty segment, an escaped one has no `/`), so it can + // no longer erase the distinction between `a`, `/a`, `a/`, and `//a`. Self::new(Some(merged)) } @@ -420,10 +475,375 @@ fn registry_object_id(tenant_id: &str) -> String { out } +/// Path segments the keyspace reserves *beneath* a tenant prefix. +/// +/// These are the roots every tenant-scoped key hangs off, and — the reason they +/// matter here — the prefixes `S3Backend::purge_tenant_data` sweeps and +/// `list_current_keys` enumerates. A tenant whose id carried one of these as a +/// later segment would place its entire keyspace *inside* another tenant's +/// radius: tenant `acme` sweeps `acme/resources/`, which contains every object +/// of a tenant literally named `acme/resources` — a cross-tenant purge, and a +/// cross-tenant read, since `list_current_keys` selects on `/current.json` +/// anywhere under the prefix. +/// +/// `tenants` and `_system.user-settings` are deliberately absent: those live on +/// the *base* keyspace, never under a tenant prefix, and are already protected +/// structurally by the direct-child filter in `list_tenants` and the digest leaf +/// in `user_settings_key` (see [`S3Keyspace::tenant_registry_prefix`], #271). +/// Adding them here would relocate a top-level tenant named `tenants` for no +/// gain. +const SWEEP_ROOT_SEGMENTS: [&str; 3] = ["resources", "history", "bulk"]; + +/// Prefix component standing in for the empty tenant id. +/// +/// `registry_object_id("")` is `""`, which would collapse the tenant prefix into +/// the *base* keyspace — the one namespace no tenant may occupy. A bare `%` is +/// used instead: [`registry_object_id`] emits `%` only as the first byte of a +/// three-byte escape, so no non-empty id can produce it, and [`is_keyspace_safe`] +/// bars `%` from every unescaped prefix. The empty id is unreachable through +/// every ingress today; this exists so the derivation is total and disjoint +/// without depending on that. +const EMPTY_TENANT_PREFIX: &str = "%"; + +/// Whether `tenant_id` can be used verbatim as its tenant prefix component. +/// +/// This is deliberately the **complement of "currently broken"**: an id is unsafe +/// exactly when using it raw would either collide with another id's prefix or +/// nest inside another tenant's sweep radius. Keeping the predicate that tight is +/// what makes [`S3Keyspace::with_tenant_prefix`] an identity for every id a +/// deployment can already be storing safely — see that function for why moving +/// objects on a system of record is the cost that governs this design. +/// +/// Do **not** widen this into a general tenant-id validator. It answers one +/// question — "is this id safe as an S3 key prefix?" — and nothing here may +/// depend on ingress validation having run: `purge_tenant_data` constructs its +/// `TenantId` directly from an operator-supplied path segment, with no validator +/// in the path. (The canonical ingress validator is issue #385 / PR #450; it +/// bounds the input, it does not make a lossy mapping safe.) +fn is_keyspace_safe(tenant_id: &str) -> bool { + // The empty id has no prefix of its own; see `EMPTY_TENANT_PREFIX`. + if tenant_id.is_empty() { + return false; + } + // Exactly the characters `registry_object_id` escapes. Barring them from the + // unescaped range is what keeps the two ranges disjoint, and hence the whole + // derivation injective. + if tenant_id.contains(['%', '\\', ' ']) { + return false; + } + + let mut segments = tenant_id.split('/'); + // An empty first segment is a leading `/` — the many-to-one case issue #447 + // is named for (`a`, `/a`, `a/`, `//a` all trimmed to `a`). + let Some(first) = segments.next() else { + return false; + }; + if first.is_empty() { + return false; + } + for segment in segments { + // Empty: a trailing or repeated `/`. + if segment.is_empty() { + return false; + } + // A sweep root anywhere but the first segment nests this tenant inside + // the parent id's purge/list radius. The first segment is exempt: it + // sits directly under the base prefix, where nothing sweeps a bare + // `resources/`, so a top-level tenant named `resources` is harmless and + // must not be relocated. + if SWEEP_ROOT_SEGMENTS.contains(&segment) { + return false; + } + } + true +} + +/// Derives the tenant's prefix component: the id itself when +/// [`is_keyspace_safe`], otherwise one opaque [`registry_object_id`] segment. +fn tenant_prefix_component(tenant_id: &str) -> String { + if is_keyspace_safe(tenant_id) { + return tenant_id.to_string(); + } + let escaped = registry_object_id(tenant_id); + if escaped.is_empty() { + return EMPTY_TENANT_PREFIX.to_string(); + } + escaped +} + #[cfg(test)] mod tests { use super::*; + // ── Tenant prefix derivation (issue #447) ────────────────────────────── + // + // `with_tenant_prefix` is the single derivation every S3 data key passes + // through, so these are property assertions over an adversarial corpus + // rather than example-by-example checks: the properties are what tenant + // isolation rests on, and a new id shape should be added to the corpus + // rather than given its own test. + + /// Ids that collided under the old `trim_matches('/')`, ids that nested + /// inside another tenant's sweep radius, ordinary ids that must not move, + /// and forged-escape attempts. + const TENANT_ID_CORPUS: &[&str] = &[ + // Ordinary ids — the derivation must be the identity for these. + "default", + "acme", + "__system__", + "acme-research", + "tenant_123", + "acme.org", + "acme/research", + "acme/research/oncology", + // Slash padding: all of these trimmed to `acme` before the fix. + "/acme", + "acme/", + "//acme", + "acme//", + "/acme/", + "acme//research", + // Nested inside a parent tenant's purge/list radius. + "acme/resources", + "acme/history", + "acme/bulk", + "acme/research/resources", + // Top-level namespace names: harmless (nothing sweeps a bare + // `resources/`), so these must NOT be relocated. + "resources", + "history", + "bulk", + "tenants", + // Characters the escape itself uses — a literal must not be able to + // forge another id's escaped form. + "%2Facme", + "acme%2Fresearch", + "acme%25", + "acme\\research", + "acme research", + // Degenerate. + "", + ]; + + /// The subset of the corpus that must round-trip unchanged. Anything here + /// that started moving would be silent data loss on upgrade: S3 is the + /// system of record and there is no `$reindex` to rebuild objects at a new + /// key. `__system__` in particular holds every shared terminology resource. + const UNCHANGED_TENANT_IDS: &[&str] = &[ + "default", + "acme", + "__system__", + "acme-research", + "tenant_123", + "acme.org", + "acme/research", + "acme/research/oncology", + "resources", + "history", + "bulk", + "tenants", + ]; + + /// Every key a tenant can write, for prefix-containment checks. + fn sample_keys(ks: &S3Keyspace) -> Vec { + vec![ + ks.current_resource_key("Patient", "p1"), + ks.history_version_key("Patient", "p1", "2"), + ks.resource_type_prefix("Patient"), + ks.history_system_prefix(), + ks.submit_state_key("submitter", "sub-1"), + ] + } + + /// The prefixes a tenant sweeps on purge / enumerates on list. Anything of + /// another tenant's underneath one of these is a cross-tenant purge and a + /// cross-tenant read. + fn sweep_prefixes(ks: &S3Keyspace) -> Vec { + vec![ + ks.resources_prefix(), + ks.history_root_prefix(), + ks.submit_root_prefix(), + ] + } + + /// Guarantee 1: distinct tenant ids never share a prefix. + /// + /// Before the fix `trim_matches('/')` made this many-to-one — `acme`, + /// `/acme`, `acme/` and `//acme` all resolved to `acme`, so the registry + /// held four tenants while the keyspace held one (issue #447). + #[test] + fn tenant_prefix_derivation_is_injective() { + for base in [None, Some("hfs".to_string())] { + let ks = S3Keyspace::new(base.clone()); + for (i, x) in TENANT_ID_CORPUS.iter().enumerate() { + for y in TENANT_ID_CORPUS.iter().skip(i + 1) { + assert_ne!( + ks.with_tenant_prefix(x).resources_prefix(), + ks.with_tenant_prefix(y).resources_prefix(), + "tenants {x:?} and {y:?} share a data prefix (base={base:?})" + ); + } + } + } + } + + /// Guarantee 2: no tenant's sweep prefix covers another tenant's keys. + /// + /// Injectivity alone does not give this. Tenant `acme/resources` has a + /// prefix all its own, yet every one of its objects used to sit under + /// `acme/resources/` — exactly what `purge_tenant_data("acme")` deletes and + /// `list_current_keys` enumerates. + #[test] + fn no_tenant_sweep_prefix_covers_another_tenants_keys() { + for base in [None, Some("hfs".to_string())] { + let ks = S3Keyspace::new(base.clone()); + for x in TENANT_ID_CORPUS { + for y in TENANT_ID_CORPUS { + if x == y { + continue; + } + let victim = ks.with_tenant_prefix(y); + for prefix in sweep_prefixes(&ks.with_tenant_prefix(x)) { + for key in sample_keys(&victim) { + assert!( + !key.starts_with(&prefix), + "tenant {x:?} sweeping {prefix:?} would reach \ + tenant {y:?}'s key {key:?} (base={base:?})" + ); + } + } + } + } + } + } + + /// The derivation is the identity for every id that is already safe, so an + /// upgrade relocates no object that was working. This is the constraint that + /// ruled out escaping the whole id unconditionally. + #[test] + fn safe_tenant_ids_keep_their_existing_prefix() { + for base in [None, Some("hfs".to_string())] { + let ks = S3Keyspace::new(base.clone()); + for id in UNCHANGED_TENANT_IDS { + let expected = match &base { + Some(b) => format!("{b}/{id}/resources/"), + None => format!("{id}/resources/"), + }; + assert_eq!( + ks.with_tenant_prefix(id).resources_prefix(), + expected, + "id {id:?} must keep its pre-fix prefix (base={base:?})" + ); + } + } + } + + /// The ids that *do* move, and where to. These are exactly the ids whose + /// objects are currently commingled with another tenant's or sitting inside + /// another tenant's purge radius, so relocating them is the remediation — + /// there is no coherent single-tenant dataset at the old location. + #[test] + fn unsafe_tenant_ids_move_to_one_escaped_segment() { + let ks = S3Keyspace::new(None); + for (id, expected) in [ + ("/acme", "%2Facme"), + ("acme/", "acme%2F"), + ("//acme", "%2F%2Facme"), + ("acme/resources", "acme%2Fresources"), + ("acme//research", "acme%2F%2Fresearch"), + ("acme\\research", "acme%5Cresearch"), + ("acme research", "acme%20research"), + ("%2Facme", "%252Facme"), + ("", "%"), + ] { + assert_eq!( + ks.with_tenant_prefix(id).resources_prefix(), + format!("{expected}/resources/"), + "id {id:?}" + ); + } + } + + /// The escape must not be forgeable: a tenant whose id is the literal text + /// `%2Facme` must not land where `/acme` lands. `%` is escaped first, which + /// is what makes the mapping reversible. + #[test] + fn an_escaped_prefix_cannot_be_forged_by_a_literal() { + let ks = S3Keyspace::new(None); + assert_ne!( + ks.with_tenant_prefix("/acme").resources_prefix(), + ks.with_tenant_prefix("%2Facme").resources_prefix(), + ); + assert_ne!( + ks.with_tenant_prefix("acme/resources").resources_prefix(), + ks.with_tenant_prefix("acme%2Fresources").resources_prefix(), + ); + } + + /// The empty id must not collapse into the base keyspace, where the tenant + /// registry and the user-settings store live. + #[test] + fn the_empty_tenant_id_does_not_become_the_base_keyspace() { + for base in [None, Some("hfs".to_string())] { + let ks = S3Keyspace::new(base); + assert_ne!( + ks.with_tenant_prefix("").resources_prefix(), + ks.resources_prefix(), + ); + assert_ne!( + ks.with_tenant_prefix("").tenant_registry_prefix(), + ks.tenant_registry_prefix(), + ); + } + } + + /// The headline scenario from issue #447, stated as the operator sees it: + /// purging `/acme` must not touch `acme`'s resources or its version history. + #[test] + fn purging_a_slash_padded_id_no_longer_reaches_the_bare_id() { + let ks = S3Keyspace::new(None); + let padded = ks.with_tenant_prefix("/acme"); + let bare = ks.with_tenant_prefix("acme"); + + for prefix in sweep_prefixes(&padded) { + for key in sample_keys(&bare) { + assert!( + !key.starts_with(&prefix), + "purging `/acme` ({prefix}) would delete `acme`'s {key}" + ); + } + } + } + + /// `is_keyspace_safe` is the whole rule; assert it directly so a change to + /// the predicate has to confront each clause. + #[test] + fn keyspace_safety_predicate_clauses() { + // Ordinary and hierarchical ids are safe. + assert!(is_keyspace_safe("acme")); + assert!(is_keyspace_safe("acme/research")); + assert!(is_keyspace_safe("__system__")); + // Empty segments — leading, trailing, repeated. + assert!(!is_keyspace_safe("/acme")); + assert!(!is_keyspace_safe("acme/")); + assert!(!is_keyspace_safe("//acme")); + assert!(!is_keyspace_safe("acme//research")); + assert!(!is_keyspace_safe("")); + // Characters the escape uses. + assert!(!is_keyspace_safe("acme%25")); + assert!(!is_keyspace_safe("acme\\research")); + assert!(!is_keyspace_safe("acme research")); + // Sweep roots after the first segment. + for ns in SWEEP_ROOT_SEGMENTS { + assert!(!is_keyspace_safe(&format!("acme/{ns}"))); + assert!(!is_keyspace_safe(&format!("acme/research/{ns}"))); + // ...but harmless as the first segment: nothing sweeps a bare + // `resources/`, so a top-level tenant so named must not move. + assert!(is_keyspace_safe(ns)); + assert!(is_keyspace_safe(&format!("{ns}/acme"))); + } + } + /// The registry key establishes tenant identity: ids that a lossy sanitiser /// would collapse together must map to distinct objects. A collision here /// lets one tenant read, overwrite, and deregister another's record. diff --git a/crates/persistence/src/backends/s3/tests.rs b/crates/persistence/src/backends/s3/tests.rs index f383e2f49..14adb2e73 100644 --- a/crates/persistence/src/backends/s3/tests.rs +++ b/crates/persistence/src/backends/s3/tests.rs @@ -141,6 +141,24 @@ impl MockS3Client { state.objects.keys().filter(|(b, _)| b == bucket).count() } + /// Every object key currently stored in `bucket`, sorted. + /// + /// Used to assert the *literal* key layout, which + /// `bucket_object_count` cannot express — the tenant-prefix fidelity tests + /// (issue #447) need to prove that a safe tenant id still lands on exactly + /// its pre-fix key, not merely that some object exists. + fn keys(&self, bucket: &str) -> Vec { + let state = self.state.lock().unwrap(); + let mut keys: Vec = state + .objects + .keys() + .filter(|(b, _)| b == bucket) + .map(|(_, k)| k.clone()) + .collect(); + keys.sort(); + keys + } + /// Total number of `put_object` calls received so far. fn put_count(&self) -> u64 { self.state.lock().unwrap().put_count @@ -1386,6 +1404,222 @@ async fn purge_tenant_data_sweeps_resources_and_history() { assert_eq!(backend.count(&tb, None).await.unwrap(), 1); } +// ============================================================================ +// Tenant prefix fidelity (issue #447) +// ============================================================================ +// +// `S3Keyspace::with_tenant_prefix` used `trim_matches('/')`, so `acme`, `/acme`, +// `acme/` and `//acme` all resolved to the data prefix `acme` while the registry +// (injective since #271) held them as four separate tenants. A second defect in +// the same derivation let a tenant named `acme/resources` nest its entire +// keyspace inside the prefix tenant `acme` sweeps and lists. +// +// `keyspace.rs` proves the properties over an adversarial corpus of ids. These +// drive the consequences through the real storage API instead, because that is +// what an operator experiences and what the issue reported: a purge that deletes +// a different tenant than the one named, and a read that returns another +// tenant's resources. + +/// Issue #447, consequence 1: `DELETE /admin/tenants/%2Facme?purge=true` used to +/// delete tenant `acme`'s resources *and its version history*, while leaving +/// `acme`'s registry record in place — a still-registered, still-listed tenant +/// whose data was gone, reported to the caller as success. +#[tokio::test] +async fn purging_a_slash_padded_tenant_leaves_the_bare_tenant_intact() { + let mock = Arc::new(MockS3Client::with_buckets(&["test-bucket"])); + let backend = make_prefix_backend(mock); + let bare = tenant("acme"); + + let created = backend + .create( + &bare, + "Patient", + json!({"resourceType":"Patient","id":"p1"}), + FhirVersion::default(), + ) + .await + .unwrap(); + backend + .update( + &bare, + &created, + json!({"resourceType":"Patient","id":"p1","active":true}), + ) + .await + .unwrap(); + + // Every id that used to collapse onto `acme`. + for padded in ["/acme", "acme/", "//acme", "/acme/"] { + let removed = backend.purge_tenant_data(padded).await.unwrap(); + assert_eq!(removed, 0, "purging {padded:?} must not reach any resource"); + } + + assert!( + backend + .read(&bare, "Patient", "p1") + .await + .unwrap() + .is_some(), + "tenant `acme`'s current resource must survive" + ); + assert!( + backend + .vread(&bare, "Patient", "p1", "1") + .await + .unwrap() + .is_some(), + "tenant `acme`'s version history must survive" + ); + assert_eq!(backend.count(&bare, None).await.unwrap(), 1); +} + +/// Issue #447: slash-padded ids are distinct tenants in the registry, so they +/// must also be distinct in the data keyspace — writes under one must be +/// invisible to the others. +#[tokio::test] +async fn slash_padded_tenants_do_not_share_resources() { + let mock = Arc::new(MockS3Client::with_buckets(&["test-bucket"])); + let backend = make_prefix_backend(mock); + + let ids = ["acme", "/acme", "acme/", "//acme"]; + for (i, id) in ids.iter().enumerate() { + backend + .create( + &tenant(id), + "Patient", + json!({"resourceType":"Patient","id": format!("p{i}")}), + FhirVersion::default(), + ) + .await + .unwrap(); + } + + for (i, id) in ids.iter().enumerate() { + let ctx = tenant(id); + assert_eq!( + backend.count(&ctx, None).await.unwrap(), + 1, + "tenant {id:?} must see only its own resource" + ); + assert!( + backend + .read(&ctx, "Patient", &format!("p{i}")) + .await + .unwrap() + .is_some() + ); + for (j, _) in ids.iter().enumerate() { + if i == j { + continue; + } + assert!( + backend + .read(&ctx, "Patient", &format!("p{j}")) + .await + .unwrap() + .is_none(), + "tenant {id:?} must not see p{j}" + ); + } + } +} + +/// The second defect in the same derivation: a tenant whose id carries a +/// keyspace namespace as a later segment used to store its objects *inside* the +/// parent id's radius. `acme` sweeps `acme/resources/` on purge and enumerates +/// it on list, which is exactly where `acme/resources` kept everything — a +/// cross-tenant delete and a cross-tenant read. +/// +/// Reachable through the admin API today: `RESERVED_TENANT_IDS` is compared +/// against the whole id, so `acme/resources` provisions cleanly. +#[tokio::test] +async fn a_tenant_named_after_a_sweep_root_survives_its_parents_purge() { + let mock = Arc::new(MockS3Client::with_buckets(&["test-bucket"])); + let backend = make_prefix_backend(mock); + let parent = tenant("acme"); + + for nested_id in ["acme/resources", "acme/history", "acme/bulk"] { + let nested = tenant(nested_id); + backend + .create( + &nested, + "Patient", + json!({"resourceType":"Patient","id":"nested"}), + FhirVersion::default(), + ) + .await + .unwrap(); + + // The parent must not be able to see it... + assert_eq!( + backend.count(&parent, None).await.unwrap(), + 0, + "tenant `acme` must not enumerate {nested_id:?}'s resources" + ); + assert!( + backend + .read(&parent, "Patient", "nested") + .await + .unwrap() + .is_none() + ); + + // ...nor destroy it by purging itself. + backend.purge_tenant_data("acme").await.unwrap(); + assert!( + backend + .read(&nested, "Patient", "nested") + .await + .unwrap() + .is_some(), + "purging `acme` must not delete {nested_id:?}'s data" + ); + + backend.purge_tenant_data(nested_id).await.unwrap(); + } +} + +/// Hierarchical and ordinary ids must keep their exact prefix, so upgrading +/// relocates nothing that was already stored safely. Asserted through the API: +/// a resource written before the fix is still readable after it. +/// +/// `__system__` matters most — it holds the shared terminology resources, and +/// silently orphaning those would be far worse than the defect being fixed. +#[tokio::test] +async fn safe_tenant_ids_still_resolve_to_their_original_prefix() { + let mock = Arc::new(MockS3Client::with_buckets(&["test-bucket"])); + let backend = make_prefix_backend(mock.clone()); + + for id in ["default", "acme", "acme/research", "__system__"] { + let ctx = tenant(id); + backend + .create( + &ctx, + "Patient", + json!({"resourceType":"Patient","id":"p1"}), + FhirVersion::default(), + ) + .await + .unwrap(); + assert!(backend.read(&ctx, "Patient", "p1").await.unwrap().is_some()); + } + + // The object keys themselves are unchanged — this is what makes the upgrade + // a no-op for these tenants rather than a silent relocation. + let keys = mock.keys("test-bucket"); + for expected in [ + "default/resources/Patient/p1/current.json", + "acme/resources/Patient/p1/current.json", + "acme/research/resources/Patient/p1/current.json", + "__system__/resources/Patient/p1/current.json", + ] { + assert!( + keys.iter().any(|k| k == expected), + "expected unchanged key {expected:?}; got {keys:?}" + ); + } +} + // ============================================================================ // Backend error handling — a misconfigured bucket is not an empty store // ============================================================================ diff --git a/crates/persistence/tests/mongodb_tests.rs b/crates/persistence/tests/mongodb_tests.rs index f7b9b2c3e..7a2d30612 100644 --- a/crates/persistence/tests/mongodb_tests.rs +++ b/crates/persistence/tests/mongodb_tests.rs @@ -309,6 +309,14 @@ mod shared_mongo { } } +/// The backend-agnostic tenant-id fidelity scenarios (issue #447), shared +/// verbatim with the SQLite and PostgreSQL suites. +/// +/// `#[path]`-included rather than placed in `tests/common/`, which no test +/// target declares and cargo therefore never compiles (issue #306). +#[path = "multitenancy/tenant_id_fidelity_suite.rs"] +mod tenant_id_fidelity_suite; + fn create_tenant(tenant_id: &str) -> TenantContext { TenantContext::new(TenantId::new(tenant_id), TenantPermissions::full_access()) } @@ -3618,3 +3626,44 @@ async fn mongodb_integration_purge_tenant_settings() { let dotted_doc = backend.get_settings(&dotted).await.unwrap().unwrap(); assert_eq!(dotted_doc.document, json!({})); } + +// ============================================================================ +// Issue #447 — tenant-id fidelity +// ============================================================================ +// +// The #447 defect is S3's: it *derives* a key prefix from the tenant id, and +// `trim_matches('/')` made that derivation many-to-one. MongoDB derives +// nothing — documents carry a `tenant_id` field matched exactly, under the +// default (case-sensitive) collation — so the scoping is the identity mapping +// and the defect cannot occur here. +// +// That is a code reading, and a code reading is exactly what let the same +// defect class sit undiscovered in the two backends that *do* derive (#384 on +// Elasticsearch, #447 on S3). So it is checked against a real server, where +// BSON matching and collation are the engine's behaviour rather than the Rust's. +// +// Each test gets its own database (`build_test_database_name`), so a fixed base +// id is safe here. + +#[tokio::test] +async fn mongodb_integration_distinct_tenant_ids_never_share_data() { + let Some(backend) = create_backend("tenant_fidelity").await else { + eprintln!( + "Skipping mongodb_integration_distinct_tenant_ids_never_share_data (requires Docker or HFS_TEST_MONGODB_URL)" + ); + return; + }; + tenant_id_fidelity_suite::distinct_tenant_ids_never_share_data(&backend, "acme").await; +} + +#[tokio::test] +async fn mongodb_integration_purging_one_tenant_leaves_the_look_alikes_intact() { + let Some(backend) = create_backend("tenant_fidelity_purge").await else { + eprintln!( + "Skipping mongodb_integration_purging_one_tenant_leaves_the_look_alikes_intact (requires Docker or HFS_TEST_MONGODB_URL)" + ); + return; + }; + tenant_id_fidelity_suite::purging_one_tenant_leaves_the_look_alikes_intact(&backend, "acme") + .await; +} diff --git a/crates/persistence/tests/multitenancy/mod.rs b/crates/persistence/tests/multitenancy/mod.rs index 397512692..a684c5eea 100644 --- a/crates/persistence/tests/multitenancy/mod.rs +++ b/crates/persistence/tests/multitenancy/mod.rs @@ -5,3 +5,5 @@ pub mod cross_tenant_tests; pub mod isolation_tests; +pub mod tenant_id_fidelity_suite; +pub mod tenant_id_fidelity_tests; diff --git a/crates/persistence/tests/multitenancy/tenant_id_fidelity_suite.rs b/crates/persistence/tests/multitenancy/tenant_id_fidelity_suite.rs new file mode 100644 index 000000000..704139ce8 --- /dev/null +++ b/crates/persistence/tests/multitenancy/tenant_id_fidelity_suite.rs @@ -0,0 +1,182 @@ +//! Backend-agnostic tenant-id fidelity suite (issue #447). +//! +//! Issue #447 is an S3 defect: `S3Keyspace::with_tenant_prefix` derived a +//! tenant's data-key prefix with `trim_matches('/')`, so `acme`, `/acme`, +//! `acme/` and `//acme` all resolved to one prefix while the tenant registry +//! held them as four separate tenants. The fix lives in `s3/keyspace.rs` and is +//! proven there, both as properties over an adversarial id corpus and end to end +//! through the mock S3 client. +//! +//! This suite exists because the defect is an instance of a *class*: **a backend +//! that derives a storage location from the tenant id rather than matching on +//! the id itself owes an injective derivation.** Two backends derive; three do +//! not: +//! +//! | backend | tenant scoping | injective? | +//! |---|---|---| +//! | SQLite | `tenant_id TEXT` in the composite primary key, bound and compared with `=` (BINARY collation) | identity | +//! | PostgreSQL | `tenant_id TEXT` in the composite primary key, bound and compared with `=` | identity | +//! | MongoDB | `"tenant_id": ` exact BSON match, default (case-sensitive) collation | identity | +//! | S3 | **derives a key prefix** | issue #447 — fixed in `s3/keyspace.rs` | +//! | Elasticsearch | **derives an index name** (`to_lowercase`) | issue #384 — fixed in PR #446 | +//! +//! For the three "identity" rows that claim is a code reading, and a code +//! reading is exactly what let #384 and #447 sit undiscovered in the two rows +//! that derive. So the claim is asserted instead: these scenarios drive the id +//! shapes that S3 collapsed through each backend's real storage path and check +//! that nothing commingles. They are cheap, they run against SQLite in ordinary +//! CI, and they run against PostgreSQL and MongoDB wherever those containers are +//! available. +//! +//! Included by `#[path]` into each backend's test binary rather than living in +//! `tests/common/`, which no test target declares and cargo therefore never +//! compiles (issue #306) — the same arrangement as +//! `transactions/if_match_suite.rs`. +//! +//! ## Shared-database backends +//! +//! PostgreSQL and MongoDB run the whole binary against one container database, +//! so every scenario takes a `base` id that the caller must make unique. All the +//! variant ids are derived from it, keeping runs from colliding while preserving +//! the exact *shape* relationships (padding, nesting) that are under test. + +#![allow(dead_code)] + +use serde_json::json; + +use helios_fhir::FhirVersion; +use helios_persistence::core::{PurgableStorage, ResourceStorage}; +use helios_persistence::tenant::{TenantContext, TenantId, TenantPermissions}; + +/// The id shapes that S3's derivation used to collapse onto `base`, plus `base` +/// itself. Distinct strings, therefore distinct tenants — on every backend. +fn variants(base: &str) -> Vec { + vec![ + base.to_string(), + format!("/{base}"), + format!("{base}/"), + format!("//{base}"), + // Not a padding case: this one nested *inside* the prefix `base` sweeps + // and lists, which is the second defect in the same derivation. + format!("{base}/resources"), + ] +} + +fn ctx(id: &str) -> TenantContext { + TenantContext::new(TenantId::new(id), TenantPermissions::full_access()) +} + +/// Distinct tenant ids must never share stored data. +/// +/// Writes one uniquely-named Patient per variant tenant, then checks every +/// tenant sees exactly its own — no commingled reads, no inflated counts. +pub async fn distinct_tenant_ids_never_share_data(backend: &S, base: &str) +where + S: ResourceStorage, +{ + let ids = variants(base); + + for (i, id) in ids.iter().enumerate() { + backend + .create( + &ctx(id), + "Patient", + json!({"resourceType": "Patient", "id": format!("p{i}")}), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create in tenant {id:?} failed: {e}")); + } + + for (i, id) in ids.iter().enumerate() { + let tenant = ctx(id); + + assert!( + backend + .read(&tenant, "Patient", &format!("p{i}")) + .await + .unwrap() + .is_some(), + "tenant {id:?} must see its own resource" + ); + + for (j, other) in ids.iter().enumerate() { + if i == j { + continue; + } + assert!( + backend + .read(&tenant, "Patient", &format!("p{j}")) + .await + .unwrap() + .is_none(), + "tenant {id:?} must not see p{j}, which belongs to {other:?}" + ); + } + + assert_eq!( + backend.count(&tenant, None).await.unwrap(), + 1, + "tenant {id:?} must count only its own resource" + ); + } +} + +/// Purging one tenant must not touch another whose id merely *looks* like it. +/// +/// This is issue #447's headline consequence stated backend-agnostically: on S3 +/// it destroyed the bare tenant's resources and version history while leaving +/// its registry record behind, and reported success to the caller. +pub async fn purging_one_tenant_leaves_the_look_alikes_intact(backend: &S, base: &str) +where + S: ResourceStorage + PurgableStorage, +{ + let ids = variants(base); + + for (i, id) in ids.iter().enumerate() { + backend + .create( + &ctx(id), + "Patient", + json!({"resourceType": "Patient", "id": format!("p{i}")}), + FhirVersion::default(), + ) + .await + .unwrap_or_else(|e| panic!("create in tenant {id:?} failed: {e}")); + } + + // Purge every variant *except* the bare id, one at a time. + for id in ids.iter().skip(1) { + backend + .purge_tenant_data(id) + .await + .unwrap_or_else(|e| panic!("purge of tenant {id:?} failed: {e}")); + + assert!( + backend + .read(&ctx(base), "Patient", "p0") + .await + .unwrap() + .is_some(), + "purging {id:?} must not delete tenant {base:?}'s resource" + ); + } + + assert_eq!( + backend.count(&ctx(base), None).await.unwrap(), + 1, + "tenant {base:?} must be untouched by every look-alike purge" + ); + + // And the purges did do their own job. + for (i, id) in ids.iter().enumerate().skip(1) { + assert!( + backend + .read(&ctx(id), "Patient", &format!("p{i}")) + .await + .unwrap() + .is_none(), + "tenant {id:?}'s own resource should have been purged" + ); + } +} diff --git a/crates/persistence/tests/multitenancy/tenant_id_fidelity_tests.rs b/crates/persistence/tests/multitenancy/tenant_id_fidelity_tests.rs new file mode 100644 index 000000000..4f731ddbc --- /dev/null +++ b/crates/persistence/tests/multitenancy/tenant_id_fidelity_tests.rs @@ -0,0 +1,32 @@ +//! SQLite arm of the backend-agnostic tenant-id fidelity suite (issue #447). +//! +//! SQLite stores `tenant_id` as a `TEXT` column in each table's composite +//! primary key and compares it with a bound `=` under the default BINARY +//! collation, so the tenant scoping is the identity mapping and no derivation +//! can lose information. That is a code reading; these run it. +//! +//! See [`super::tenant_id_fidelity_suite`] for why the claim is asserted per +//! backend rather than argued once. + +#![cfg(feature = "sqlite")] + +use helios_persistence::backends::sqlite::SqliteBackend; + +use super::tenant_id_fidelity_suite as suite; + +fn backend() -> SqliteBackend { + let backend = SqliteBackend::in_memory().expect("in-memory SQLite backend"); + backend.init_schema().expect("init schema"); + backend +} + +#[tokio::test] +async fn sqlite_distinct_tenant_ids_never_share_data() { + // Each test builds its own in-memory database, so a fixed base id is safe. + suite::distinct_tenant_ids_never_share_data(&backend(), "acme").await; +} + +#[tokio::test] +async fn sqlite_purging_one_tenant_leaves_the_look_alikes_intact() { + suite::purging_one_tenant_leaves_the_look_alikes_intact(&backend(), "acme").await; +} diff --git a/crates/persistence/tests/postgres_tests.rs b/crates/persistence/tests/postgres_tests.rs index 72674e123..856bcc195 100644 --- a/crates/persistence/tests/postgres_tests.rs +++ b/crates/persistence/tests/postgres_tests.rs @@ -21,6 +21,12 @@ use helios_persistence::core::BackendKind; #[path = "transactions/if_match_suite.rs"] mod if_match_suite; +/// The backend-agnostic tenant-id fidelity scenarios (issue #447), shared +/// verbatim with the SQLite and MongoDB suites. Declared at the top level for +/// the same `#[path]` resolution reason as `if_match_suite` above. +#[path = "multitenancy/tenant_id_fidelity_suite.rs"] +mod tenant_id_fidelity_suite; + // ============================================================================ // Backend Configuration Tests (no PostgreSQL instance required) // ============================================================================ @@ -5215,4 +5221,47 @@ mod postgres_integration { postgres_integration_transaction_delete_accepts_matching_if_match, transaction_delete_accepts_matching_if_match ); + + // ======================================================================== + // Issue #447 — tenant-id fidelity, on a real PostgreSQL instance + // + // The #447 defect is S3's: it *derives* a key prefix from the tenant id and + // the derivation was many-to-one. PostgreSQL derives nothing — `tenant_id` + // is a `TEXT` column in each composite primary key, bound and compared with + // `=` — so the scoping is the identity mapping and the defect cannot occur + // here. + // + // That is a code reading, and a code reading is precisely what let the same + // defect class sit undiscovered in the two backends that *do* derive (#384 + // on Elasticsearch, #447 on S3). So it is checked rather than asserted in + // prose, against a real server: bound parameters, collation, and index + // behaviour are properties of the engine, not of the Rust. + // + // Each test takes a unique base id — the whole binary shares one container + // database, and the scenarios derive fixed resource ids from it. + // ======================================================================== + + fn unique_base(label: &str) -> String { + format!("{}_{}", label, uuid::Uuid::new_v4().simple()) + } + + #[tokio::test] + async fn postgres_integration_distinct_tenant_ids_never_share_data() { + let backend = create_backend().await; + super::tenant_id_fidelity_suite::distinct_tenant_ids_never_share_data( + &backend, + &unique_base("fidelity"), + ) + .await; + } + + #[tokio::test] + async fn postgres_integration_purging_one_tenant_leaves_the_look_alikes_intact() { + let backend = create_backend().await; + super::tenant_id_fidelity_suite::purging_one_tenant_leaves_the_look_alikes_intact( + &backend, + &unique_base("fidelity_purge"), + ) + .await; + } }