diff --git a/src/backend/services/identity-resolution/src/api/seed.rs b/src/backend/services/identity-resolution/src/api/seed.rs index f1a2b4f1f..f39e1dcdc 100644 --- a/src/backend/services/identity-resolution/src/api/seed.rs +++ b/src/backend/services/identity-resolution/src/api/seed.rs @@ -219,13 +219,22 @@ pub async fn create_persons_seed( tenant_id: tenant, author_person_id: author, }; - if state.seed_tx.try_send(job).is_err() { + if let Err(err) = try_enqueue_job(&state.seed_tx, job) { // Channel full/closed — fail the row so it isn't a zombie, and tell the // caller to retry later (503, not 500 — parity with the .NET queue-full). - let _ = ops_repo::fail(&state.db, operation_id, "seed queue full; retry later").await; - return Err(CanonicalError::service_unavailable() - .with_detail("seed queue is full; retry later") - .create()); + // A failed status update is logged, not propagated: 503/retry-later is + // still the right caller signal, and a row left `queued` is reclaimed + // by the startup zombie sweep (`sweep_zombies`). + if let Err(db_err) = + ops_repo::fail(&state.db, operation_id, "seed queue full; retry later").await + { + tracing::error!( + error = %db_err, + %operation_id, + "failed to mark the refused seed operation as failed" + ); + } + return Err(err); } // Audit the enqueue (parity with the .NET `persons_seed.enqueue` audit). @@ -249,6 +258,21 @@ pub async fn create_persons_seed( Ok((StatusCode::ACCEPTED, [(LOCATION, location)], Json(body))) } +/// Hand the job to the worker channel, mapping a full/closed channel to the +/// caller-facing 503 (parity with the .NET queue-full path). Split out of the +/// handler so the refusal is unit-testable: the e2e suite cannot fill the +/// channel deterministically from outside. +fn try_enqueue_job( + tx: &mpsc::Sender, + job: PersonsSeedJob, +) -> Result<(), CanonicalError> { + tx.try_send(job).map_err(|_| { + CanonicalError::service_unavailable() + .with_detail("seed queue is full; retry later") + .create() + }) +} + /// `GET /v1/persons-seed/{id}` — poll one operation. pub async fn get_persons_seed( Extension(state): Extension>, @@ -403,3 +427,50 @@ pub async fn run_worker( } } } + +#[cfg(test)] +mod tests { + use axum::http::StatusCode; + use axum::response::IntoResponse; + + use super::*; + + fn job() -> PersonsSeedJob { + PersonsSeedJob { + operation_id: Uuid::from_u128(1), + tenant_id: Uuid::from_u128(2), + author_person_id: Uuid::from_u128(3), + } + } + + #[test] + fn enqueue_maps_closed_channel_to_503() -> anyhow::Result<()> { + let (tx, rx) = mpsc::channel(1); + drop(rx); + let Err(err) = try_enqueue_job(&tx, job()) else { + anyhow::bail!("closed channel must refuse the job"); + }; + assert_eq!( + err.into_response().status(), + StatusCode::SERVICE_UNAVAILABLE + ); + Ok(()) + } + + #[test] + fn enqueue_maps_full_channel_to_503() -> anyhow::Result<()> { + let (tx, _rx) = mpsc::channel(1); + assert!( + try_enqueue_job(&tx, job()).is_ok(), + "first job fits the 1-slot channel" + ); + let Err(err) = try_enqueue_job(&tx, job()) else { + anyhow::bail!("full channel must refuse the job"); + }; + assert_eq!( + err.into_response().status(), + StatusCode::SERVICE_UNAVAILABLE + ); + Ok(()) + } +} diff --git a/src/ingestion/tests/e2e/identity/test_error_contracts.py b/src/ingestion/tests/e2e/identity/test_error_contracts.py new file mode 100644 index 000000000..30ac9efa3 --- /dev/null +++ b/src/ingestion/tests/e2e/identity/test_error_contracts.py @@ -0,0 +1,328 @@ +"""Contract: the error-path status codes the rest of the suite leaves out. + +Closes the per-status-code gaps of the coverage report: validation 400s on +the mutating endpoints, 404s for unknown ids on DELETE/GET, the 401/403 gate +proven on every route (not just a sibling), malformed-UUID / query-param +400s, and the nil-tenant 400. Each case was probed against BOTH +implementations before being added; behavior only the Rust port has (see +`lib.identity.supports_strict_input_validation`) is asserted in its own +capability-gated section, so this file is the full Rust surface while the +.NET run stays green. + +Deliberately absent here: +- 503 on POST /v1/persons-seed (seed queue full): the queue capacity is a + compile-time constant (gear.rs, 100) and the refusal needs the channel + full at the instant of the POST — not deterministically inducible from a + black-box test, the same reason the coverage gate excludes >=500 codes + (SERVER_FAULT_FLOOR). Pinned instead by Rust unit tests on the extracted + refusal path (identity-resolution src/api/seed.rs, `try_enqueue_job`). + +Nothing here mutates state: the 400s fail validation before any write, the +404s target ids that don't exist, and the 401/403s never pass the gate. +""" + +from __future__ import annotations + +import uuid + +import pytest + +from identity.contract import problem +from lib import identity_seed as seed + +pytestmark = pytest.mark.identity + +# A well-formed UUID no fixture row uses (same convention as test_subchart). +UNKNOWN_ID = "00000000-0000-4000-8000-00000000dead" + +NIL_UUID = uuid.UUID(int=0) + +TOO_LONG_REASON = "x" * 501 + + +# ── POST /v1/persons-seed ───────────────────────────────────────────────── + + +def test_persons_seed_unsupported_mode_400(api) -> None: + """Only 'link-by-email' exists; the refusal happens before any enqueue, + so nothing is written.""" + r = api.post("/v1/persons-seed", json={"mode": "no-such-mode"}) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +# ── GET /v1/persons-seed/{id} + list — the gate proven per-route ───────── + + +def test_persons_seed_get_unknown_404(api) -> None: + r = api.get(f"/v1/persons-seed/{UNKNOWN_ID}") + assert r.status_code == 404, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_persons_seed_get_403_non_admin(bob_api) -> None: + r = bob_api.get(f"/v1/persons-seed/{UNKNOWN_ID}") + assert r.status_code == 403, f"status={r.status_code} body={r.text}" + + +def test_persons_seed_get_401_unauthenticated(anon_api) -> None: + assert anon_api.get(f"/v1/persons-seed/{UNKNOWN_ID}").status_code == 401 + + +def test_persons_seed_list_403_non_admin(bob_api) -> None: + r = bob_api.get("/v1/persons-seed") + assert r.status_code == 403, f"status={r.status_code} body={r.text}" + + +def test_persons_seed_list_401_unauthenticated(anon_api) -> None: + assert anon_api.get("/v1/persons-seed").status_code == 401 + + +# ── /v1/roles ───────────────────────────────────────────────────────────── + + +def test_role_create_empty_name_400(api) -> None: + r = api.post("/v1/roles", json={"name": ""}) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_role_create_name_too_long_400(api) -> None: + """Both validators cap the name at 64 chars.""" + r = api.post("/v1/roles", json={"name": "r" * 65}) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_role_create_401_unauthenticated(anon_api) -> None: + assert anon_api.post("/v1/roles", json={"name": "e2e-nope"}).status_code == 401 + + +def test_role_delete_unknown_404(api) -> None: + r = api.delete(f"/v1/roles/{UNKNOWN_ID}") + assert r.status_code == 404, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_role_delete_403_non_admin(bob_api) -> None: + r = bob_api.delete(f"/v1/roles/{UNKNOWN_ID}") + assert r.status_code == 403, f"status={r.status_code} body={r.text}" + + +def test_role_delete_401_unauthenticated(anon_api) -> None: + assert anon_api.delete(f"/v1/roles/{UNKNOWN_ID}").status_code == 401 + + +# ── /v1/person-roles ────────────────────────────────────────────────────── + + +def test_person_role_create_nil_person_400(api) -> None: + r = api.post( + "/v1/person-roles", + json={"person_id": str(NIL_UUID), "role_id": str(seed.ADMIN_ROLE_ID)}, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_person_role_create_nil_role_400(api) -> None: + r = api.post( + "/v1/person-roles", + json={"person_id": str(seed.BOB), "role_id": str(NIL_UUID)}, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_person_role_create_reason_too_long_400(api) -> None: + """Both validators cap `reason` at 500 chars on the CREATE (the DELETE + body is validated only by the Rust side — a divergence, not tested).""" + r = api.post( + "/v1/person-roles", + json={ + "person_id": str(seed.BOB), + "role_id": str(seed.ADMIN_ROLE_ID), + "reason": TOO_LONG_REASON, + }, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_person_role_create_401_unauthenticated(anon_api) -> None: + r = anon_api.post( + "/v1/person-roles", + json={"person_id": str(seed.BOB), "role_id": str(seed.ADMIN_ROLE_ID)}, + ) + assert r.status_code == 401 + + +def test_person_role_delete_unknown_404(api) -> None: + r = api.delete(f"/v1/person-roles/{UNKNOWN_ID}") + assert r.status_code == 404, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_person_role_delete_403_non_admin(bob_api) -> None: + r = bob_api.delete(f"/v1/person-roles/{UNKNOWN_ID}") + assert r.status_code == 403, f"status={r.status_code} body={r.text}" + + +def test_person_role_delete_401_unauthenticated(anon_api) -> None: + assert anon_api.delete(f"/v1/person-roles/{UNKNOWN_ID}").status_code == 401 + + +# ── /v1/visibility ──────────────────────────────────────────────────────── + + +def test_visibility_create_nil_viewer_400(api) -> None: + r = api.post( + "/v1/visibility", + json={"viewer_person_id": str(NIL_UUID), "viewed_person_id": str(seed.HIDDEN)}, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_visibility_create_reason_too_long_400(api) -> None: + r = api.post( + "/v1/visibility", + json={ + "viewer_person_id": str(seed.ALICE), + "viewed_person_id": str(seed.HIDDEN), + "reason": TOO_LONG_REASON, + }, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_visibility_create_401_unauthenticated(anon_api) -> None: + r = anon_api.post( + "/v1/visibility", + json={"viewer_person_id": str(seed.ALICE), "viewed_person_id": str(seed.HIDDEN)}, + ) + assert r.status_code == 401 + + +def test_visibility_delete_unknown_404(api) -> None: + r = api.delete(f"/v1/visibility/{UNKNOWN_ID}") + assert r.status_code == 404, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_visibility_delete_403_non_admin(bob_api) -> None: + r = bob_api.delete(f"/v1/visibility/{UNKNOWN_ID}") + assert r.status_code == 403, f"status={r.status_code} body={r.text}" + + +def test_visibility_delete_401_unauthenticated(anon_api) -> None: + assert anon_api.delete(f"/v1/visibility/{UNKNOWN_ID}").status_code == 401 + + +# ── GET /v1/subchart (forest) — param validation proven on THIS route ──── + + +def test_forest_negative_depth_400(api) -> None: + r = api.get("/v1/subchart?depth=-1") + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_forest_invalid_valid_at_400(api) -> None: + r = api.get("/v1/subchart?valid_at=not-a-date") + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + + +# ── malformed ids / query params → 400 (route + binder level) ──────────── + + +def test_role_delete_malformed_uuid_400(api) -> None: + assert api.delete("/v1/roles/not-a-uuid").status_code == 400 + + +def test_person_role_delete_malformed_uuid_400(api) -> None: + assert api.delete("/v1/person-roles/not-a-uuid").status_code == 400 + + +def test_visibility_delete_malformed_uuid_400(api) -> None: + assert api.delete("/v1/visibility/not-a-uuid").status_code == 400 + + +def test_persons_seed_get_malformed_uuid_400(api) -> None: + assert api.get("/v1/persons-seed/not-a-uuid").status_code == 400 + + +def test_person_roles_list_malformed_limit_400(api) -> None: + assert api.get("/v1/person-roles?limit=abc").status_code == 400 + + +def test_persons_seed_list_malformed_limit_400(api) -> None: + assert api.get("/v1/persons-seed?limit=abc").status_code == 400 + + +def test_person_roles_list_malformed_person_filter_400(api) -> None: + assert api.get("/v1/person-roles?person=not-a-uuid").status_code == 400 + + +def test_visibility_list_malformed_active_400(api) -> None: + assert api.get("/v1/visibility?active=maybe").status_code == 400 + + +# ── Rust-only strict validation (capability-gated) ──────────────────────── + + +@pytest.fixture +def strict_api(identity_svc, api): + """`api`, but only on an implementation with the strict input validation + the Rust port added — skipped (before any request) elsewhere.""" + if not identity_svc.supports_strict_input_validation: + pytest.skip("strict input validation is Rust-only (see lib.identity)") + return api + + +def test_person_role_delete_reason_too_long_400(strict_api) -> None: + """The revoke body's `reason` cap is enforced BEFORE the lookup, so an + unknown id keeps this non-mutating.""" + r = strict_api.request( + "DELETE", f"/v1/person-roles/{UNKNOWN_ID}", json={"reason": TOO_LONG_REASON} + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_visibility_delete_reason_too_long_400(strict_api) -> None: + r = strict_api.request( + "DELETE", f"/v1/visibility/{UNKNOWN_ID}", json={"reason": TOO_LONG_REASON} + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_visibility_create_nil_viewed_400(strict_api) -> None: + """A present-but-nil target is nonsense (only an ABSENT viewed_person_id + means whole-tree visibility); .NET creates the grant, Rust refuses.""" + r = strict_api.post( + "/v1/visibility", + json={"viewer_person_id": str(seed.ALICE), "viewed_person_id": str(NIL_UUID)}, + ) + assert r.status_code == 400, f"status={r.status_code} body={r.text}" + problem(r) + + +def test_subchart_malformed_uuid_400(strict_api) -> None: + """Rust rejects the unparseable path id as 400; the .NET binder answers + 404 — a reviewed divergence, so only the Rust behavior is pinned.""" + assert strict_api.get("/v1/subchart/not-a-uuid").status_code == 400 + + +# ── nil tenant in the JWT → 400 tenant_unresolved ──────────────────────── + + +def test_nil_tenant_400(identity_svc) -> None: + """A token whose tenant_id claim is the nil UUID: both implementations + refuse with an explicit 400 (tenant_unresolved) instead of silently + querying an empty tenant.""" + with identity_svc.client(sub=str(seed.ALICE), tenant=str(NIL_UUID)) as c: + r = c.get("/v1/roles") + assert r.status_code == 400, f"status={r.status_code} body={r.text}" diff --git a/src/ingestion/tests/e2e/lib/api_coverage.py b/src/ingestion/tests/e2e/lib/api_coverage.py index 3c96e4175..6cd4dfb7b 100755 --- a/src/ingestion/tests/e2e/lib/api_coverage.py +++ b/src/ingestion/tests/e2e/lib/api_coverage.py @@ -100,14 +100,51 @@ "DELETE /v1/person-roles/{id}": frozenset({200}), # answers 204 "DELETE /v1/visibility/{id}": frozenset({200}), # answers 204 } +# The identity spec declares ONLY the (often wrong) 200 per route, so every +# real code the suite proves is REQUIRED_EXTRA — the mutation success codes +# AND the error contract (identity/test_error_contracts.py): validation 400s, +# unknown-id 404s, the 401/403 gate per route, and the duplicate/guard +# conflicts. BLOCKING: a disappearing error test (or a handler regressing to +# a different code) fails the gate instead of dimming an advisory. 5xx stays +# out (SERVER_FAULT_FLOOR — e.g. the queue-full 503 on POST /v1/persons-seed +# is not deterministically inducible black-box). Self-cleaning once the spec +# starts declaring real codes (REDUNDANT then forces the move). +_IDENTITY_COMMON_REQUIRED_EXTRA: dict[str, frozenset[int]] = { + "POST /v1/profiles": frozenset({400, 401, 404}), + "POST /v1/persons-seed": frozenset({202, 400, 401, 403}), + "GET /v1/persons-seed/{id}": frozenset({400, 401, 403, 404}), + "GET /v1/persons-seed": frozenset({400, 401, 403}), + "POST /v1/roles": frozenset({201, 400, 401, 403, 409}), + "GET /v1/roles": frozenset({400, 401, 403}), + "DELETE /v1/roles/{id}": frozenset({204, 400, 401, 403, 404}), + "POST /v1/person-roles": frozenset({201, 400, 401, 403}), + "GET /v1/person-roles": frozenset({400, 401, 403}), + "DELETE /v1/person-roles/{id}": frozenset({204, 400, 401, 403, 404}), + "POST /v1/visibility": frozenset({201, 400, 401, 403}), + "GET /v1/visibility": frozenset({400, 401, 403}), + "DELETE /v1/visibility/{id}": frozenset({204, 400, 401, 403, 404}), + "GET /v1/subchart": frozenset({400, 401}), + "GET /v1/subchart/{personId}": frozenset({400, 401, 404}), +} + +# Where the implementations answer DIFFERENT codes for the same guard (the +# .NET 422 → Rust 409 family, contract.UNPROCESSABLE_OR_CONFLICT; the +# .NET-only deprecated lookup), the delta is per-suite on top of the common +# base. IDENTITY_REQUIRED_EXTRA: dict[str, frozenset[int]] = { - "POST /v1/roles": frozenset({201}), - "POST /v1/person-roles": frozenset({201}), - "POST /v1/visibility": frozenset({201}), - "POST /v1/persons-seed": frozenset({202}), - "DELETE /v1/roles/{id}": frozenset({204}), - "DELETE /v1/person-roles/{id}": frozenset({204}), - "DELETE /v1/visibility/{id}": frozenset({204}), + **_IDENTITY_COMMON_REQUIRED_EXTRA, + "POST /v1/profiles": _IDENTITY_COMMON_REQUIRED_EXTRA["POST /v1/profiles"] | {422}, + "DELETE /v1/roles/{id}": _IDENTITY_COMMON_REQUIRED_EXTRA["DELETE /v1/roles/{id}"] | {422}, + "DELETE /v1/person-roles/{id}": _IDENTITY_COMMON_REQUIRED_EXTRA["DELETE /v1/person-roles/{id}"] + | {422}, + "GET /v1/persons/{email}": frozenset({404}), +} +IDENTITY_RUST_REQUIRED_EXTRA: dict[str, frozenset[int]] = { + **_IDENTITY_COMMON_REQUIRED_EXTRA, + "POST /v1/profiles": _IDENTITY_COMMON_REQUIRED_EXTRA["POST /v1/profiles"] | {409}, + "DELETE /v1/roles/{id}": _IDENTITY_COMMON_REQUIRED_EXTRA["DELETE /v1/roles/{id}"] | {409}, + "DELETE /v1/person-roles/{id}": _IDENTITY_COMMON_REQUIRED_EXTRA["DELETE /v1/person-roles/{id}"] + | {409}, } # The Rust implementation dropped the deprecated persons lookup (approved @@ -167,7 +204,7 @@ IDENTITY_RUST_SKIP_LIST, IDENTITY_BLOCKED, IDENTITY_UNIVERSAL_BOILERPLATE, - IDENTITY_REQUIRED_EXTRA, + IDENTITY_RUST_REQUIRED_EXTRA, ), "authenticator": ( AUTHENTICATOR_SKIP_LIST, diff --git a/src/ingestion/tests/e2e/lib/identity.py b/src/ingestion/tests/e2e/lib/identity.py index ba3a0178c..fceb52adc 100644 --- a/src/ingestion/tests/e2e/lib/identity.py +++ b/src/ingestion/tests/e2e/lib/identity.py @@ -112,6 +112,19 @@ def supports_containerized_clickhouse(implementation: str) -> bool: """ return implementation == "rust" + +def supports_strict_input_validation(implementation: str) -> bool: + """Validation the Rust port ADDED beyond the .NET behavior (reviewed on + epic #1602): a too-long revoke `reason` in DELETE bodies is rejected + (400; .NET ignores the body's length), a present-but-nil + `viewed_person_id` on POST /v1/visibility is rejected (400; .NET happily + creates the nonsense grant), and a malformed person_id on + GET /v1/subchart/{person_id} is a 400 (the .NET route binder answers + 404). A capability of the EXPLICIT selection, never probed from runtime + behavior.""" + return implementation == "rust" + + _HEALTH_TIMEOUT_S = float( os.environ.get( "E2E_IDENTITY_HEALTH_TIMEOUT_S", "120" @@ -238,6 +251,10 @@ def supports_deprecated_person_lookup(self) -> bool: def supports_containerized_clickhouse(self) -> bool: return supports_containerized_clickhouse(self.implementation) + @property + def supports_strict_input_validation(self) -> bool: + return supports_strict_input_validation(self.implementation) + def start(self) -> None: create_identity_database(self.cfg) if self.implementation == "rust":