diff --git a/src/lib/integrations/__tests__/admin-alert.test.ts b/src/lib/integrations/__tests__/admin-alert.test.ts index b45552553..36354eb21 100644 --- a/src/lib/integrations/__tests__/admin-alert.test.ts +++ b/src/lib/integrations/__tests__/admin-alert.test.ts @@ -8,10 +8,51 @@ * * The formatter is a pure function so we can test deterministically * without touching Prisma or the dispatcher. + * + * v1.4.27 — additionally asserts `parkIntegrationAtReauth` semantics: + * the helper writes the row but neither increments the failure counter + * nor pages admins through the dispatcher. The two scope-skip + * call-sites in `withings/sync-{activity,sleep}.ts` must use this + * helper rather than `recordSyncFailure`. */ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi, beforeEach } from "vitest"; -import { formatAdminAlertPayload } from "../status"; +vi.mock("@/lib/db", () => ({ + prisma: { + integrationStatus: { + findUnique: vi.fn(), + upsert: vi.fn(), + update: vi.fn(), + }, + user: { + findUnique: vi.fn(), + findMany: vi.fn(), + }, + }, +})); + +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/notifications/dispatcher", () => ({ + dispatchNotification: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/crypto", () => ({ + encrypt: (s: string) => `enc(${s})`, + decrypt: (s: string) => + s.startsWith("enc(") && s.endsWith(")") ? s.slice(4, -1) : s, +})); + +import { formatAdminAlertPayload, parkIntegrationAtReauth } from "../status"; +import { prisma } from "@/lib/db"; +import { auditLog } from "@/lib/auth/audit"; +import { dispatchNotification } from "@/lib/notifications/dispatcher"; + +beforeEach(() => { + vi.resetAllMocks(); +}); describe("formatAdminAlertPayload — Withings re-auth", () => { it("uses the integration display name (Withings, not 'withings')", () => { @@ -148,3 +189,109 @@ describe("formatAdminAlertPayload — omits errorCode parens when undefined", () expect(out.message).toContain("transient error — network reset"); }); }); + +describe("parkIntegrationAtReauth — silent scope-skip park (v1.4.27 F20)", () => { + it("sets state=error_reauth without incrementing consecutiveFailures and without paging admins", async () => { + // Pre-existing row at counter=87 — the maintainer's reported + // surviving-deploy state. The first scope-skip after the v1.4.27 + // deploy must NOT push the row to 88 and must NOT trigger the + // 3-strike alert ladder. + vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce({ + state: "error_transient", + lastError: "enc(old)", + } as never); + vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce( + {} as never, + ); + + await parkIntegrationAtReauth({ + userId: "u1", + integration: "withings", + message: "Withings connection is missing the user.activity scope.", + errorCode: "scope_missing", + }); + + const upsertArgs = vi.mocked(prisma.integrationStatus.upsert).mock + .calls[0][0]; + expect(upsertArgs.update).toMatchObject({ + state: "error_reauth", + lastError: "enc(Withings connection is missing the user.activity scope.)", + }); + // The whole point of the helper: counter is not in the update set. + expect(upsertArgs.update).not.toHaveProperty("consecutiveFailures"); + + // No admin alert fired. + expect(dispatchNotification).not.toHaveBeenCalled(); + + // A standalone reauth audit row IS written so the ops trail still + // shows the park event — but it lands on + // `integrations.reauth_required`, not `integrations.sync.failed`. + expect(auditLog).toHaveBeenCalledWith( + "integrations.reauth_required", + expect.objectContaining({ + userId: "u1", + details: expect.objectContaining({ + integration: "withings", + errorCode: "scope_missing", + source: "scope_skip", + }), + }), + ); + expect(auditLog).not.toHaveBeenCalledWith( + "integrations.sync.failed", + expect.anything(), + ); + }); + + it("is idempotent on the audit log — a second call for the same scope-skip writes no extra audit row", async () => { + // First call: row already parked with the same lastError. The + // helper detects "no fresh park" and skips the audit write. + vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce({ + state: "error_reauth", + lastError: "enc(Withings connection is missing the user.activity scope.)", + } as never); + vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce( + {} as never, + ); + + await parkIntegrationAtReauth({ + userId: "u1", + integration: "withings", + message: "Withings connection is missing the user.activity scope.", + errorCode: "scope_missing", + }); + + // Upsert still runs (it's the "set lastAttemptAt" touch), but the + // audit log is unchanged. + expect(prisma.integrationStatus.upsert).toHaveBeenCalledOnce(); + expect(auditLog).not.toHaveBeenCalled(); + expect(dispatchNotification).not.toHaveBeenCalled(); + }); + + it("creates a fresh row at counter=0 when no row exists yet", async () => { + // First-ever sync attempt on a legacy connection: no row in the + // table. Helper must create the row at counter=0 so a later + // genuine transient burst still has the full 3-strike runway. + vi.mocked(prisma.integrationStatus.findUnique).mockResolvedValueOnce(null); + vi.mocked(prisma.integrationStatus.upsert).mockResolvedValueOnce( + {} as never, + ); + + await parkIntegrationAtReauth({ + userId: "u-new", + integration: "withings", + message: "scope missing", + errorCode: "scope_missing", + }); + + const upsertArgs = vi.mocked(prisma.integrationStatus.upsert).mock + .calls[0][0]; + expect(upsertArgs.create).toMatchObject({ + userId: "u-new", + integration: "withings", + state: "error_reauth", + consecutiveFailures: 0, + }); + expect(dispatchNotification).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/integrations/status.ts b/src/lib/integrations/status.ts index cd497a3f7..537c770ea 100644 --- a/src/lib/integrations/status.ts +++ b/src/lib/integrations/status.ts @@ -270,6 +270,80 @@ export async function recordSyncFailure( } } +/** + * Park a connection at `error_reauth` from a deliberate scope-skip + * short-circuit. Unlike `recordSyncFailure`, this helper: + * + * 1. does NOT increment `consecutiveFailures`. + * 2. does NOT write an `integrations.sync.failed` audit row through + * `recordSyncFailure`. A standalone `integrations.reauth_required` + * row is written instead so the operations trail still shows the + * park event. + * 3. does NOT enter the 3-strike alert ladder — no admin Telegram + * page fires. + * + * Idempotent: a second call for the same scope-skip leaves the row at + * `error_reauth` with the same encrypted message and the same counter + * value (no increment). Use it from sync routines that have detected a + * deliberate, structural scope gap (e.g. legacy Withings connection + * missing `user.activity`). The defence-in-depth catch-block path + * stays on `recordSyncFailure` because a 403 reaching the catch is + * genuinely unexpected once the scope-skip lands. + * + * The audit-row-once semantics mean a row is only written if the row + * is not already parked at `error_reauth` with the same `lastError`. + * Re-parking the same scope-skip is a no-op for the audit log. + */ +export async function parkIntegrationAtReauth(opts: { + userId: string; + integration: IntegrationKey; + message: string; + errorCode: string; +}): Promise { + const { userId, integration, message, errorCode } = opts; + const now = new Date(); + const encryptedError = safeEncryptError(message); + + // Idempotency probe: re-parking the same scope-skip should NOT emit + // another audit row. We read the current row first; only write the + // audit log when the state or error changes. + const existing = await prisma.integrationStatus.findUnique({ + where: { userId_integration: { userId, integration } }, + select: { state: true, lastError: true }, + }); + const isFreshPark = + existing?.state !== "error_reauth" || existing.lastError !== encryptedError; + + await prisma.integrationStatus.upsert({ + where: { userId_integration: { userId, integration } }, + create: { + userId, + integration, + state: "error_reauth", + lastError: encryptedError, + lastAttemptAt: now, + // First-ever row for this (user, integration) — counter stays at + // 0 so a later genuine transient burst still has the full + // 3-strike runway before paging. + consecutiveFailures: 0, + }, + update: { + state: "error_reauth", + lastError: encryptedError, + lastAttemptAt: now, + // Deliberately omit `consecutiveFailures` — the existing value + // is preserved exactly. This is the whole point of the helper. + }, + }); + + if (isFreshPark) { + await auditLog("integrations.reauth_required", { + userId, + details: { integration, message, errorCode, source: "scope_skip" }, + }); + } +} + /** * Mark a connection as needing re-auth without recording a fresh * "attempt". Used by the OAuth/refresh-token flows when they detect diff --git a/src/lib/withings/__tests__/sync-activity.test.ts b/src/lib/withings/__tests__/sync-activity.test.ts index 691de676f..38c5c8a9a 100644 --- a/src/lib/withings/__tests__/sync-activity.test.ts +++ b/src/lib/withings/__tests__/sync-activity.test.ts @@ -22,6 +22,7 @@ vi.mock("@/lib/db", () => ({ vi.mock("@/lib/integrations/status", () => ({ isReauthRequired: vi.fn().mockResolvedValue(false), + parkIntegrationAtReauth: vi.fn(), recordSyncFailure: vi.fn(), recordSyncSuccess: vi.fn(), })); @@ -45,7 +46,11 @@ vi.mock("@/lib/logging/context", () => ({ })); import { prisma } from "@/lib/db"; -import { recordSyncFailure, recordSyncSuccess } from "@/lib/integrations/status"; +import { + parkIntegrationAtReauth, + recordSyncFailure, + recordSyncSuccess, +} from "@/lib/integrations/status"; import { fetchWithingsActivity, syncUserActivity } from "../sync-activity"; @@ -284,7 +289,12 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => { expect(recordSyncSuccess).not.toHaveBeenCalled(); }); - it("parks the connection at error_reauth with kind=reauth_required + errorCode=scope_missing", async () => { + it("parks the connection via parkIntegrationAtReauth (NOT recordSyncFailure) — v1.4.27 F20", async () => { + // The deliberate scope-skip is a no-op park, not a failure burst. + // Calling `recordSyncFailure` here would increment the per-user + // counter and trip the 3-strike admin alert ladder. The v1.4.27 + // fix swaps to `parkIntegrationAtReauth`, which leaves the counter + // untouched and never pages admins. vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: "user.metrics", } as never); @@ -292,18 +302,20 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => { await syncUserActivity("user-1"); - expect(recordSyncFailure).toHaveBeenCalledTimes(1); - expect(recordSyncFailure).toHaveBeenCalledWith( + expect(parkIntegrationAtReauth).toHaveBeenCalledTimes(1); + expect(parkIntegrationAtReauth).toHaveBeenCalledWith( expect.objectContaining({ userId: "user-1", integration: "withings", - kind: "reauth_required", errorCode: "scope_missing", }), ); + // CRITICAL: the failure path is NOT touched on a scope-skip — that + // is what silences the false 3-strike Telegram alert. + expect(recordSyncFailure).not.toHaveBeenCalled(); }); - it("treats a null scope (pre-v1.4.25 connection) as missing user.activity", async () => { + it("treats a null scope (pre-v1.4.25 connection) as missing user.activity and parks silently", async () => { vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: null, } as never); @@ -314,18 +326,19 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => { expect(imported).toBe(0); expect(fetchMock).not.toHaveBeenCalled(); - expect(recordSyncFailure).toHaveBeenCalledWith( - expect.objectContaining({ kind: "reauth_required" }), + expect(parkIntegrationAtReauth).toHaveBeenCalledWith( + expect.objectContaining({ errorCode: "scope_missing" }), ); + expect(recordSyncFailure).not.toHaveBeenCalled(); }); - it("classifies a Withings 403 in the catch-block as reauth_required (defence-in-depth)", async () => { - // The scope-skip above is the primary guard. This test covers the - // race where Withings revokes scope between OAuth and the next - // sync without invalidating the refresh token — `getValidToken` - // succeeds, the connection scope still claims `user.activity`, - // but the resource call 403s. Must park at reauth_required, not - // transient, so pg-boss stops retrying. + it("classifies a Withings 403 in the catch-block as reauth_required and STILL pages (defence-in-depth)", async () => { + // BL-P3-2 — the defence-in-depth 403 catch-block stays on + // `recordSyncFailure` so a genuinely unexpected 403 (Withings + // revokes scope between OAuth and the next sync without + // invalidating the refresh token) still trips the 3-strike alert. + // This is the asymmetric pair to the scope-skip park above: the + // park is silent, the catch is loud. vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: "user.metrics,user.activity", } as never); @@ -346,5 +359,7 @@ describe("syncUserActivity — scope-skip guard (v1.4.26)", () => { errorCode: "403", }), ); + // The park helper is NOT used for the catch — only the scope-skip. + expect(parkIntegrationAtReauth).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/withings/__tests__/sync-sleep.test.ts b/src/lib/withings/__tests__/sync-sleep.test.ts index 1ec9d1ee0..e79c9cfa0 100644 --- a/src/lib/withings/__tests__/sync-sleep.test.ts +++ b/src/lib/withings/__tests__/sync-sleep.test.ts @@ -21,6 +21,7 @@ vi.mock("@/lib/db", () => ({ vi.mock("@/lib/integrations/status", () => ({ isReauthRequired: vi.fn().mockResolvedValue(false), + parkIntegrationAtReauth: vi.fn(), recordSyncFailure: vi.fn(), recordSyncSuccess: vi.fn(), })); @@ -44,7 +45,11 @@ vi.mock("@/lib/logging/context", () => ({ })); import { prisma } from "@/lib/db"; -import { recordSyncFailure, recordSyncSuccess } from "@/lib/integrations/status"; +import { + parkIntegrationAtReauth, + recordSyncFailure, + recordSyncSuccess, +} from "@/lib/integrations/status"; import { fetchWithingsSleep, @@ -279,7 +284,11 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => { expect(recordSyncSuccess).not.toHaveBeenCalled(); }); - it("parks the connection at error_reauth with kind=reauth_required + errorCode=scope_missing", async () => { + it("parks the connection via parkIntegrationAtReauth (NOT recordSyncFailure) — v1.4.27 F20 + BL-P3-2 parity", async () => { + // BL-P3-2 — sleep mirrors activity. The deliberate scope-skip is a + // no-op park, not a failure burst; calling `recordSyncFailure` + // here would increment the counter and could trip the 3-strike + // admin alert ladder. Swap to `parkIntegrationAtReauth`. vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: "user.metrics", } as never); @@ -287,18 +296,18 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => { await syncUserSleep("user-1"); - expect(recordSyncFailure).toHaveBeenCalledTimes(1); - expect(recordSyncFailure).toHaveBeenCalledWith( + expect(parkIntegrationAtReauth).toHaveBeenCalledTimes(1); + expect(parkIntegrationAtReauth).toHaveBeenCalledWith( expect.objectContaining({ userId: "user-1", integration: "withings", - kind: "reauth_required", errorCode: "scope_missing", }), ); + expect(recordSyncFailure).not.toHaveBeenCalled(); }); - it("treats a null scope (pre-v1.4.25 connection) as missing user.activity", async () => { + it("treats a null scope (pre-v1.4.25 connection) as missing user.activity and parks silently", async () => { vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: null, } as never); @@ -309,16 +318,17 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => { expect(imported).toBe(0); expect(fetchMock).not.toHaveBeenCalled(); - expect(recordSyncFailure).toHaveBeenCalledWith( - expect.objectContaining({ kind: "reauth_required" }), + expect(parkIntegrationAtReauth).toHaveBeenCalledWith( + expect.objectContaining({ errorCode: "scope_missing" }), ); + expect(recordSyncFailure).not.toHaveBeenCalled(); }); - it("classifies a Withings 403 in the catch-block as reauth_required (defence-in-depth)", async () => { - // Scope claim says we're fine but the resource call 403s — the - // race where Withings revokes scope without invalidating the - // refresh token. Must park at reauth_required, not transient, - // so pg-boss stops retrying. + it("classifies a Withings 403 in the catch-block as reauth_required and STILL pages (defence-in-depth)", async () => { + // BL-P3-2 — symmetric to sync-activity. The catch-block stays on + // `recordSyncFailure` because a 403 reaching the catch IS + // genuinely unexpected after the scope-skip lands above. The + // 3-strike alert ladder fires here. vi.mocked(prisma.withingsConnection.findUnique).mockResolvedValue({ scope: "user.metrics,user.activity", } as never); @@ -339,5 +349,6 @@ describe("syncUserSleep — scope-skip guard (v1.4.26)", () => { errorCode: "403", }), ); + expect(parkIntegrationAtReauth).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/withings/sync-activity.ts b/src/lib/withings/sync-activity.ts index 018eea583..f541a6b7c 100644 --- a/src/lib/withings/sync-activity.ts +++ b/src/lib/withings/sync-activity.ts @@ -47,6 +47,7 @@ import { import { getValidToken } from "./sync"; import { isReauthRequired, + parkIntegrationAtReauth, recordSyncFailure, recordSyncSuccess, } from "@/lib/integrations/status"; @@ -226,10 +227,16 @@ export async function syncUserActivity( getEvent()?.addWarning( `withings activity sync skipped for ${userId}: missing user.activity scope (legacy connection — reconnect required)`, ); - await recordSyncFailure({ + // v1.4.27 — silent park. The scope-skip is a deliberate no-op, not + // a failure burst, so the 3-strike admin alert must NOT fire from + // this branch. `parkIntegrationAtReauth` sets the row to + // `error_reauth` without incrementing the failure counter and + // without entering the threshold ladder. The defence-in-depth 403 + // catch-block below stays on `recordSyncFailure` — a 403 reaching + // the catch IS unexpected once the scope-skip lands. + await parkIntegrationAtReauth({ userId, integration: "withings", - kind: "reauth_required", message: "Withings connection is missing the user.activity scope. Reconnect Withings in Settings to enable activity sync.", errorCode: "scope_missing", diff --git a/src/lib/withings/sync-sleep.ts b/src/lib/withings/sync-sleep.ts index c3c4a8f78..b69f16ddf 100644 --- a/src/lib/withings/sync-sleep.ts +++ b/src/lib/withings/sync-sleep.ts @@ -45,6 +45,7 @@ import { import { getValidToken } from "./sync"; import { isReauthRequired, + parkIntegrationAtReauth, recordSyncFailure, recordSyncSuccess, } from "@/lib/integrations/status"; @@ -173,10 +174,14 @@ export async function syncUserSleep( getEvent()?.addWarning( `withings sleep sync skipped for ${userId}: missing user.activity scope (legacy connection — reconnect required)`, ); - await recordSyncFailure({ + // v1.4.27 — silent park (BL-P3-2 parity with activity sync). The + // scope-skip is a deliberate no-op; the 3-strike admin alert must + // NOT fire from this branch. The defence-in-depth 403 catch-block + // below stays on `recordSyncFailure` so a genuinely unexpected 403 + // after the scope-skip lands still pages admins. + await parkIntegrationAtReauth({ userId, integration: "withings", - kind: "reauth_required", message: "Withings connection is missing the user.activity scope. Reconnect Withings in Settings to enable sleep sync.", errorCode: "scope_missing",