diff --git a/charts/insight/templates/secrets.yaml b/charts/insight/templates/secrets.yaml index 5f00a844f..9a71f991e 100644 --- a/charts/insight/templates/secrets.yaml +++ b/charts/insight/templates/secrets.yaml @@ -46,6 +46,22 @@ Secret. ============================================================================== */}} +{{/* +First-login provisioning preconditions. Checked OUTSIDE the credential-mode +body below on purpose: `credentials.autoGenerate: false` is the documented +GitOps path, where this file emits nothing and the config Secret is composed by +deploy/gitops/scripts/compose-app-secrets.sh — a guard living inside that body +would fall silent in exactly the mode that most needs it. +*/}} +{{- if .Values.authenticator.oidc.provisionOnLogin }} +{{- if not ((.Values.global | default dict).tenantDefaultId) }} +{{- fail "authenticator.oidc.provisionOnLogin=true requires global.tenantDefaultId — it feeds tenant_default_id in insight-identity-resolution-config, which the login bootstrap checks the token's tenant against before writing. Without it identity refuses every provision, so first-login sign-in would still fail. Set global.tenantDefaultId, or leave provisionOnLogin off." }} +{{- end }} +{{- if not .Values.identityResolution.deploy }} +{{- fail "authenticator.oidc.provisionOnLogin=true requires identityResolution.deploy=true — the login bootstrap provisions by calling identity-resolution's internal route, and there is nothing to call when the service is not deployed." }} +{{- end }} +{{- end }} + {{- if .Values.credentials.autoGenerate }} {{- /* Attempt lookup; on first install this returns nil. Helm 3 skips @@ -220,6 +236,11 @@ stringData: # id_token claim carrying the IdP's stable external user id for source_type # (Entra: "oid"; the generic OIDC "sub" is not the same directory-stable id). APP__gears__authenticator__config__idp__external_id_claim: {{ .Values.authenticator.oidc.externalIdClaim | default "sub" | quote }} + # Provision a person on first login instead of refusing one the journal has + # no binding for yet. Off by default: it widens who may ENTER, so it is a + # deployment's policy to set. Identity still mints only for an account a + # connector has already observed, so the roster keeps deciding who exists. + APP__gears__authenticator__config__idp__provision_on_login: {{ .Values.authenticator.oidc.provisionOnLogin | default false | quote }} {{- with .Values.authenticator.oidc.scopes }} # Requested OIDC scopes (space-delimited; the gear splits on space/comma). # `offline_access` is what makes the IdP issue a refresh token — required for diff --git a/charts/insight/values.yaml b/charts/insight/values.yaml index f8d32cd4c..d1e9fd137 100644 --- a/charts/insight/values.yaml +++ b/charts/insight/values.yaml @@ -384,6 +384,19 @@ authenticator: # id_token claim carrying the IdP's stable external user id for # sourceType (e.g. Entra's "oid"). Defaults to "sub". externalIdClaim: "sub" + # Provision a person on first successful login instead of refusing one the + # journal has no binding for yet. + # + # The login-bootstrap row is otherwise written only by the nightly + # persons-seed, which links a person by e-mail — so a roster member whose + # directory publishes no address (a GitHub member with a hidden e-mail, for + # one) cannot enter until an operator binds them by hand. + # + # Off by default because it widens who may ENTER. It does not widen who + # EXISTS: identity mints only for an account a connector has already + # observed, refuses one the source has closed, refuses one an operator + # excluded, and writes only under the tenant its own journal is keyed by. + provisionOnLogin: false # Honor `/auth/login?__override=` (view-as, #1941): the session is # minted for that person instead of the authenticated one. Dev/demo # environments ONLY — MUST stay false anywhere real users log in. diff --git a/deploy/HELM_DEPLOY.md b/deploy/HELM_DEPLOY.md index 7d8264ddc..08b86f214 100644 --- a/deploy/HELM_DEPLOY.md +++ b/deploy/HELM_DEPLOY.md @@ -253,6 +253,12 @@ authenticator: # for sourceType. Default "sub" is correct when `sub` itself is # that stable id; Entra needs "oid" instead (its `sub` is # pairwise-unique per client, NOT the directory-stable id). + # provisionOnLogin: true # let a roster member enter on first login instead of waiting + # for the nightly persons-seed — which links by e-mail, so a + # member whose directory publishes none never gets a person. + # Note the indentation: this is authenticator.oidc.*, and a + # misplaced key is ignored in silence. See "First login + # provisioning" below before turning it on. # csrfOrigins: ["https://"] # fail-closed by default: if the UI's POST /auth/logout, # /auth/refresh or DELETE /auth/sessions return 403, set this @@ -442,6 +448,21 @@ Other notable (non-placeholder) settings in this file: - Image tags are omitted deliberately. Each subchart renders `image.tag | default .Chart.AppVersion`, so a chart release already carries a tested set of product images. Set `.image.tag` only to pin one service to a different build. - `credentials.deploymentMode: helm` and `credentials.autoGenerate: true` — this enables the "bring your own" credentials path, where the chart keeps a labelless `insight-db-creds` Secret instead of generating random passwords. - `identityResolution.deploy: true` — the chart default; don't flip it off. +- `authenticator.oidc.provisionOnLogin` — **First login provisioning.** Off by + default. A person can sign in only once `persons` holds a `value_type='id'` + row binding their IdP external id to a person, and that row is written by the + nightly persons-seed, which groups accounts **by e-mail**. A roster member + whose directory publishes no address therefore never gets one: they + authenticate at the IdP and are still refused, until an operator binds them by + hand in Manage → Identities. Turning this on lets identity mint the person + during the login itself. It widens who may **enter**, not who **exists** — + identity mints only for an account a connector has already observed, refuses + one the source has closed, refuses one an operator excluded as not-a-person, + and writes only under the tenant its own journal is keyed by — so + `global.tenantDefaultId` is required, and the chart refuses to render without + it rather than leaving the switch on and inert. The minted person carries + the source-native id alone until the next seed run attaches the roster's name + and org placement to it. - `authenticator.tlsDiscovery.issuerRef.name` — the cert-manager `ClusterIssuer` the JWKS-discovery Certificate is issued from. Always set this: the chart ships `local-ca`, which is the self-signed root that `make bootstrap-cert-manager ENV=local` creates for the local k3s sandbox, not anything a real cluster has. - There is no auth-off toggle anywhere in this chart. `authenticator.oidc.issuerUrl` and `authenticator.oidc.redirectUri` are hard `required` fields, so a real IdP is a prerequisite; install Keycloak as a separate release if the stand has none. The bundled `keycloak` subchart is wired for this repo's own environments (roster realm, config-cli-managed content) and not a substitute here. diff --git a/deploy/compose/authenticator-fullauth.yaml b/deploy/compose/authenticator-fullauth.yaml index 214faf153..826ac0dca 100644 --- a/deploy/compose/authenticator-fullauth.yaml +++ b/deploy/compose/authenticator-fullauth.yaml @@ -123,6 +123,12 @@ gears: tenant_claim: "tenant_id" # Fallback tenant for a claim-less IdP (e.g. Okta); empty = fail closed. default_tenant_id: "" + # Mint a person on first login instead of refusing one the journal has + # no binding for yet (the nightly persons-seed links by e-mail, so a + # roster member without one never gets a binding). Off here: the + # compose stand seeds `persons` up front, so nothing needs it — turn it + # on to exercise the path. + provision_on_login: false # Service tokens (§10 G1 / DD-AUTH-05). The token endpoint runs on its own # listener (token_bind_addr) so it never shares the main port. Service # tokens are always tenant-scoped; the caller names the tenant. diff --git a/deploy/gitops/scripts/compose-app-secrets.sh b/deploy/gitops/scripts/compose-app-secrets.sh index 1eb9c1215..08753b9a4 100755 --- a/deploy/gitops/scripts/compose-app-secrets.sh +++ b/deploy/gitops/scripts/compose-app-secrets.sh @@ -148,6 +148,9 @@ AUTH_SOURCE_TYPE=$(yq -r '.authenticator.oidc.sourceType // ""' "$VALUES") # id_token claim carrying the IdP's stable external user id for source_type # (Entra: "oid"; the generic OIDC "sub" is not the same directory-stable id). AUTH_EXTERNAL_ID_CLAIM=$(yq -r '.authenticator.oidc.externalIdClaim // "sub"' "$VALUES") +# Off unless a values file says otherwise: it widens who may enter, and the +# chart-rendered path defaults it the same way. +AUTH_PROVISION_ON_LOGIN=$(yq -r '.authenticator.oidc.provisionOnLogin // false' "$VALUES") # `__override` view-as login (insight#1941/#1944) — dev/demo stands ONLY. AUTH_OVERRIDE_ENABLED=$(yq -r '.authenticator.overrideEnabled // false' "$VALUES") AUTH_EXPERIMENTS_ENABLED=$(yq -r '.authenticator.experimentsEnabled // false' "$VALUES") @@ -280,6 +283,7 @@ stringData: APP__gears__authenticator__config__idp__default_tenant_id: "${AUTH_DEFAULT_TENANT_ID}" APP__gears__authenticator__config__idp__source_type: "${AUTH_SOURCE_TYPE}" APP__gears__authenticator__config__idp__external_id_claim: "${AUTH_EXTERNAL_ID_CLAIM}" + APP__gears__authenticator__config__idp__provision_on_login: "${AUTH_PROVISION_ON_LOGIN}" APP__gears__authenticator__config__redirect_uri: "${AUTH_REDIRECT_URI}" APP__gears__authenticator__config__oidc_scopes: "${AUTH_SCOPES}" APP__gears__authenticator__config__service_tokens__audience: "${AUTH_TOKEN_AUD}" diff --git a/docker-compose.yml b/docker-compose.yml index 1b111c389..c7ebe400a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -320,6 +320,11 @@ services: APP__gears__identity_resolution__config__clickhouse_database: "${CLICKHOUSE_DATABASE:-insight}" APP__gears__identity_resolution__config__clickhouse_user: "${CLICKHOUSE_USER:-insight}" APP__gears__identity_resolution__config__clickhouse_password: "${CLICKHOUSE_PASSWORD:-insight-local}" + # The tenant this journal is keyed by. The seed already passes it per + # invocation; the SERVICE needs it too, because a write that has no + # caller context to infer a tenant from (the login bootstrap) checks the + # asserted one against this rather than trusting it. + APP__gears__identity_resolution__config__tenant_default_id: "${TENANT_DEFAULT_ID:-00000000-df51-5b42-9538-d2b56b7ee953}" volumes: - type: bind source: ./deploy/compose/build/identity-resolution/identity-resolution diff --git a/src/backend/services/authenticator/src/api/handlers.rs b/src/backend/services/authenticator/src/api/handlers.rs index 28efd2478..0ed09d3a6 100644 --- a/src/backend/services/authenticator/src/api/handlers.rs +++ b/src/backend/services/authenticator/src/api/handlers.rs @@ -304,9 +304,17 @@ pub async fn callback( tracing::info!(session_id = %old_sid, "session-fixation guard: revoked presented session"); } - // Resolve the internal person. Unknown -> 403 (first-admin bootstrap / RBAC - // are out of step-04 scope; local dev seeds the persons table). - let resolution = match state.resolver.resolve(&idp.identity).await { + // Identity requires a connector to have observed the principal before it + // mints, so this decides who waits for the batch, not who exists. + let resolved = match state.resolver.resolve(&idp.identity).await { + Ok(Some(p)) => Ok(Some(p)), + Ok(None) if state.cfg.idp.provision_on_login => { + state.resolver.provision(&idp.identity).await + } + other => other, + }; + + let resolution = match resolved { Ok(Some(p)) => p, Ok(None) => { tracing::warn!( diff --git a/src/backend/services/authenticator/src/config.rs b/src/backend/services/authenticator/src/config.rs index c5e0915e1..b8c2e477d 100644 --- a/src/backend/services/authenticator/src/config.rs +++ b/src/backend/services/authenticator/src/config.rs @@ -71,6 +71,10 @@ pub struct IdpConfig { /// to `sub` (fine for IdPs where `sub` IS the stable directory id, e.g. /// Keycloak). pub external_id_claim: String, + // INVARIANT: off by default — it widens who may ENTER, which is a + // deployment's policy to set. Identity refuses to mint for a principal no + // connector has observed, so it never widens who exists. + pub provision_on_login: bool, /// Fallback tenant when the id_token carries no tenant claim at all (e.g. /// Okta). Empty = no fallback: the gateway JWT gets an empty `tenant_id` /// and downstream services fail closed. Interim until the Identity @@ -115,6 +119,7 @@ impl Default for IdpConfig { tenant_claim: "tenant_id".to_owned(), source_type: String::new(), external_id_claim: "sub".to_owned(), + provision_on_login: false, default_tenant_id: String::new(), extra_ca_cert_path: String::new(), hosts: HashMap::new(), diff --git a/src/backend/services/authenticator/src/identity.rs b/src/backend/services/authenticator/src/identity.rs index 755d7264f..02b3cc812 100644 --- a/src/backend/services/authenticator/src/identity.rs +++ b/src/backend/services/authenticator/src/identity.rs @@ -78,6 +78,18 @@ pub trait PersonResolver: Send + Sync { /// # Errors /// Fails when the Identity Service is unreachable or errors. async fn resolve(&self, id: &IdpIdentity) -> anyhow::Result>; + + /// Resolve, minting a person when the journal has no binding yet. + /// `Ok(None)` = still unknown, and the caller denies the login. + /// + /// # Errors + /// Fails when the Identity Service is unreachable or errors. + // INVARIANT: the default refuses, so a resolver without minting power + // fails closed rather than by omission. + async fn provision(&self, id: &IdpIdentity) -> anyhow::Result> { + let _ = id; + Ok(None) + } } /// `PersonResolver` backed by the Identity Service. @@ -111,6 +123,16 @@ struct ResolveProfile { insight_source_id: Option, } +// INVARIANT: only a normal login may provision. The `__override` view-as +// resolves by an email its operator typed, and minting there would turn a typo +// into a person to become. +fn provisionable_external_id(target: &ResolveTarget) -> Option<&str> { + match target { + ResolveTarget::ExternalId(external_id) => Some(external_id), + ResolveTarget::Email(_) => None, + } +} + impl IdentityPersonResolver { /// `base_url` is the Identity Service root, e.g. `http://identity:8082`. /// `keystore` / `issuer` / `audience` are used to mint the service JWT that @@ -206,6 +228,43 @@ impl IdentityPersonResolver { .await } + async fn provision_person_by_external_id( + &self, + external_id: &str, + tenant_id: &str, + ) -> anyhow::Result> { + if self.base_url.is_empty() { + return Ok(None); + } + let url = format!("{}/internal/persons/provision", self.base_url); + let token = self.mint_service_token(tenant_id)?; + let resp = self + .http + .post(&url) + .bearer_auth(token) + .json(&serde_json::json!({ + "source_type": self.source_type, + "external_id": external_id, + "tenant_id": tenant_id, + })) + .send() + .await + .context("Identity provision request")?; + // INVARIANT: only 404 means "no such principal". Folding any other + // status into it would dress a broken deployment up as an ordinary + // access denial, which is the version nobody diagnoses. + let status = resp.status(); + if status == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } + anyhow::ensure!( + status.is_success(), + "Identity returned {status} for /internal/persons/provision" + ); + let profile: ResolveProfile = resp.json().await.context("decode ResolveProfile")?; + Ok(profile.insight_source_id.filter(|id| !id.is_nil())) + } + /// Admin `__override` (view-as) lookup: resolve by email — an operator /// types an email, not an IdP external id. A DISTINCT route from the /// login-bootstrap lookup above (never dispatched from the same call). @@ -243,4 +302,65 @@ impl PersonResolver for IdentityPersonResolver { tenant_id: id.tenant_id.clone(), })) } + + async fn provision(&self, id: &IdpIdentity) -> anyhow::Result> { + let Some(external_id) = provisionable_external_id(&id.resolve_by) else { + return Ok(None); + }; + let person_id = self + .provision_person_by_external_id(external_id, &id.tenant_id) + .await?; + Ok(person_id.map(|person_id| PersonResolution { + person_id: person_id.to_string(), + tenant_id: id.tenant_id.clone(), + })) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A resolver with no minting power at all — the trait default is what a + /// future implementation inherits, so it must refuse rather than forget. + struct LookupOnly; + + #[async_trait] + impl PersonResolver for LookupOnly { + async fn resolve(&self, _id: &IdpIdentity) -> anyhow::Result> { + Ok(None) + } + } + + fn identity(resolve_by: ResolveTarget) -> IdpIdentity { + IdpIdentity { + sub: "subject".to_owned(), + email: "someone@example.com".to_owned(), + tenant_id: Uuid::from_u128(7).to_string(), + resolve_by, + } + } + + #[test] + fn only_a_login_is_provisionable_never_the_view_as_override() { + assert_eq!( + provisionable_external_id(&ResolveTarget::ExternalId("octocat".to_owned())), + Some("octocat"), + ); + assert_eq!( + provisionable_external_id(&ResolveTarget::Email("typo@example.com".to_owned())), + None, + "an operator's typed email must never mint the person it names", + ); + } + + #[tokio::test] + async fn a_resolver_without_minting_power_fails_closed() -> anyhow::Result<()> { + let provisioned = LookupOnly + .provision(&identity(ResolveTarget::ExternalId("octocat".to_owned()))) + .await?; + + assert!(provisioned.is_none()); + Ok(()) + } } diff --git a/src/backend/services/identity-resolution/src/api/handlers.rs b/src/backend/services/identity-resolution/src/api/handlers.rs index 2d981534e..5d381314a 100644 --- a/src/backend/services/identity-resolution/src/api/handlers.rs +++ b/src/backend/services/identity-resolution/src/api/handlers.rs @@ -20,11 +20,12 @@ use super::AppState; use super::canonical_json::CanonicalJson; use super::error::ProfileError; use super::gate::{require_caller, require_service}; +use crate::domain::login_bootstrap; use crate::domain::profile::{ ParentProjection, PersonResponse, ResolveProfileRequest, assemble_person, assemble_profile, latest_values, }; -use crate::infra::db::{persons_repo, subchart_repo}; +use crate::infra::db::{persons_repo, resolution_repo, subchart_repo}; /// `POST /v1/profiles` — resolve one identity (email or source-native id) to a /// person, then assemble the profile. @@ -190,27 +191,190 @@ pub async fn internal_person_by_external_id( .create()); } - let person_id = - persons_repo::resolve_person_id_by_source_any_tenant(&state.db, source_type, external_id) - .await - .map_err(|e| { - tracing::error!(error = %e, "internal by-external-id lookup failed"); - CanonicalError::internal("lookup failed").create() - })? - .ok_or_else(|| { - ProfileError::not_found(format!( - "person with source_type '{source_type}' external_id '{external_id}' not found" - )) - .with_resource(external_id.to_owned()) - .create() - })?; + let person_id = lookup_by_external_id(&state, source_type, external_id) + .await? + .ok_or_else(|| { + ProfileError::not_found(format!( + "person with source_type '{source_type}' external_id '{external_id}' not found" + )) + .with_resource(external_id.to_owned()) + .create() + })?; - Ok(Json(InternalPersonResponse { + Ok(Json(person_response(external_id, person_id))) +} + +/// Body for `POST /internal/persons/provision`. +#[derive(Debug, serde::Deserialize)] +pub struct InternalProvisionRequest { + source_type: String, + external_id: String, + /// The tenant the `id_token` asserted. A read can stay tenant-agnostic; a + /// write cannot, and at login there is no caller context to infer it from. + tenant_id: Uuid, +} + +/// `POST /internal/persons/provision` — SERVICE-ONLY login bootstrap that +/// MINTS a person when the journal has no binding for this IdP principal yet. +/// Same contract and gate as [`internal_person_by_external_id`], and the same +/// response shape, so the caller can treat the two identically. +/// +/// Why this exists: the login-bootstrap row is otherwise written only by the +/// nightly persons-seed, which links a person by e-mail and skips an account +/// that carries none. A member of the IdP's roster with no published address +/// is therefore refused at login until an operator binds them by hand. +/// +/// It mints only for an account a connector has ALREADY OBSERVED, and reuses +/// that observation's `insight_source_id`. Both halves matter: +/// +/// - the roster stays the authority on who exists, so this is "the IdP +/// authenticated someone the org already lists", never "anyone who reaches +/// the IdP becomes a person"; +/// - the persons-seed recognises an account by the whole triple, so a binding +/// written under any other instance id would be invisible to it and the +/// account would stay unbound forever. Matching the observed id is what +/// makes the next batch run ADOPT this person rather than mint a second. +pub async fn internal_provision_person( + Extension(state): Extension>, + Extension(ctx): Extension, + CanonicalJson(req): CanonicalJson, +) -> Result { + require_service(&ctx)?; + + let principal = + login_bootstrap::parse_principal(&req.source_type, &req.external_id, req.tenant_id) + .map_err(|refusal| refused(refusal, &req))?; + let (source_type, external_id) = (principal.source_type, principal.external_id); + // Validated before the lookup, not just before the write: a route that + // answered an existing person for any asserted tenant and refused only a + // new one would fail intermittently under a misconfigured tenant claim, + // which is the shape nobody diagnoses. + let tenant = login_bootstrap::provisioning_tenant( + &state.config.tenant_default_id, + principal.asserted_tenant, + ) + .map_err(|refusal| refused(login_bootstrap::Refusal::Tenant(refusal), &req))?; + + if let Some(person_id) = lookup_by_external_id(&state, source_type, external_id).await? { + return Ok(Json(person_response(external_id, person_id))); + } + + let observed = super::resolution::evidence_reader(&state) + .observed_account(source_type, external_id) + .await + .map_err(|e| { + tracing::error!(error = %e, "login bootstrap: connector evidence lookup failed"); + CanonicalError::internal("lookup failed").create() + })? + .ok_or_else(|| { + ProfileError::not_found(format!( + "no connector has observed source_type '{source_type}' external_id '{external_id}'" + )) + .with_resource(external_id.to_owned()) + .create() + })?; + + let row = login_bootstrap::decide(principal, &observed, tenant, chrono::Utc::now().naive_utc()) + .map_err(|refusal| refused(refusal, &req))?; + + let minted = resolution_repo::append_binding_if_unbound(&state.db, tenant, &row) + .await + .map_err(|e| { + tracing::error!(error = %e, "login bootstrap: binding write failed"); + CanonicalError::internal("provisioning failed").create() + })?; + + // Read what is in force, never what was intended. Two interleavings end up + // here: a racing login wrote first, or an operator decided first — + // including an exclusion, which the lookup hides and which must read as + // "no person to enter as" rather than as a fresh mint. + let person_id = lookup_by_external_id(&state, source_type, external_id) + .await? + .ok_or_else(|| { + tracing::warn!( + target: "audit", + event = "login_bootstrap_refused_decided_account", + source_type, + external_id, + "the account is already decided as not-a-person; no login identity for it" + ); + ProfileError::not_found(format!( + "source_type '{source_type}' external_id '{external_id}' resolves to no person" + )) + .with_resource(external_id.to_owned()) + .create() + })?; + + if minted { + tracing::info!( + target: "audit", + event = "login_bootstrap_person_provisioned", + source_type, + external_id, + person_id = %person_id, + "minted a person for an authenticated principal the roster already lists" + ); + } + + Ok(Json(person_response(external_id, person_id))) +} + +/// Map a domain refusal onto the wire. Each is an answer about the principal, +/// so none of them is a 500. +fn refused(refusal: login_bootstrap::Refusal, req: &InternalProvisionRequest) -> CanonicalError { + use login_bootstrap::{Refusal, TenantRefusal}; + + let resource = req.external_id.trim().to_owned(); + match refusal { + Refusal::Invalid { field, message } => ProfileError::invalid_argument() + .with_field_violation(field, message, "INVALID") + .create(), + Refusal::Tenant(TenantRefusal::Unconfigured) => ProfileError::failed_precondition() + .with_precondition_violation( + "tenant", + "provisioning needs the service's default tenant to be configured", + "tenant_unconfigured", + ) + .create(), + Refusal::Tenant(TenantRefusal::Mismatch) => ProfileError::invalid_argument() + .with_field_violation( + "tenant_id", + "tenant_id is not the tenant this journal is keyed by", + "TENANT_MISMATCH", + ) + .create(), + Refusal::Closed => ProfileError::not_found(format!("'{resource}' is closed at its source")) + .with_resource(resource) + .create(), + Refusal::Addressed => ProfileError::not_found(format!( + "'{resource}' carries an address; identity resolution links it, \ + so there is nothing to bootstrap" + )) + .with_resource(resource) + .create(), + } +} + +async fn lookup_by_external_id( + state: &AppState, + source_type: &str, + external_id: &str, +) -> Result, CanonicalError> { + persons_repo::resolve_person_id_by_source_any_tenant(&state.db, source_type, external_id) + .await + .map_err(|e| { + tracing::error!(error = %e, "internal by-external-id lookup failed"); + CanonicalError::internal("lookup failed").create() + }) +} + +fn person_response(external_id: &str, person_id: Uuid) -> InternalPersonResponse { + InternalPersonResponse { value_type: "id".to_owned(), value: external_id.to_owned(), insight_source_type: "person", insight_source_id: person_id, - })) + } } /// Query params for `GET /internal/persons/by-email-override`. diff --git a/src/backend/services/identity-resolution/src/api/mod.rs b/src/backend/services/identity-resolution/src/api/mod.rs index 726319516..b63ce4e57 100644 --- a/src/backend/services/identity-resolution/src/api/mod.rs +++ b/src/backend/services/identity-resolution/src/api/mod.rs @@ -114,6 +114,10 @@ fn build_operations(router: Router, openapi: &dyn OpenApiRegistry) -> Router { "/internal/persons/by-email-override", axum::routing::get(handlers::internal_person_by_email_override), ); + let router = router.route( + "/internal/persons/provision", + axum::routing::post(handlers::internal_provision_person), + ); let router = OperationBuilder::post("/v1/profiles") .operation_id("identity_resolution.profiles.resolve") diff --git a/src/backend/services/identity-resolution/src/api/resolution.rs b/src/backend/services/identity-resolution/src/api/resolution.rs index cf49b4bcb..8d2114cd3 100644 --- a/src/backend/services/identity-resolution/src/api/resolution.rs +++ b/src/backend/services/identity-resolution/src/api/resolution.rs @@ -928,7 +928,7 @@ async fn candidate_cards( .map_err(|e| internal(&e, "failed to read candidate cards")) } -fn evidence_reader(state: &AppState) -> ClickHouseEvidenceReader { +pub(super) fn evidence_reader(state: &AppState) -> ClickHouseEvidenceReader { ClickHouseEvidenceReader::connect( &state.config.clickhouse_url, &state.config.clickhouse_database, diff --git a/src/backend/services/identity-resolution/src/domain/login_bootstrap.rs b/src/backend/services/identity-resolution/src/domain/login_bootstrap.rs new file mode 100644 index 000000000..e6fc7d4db --- /dev/null +++ b/src/backend/services/identity-resolution/src/domain/login_bootstrap.rs @@ -0,0 +1,379 @@ +use uuid::Uuid; + +use super::resolution::BindingRow; +use super::seed::SourceAccountKey; +use crate::infra::identity_evidence::ObservedAccount; + +/// Column widths a binding lands in (`001_persons.sql`). 320 is also the limit +/// `POST /v1/profiles` states for an id value, so one number is one contract +/// across both entrances. +pub const MAX_VALUE_ID_CHARS: usize = 320; +pub const MAX_SOURCE_TYPE_CHARS: usize = 100; + +/// The journal reason marking a row the login bootstrap wrote. +pub const LOGIN_BOOTSTRAP_REASON: &str = "login-bootstrap"; + +/// Namespace for [`derived_person_id`]. Fixed, and used for nothing else. +const PERSON_NAMESPACE: Uuid = Uuid::from_u128(0x9f2c_6ad1_4e83_4f27_bd51_7c0a_38e9_1b64); + +/// Why a principal may not be provisioned. Each variant is an answer about the +/// principal, not a fault — the caller turns them into refusals. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Refusal { + /// A field is unusable: empty, or longer than the column it lands in. + Invalid { + field: &'static str, + message: String, + }, + /// The token asserts a tenant this journal is not keyed by, or the service + /// cannot say which tenant that is. + Tenant(TenantRefusal), + /// The source has deactivated the account. + Closed, + /// The account carries an address, so identity resolution links it. + Addressed, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum TenantRefusal { + /// The service has no configured tenant, so it cannot know which journal + /// it would be writing into. + Unconfigured, + /// The asserted tenant is not the configured one. + Mismatch, +} + +/// What the caller asked for, already parsed. +#[derive(Debug, Clone, Copy)] +pub struct Principal<'a> { + pub source_type: &'a str, + pub external_id: &'a str, + pub asserted_tenant: Uuid, +} + +/// Trim and bound the principal a request names. +/// +/// Left unbounded, an over-long id is a database error under strict SQL and, +/// under a lax mode, a silently truncated row that neither the write guard nor +/// the read-back can match again — so every attempt appends another unusable +/// row and still refuses the caller. +pub fn parse_principal<'a>( + source_type: &'a str, + external_id: &'a str, + asserted_tenant: Uuid, +) -> Result, Refusal> { + let source_type = source_type.trim(); + let external_id = external_id.trim(); + + let too_long = |field: &'static str, limit: usize| Refusal::Invalid { + field, + message: format!("{field} must be at most {limit} characters"), + }; + let required = |field: &'static str| Refusal::Invalid { + field, + message: format!("{field} must not be empty"), + }; + + if source_type.is_empty() { + return Err(required("source_type")); + } + if external_id.is_empty() { + return Err(required("external_id")); + } + if external_id.chars().count() > MAX_VALUE_ID_CHARS { + return Err(too_long("external_id", MAX_VALUE_ID_CHARS)); + } + if source_type.chars().count() > MAX_SOURCE_TYPE_CHARS { + return Err(too_long("source_type", MAX_SOURCE_TYPE_CHARS)); + } + if asserted_tenant.is_nil() { + return Err(required("tenant_id")); + } + + Ok(Principal { + source_type, + external_id, + asserted_tenant, + }) +} + +/// The tenant a provisioned binding may be written under. +/// +/// A row under any other tenant is invisible to the persons-seed: never +/// adopted, and a second person minted for the same account on its next run. +pub fn provisioning_tenant(configured: &str, asserted: Uuid) -> Result { + let configured = configured.trim(); + if configured.is_empty() { + return Err(TenantRefusal::Unconfigured); + } + let Ok(configured) = Uuid::parse_str(configured) else { + return Err(TenantRefusal::Unconfigured); + }; + if configured != asserted { + return Err(TenantRefusal::Mismatch); + } + Ok(configured) +} + +/// The person a given account provisions to. +/// +/// Derived, not random: the journal's natural key carries `person_id`, so two +/// concurrent logins minting random ids would both insert and split one human +/// across two people. Deriving it makes the racers agree. +pub fn derived_person_id(tenant: Uuid, account: &SourceAccountKey) -> Uuid { + // The unit separator is what keeps the parts from running together: plain + // concatenation lets ("a", "bc") and ("ab", "c") name one person. + let name = format!( + "{tenant}\u{1f}{}\u{1f}{}\u{1f}{}", + account.source_type, account.source_id, account.account_id + ); + Uuid::new_v5(&PERSON_NAMESPACE, name.as_bytes()) +} + +/// The binding to append for this principal, or why it may not be provisioned. +/// +/// Every rule that decides WHO may be minted lives here, over values, so it is +/// answerable without a database or an evidence client. +pub fn decide( + principal: Principal<'_>, + observed: &ObservedAccount, + tenant: Uuid, + now: sea_orm::prelude::DateTime, +) -> Result { + // Gone from its source: the review queue drops such accounts before it + // counts anything, and entering through one would keep a door the roster + // has already shut. + if observed.is_closed { + return Err(Refusal::Closed); + } + + // An account with an address is the BATCH's to resolve, and minting here + // would do harm rather than duplicate work: the seed groups by address, so + // it would have attached this account to whoever already holds that one. A + // person minted first takes the binding, and the seed then reads the group + // as a conflict between two persons with no operator decision to settle it + // — it keeps both, so one human stays split until somebody merges by hand. + if observed.email.is_some() { + return Err(Refusal::Addressed); + } + + let account = SourceAccountKey { + source_type: principal.source_type.to_owned(), + // The instance the evidence names, never one of our choosing: the + // persons-seed matches accounts on the whole triple, so a binding + // under any other id would never be recognised as this account's. + source_id: observed.source_id, + account_id: principal.external_id.to_owned(), + }; + + Ok(BindingRow { + person_id: derived_person_id(tenant, &account), + account, + // Automation, not an operator decision: an operator-authored binding + // settles a contested group (ADR-0003), and this one settles nothing. + author_person_id: Uuid::nil(), + reason: LOGIN_BOOTSTRAP_REASON.to_owned(), + created_at: now, + }) +} + +#[cfg(test)] +mod tests { + use std::error::Error; + + use super::*; + + type R = Result<(), Box>; + + const TENANT: Uuid = Uuid::from_u128(9); + + fn account(account_id: &str) -> SourceAccountKey { + SourceAccountKey { + source_type: "github".to_owned(), + source_id: Uuid::from_u128(0xaa01), + account_id: account_id.to_owned(), + } + } + + fn observed(email: Option<&str>, is_closed: bool) -> ObservedAccount { + ObservedAccount { + source_id: Uuid::from_u128(0xaa01), + is_closed, + email: email.map(str::to_owned), + } + } + + fn now() -> sea_orm::prelude::DateTime { + // A fixed instant: nothing here depends on the clock, and a literal + // keeps the rows a test builds comparable. + chrono::DateTime::UNIX_EPOCH.naive_utc() + } + + #[test] + fn a_principal_is_bounded_by_the_columns_it_lands_in() { + let long_id = "x".repeat(MAX_VALUE_ID_CHARS + 1); + let long_source = "s".repeat(MAX_SOURCE_TYPE_CHARS + 1); + for (case, source_type, external_id, tenant, field) in [ + ("empty source_type", " ", "octocat", TENANT, "source_type"), + ("empty external_id", "github", " ", TENANT, "external_id"), + ( + "over-long external_id", + "github", + long_id.as_str(), + TENANT, + "external_id", + ), + ( + "over-long source_type", + long_source.as_str(), + "octocat", + TENANT, + "source_type", + ), + ("nil tenant", "github", "octocat", Uuid::nil(), "tenant_id"), + ] { + let refused = parse_principal(source_type, external_id, tenant); + match refused { + Err(Refusal::Invalid { field: got, .. }) => { + assert_eq!(got, field, "wrong field named for: {case}"); + } + other => panic!("should refuse {case}, got {other:?}"), + } + } + } + + #[test] + fn a_principal_is_trimmed_not_merely_accepted() -> R { + let principal = parse_principal(" github ", " octocat ", TENANT) + .map_err(|r| format!("a well-formed principal was refused: {r:?}"))?; + + assert_eq!(principal.source_type, "github"); + assert_eq!(principal.external_id, "octocat"); + Ok(()) + } + + #[test] + fn a_write_goes_only_to_the_journals_own_tenant() { + for (case, configured, asserted, expected) in [ + ("matching", TENANT.to_string(), TENANT, Ok(TENANT)), + ( + "another tenant", + TENANT.to_string(), + Uuid::from_u128(10), + Err(TenantRefusal::Mismatch), + ), + ( + "unconfigured", + String::new(), + TENANT, + Err(TenantRefusal::Unconfigured), + ), + ( + "unreadable configuration", + "not-a-uuid".to_owned(), + TENANT, + Err(TenantRefusal::Unconfigured), + ), + ] { + assert_eq!( + provisioning_tenant(&configured, asserted), + expected, + "wrong answer for: {case}" + ); + } + } + + #[test] + fn only_an_account_the_batch_cannot_resolve_is_provisioned() -> R { + let principal = parse_principal("github", "octocat", TENANT) + .map_err(|r| format!("a well-formed principal was refused: {r:?}"))?; + + for (case, evidence, expected) in [ + ( + "closed at its source", + observed(None, true), + Some(Refusal::Closed), + ), + ( + "carries an address", + observed(Some("jane@example.com"), false), + Some(Refusal::Addressed), + ), + ( + "closed AND addressed — closure answers first", + observed(Some("jane@example.com"), true), + Some(Refusal::Closed), + ), + ("no address, still open", observed(None, false), None), + ] { + let decided = decide(principal, &evidence, TENANT, now()); + match (decided, expected) { + (Err(got), Some(want)) => assert_eq!(got, want, "wrong refusal for: {case}"), + (Ok(row), None) => { + assert_eq!(row.reason, LOGIN_BOOTSTRAP_REASON, "case: {case}"); + assert!( + row.author_person_id.is_nil(), + "case: {case} — not an operator" + ); + } + (got, want) => panic!("case {case}: got {got:?}, wanted {want:?}"), + } + } + Ok(()) + } + + #[test] + fn the_binding_carries_the_instance_the_evidence_named() -> R { + let principal = parse_principal("github", "octocat", TENANT) + .map_err(|r| format!("a well-formed principal was refused: {r:?}"))?; + let evidence = ObservedAccount { + source_id: Uuid::from_u128(0xbb02), + is_closed: false, + email: None, + }; + + let row = decide(principal, &evidence, TENANT, now()) + .map_err(|r| format!("should provision, refused: {r:?}"))?; + + // Not an id of our choosing: the persons-seed recognises an account by + // the whole triple, so anything else is never adopted. + assert_eq!(row.account.source_id, Uuid::from_u128(0xbb02)); + Ok(()) + } + + #[test] + fn the_same_account_always_derives_the_same_person() { + assert_eq!( + derived_person_id(TENANT, &account("octocat")), + derived_person_id(TENANT, &account("octocat")), + ); + assert!(!derived_person_id(TENANT, &account("octocat")).is_nil()); + } + + #[test] + fn distinct_accounts_never_derive_one_person() { + let mut seen = std::collections::HashSet::new(); + for (case, tenant, account) in [ + ("baseline", TENANT, account("octocat")), + ("another account", TENANT, account("octocat2")), + ("another tenant", Uuid::from_u128(10), account("octocat")), + // The pair plain concatenation would collide. + ("separator on the left", TENANT, account("a\u{1f}bc")), + ("separator on the right", TENANT, account("ab\u{1f}c")), + ( + "another source", + TENANT, + SourceAccountKey { + source_type: "gitlab".to_owned(), + source_id: Uuid::from_u128(0xaa01), + account_id: "octocat".to_owned(), + }, + ), + ] { + assert!( + seen.insert(derived_person_id(tenant, &account)), + "collided with an earlier case at: {case}" + ); + } + } +} diff --git a/src/backend/services/identity-resolution/src/domain/mod.rs b/src/backend/services/identity-resolution/src/domain/mod.rs index cfa99e681..415e827ed 100644 --- a/src/backend/services/identity-resolution/src/domain/mod.rs +++ b/src/backend/services/identity-resolution/src/domain/mod.rs @@ -1,5 +1,6 @@ //! Domain layer — DTOs and business logic (profile resolution/assembly). +pub mod login_bootstrap; pub mod observation_slot; pub mod person_card; pub mod profile; diff --git a/src/backend/services/identity-resolution/src/infra/db/resolution_repo.rs b/src/backend/services/identity-resolution/src/infra/db/resolution_repo.rs index 94c4b6ab3..895e23a78 100644 --- a/src/backend/services/identity-resolution/src/infra/db/resolution_repo.rs +++ b/src/backend/services/identity-resolution/src/infra/db/resolution_repo.rs @@ -9,7 +9,10 @@ use std::collections::{HashMap, HashSet}; -use sea_orm::{ConnectionTrait, DatabaseConnection, DbBackend, Statement, TransactionTrait, Value}; +use sea_orm::{ + AccessMode, ConnectionTrait, DatabaseConnection, DbBackend, IsolationLevel, Statement, + TransactionTrait, Value, +}; use uuid::Uuid; use crate::domain::resolution::{BINDING_VALUE_TYPE, BindingRow}; @@ -207,6 +210,127 @@ pub async fn person_exists( /// # Errors /// /// Returns an error if any statement fails; the transaction is rolled back. +/// Append one binding ONLY IF the account has none yet, in a single statement. +/// +/// The login bootstrap cannot check-then-write: between the two an operator's +/// exclusion or the seed's own link can land, and because the binding in force +/// is the LATEST row, an automation row written after it would silently +/// override a human's decision. Making "nobody has decided this account" part +/// of the same statement as the insert removes the window — the condition is +/// evaluated against the same snapshot that writes. +/// +/// "Nobody has decided it" is scoped exactly as the login lookup scopes it — +/// by (`source_type`, `value_id`), across every tenant and connector instance. +/// Narrowing the guard to the instance the evidence names would leave the +/// decisions it is meant to protect invisible: an exclusion recorded before a +/// connector was re-registered lives under the OLD instance id, while the +/// lookup that answers "who is in force" ignores the instance entirely — so a +/// narrow guard would write, and the fresh row would win. +/// +/// The inner derived table is not decoration: MariaDB refuses a bare subquery +/// on the INSERT's own target, and materialising it is what makes the +/// self-reference legal. +/// +/// Returns whether the row was written. `false` means somebody else had +/// already decided the account, and the caller must read what they decided +/// rather than assume its own row is in force. +/// +/// # Errors +/// +/// Returns an error if the statement fails. +pub async fn append_binding_if_unbound( + db: &DatabaseConnection, + tenant_id: Uuid, + row: &BindingRow, +) -> anyhow::Result { + const SQL: &str = r" + INSERT INTO persons + (value_type, insight_source_type, insight_source_id, insight_tenant_id, + value_id, value_full_text, value, person_id, author_person_id, reason, + created_at) + SELECT * FROM ( + SELECT 'id' AS c1, ? AS c2, ? AS c3, ? AS c4, ? AS c5, + NULL AS c6, NULL AS c7, ? AS c8, ? AS c9, ? AS c10, ? AS c11 + ) AS incoming + WHERE NOT EXISTS ( + SELECT 1 FROM ( + SELECT 1 FROM persons + WHERE value_type = 'id' + AND insight_source_type = ? + AND value_id = ? + LIMIT 1 + ) AS decided + ) + "; + + // The isolation level is stated, not inherited: the guard's protection rests + // on the self-read seeing the same snapshot the insert writes into, and + // under READ COMMITTED that read does not lock, so an operator decision can + // slip between them. MariaDB defaults to REPEATABLE READ, which is what + // makes `INSERT ... SELECT` take a locking read — a deployment that changed + // the default must not silently weaken this. + let txn = db + .begin_with_config( + Some(IsolationLevel::RepeatableRead), + Some(AccessMode::ReadWrite), + ) + .await?; + + let statement = Statement::from_sql_and_values( + DbBackend::MySql, + SQL, + [ + row.account.source_type.clone().into(), + row.account.source_id.as_bytes().to_vec().into(), + tenant_id.as_bytes().to_vec().into(), + row.account.account_id.clone().into(), + row.person_id.as_bytes().to_vec().into(), + row.author_person_id.as_bytes().to_vec().into(), + row.reason.clone().into(), + row.created_at.into(), + row.account.source_type.clone().into(), + row.account.account_id.clone().into(), + ], + ); + + let result = match txn.execute(statement).await { + Ok(result) => result, + // A lock conflict here means another login is writing THIS account + // right now. "We did not write" is the truthful answer, and the + // caller's read-back then reports whatever the winner decided — + // whereas surfacing it would put a 500 on the login screen for a + // condition the design already handles. + Err(e) if is_lock_conflict(&e) => { + tracing::info!( + source_type = %row.account.source_type, + account_id = %row.account.account_id, + "login bootstrap: another writer holds this account; deferring to it" + ); + let _ = txn.rollback().await; + return Ok(false); + } + Err(e) => return Err(e.into()), + }; + + txn.commit().await?; + + Ok(result.rows_affected() > 0) +} + +/// Whether MariaDB refused the statement because another transaction held the +/// rows: deadlock (1213) or lock-wait timeout (1205). +/// +/// Classified from the message for the same reason `is_missing_relation` does +/// it for ClickHouse — the driver surfaces the server's code in the text and +/// exposes no typed variant for either condition. +fn is_lock_conflict(error: &sea_orm::DbErr) -> bool { + let message = error.to_string(); + message.contains("1213") + || message.contains("1205") + || message.contains("Deadlock found") + || message.contains("Lock wait timeout exceeded") +} + pub async fn append_bindings( db: &DatabaseConnection, tenant_id: Uuid, @@ -394,3 +518,59 @@ pub async fn present_rows( }) .collect()) } + +#[cfg(test)] +mod tests { + use sea_orm::{DbErr, RuntimeErr}; + + use super::*; + + /// A lock conflict is not a fault to surface: it means another login is + /// writing this very account, and the caller's read-back is what resolves + /// it. Misclassifying either code puts a 500 on the login screen instead. + #[test] + fn a_lock_conflict_is_told_apart_from_a_real_failure() { + for (case, message, expected) in [ + ( + "deadlock, by code", + "Execution Error: error returned from database: 1213 (40001): \ + Deadlock found when trying to get lock; try restarting transaction", + true, + ), + ( + "lock-wait timeout, by code", + "Execution Error: error returned from database: 1205 (HY000): \ + Lock wait timeout exceeded; try restarting transaction", + true, + ), + ( + "deadlock, wording only", + "Deadlock found when trying to get lock", + true, + ), + ( + "lock-wait timeout, wording only", + "Lock wait timeout exceeded", + true, + ), + ( + "a duplicate key is NOT a lock conflict", + "error returned from database: 1062 (23000): Duplicate entry for key 'PRIMARY'", + false, + ), + ( + "a syntax error is NOT a lock conflict", + "error returned from database: 1064 (42000): You have an error in your SQL syntax", + false, + ), + ( + "a dead connection is NOT a lock conflict", + "Connection Error: closed", + false, + ), + ] { + let error = DbErr::Exec(RuntimeErr::Internal(message.to_owned())); + assert_eq!(is_lock_conflict(&error), expected, "misclassified: {case}"); + } + } +} diff --git a/src/backend/services/identity-resolution/src/infra/identity_evidence.rs b/src/backend/services/identity-resolution/src/infra/identity_evidence.rs index d3a456274..78fe63320 100644 --- a/src/backend/services/identity-resolution/src/infra/identity_evidence.rs +++ b/src/backend/services/identity-resolution/src/infra/identity_evidence.rs @@ -174,6 +174,54 @@ struct CountRow { hits: u64, } +/// The account as the connectors last described it, for the login bootstrap: +/// which instance carries it, whether the source has closed it, and whether it +/// carries an e-mail. +/// +/// All three are decisions the bootstrap has to make before writing: +/// - the instance id, because the persons-seed matches accounts on the whole +/// triple (`source_type`, `source_id`, `account_id`) and a binding written +/// under any other one would never be recognised as this account's; +/// - closure, because an account gone from its source must not open a door; +/// - the e-mail, because an account that HAS one is the batch's to link — it +/// groups by e-mail, and minting a fresh person for such an account races +/// the link and splits one human across two persons. +/// +/// `GROUP BY` is load-bearing, not tidiness: a bare aggregate returns ONE row +/// over an empty match set, carrying zero values, which would read as a real +/// answer and turn "nothing observed this account" into "observed". +const OBSERVED_ACCOUNT_SQL: &str = r" + SELECT + toString(argMax(insight_source_id, _synced_at)) AS source_id, + argMax(ifNull(operation_type, ''), _synced_at) AS latest_op, + argMaxIf( + ifNull(value, ''), _synced_at, + value_type = 'email' AND operation_type = 'UPSERT' AND value != '' + ) AS email + FROM identity.identity_inputs + WHERE insight_source_type = ? + AND source_account_id = ? + GROUP BY insight_source_type, source_account_id +"; + +#[derive(Debug, Row, Deserialize)] +struct ObservedRow { + source_id: String, + latest_op: String, + email: String, +} + +/// One account as the login bootstrap needs to see it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ObservedAccount { + pub source_id: Uuid, + /// The latest event is a closure signal — deactivated at the source. + pub is_closed: bool, + /// The address the connectors carry for it, if any. `Some` means the + /// persons-seed can link this account by itself. + pub email: Option, +} + fn map_row(row: FoldedRow) -> anyhow::Result { Ok(AccountEvidence { account: SourceAccountKey { @@ -212,6 +260,54 @@ impl ClickHouseEvidenceReader { Err(e) => Err(e.into()), } } + + /// The account as the connectors last described it, or `None` when none + /// has seen it. See [`SOURCE_ID_SQL`] for why the caller needs the + /// instance id rather than one of its own choosing. + /// + /// # Errors + /// + /// Returns an error if the query fails or the stored id is not a UUID. + pub async fn observed_account( + &self, + source_type: &str, + account_id: &str, + ) -> anyhow::Result> { + let row: Result, _> = self + .client + .query(OBSERVED_ACCOUNT_SQL) + .bind(source_type) + .bind(account_id) + .fetch_optional() + .await; + + let found = match row { + Ok(found) => found, + // Same reading as `has_account`: no relation yet is "nothing + // observed", not a failure to answer. + Err(e) if is_missing_relation(&e) => None, + Err(e) => return Err(e.into()), + }; + + // An account nothing has observed yields no row at all. A blank or + // all-zero stored id is not an instance id either: a binding written + // under one would be invisible to the seed's account matching, so it + // reads as "no such account" rather than as an answer. + let Some(row) = found.filter(|r| !r.source_id.trim().is_empty()) else { + return Ok(None); + }; + + let source_id = Uuid::parse_str(row.source_id.trim())?; + if source_id.is_nil() { + return Ok(None); + } + + Ok(Some(ObservedAccount { + source_id, + is_closed: row.latest_op == "DELETE", + email: non_empty(row.email), + })) + } } /// ClickHouse reports an absent table as `UNKNOWN_TABLE` (code 60) — and a diff --git a/tests/stand/api/identity/test_internal.py b/tests/stand/api/identity/test_internal.py index df45b0cc1..fae6919bb 100644 --- a/tests/stand/api/identity/test_internal.py +++ b/tests/stand/api/identity/test_internal.py @@ -8,6 +8,10 @@ the login-bootstrap resolve, scoped to the IdP's `source_type` + its source-native external id (e.g. the Entra `oid` claim). This is what the authenticator actually calls during login. +- `POST /internal/persons/provision` — the login bootstrap's WRITE half: the + same principal, minted when the journal has no binding for it yet and no + connector observation carries an address for it (the batch resolves anything + that does). - `GET /internal/persons/by-email-override?email=...` — the authenticator's admin `__override` (view-as) resolve; never used by login. This is the URL the OLD, now-removed `GET /internal/persons/by-email/{email}` login-bootstrap @@ -259,3 +263,132 @@ def test_by_external_id_never_resolves_by_email( f"an email-shaped external_id resolved (status {response.status_code}) instead of " f"404ing like any other unknown id: {response.text[:300]}" ) + + +# ── POST /internal/persons/provision ──────────────────────────────────────── +# The login bootstrap's write half: same gate and same response shape as the +# resolve above, but it MINTS when the journal has no binding yet. It exists +# because the nightly persons-seed groups accounts by e-mail and skips one +# that carries none, so a roster member whose directory publishes no address +# is refused at login until an operator binds them by hand. +# +# The seeded stand cannot exercise the mint itself — every roster persona is +# seeded WITH an e-mail and therefore already bound, and creating an +# e-mail-less observation would mean writing to the connector evidence, which +# this suite does not do. What it can pin, and what actually carries the risk, +# is the two answers that are not a mint: an already-bound principal must come +# back as THEIR person rather than a second one, and a principal no connector +# has observed must be refused rather than invented. + +PROVISION: str = "/internal/persons/provision" + + +def _provision_body(stand_manifest: Manifest, external_id: str) -> dict[str, str]: + return { + "source_type": stand_manifest.capabilities.idp, + "external_id": external_id, + "tenant_id": stand_manifest.tenant, + } + + +@pytest.mark.requires_service_principal +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.reliability +def test_provision_returns_the_existing_person_rather_than_a_second_one( + service_client: ApiClient, stand_manifest: Manifest +) -> None: + """A principal the journal already knows is looked up, never minted. + + This is the property that keeps provisioning safe to put on the login + path: it runs for EVERY login once enabled, so an already-bound person + must come back unchanged. A fresh id here would mean every sign-in split + its owner into another person. + """ + _, external_id = _dev_lead_login_id(stand_manifest) + person = stand_manifest.fixture("dev_lead") + + first = service_client.post(PROVISION, json_body=_provision_body(stand_manifest, external_id)) + assert first.status_code == 200, f"{first.status_code} {first.text[:300]}" + assert str(first.parse(IdentityValue).insight_source_id) == person.uuid + + # Twice, because idempotence is the whole claim: two tabs, two pods, two + # retries of one login must not produce two people. + second = service_client.post(PROVISION, json_body=_provision_body(stand_manifest, external_id)) + assert second.status_code == 200, f"{second.status_code} {second.text[:300]}" + assert str(second.parse(IdentityValue).insight_source_id) == person.uuid + + +@pytest.mark.requires_service_principal +@pytest.mark.security +def test_provision_refuses_a_principal_no_connector_has_observed( + service_client: ApiClient, stand_manifest: Manifest +) -> None: + """The roster decides who exists; the IdP only decides who may enter. + + Without this the endpoint would mint for anything the IdP authenticates, + which is the difference between "a member of the org may sign in" and + "whoever reaches the IdP becomes a person here". + """ + response = service_client.post( + PROVISION, json_body=_provision_body(stand_manifest, "nobody-has-ever-observed-this") + ) + assert response.status_code == 404, ( + f"an unobserved principal was provisioned (status {response.status_code}) instead of " + f"being refused: {response.text[:300]}" + ) + + +@pytest.mark.requires_service_principal +@pytest.mark.security +def test_provision_refuses_an_over_long_external_id( + service_client: ApiClient, stand_manifest: Manifest +) -> None: + """An id longer than the column it lands in is a bad argument, not a 500 — + and under a lax SQL mode an unchecked one would be stored truncated, so + neither the write guard nor the read-back could match it again and every + login attempt would append another unusable journal row.""" + response = service_client.post( + PROVISION, json_body=_provision_body(stand_manifest, "x" * 400) + ) + assert response.status_code == 400, ( + f"an over-long external_id answered {response.status_code}, not 400: {response.text[:300]}" + ) + + +@pytest.mark.requires_service_principal +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.security +def test_provision_refuses_a_tenant_the_journal_is_not_keyed_by( + service_client: ApiClient, stand_manifest: Manifest +) -> None: + """A row written under another tenant is invisible to the persons-seed: + never adopted, and a second person minted for the same account on the next + run. The caller's asserted tenant is therefore checked, not trusted.""" + _, external_id = _dev_lead_login_id(stand_manifest) + body = _provision_body(stand_manifest, external_id) + body["tenant_id"] = "01900000-0000-7000-8000-00000000dead" + + response = service_client.post(PROVISION, json_body=body) + assert response.status_code == 400, ( + f"a foreign tenant answered {response.status_code}, not 400: {response.text[:300]}" + ) + + +@pytest.mark.requires_service_principal +@pytest.mark.requires_seed("dev_lead") +@pytest.mark.security +def test_provision_refuses_a_person( + lead_session: PersonaSession, stand_manifest: Manifest +) -> None: + """The write half is service-only too. Without this, a 200 for the service + principal above would be equally consistent with the route being open to + anybody authenticated — and this one WRITES.""" + _, external_id = _dev_lead_login_id(stand_manifest) + + response = lead_session.client.post( + identity_path(PROVISION), json_body=_provision_body(stand_manifest, external_id) + ) + assert response.status_code == 403, ( + f"a person reached the service-only write route (status {response.status_code}): " + f"{response.text[:300]}" + )