From 9602d09964199555099316e1676fd4736bcdeba2 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 9 Sep 2023 23:26:22 -0400 Subject: [PATCH] fix: Add locks to user mutations (#5805) --- server/commands/userDemoter.ts | 41 ++++++++++-------- server/commands/userSuspender.ts | 65 ++++++++++++++-------------- server/commands/userUnsuspender.ts | 47 ++++++++++----------- server/models/User.ts | 14 +++--- server/routes/api/users/users.ts | 68 +++++++++++++++++++++++------- 5 files changed, 140 insertions(+), 95 deletions(-) diff --git a/server/commands/userDemoter.ts b/server/commands/userDemoter.ts index 8ecb7b6dc..e841e50df 100644 --- a/server/commands/userDemoter.ts +++ b/server/commands/userDemoter.ts @@ -1,36 +1,41 @@ +import { Transaction } from "sequelize"; import { UserRole } from "@shared/types"; import { ValidationError } from "@server/errors"; import { Event, User } from "@server/models"; import CleanupDemotedUserTask from "@server/queues/tasks/CleanupDemotedUserTask"; -import { sequelize } from "@server/storage/database"; type Props = { user: User; actorId: string; to: UserRole; + transaction?: Transaction; ip: string; }; -export default async function userDemoter({ user, actorId, to, ip }: Props) { +export default async function userDemoter({ + user, + actorId, + to, + transaction, + ip, +}: Props) { if (user.id === actorId) { throw ValidationError("Unable to demote the current user"); } - return sequelize.transaction(async (transaction) => { - await user.demote(to, { transaction }); - await Event.create( - { - name: "users.demote", - actorId, - userId: user.id, - teamId: user.teamId, - data: { - name: user.name, - }, - ip, + await user.demote(to, { transaction }); + await Event.create( + { + name: "users.demote", + actorId, + userId: user.id, + teamId: user.teamId, + data: { + name: user.name, }, - { transaction } - ); - await CleanupDemotedUserTask.schedule({ userId: user.id }); - }); + ip, + }, + { transaction } + ); + await CleanupDemotedUserTask.schedule({ userId: user.id }); } diff --git a/server/commands/userSuspender.ts b/server/commands/userSuspender.ts index 03c7b5c54..023a7f3f9 100644 --- a/server/commands/userSuspender.ts +++ b/server/commands/userSuspender.ts @@ -1,12 +1,12 @@ import { Transaction } from "sequelize"; import { User, Event, GroupUser } from "@server/models"; import CleanupDemotedUserTask from "@server/queues/tasks/CleanupDemotedUserTask"; -import { sequelize } from "@server/storage/database"; import { ValidationError } from "../errors"; type Props = { user: User; actorId: string; + transaction?: Transaction; ip: string; }; @@ -17,44 +17,43 @@ type Props = { export default async function userSuspender({ user, actorId, + transaction, ip, }: Props): Promise { if (user.id === actorId) { throw ValidationError("Unable to suspend the current user"); } - await sequelize.transaction(async (transaction: Transaction) => { - await user.update( - { - suspendedById: actorId, - suspendedAt: new Date(), - }, - { - transaction, - } - ); - await GroupUser.destroy({ - where: { - userId: user.id, - }, + await user.update( + { + suspendedById: actorId, + suspendedAt: new Date(), + }, + { transaction, - }); - await Event.create( - { - name: "users.suspend", - actorId, - userId: user.id, - teamId: user.teamId, - data: { - name: user.name, - }, - ip, - }, - { - transaction, - } - ); - - await CleanupDemotedUserTask.schedule({ userId: user.id }); + } + ); + await GroupUser.destroy({ + where: { + userId: user.id, + }, + transaction, }); + await Event.create( + { + name: "users.suspend", + actorId, + userId: user.id, + teamId: user.teamId, + data: { + name: user.name, + }, + ip, + }, + { + transaction, + } + ); + + await CleanupDemotedUserTask.schedule({ userId: user.id }); } diff --git a/server/commands/userUnsuspender.ts b/server/commands/userUnsuspender.ts index ba13a5273..a3b14c3f9 100644 --- a/server/commands/userUnsuspender.ts +++ b/server/commands/userUnsuspender.ts @@ -1,11 +1,11 @@ import { Transaction } from "sequelize"; import { User, Event } from "@server/models"; -import { sequelize } from "@server/storage/database"; import { ValidationError } from "../errors"; type Props = { user: User; actorId: string; + transaction?: Transaction; ip: string; }; @@ -16,34 +16,33 @@ type Props = { export default async function userUnsuspender({ user, actorId, + transaction, ip, }: Props): Promise { if (user.id === actorId) { throw ValidationError("Unable to unsuspend the current user"); } - await sequelize.transaction(async (transaction: Transaction) => { - await user.update( - { - suspendedById: null, - suspendedAt: null, + await user.update( + { + suspendedById: null, + suspendedAt: null, + }, + { transaction } + ); + await Event.create( + { + name: "users.activate", + actorId, + userId: user.id, + teamId: user.teamId, + data: { + name: user.name, }, - { transaction } - ); - await Event.create( - { - name: "users.activate", - actorId, - userId: user.id, - teamId: user.teamId, - data: { - name: user.name, - }, - ip, - }, - { - transaction, - } - ); - }); + ip, + }, + { + transaction, + } + ); } diff --git a/server/models/User.ts b/server/models/User.ts index 49feeaa22..020e4058b 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -525,6 +525,7 @@ class User extends ParanoidModel { }, }, limit: 1, + ...options, }); if (res.count >= 1) { @@ -563,11 +564,14 @@ class User extends ParanoidModel { } }; - promote = () => - this.update({ - isAdmin: true, - isViewer: false, - }); + promote = (options?: SaveOptions) => + this.update( + { + isAdmin: true, + isViewer: false, + }, + options + ); // hooks diff --git a/server/routes/api/users/users.ts b/server/routes/api/users/users.ts index 89a9650da..c020b8444 100644 --- a/server/routes/api/users/users.ts +++ b/server/routes/api/users/users.ts @@ -187,8 +187,8 @@ router.post( router.post( "users.update", auth(), - transaction(), validate(T.UsersUpdateSchema), + transaction(), async (ctx: APIContext) => { const { auth, transaction } = ctx.state; const actor = auth.user; @@ -196,7 +196,11 @@ router.post( let user: User | null = actor; if (id) { - user = await User.findByPk(id); + user = await User.findByPk(id, { + rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, + }); } authorize(actor, "update", user); const includeDetails = can(actor, "readDetails", user); @@ -240,24 +244,37 @@ router.post( "users.promote", auth(), validate(T.UsersPromoteSchema), + transaction(), async (ctx: APIContext) => { + const { transaction } = ctx.state; const userId = ctx.input.body.id; const actor = ctx.state.auth.user; const teamId = actor.teamId; - const user = await User.findByPk(userId); + const user = await User.findByPk(userId, { + rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, + }); authorize(actor, "promote", user); - await user.promote(); - await Event.create({ - name: "users.promote", - actorId: actor.id, - userId, - teamId, - data: { - name: user.name, - }, - ip: ctx.request.ip, + await user.promote({ + transaction, }); + await Event.create( + { + name: "users.promote", + actorId: actor.id, + userId, + teamId, + data: { + name: user.name, + }, + ip: ctx.request.ip, + }, + { + transaction, + } + ); const includeDetails = can(actor, "readDetails", user); ctx.body = { @@ -273,20 +290,29 @@ router.post( "users.demote", auth(), validate(T.UsersDemoteSchema), + transaction(), async (ctx: APIContext) => { - const userId = ctx.input.body.id; - const to = ctx.input.body.to; + const { transaction } = ctx.state; + const { to, id: userId } = ctx.input.body; const actor = ctx.state.auth.user; const user = await User.findByPk(userId, { rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, }); authorize(actor, "demote", user); + await Team.findByPk(user.teamId, { + transaction, + lock: transaction.LOCK.UPDATE, + }); + await userDemoter({ to, user, actorId: actor.id, + transaction, ip: ctx.request.ip, }); const includeDetails = can(actor, "readDetails", user); @@ -304,11 +330,15 @@ router.post( "users.suspend", auth(), validate(T.UsersSuspendSchema), + transaction(), async (ctx: APIContext) => { + const { transaction } = ctx.state; const userId = ctx.input.body.id; const actor = ctx.state.auth.user; const user = await User.findByPk(userId, { rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, }); authorize(actor, "suspend", user); @@ -316,6 +346,7 @@ router.post( user, actorId: actor.id, ip: ctx.request.ip, + transaction, }); const includeDetails = can(actor, "readDetails", user); @@ -332,17 +363,22 @@ router.post( "users.activate", auth(), validate(T.UsersActivateSchema), + transaction(), async (ctx: APIContext) => { + const { transaction } = ctx.state; const userId = ctx.input.body.id; const actor = ctx.state.auth.user; const user = await User.findByPk(userId, { rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, }); authorize(actor, "activate", user); await userUnsuspender({ user, actorId: actor.id, + transaction, ip: ctx.request.ip, }); const includeDetails = can(actor, "readDetails", user); @@ -465,6 +501,8 @@ router.post( if (id) { user = await User.findByPk(id, { rejectOnEmpty: true, + transaction, + lock: transaction.LOCK.UPDATE, }); } else { user = actor;