Merge pull request #8955 from Budibase/fix/7732

Dis-allow users updating their roles/deleting themselves through the public API

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

@@ -5,9 +5,18 @@ import {
   saveGlobalUser,
 } from "../../../utilities/workerRequests"
 import { publicApiUserFix } from "../../../utilities/users"
+import { db as dbCore } from "@budibase/backend-core"
 import { search as stringSearch } from "./utils"
+import { BBContext, User } from "@budibase/types"
 
-function getUser(ctx: any, userId?: string) {
+function isLoggedInUser(ctx: BBContext, user: User) {
+  const loggedInId = ctx.user?._id
+  const globalUserId = dbCore.getGlobalIDFromUserMetadataID(loggedInId!)
+  // check both just incase
+  return globalUserId === user._id || loggedInId === user._id
+}
+
+function getUser(ctx: BBContext, userId?: string) {
   if (userId) {
     ctx.params = { userId }
   } else if (!ctx.params?.userId) {
@@ -16,37 +25,45 @@ function getUser(ctx: any, userId?: string) {
   return readGlobalUser(ctx)
 }
 
-export async function search(ctx: any, next: any) {
+export async function search(ctx: BBContext, next: any) {
   const { name } = ctx.request.body
   const users = await allGlobalUsers(ctx)
   ctx.body = stringSearch(users, name, "email")
   await next()
 }
 
-export async function create(ctx: any, next: any) {
+export async function create(ctx: BBContext, next: any) {
   const response = await saveGlobalUser(publicApiUserFix(ctx))
   ctx.body = await getUser(ctx, response._id)
   await next()
 }
 
-export async function read(ctx: any, next: any) {
+export async function read(ctx: BBContext, next: any) {
   ctx.body = await readGlobalUser(ctx)
   await next()
 }
 
-export async function update(ctx: any, next: any) {
+export async function update(ctx: BBContext, next: any) {
   const user = await readGlobalUser(ctx)
   ctx.request.body = {
     ...ctx.request.body,
     _rev: user._rev,
   }
+  // disallow updating your own role - always overwrite with DB roles
+  if (isLoggedInUser(ctx, user)) {
+    ctx.request.body.roles = user.roles
+  }
   const response = await saveGlobalUser(publicApiUserFix(ctx))
   ctx.body = await getUser(ctx, response._id)
   await next()
 }
 
-export async function destroy(ctx: any, next: any) {
+export async function destroy(ctx: BBContext, next: any) {
   const user = await getUser(ctx)
+  // disallow deleting yourself
+  if (isLoggedInUser(ctx, user)) {
+    ctx.throw(405, "Cannot delete user using its own API key.")
+  }
   await deleteGlobalUser(ctx)
   ctx.body = user
   await next()

packages/server/src/api/routes/public/tests/compare.spec.js

@@ -1,41 +1,23 @@
 const jestOpenAPI = require("jest-openapi").default
 const generateSchema = require("../../../../../specs/generate")
 const setup = require("../../tests/utilities")
-const { checkSlashesInUrl } = require("../../../../utilities")
+const { generateMakeRequest } = require("./utils")
 
 const yamlPath = generateSchema()
 jestOpenAPI(yamlPath)
 
-let request = setup.getRequest()
 let config = setup.getConfig()
-let apiKey, table, app
+let apiKey, table, app, makeRequest
 
 beforeAll(async () => {
   app = await config.init()
   table = await config.updateTable()
   apiKey = await config.generateApiKey()
+  makeRequest = generateMakeRequest(apiKey, setup)
 })
 
 afterAll(setup.afterAll)
 
-async function makeRequest(method, endpoint, body, appId = config.getAppId()) {
-  const extraHeaders = {
-    "x-budibase-api-key": apiKey,
-  }
-  if (appId) {
-    extraHeaders["x-budibase-app-id"] = appId
-  }
-  const req = request
-    [method](checkSlashesInUrl(`/api/public/v1/${endpoint}`))
-    .set(config.defaultHeaders(extraHeaders))
-  if (body) {
-    req.send(body)
-  }
-  const res = await req
-  expect(res.body).toBeDefined()
-  return res
-}
-
 describe("check the applications endpoints", () => {
   it("should allow retrieving applications through search", async () => {
     const res = await makeRequest("post", "/applications/search")

packages/server/src/api/routes/public/tests/users.spec.js

@@ -0,0 +1,38 @@
+const setup = require("../../tests/utilities")
+const { generateMakeRequest } = require("./utils")
+
+const workerRequests = require("../../../../utilities/workerRequests")
+
+let config = setup.getConfig()
+let apiKey, globalUser, makeRequest
+
+beforeAll(async () => {
+  await config.init()
+  globalUser = await config.globalUser()
+  apiKey = await config.generateApiKey(globalUser._id)
+  makeRequest = generateMakeRequest(apiKey, setup)
+  workerRequests.readGlobalUser.mockReturnValue(globalUser)
+})
+
+afterAll(setup.afterAll)
+
+describe("check user endpoints", () => {
+  it("should not allow a user to update their own roles", async () => {
+    const res = await makeRequest("put", `/users/${globalUser._id}`, {
+      ...globalUser,
+      roles: {
+        "app_1": "ADMIN",
+      }
+    })
+    expect(workerRequests.saveGlobalUser.mock.lastCall[0].body.data.roles["app_1"]).toBeUndefined()
+    expect(res.status).toBe(200)
+    expect(res.body.data.roles["app_1"]).toBeUndefined()
+  })
+
+  it("should not allow a user to delete themselves", async () => {
+    const res = await makeRequest("delete", `/users/${globalUser._id}`)
+    expect(res.status).toBe(405)
+    expect(workerRequests.deleteGlobalUser.mock.lastCall).toBeUndefined()
+  })
+})
+

packages/server/src/api/routes/public/tests/utils.ts

@@ -0,0 +1,28 @@
+import { checkSlashesInUrl } from "../../../../utilities"
+
+export function generateMakeRequest(apiKey: string, setup: any) {
+  const request = setup.getRequest()
+  const config = setup.getConfig()
+  return async (
+    method: string,
+    endpoint: string,
+    body?: any,
+    intAppId: string = config.getAppId()
+  ) => {
+    const extraHeaders: any = {
+      "x-budibase-api-key": apiKey,
+    }
+    if (intAppId) {
+      extraHeaders["x-budibase-app-id"] = intAppId
+    }
+    const req = request[method](
+      checkSlashesInUrl(`/api/public/v1/${endpoint}`)
+    ).set(config.defaultHeaders(extraHeaders))
+    if (body) {
+      req.send(body)
+    }
+    const res = await req
+    expect(res.body).toBeDefined()
+    return res
+  }
+}

packages/server/src/utilities/workerRequests.ts

@@ -3,7 +3,7 @@ import env from "../environment"
 import { checkSlashesInUrl } from "./index"
 import { db as dbCore, constants, tenancy } from "@budibase/backend-core"
 import { updateAppRole } from "./global"
-import { BBContext, Automation } from "@budibase/types"
+import { BBContext, User } from "@budibase/types"
 
 export function request(ctx?: BBContext, request?: any) {
   if (!request.headers) {
@@ -138,7 +138,7 @@ export async function deleteGlobalUser(ctx: BBContext) {
   return checkResponse(response, "delete user", { ctx })
 }
 
-export async function readGlobalUser(ctx: BBContext) {
+export async function readGlobalUser(ctx: BBContext): Promise<User> {
   const response = await fetch(
     checkSlashesInUrl(
       env.WORKER_URL + `/api/global/users/${ctx.params.userId}`