fix(api): stop logging an error when an anonymous user logs in to an existing account

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);
+  });
+});

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();
+  });
+});

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;
+  }
+}

apps/api/src/auth/index.ts

@@ -4,13 +4,15 @@
 //                        `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.
 
 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,

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,65 +284,50 @@ 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") {
         // Validated here rather than read off `profile.name`, which is
         // 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) => {

apps/api/src/query/user.ts

@@ -132,19 +132,42 @@ 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,
 }: {
   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({

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();