impr: error handling on user deletion (@fehmer) (#6363)

!nuf

backend/__tests__/api/controllers/user.spec.ts

@@ -29,7 +29,7 @@ import {
 import { randomUUID } from "node:crypto";
 import _ from "lodash";
 import { MonkeyMail, UserStreak } from "@monkeytype/contracts/schemas/users";
-import { isFirebaseError } from "../../../src/utils/error";
+import MonkeyError, { isFirebaseError } from "../../../src/utils/error";
 import { LeaderboardEntry } from "@monkeytype/contracts/schemas/leaderboards";
 import * as WeeklyXpLeaderboard from "../../../src/services/weekly-xp-leaderboard";
 
@@ -704,6 +704,141 @@ describe("user controller test", () => {
         (await configuration).leaderboards.weeklyXp
       );
     });
+
+    it("should not fail if userInfo cannot be found", async () => {
+      //GIVEN
+      getUserMock.mockRejectedValue(new MonkeyError(404, "user not found"));
+
+      //WHEN
+      await mockApp
+        .delete("/users/")
+        .set("Authorization", `Bearer ${uid}`)
+        .expect(200);
+
+      //THEN
+      expect(blocklistAddMock).not.toHaveBeenCalled();
+
+      expect(deleteUserMock).toHaveBeenCalledWith(uid);
+      expect(firebaseDeleteUserMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllApeKeysMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllPresetsMock).toHaveBeenCalledWith(uid);
+      expect(deleteConfigMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllResultMock).toHaveBeenCalledWith(uid);
+      expect(purgeUserFromDailyLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).dailyLeaderboards
+      );
+      expect(purgeUserFromXpLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).leaderboards.weeklyXp
+      );
+    });
+
+    it("should fail for unknown error from UserDal", async () => {
+      //GIVEN
+      getUserMock.mockRejectedValue(new Error("oops"));
+
+      //WHEN
+      await mockApp
+        .delete("/users/")
+        .set("Authorization", `Bearer ${uid}`)
+        .expect(500);
+
+      //THEN
+      expect(blocklistAddMock).not.toHaveBeenCalled();
+      expect(deleteUserMock).not.toHaveBeenCalledWith(uid);
+      expect(firebaseDeleteUserMock).not.toHaveBeenCalledWith(uid);
+      expect(deleteAllApeKeysMock).not.toHaveBeenCalledWith(uid);
+      expect(deleteAllPresetsMock).not.toHaveBeenCalledWith(uid);
+      expect(deleteConfigMock).not.toHaveBeenCalledWith(uid);
+      expect(deleteAllResultMock).not.toHaveBeenCalledWith(uid);
+      expect(purgeUserFromDailyLeaderboardsMock).not.toHaveBeenCalledWith(
+        uid,
+        (await configuration).dailyLeaderboards
+      );
+      expect(purgeUserFromXpLeaderboardsMock).not.toHaveBeenCalledWith(
+        uid,
+        (await configuration).leaderboards.weeklyXp
+      );
+    });
+    it("should not fail if firebase user cannot be found", async () => {
+      //GIVEN
+      const user = {
+        uid,
+        name: "name",
+        email: "email",
+        discordId: "discordId",
+      } as Partial<UserDal.DBUser> as UserDal.DBUser;
+      getUserMock.mockResolvedValue(user);
+      firebaseDeleteUserMock.mockRejectedValue({
+        code: "user-not-found",
+        codePrefix: "auth",
+        errorInfo: { code: "auth/user-not-found", message: "user not found" },
+      });
+
+      //WHEN
+      await mockApp
+        .delete("/users/")
+        .set("Authorization", `Bearer ${uid}`)
+        .expect(200);
+
+      //THEN
+      expect(blocklistAddMock).not.toHaveBeenCalled();
+
+      expect(deleteUserMock).toHaveBeenCalledWith(uid);
+      expect(firebaseDeleteUserMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllApeKeysMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllPresetsMock).toHaveBeenCalledWith(uid);
+      expect(deleteConfigMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllResultMock).toHaveBeenCalledWith(uid);
+      expect(purgeUserFromDailyLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).dailyLeaderboards
+      );
+      expect(purgeUserFromXpLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).leaderboards.weeklyXp
+      );
+    });
+
+    it("should fail for unknown error from firebase", async () => {
+      //GIVEN
+      const user = {
+        uid,
+        name: "name",
+        email: "email",
+        discordId: "discordId",
+      } as Partial<UserDal.DBUser> as UserDal.DBUser;
+      getUserMock.mockResolvedValue(user);
+      firebaseDeleteUserMock.mockRejectedValue({
+        code: "unknown",
+        codePrefix: "auth",
+        errorInfo: { code: "auth/unknown", message: "unknown" },
+      });
+
+      //WHEN
+      await mockApp
+        .delete("/users/")
+        .set("Authorization", `Bearer ${uid}`)
+        .expect(500);
+
+      //THEN
+      expect(blocklistAddMock).not.toHaveBeenCalled();
+      expect(deleteUserMock).toHaveBeenCalledWith(uid);
+      expect(firebaseDeleteUserMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllApeKeysMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllPresetsMock).toHaveBeenCalledWith(uid);
+      expect(deleteConfigMock).toHaveBeenCalledWith(uid);
+      expect(deleteAllResultMock).toHaveBeenCalledWith(uid);
+      expect(purgeUserFromDailyLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).dailyLeaderboards
+      );
+      expect(purgeUserFromXpLeaderboardsMock).toHaveBeenCalledWith(
+        uid,
+        (await configuration).leaderboards.weeklyXp
+      );
+    });
   });
   describe("resetUser", () => {
     const getPartialUserMock = vi.spyOn(UserDal, "getPartialUser");

backend/src/api/controllers/user.ts

@@ -259,14 +259,26 @@ export async function sendForgotPasswordEmail(
 export async function deleteUser(req: MonkeyRequest): Promise<MonkeyResponse> {
   const { uid } = req.ctx.decodedToken;
 
-  const userInfo = await UserDAL.getPartialUser(uid, "delete user", [
-    "banned",
-    "name",
-    "email",
-    "discordId",
-  ]);
+  let userInfo:
+    | Pick<UserDAL.DBUser, "banned" | "name" | "email" | "discordId">
+    | undefined;
 
-  if (userInfo.banned === true) {
+  try {
+    userInfo = await UserDAL.getPartialUser(uid, "delete user", [
+      "banned",
+      "name",
+      "email",
+      "discordId",
+    ]);
+  } catch (e) {
+    if (e instanceof MonkeyError && e.status === 404) {
+      //userinfo was already deleted. We ignore this and still try to remove the  other data
+    } else {
+      throw e;
+    }
+  }
+
+  if (userInfo?.banned === true) {
     await BlocklistDal.add(userInfo);
   }
 
@@ -288,12 +300,20 @@ export async function deleteUser(req: MonkeyRequest): Promise<MonkeyResponse> {
     ),
   ]);
 
-  //delete user from
-  await AuthUtil.deleteUser(uid);
+  try {
+    //delete user from firebase
+    await AuthUtil.deleteUser(uid);
+  } catch (e) {
+    if (isFirebaseError(e) && e.errorInfo.code === "auth/user-not-found") {
+      //user was already deleted, ok to ignore
+    } else {
+      throw e;
+    }
+  }
 
   void addImportantLog(
     "user_deleted",
-    `${userInfo.email} ${userInfo.name}`,
+    `${userInfo?.email} ${userInfo?.name}`,
     uid
   );