From cd1caa88cbdfdee7cc3eafd3982cfd4c742d1107 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20Pe=C3=B1aranda?= Date: Fri, 31 Jul 2026 11:13:57 -0400 Subject: [PATCH] =?UTF-8?q?fix(persistence):=20make=20the=20S3=20tenant?= =?UTF-8?q?=E2=86=92prefix=20derivation=20injective=20and=20prefix-disjoin?= =?UTF-8?q?t=20(#447)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `S3Keyspace::with_tenant_prefix` derived a tenant's data-key prefix with `trim_matches('/')`, so `acme`, `/acme`, `acme/` and `//acme` all resolved to the prefix `acme` while the tenant registry — injective since #271 — held them as four separate, separately-registrable tenants. `purge_tenant_data` resolves its location through that same derivation, so `DELETE /admin/tenants/%2Facme?purge=true` deleted tenant `acme`'s resources *and* its version history, left `acme`'s registry record in place, and reported success. The end state was a tenant still registered, still listed, with all of its data gone. A second defect lives in the same derivation. `RESERVED_TENANT_IDS` is compared against the whole id, so `acme/resources` provisions cleanly — and then stores everything under `acme/resources/…`, which is exactly what tenant `acme` sweeps on purge and enumerates on list (`list_current_keys` selects on `/current.json` anywhere beneath the prefix). That is a cross-tenant delete *and* a cross-tenant read, reachable through the admin API alone. The derivation now escapes exactly the ids that are not already safe, via the `registry_object_id` helper #271 introduced, and is the identity for every id that is. That distinction is the whole design: S3 is the system of record, not a derived index, and has no `$reindex` equivalent — changing a computed prefix does not move objects, it makes them unreachable. So `acme`, `acme/research` and `__system__` keep their exact prefixes, and the only ids that move are those whose objects are currently commingled with another tenant's or sitting inside another tenant's purge radius, where there is no coherent single-tenant dataset at the old location to preserve. Deliberately does not touch any ingress validator: that is #385 / PR #450, which already rejects these ids at every entry point. This is the half #450 cannot cover — it guards only `register_tenant`, leaving `purge_tenant_data` and `deregister_tenant` unguarded by design so operators can clean up legacy tenants, and those construct a `TenantId` directly with no validator in the path. Tests: the two guarantees are asserted as properties over an adversarial id corpus, and the consequences are driven end to end through the mock S3 client. All 8 fail against the previous derivation. A backend-agnostic tenant-id fidelity suite runs the same id shapes through SQLite, PostgreSQL and MongoDB, whose scoping is exact-match on the raw id, so the "no derivation, therefore no defect" claim for those backends is checked rather than asserted in prose. --- .../persistence/src/backends/s3/keyspace.rs | 426 +++++++++++++++++- crates/persistence/src/backends/s3/tests.rs | 234 ++++++++++ crates/persistence/tests/mongodb_tests.rs | 49 ++ crates/persistence/tests/multitenancy/mod.rs | 2 + .../multitenancy/tenant_id_fidelity_suite.rs | 182 ++++++++ .../multitenancy/tenant_id_fidelity_tests.rs | 32 ++ crates/persistence/tests/postgres_tests.rs | 49 ++ 7 files changed, 971 insertions(+), 3 deletions(-) create mode 100644 crates/persistence/tests/multitenancy/tenant_id_fidelity_suite.rs create mode 100644 crates/persistence/tests/multitenancy/tenant_id_fidelity_tests.rs 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; + } }