diff --git a/src/hooks/use-auth.tsx b/src/hooks/use-auth.tsx index a4b6a33..47284dd 100644 --- a/src/hooks/use-auth.tsx +++ b/src/hooks/use-auth.tsx @@ -131,12 +131,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { const { data, error } = await supabase .from("profiles") .select( - // `account:accounts!inner(id, name)` — explicit join on the - // single FK profiles.account_id → accounts.id. `!inner` so a - // missing account collapses to null rather than a half- - // populated row (shouldn't happen post-017 NOT NULL, but - // belt-and-braces against forks running older schemas). - "id, full_name, email, avatar_url, role, beta_features, account_id, account_role, account:accounts!inner(id, name, default_currency)", + "id, full_name, email, avatar_url, role, beta_features, account_id, account_role", ) .eq("user_id", userId) .maybeSingle(); @@ -152,27 +147,40 @@ export function AuthProvider({ children }: { children: ReactNode }) { } if (data) { - // Supabase's typed client surfaces an embedded `!inner` row - // as either an object or a single-element array depending on - // the schema's inferred cardinality — normalise to the object - // form before reading. - const accountRaw = Array.isArray(data.account) - ? data.account[0] ?? null - : (data.account as { - id: string; - name: string; - default_currency: string | null; - } | null); - // Narrow default_currency defensively: forks running pre-021 - // schemas won't have the column, so a missing/null value reads - // as the safe USD fallback rather than crashing the picker. - const accountRow: AccountSummary | null = accountRaw - ? { - id: accountRaw.id, - name: accountRaw.name, - default_currency: accountRaw.default_currency ?? DEFAULT_CURRENCY, - } - : null; + // Load the account with a plain lookup by id instead of an + // embedded FK join. The embed (`account:accounts!inner(...)`) + // forces PostgREST to resolve the profiles.account_id → + // accounts.id relationship from its schema cache; a stale cache + // (common right after a migration adds the FK) makes it fail + // hard with PGRST200 and blanks the whole profile — the user + // then loses account context everywhere (issue #294). A point + // lookup by id needs no relationship inference, so the profile + // (with account_id / account_role) still resolves even if the + // account name lookup itself can't. + let accountRow: AccountSummary | null = null; + if (data.account_id) { + const { data: account, error: accountErr } = await supabase + .from("accounts") + // default_currency added in migration 021; narrowed to the + // USD fallback below for older schemas where it reads null. + .select("id, name, default_currency") + .eq("id", data.account_id) + .maybeSingle(); + if (accountErr) { + console.error("[AuthProvider] fetchAccount error:", { + message: accountErr.message, + details: accountErr.details, + hint: accountErr.hint, + code: accountErr.code, + }); + } else if (account) { + accountRow = { + id: account.id, + name: account.name, + default_currency: account.default_currency ?? DEFAULT_CURRENCY, + }; + } + } // Narrow the DB enum into our AccountRole union. The DB // constraint should make this unconditional, but a future diff --git a/src/lib/auth/account.test.ts b/src/lib/auth/account.test.ts new file mode 100644 index 0000000..29f7aa8 --- /dev/null +++ b/src/lib/auth/account.test.ts @@ -0,0 +1,176 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +// getCurrentAccount resolves the caller's account context. The +// regression this file guards (issue #294): account loading must NOT +// depend on a PostgREST embedded FK join (`accounts!inner`), because a +// stale schema cache makes that embed fail hard and blanks the whole +// context. It must instead read the profile and then the account with +// two plain point queries. + +// ------------------------------------------------------------ +// Chainable Supabase query-builder mock. Each `.from(table)` hands back +// a thenable builder pre-loaded with the result queued for that table, +// so we can assert which tables were queried and with what filters. +// ------------------------------------------------------------ +interface BuilderCall { + table: string; + columns?: string; + eqArgs: [string, unknown][]; +} + +function makeClient(opts: { + user: { id: string } | null; + userErr?: unknown; + byTable: Record; +}) { + const calls: BuilderCall[] = []; + + const from = (table: string) => { + const call: BuilderCall = { table, eqArgs: [] }; + calls.push(call); + const builder = { + select(columns: string) { + call.columns = columns; + return builder; + }, + eq(col: string, val: unknown) { + call.eqArgs.push([col, val]); + return builder; + }, + maybeSingle() { + return Promise.resolve( + opts.byTable[table] ?? { data: null, error: null }, + ); + }, + }; + return builder; + }; + + return { + calls, + client: { + auth: { + getUser: () => + Promise.resolve({ + data: { user: opts.user }, + error: opts.userErr ?? null, + }), + }, + from, + }, + }; +} + +const createClient = vi.fn(); +vi.mock("@/lib/supabase/server", () => ({ + createClient: () => createClient(), +})); + +const { getCurrentAccount, UnauthorizedError, ForbiddenError } = await import( + "./account" +); + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe("getCurrentAccount", () => { + it("resolves context via a plain accounts lookup, not an embedded join", async () => { + const { client, calls } = makeClient({ + user: { id: "user-1" }, + byTable: { + profiles: { + data: { account_id: "acct-1", account_role: "owner" }, + error: null, + }, + accounts: { data: { id: "acct-1", name: "Acme" }, error: null }, + }, + }); + createClient.mockReturnValue(client); + + const ctx = await getCurrentAccount(); + + expect(ctx).toMatchObject({ + userId: "user-1", + accountId: "acct-1", + role: "owner", + account: { id: "acct-1", name: "Acme" }, + }); + + // Two queries: profiles by user_id, then accounts by id. Neither + // selects an embedded relationship — the regression guard. + expect(calls.map((c) => c.table)).toEqual(["profiles", "accounts"]); + expect(calls[0].columns).not.toMatch(/accounts!/); + expect(calls[0].eqArgs).toEqual([["user_id", "user-1"]]); + expect(calls[1].columns).not.toMatch(/accounts!/); + expect(calls[1].eqArgs).toEqual([["id", "acct-1"]]); + }); + + it("throws UnauthorizedError when there is no session", async () => { + const { client } = makeClient({ user: null, byTable: {} }); + createClient.mockReturnValue(client); + await expect(getCurrentAccount()).rejects.toBeInstanceOf(UnauthorizedError); + }); + + it("maps a profiles query error to 'Could not load account context'", async () => { + const { client } = makeClient({ + user: { id: "user-1" }, + byTable: { + profiles: { data: null, error: { code: "PGRST200" } }, + }, + }); + createClient.mockReturnValue(client); + await expect(getCurrentAccount()).rejects.toThrow( + "Could not load account context", + ); + }); + + it("maps an accounts query error to 'Could not load account context'", async () => { + // The exact #294 shape if the embed were still in play, but now on + // the decoupled accounts lookup: profile resolves, account read errors. + const { client } = makeClient({ + user: { id: "user-1" }, + byTable: { + profiles: { + data: { account_id: "acct-1", account_role: "admin" }, + error: null, + }, + accounts: { data: null, error: { code: "PGRST200" } }, + }, + }); + createClient.mockReturnValue(client); + const err = await getCurrentAccount().catch((e) => e); + expect(err).toBeInstanceOf(ForbiddenError); + expect(err.message).toBe("Could not load account context"); + }); + + it("rejects a profile not linked to an account", async () => { + const { client } = makeClient({ + user: { id: "user-1" }, + byTable: { + profiles: { data: { account_id: null, account_role: null }, error: null }, + }, + }); + createClient.mockReturnValue(client); + await expect(getCurrentAccount()).rejects.toThrow( + "Profile is not linked to an account", + ); + }); + + it("rejects an account_id that resolves to no readable account", async () => { + const { client } = makeClient({ + user: { id: "user-1" }, + byTable: { + profiles: { + data: { account_id: "acct-1", account_role: "viewer" }, + error: null, + }, + accounts: { data: null, error: null }, + }, + }); + createClient.mockReturnValue(client); + await expect(getCurrentAccount()).rejects.toThrow( + "Profile is not linked to an account", + ); + }); +}); diff --git a/src/lib/auth/account.ts b/src/lib/auth/account.ts index a465075..5fba21a 100644 --- a/src/lib/auth/account.ts +++ b/src/lib/auth/account.ts @@ -114,14 +114,9 @@ export async function getCurrentAccount(): Promise { throw new UnauthorizedError(); } - // Selecting through the FK gives us the account name in one - // query — `account:accounts!inner(id,name)` is Supabase's - // explicit-join syntax. `!inner` so a NULL account_id (which - // shouldn't exist) yields no row and trips the guard below - // rather than silently returning a half-populated profile. const { data, error } = await supabase .from("profiles") - .select("account_id, account_role, account:accounts!inner(id, name)") + .select("account_id, account_role") .eq("user_id", user.id) .maybeSingle(); @@ -129,7 +124,7 @@ export async function getCurrentAccount(): Promise { console.error("[getCurrentAccount] profile fetch error:", error); throw new ForbiddenError("Could not load account context"); } - if (!data || !data.account_id || !data.account_role || !data.account) { + if (!data || !data.account_id || !data.account_role) { // Pre-migration profile, or a manual insert that skipped the // signup trigger. The user is authenticated but the app has // no way to scope their queries — treat as forbidden. @@ -142,16 +137,38 @@ export async function getCurrentAccount(): Promise { throw new ForbiddenError(`Unknown account role: ${data.account_role}`); } - // Supabase's typed client returns related rows as an array even - // for `!inner` single-record joins; normalise to a single object. - const accountRow = Array.isArray(data.account) ? data.account[0] : data.account; + // Load the account with a plain point lookup by id rather than an + // embedded FK join (`account:accounts!inner(...)`). The embed forces + // PostgREST to resolve the profiles.account_id → accounts.id + // relationship from its schema cache; when that cache is stale — a + // common Supabase state right after a migration adds the FK, or when + // migrations are applied out of band — the embed fails hard with + // PGRST200 ("could not find a relationship … in the schema cache") + // and takes down the entire account context (issue #294). A lookup by + // id needs no relationship inference and is gated by the same accounts + // RLS, so it stays robust against cache staleness and older schemas. + const { data: account, error: accountErr } = await supabase + .from("accounts") + .select("id, name") + .eq("id", data.account_id) + .maybeSingle(); + + if (accountErr) { + console.error("[getCurrentAccount] account fetch error:", accountErr); + throw new ForbiddenError("Could not load account context"); + } + if (!account) { + // account_id points at no readable account row — orphaned profile + // or an RLS gap. Same "can't scope this user" outcome as above. + throw new ForbiddenError("Profile is not linked to an account"); + } return { supabase, userId: user.id, accountId: data.account_id, role: data.account_role, - account: { id: accountRow.id, name: accountRow.name }, + account: { id: account.id, name: account.name }, }; }