diff --git a/app/models/User.ts b/app/models/User.ts index 6ae9274fd..ffa5a9cb2 100644 --- a/app/models/User.ts +++ b/app/models/User.ts @@ -52,10 +52,7 @@ class User extends ParanoidModel { email: string; @observable - isAdmin: boolean; - - @observable - isViewer: boolean; + role: UserRole; @observable lastActiveAt: string; @@ -68,11 +65,27 @@ class User extends ParanoidModel { return (this.name ? this.name[0] : "?").toUpperCase(); } - @computed + /** + * Whether the user has been invited but not yet signed in. + */ get isInvited(): boolean { return !this.lastActiveAt; } + /** + * Whether the user is an admin. + */ + get isAdmin(): boolean { + return this.role === UserRole.Admin; + } + + /** + * Whether the user is a viewer. + */ + get isViewer(): boolean { + return this.role === UserRole.Viewer; + } + /** * Whether the user has been recently active. Recently is currently defined * as within the last 5 minutes. @@ -84,17 +97,6 @@ class User extends ParanoidModel { return new Date(this.lastActiveAt) > subMinutes(now(10000), 5); } - @computed - get role(): UserRole { - if (this.isAdmin) { - return UserRole.Admin; - } else if (this.isViewer) { - return UserRole.Viewer; - } else { - return UserRole.Member; - } - } - /** * Returns whether this user is using a separate editing mode behind an "Edit" * button rather than seamless always-editing. diff --git a/app/models/base/Model.ts b/app/models/base/Model.ts index bf4b5ec1a..2704c5a39 100644 --- a/app/models/base/Model.ts +++ b/app/models/base/Model.ts @@ -116,7 +116,12 @@ export default abstract class Model { updateData = action((data: Partial) => { for (const key in data) { - this[key] = data[key]; + try { + this[key] = data[key]; + } catch (error) { + // Temporary as isViewer and isAdmin properties changed to getters + Logger.warn(`Error setting ${key} on model`, error); + } } this.isNew = false; diff --git a/server/commands/accountProvisioner.ts b/server/commands/accountProvisioner.ts index e91702fd7..b93116a51 100644 --- a/server/commands/accountProvisioner.ts +++ b/server/commands/accountProvisioner.ts @@ -1,4 +1,5 @@ import invariant from "invariant"; +import { UserRole } from "@shared/types"; import WelcomeEmail from "@server/emails/templates/WelcomeEmail"; import { InvalidAuthenticationError, @@ -132,7 +133,7 @@ async function accountProvisioner({ name: userParams.name, email: userParams.email, language: userParams.language, - isAdmin: isNewTeam || undefined, + role: isNewTeam ? UserRole.Admin : undefined, avatarUrl: userParams.avatarUrl, teamId: team.id, ip, diff --git a/server/commands/userDestroyer.ts b/server/commands/userDestroyer.ts index 1d15e7086..40d47ad9b 100644 --- a/server/commands/userDestroyer.ts +++ b/server/commands/userDestroyer.ts @@ -1,4 +1,5 @@ import { Op, Transaction } from "sequelize"; +import { UserRole } from "@shared/types"; import { Event, User } from "@server/models"; import { ValidationError } from "../errors"; @@ -29,7 +30,7 @@ export default async function userDestroyer({ if (user.isAdmin) { const otherAdminsCount = await User.count({ where: { - isAdmin: true, + role: UserRole.Admin, teamId, id: { [Op.ne]: user.id, diff --git a/server/commands/userInviter.ts b/server/commands/userInviter.ts index 567f1ac61..6b811cd7c 100644 --- a/server/commands/userInviter.ts +++ b/server/commands/userInviter.ts @@ -58,8 +58,12 @@ export default async function userInviter({ teamId: user.teamId, name: invite.name, email: invite.email, - isAdmin: user.isAdmin && invite.role === UserRole.Admin, - isViewer: user.isViewer || invite.role === UserRole.Viewer, + role: + user.isAdmin && invite.role === UserRole.Admin + ? UserRole.Admin + : user.isViewer || invite.role === UserRole.Viewer + ? UserRole.Viewer + : UserRole.Member, invitedById: user.id, flags: { [UserFlag.InviteSent]: 1, diff --git a/server/commands/userProvisioner.test.ts b/server/commands/userProvisioner.test.ts index 2740387cd..0897ed861 100644 --- a/server/commands/userProvisioner.test.ts +++ b/server/commands/userProvisioner.test.ts @@ -1,5 +1,6 @@ import { faker } from "@faker-js/faker"; import { v4 as uuidv4 } from "uuid"; +import { UserRole } from "@shared/types"; import { TeamDomain } from "@server/models"; import { buildUser, @@ -185,8 +186,7 @@ describe("userProvisioner", () => { expect(authentication?.scopes.length).toEqual(1); expect(authentication?.scopes[0]).toEqual("read"); expect(user.email).toEqual("test@example.com"); - expect(user.isAdmin).toEqual(false); - expect(user.isViewer).toEqual(false); + expect(user.role).toEqual(UserRole.Member); expect(isNewUser).toEqual(true); }); @@ -200,7 +200,7 @@ describe("userProvisioner", () => { name: "Test Name", email: "test@example.com", teamId: team.id, - isAdmin: true, + role: UserRole.Admin, ip, authentication: { authenticationProviderId: authenticationProvider.id, @@ -210,12 +210,12 @@ describe("userProvisioner", () => { }, }); const { user } = result; - expect(user.isAdmin).toEqual(true); + expect(user.role).toEqual(UserRole.Admin); }); it("should prefer defaultUserRole when isAdmin is undefined or false", async () => { const team = await buildTeam({ - defaultUserRole: "viewer", + defaultUserRole: UserRole.Viewer, }); const authenticationProviders = await team.$get("authenticationProviders"); const authenticationProvider = authenticationProviders[0]; @@ -232,13 +232,11 @@ describe("userProvisioner", () => { }, }); const { user: tname } = result; - expect(tname.isAdmin).toEqual(false); - expect(tname.isViewer).toEqual(true); + expect(tname.role).toEqual(UserRole.Viewer); const tname2Result = await userProvisioner({ name: "Test2 Name", email: "tes2@example.com", teamId: team.id, - isAdmin: false, ip, authentication: { authenticationProviderId: authenticationProvider.id, @@ -248,8 +246,7 @@ describe("userProvisioner", () => { }, }); const { user: tname2 } = tname2Result; - expect(tname2.isAdmin).toEqual(false); - expect(tname2.isViewer).toEqual(true); + expect(tname2.role).toEqual(UserRole.Viewer); }); it("should create a user from an invited user", async () => { diff --git a/server/commands/userProvisioner.ts b/server/commands/userProvisioner.ts index 801f624dd..207aa9526 100644 --- a/server/commands/userProvisioner.ts +++ b/server/commands/userProvisioner.ts @@ -1,4 +1,5 @@ import { InferCreationAttributes } from "sequelize"; +import { UserRole } from "@shared/types"; import InviteAcceptedEmail from "@server/emails/templates/InviteAcceptedEmail"; import { DomainNotAllowedError, @@ -22,8 +23,8 @@ type Props = { email: string; /** The language of the user, if known */ language?: string; - /** Provision the new user as an administrator */ - isAdmin?: boolean; + /** The role for new user, Member if none is provided */ + role?: UserRole; /** The public url of an image representing the user */ avatarUrl?: string | null; /** @@ -52,7 +53,7 @@ type Props = { export default async function userProvisioner({ name, email, - isAdmin, + role, language, avatarUrl, teamId, @@ -230,15 +231,12 @@ export default async function userProvisioner({ throw DomainNotAllowedError(); } - const defaultUserRole = team?.defaultUserRole; - const user = await User.create( { name, email, language, - isAdmin: typeof isAdmin === "boolean" && isAdmin, - isViewer: isAdmin === true ? false : defaultUserRole === "viewer", + role: role ?? team?.defaultUserRole, teamId, avatarUrl, authentications: authentication ? [authentication] : [], diff --git a/server/migrations/20240327235446-role-non-nullable.js b/server/migrations/20240327235446-role-non-nullable.js new file mode 100644 index 000000000..a61504c64 --- /dev/null +++ b/server/migrations/20240327235446-role-non-nullable.js @@ -0,0 +1,15 @@ +'use strict'; + +module.exports = { + async up (queryInterface, Sequelize) { + await queryInterface.sequelize.query( + 'ALTER TABLE users ALTER COLUMN role SET NOT NULL;' + ); + }, + + async down (queryInterface) { + await queryInterface.sequelize.query( + 'ALTER TABLE users ALTER COLUMN role DROP NOT NULL;' + ); + } +}; \ No newline at end of file diff --git a/server/models/Team.ts b/server/models/Team.ts index 4d02e4a59..c3d37e340 100644 --- a/server/models/Team.ts +++ b/server/models/Team.ts @@ -34,6 +34,7 @@ import { CollectionPermission, TeamPreference, TeamPreferences, + UserRole, } from "@shared/types"; import { getBaseDomain, RESERVED_SUBDOMAINS } from "@shared/utils/domains"; import env from "@server/env"; @@ -151,10 +152,10 @@ class Team extends ParanoidModel< @Column memberCollectionCreate: boolean; - @Default("member") - @IsIn([["viewer", "member"]]) - @Column - defaultUserRole: string; + @Default(UserRole.Member) + @IsIn([[UserRole.Viewer, UserRole.Member]]) + @Column(DataType.STRING) + defaultUserRole: UserRole; @AllowNull @Column(DataType.JSONB) diff --git a/server/models/User.ts b/server/models/User.ts index da616b8d7..712f46810 100644 --- a/server/models/User.ts +++ b/server/models/User.ts @@ -29,7 +29,6 @@ import { IsDate, AllowNull, AfterUpdate, - BeforeSave, } from "sequelize-typescript"; import { UserPreferenceDefaults } from "@shared/constants"; import { languages } from "@shared/i18n"; @@ -137,14 +136,6 @@ class User extends ParanoidModel< @Column name: string; - @Default(false) - @Column - isAdmin: boolean; - - @Default(false) - @Column - isViewer: boolean; - @Default(UserRole.Member) @Column(DataType.ENUM(...Object.values(UserRole))) role: UserRole; @@ -251,6 +242,14 @@ class User extends ParanoidModel< return !this.lastActiveAt; } + get isAdmin() { + return this.role === UserRole.Admin; + } + + get isViewer() { + return this.role === UserRole.Viewer; + } + get color() { return stringToColor(this.id); } @@ -551,7 +550,7 @@ class User extends ParanoidModel< const res = await (this.constructor as typeof User).findAndCountAll({ where: { teamId: this.teamId, - isAdmin: true, + role: UserRole.Admin, id: { [Op.ne]: this.id, }, @@ -562,21 +561,9 @@ class User extends ParanoidModel< if (res.count >= 1) { if (to === UserRole.Member) { - await this.update( - { - isAdmin: false, - isViewer: false, - }, - options - ); + await this.update({ role: to }, options); } else if (to === UserRole.Viewer) { - await this.update( - { - isAdmin: false, - isViewer: true, - }, - options - ); + await this.update({ role: to }, options); await UserMembership.update( { permission: CollectionPermission.Read, @@ -599,13 +586,7 @@ class User extends ParanoidModel< promote: ( options?: InstanceUpdateOptions> ) => Promise = (options) => - this.update( - { - isAdmin: true, - isViewer: false, - }, - options - ); + this.update({ role: UserRole.Admin }, options); // hooks @@ -628,20 +609,6 @@ class User extends ParanoidModel< }); }; - /** - * Temporary hook to double write role while we transition to the new field. - */ - @BeforeSave - static doubleWriteRole = async (model: User) => { - if (model.isAdmin) { - model.role = UserRole.Admin; - } else if (model.isViewer) { - model.role = UserRole.Viewer; - } else { - model.role = UserRole.Member; - } - }; - @BeforeCreate static setRandomJwtSecret = (model: User) => { model.jwtSecret = crypto.randomBytes(64).toString("hex"); @@ -677,8 +644,8 @@ class User extends ParanoidModel< const countSql = ` SELECT COUNT(CASE WHEN "suspendedAt" IS NOT NULL THEN 1 END) as "suspendedCount", - COUNT(CASE WHEN "isAdmin" = true THEN 1 END) as "adminCount", - COUNT(CASE WHEN "isViewer" = true THEN 1 END) as "viewerCount", + COUNT(CASE WHEN "role" = :roleAdmin THEN 1 END) as "adminCount", + COUNT(CASE WHEN "role" = :roleViewer THEN 1 END) as "viewerCount", COUNT(CASE WHEN "lastActiveAt" IS NULL THEN 1 END) as "invitedCount", COUNT(CASE WHEN "suspendedAt" IS NULL AND "lastActiveAt" IS NOT NULL THEN 1 END) as "activeCount", COUNT(*) as count @@ -690,6 +657,8 @@ class User extends ParanoidModel< type: QueryTypes.SELECT, replacements: { teamId, + roleAdmin: UserRole.Admin, + roleViewer: UserRole.Viewer, }, }); diff --git a/server/policies/collection.test.ts b/server/policies/collection.test.ts index 451b45eef..de1f8595e 100644 --- a/server/policies/collection.test.ts +++ b/server/policies/collection.test.ts @@ -1,4 +1,4 @@ -import { CollectionPermission } from "@shared/types"; +import { CollectionPermission, UserRole } from "@shared/types"; import { UserMembership, Collection } from "@server/models"; import { buildUser, @@ -217,7 +217,7 @@ describe("viewer", () => { it("should allow read permissions for viewer", async () => { const team = await buildTeam(); const user = await buildUser({ - isViewer: true, + role: UserRole.Viewer, teamId: team.id, }); const collection = await buildCollection({ @@ -235,7 +235,7 @@ describe("viewer", () => { it("should override read membership permission", async () => { const team = await buildTeam(); const user = await buildUser({ - isViewer: true, + role: UserRole.Viewer, teamId: team.id, }); const collection = await buildCollection({ @@ -264,7 +264,7 @@ describe("viewer", () => { it("should allow override with read_write membership permission", async () => { const team = await buildTeam(); const user = await buildUser({ - isViewer: true, + role: UserRole.Viewer, teamId: team.id, }); const collection = await buildCollection({ @@ -294,7 +294,7 @@ describe("viewer", () => { it("should allow no permissions for viewer", async () => { const team = await buildTeam(); const user = await buildUser({ - isViewer: true, + role: UserRole.Viewer, teamId: team.id, }); const collection = await buildCollection({ @@ -310,7 +310,7 @@ describe("viewer", () => { it("should allow override with team member membership permission", async () => { const team = await buildTeam(); const user = await buildUser({ - isViewer: true, + role: UserRole.Viewer, teamId: team.id, }); const collection = await buildCollection({ diff --git a/server/policies/document.test.ts b/server/policies/document.test.ts index bc7e50784..793da61ba 100644 --- a/server/policies/document.test.ts +++ b/server/policies/document.test.ts @@ -1,4 +1,4 @@ -import { CollectionPermission } from "@shared/types"; +import { CollectionPermission, UserRole } from "@shared/types"; import { Document } from "@server/models"; import { buildUser, @@ -39,7 +39,7 @@ describe("read_write collection", () => { const team = await buildTeam(); const user = await buildUser({ teamId: team.id, - isViewer: true, + role: UserRole.Viewer, }); const collection = await buildCollection({ teamId: team.id, diff --git a/server/presenters/__snapshots__/user.test.ts.snap b/server/presenters/__snapshots__/user.test.ts.snap index 6d9b83f95..3869fbb86 100644 --- a/server/presenters/__snapshots__/user.test.ts.snap +++ b/server/presenters/__snapshots__/user.test.ts.snap @@ -11,6 +11,7 @@ exports[`presents a user 1`] = ` "isViewer": false, "lastActiveAt": undefined, "name": "Test User", + "role": "member", "updatedAt": undefined, } `; @@ -26,6 +27,7 @@ exports[`presents a user without slack data 1`] = ` "isViewer": false, "lastActiveAt": undefined, "name": "Test User", + "role": "member", "updatedAt": undefined, } `; diff --git a/server/presenters/user.ts b/server/presenters/user.ts index 73032da49..2d6821448 100644 --- a/server/presenters/user.ts +++ b/server/presenters/user.ts @@ -1,4 +1,4 @@ -import { NotificationSettings, UserPreferences } from "@shared/types"; +import { NotificationSettings, UserPreferences, UserRole } from "@shared/types"; import env from "@server/env"; import { User } from "@server/models"; @@ -14,6 +14,7 @@ type UserPresentation = { updatedAt: Date; lastActiveAt: Date | null; color: string; + role: UserRole; isAdmin: boolean; isSuspended: boolean; isViewer: boolean; @@ -32,6 +33,7 @@ export default function presentUser( name: user.name, avatarUrl: user.avatarUrl, color: user.color, + role: user.role, isAdmin: user.isAdmin, isSuspended: user.isSuspended, isViewer: user.isViewer, diff --git a/server/routes/api/teams/teams.ts b/server/routes/api/teams/teams.ts index ce03e285d..cb89366f7 100644 --- a/server/routes/api/teams/teams.ts +++ b/server/routes/api/teams/teams.ts @@ -1,5 +1,6 @@ import invariant from "invariant"; import Router from "koa-router"; +import { UserRole } from "@shared/types"; import teamCreator from "@server/commands/teamCreator"; import teamDestroyer from "@server/commands/teamDestroyer"; import teamUpdater from "@server/commands/teamUpdater"; @@ -164,7 +165,7 @@ router.post( teamId: team.id, name: user.name, email: user.email, - isAdmin: true, + role: UserRole.Admin, }, { transaction } ); diff --git a/server/routes/api/users/users.test.ts b/server/routes/api/users/users.test.ts index f21ddcb93..5c9bfa67c 100644 --- a/server/routes/api/users/users.test.ts +++ b/server/routes/api/users/users.test.ts @@ -1,4 +1,4 @@ -import { TeamPreference } from "@shared/types"; +import { TeamPreference, UserRole } from "@shared/types"; import { buildTeam, buildAdmin, @@ -368,8 +368,7 @@ describe("#users.invite", () => { const body = await res.json(); expect(res.status).toEqual(200); expect(body.data.sent.length).toEqual(1); - expect(body.data.users[0].isViewer).toBeTruthy(); - expect(body.data.users[0].isAdmin).toBeFalsy(); + expect(body.data.users[0].role).toEqual(UserRole.Viewer); }); it("should require authentication", async () => { @@ -383,7 +382,6 @@ describe("#users.delete", () => { const user = await buildAdmin(); await buildUser({ teamId: user.teamId, - isAdmin: false, }); const res = await server.post("/api/users.delete", { body: { @@ -397,7 +395,6 @@ describe("#users.delete", () => { const user = await buildAdmin(); await buildUser({ teamId: user.teamId, - isAdmin: false, }); const res = await server.post("/api/users.delete", { body: { @@ -572,11 +569,7 @@ describe("#users.demote", () => { it("should demote an admin", async () => { const team = await buildTeam(); const admin = await buildAdmin({ teamId: team.id }); - const user = await buildUser({ teamId: team.id }); - - await user.update({ - isAdmin: true, - }); // Make another admin + const user = await buildAdmin({ teamId: team.id }); const res = await server.post("/api/users.demote", { body: { @@ -590,11 +583,7 @@ describe("#users.demote", () => { it("should demote an admin to viewer", async () => { const team = await buildTeam(); const admin = await buildAdmin({ teamId: team.id }); - const user = await buildUser({ teamId: team.id }); - - await user.update({ - isAdmin: true, - }); // Make another admin + const user = await buildAdmin({ teamId: team.id }); const res = await server.post("/api/users.demote", { body: { @@ -609,11 +598,7 @@ describe("#users.demote", () => { it("should demote an admin to member", async () => { const team = await buildTeam(); const admin = await buildAdmin({ teamId: team.id }); - const user = await buildUser({ teamId: team.id }); - - await user.update({ - isAdmin: true, - }); // Make another admin + const user = await buildAdmin({ teamId: team.id }); const res = await server.post("/api/users.demote", { body: { @@ -748,9 +733,8 @@ describe("#users.count", () => { it("should count admin users", async () => { const team = await buildTeam(); - const user = await buildUser({ + const user = await buildAdmin({ teamId: team.id, - isAdmin: true, }); const res = await server.post("/api/users.count", { body: { diff --git a/server/routes/api/users/users.ts b/server/routes/api/users/users.ts index f0f96bd76..740dd6caa 100644 --- a/server/routes/api/users/users.ts +++ b/server/routes/api/users/users.ts @@ -1,6 +1,6 @@ import Router from "koa-router"; import { Op, WhereOptions } from "sequelize"; -import { UserPreference } from "@shared/types"; +import { UserPreference, UserRole } from "@shared/types"; import { UserValidation } from "@shared/validations"; import userDemoter from "@server/commands/userDemoter"; import userDestroyer from "@server/commands/userDestroyer"; @@ -59,17 +59,17 @@ router.post( } case "viewers": { - where = { ...where, isViewer: true }; + where = { ...where, role: UserRole.Viewer }; break; } case "admins": { - where = { ...where, isAdmin: true }; + where = { ...where, role: UserRole.Admin }; break; } case "members": { - where = { ...where, isAdmin: false, isViewer: false }; + where = { ...where, role: UserRole.Member }; break; } diff --git a/server/scripts/seed.ts b/server/scripts/seed.ts index 8498f8a0e..cef1735e2 100644 --- a/server/scripts/seed.ts +++ b/server/scripts/seed.ts @@ -1,4 +1,5 @@ import "./bootstrap"; +import { UserRole } from "@shared/types"; import teamCreator from "@server/commands/teamCreator"; import env from "@server/env"; import { Team, User } from "@server/models"; @@ -23,8 +24,7 @@ export default async function main(exit = false) { teamId: team.id, name: email.split("@")[0], email, - isAdmin: true, - isViewer: false, + role: UserRole.Admin, }, { transaction, diff --git a/server/test/factories.ts b/server/test/factories.ts index 6636e87db..e6ca51ca7 100644 --- a/server/test/factories.ts +++ b/server/test/factories.ts @@ -11,6 +11,7 @@ import { IntegrationService, IntegrationType, NotificationEventType, + UserRole, } from "@shared/types"; import { Share, @@ -213,11 +214,11 @@ export async function buildUser(overrides: Partial = {}) { } export async function buildAdmin(overrides: Partial = {}) { - return buildUser({ ...overrides, isAdmin: true }); + return buildUser({ ...overrides, role: UserRole.Admin }); } export async function buildViewer(overrides: Partial = {}) { - return buildUser({ ...overrides, isViewer: true }); + return buildUser({ ...overrides, role: UserRole.Viewer }); } export async function buildInvite(overrides: Partial = {}) { diff --git a/yarn.lock b/yarn.lock index 50d088963..81bcc5745 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1079,16 +1079,7 @@ debug "^4.3.1" globals "^11.1.0" -"@babel/types@^7.0.0", "@babel/types@^7.18.6", "@babel/types@^7.18.9", "@babel/types@^7.20.2", "@babel/types@^7.20.5", "@babel/types@^7.22.15", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.6", "@babel/types@^7.3.0", "@babel/types@^7.3.3", "@babel/types@^7.4.4", "@babel/types@^7.7.0": - version "7.23.6" - resolved "https://registry.yarnpkg.com/@babel/types/-/types-7.23.6.tgz#be33fdb151e1f5a56877d704492c240fc71c7ccd" - integrity "sha1-vjP9sVHh9aVod9cESSwkD8ccfM0= sha512-+uarb83brBzPKN38NX1MkB6vb6+mwvR6amUulqAE7ccQw1pEl+bCia9TbdG1lsnFP7lZySvUn37CHyXQdfTwzg==" - dependencies: - "@babel/helper-string-parser" "^7.23.4" - "@babel/helper-validator-identifier" "^7.22.20" - to-fast-properties "^2.0.0" - -"@babel/types@^7.23.4": +"@babel/types@^7.0.0", "@babel/types@^7.18.6", "@babel/types@^7.18.9", "@babel/types@^7.20.2", "@babel/types@^7.20.5", "@babel/types@^7.22.15", "@babel/types@^7.22.5", "@babel/types@^7.23.0", "@babel/types@^7.23.4", "@babel/types@^7.23.6", "@babel/types@^7.3.0", "@babel/types@^7.3.3", "@babel/types@^7.4.4", "@babel/types@^7.7.0": version "7.24.0" resolved "https://registry.yarnpkg.com/@babel/types/-/types-7.24.0.tgz#3b951f435a92e7333eba05b7566fd297960ea1bf" integrity sha512-+j7a5c253RfKh8iABBhywc8NSfP5LURe7Uh4qpsh6jc+aLJguvmIUBdjSdEMQv2bENrCR5MfRdjGo7vzS/ob7w==