fix: Correctly guard against last admin deleting their account (outline#2069)

* fix: Correctly guard against last admin deleting their account

* test

Author: tommoor

server/api/users.js

@@ -1,5 +1,6 @@
 // @flow
 import Router from "koa-router";
+import userDestroyer from "../commands/userDestroyer";
 import userInviter from "../commands/userInviter";
 import userSuspender from "../commands/userSuspender";
 import auth from "../middlewares/authentication";
@@ -232,17 +233,17 @@ router.post("users.delete", auth(), async (ctx) => {
   const { confirmation, id } = ctx.body;
   ctx.assertPresent(confirmation, "confirmation is required");
 
-  let user = ctx.state.user;
-  if (id) user = await User.findByPk(id);
-  authorize(ctx.state.user, "delete", user);
+  const actor = ctx.state.user;
+  let user = actor;
+  if (id) {
+    user = await User.findByPk(id);
+  }
 
-  await user.destroy();
-  await Event.create({
-    name: "users.delete",
-    actorId: user.id,
-    userId: user.id,
-    teamId: user.teamId,
-    data: { name: user.name },
+  authorize(actor, "delete", user);
+
+  await userDestroyer({
+    user,
+    actor,
     ip: ctx.request.ip,
   });
 

server/api/users.test.js

@@ -145,14 +145,6 @@ describe("#users.delete", () => {
     expect(res.status).toEqual(400);
   });
 
-  it("should allow deleting last admin if only user", async () => {
-    const user = await buildAdmin();
-    const res = await server.post("/api/users.delete", {
-      body: { token: user.getJwtToken(), confirmation: true },
-    });
-    expect(res.status).toEqual(200);
-  });
-
   it("should not allow deleting last admin if many users", async () => {
     const user = await buildAdmin();
     await buildUser({ teamId: user.teamId, isAdmin: false });
@@ -165,6 +157,8 @@ describe("#users.delete", () => {
 
   it("should allow deleting user account with confirmation", async () => {
     const user = await buildUser();
+    await buildUser({ teamId: user.teamId });
+
     const res = await server.post("/api/users.delete", {
       body: { token: user.getJwtToken(), confirmation: true },
     });

server/commands/userDestroyer.js

@@ -0,0 +1,67 @@
+// @flow
+import { ValidationError } from "../errors";
+import { Event, User } from "../models";
+import { Op, sequelize } from "../sequelize";
+
+export default async function userDestroyer({
+  user,
+  actor,
+  ip,
+}: {
+  user: User,
+  actor: User,
+  ip: string,
+}) {
+  const { teamId } = user;
+
+  const usersCount = await User.count({
+    where: {
+      teamId,
+    },
+  });
+
+  if (usersCount === 1) {
+    throw new ValidationError("Cannot delete last user on the team.");
+  }
+
+  if (user.isAdmin) {
+    const otherAdminsCount = await User.count({
+      where: {
+        isAdmin: true,
+        teamId,
+        id: { [Op.ne]: user.id },
+      },
+    });
+
+    if (otherAdminsCount === 0) {
+      throw new ValidationError(
+        "Cannot delete account as only admin. Please make another user admin and try again."
+      );
+    }
+  }
+
+  let transaction = await sequelize.transaction();
+  let response;
+
+  try {
+    response = await user.destroy({ transaction });
+    await Event.create(
+      {
+        name: "users.delete",
+        actorId: actor.id,
+        userId: user.id,
+        teamId,
+        data: { name: user.name },
+        ip,
+      },
+      { transaction }
+    );
+
+    await transaction.commit();
+  } catch (err) {
+    await transaction.rollback();
+    throw err;
+  }
+
+  return response;
+}

server/commands/userDestroyer.test.js

@@ -0,0 +1,83 @@
+// @flow
+import { buildUser, buildAdmin } from "../test/factories";
+import { flushdb } from "../test/support";
+import userDestroyer from "./userDestroyer";
+
+beforeEach(() => flushdb());
+
+describe("userDestroyer", () => {
+  const ip = "127.0.0.1";
+
+  it("should prevent last user from deleting account", async () => {
+    const user = await buildUser();
+
+    let error;
+
+    try {
+      await userDestroyer({
+        user,
+        actor: user,
+        ip,
+      });
+    } catch (err) {
+      error = err;
+    }
+    expect(error && error.message).toContain("Cannot delete last user");
+  });
+
+  it("should prevent last admin from deleting account", async () => {
+    const user = await buildAdmin();
+    await buildUser({ teamId: user.teamId });
+
+    let error;
+
+    try {
+      await userDestroyer({
+        user,
+        actor: user,
+        ip,
+      });
+    } catch (err) {
+      error = err;
+    }
+    expect(error && error.message).toContain("Cannot delete account");
+  });
+
+  it("should not prevent multiple admin from deleting account", async () => {
+    const actor = await buildAdmin();
+    const user = await buildAdmin({ teamId: actor.teamId });
+
+    let error;
+
+    try {
+      await userDestroyer({
+        user,
+        actor,
+        ip,
+      });
+    } catch (err) {
+      error = err;
+    }
+    expect(error).toBeFalsy();
+    expect(user.deletedAt).toBeTruthy();
+  });
+
+  it("should not prevent last non-admin from deleting account", async () => {
+    const user = await buildUser();
+    await buildUser({ teamId: user.teamId });
+
+    let error;
+
+    try {
+      await userDestroyer({
+        user,
+        actor: user,
+        ip,
+      });
+    } catch (err) {
+      error = err;
+    }
+    expect(error).toBeFalsy();
+    expect(user.deletedAt).toBeTruthy();
+  });
+});

server/models/User.js

@@ -233,22 +233,6 @@ const removeIdentifyingInfo = async (model, options) => {
   await model.save({ hooks: false, transaction: options.transaction });
 };
 
-const checkLastAdmin = async (model) => {
-  const teamId = model.teamId;
-
-  if (model.isAdmin) {
-    const userCount = await User.count({ where: { teamId } });
-    const adminCount = await User.count({ where: { isAdmin: true, teamId } });
-
-    if (userCount > 1 && adminCount <= 1) {
-      throw new ValidationError(
-        "Cannot delete account as only admin. Please transfer admin permissions to another user and try again."
-      );
-    }
-  }
-};
-
-User.beforeDestroy(checkLastAdmin);
 User.beforeDestroy(removeIdentifyingInfo);
 User.beforeSave(uploadAvatar);
 User.beforeCreate(setRandomJwtSecret);