tests: Harden web upgrade and authorization flows

web-admin/src/features/billing/plans/upgrade-to-plan.spec.ts

@@ -0,0 +1,219 @@
+import { describe, expect, it, vi } from "vitest";
+import type { CategorisedOrganizationBillingIssues } from "../selectors";
+import {
+  createUpgradeToPlanAction,
+  type UpgradeToPlanDependencies,
+} from "./upgrade-to-plan";
+
+const CURRENT_URL = "https://app.example/acme/settings";
+
+function issues(
+  overrides: Partial<CategorisedOrganizationBillingIssues> = {},
+): CategorisedOrganizationBillingIssues {
+  return {
+    payment: [],
+    needsPaymentSetup: false,
+    ...overrides,
+  };
+}
+
+function dependencies(
+  overrides: Partial<UpgradeToPlanDependencies> = {},
+): UpgradeToPlanDependencies {
+  return {
+    getPage: vi.fn(
+      () =>
+        ({ url: new URL(CURRENT_URL) }) as ReturnType<
+          UpgradeToPlanDependencies["getPage"]
+        >,
+    ),
+    getCurrentUrl: vi.fn(() => CURRENT_URL),
+    getAllowedRedirectOrigins: vi.fn(() => ["https://developer.example"]),
+    fetchPaymentPortalUrl: vi
+      .fn()
+      .mockResolvedValue("https://payments.example/session"),
+    fetchPlan: vi.fn().mockResolvedValue({ displayName: "Growth" }),
+    renew: vi.fn().mockResolvedValue(undefined),
+    update: vi.fn().mockResolvedValue(undefined),
+    notifyRenewed: vi.fn(),
+    showWelcome: vi.fn(),
+    invalidate: vi.fn(),
+    open: vi.fn(),
+    ...overrides,
+  };
+}
+
+describe("upgradeToPlan", () => {
+  it("requests one short-lived portal URL when payment setup is required", async () => {
+    // Payment issues must take the portal path and skip plan mutations.
+    const deps = dependencies();
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await upgrade(
+      "acme",
+      "growth",
+      issues({ payment: [{}] as never[], needsPaymentSetup: true }),
+      null,
+    );
+
+    expect(deps.fetchPaymentPortalUrl).toHaveBeenCalledOnce();
+    expect(deps.fetchPaymentPortalUrl).toHaveBeenCalledWith(
+      "acme",
+      "https://app.example/acme/-/upgrade-callback?plan=growth",
+      true,
+    );
+    expect(deps.open).toHaveBeenCalledWith("https://payments.example/session");
+    expect(deps.fetchPlan).not.toHaveBeenCalled();
+    expect(deps.invalidate).not.toHaveBeenCalled();
+  });
+
+  it("does not navigate when the payment portal request fails", async () => {
+    // A failed portal lookup must not trigger any post-request effects.
+    const deps = dependencies({
+      fetchPaymentPortalUrl: vi.fn().mockRejectedValue(new Error("offline")),
+    });
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await expect(
+      upgrade("acme", "growth", issues({ payment: [{}] as never[] }), null),
+    ).rejects.toThrow("offline");
+
+    expect(deps.open).not.toHaveBeenCalled();
+    expect(deps.invalidate).not.toHaveBeenCalled();
+  });
+
+  it("renews a cancelled plan and reports success", async () => {
+    // Cancelled subscriptions renew instead of issuing an update.
+    const deps = dependencies();
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await upgrade("acme", "growth", issues({ cancelled: {} as never }), null);
+
+    expect(deps.renew).toHaveBeenCalledOnce();
+    expect(deps.renew).toHaveBeenCalledWith("acme", "growth");
+    expect(deps.update).not.toHaveBeenCalled();
+    expect(deps.notifyRenewed).toHaveBeenCalledWith("Growth");
+    expect(deps.invalidate).toHaveBeenCalledWith("acme");
+  });
+
+  it("leaves post-mutation effects untouched when renewal fails", async () => {
+    // Renewal errors must stop notification, invalidation, and navigation.
+    const deps = dependencies({
+      renew: vi.fn().mockRejectedValue(new Error("renew failed")),
+    });
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await expect(
+      upgrade(
+        "acme",
+        "growth",
+        issues({ cancelled: {} as never }),
+        "https://app.example/return",
+      ),
+    ).rejects.toThrow("renew failed");
+
+    expect(deps.notifyRenewed).not.toHaveBeenCalled();
+    expect(deps.invalidate).not.toHaveBeenCalled();
+    expect(deps.open).not.toHaveBeenCalled();
+  });
+
+  it("updates a valid plan and shows the welcome state", async () => {
+    // A standard upgrade updates the plan and shows the default success UI.
+    const deps = dependencies();
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await upgrade("acme", "growth", issues(), null);
+
+    expect(deps.update).toHaveBeenCalledWith("acme", "growth");
+    expect(deps.showWelcome).toHaveBeenCalledWith("growth");
+    expect(deps.invalidate).toHaveBeenCalledWith("acme");
+  });
+
+  it("surfaces update failure and permits a clean retry", async () => {
+    // Failed mutations release the in-flight guard for a later retry.
+    const update = vi
+      .fn()
+      .mockRejectedValueOnce(new Error("update failed"))
+      .mockResolvedValueOnce(undefined);
+    const deps = dependencies({ update });
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await expect(
+      upgrade("acme", "growth", issues(), "https://app.example/return"),
+    ).rejects.toThrow("update failed");
+    expect(deps.invalidate).not.toHaveBeenCalled();
+    expect(deps.open).not.toHaveBeenCalled();
+
+    await upgrade("acme", "growth", issues(), null);
+    expect(update).toHaveBeenCalledTimes(2);
+    expect(deps.invalidate).toHaveBeenCalledOnce();
+  });
+
+  it("rejects an invalid plan before mutation", async () => {
+    // Missing plan metadata must fail before any billing write.
+    const deps = dependencies({
+      fetchPlan: vi.fn().mockResolvedValue(undefined),
+    });
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await expect(upgrade("acme", "bogus", issues(), null)).rejects.toThrow(
+      "Plan bogus not found",
+    );
+
+    expect(deps.renew).not.toHaveBeenCalled();
+    expect(deps.update).not.toHaveBeenCalled();
+    expect(deps.invalidate).not.toHaveBeenCalled();
+  });
+
+  it.each([
+    ["same-origin", "https://app.example/return"],
+    ["configured origin", "https://developer.example/return"],
+  ])("opens an approved %s redirect", async (_name, redirect) => {
+    // Exact same-origin and configured-origin redirects are allowed.
+    const deps = dependencies();
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await upgrade("acme", "growth", issues(), redirect);
+
+    expect(deps.open).toHaveBeenCalledWith(redirect);
+    expect(deps.showWelcome).not.toHaveBeenCalled();
+  });
+
+  it("rejects a lookalike external redirect", async () => {
+    // Hostname lookalikes must fall back to the normal welcome state.
+    const deps = dependencies();
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    await upgrade(
+      "acme",
+      "growth",
+      issues(),
+      "https://developer.example.evil.test/return",
+    );
+
+    expect(deps.open).not.toHaveBeenCalled();
+    expect(deps.showWelcome).toHaveBeenCalledWith("growth");
+  });
+
+  it("coalesces repeated clicks into one mutation", async () => {
+    // Concurrent clicks must share one request and one invalidation.
+    let resolveUpdate!: () => void;
+    const update = vi.fn(
+      () =>
+        new Promise<void>((resolve) => {
+          resolveUpdate = resolve;
+        }),
+    );
+    const deps = dependencies({ update });
+    const upgrade = createUpgradeToPlanAction(deps);
+
+    const first = upgrade("acme", "growth", issues(), null);
+    const second = upgrade("acme", "growth", issues(), null);
+    await vi.waitFor(() => expect(update).toHaveBeenCalledOnce());
+
+    expect(second).toBe(first);
+    resolveUpdate();
+    await Promise.all([first, second]);
+    expect(deps.invalidate).toHaveBeenCalledOnce();
+  });
+});

web-admin/src/features/billing/plans/upgrade-to-plan.ts

@@ -14,47 +14,129 @@ import { triggerWelcomeToRillDialog } from "@rilldata/web-admin/features/billing
 import { invalidateBillingInfo } from "@rilldata/web-admin/features/billing/invalidations.ts";
 import { page } from "$app/stores";
 import { get } from "svelte/store";
+import {
+  getConfiguredRedirectOrigins,
+  getSafeRedirectTarget,
+} from "@rilldata/web-admin/lib/safe-redirect.ts";
+
+type UpgradePlan = {
+  displayName?: string | null;
+};
+
+export type UpgradeToPlanDependencies = {
+  getPage: () => Parameters<typeof getBillingUpgradeUrl>[0];
+  getCurrentUrl: () => string;
+  getAllowedRedirectOrigins: () => readonly string[];
+  fetchPaymentPortalUrl: (
+    org: string,
+    returnUrl: string,
+    setup: boolean,
+  ) => Promise<string>;
+  fetchPlan: (planName: string) => Promise<UpgradePlan | null | undefined>;
+  renew: (org: string, planName: string) => Promise<unknown>;
+  update: (org: string, planName: string) => Promise<unknown>;
+  notifyRenewed: (displayName: string) => void;
+  showWelcome: (planName: string) => void;
+  invalidate: (org: string) => unknown;
+  open: (url: string) => void;
+};
+
+const defaultDependencies: UpgradeToPlanDependencies = {
+  getPage: () => get(page),
+  getCurrentUrl: () => window.location.href,
+  getAllowedRedirectOrigins: getConfiguredRedirectOrigins,
+  fetchPaymentPortalUrl: fetchPaymentsPortalURL,
+  fetchPlan: maybeFetchPublicPlanByName,
+  renew: (org, planName) =>
+    adminServiceRenewBillingSubscription(org, { planName }),
+  update: (org, planName) =>
+    adminServiceUpdateBillingSubscription(org, { planName }),
+  notifyRenewed: (displayName) =>
+    eventBus.emit("notification", {
+      type: "success",
+      message: m.billing_plan_renewed({ planName: displayName }),
+    }),
+  showWelcome: triggerWelcomeToRillDialog,
+  invalidate: invalidateBillingInfo,
+  open: (url) => {
+    window.open(url, "_self");
+  },
+};
 
-export async function upgradeToPlan(
+async function performUpgradeToPlan(
+  dependencies: UpgradeToPlanDependencies,
   org: string,
   planName: string,
   categorisedIssues: CategorisedOrganizationBillingIssues,
   redirect: string | null,
 ) {
   if (categorisedIssues.payment.length > 0) {
-    // Payment setup is required first. Carry the chosen plan through the Stripe
-    // return URL so the upgrade-callback page can complete the upgrade.
-    window.open(
-      await fetchPaymentsPortalURL(
-        org,
-        getBillingUpgradeUrl(get(page), org, planName),
-        categorisedIssues.needsPaymentSetup,
-      ),
-      "_self",
+    // The server-issued portal URL is short-lived and signed, so only the
+    // winning in-flight request may fetch and consume it.
+    const portalUrl = await dependencies.fetchPaymentPortalUrl(
+      org,
+      getBillingUpgradeUrl(dependencies.getPage(), org, planName),
+      categorisedIssues.needsPaymentSetup,
     );
+    if (!portalUrl) throw new Error("Payment portal URL is unavailable");
+    dependencies.open(portalUrl);
     return;
   }
 
-  const plan = await maybeFetchPublicPlanByName(planName);
-  if (!plan) return;
+  const plan = await dependencies.fetchPlan(planName);
+  if (!plan) throw new Error(`Plan ${planName} not found`);
+
+  const safeRedirect = getSafeRedirectTarget(
+    redirect,
+    dependencies.getCurrentUrl(),
+    { allowedOrigins: dependencies.getAllowedRedirectOrigins() },
+  );
+
   if (categorisedIssues.cancelled) {
-    await adminServiceRenewBillingSubscription(org, {
-      planName,
-    });
-    eventBus.emit("notification", {
-      type: "success",
-      message: m.billing_plan_renewed({ planName: plan.displayName ?? "" }),
-    });
+    await dependencies.renew(org, planName);
+    dependencies.notifyRenewed(plan.displayName ?? "");
   } else {
-    await adminServiceUpdateBillingSubscription(org, {
+    await dependencies.update(org, planName);
+    // A rejected redirect behaves like no redirect, so the successful upgrade
+    // still gets its normal welcome state.
+    if (!safeRedirect) dependencies.showWelcome(planName);
+  }
+
+  // Mutation success is the commit point. Failed requests never invalidate
+  // cached billing data or navigate away from the error shown by the dialog.
+  void dependencies.invalidate(org);
+  if (safeRedirect) dependencies.open(safeRedirect);
+}
+
+export function createUpgradeToPlanAction(
+  dependencies: UpgradeToPlanDependencies,
+) {
+  const inFlight = new Map<string, Promise<void>>();
+
+  return function runUpgradeToPlan(
+    org: string,
+    planName: string,
+    categorisedIssues: CategorisedOrganizationBillingIssues,
+    redirect: string | null,
+  ): Promise<void> {
+    const key = JSON.stringify([org, planName]);
+    const activeRequest = inFlight.get(key);
+    if (activeRequest !== undefined) return activeRequest;
+
+    const request = performUpgradeToPlan(
+      dependencies,
+      org,
       planName,
+      categorisedIssues,
+      redirect,
+    ).finally(() => {
+      // Release both successes and failures; a failed attempt can be retried,
+      // while rapid repeated clicks still share exactly one mutation.
+      if (inFlight.get(key) === request) inFlight.delete(key);
     });
-    triggerWelcomeToRillDialog(planName);
-  }
-  void invalidateBillingInfo(org);
-  if (redirect) {
-    // redirect param could be on a different domain like the rill developer instance
-    // so using goto won't work
-    window.open(redirect, "_self");
-  }
+    inFlight.set(key, request);
+    return request;
+  };
 }
+
+export const upgradeToPlan = createUpgradeToPlanAction(defaultDependencies);