Merge pull request #8968 from Budibase/fix/user-self-update

Minor update - stop users updating their own admin/global flags

packages/backend-core/src/middleware/passport/google.ts

@@ -1,9 +1,9 @@
 import { ssoCallbackUrl } from "./utils"
-import { authenticateThirdParty } from "./third-party-common"
+import { authenticateThirdParty, SaveUserFunction } from "./third-party-common"
 import { ConfigType, GoogleConfig, Database, SSOProfile } from "@budibase/types"
 const GoogleStrategy = require("passport-google-oauth").OAuth2Strategy
 
-export function buildVerifyFn(saveUserFn?: Function) {
+export function buildVerifyFn(saveUserFn?: SaveUserFunction) {
   return (
     accessToken: string,
     refreshToken: string,
@@ -39,7 +39,7 @@ export function buildVerifyFn(saveUserFn?: Function) {
 export async function strategyFactory(
   config: GoogleConfig["config"],
   callbackUrl: string,
-  saveUserFn?: Function
+  saveUserFn?: SaveUserFunction
 ) {
   try {
     const { clientID, clientSecret } = config

packages/backend-core/src/middleware/passport/oidc.ts

@@ -1,5 +1,5 @@
 import fetch from "node-fetch"
-import { authenticateThirdParty } from "./third-party-common"
+import { authenticateThirdParty, SaveUserFunction } from "./third-party-common"
 import { ssoCallbackUrl } from "./utils"
 import {
   Config,
@@ -17,7 +17,7 @@ type JwtClaims = {
   email: string
 }
 
-export function buildVerifyFn(saveUserFn?: Function) {
+export function buildVerifyFn(saveUserFn?: SaveUserFunction) {
   /**
    * @param {*} issuer The identity provider base URL
    * @param {*} sub The user ID
@@ -106,7 +106,7 @@ function validEmail(value: string) {
  */
 export async function strategyFactory(
   config: OIDCConfiguration,
-  saveUserFn?: Function
+  saveUserFn?: SaveUserFunction
 ) {
   try {
     const verify = buildVerifyFn(saveUserFn)

packages/backend-core/src/middleware/passport/third-party-common.ts

@@ -9,6 +9,17 @@ import fetch from "node-fetch"
 import { ThirdPartyUser } from "@budibase/types"
 const jwt = require("jsonwebtoken")
 
+type SaveUserOpts = {
+  requirePassword?: boolean
+  hashPassword?: boolean
+  currentUserId?: string
+}
+
+export type SaveUserFunction = (
+  user: ThirdPartyUser,
+  opts: SaveUserOpts
+) => Promise<any>
+
 /**
  * Common authentication logic for third parties. e.g. OAuth, OIDC.
  */
@@ -16,7 +27,7 @@ export async function authenticateThirdParty(
   thirdPartyUser: ThirdPartyUser,
   requireLocalAccount: boolean = true,
   done: Function,
-  saveUserFn?: Function
+  saveUserFn?: SaveUserFunction
 ) {
   if (!saveUserFn) {
     throw new Error("Save user function must be provided")
@@ -81,7 +92,7 @@ export async function authenticateThirdParty(
 
   // create or sync the user
   try {
-    await saveUserFn(dbUser, false, false)
+    await saveUserFn(dbUser, { hashPassword: false, requirePassword: false })
   } catch (err: any) {
     return authError(done, err)
   }

packages/server/src/api/controllers/public/users.ts

@@ -51,6 +51,8 @@ export async function update(ctx: BBContext, next: any) {
   }
   // disallow updating your own role - always overwrite with DB roles
   if (isLoggedInUser(ctx, user)) {
+    ctx.request.body.builder = user.builder
+    ctx.request.body.admin = user.admin
     ctx.request.body.roles = user.roles
   }
   const response = await saveGlobalUser(publicApiUserFix(ctx))

packages/types/package.json

@@ -13,6 +13,7 @@
   },
   "jest": {},
   "devDependencies": {
+    "@types/json5": "^2.2.0",
     "@types/koa": "2.13.4",
     "@types/node": "14.18.20",
     "@types/pouchdb": "6.4.0",

packages/types/src/documents/global/user.ts

@@ -25,6 +25,7 @@ export interface ThirdPartyUser extends Document {
   email: string
   userId?: string
   forceResetPassword?: boolean
+  userGroups?: string[]
 }
 
 export interface User extends ThirdPartyUser {
@@ -42,7 +43,6 @@ export interface User extends ThirdPartyUser {
   password?: string
   status?: string
   createdAt?: number // override the default createdAt behaviour - users sdk historically set this to Date.now()
-  userGroups?: string[]
   dayPassRecordedAt?: string
   account?: {
     authType: string

packages/types/yarn.lock

@@ -75,6 +75,13 @@
   resolved "https://registry.yarnpkg.com/@types/http-errors/-/http-errors-1.8.2.tgz#7315b4c4c54f82d13fa61c228ec5c2ea5cc9e0e1"
   integrity sha512-EqX+YQxINb+MeXaIqYDASb6U6FCHbWjkj4a1CKDBks3d/QiB2+PqBLyO72vLDgAO1wUI4O+9gweRcQK11bTL/w==
 
+"@types/json5@^2.2.0":
+  version "2.2.0"
+  resolved "https://registry.yarnpkg.com/@types/json5/-/json5-2.2.0.tgz#afff29abf9182a7d4a7e39105ca051f11c603d13"
+  integrity sha512-NrVug5woqbvNZ0WX+Gv4R+L4TGddtmFek2u8RtccAgFZWtS9QXF2xCXY22/M4nzkaKF0q9Fc6M/5rxLDhfwc/A==
+  dependencies:
+    json5 "*"
+
 "@types/keygrip@*":
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/@types/keygrip/-/keygrip-1.0.2.tgz#513abfd256d7ad0bf1ee1873606317b33b1b2a72"
@@ -447,6 +454,11 @@ inherits@2:
   resolved "https://registry.yarnpkg.com/inherits/-/inherits-2.0.4.tgz#0fa2c64f932917c3433a0ded55363aae37416b7c"
   integrity sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ==
 
+json5@*:
+  version "2.2.1"
+  resolved "https://registry.yarnpkg.com/json5/-/json5-2.2.1.tgz#655d50ed1e6f95ad1a3caababd2b0efda10b395c"
+  integrity sha512-1hqLFMSrGHRHxav9q9gNjJ5EXznIxGVO09xQRrwplcS8qs28pZ8s8hupZAmqDwZUmVZ2Qb2jnyPOWcDH8m8dlA==
+
 mime-db@1.52.0:
   version "1.52.0"
   resolved "https://registry.yarnpkg.com/mime-db/-/mime-db-1.52.0.tgz#bbabcdc02859f4987301c856e3387ce5ec43bf70"

packages/worker/src/api/controllers/global/users.ts

@@ -24,7 +24,8 @@ const MAX_USERS_UPLOAD_LIMIT = 1000
 
 export const save = async (ctx: any) => {
   try {
-    ctx.body = await sdk.users.save(ctx.request.body)
+    const currentUserId = ctx.user._id
+    ctx.body = await sdk.users.save(ctx.request.body, { currentUserId })
   } catch (err: any) {
     ctx.throw(err.status || 400, err)
   }

packages/worker/src/api/routes/global/tests/users.spec.ts

@@ -1,4 +1,4 @@
-import { InviteUsersResponse } from "@budibase/types"
+import { InviteUsersResponse, User } from "@budibase/types"
 
 jest.mock("nodemailer")
 import {
@@ -298,6 +298,23 @@ describe("/api/global/users", () => {
       expect(events.user.passwordForceReset).not.toBeCalled()
     })
 
+    it("should not allow a user to update their own admin/builder status", async () => {
+      const user = (await config.api.users.getUser(config.defaultUser?._id!))
+        .body as User
+      await config.api.users.saveUser({
+        ...user,
+        admin: {
+          global: false,
+        },
+        builder: {
+          global: false,
+        },
+      })
+      const userOut = (await config.api.users.getUser(user._id!)).body
+      expect(userOut.admin.global).toBe(true)
+      expect(userOut.builder.global).toBe(true)
+    })
+
     it("should be able to force reset password", async () => {
       const user = await config.createUser()
       jest.clearAllMocks()

packages/worker/src/sdk/users/users.ts

@@ -29,6 +29,7 @@ import {
   RowResponse,
   SearchUsersRequest,
   User,
+  ThirdPartyUser,
 } from "@budibase/types"
 import { sendEmail } from "../../utilities/email"
 import { EmailTemplatePurpose } from "../../constants"
@@ -103,13 +104,14 @@ export const getUser = async (userId: string) => {
   return user
 }
 
-interface SaveUserOpts {
+export interface SaveUserOpts {
   hashPassword?: boolean
   requirePassword?: boolean
+  currentUserId?: string
 }
 
 const buildUser = async (
-  user: User,
+  user: User | ThirdPartyUser,
   opts: SaveUserOpts = {
     hashPassword: true,
     requirePassword: true,
@@ -117,7 +119,8 @@ const buildUser = async (
   tenantId: string,
   dbUser?: any
 ): Promise<User> => {
-  let { password, _id } = user
+  let fullUser = user as User
+  let { password, _id } = fullUser
 
   let hashedPassword
   if (password) {
@@ -130,24 +133,24 @@ const buildUser = async (
 
   _id = _id || dbUtils.generateGlobalUserID()
 
-  user = {
+  fullUser = {
     createdAt: Date.now(),
     ...dbUser,
-    ...user,
+    ...fullUser,
     _id,
     password: hashedPassword,
     tenantId,
   }
   // make sure the roles object is always present
-  if (!user.roles) {
+  if (!fullUser.roles) {
-    user.roles = {}
+    fullUser.roles = {}
   }
   // add the active status to a user if its not provided
-  if (user.status == null) {
+  if (fullUser.status == null) {
-    user.status = constants.UserStatus.ACTIVE
+    fullUser.status = constants.UserStatus.ACTIVE
   }
 
-  return user
+  return fullUser
 }
 
 const validateUniqueUser = async (email: string, tenantId: string) => {
@@ -169,12 +172,16 @@ const validateUniqueUser = async (email: string, tenantId: string) => {
 }
 
 export const save = async (
-  user: User,
+  user: User | ThirdPartyUser,
-  opts: SaveUserOpts = {
+  opts: SaveUserOpts = {}
-    hashPassword: true,
-    requirePassword: true,
-  }
 ): Promise<CreateUserResponse> => {
+  // default booleans to true
+  if (opts.hashPassword == null) {
+    opts.hashPassword = true
+  }
+  if (opts.requirePassword == null) {
+    opts.requirePassword = true
+  }
   const tenantId = tenancy.getTenantId()
   const db = tenancy.getGlobalDB()
 
@@ -213,6 +220,12 @@ export const save = async (
   await validateUniqueUser(email, tenantId)
 
   let builtUser = await buildUser(user, opts, tenantId, dbUser)
+  // don't allow a user to update its own roles/perms
+  if (opts.currentUserId && opts.currentUserId === dbUser?._id) {
+    builtUser.builder = dbUser.builder
+    builtUser.admin = dbUser.admin
+    builtUser.roles = dbUser.roles
+  }
 
   // make sure we set the _id field for a new user
   // Also if this is a new user, associate groups with them