From dd803f18680519af19fb052ce93046fc66c07704 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sun, 9 Aug 2026 14:32:50 +0300 Subject: [PATCH 1/2] fix: persist self-hosted address provider bindings Persist address provider_id through the self-hosted API and store, validate tenant scope, and return exact readback. Carrier task: c3964d26-5256-4a24-ab5b-24636e261e91 Incident: 690284 Agent: quintilianus --- src/cli/commands/address.self-hosted.test.ts | 55 ++++++++++++++ src/cli/commands/address.ts | 6 +- src/db/addresses.local.ts | 15 ++-- src/db/addresses.remote.ts | 17 +++-- src/lib/health.test.ts | 7 +- ...elf-hosted-response-contracts.generated.ts | 32 ++++++++ src/selfhost.ts | 4 +- src/server/self-hosted/migrations.ts | 10 +++ .../openapi-error-status-parity.test.ts | 2 +- src/server/self-hosted/openapi.ts | 16 +++- src/server/self-hosted/parity.test.ts | 75 ++++++++++++++++++- src/server/self-hosted/service.ts | 10 +++ src/server/self-hosted/store.ts | 36 ++++++++- 13 files changed, 255 insertions(+), 30 deletions(-) diff --git a/src/cli/commands/address.self-hosted.test.ts b/src/cli/commands/address.self-hosted.test.ts index 86fa6027..c6813c9d 100644 --- a/src/cli/commands/address.self-hosted.test.ts +++ b/src/cli/commands/address.self-hosted.test.ts @@ -160,4 +160,59 @@ describe("address CLI — self-hosted (/v1) routing", () => { expect(out).toContain("ops-bot"); expect(data).toMatchObject([{ email: "owned@example.com", owner: { id: ownerId, name: "ops-bot" } }]); }); + + it("persists the requested provider through create and exact filtered readback", async () => { + const providerId = "a6ac055d-df08-4577-acba-b5de1addc732"; + await stub.seed({ + addresses: [{ + id: crypto.randomUUID(), + email: "provider-control@example.com", + provider_id: providerId, + status: "active", + verified: false, + created_at: "2026-01-01T00:00:00.000Z", + updated_at: "2026-01-01T00:00:00.000Z", + }], + }); + + // Positive control: the same provider-filtered probe can find a row that + // already carries provider_id in the server response. + const control = await runAddressCommand(["address", "list", "--provider", providerId]); + expect(control.data).toEqual([ + expect.objectContaining({ email: "provider-control@example.com", provider_id: providerId }), + ]); + + const created = await runAddressCommand([ + "address", + "add", + "packages@example.test", + "--provider", + providerId, + ]); + // Before the fix, the immediate create response overlaid the request field + // locally; this assertion preserves the regression's positive control. + expect(created.data).toMatchObject({ + email: "packages@example.test", + provider_id: providerId, + }); + + const duplicate = await runAddressCommand([ + "address", + "add", + "packages@example.test", + "--provider", + providerId, + ]); + expect(duplicate.data).toMatchObject({ + email: "packages@example.test", + provider_id: providerId, + id: created.data.id, + }); + + const readback = await runAddressCommand(["address", "list", "--provider", providerId]); + expect(readback.data).toEqual(expect.arrayContaining([ + expect.objectContaining({ email: "packages@example.test", provider_id: providerId }), + ])); + expect((await stub.list("addresses")).filter((address) => address.email === "packages@example.test")).toHaveLength(1); + }); }); diff --git a/src/cli/commands/address.ts b/src/cli/commands/address.ts index 5a505807..88b61ee8 100644 --- a/src/cli/commands/address.ts +++ b/src/cli/commands/address.ts @@ -46,9 +46,9 @@ function notImplementedAnywhere(command: string): never { const MAX_OWNER_HISTORY_LIMIT = 100; /** - * Provider label for display. The /v1 address entity carries no provider - * association, so an empty provider_id is reported as `self_hosted` — the - * DomainType value it corresponds to, not a claim about where it is served. + * Provider label for display. Legacy /v1 address rows may omit provider_id, + * so an empty value is reported as `self_hosted` — the DomainType value it + * corresponds to, not a claim about where it is served. */ function providerLabel(address: { provider_name: string | null; provider_id: string }): string { return address.provider_name ?? (address.provider_id || "self_hosted"); diff --git a/src/db/addresses.local.ts b/src/db/addresses.local.ts index 46846e2f..77b61466 100644 --- a/src/db/addresses.local.ts +++ b/src/db/addresses.local.ts @@ -25,8 +25,8 @@ export function selfHostedAddresses(db?: Database): SelfHostedResourceStore | nu /** Map a selfHosted API address entity to the local EmailAddress shape (defaults filled). * The self-hosted /v1/addresses record carries {id, email, domain, display_name, - * status, created_at, updated_at}; provider/owner/quota are not modelled in the - * selfHosted, so they default to null (enrichment then resolves to "-" in the CLI). */ + * status, provider_id, created_at, updated_at}; omitted provider/owner/quota + * fields default to null (enrichment then resolves to "-" in the CLI). */ export function apiToAddress(e: Record): EmailAddress { const str = (v: unknown): string | null => (v == null ? null : String(v)); const updatedAt = str(e["updated_at"]) ?? new Date().toISOString(); @@ -60,10 +60,13 @@ function rowToAddress(row: AddressRow): EmailAddress { export function createAddress(input: CreateAddressInput, db?: Database): EmailAddress { const selfHosted = selfHostedAddresses(db); if (selfHosted) { - const created = apiToAddress(selfHosted.create({ email: input.email, display_name: input.display_name || null })); - // The selfHosted address model does not persist provider_id; carry the caller's - // provider through on the returned entity so the command output is correct. - return { ...created, provider_id: input.provider_id }; + return apiToAddress( + selfHosted.create({ + email: input.email, + display_name: input.display_name || null, + provider_id: input.provider_id, + }), + ); } const d = db || getDatabase(); diff --git a/src/db/addresses.remote.ts b/src/db/addresses.remote.ts index 2f8ef86f..4ae31608 100644 --- a/src/db/addresses.remote.ts +++ b/src/db/addresses.remote.ts @@ -11,9 +11,9 @@ import type { SelfHostedResourceStore } from "./self-hosted-store.js"; // // Every address read/write routes to the operator's `/v1/addresses` API. There // is no local SQLite island. The `/v1` address entity carries -// {id, email, display_name, status, verified, owner_id, administrator_id, -// daily_quota, created_at, updated_at}; provider/quota fields not modelled over -// /v1 default to null and enrich to "-" in the CLI. +// {id, email, display_name, status, verified, provider_id, owner_id, +// administrator_id, daily_quota, created_at, updated_at}; omitted provider_id +// remains empty for legacy rows and enriches to "-" in the CLI. export const ADDRESS_RESOURCE = "addresses"; export function selfHostedAddresses(): SelfHostedResourceStore { @@ -43,12 +43,13 @@ export function apiToAddress(e: Record): EmailAddress { } export function createAddress(input: CreateAddressInput): EmailAddress { - const created = apiToAddress( - selfHostedAddresses().create({ email: input.email, display_name: input.display_name || null }), + return apiToAddress( + selfHostedAddresses().create({ + email: input.email, + display_name: input.display_name || null, + provider_id: input.provider_id, + }), ); - // The self-hosted address model does not persist provider_id; carry the - // caller's provider through on the returned entity so command output is right. - return { ...created, provider_id: input.provider_id }; } export function getAddress(id: string): EmailAddress | null { diff --git a/src/lib/health.test.ts b/src/lib/health.test.ts index d66cb639..cf9d6cc2 100644 --- a/src/lib/health.test.ts +++ b/src/lib/health.test.ts @@ -102,7 +102,7 @@ describe("checkProviderHealth", () => { expect(health.status).toBe("error"); }); - it("counts provider domains from /v1 (addresses have no provider association)", async () => { + it("counts provider domains and addresses from /v1", async () => { // Providers created via /v1 do not persist api keys (secrets are server-side), // so a sandbox provider is used — it is always locally configured. const provider = createProvider({ name: "Sandbox", type: "sandbox" }); @@ -116,8 +116,7 @@ describe("checkProviderHealth", () => { expect(health.domainCount).toBe(2); expect(health.verifiedDomains).toBe(1); - // Addresses are not provider-scoped server-side, so provider health reports 0. - expect(health.addressCount).toBe(0); + expect(health.addressCount).toBe(2); expect(health.verifiedAddresses).toBe(0); // Delivery events are server-side, so the client always reports a 0 bounce rate. expect(health.bounceRate).toBe(0); @@ -157,7 +156,7 @@ describe("checkAllProviders", () => { expect(byName.get("First")).toMatchObject({ domainCount: 1, verifiedDomains: 1, - addressCount: 0, + addressCount: 1, bounceRate: 0, status: "healthy", }); diff --git a/src/lib/self-hosted-response-contracts.generated.ts b/src/lib/self-hosted-response-contracts.generated.ts index 8a541bc4..71917af2 100644 --- a/src/lib/self-hosted-response-contracts.generated.ts +++ b/src/lib/self-hosted-response-contracts.generated.ts @@ -1517,6 +1517,34 @@ export const SELF_HOSTED_RESPONSE_CONTRACTS: readonly SelfHostedResponseContract ] } }, + { + "method": "POST", + "operationId": "createAddress", + "path": "/v1/addresses", + "status": 404, + "schema": { + "type": "object", + "additionalProperties": false, + "properties": { + "error": { + "type": "string", + "enum": [ + "provider not found" + ] + }, + "reason": { + "type": "string", + "enum": [ + "provider_not_found" + ] + } + }, + "required": [ + "error", + "reason" + ] + } + }, { "method": "POST", "operationId": "createAddress", @@ -36328,6 +36356,10 @@ export const SELF_HOSTED_RESPONSE_COMPONENTS: Readonly> "verified": { "type": "boolean" }, + "provider_id": { + "type": "string", + "nullable": true + }, "daily_quota": { "type": "integer", "nullable": true diff --git a/src/selfhost.ts b/src/selfhost.ts index dfbf5dec..ebdc0f3f 100644 --- a/src/selfhost.ts +++ b/src/selfhost.ts @@ -36,7 +36,7 @@ export interface ApiKeyMetadata { "kid": string; "app": string; "agent": string export interface Domain { "id": string; "domain": string; "status": string; "provider"?: string | null; "verified": boolean; "notes"?: string | null; "provisioning_status"?: string; "purchase_provider"?: string | null; "dns_provider"?: string; "send_provider"?: string | null; "cf_zone_id"?: string | null; "registrar"?: string | null; "nameservers_json"?: Array; "mail_from_domain"?: string | null; "last_error"?: string | null; "next_check_at"?: string | null; "created_at": string; "updated_at": string } -export interface Address { "id": string; "email": string; "domain"?: string | null; "display_name"?: string | null; "status": string; "verified"?: boolean; "daily_quota"?: number | null; "owner_id"?: string | null; "administrator_id"?: string | null; "domain_id"?: string | null; "receive_strategy"?: string | null; "forward_to"?: string | null; "routing_rule_id"?: string | null; "provisioning_status"?: string; "last_validated_at"?: string | null; "last_error"?: string | null; "next_check_at"?: string | null; "created_at": string; "updated_at": string } +export interface Address { "id": string; "email": string; "domain"?: string | null; "display_name"?: string | null; "status": string; "verified"?: boolean; "provider_id"?: string | null; "daily_quota"?: number | null; "owner_id"?: string | null; "administrator_id"?: string | null; "domain_id"?: string | null; "receive_strategy"?: string | null; "forward_to"?: string | null; "routing_rule_id"?: string | null; "provisioning_status"?: string; "last_validated_at"?: string | null; "last_error"?: string | null; "next_check_at"?: string | null; "created_at": string; "updated_at": string } export interface SendKey { "id": string; "owner_id": string | null; "prefix": string | null; "label": string | null; "last_used_at": string | null; "revoked_at": string | null; "created_at": string; "updated_at": string } @@ -308,7 +308,7 @@ export class EmailsSelfHostClient { } /** Register an email address (scope emails:write) */ - async createAddress(body: { "email": string; "display_name"?: string | null; "status"?: string }, init?: RequestInit): Promise<{ "address": Address }> { + async createAddress(body: { "email": string; "display_name"?: string | null; "status"?: string; "provider_id"?: string | null }, init?: RequestInit): Promise<{ "address": Address }> { return this.request("POST", `/v1/addresses`, { body, query: undefined, diff --git a/src/server/self-hosted/migrations.ts b/src/server/self-hosted/migrations.ts index 2338c340..b54d3f48 100644 --- a/src/server/self-hosted/migrations.ts +++ b/src/server/self-hosted/migrations.ts @@ -2844,6 +2844,15 @@ const IDP_PRINCIPAL_TENANTS_MULTI_GRANT = defineMigration( `, ); +/** 0025 — persist the provider binding carried by address creation. */ +const ADDRESS_PROVIDER_BINDING = defineMigration( + "0025_address_provider_binding", + ` + ALTER TABLE addresses ADD COLUMN IF NOT EXISTS provider_id TEXT; + CREATE INDEX IF NOT EXISTS addresses_provider_idx ON addresses (provider_id); + `, +); + /** All migrations, in order: api-keys table (auth), the core schema, inbound. */ export function emailsSelfHostedMigrations(): Migration[] { const authMigrations = apiKeyMigrations().map((m) => defineMigration(m.id, m.sql)); @@ -2875,5 +2884,6 @@ export function emailsSelfHostedMigrations(): Migration[] { EVENTS_TYPE_ENUM_CHECK, WEBHOOK_EVENT_IDEMPOTENCY, IDP_PRINCIPAL_TENANTS_MULTI_GRANT, + ADDRESS_PROVIDER_BINDING, ]; } diff --git a/src/server/self-hosted/openapi-error-status-parity.test.ts b/src/server/self-hosted/openapi-error-status-parity.test.ts index fc26f5d6..4b4c2926 100644 --- a/src/server/self-hosted/openapi-error-status-parity.test.ts +++ b/src/server/self-hosted/openapi-error-status-parity.test.ts @@ -82,7 +82,7 @@ const SERVICE_STATUS_MATRIX: ReadonlyArray { }); }); +describe("self-hosted parity: address provider binding over /v1/addresses", () => { + test("create persists provider_id, supports omitted providers, and rejects unknown providers", async () => { + const d = deps(); + const providerId = "provider-a"; + await d.client.one( + "INSERT INTO self_hosted_providers (id, tenant_id, name, type, active) VALUES ($1, $2, $3, $4, $5)", + [providerId, DEFAULT_TENANT_ID, "Provider A", "smtp", true], + ); + + const response = await handleSelfHostedRequest(d, req("POST", "/v1/addresses", { + token: writeToken(), + body: { email: "http-bound@example.com", provider_id: providerId }, + })); + expect(response?.status).toBe(201); + const responseBody = await response?.json() as { address?: { id?: string; provider_id?: string | null } }; + expect(responseBody.address?.provider_id).toBe(providerId); + + const exact = await handleSelfHostedRequest( + d, + req("GET", `/v1/addresses/${responseBody.address?.id}`, { token: writeToken() }), + ); + expect(exact?.status).toBe(200); + expect((await exact?.json()).address.provider_id).toBe(providerId); + + const created = await d.store.createAddress({ + email: "bound@example.com", + provider_id: providerId, + }); + expect(created.provider_id).toBe(providerId); + expect((await d.store.getAddress(created.id))?.provider_id).toBe(providerId); + + const legacy = await d.store.createAddress({ email: "legacy@example.com" }); + expect(legacy.provider_id).toBeNull(); + + await expect( + d.store.createAddress({ email: "unknown@example.com", provider_id: "provider-missing" }), + ).rejects.toBeInstanceOf(AddressProviderNotFoundError); + }); + + test("POST maps an unknown provider to a scoped 404 without changing address policy", async () => { + const d = deps(); + d.store.createAddress = async () => { + throw new AddressProviderNotFoundError(); + }; + + const res = await handleSelfHostedRequest(d, req("POST", "/v1/addresses", { + token: writeToken(), + body: { email: "unknown@example.com", provider_id: "provider-missing" }, + })); + expect(res?.status).toBe(404); + expect(await res?.json()).toEqual({ error: "provider not found", reason: "provider_not_found" }); + }); + + test("POST maps a cross-tenant provider reference to a scoped 404", async () => { + const d = deps(); + d.store.createAddress = async () => { + throw new CrossTenantReferenceError("self_hosted_providers", "provider_id"); + }; + + const res = await handleSelfHostedRequest(d, req("POST", "/v1/addresses", { + token: writeToken(), + body: { email: "wrong-tenant@example.com", provider_id: "provider-other-tenant" }, + })); + expect(res?.status).toBe(404); + expect(await res?.json()).toEqual({ + error: "referenced self_hosted_providers not found", + reason: "cross_tenant_reference", + }); + }); +}); + describe("self-hosted parity: scoped send-key mint/verify routing", () => { test("POST /v1/send-keys/mint returns the one-time token + summary (not read as an id)", async () => { const d = deps(); diff --git a/src/server/self-hosted/service.ts b/src/server/self-hosted/service.ts index b62f5dfb..7bdf15b2 100644 --- a/src/server/self-hosted/service.ts +++ b/src/server/self-hosted/service.ts @@ -13,6 +13,7 @@ import { migrationAcceptsChecksum, type TypedQueryClient, type Migration } from import { checkHealth } from "../../storage-kit/index.js"; import { EmailsSelfHostedStore, + AddressProviderNotFoundError, IdempotencyKeyConflictError, SendIntentDeletionForbiddenError, SendIntentTombstonedError, @@ -998,6 +999,12 @@ export async function handleSelfHostedRequest( status: body.status ? String(body.status) : undefined, verified: typeof body.verified === "boolean" ? body.verified : undefined, daily_quota: quota.provided ? quota.value : undefined, + provider_id: + body.provider_id === undefined + ? undefined + : body.provider_id === null + ? null + : String(body.provider_id), }); return json(201, { address: created }); } @@ -2232,6 +2239,9 @@ export async function handleSelfHostedRequest( // found" for this tenant (no cross-tenant existence is revealed). return json(404, { error: `referenced ${err.column} not found`, reason: "cross_tenant_reference" }); } + if (err instanceof AddressProviderNotFoundError) { + return json(404, { error: "provider not found", reason: "provider_not_found" }); + } if (err instanceof InboundDomainRouteConflictError) { return json(409, { error: "inbound domain route is already claimed", reason: "inbound_route_conflict" }); } diff --git a/src/server/self-hosted/store.ts b/src/server/self-hosted/store.ts index 272cb025..789b0b3e 100644 --- a/src/server/self-hosted/store.ts +++ b/src/server/self-hosted/store.ts @@ -119,6 +119,8 @@ export interface AddressRecord { status: string; verified: boolean; daily_quota: number | null; + /** Provider binding. Nullable for legacy/direct API clients that omit it. */ + provider_id?: string | null; // Ownership (migration 0011). An address is owned by a human OR agent owner and // administered by an agent. Optional so older/fake rows still satisfy the type. owner_id?: string | null; @@ -1524,6 +1526,14 @@ export class CrossTenantReferenceError extends Error { } } +/** Raised when an address create names no provider in the caller's tenant. */ +export class AddressProviderNotFoundError extends Error { + constructor() { + super("address provider not found"); + this.name = "AddressProviderNotFoundError"; + } +} + /** A receive-ready physical domain may be claimed by exactly one tenant. */ export class InboundDomainRouteConflictError extends Error { constructor(public readonly domain: string) { @@ -2236,15 +2246,35 @@ export class TenantScopedStore { status?: string; verified?: boolean; daily_quota?: number | null; + provider_id?: string | null; }): Promise { const id = randomUUID(); const email = input.email.trim().toLowerCase(); const domain = email.includes("@") ? email.slice(email.indexOf("@") + 1) : null; + const providerId = input.provider_id?.trim() || null; + if (providerId) { + await this.assertNotOtherTenant("self_hosted_providers", providerId, "provider_id"); + const provider = await this.client.get<{ id: string }>( + `SELECT id FROM self_hosted_providers WHERE id = $1 AND tenant_id = $2`, + [providerId, this.tenantId], + ); + if (!provider) throw new AddressProviderNotFoundError(); + } return this.client.one( - `INSERT INTO addresses (id, email, domain, display_name, status, verified, daily_quota, tenant_id) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8) + `INSERT INTO addresses (id, email, domain, display_name, status, verified, daily_quota, provider_id, tenant_id) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) RETURNING *`, - [id, email, domain, input.display_name ?? null, input.status ?? "active", input.verified ?? false, input.daily_quota ?? null, this.tenantId], + [ + id, + email, + domain, + input.display_name ?? null, + input.status ?? "active", + input.verified ?? false, + input.daily_quota ?? null, + providerId, + this.tenantId, + ], ); } From 32297d9ca347ffccef4847ef1dcf4a9a89360bb1 Mon Sep 17 00:00:00 2001 From: Andrei Hasna Date: Sun, 9 Aug 2026 15:00:10 +0300 Subject: [PATCH 2/2] fix: preserve requested address provider binding Make self-hosted address lookup and uniqueness provider-aware, and advance the migration ordering gate to 0025. Remediation: PR #217 review cycle 1 Agent: quintilianus --- src/cli/commands/address.self-hosted.test.ts | 23 +++++++++- src/db/addresses.local.ts | 10 +++-- src/db/addresses.remote.test.ts | 17 ++++++++ src/db/addresses.remote.ts | 13 ++++-- src/db/addresses.test.ts | 11 ++--- src/server/self-hosted/inbound.test.ts | 7 +++- src/server/self-hosted/migrations.ts | 18 +++++++- src/server/self-hosted/parity.test.ts | 18 ++++++++ .../self-hosted/postgres.integration.test.ts | 42 +++++++++++++++++++ 9 files changed, 142 insertions(+), 17 deletions(-) diff --git a/src/cli/commands/address.self-hosted.test.ts b/src/cli/commands/address.self-hosted.test.ts index c6813c9d..62a22781 100644 --- a/src/cli/commands/address.self-hosted.test.ts +++ b/src/cli/commands/address.self-hosted.test.ts @@ -209,10 +209,31 @@ describe("address CLI — self-hosted (/v1) routing", () => { id: created.data.id, }); + const alternateProviderId = "0df88cb6-a671-48d6-a01d-04516e2af958"; + const alternate = await runAddressCommand([ + "address", + "add", + "packages@example.test", + "--provider", + alternateProviderId, + ]); + expect(alternate.data).toMatchObject({ + email: "packages@example.test", + provider_id: alternateProviderId, + }); + const readback = await runAddressCommand(["address", "list", "--provider", providerId]); expect(readback.data).toEqual(expect.arrayContaining([ expect.objectContaining({ email: "packages@example.test", provider_id: providerId }), ])); - expect((await stub.list("addresses")).filter((address) => address.email === "packages@example.test")).toHaveLength(1); + const alternateReadback = await runAddressCommand(["address", "list", "--provider", alternateProviderId]); + expect(alternateReadback.data).toEqual([ + expect.objectContaining({ email: "packages@example.test", provider_id: alternateProviderId }), + ]); + const stored = (await stub.list("addresses")).filter((address) => address.email === "packages@example.test"); + expect(stored).toHaveLength(2); + expect(stored.map((address) => address.provider_id).sort()).toEqual( + [providerId, alternateProviderId].sort(), + ); }); }); diff --git a/src/db/addresses.local.ts b/src/db/addresses.local.ts index 77b61466..ac837609 100644 --- a/src/db/addresses.local.ts +++ b/src/db/addresses.local.ts @@ -97,10 +97,14 @@ export function getAddress(id: string, db?: Database): EmailAddress | null { export function getAddressByEmail(provider_id: string, email: string, db?: Database): EmailAddress | null { const selfHosted = selfHostedAddresses(db); if (selfHosted) { - // The selfHosted model keys addresses by email (no provider dimension). Match on - // email so `address add` dedup, get, and remove all resolve the same record. + // Preserve the explicit provider binding when this compatibility route uses + // the self-hosted API; a same-email row from another provider is not a match. + const provider = provider_id.trim(); const target = email.trim().toLowerCase(); - const found = selfHosted.list().map(apiToAddress).find((a) => a.email.trim().toLowerCase() === target); + const found = selfHosted + .list() + .map(apiToAddress) + .find((a) => a.provider_id === provider && a.email.trim().toLowerCase() === target); return found ?? null; } const d = db || getDatabase(); diff --git a/src/db/addresses.remote.test.ts b/src/db/addresses.remote.test.ts index 4ec38d86..37530831 100644 --- a/src/db/addresses.remote.test.ts +++ b/src/db/addresses.remote.test.ts @@ -146,6 +146,23 @@ describe("address lookups that must see the whole table", () => { expect(found!.id).toBe("addr-000519"); }); + it("getAddressByEmail preserves the explicitly requested provider binding", async () => { + await stub.seed({ + addresses: addressRows(2, (i) => ({ + email: "shared@example.com", + provider_id: i === 0 ? "provider-one" : "provider-two", + })), + }); + + const found = getAddressByEmail("provider-two", "shared@example.com"); + expect(found).not.toBeNull(); + expect(found).toMatchObject({ + id: "addr-000001", + email: "shared@example.com", + provider_id: "provider-two", + }); + }); + it("findAddressesByEmail returns matches on both sides of the page cap", async () => { await stub.seed({ addresses: addressRows(520, (i) => (i === 10 || i === 519 ? { email: "dup@example.com" } : {})), diff --git a/src/db/addresses.remote.ts b/src/db/addresses.remote.ts index 4ae31608..705efe18 100644 --- a/src/db/addresses.remote.ts +++ b/src/db/addresses.remote.ts @@ -91,15 +91,20 @@ function readAddresses(bound: number | null, keep?: (address: EmailAddress) => b const byNewestFirst = (a: EmailAddress, b: EmailAddress): number => (b.created_at ?? "").localeCompare(a.created_at ?? ""); -export function getAddressByEmail(_provider_id: string, email: string): EmailAddress | null { - // The self-hosted model keys addresses by email (no provider dimension). Match - // on email so `address add` dedup, get, and remove all resolve the same record. +export function getAddressByEmail(provider_id: string, email: string): EmailAddress | null { + // Address identity includes the explicit provider binding, matching the local + // store's `(provider_id, email)` lookup. Returning a same-email row from another + // provider would make `address add --provider` silently substitute its binding. // // Bounded to ONE row: existence is the question. On a table the pager cannot // finish, this REFUSES rather than returning null — this null is what // `address add` dedupes against, and a false null mints a duplicate address. + const provider = provider_id.trim(); const target = email.trim().toLowerCase(); - const rows = readAddresses(1, (a) => a.email.trim().toLowerCase() === target); + const rows = readAddresses( + 1, + (a) => a.provider_id === provider && a.email.trim().toLowerCase() === target, + ); return rows[0] ?? null; } diff --git a/src/db/addresses.test.ts b/src/db/addresses.test.ts index 92296978..7cda44a6 100644 --- a/src/db/addresses.test.ts +++ b/src/db/addresses.test.ts @@ -5,11 +5,9 @@ // // Migrated from the deleted local-SQLite pattern. Notes on behavior that changed // with the self-hosted model: -// - The /v1 address entity does NOT persist provider_id (the operator model -// keys addresses by email, not by a local provider row). createAddress carries -// the caller's provider through on the RETURNED entity, but stored rows have no -// provider dimension. Every test that exercises provider filtering therefore -// SEEDS rows with an explicit provider_id instead of relying on createAddress. +// - The /v1 address entity persists provider_id and keys address creation by +// provider plus email. Tests that exercise larger provider-filtered datasets +// still seed rows directly so their ordering and population remain explicit. // - Ordering-sensitive tests seed explicit created_at (create sets created_at≈now // so freshly-created rows tie). // - Readiness keys off verified + not-suspended (the rich local DKIM/SPF/domain @@ -86,7 +84,6 @@ describe("createAddress", () => { const a = createAddress({ provider_id: PROVIDER, email: "test@example.com" }); expect(a.id).toHaveLength(36); expect(a.email).toBe("test@example.com"); - // provider_id is carried through on the returned entity (not persisted over /v1). expect(a.provider_id).toBe(PROVIDER); expect(a.verified).toBe(false); expect(a.display_name).toBeNull(); @@ -111,7 +108,7 @@ describe("getAddress", () => { }); describe("getAddressByEmail", () => { - it("finds address by email (provider is not part of the self-hosted identity)", () => { + it("finds an address by provider and email", () => { const a = createAddress({ provider_id: PROVIDER, email: "test@example.com" }); const found = getAddressByEmail(PROVIDER, "test@example.com"); expect(found?.id).toBe(a.id); diff --git a/src/server/self-hosted/inbound.test.ts b/src/server/self-hosted/inbound.test.ts index 3e373828..36d449f2 100644 --- a/src/server/self-hosted/inbound.test.ts +++ b/src/server/self-hosted/inbound.test.ts @@ -215,7 +215,12 @@ describe("Emails self-hosted inbound messages", () => { expect(ids).toContain("0021_idp_principal_tenants"); expect(ids).toContain("0022_events_type_enum_check"); expect(ids).toContain("0023_webhook_event_idempotency"); - expect(ids.at(-1)).toBe("0024_idp_principal_tenants_multi_grant"); + expect(ids).toContain("0024_idp_principal_tenants_multi_grant"); + expect(ids).toContain("0025_address_provider_binding"); + expect(ids.indexOf("0025_address_provider_binding")).toBeGreaterThan( + ids.indexOf("0024_idp_principal_tenants_multi_grant"), + ); + expect(ids.at(-1)).toBe("0025_address_provider_binding"); }); test("POST inbound preserves all fields and returns 201", async () => { diff --git a/src/server/self-hosted/migrations.ts b/src/server/self-hosted/migrations.ts index b54d3f48..f742e5f6 100644 --- a/src/server/self-hosted/migrations.ts +++ b/src/server/self-hosted/migrations.ts @@ -2844,11 +2844,27 @@ const IDP_PRINCIPAL_TENANTS_MULTI_GRANT = defineMigration( `, ); -/** 0025 — persist the provider binding carried by address creation. */ +/** + * 0025 — persist the provider binding carried by address creation. + * + * Before provider_id became part of the /v1 address contract, 0012 deliberately + * keyed addresses by (tenant_id, email). The client has always keyed creation by + * (provider_id, email), though, so retaining that index would reject the second + * explicit provider binding after the lookup correctly stops substituting the + * first one. Bound rows therefore use the provider-aware tenant key; legacy + * unbound rows retain one email per tenant. + */ const ADDRESS_PROVIDER_BINDING = defineMigration( "0025_address_provider_binding", ` ALTER TABLE addresses ADD COLUMN IF NOT EXISTS provider_id TEXT; + DROP INDEX IF EXISTS addresses_tenant_email_uidx; + CREATE UNIQUE INDEX IF NOT EXISTS addresses_tenant_provider_email_uidx + ON addresses (tenant_id, provider_id, email) + WHERE provider_id IS NOT NULL; + CREATE UNIQUE INDEX IF NOT EXISTS addresses_tenant_unbound_email_uidx + ON addresses (tenant_id, email) + WHERE provider_id IS NULL; CREATE INDEX IF NOT EXISTS addresses_provider_idx ON addresses (provider_id); `, ); diff --git a/src/server/self-hosted/parity.test.ts b/src/server/self-hosted/parity.test.ts index 9387bd61..10dff49d 100644 --- a/src/server/self-hosted/parity.test.ts +++ b/src/server/self-hosted/parity.test.ts @@ -148,6 +148,24 @@ describe("self-hosted parity: new migrations", () => { ); }); + test("0025 appends provider-aware address uniqueness after 0024", () => { + const list = emailsSelfHostedMigrations(); + const ids = list.map((migration) => migration.id); + expect(ids.at(-1)).toBe("0025_address_provider_binding"); + expect(ids.indexOf("0025_address_provider_binding")).toBeGreaterThan( + ids.indexOf("0024_idp_principal_tenants_multi_grant"), + ); + + const sql = list.find((migration) => migration.id === "0025_address_provider_binding")!.sql; + expect(sql).toContain("DROP INDEX IF EXISTS addresses_tenant_email_uidx"); + expect(sql).toContain("addresses_tenant_provider_email_uidx"); + expect(sql).toContain("ON addresses (tenant_id, provider_id, email)"); + expect(sql).toContain("WHERE provider_id IS NOT NULL"); + expect(sql).toContain("addresses_tenant_unbound_email_uidx"); + expect(sql).toContain("ON addresses (tenant_id, email)"); + expect(sql).toContain("WHERE provider_id IS NULL"); + }); + test("0009 seeds the three email agent settings rows and 0010 adds provisioning columns", () => { const m = Object.fromEntries(emailsSelfHostedMigrations().map((x) => [x.id, x.sql])); const parity = m["0009_emails_selfhosted_parity_tables"]!; diff --git a/src/server/self-hosted/postgres.integration.test.ts b/src/server/self-hosted/postgres.integration.test.ts index 4c7990b1..bc198af5 100644 --- a/src/server/self-hosted/postgres.integration.test.ts +++ b/src/server/self-hosted/postgres.integration.test.ts @@ -234,6 +234,48 @@ describe("self-hosted Postgres integration", () => { expect((await store.completeSendIntent(first.record.id, "provider-ci")).send_state).toBe("sent"); }); + it.skipIf(!client)("0025 preserves provider-scoped address identity with tenant isolation", async () => { + await resetPublicSchema(); + await new MigrationLedger(client!, emailsSelfHostedMigrations()).migrate(); + + const tenantB = "20202020-2020-4020-8020-202020202020"; + await client!.execute( + `INSERT INTO tenants (id, slug, name) VALUES ($1, 'address-provider-b', 'Address Provider B')`, + [tenantB], + ); + await client!.execute( + `INSERT INTO self_hosted_providers (id, tenant_id, name, type, active) VALUES + ('provider-a-one', $1, 'Provider A One', 'smtp', true), + ('provider-a-two', $1, 'Provider A Two', 'smtp', true), + ('provider-b-one', $2, 'Provider B One', 'smtp', true)`, + [DEFAULT_TENANT_ID, tenantB], + ); + + const root = new EmailsSelfHostedStore(client!); + const tenantAStore = root.forTenant(DEFAULT_TENANT_ID); + const tenantBStore = root.forTenant(tenantB); + const email = "shared-provider@example.test"; + + const aOne = await tenantAStore.createAddress({ email, provider_id: "provider-a-one" }); + const aTwo = await tenantAStore.createAddress({ email, provider_id: "provider-a-two" }); + const bOne = await tenantBStore.createAddress({ email, provider_id: "provider-b-one" }); + + expect(aOne.provider_id).toBe("provider-a-one"); + expect(aTwo.provider_id).toBe("provider-a-two"); + expect(bOne.provider_id).toBe("provider-b-one"); + expect(aOne.id).not.toBe(aTwo.id); + expect((await tenantAStore.getAddress(aTwo.id))?.provider_id).toBe("provider-a-two"); + expect(await tenantAStore.getAddress(bOne.id)).toBeNull(); + expect(await tenantBStore.getAddress(aOne.id)).toBeNull(); + await expect( + tenantAStore.createAddress({ email, provider_id: "provider-a-two" }), + ).rejects.toThrow(); + + expect(await indexExists("addresses_tenant_email_uidx")).toBe(false); + expect(await indexExists("addresses_tenant_provider_email_uidx")).toBe(true); + expect(await indexExists("addresses_tenant_unbound_email_uidx")).toBe(true); + }); + it.skipIf(!client)("0022 rejects new non-enum event types without deleting legacy poison", async () => { await resetPublicSchema(); const migrations = emailsSelfHostedMigrations();