Merge pull request #516 from trails-cool/oauth-flow-module

journal: OAuth connect→callback→resume lifecycle as one module

apps/journal/app/lib/connected-services/oauth-flow.server.test.ts

@@ -0,0 +1,162 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { createHash } from "node:crypto";
+
+const link = vi.fn();
+vi.mock("./manager.ts", () => ({
+  link: (...args: unknown[]) => link(...args),
+}));
+vi.mock("../config.server.ts", () => ({
+  getOrigin: () => "https://journal.test",
+}));
+
+import { initiateOAuthFlow, completeOAuthFlow } from "./oauth-flow.server.ts";
+import { decodeOAuthState } from "./oauth-state.server.ts";
+import type { ProviderManifest } from "./registry.ts";
+
+function fakeManifest(overrides: Partial<ProviderManifest> = {}): ProviderManifest {
+  return {
+    id: "fakeprov",
+    displayName: "Fake Provider",
+    credentialKind: "oauth",
+    credentialAdapter: { isExpired: () => false, refresh: vi.fn() },
+    buildAuthUrl: vi.fn(
+      (redirectUri: string, state: string, extras?: { codeChallenge?: string }) =>
+        `https://provider.test/authorize?redirect_uri=${encodeURIComponent(redirectUri)}&state=${state}` +
+        (extras?.codeChallenge ? `&code_challenge=${extras.codeChallenge}` : ""),
+    ),
+    exchangeCode: vi.fn(),
+    ...overrides,
+  } as unknown as ProviderManifest;
+}
+
+function callbackRequest(params: Record<string, string>, cookie?: string): Request {
+  const url = new URL("https://journal.test/api/sync/callback/fakeprov");
+  for (const [k, v] of Object.entries(params)) url.searchParams.set(k, v);
+  return new Request(url, { headers: cookie ? { Cookie: cookie } : {} });
+}
+
+beforeEach(() => vi.clearAllMocks());
+
+describe("initiateOAuthFlow", () => {
+  it("redirects to the provider with the intent in the state, no cookie for non-PKCE", () => {
+    const manifest = fakeManifest();
+    const resp = initiateOAuthFlow(manifest, { returnTo: "/settings/connections" });
+
+    expect(resp.status).toBe(302);
+    const location = new URL(resp.headers.get("Location")!);
+    expect(location.origin).toBe("https://provider.test");
+    expect(location.searchParams.get("redirect_uri")).toBe(
+      "https://journal.test/api/sync/callback/fakeprov",
+    );
+    expect(decodeOAuthState(location.searchParams.get("state"))).toEqual({
+      returnTo: "/settings/connections",
+    });
+    expect(resp.headers.get("Set-Cookie")).toBeNull();
+  });
+
+  it("PKCE: sets the verifier cookie and sends its S256 challenge", () => {
+    const manifest = fakeManifest({ pkce: true });
+    const resp = initiateOAuthFlow(manifest, { pushAfter: { routeId: "r-1" }, returnTo: "/r" });
+
+    const cookie = resp.headers.get("Set-Cookie")!;
+    expect(cookie).toContain("__oauth_pkce=");
+    expect(cookie).toContain("HttpOnly");
+    const verifier = cookie.match(/__oauth_pkce=([^;]+)/)![1]!;
+
+    const location = new URL(resp.headers.get("Location")!);
+    const expectedChallenge = createHash("sha256").update(verifier).digest("base64url");
+    expect(location.searchParams.get("code_challenge")).toBe(expectedChallenge);
+    // push intent rides the state even on the PKCE path
+    expect(decodeOAuthState(location.searchParams.get("state"))).toEqual({
+      pushAfter: { routeId: "r-1" },
+      returnTo: "/r",
+    });
+  });
+});
+
+describe("completeOAuthFlow", () => {
+  it("maps access_denied to denied with decoded state", async () => {
+    const manifest = fakeManifest();
+    const state = new URL(
+      initiateOAuthFlow(manifest, { returnTo: "/x" }).headers.get("Location")!,
+    ).searchParams.get("state")!;
+
+    const result = await completeOAuthFlow(
+      manifest,
+      callbackRequest({ error: "access_denied", state }),
+      "user-1",
+    );
+    expect(result).toEqual({ status: "denied", state: { returnTo: "/x" } });
+    expect(link).not.toHaveBeenCalled();
+  });
+
+  it("returns missing_code when no code param", async () => {
+    const result = await completeOAuthFlow(fakeManifest(), callbackRequest({}), "user-1");
+    expect(result.status).toBe("missing_code");
+  });
+
+  it("PKCE: returns missing_verifier when the cookie is gone", async () => {
+    const result = await completeOAuthFlow(
+      fakeManifest({ pkce: true }),
+      callbackRequest({ code: "abc" }),
+      "user-1",
+    );
+    expect(result.status).toBe("missing_verifier");
+  });
+
+  it("exchanges the code and links the connection", async () => {
+    const manifest = fakeManifest();
+    (manifest.exchangeCode as ReturnType<typeof vi.fn>).mockResolvedValue({
+      credentials: { accessToken: "t" },
+      providerUserId: "prov-9",
+      grantedScopes: ["routes_write"],
+    });
+
+    const result = await completeOAuthFlow(manifest, callbackRequest({ code: "abc" }), "user-1");
+
+    expect(result.status).toBe("linked");
+    expect(manifest.exchangeCode).toHaveBeenCalledWith(
+      "abc",
+      "https://journal.test/api/sync/callback/fakeprov",
+      undefined,
+    );
+    expect(link).toHaveBeenCalledWith({
+      userId: "user-1",
+      provider: "fakeprov",
+      credentialKind: "oauth",
+      credentials: { accessToken: "t" },
+      providerUserId: "prov-9",
+      grantedScopes: ["routes_write"],
+    });
+  });
+
+  it("PKCE: passes the cookie verifier to exchangeCode", async () => {
+    const manifest = fakeManifest({ pkce: true });
+    (manifest.exchangeCode as ReturnType<typeof vi.fn>).mockResolvedValue({
+      credentials: {},
+      providerUserId: "p",
+      grantedScopes: [],
+    });
+
+    await completeOAuthFlow(
+      manifest,
+      callbackRequest({ code: "abc" }, "__oauth_pkce=my-verifier"),
+      "user-1",
+    );
+    expect(manifest.exchangeCode).toHaveBeenCalledWith(
+      "abc",
+      expect.any(String),
+      { codeVerifier: "my-verifier" },
+    );
+  });
+
+  it("maps exchange failures to error with the provider code", async () => {
+    const manifest = fakeManifest();
+    (manifest.exchangeCode as ReturnType<typeof vi.fn>).mockRejectedValue(
+      Object.assign(new Error("nope"), { code: "rate_limited" }),
+    );
+
+    const result = await completeOAuthFlow(manifest, callbackRequest({ code: "x" }), "user-1");
+    expect(result).toMatchObject({ status: "error", code: "rate_limited" });
+  });
+});

apps/journal/app/lib/connected-services/oauth-flow.server.ts

@@ -0,0 +1,122 @@
+// The OAuth connect → callback lifecycle as one module. Routes stay
+// thin adapters: they look up the manifest, call initiate/complete,
+// and map the result to redirects. Everything protocol-shaped —
+// state encoding, PKCE pair generation, the verifier cookie's
+// lifecycle, redirect-URI construction, code exchange, linking the
+// connection — lives here, so the next OAuth provider (Coros, Strava)
+// reuses the flow instead of the pattern.
+
+import { redirect } from "react-router";
+import { getOrigin } from "../config.server.ts";
+import { link } from "./manager.ts";
+import type { ProviderManifest } from "./registry.ts";
+import {
+  clearPkceCookieHeader,
+  decodeOAuthState,
+  encodeOAuthState,
+  generatePkcePair,
+  pkceCookieHeader,
+  readPkceVerifier,
+  type PushOAuthState,
+} from "./oauth-state.server.ts";
+
+function callbackUri(manifest: ProviderManifest): string {
+  return `${getOrigin()}/api/sync/callback/${manifest.id}`;
+}
+
+/**
+ * Build the provider authorization redirect. Encodes post-callback
+ * intent into the state param and, for PKCE providers, sets the
+ * httpOnly verifier cookie — including when the flow is initiated
+ * from a push-resume (the old inline code only handled PKCE on the
+ * plain connect path).
+ *
+ * Caller must have verified `manifest.buildAuthUrl` exists.
+ */
+export function initiateOAuthFlow(
+  manifest: ProviderManifest,
+  intent: PushOAuthState,
+): Response {
+  if (!manifest.buildAuthUrl) {
+    throw new Error(`Provider ${manifest.id} has no buildAuthUrl`);
+  }
+  const state = encodeOAuthState(intent);
+  const redirectUri = callbackUri(manifest);
+
+  if (manifest.pkce) {
+    const { verifier, challenge } = generatePkcePair();
+    return redirect(manifest.buildAuthUrl(redirectUri, state, { codeChallenge: challenge }), {
+      headers: { "Set-Cookie": pkceCookieHeader(verifier) },
+    });
+  }
+  return redirect(manifest.buildAuthUrl(redirectUri, state));
+}
+
+export type OAuthCompletion =
+  /** Provider sent the user back with access_denied. */
+  | { status: "denied"; state: PushOAuthState }
+  /** No authorization code in the callback URL. */
+  | { status: "missing_code"; state: PushOAuthState }
+  /** PKCE provider but the verifier cookie is gone (expired / cleared). */
+  | { status: "missing_verifier"; state: PushOAuthState }
+  /** Code exchange or linking failed; `code` is provider-specific or "sync_failed". */
+  | { status: "error"; code: string; state: PushOAuthState }
+  /** Connection linked. */
+  | { status: "linked"; state: PushOAuthState };
+
+/**
+ * Consume the provider callback: decode state, recover the PKCE
+ * verifier, exchange the code, and link the connection. Never throws —
+ * every outcome is a variant the route maps to a redirect. The spent
+ * verifier cookie should be cleared on whatever response the route
+ * returns (`clearPkceCookieHeader`).
+ */
+export async function completeOAuthFlow(
+  manifest: ProviderManifest,
+  request: Request,
+  userId: string,
+): Promise<OAuthCompletion> {
+  if (!manifest.exchangeCode) {
+    throw new Error(`Provider ${manifest.id} has no exchangeCode`);
+  }
+  const url = new URL(request.url);
+  const state = decodeOAuthState(url.searchParams.get("state"));
+
+  if (url.searchParams.get("error") === "access_denied") {
+    return { status: "denied", state };
+  }
+
+  const code = url.searchParams.get("code");
+  if (!code) return { status: "missing_code", state };
+
+  const codeVerifier = manifest.pkce ? readPkceVerifier(request) : null;
+  if (manifest.pkce && !codeVerifier) {
+    return { status: "missing_verifier", state };
+  }
+
+  try {
+    const exchange = await manifest.exchangeCode(
+      code,
+      callbackUri(manifest),
+      codeVerifier ? { codeVerifier } : undefined,
+    );
+    await link({
+      userId,
+      provider: manifest.id,
+      credentialKind: manifest.credentialKind,
+      credentials: exchange.credentials as Record<string, unknown>,
+      providerUserId: exchange.providerUserId,
+      grantedScopes: exchange.grantedScopes,
+    });
+    return { status: "linked", state };
+  } catch (e) {
+    console.error(`OAuth callback failed for ${manifest.id}:`, e);
+    const code =
+      typeof (e as { code?: string }).code === "string"
+        ? (e as { code: string }).code
+        : "sync_failed";
+    return { status: "error", code, state };
+  }
+}
+
+export { clearPkceCookieHeader };

apps/journal/app/routes/api.sync.callback.$provider.ts

@@ -1,13 +1,11 @@
 import { redirect, data } from "react-router";
-import { getOrigin } from "~/lib/config.server";
 import type { Route } from "./+types/api.sync.callback.$provider";
 import { requireSessionUser } from "~/lib/auth/session.server";
-import { getManifest, link } from "~/lib/connected-services";
+import { getManifest } from "~/lib/connected-services";
 import {
-  decodeOAuthState,
-  readPkceVerifier,
+  completeOAuthFlow,
   clearPkceCookieHeader,
-} from "~/lib/connected-services/oauth-state.server";
+} from "~/lib/connected-services/oauth-flow.server";
 import { pushRouteToProvider } from "~/lib/connected-services/push-action.server";
 
 export async function loader({ params, request }: Route.LoaderArgs) {
@@ -18,51 +16,28 @@ export async function loader({ params, request }: Route.LoaderArgs) {
     return data({ error: "Unknown provider" }, { status: 404 });
   }
 
-  const url = new URL(request.url);
-  const state = decodeOAuthState(url.searchParams.get("state"));
+  const result = await completeOAuthFlow(manifest, request, user.id);
+  const state = result.state;
   const fallbackReturn = state.returnTo ?? "/settings";
+  // Spent (or irrelevant) verifier — clear it on every outcome.
+  const headers = { "Set-Cookie": clearPkceCookieHeader() };
 
-  // User denied the new scope at Wahoo. Send them back to the originating
-  // page with a notice instead of looping them through OAuth again.
-  if (url.searchParams.get("error") === "access_denied") {
-    return redirect(`${fallbackReturn}?push=needs_permission`);
-  }
-
-  const code = url.searchParams.get("code");
-  if (!code) return data({ error: "Missing authorization code" }, { status: 400 });
-
-  const origin = getOrigin();
-  const redirectUri = `${origin}/api/sync/callback/${params.provider}`;
-
-  // PKCE providers: recover the verifier from the connect-time cookie.
-  const codeVerifier = manifest.pkce ? readPkceVerifier(request) : null;
-  if (manifest.pkce && !codeVerifier) {
-    return redirect(`${fallbackReturn}?error=sync_failed`);
-  }
-
-  try {
-    const exchange = await manifest.exchangeCode(
-      code,
-      redirectUri,
-      codeVerifier ? { codeVerifier } : undefined,
-    );
-    await link({
-      userId: user.id,
-      provider: manifest.id,
-      credentialKind: manifest.credentialKind,
-      credentials: exchange.credentials as Record<string, unknown>,
-      providerUserId: exchange.providerUserId,
-      grantedScopes: exchange.grantedScopes,
-    });
-  } catch (e) {
-    console.error(`OAuth callback failed for ${params.provider}:`, e);
-    const errCode =
-      typeof (e as { code?: string }).code === "string"
-        ? (e as { code: string }).code
-        : "sync_failed";
-    return redirect(`${fallbackReturn}?error=${errCode}`);
+  switch (result.status) {
+    case "denied":
+      // User declined at the provider. Back to the originating page
+      // with a notice instead of looping them through OAuth again.
+      return redirect(`${fallbackReturn}?push=needs_permission`, { headers });
+    case "missing_code":
+      return data({ error: "Missing authorization code" }, { status: 400 });
+    case "missing_verifier":
+      return redirect(`${fallbackReturn}?error=sync_failed`, { headers });
+    case "error":
+      return redirect(`${fallbackReturn}?error=${result.code}`, { headers });
+    case "linked":
+      break;
   }
 
+  // Resume an interrupted push now that the connection has the scope.
   if (state.pushAfter?.routeId) {
     const outcome = await pushRouteToProvider({
       userId: user.id,
@@ -70,16 +45,12 @@ export async function loader({ params, request }: Route.LoaderArgs) {
       routeId: state.pushAfter.routeId,
     });
     const target = state.returnTo ?? `/routes/${state.pushAfter.routeId}`;
-    if (outcome.status === "success") return redirect(`${target}?push=success`);
-    if (outcome.status === "scope_missing") return redirect(`${target}?push=needs_permission`);
-    if (outcome.status === "needs_relink") return redirect(`${target}?push=needs_permission`);
-    if (outcome.status === "error") return redirect(`${target}?push=error&code=${outcome.code}`);
-    return redirect(`${target}?push=${outcome.status}`);
+    if (outcome.status === "success") return redirect(`${target}?push=success`, { headers });
+    if (outcome.status === "scope_missing") return redirect(`${target}?push=needs_permission`, { headers });
+    if (outcome.status === "needs_relink") return redirect(`${target}?push=needs_permission`, { headers });
+    if (outcome.status === "error") return redirect(`${target}?push=error&code=${outcome.code}`, { headers });
+    return redirect(`${target}?push=${outcome.status}`, { headers });
   }
 
-  return redirect(state.returnTo ?? "/settings", {
-    // Spent verifier — clear it regardless of which provider this was
-    // (harmless no-op for non-PKCE providers without the cookie).
-    headers: { "Set-Cookie": clearPkceCookieHeader() },
-  });
+  return redirect(state.returnTo ?? "/settings", { headers });
 }

apps/journal/app/routes/api.sync.connect.$provider.ts

@@ -1,13 +1,8 @@
-import { redirect, data } from "react-router";
-import { getOrigin } from "~/lib/config.server";
+import { data } from "react-router";
 import type { Route } from "./+types/api.sync.connect.$provider";
 import { requireSessionUser } from "~/lib/auth/session.server";
 import { getManifest } from "~/lib/connected-services";
-import {
-  encodeOAuthState,
-  generatePkcePair,
-  pkceCookieHeader,
-} from "~/lib/connected-services/oauth-state.server";
+import { initiateOAuthFlow } from "~/lib/connected-services/oauth-flow.server";
 
 export async function loader({ params, request }: Route.LoaderArgs) {
   await requireSessionUser(request);
@@ -17,19 +12,5 @@ export async function loader({ params, request }: Route.LoaderArgs) {
     return data({ error: "Unknown provider" }, { status: 404 });
   }
 
-  const origin = getOrigin();
-  const redirectUri = `${origin}/api/sync/callback/${params.provider}`;
-  const state = encodeOAuthState({ returnTo: "/settings/connections" });
-
-  // PKCE providers (Garmin): the verifier crosses the redirect in an
-  // httpOnly cookie; only the S256 challenge goes to the provider.
-  if (manifest.pkce) {
-    const { verifier, challenge } = generatePkcePair();
-    return redirect(
-      manifest.buildAuthUrl(redirectUri, state, { codeChallenge: challenge }),
-      { headers: { "Set-Cookie": pkceCookieHeader(verifier) } },
-    );
-  }
-
-  return redirect(manifest.buildAuthUrl(redirectUri, state));
+  return initiateOAuthFlow(manifest, { returnTo: "/settings/connections" });
 }

apps/journal/app/routes/api.sync.push.$provider.$routeId.ts

@@ -1,10 +1,9 @@
 import { redirect, data } from "react-router";
-import { getOrigin } from "~/lib/config.server";
 import type { Route } from "./+types/api.sync.push.$provider.$routeId";
 import { requireSessionUser } from "~/lib/auth/session.server";
 import { getManifest } from "~/lib/connected-services";
 import { pushRouteToProvider } from "~/lib/connected-services/push-action.server";
-import { encodeOAuthState } from "~/lib/connected-services/oauth-state.server";
+import { initiateOAuthFlow } from "~/lib/connected-services/oauth-flow.server";
 
 export async function action({ params, request }: Route.ActionArgs) {
   const user = await requireSessionUser(request);
@@ -26,13 +25,12 @@ export async function action({ params, request }: Route.ActionArgs) {
       if (!manifest.buildAuthUrl) {
         return redirect(`${returnTo}?push=needs_permission`);
       }
-      const origin = getOrigin();
-      const redirectUri = `${origin}/api/sync/callback/${manifest.id}`;
-      const state = encodeOAuthState({
+      // Re-authorize with the push intent in the state; the callback
+      // resumes the push once the scope is granted.
+      return initiateOAuthFlow(manifest, {
         pushAfter: { routeId: params.routeId },
         returnTo,
       });
-      return redirect(manifest.buildAuthUrl(redirectUri, state));
     }
     case "needs_relink":
       return redirect(`${returnTo}?push=needs_permission`);