From 467499b5f72614bcd52ae146b0d90c7ce7abf047 Mon Sep 17 00:00:00 2001 From: Charles Nykamp <16085675+cqnykamp@users.noreply.github.com> Date: Tue, 1 Sep 2026 09:23:43 -0500 Subject: [PATCH] fix(api): stop logging an error when an anonymous user logs in to an existing account `upgradeAnonymousUser` sets `users.email`, so it hits the `users_email_key` unique constraint whenever the email someone authenticates with already has an account -- i.e. every time a returning user browses anonymously and then logs in. Production logged a P2002 stack trace for each one. `serializeUser` already handled this by catching everything and falling back to `findOrCreateUser`, which is the right outcome but hid genuine failures behind the same `console.log`. Make the no-op cases part of the contract instead: `upgradeAnonymousUser` returns `null` for P2002 (email taken) and P2025 (not anonymous, or no such user) and rethrows anything else, so a real database failure now fails the login rather than passing silently. `parseFromAnonymous` takes over the one thing the blanket catch was legitimately guarding -- `toUUID` throwing on a malformed `fromAnonymous` value -- and folds in the magic-link `" "` placeholder check. The Google branch now runs `updateUser` only when the upgrade actually happened; previously a failure there discarded the successful upgrade. Anonymous work is still not merged into the existing account, so a student who takes an assignment anonymously and then logs in remains two rows in the instructor's list. That is a separate decision. Tests move to `auth/anonymousUpgrade.test.ts` and pin all five outcomes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T6R3jTUpCKjeAKV64cd6FC --- apps/api/src/auth/anonymousUpgrade.test.ts | 104 +++++++++++++++++++++ apps/api/src/auth/fromAnonymous.test.ts | 30 ++++++ apps/api/src/auth/fromAnonymous.ts | 26 ++++++ apps/api/src/auth/index.ts | 2 + apps/api/src/index.ts | 75 ++++++--------- apps/api/src/query/user.ts | 35 +++++-- apps/api/src/test/users.test.ts | 25 +---- 7 files changed, 222 insertions(+), 75 deletions(-) create mode 100644 apps/api/src/auth/anonymousUpgrade.test.ts create mode 100644 apps/api/src/auth/fromAnonymous.test.ts create mode 100644 apps/api/src/auth/fromAnonymous.ts diff --git a/apps/api/src/auth/anonymousUpgrade.test.ts b/apps/api/src/auth/anonymousUpgrade.test.ts new file mode 100644 index 0000000000..118c3f4acd --- /dev/null +++ b/apps/api/src/auth/anonymousUpgrade.test.ts @@ -0,0 +1,104 @@ +import { describe, expect, test } from "vitest"; +import { upgradeAnonymousUser } from "../query/user"; +import { prisma } from "../model"; +import { createTestAnonymousUser, createTestUser } from "../test/utils"; + +// `upgradeAnonymousUser` runs from `serializeUser` when someone who has been +// browsing anonymously finishes a magic-link or Google login. The email they +// authenticated with may or may not already belong to a real account, so these +// tests pin both outcomes: the caller relies on a `null` return (not a throw) +// to fall back to `findOrCreateUser`. + +function freshEmail(label: string) { + const id = + Date.now().toString() + Math.round(Math.random() * 100000).toString(); + return `${label}${id}@vitest.test`; +} + +describe("upgradeAnonymousUser", () => { + test("claims an unused email and keeps the same account", async () => { + const anonUser = await createTestAnonymousUser(); + expect(anonUser.isAnonymous).eq(true); + + const email = freshEmail("upgrade"); + const upgraded = await upgradeAnonymousUser({ + userId: anonUser.userId, + email, + }); + + expect(upgraded).not.eq(null); + expect(upgraded!.isAnonymous).eq(false); + expect(upgraded!.email).eq(email); + // Same row, so the session and any content made while anonymous survive. + expect(upgraded!.userId).toStrictEqual(anonUser.userId); + }); + + test("returns null when the email already belongs to another account", async () => { + const existingUser = await createTestUser(); + const anonUser = await createTestAnonymousUser(); + + const upgraded = await upgradeAnonymousUser({ + userId: anonUser.userId, + email: existingUser.email!, + }); + + // The caller falls back to `findOrCreateUser`, which logs them in to + // `existingUser`. This is an ordinary returning-user login, not an error. + expect(upgraded).eq(null); + }); + + test("leaves both accounts untouched when the email is taken", async () => { + const existingUser = await createTestUser(); + const anonUser = await createTestAnonymousUser(); + + await upgradeAnonymousUser({ + userId: anonUser.userId, + email: existingUser.email!, + }); + + const anonAfter = await prisma.users.findUniqueOrThrow({ + where: { userId: anonUser.userId }, + select: { email: true, isAnonymous: true }, + }); + expect(anonAfter.isAnonymous).eq(true); + expect(anonAfter.email).eq(anonUser.email); + + const existingAfter = await prisma.users.findUniqueOrThrow({ + where: { userId: existingUser.userId }, + select: { email: true, isAnonymous: true, firstNames: true }, + }); + expect(existingAfter.email).eq(existingUser.email); + expect(existingAfter.isAnonymous).eq(false); + expect(existingAfter.firstNames).eq(existingUser.firstNames); + }); + + test("returns null when the account is not anonymous", async () => { + const user = await createTestUser(); + + // Re-running the upgrade (a second login from a stale `fromAnonymous` + // cookie) must not re-stamp the email of an already-real account. + const upgraded = await upgradeAnonymousUser({ + userId: user.userId, + email: freshEmail("second"), + }); + + expect(upgraded).eq(null); + + const after = await prisma.users.findUniqueOrThrow({ + where: { userId: user.userId }, + select: { email: true }, + }); + expect(after.email).eq(user.email); + }); + + test("returns null when the user does not exist", async () => { + const missingUserId = new Uint8Array(16).fill(0); + + const upgraded = await upgradeAnonymousUser({ + userId: missingUserId, + email: freshEmail("missing"), + }); + + expect(upgraded).eq(null); + }); +}); diff --git a/apps/api/src/auth/fromAnonymous.test.ts b/apps/api/src/auth/fromAnonymous.test.ts new file mode 100644 index 0000000000..c047dd5f73 --- /dev/null +++ b/apps/api/src/auth/fromAnonymous.test.ts @@ -0,0 +1,30 @@ +import { describe, expect, test, vi, afterEach } from "vitest"; +import { parseFromAnonymous } from "./fromAnonymous"; +import { fromUUID, newUUID } from "../utils/uuid"; + +describe("parseFromAnonymous", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + test("reads a short-form user id", () => { + const userId = newUUID(); + + expect(parseFromAnonymous(fromUUID(userId))).toStrictEqual(userId); + }); + + test("treats the magic-link placeholder as no anonymous account", () => { + // passport-magic-link requires the field, so the login route sends `" "` + // when there is no anonymous account. + expect(parseFromAnonymous(" ")).eq(undefined); + expect(parseFromAnonymous("")).eq(undefined); + expect(parseFromAnonymous(undefined)).eq(undefined); + }); + + test("ignores an unparsable id rather than failing the login", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + + expect(parseFromAnonymous("not-a-uuid")).eq(undefined); + expect(warn).toHaveBeenCalled(); + }); +}); diff --git a/apps/api/src/auth/fromAnonymous.ts b/apps/api/src/auth/fromAnonymous.ts new file mode 100644 index 0000000000..dfe36c005b --- /dev/null +++ b/apps/api/src/auth/fromAnonymous.ts @@ -0,0 +1,26 @@ +import { toUUID } from "../utils/uuid"; + +/** + * Read the `fromAnonymous` field carried through a magic-link or Google login. + * + * It holds the short-form id of the anonymous account the person was browsing + * under, or a placeholder when there was none: the magic-link flow sends `" "` + * because passport-magic-link requires the field to be present. + * + * Returns `undefined` when there is no anonymous account to upgrade, including + * when the value is unparsable — a malformed id must not fail the login. + */ +export function parseFromAnonymous( + fromAnonymous: string | undefined, +): Uint8Array | undefined { + if (!fromAnonymous || fromAnonymous.trim() === "") { + return undefined; + } + + try { + return toUUID(fromAnonymous); + } catch (e) { + console.warn("Ignoring unparsable fromAnonymous value", e); + return undefined; + } +} diff --git a/apps/api/src/auth/index.ts b/apps/api/src/auth/index.ts index d00dbac450..4045b109db 100644 --- a/apps/api/src/auth/index.ts +++ b/apps/api/src/auth/index.ts @@ -4,6 +4,7 @@ // `done(err)` instead of escaping as an unhandled rejection // toGoogleAccount — validate Google's raw OIDC payload and reduce it to the // fields we store +// parseFromAnonymous — read the anonymous account id carried through a login // SessionUser — the payload shapes that reach `serializeUser` // // Import from here, not from a source file. @@ -11,6 +12,7 @@ export { asyncPassport } from "./asyncPassport"; export type { DoneCallback } from "./asyncPassport"; export { toGoogleAccount, googleProfileJsonSchema } from "./googleProfile"; +export { parseFromAnonymous } from "./fromAnonymous"; export type { GoogleAccount, GoogleProfileJson } from "./googleProfile"; export type { SessionUser, diff --git a/apps/api/src/index.ts b/apps/api/src/index.ts index c36ad5bdae..ebf161e159 100644 --- a/apps/api/src/index.ts +++ b/apps/api/src/index.ts @@ -52,7 +52,7 @@ import { metricsRouter } from "./routes/metricsRoutes"; import { contentRouter } from "./routes/content.route"; import { loadMediaConfig, mediaRouter } from "./media"; import { getEnvVar, isTestAuthBypassEnabled } from "./utils/env"; -import { asyncPassport, toGoogleAccount } from "./auth"; +import { asyncPassport, parseFromAnonymous, toGoogleAccount } from "./auth"; import type { DoneCallback, SessionUser } from "./auth"; import { installProcessErrorHandlers } from "./errors/processErrorHandlers"; @@ -284,29 +284,21 @@ passport.serializeUser( async (req: Request, user: SessionUser, done: DoneCallback) => { if (user.provider === "magiclink") { const email: string = user.email; - const fromAnonymous: string = user.fromAnonymous; - - let u; - - if (fromAnonymous !== " ") { - try { - u = await upgradeAnonymousUser({ - userId: toUUID(fromAnonymous), - email, - }); - } catch (_e) { - console.log("Error upgrading anonymous user", _e); - /// ignore any error - } - } + const anonymousUserId = parseFromAnonymous(user.fromAnonymous); - if (!u) { - u = await findOrCreateUser({ + const upgraded = anonymousUserId + ? await upgradeAnonymousUser({ userId: anonymousUserId, email }) + : null; + + // `email` already has an account (or there was nothing to upgrade): + // log in to that account and leave the anonymous one alone. + const u = + upgraded ?? + (await findOrCreateUser({ email, firstNames: null, lastNames: "", - }); - } + })); return done(undefined, fromUUID(u.userId)); } else if (user.provider === "google") { @@ -314,35 +306,28 @@ passport.serializeUser( // passport's reshape of Google's payload and is where two production // crashes came from. See `auth/googleProfile.ts`. const { email, firstNames, lastNames } = toGoogleAccount(user._json); - const fromAnonymous = user.fromAnonymous; - - let u; - - if (fromAnonymous) { - try { - u = await upgradeAnonymousUser({ - userId: toUUID(fromAnonymous), - email, - }); - - // Use name from google account - await updateUser({ - loggedInUserId: u.userId, - firstNames: firstNames ?? undefined, - lastNames, - }); - } catch (_e) { - console.log("Error upgrading anonymous user", _e); - /// ignore any error - } + const anonymousUserId = parseFromAnonymous(user.fromAnonymous); + + const upgraded = anonymousUserId + ? await upgradeAnonymousUser({ userId: anonymousUserId, email }) + : null; + + if (upgraded) { + // Use name from google account + await updateUser({ + loggedInUserId: upgraded.userId, + firstNames: firstNames ?? undefined, + lastNames, + }); } - if (!u) { - u = await findOrCreateUser({ email, firstNames, lastNames }); - } + // `email` already has an account (or there was nothing to upgrade): + // log in to that account and leave the anonymous one alone. + const u = + upgraded ?? + (await findOrCreateUser({ email, firstNames, lastNames })); return done(undefined, fromUUID(u.userId)); - // TODO: upgrade from anonymous user? } else if (user.provider === "local") { const pause1000 = function () { return new Promise((resolve, _reject) => { diff --git a/apps/api/src/query/user.ts b/apps/api/src/query/user.ts index d66e2d6bae..b127590010 100644 --- a/apps/api/src/query/user.ts +++ b/apps/api/src/query/user.ts @@ -132,6 +132,21 @@ export async function getUserInfoFromEmail( return user; } +/** + * Turn the anonymous account `userId` into a real account owned by `email`. + * + * Returns `null` when there is nothing to upgrade, which is an ordinary + * outcome rather than an error: + * + * - `email` already belongs to an account (P2002) — a returning user who + * browsed anonymously before logging in. + * - `userId` is not an anonymous account, or does not exist (P2025) — e.g. a + * second login carrying a stale `fromAnonymous` value. + * + * In both cases the caller falls back to `findOrCreateUser`, which logs them + * in to the account that owns `email`. The anonymous account is left as it is; + * any work done under it stays there and is not merged. + */ export async function upgradeAnonymousUser({ userId, email, @@ -139,12 +154,20 @@ export async function upgradeAnonymousUser({ userId: Uint8Array; email: string; }) { - const user = await prisma.users.update({ - where: { userId, isAnonymous: true }, - data: { isAnonymous: false, email }, - }); - - return user; + try { + return await prisma.users.update({ + where: { userId, isAnonymous: true }, + data: { isAnonymous: false, email }, + }); + } catch (e) { + if ( + e instanceof Prisma.PrismaClientKnownRequestError && + (e.code === "P2002" || e.code === "P2025") + ) { + return null; + } + throw e; + } } export async function updateUser({ diff --git a/apps/api/src/test/users.test.ts b/apps/api/src/test/users.test.ts index 3266b887fc..9bd78a89ac 100644 --- a/apps/api/src/test/users.test.ts +++ b/apps/api/src/test/users.test.ts @@ -1,10 +1,5 @@ import { describe, expect, test } from "vitest"; -import { - createTestAnonymousUser, - createTestUser, - fold, - setupTestContent, -} from "./utils"; +import { createTestUser, fold, setupTestContent } from "./utils"; import { fromUUID } from "../utils/uuid"; import { createStudentHandleAccounts, @@ -15,7 +10,6 @@ import { setIsAuthor, setTheme, updateUser, - upgradeAnonymousUser, } from "../query/user"; import { getMyContent } from "../query/content_list"; import { createContent } from "../query/activity"; @@ -75,23 +69,6 @@ test("findOrCreateUser finds an existing user or creates a new one", async () => expect(sameUser).toStrictEqual(user); }); -test("upgrade anonymous user", async () => { - const anonUser = await createTestAnonymousUser(); - - expect(anonUser.isAnonymous).eq(true); - - const id = Date.now().toString(); - const realEmail = `real${id}@vitest.test`; - - const upgraded = await upgradeAnonymousUser({ - userId: anonUser.userId, - email: realEmail, - }); - - expect(upgraded.isAnonymous).eq(false); - expect(upgraded.email).eq(realEmail); -}); - test("turn author mode on and off", async () => { const { userId } = await createTestUser();