diff --git a/server/commands/userDemoter.ts b/server/commands/userDemoter.ts new file mode 100644 index 0000000000..10ac27b128 --- /dev/null +++ b/server/commands/userDemoter.ts @@ -0,0 +1,36 @@ +import { sequelize } from "@server/database/sequelize"; +import { ValidationError } from "@server/errors"; +import { Event, User } from "@server/models"; +import type { UserRole } from "@server/models/User"; +import CleanupDemotedUserTask from "@server/queues/tasks/CleanupDemotedUserTask"; + +type Props = { + user: User; + actorId: string; + to: UserRole; + ip: string; +}; + +export default async function userDemoter({ user, actorId, to, 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, + }, + { transaction } + ); + await CleanupDemotedUserTask.schedule({ userId: user.id }); + }); +} diff --git a/server/commands/userSuspender.ts b/server/commands/userSuspender.ts index a42aee4872..fd10b4ef6f 100644 --- a/server/commands/userSuspender.ts +++ b/server/commands/userSuspender.ts @@ -1,6 +1,7 @@ import { Transaction } from "sequelize"; import { sequelize } from "@server/database/sequelize"; import { User, Event, GroupUser } from "@server/models"; +import CleanupDemotedUserTask from "@server/queues/tasks/CleanupDemotedUserTask"; import { ValidationError } from "../errors"; type Props = { @@ -49,5 +50,7 @@ export default async function userSuspender({ transaction, } ); + + await CleanupDemotedUserTask.schedule({ userId: user.id }); }); } diff --git a/server/models/User.ts b/server/models/User.ts index 31f904e62a..04ca6b5d49 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -46,6 +46,11 @@ export enum UserFlag { InviteReminderSent = "inviteReminderSent", } +export enum UserRole { + Member = "member", + Viewer = "viewer", +} + @Scopes(() => ({ withAuthentications: { include: [ @@ -373,10 +378,10 @@ class User extends ParanoidModel { ); }; - demote = async (teamId: string, to: "member" | "viewer") => { + demote = async (to: UserRole, options?: SaveOptions) => { const res = await (this.constructor as typeof User).findAndCountAll({ where: { - teamId, + teamId: this.teamId, isAdmin: true, id: { [Op.ne]: this.id, @@ -387,15 +392,21 @@ class User extends ParanoidModel { if (res.count >= 1) { if (to === "member") { - return this.update({ - isAdmin: false, - isViewer: false, - }); + return this.update( + { + isAdmin: false, + isViewer: false, + }, + options + ); } else if (to === "viewer") { - return this.update({ - isAdmin: false, - isViewer: true, - }); + return this.update( + { + isAdmin: false, + isViewer: true, + }, + options + ); } return undefined; diff --git a/server/models/WebhookSubscription.ts b/server/models/WebhookSubscription.ts index 84fb1e9809..5e3d081622 100644 --- a/server/models/WebhookSubscription.ts +++ b/server/models/WebhookSubscription.ts @@ -8,6 +8,7 @@ import { DataType, IsUrl, } from "sequelize-typescript"; +import { SaveOptions } from "sequelize/types"; import { Event } from "@server/types"; import Team from "./Team"; import User from "./User"; @@ -55,7 +56,18 @@ class WebhookSubscription extends ParanoidModel { teamId: string; // methods - validForEvent = (event: Event): bool => { + + /** + * Disables the webhook subscription + * + * @param options Save options + * @returns Promise + */ + public async disable(options?: SaveOptions) { + return this.update({ enabled: false }, options); + } + + public validForEvent = (event: Event): bool => { if (this.events.length === 1 && this.events[0] === "*") { return true; } diff --git a/server/queues/tasks/CleanupDemotedUserTask.test.ts b/server/queues/tasks/CleanupDemotedUserTask.test.ts new file mode 100644 index 0000000000..2bc90742a9 --- /dev/null +++ b/server/queues/tasks/CleanupDemotedUserTask.test.ts @@ -0,0 +1,102 @@ +import { ApiKey } from "@server/models"; +import { + buildUser, + buildApiKey, + buildAdmin, + buildWebhookSubscription, + buildViewer, +} from "@server/test/factories"; +import { flushdb } from "@server/test/support"; +import CleanupDemotedUserTask from "./CleanupDemotedUserTask"; + +beforeEach(() => flushdb()); + +describe("CleanupDemotedUserTask", () => { + it("should delete api keys for suspended user", async () => { + const admin = await buildAdmin(); + const user = await buildUser({ + teamId: admin.teamId, + suspendedAt: new Date(), + suspendedById: admin.id, + }); + const apiKey = await buildApiKey({ + userId: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + expect(await ApiKey.findByPk(apiKey.id)).toBeNull(); + }); + + it("should delete api keys for viewer", async () => { + const user = await buildViewer(); + const apiKey = await buildApiKey({ + userId: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + expect(await ApiKey.findByPk(apiKey.id)).toBeNull(); + }); + + it("should retain api keys for member", async () => { + const user = await buildUser(); + const apiKey = await buildApiKey({ + userId: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + expect(await ApiKey.findByPk(apiKey.id)).toBeTruthy(); + }); + + it("should disable webhooks for suspended user", async () => { + const admin = await buildAdmin(); + const user = await buildUser({ + teamId: admin.teamId, + suspendedAt: new Date(), + suspendedById: admin.id, + }); + const webhook = await buildWebhookSubscription({ + teamId: user.teamId, + createdById: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + + await webhook.reload(); + expect(webhook.enabled).toEqual(false); + }); + + it("should disable webhooks for member", async () => { + const admin = await buildAdmin(); + const user = await buildUser({ + teamId: admin.teamId, + }); + const webhook = await buildWebhookSubscription({ + teamId: user.teamId, + createdById: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + + await webhook.reload(); + expect(webhook.enabled).toEqual(false); + }); + + it("should retain webhooks for admin", async () => { + const user = await buildAdmin(); + const webhook = await buildWebhookSubscription({ + teamId: user.teamId, + createdById: user.id, + }); + + const task = new CleanupDemotedUserTask(); + await task.perform({ userId: user.id }); + + await webhook.reload(); + expect(webhook.enabled).toEqual(true); + }); +}); diff --git a/server/queues/tasks/CleanupDemotedUserTask.ts b/server/queues/tasks/CleanupDemotedUserTask.ts new file mode 100644 index 0000000000..bf70eb39a9 --- /dev/null +++ b/server/queues/tasks/CleanupDemotedUserTask.ts @@ -0,0 +1,57 @@ +import { sequelize } from "@server/database/sequelize"; +import Logger from "@server/logging/Logger"; +import { WebhookSubscription, ApiKey, User } from "@server/models"; +import BaseTask from "./BaseTask"; + +type Props = { + userId: string; +}; + +/** + * Task to disable mechanisms for exporting data from a suspended or demoted user, + * currently this is done by destroying associated Api Keys and disabling webhooks. + */ +export default class CleanupDemotedUserTask extends BaseTask { + public async perform(props: Props) { + await sequelize.transaction(async (transaction) => { + const user = await User.findByPk(props.userId, { rejectOnEmpty: true }); + + if (user.isSuspended || !user.isAdmin) { + const subscriptions = await WebhookSubscription.findAll({ + where: { + createdById: props.userId, + enabled: true, + }, + transaction, + lock: transaction.LOCK.UPDATE, + }); + await Promise.all( + subscriptions.map((subscription) => + subscription.disable({ transaction }) + ) + ); + Logger.info( + "task", + `Disabled ${subscriptions.length} webhooks for user ${props.userId}` + ); + } + + if (user.isSuspended || user.isViewer) { + const apiKeys = await ApiKey.findAll({ + where: { + userId: props.userId, + }, + transaction, + lock: transaction.LOCK.UPDATE, + }); + await Promise.all( + apiKeys.map((apiKey) => apiKey.destroy({ transaction })) + ); + Logger.info( + "task", + `Destroyed ${apiKeys.length} api keys for user ${props.userId}` + ); + } + }); + } +} diff --git a/server/queues/tasks/DeliverWebhookTask.ts b/server/queues/tasks/DeliverWebhookTask.ts index 68a9bf15d1..a89382b167 100644 --- a/server/queues/tasks/DeliverWebhookTask.ts +++ b/server/queues/tasks/DeliverWebhookTask.ts @@ -586,7 +586,7 @@ export default class DeliverWebhookTask extends BaseTask { if (recentDeliveries.length === 25 && allFailed) { // If the last 25 deliveries failed, disable the subscription - await subscription.update({ enabled: false }); + await subscription.disable(); // Send an email to the creator of the webhook to let them know const [createdBy, team] = await Promise.all([ diff --git a/server/routes/api/__snapshots__/users.test.ts.snap b/server/routes/api/__snapshots__/users.test.ts.snap index b40811bfcd..a02b0473e4 100644 --- a/server/routes/api/__snapshots__/users.test.ts.snap +++ b/server/routes/api/__snapshots__/users.test.ts.snap @@ -166,10 +166,10 @@ Object { } `; -exports[`#users.demote should not demote admins if only one available 1`] = ` +exports[`#users.demote should not allow demoting self 1`] = ` Object { "error": "validation_error", - "message": "At least one admin is required", + "message": "Unable to demote the current user", "ok": false, "status": 400, } diff --git a/server/routes/api/users.test.ts b/server/routes/api/users.test.ts index 1a2356a0f9..b0906874d4 100644 --- a/server/routes/api/users.test.ts +++ b/server/routes/api/users.test.ts @@ -483,7 +483,7 @@ describe("#users.demote", () => { expect(body).toMatchSnapshot(); }); - it("should not demote admins if only one available", async () => { + it("should not allow demoting self", async () => { const admin = await buildAdmin(); const res = await server.post("/api/users.demote", { body: { diff --git a/server/routes/api/users.ts b/server/routes/api/users.ts index 3274cedb9c..5438114f2e 100644 --- a/server/routes/api/users.ts +++ b/server/routes/api/users.ts @@ -1,5 +1,6 @@ import Router from "koa-router"; import { Op, WhereOptions } from "sequelize"; +import userDemoter from "@server/commands/userDemoter"; import userDestroyer from "@server/commands/userDestroyer"; import userInviter from "@server/commands/userInviter"; import userSuspender from "@server/commands/userSuspender"; @@ -10,7 +11,7 @@ import { ValidationError } from "@server/errors"; import logger from "@server/logging/Logger"; import auth from "@server/middlewares/authentication"; import { Event, User, Team } from "@server/models"; -import { UserFlag } from "@server/models/User"; +import { UserFlag, UserRole } from "@server/models/User"; import { can, authorize } from "@server/policies"; import { presentUser, presentPolicies } from "@server/presenters"; import { @@ -223,23 +224,21 @@ router.post("users.promote", auth(), async (ctx) => { router.post("users.demote", auth(), async (ctx) => { const userId = ctx.body.id; - const teamId = ctx.state.user.teamId; let { to } = ctx.body; - const actor = ctx.state.user; + const actor = ctx.state.user as User; assertPresent(userId, "id is required"); - to = to === "viewer" ? "viewer" : "member"; - const user = await User.findByPk(userId); + + to = (to === "viewer" ? "viewer" : "member") as UserRole; + + const user = await User.findByPk(userId, { + rejectOnEmpty: true, + }); authorize(actor, "demote", user); - await user.demote(teamId, to); - await Event.create({ - name: "users.demote", + await userDemoter({ + to, + user, actorId: actor.id, - userId, - teamId, - data: { - name: user.name, - }, ip: ctx.request.ip, }); const includeDetails = can(actor, "readDetails", user); @@ -256,7 +255,9 @@ router.post("users.suspend", auth(), async (ctx) => { const userId = ctx.body.id; const actor = ctx.state.user; assertPresent(userId, "id is required"); - const user = await User.findByPk(userId); + const user = await User.findByPk(userId, { + rejectOnEmpty: true, + }); authorize(actor, "suspend", user); await userSuspender({ diff --git a/server/test/factories.ts b/server/test/factories.ts index 211defb009..f536e5e1ef 100644 --- a/server/test/factories.ts +++ b/server/test/factories.ts @@ -16,6 +16,7 @@ import { FileOperation, WebhookSubscription, WebhookDelivery, + ApiKey, } from "@server/models"; import { FileOperationState, @@ -24,6 +25,18 @@ import { let count = 1; +export async function buildApiKey(overrides: Partial = {}) { + if (!overrides.userId) { + const user = await buildUser(); + overrides.userId = user.id; + } + + return ApiKey.create({ + name: "My API Key", + ...overrides, + }); +} + export async function buildShare(overrides: Partial = {}) { if (!overrides.teamId) { const team = await buildTeam();