From 3f209101e69b5f8cf8ee79cf37ba4f9615bd1112 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sat, 6 Apr 2024 07:32:15 -0600 Subject: [PATCH] Add `updateRole` endpoint (#6771) --- server/commands/userDemoter.test.ts | 33 ---- server/commands/userDemoter.ts | 41 ---- server/models/User.ts | 106 ++++++----- server/policies/apiKey.ts | 3 +- server/policies/webhookSubscription.ts | 3 +- .../queues/processors/UserDeletedProcessor.ts | 2 +- .../queues/processors/UserDemotedProcessor.ts | 11 ++ server/queues/tasks/CleanupDemotedUserTask.ts | 11 +- .../users/__snapshots__/users.test.ts.snap | 6 +- server/routes/api/users/schema.ts | 11 +- server/routes/api/users/users.test.ts | 50 +++++ server/routes/api/users/users.ts | 180 +++++++++++------- shared/i18n/locales/en_US/translation.json | 3 + shared/utils/UserRoleHelper.ts | 61 ++++++ 14 files changed, 314 insertions(+), 207 deletions(-) delete mode 100644 server/commands/userDemoter.test.ts delete mode 100644 server/commands/userDemoter.ts create mode 100644 server/queues/processors/UserDemotedProcessor.ts create mode 100644 shared/utils/UserRoleHelper.ts diff --git a/server/commands/userDemoter.test.ts b/server/commands/userDemoter.test.ts deleted file mode 100644 index b91e6c44d..000000000 --- a/server/commands/userDemoter.test.ts +++ /dev/null @@ -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); - }); -}); diff --git a/server/commands/userDemoter.ts b/server/commands/userDemoter.ts deleted file mode 100644 index e841e50df..000000000 --- a/server/commands/userDemoter.ts +++ /dev/null @@ -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 }); -} diff --git a/server/models/User.ts b/server/models/User.ts index 6961aaa2d..90eeb5d49 100644 --- a/server/models/User.ts +++ b/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> - ) => Promise = 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> - ) => Promise = (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> + ) { + 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> + ) { + 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"); diff --git a/server/policies/apiKey.ts b/server/policies/apiKey.ts index b128d698f..14f642672 100644 --- a/server/policies/apiKey.ts +++ b/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 ) ); diff --git a/server/policies/webhookSubscription.ts b/server/policies/webhookSubscription.ts index 95f71ebb5..7ea1e64d8 100644 --- a/server/policies/webhookSubscription.ts +++ b/server/policies/webhookSubscription.ts @@ -6,7 +6,8 @@ allow(User, "createWebhookSubscription", Team, (actor, team) => and( // isTeamAdmin(actor, team), - isTeamMutable(actor) + isTeamMutable(actor), + !actor.isSuspended ) ); diff --git a/server/queues/processors/UserDeletedProcessor.ts b/server/queues/processors/UserDeletedProcessor.ts index d2cb39492..d6dcef76f 100644 --- a/server/queues/processors/UserDeletedProcessor.ts +++ b/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) { diff --git a/server/queues/processors/UserDemotedProcessor.ts b/server/queues/processors/UserDemotedProcessor.ts new file mode 100644 index 000000000..a2bc23c56 --- /dev/null +++ b/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 }); + } +} diff --git a/server/queues/tasks/CleanupDemotedUserTask.ts b/server/queues/tasks/CleanupDemotedUserTask.ts index 5fd16e843..9255a45e6 100644 --- a/server/queues/tasks/CleanupDemotedUserTask.ts +++ b/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 { public async perform(props: Props) { - await sequelize.transaction(async (transaction) => { - const user = await User.findByPk(props.userId, { rejectOnEmpty: true }); + const user = await User.scope("withTeam").findByPk(props.userId, { + 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 { ); } - if (user.isSuspended || user.isViewer) { + if (cannot(user, "createApiKey", user.team)) { const apiKeys = await ApiKey.findAll({ where: { userId: props.userId, diff --git a/server/routes/api/users/__snapshots__/users.test.ts.snap b/server/routes/api/users/__snapshots__/users.test.ts.snap index b2929a417..3ab141b70 100644 --- a/server/routes/api/users/__snapshots__/users.test.ts.snap +++ b/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, } diff --git a/server/routes/api/users/schema.ts b/server/routes/api/users/schema.ts index 706b70f05..79f7c9134 100644 --- a/server/routes/api/users/schema.ts +++ b/server/routes/api/users/schema.ts @@ -110,6 +110,14 @@ export const UsersActivateSchema = BaseSchema.extend({ export type UsersActivateReq = z.infer; +export const UsersChangeRoleSchema = BaseSchema.extend({ + body: BaseIdSchema.extend({ + role: z.nativeEnum(UserRole), + }), +}); + +export type UsersChangeRoleReq = z.infer; + export const UsersPromoteSchema = BaseSchema.extend({ body: BaseIdSchema, }); @@ -117,8 +125,7 @@ export const UsersPromoteSchema = BaseSchema.extend({ export type UsersPromoteReq = z.infer; export const UsersDemoteSchema = BaseSchema.extend({ - body: z.object({ - id: z.string().uuid(), + body: BaseIdSchema.extend({ to: z.nativeEnum(UserRole).default(UserRole.Member), }), }); diff --git a/server/routes/api/users/users.test.ts b/server/routes/api/users/users.test.ts index 07b438428..4f4f15ec2 100644 --- a/server/routes/api/users/users.test.ts +++ b/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(), diff --git a/server/routes/api/users/users.ts b/server/routes/api/users/users.ts index 9bd932c77..e9bf18b38 100644 --- a/server/routes/api/users/users.ts +++ b/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) => { - 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, { - 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, + (ctx: APIContext) => { + const forward = ctx as unknown as APIContext; + forward.input = { + body: { + id: ctx.input.body.id, + role: UserRole.Admin, }, - { - 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) => { + const forward = ctx as unknown as APIContext; + forward.input = { + body: { + id: ctx.input.body.id, + role: ctx.input.body.to, + }, + }; + + return updateRole(forward); } ); router.post( - "users.demote", - auth(), - validate(T.UsersDemoteSchema), + "users.updateRole", + auth({ admin: true }), + validate(T.UsersChangeRoleSchema), transaction(), - async (ctx: APIContext) => { - 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]), - }; - } + updateRole ); +async function updateRole(ctx: APIContext) { + 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(), diff --git a/shared/i18n/locales/en_US/translation.json b/shared/i18n/locales/en_US/translation.json index b7d87d5ee..b18df5af7 100644 --- a/shared/i18n/locales/en_US/translation.json +++ b/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", diff --git a/shared/utils/UserRoleHelper.ts b/shared/utils/UserRoleHelper.ts new file mode 100644 index 000000000..f70a8df9b --- /dev/null +++ b/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, + ]; +}