feat: Cleanup api keys and webhooks for suspended users (#3756)

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 });
+  });
+}

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 });
   });
 }

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<User>) => {
     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({
+        return this.update(
-          isAdmin: false,
+          {
-          isViewer: false,
+            isAdmin: false,
-        });
+            isViewer: false,
+          },
+          options
+        );
       } else if (to === "viewer") {
-        return this.update({
+        return this.update(
-          isAdmin: false,
+          {
-          isViewer: true,
+            isAdmin: false,
-        });
+            isViewer: true,
+          },
+          options
+        );
       }
 
       return undefined;

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<void>
+   */
+  public async disable(options?: SaveOptions<WebhookSubscription>) {
+    return this.update({ enabled: false }, options);
+  }
+
+  public validForEvent = (event: Event): bool => {
     if (this.events.length === 1 && this.events[0] === "*") {
       return true;
     }

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);
+  });
+});

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<Props> {
+  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}`
+        );
+      }
+    });
+  }
+}

server/queues/tasks/DeliverWebhookTask.ts

@@ -586,7 +586,7 @@ export default class DeliverWebhookTask extends BaseTask<Props> {
 
     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([

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,
 }

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: {

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 userDemoter({
-  await Event.create({
+    to,
-    name: "users.demote",
+    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({

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<ApiKey> = {}) {
+  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<Share> = {}) {
   if (!overrides.teamId) {
     const team = await buildTeam();