fix(rest): derive the per-user settings key injectively (#270)

[mauripunzueta]

Fixes #270.

What was actually still broken

Two of the issue's four checkboxes had already landed on main and are left alone here:

• Missing/empty sub is already rejected — crates/auth/src/provider/jwks_bearer.rs filters empty and adds sub to required_spec_claims.
• HFS_AUTH_ISSUER is already required — crates/hfs/src/main.rs, since 5dda82ba9 (the original auth PR). No deployment with auth on and no issuer has ever booted, so there is no compat cost to worry about.

What remained: the non-injective key, and the If-Match note at the bottom of the issue.

The key

format!("{iss}|{sub}") is not injective — (iss="https://idp", sub="a|b") and (iss="https://idp|a", sub="b") render identically, so two principals shared one settings document. Subjects containing | are routine (Auth0/Cognito emit google-oauth2|1076…).

Now u2:{iss_len}:{iss}:{sub}. Length-prefixing the issuer is unambiguous for any byte content and reserves no character. A \0 delimiter was considered and rejected: PostgreSQL refuses U+0000 in TEXT and user_key is a TEXT PRIMARY KEY, so it would fail at insert on one of the four backends.

UserKey is now a private-field newtype with exactly two constructors and no From<String>/FromStr/Deserialize, so a colliding key can't be hand-assembled. This deliberately diverges from TenantId, which is freely constructible because tenant IDs legitimately come from config and headers — there's a note in the code so nobody "fixes" the inconsistency later.

The auth-disabled key becomes l2:, which no principal-derived key can equal.

Migration — all four backends, no schema change

user_key is an opaque primary key on SQLite, PostgreSQL, MongoDB and S3, so changing the derivation needs no DDL anywhere. What it would do is orphan every deployed document — including recentQueries, which holds FHIR search strings and therefore potentially names/MRNs/DOBs. Orphaned means unreachable and undeletable through any API, so leaving them was not acceptable.

A batch migration is impossible by construction: deriving the new key needs iss and sub separately, but the stored legacy key is exactly the ambiguous string that cannot be split back apart. (SettingsStore also has no enumeration primitive.) So adoption runs lazily on the request path, where the real Principal is in hand. It's backend-agnostic, so all four are covered by one code path.

Two safety properties:

• Skipped when either component contains |. That document may already be commingled between principals; promoting it would launder one user's settings into another's. Those callers correctly start empty.
• Copy-then-delete, conditioned on the new key being empty. An interruption leaves the legacy copy and the next request retries; a concurrent adopter isn't clobbered.

The hit path is still a single read — the legacy key is only consulted on a miss, which after first access never happens again for that user.

If-Match

The issue's closing note was right, and the surrounding code had more wrong with it:

• Malformed If-Match was discarded. .parse().ok() → None → no precondition → unconditional last-writer-wins. A request that explicitly asked to be conditional silently became unconditional. Now 412. On PATCH it was worse: None also opted the request into the conflict-absorbing retry loop.
• If-Match: * was treated as "no precondition" and blind-created the document. Per RFC 9110 §13.1.1 it asserts the resource exists, so it now 412s against an empty store. (update.rs already got this right for FHIR routes.)
• Weak ETags were a spec violation here. If-Match requires strong comparison (§8.8.3.2), under which a W/ validator can never match — a conformant client echoing our own ETag back would be 412'd forever. Now emits "{n}", still accepts W/"{n}" from clients built against earlier releases. The FHIR handlers keep weak ETags, which FHIR mandates — that divergence is intentional and documented at both sites.

The three assertions that encoded the old behavior were rewritten to assert the correct behavior, not deleted.

Also

• SettingsStore::delete_settings across all four backends. Needed to move rather than duplicate a document, and it's the lifecycle the other three methods already implied (a document you can create should be erasable).
• AuthConfig::validate() hoists the two startup checks off main.rs onto the type. They only existed in the binary, so any other embedder of the library crates could build an enabled config with no pinned issuer — and iss is what qualifies a subject into a per-user identity.
• EXPECTED_MINIO_SETTINGS_TESTS bumped 4 → 5 for the added MinIO test. That guard fails the build on a count mismatch by design, so it needed updating with the test.

Testing

• Unit: injectivity against the exact colliding pair, |-bearing subjects, empty/multibyte issuers, local-key unforgeability, legacy-key gating; the full If-Match tri-state; AuthConfig::validate.
• Integration: malformed If-Match on PUT and PATCH (asserting the write did not land), * against empty and populated stores, strong-ETag emission with weak still accepted, legacy adoption end-to-end, and adoption not clobbering an existing current document.
• Per-backend delete_settings idempotency for SQLite, PostgreSQL, MongoDB, MinIO/S3.

Not compiled or run locally — this environment has no C linker, so build scripts can't compile. Everything here is cargo fmt-clean and reviewed by hand; CI is the first real check. Draft for that reason.

Deliberately out of scope

Found while working on this, each worth its own issue rather than growing this PR:

1. If-Match is a comma-separated #entity-tag list and none of the four call sites split on it. Here a multi-tag header now 412s (safe); on update.rs/patch.rs it produces a permanent 412 (a live FHIR conformance bug). The fix belongs in ConditionalHeaders.
2. delete.rs ignores If-Match entirely — same defect class, on a destructive method.
3. user_settings has no tenant column on any backend, so purge_tenant_data provably cannot reach PHI-bearing recentQueries. Deliberate and documented as user-global, but it leaves tenant offboarding/erasure incomplete.
4. validate_aud = false whenever no audience is configured — currently a startup warning; worth deciding if it should be more.

[mauripunzueta]

Filed the out-of-scope findings from this PR as follow-ups:

• If-Match is a comma-separated list, but all four call sites compare it whole (permanent 412 on FHIR update/patch) #311 — If-Match comma-lists unsplit at all four call sites (permanent 412 on FHIR update/patch; root cause is ConditionalHeaders::from_headers)
• delete.rs ignores If-Match entirely — version preconditions are silently dropped on a destructive method #312 — delete.rs ignores If-Match entirely
• user_settings has no tenant attribution, so purge_tenant_data cannot reach PHI-bearing recentQueries/savedQueries #313 — user_settings has no tenant attribution, so purge_tenant_data can't reach PHI-bearing savedQueries
• Audience validation is disabled entirely when HFS_AUTH_AUDIENCE is unset — decide whether a startup warning is enough #314 — audience validation disabled when HFS_AUTH_AUDIENCE is unset (decision, not a defect)

#311 subsumes the private IfMatch enum introduced here — it should move into ConditionalHeaders so all four sites share one parser.

[codecov]

Codecov Report

❌ Patch coverage is 97.89157% with 7 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/rest/src/handlers/user_settings.rs	96.52%	4 Missing ⚠️
.../persistence/src/backends/mongodb/user_settings.rs	83.33%	1 Missing ⚠️
...persistence/src/backends/postgres/user_settings.rs	66.66%	1 Missing ⚠️
...s/persistence/src/backends/sqlite/user_settings.rs	96.87%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!