From 2d22399bbc2097f4257806d8a638a100ae43e9f5 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 24 Apr 2021 20:52:46 -0700 Subject: [PATCH] fix: Correctly guard against last admin deleting their account (#2069) * fix: Correctly guard against last admin deleting their account * test --- server/api/users.js | 21 +++---- server/api/users.test.js | 10 +--- server/commands/userDestroyer.js | 67 +++++++++++++++++++++ server/commands/userDestroyer.test.js | 83 +++++++++++++++++++++++++++ server/models/User.js | 16 ------ 5 files changed, 163 insertions(+), 34 deletions(-) create mode 100644 server/commands/userDestroyer.js create mode 100644 server/commands/userDestroyer.test.js diff --git a/server/api/users.js b/server/api/users.js index 7701d30b..36959fae 100644 --- a/server/api/users.js +++ b/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, }); diff --git a/server/api/users.test.js b/server/api/users.test.js index 423191f5..1f062549 100644 --- a/server/api/users.test.js +++ b/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 }, }); diff --git a/server/commands/userDestroyer.js b/server/commands/userDestroyer.js new file mode 100644 index 00000000..8a1cdcd1 --- /dev/null +++ b/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; +} diff --git a/server/commands/userDestroyer.test.js b/server/commands/userDestroyer.test.js new file mode 100644 index 00000000..c5f050c1 --- /dev/null +++ b/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(); + }); +}); diff --git a/server/models/User.js b/server/models/User.js index 439e7d0e..374378f6 100644 --- a/server/models/User.js +++ b/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);