Add updateRole endpoint (#6771)

server/commands/userDemoter.test.ts

@@ -1,33 +0,0 @@
-import { CollectionPermission, UserRole } from "@shared/types";
-import { UserMembership } from "@server/models";
-import { buildUser, buildAdmin, buildCollection } from "@server/test/factories";
-import userDemoter from "./userDemoter";
-
-describe("userDemoter", () => {
-  const ip = "127.0.0.1";
-
-  it("should change role and associated collection permissions", async () => {
-    const admin = await buildAdmin();
-    const user = await buildUser({ teamId: admin.teamId });
-    const collection = await buildCollection({ teamId: admin.teamId });
-
-    const membership = await UserMembership.create({
-      createdById: admin.id,
-      userId: user.id,
-      collectionId: collection.id,
-      permission: CollectionPermission.ReadWrite,
-    });
-
-    await userDemoter({
-      user,
-      actorId: admin.id,
-      to: UserRole.Viewer,
-      ip,
-    });
-
-    expect(user.isViewer).toEqual(true);
-
-    await membership.reload();
-    expect(membership.permission).toEqual(CollectionPermission.Read);
-  });
-});

server/commands/userDemoter.ts

@@ -1,41 +0,0 @@
-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";
-
-type Props = {
-  user: User;
-  actorId: string;
-  to: UserRole;
-  transaction?: Transaction;
-  ip: string;
-};
-
-export default async function userDemoter({
-  user,
-  actorId,
-  to,
-  transaction,
-  ip,
-}: Props) {
-  if (user.id === actorId) {
-    throw ValidationError("Unable to demote the current user");
-  }
-
-  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/models/User.ts

@@ -10,8 +10,8 @@ import {
   FindOptions,
   InferAttributes,
   InferCreationAttributes,
-  InstanceUpdateOptions,
 } from "sequelize";
+import { type InstanceUpdateOptions } from "sequelize";
 import {
   Table,
   Column,
@@ -29,6 +29,7 @@ import {
   IsDate,
   AllowNull,
   AfterUpdate,
+  BeforeUpdate,
 } from "sequelize-typescript";
 import { UserPreferenceDefaults } from "@shared/constants";
 import { languages } from "@shared/i18n";
@@ -42,9 +43,9 @@ import {
   UserRole,
   DocumentPermission,
 } from "@shared/types";
+import { UserRoleHelper } from "@shared/utils/UserRoleHelper";
 import { stringToColor } from "@shared/utils/color";
 import env from "@server/env";
-import Model from "@server/models/base/Model";
 import DeleteAttachmentTask from "@server/queues/tasks/DeleteAttachmentTask";
 import parseAttachmentIds from "@server/utils/parseAttachmentIds";
 import { ValidationError } from "../errors";
@@ -567,51 +568,6 @@ class User extends ParanoidModel<
       ],
     });
 
-  demote: (
-    to: UserRole,
-    options?: InstanceUpdateOptions<InferAttributes<Model>>
-  ) => Promise<void> = async (to, options) => {
-    const res = await (this.constructor as typeof User).findAndCountAll({
-      where: {
-        teamId: this.teamId,
-        role: UserRole.Admin,
-        id: {
-          [Op.ne]: this.id,
-        },
-      },
-      limit: 1,
-      ...options,
-    });
-
-    if (res.count >= 1) {
-      if (to === UserRole.Member) {
-        await this.update({ role: to }, options);
-      } else if (to === UserRole.Viewer) {
-        await this.update({ role: to }, options);
-        await UserMembership.update(
-          {
-            permission: CollectionPermission.Read,
-          },
-          {
-            ...options,
-            where: {
-              userId: this.id,
-            },
-          }
-        );
-      }
-
-      return undefined;
-    } else {
-      throw ValidationError("At least one admin is required");
-    }
-  };
-
-  promote: (
-    options?: InstanceUpdateOptions<InferAttributes<User>>
-  ) => Promise<User> = (options) =>
-    this.update({ role: UserRole.Admin }, options);
-
   // hooks
 
   @BeforeDestroy
@@ -638,6 +594,62 @@ class User extends ParanoidModel<
     model.jwtSecret = crypto.randomBytes(64).toString("hex");
   };
 
+  @BeforeUpdate
+  static async checkRoleChange(
+    model: User,
+    options: InstanceUpdateOptions<InferAttributes<User>>
+  ) {
+    const previousRole = model.previous("role");
+
+    if (
+      model.changed("role") &&
+      previousRole === UserRole.Admin &&
+      UserRoleHelper.isRoleLower(model.role, UserRole.Admin)
+    ) {
+      const { count } = await this.findAndCountAll({
+        where: {
+          teamId: model.teamId,
+          role: UserRole.Admin,
+          id: {
+            [Op.ne]: model.id,
+          },
+        },
+        limit: 1,
+        transaction: options.transaction,
+      });
+      if (count === 0) {
+        throw ValidationError("At least one admin is required");
+      }
+    }
+  }
+
+  @AfterUpdate
+  static async updateMembershipPermissions(
+    model: User,
+    options: InstanceUpdateOptions<InferAttributes<User>>
+  ) {
+    const previousRole = model.previous("role");
+
+    if (
+      previousRole &&
+      model.changed("role") &&
+      UserRoleHelper.isRoleLower(model.role, UserRole.Member) &&
+      UserRoleHelper.isRoleHigher(previousRole, UserRole.Viewer)
+    ) {
+      await UserMembership.update(
+        {
+          permission: CollectionPermission.Read,
+        },
+        {
+          transaction: options.transaction,
+          where: {
+            userId: model.id,
+          },
+        }
+      );
+    }
+  }
+
   @AfterUpdate
   static deletePreviousAvatar = async (model: User) => {
     const previousAvatarUrl = model.previous("avatarUrl");

server/policies/apiKey.ts

@@ -8,7 +8,8 @@ allow(User, "createApiKey", Team, (actor, team) =>
     isTeamModel(actor, team),
     isTeamMutable(actor),
     !actor.isViewer,
-    !actor.isGuest
+    !actor.isGuest,
+    !actor.isSuspended
   )
 );
 

server/policies/webhookSubscription.ts

@@ -6,7 +6,8 @@ allow(User, "createWebhookSubscription", Team, (actor, team) =>
   and(
     //
     isTeamAdmin(actor, team),
-    isTeamMutable(actor)
+    isTeamMutable(actor),
+    !actor.isSuspended
   )
 );
 

server/queues/processors/UserDeletedProcessor.ts

@@ -10,7 +10,7 @@ import { sequelize } from "@server/storage/database";
 import { Event as TEvent, UserEvent } from "@server/types";
 import BaseProcessor from "./BaseProcessor";
 
-export default class UsersDeletedProcessor extends BaseProcessor {
+export default class UserDeletedProcessor extends BaseProcessor {
   static applicableEvents: TEvent["name"][] = ["users.delete"];
 
   async perform(event: UserEvent) {

server/queues/processors/UserDemotedProcessor.ts

@@ -0,0 +1,11 @@
+import { Event as TEvent, UserEvent } from "@server/types";
+import CleanupDemotedUserTask from "../tasks/CleanupDemotedUserTask";
+import BaseProcessor from "./BaseProcessor";
+
+export default class UserDemotedProcessor extends BaseProcessor {
+  static applicableEvents: TEvent["name"][] = ["users.demote"];
+
+  async perform(event: UserEvent) {
+    await CleanupDemotedUserTask.schedule({ userId: event.userId });
+  }
+}

server/queues/tasks/CleanupDemotedUserTask.ts

@@ -1,5 +1,6 @@
 import Logger from "@server/logging/Logger";
 import { WebhookSubscription, ApiKey, User } from "@server/models";
+import { cannot } from "@server/policies";
 import { sequelize } from "@server/storage/database";
 import BaseTask, { TaskPriority } from "./BaseTask";
 
@@ -13,10 +14,12 @@ type Props = {
  */
 export default class CleanupDemotedUserTask extends BaseTask<Props> {
   public async perform(props: Props) {
-    await sequelize.transaction(async (transaction) => {
+    const user = await User.scope("withTeam").findByPk(props.userId, {
-      const user = await User.findByPk(props.userId, { rejectOnEmpty: true });
+      rejectOnEmpty: true,
+    });
 
-      if (user.isSuspended || !user.isAdmin) {
+    await sequelize.transaction(async (transaction) => {
+      if (cannot(user, "createWebhookSubscription", user.team)) {
         const subscriptions = await WebhookSubscription.findAll({
           where: {
             createdById: props.userId,
@@ -36,7 +39,7 @@ export default class CleanupDemotedUserTask extends BaseTask<Props> {
         );
       }
 
-      if (user.isSuspended || user.isViewer) {
+      if (cannot(user, "createApiKey", user.team)) {
         const apiKeys = await ApiKey.findAll({
           where: {
             userId: props.userId,

server/routes/api/users/__snapshots__/users.test.ts.snap

@@ -21,7 +21,7 @@ exports[`#users.delete should require authentication 1`] = `
 exports[`#users.demote should not allow demoting self 1`] = `
 {
   "error": "validation_error",
-  "message": "Unable to demote the current user",
+  "message": "You cannot change your own role",
   "ok": false,
   "status": 400,
 }
@@ -30,7 +30,7 @@ exports[`#users.demote should not allow demoting self 1`] = `
 exports[`#users.demote should require admin 1`] = `
 {
   "error": "authorization_error",
-  "message": "Authorization error",
+  "message": "Admin role required",
   "ok": false,
   "status": 403,
 }
@@ -39,7 +39,7 @@ exports[`#users.demote should require admin 1`] = `
 exports[`#users.promote should require admin 1`] = `
 {
   "error": "authorization_error",
-  "message": "Authorization error",
+  "message": "Admin role required",
   "ok": false,
   "status": 403,
 }

server/routes/api/users/schema.ts

@@ -110,6 +110,14 @@ export const UsersActivateSchema = BaseSchema.extend({
 
 export type UsersActivateReq = z.infer<typeof UsersActivateSchema>;
 
+export const UsersChangeRoleSchema = BaseSchema.extend({
+  body: BaseIdSchema.extend({
+    role: z.nativeEnum(UserRole),
+  }),
+});
+
+export type UsersChangeRoleReq = z.infer<typeof UsersChangeRoleSchema>;
+
 export const UsersPromoteSchema = BaseSchema.extend({
   body: BaseIdSchema,
 });
@@ -117,8 +125,7 @@ export const UsersPromoteSchema = BaseSchema.extend({
 export type UsersPromoteReq = z.infer<typeof UsersPromoteSchema>;
 
 export const UsersDemoteSchema = BaseSchema.extend({
-  body: z.object({
+  body: BaseIdSchema.extend({
-    id: z.string().uuid(),
     to: z.nativeEnum(UserRole).default(UserRole.Member),
   }),
 });

server/routes/api/users/users.test.ts

@@ -555,6 +555,55 @@ describe("#users.update", () => {
   });
 });
 
+describe("#users.updateRole", () => {
+  it("should promote", async () => {
+    const team = await buildTeam();
+    const admin = await buildAdmin({ teamId: team.id });
+    const user = await buildUser({ teamId: team.id });
+
+    const res = await server.post("/api/users.updateRole", {
+      body: {
+        token: admin.getJwtToken(),
+        id: user.id,
+        role: UserRole.Admin,
+      },
+    });
+    expect(res.status).toEqual(200);
+    expect((await user.reload()).role).toEqual(UserRole.Admin);
+  });
+
+  it("should demote", async () => {
+    const team = await buildTeam();
+    const admin = await buildAdmin({ teamId: team.id });
+    const user = await buildAdmin({ teamId: team.id });
+
+    const res = await server.post("/api/users.updateRole", {
+      body: {
+        token: admin.getJwtToken(),
+        id: user.id,
+        role: UserRole.Viewer,
+      },
+    });
+    expect(res.status).toEqual(200);
+    expect((await user.reload()).role).toEqual(UserRole.Viewer);
+  });
+
+  it("should error on same role", async () => {
+    const team = await buildTeam();
+    const admin = await buildAdmin({ teamId: team.id });
+    const user = await buildAdmin({ teamId: team.id });
+
+    const res = await server.post("/api/users.updateRole", {
+      body: {
+        token: admin.getJwtToken(),
+        id: user.id,
+        role: UserRole.Admin,
+      },
+    });
+    expect(res.status).toEqual(400);
+  });
+});
+
 describe("#users.promote", () => {
   it("should promote a new admin", async () => {
     const team = await buildTeam();
@@ -631,6 +680,7 @@ describe("#users.demote", () => {
 
   it("should not allow demoting self", async () => {
     const admin = await buildAdmin();
+    await buildAdmin({ teamId: admin.teamId });
     const res = await server.post("/api/users.demote", {
       body: {
         token: admin.getJwtToken(),

server/routes/api/users/users.ts

@@ -1,8 +1,8 @@
 import Router from "koa-router";
 import { Op, WhereOptions } from "sequelize";
 import { UserPreference, UserRole } from "@shared/types";
+import { UserRoleHelper } from "@shared/utils/UserRoleHelper";
 import { UserValidation } from "@shared/validations";
-import userDemoter from "@server/commands/userDemoter";
 import userDestroyer from "@server/commands/userDestroyer";
 import userInviter from "@server/commands/userInviter";
 import userSuspender from "@server/commands/userSuspender";
@@ -255,92 +255,124 @@ router.post(
 );
 
 // Admin specific
+
+/**
+ * Promote a user to an admin.
+ *
+ * @deprecated Use `users.updateRole` instead.
+ */
 router.post(
   "users.promote",
-  auth(),
+  auth({ admin: true }),
   validate(T.UsersPromoteSchema),
   transaction(),
-  async (ctx: APIContext<T.UsersPromoteReq>) => {
+  (ctx: APIContext<T.UsersPromoteReq>) => {
-    const { transaction } = ctx.state;
+    const forward = ctx as unknown as APIContext<T.UsersChangeRoleReq>;
-    const userId = ctx.input.body.id;
+    forward.input = {
-    const actor = ctx.state.auth.user;
+      body: {
-    const teamId = actor.teamId;
+        id: ctx.input.body.id,
-    const user = await User.findByPk(userId, {
+        role: UserRole.Admin,
-      rejectOnEmpty: true,
-      transaction,
-      lock: transaction.LOCK.UPDATE,
-    });
-    authorize(actor, "promote", user);
-
-    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 = {
-      data: presentUser(user, {
-        includeDetails,
-      }),
-      policies: presentPolicies(actor, [user]),
     };
+
+    return updateRole(forward);
+  }
+);
+
+/**
+ * Demote a user to another role.
+ *
+ * @deprecated Use `users.updateRole` instead.
+ */
+router.post(
+  "users.demote",
+  auth({ admin: true }),
+  validate(T.UsersDemoteSchema),
+  transaction(),
+  (ctx: APIContext<T.UsersDemoteReq>) => {
+    const forward = ctx as unknown as APIContext<T.UsersChangeRoleReq>;
+    forward.input = {
+      body: {
+        id: ctx.input.body.id,
+        role: ctx.input.body.to,
+      },
+    };
+
+    return updateRole(forward);
   }
 );
 
 router.post(
-  "users.demote",
+  "users.updateRole",
-  auth(),
+  auth({ admin: true }),
-  validate(T.UsersDemoteSchema),
+  validate(T.UsersChangeRoleSchema),
   transaction(),
-  async (ctx: APIContext<T.UsersDemoteReq>) => {
+  updateRole
-    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);
-
-    ctx.body = {
-      data: presentUser(user, {
-        includeDetails,
-      }),
-      policies: presentPolicies(actor, [user]),
-    };
-  }
 );
 
+async function updateRole(ctx: APIContext<T.UsersChangeRoleReq>) {
+  const { transaction } = ctx.state;
+  const userId = ctx.input.body.id;
+  const role = ctx.input.body.role;
+  const actor = ctx.state.auth.user;
+
+  const user = await User.findByPk(userId, {
+    rejectOnEmpty: true,
+    transaction,
+    lock: transaction.LOCK.UPDATE,
+  });
+  await Team.findByPk(user.teamId, {
+    transaction,
+    lock: transaction.LOCK.UPDATE,
+  });
+
+  let name;
+
+  if (user.role === role) {
+    throw ValidationError("User is already in that role");
+  }
+  if (user.id === actor.id) {
+    throw ValidationError("You cannot change your own role");
+  }
+
+  if (UserRoleHelper.canDemote(user, role)) {
+    name = "users.demote";
+    authorize(actor, "demote", user);
+  }
+  if (UserRoleHelper.canPromote(user, role)) {
+    name = "users.promote";
+    authorize(actor, "promote", user);
+  }
+
+  await user.update({ role }, { transaction });
+
+  await Event.create(
+    {
+      name,
+      userId,
+      actorId: actor.id,
+      teamId: actor.teamId,
+      data: {
+        name: user.name,
+        role,
+      },
+      ip: ctx.request.ip,
+    },
+    {
+      transaction,
+    }
+  );
+
+  const includeDetails = can(actor, "readDetails", user);
+
+  ctx.body = {
+    data: presentUser(user, {
+      includeDetails,
+    }),
+    policies: presentPolicies(actor, [user]),
+  };
+}
+
 router.post(
   "users.suspend",
   auth(),

shared/i18n/locales/en_US/translation.json

@@ -460,6 +460,9 @@
   "Suspend user": "Suspend user",
   "An error occurred while sending the invite": "An error occurred while sending the invite",
   "User options": "User options",
+  "Change role": "Change role",
+  "Promote to {{ role }}": "Promote to {{ role }}",
+  "Demote to {{ role }}": "Demote to {{ role }}",
   "Resend invite": "Resend invite",
   "Revoke invite": "Revoke invite",
   "Activate user": "Activate user",

shared/utils/UserRoleHelper.ts

@@ -0,0 +1,61 @@
+import { UserRole } from "../types";
+
+interface User {
+  role: UserRole;
+}
+
+export class UserRoleHelper {
+  /**
+   * Check if the first role is higher than the second role.
+   *
+   * @param role The role to check
+   * @param otherRole The role to compare against
+   * @returns `true` if the first role is higher than the second role, `false` otherwise
+   */
+  static isRoleHigher(role: UserRole, otherRole: UserRole): boolean {
+    return this.roles.indexOf(role) > this.roles.indexOf(otherRole);
+  }
+
+  /**
+   * Check if the first role is lower than the second role.
+   *
+   * @param role The role to check
+   * @param otherRole The role to compare against
+   * @returns `true` if the first role is lower than the second role, `false` otherwise
+   */
+  static isRoleLower(role: UserRole, otherRole: UserRole): boolean {
+    return this.roles.indexOf(role) < this.roles.indexOf(otherRole);
+  }
+
+  /**
+   * Check if the users role is lower than the given role. This does not authorize the operation.
+   *
+   * @param user The user to check
+   * @param role The role to compare against
+   * @returns `true` if the users role is lower than the given role, `false` otherwise
+   */
+  static canPromote(user: User, role: UserRole): boolean {
+    return this.isRoleHigher(role, user.role);
+  }
+
+  /**
+   * Check if the users role is higher than the given role. This does not authorize the operation.
+   *
+   * @param user The user to check
+   * @param role The role to compare against
+   * @returns `true` if the users role is higher than the given role, `false` otherwise
+   */
+  static canDemote(user: User, role: UserRole): boolean {
+    return this.isRoleLower(role, user.role);
+  }
+
+  /**
+   * List of all roles in order from lowest to highest.
+   */
+  private static roles = [
+    UserRole.Guest,
+    UserRole.Viewer,
+    UserRole.Member,
+    UserRole.Admin,
+  ];
+}