From 1c73b9259502563fc6d3347fae3e9d3602d39d65 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 21 Mar 2023 13:55:28 +0000 Subject: [PATCH] Fix for app sync, base it on group roles, not just user roles - stops app sync from pulling in group users which do not actually have access to the app. --- .../server/src/api/controllers/application.ts | 37 +++++++++---------- packages/server/src/sdk/users/utils.ts | 5 ++- packages/server/src/utilities/global.ts | 30 ++++++++++----- 3 files changed, 43 insertions(+), 29 deletions(-) diff --git a/packages/server/src/api/controllers/application.ts b/packages/server/src/api/controllers/application.ts index 9cd0d8c14e..02404fdd24 100644 --- a/packages/server/src/api/controllers/application.ts +++ b/packages/server/src/api/controllers/application.ts @@ -44,7 +44,6 @@ import { Layout, Screen, MigrationType, - BBContext, Database, UserCtx, } from "@budibase/types" @@ -74,14 +73,14 @@ async function getScreens() { ).rows.map((row: any) => row.doc) } -function getUserRoleId(ctx: BBContext) { +function getUserRoleId(ctx: UserCtx) { return !ctx.user?.role || !ctx.user.role._id ? roles.BUILTIN_ROLE_IDS.PUBLIC : ctx.user.role._id } function checkAppUrl( - ctx: BBContext, + ctx: UserCtx, apps: App[], url: string, currentAppId?: string @@ -95,7 +94,7 @@ function checkAppUrl( } function checkAppName( - ctx: BBContext, + ctx: UserCtx, apps: App[], name: string, currentAppId?: string @@ -160,7 +159,7 @@ async function addDefaultTables(db: Database) { await db.bulkDocs([...defaultDbDocs]) } -export async function fetch(ctx: BBContext) { +export async function fetch(ctx: UserCtx) { const dev = ctx.query && ctx.query.status === AppStatus.DEV const all = ctx.query && ctx.query.status === AppStatus.ALL const apps = (await dbCore.getAllApps({ dev, all })) as App[] @@ -185,7 +184,7 @@ export async function fetch(ctx: BBContext) { ctx.body = await checkAppMetadata(apps) } -export async function fetchAppDefinition(ctx: BBContext) { +export async function fetchAppDefinition(ctx: UserCtx) { const layouts = await getLayouts() const userRoleId = getUserRoleId(ctx) const accessController = new roles.AccessController() @@ -231,7 +230,7 @@ export async function fetchAppPackage(ctx: UserCtx) { } } -async function performAppCreate(ctx: BBContext) { +async function performAppCreate(ctx: UserCtx) { const apps = (await dbCore.getAllApps({ dev: true })) as App[] const name = ctx.request.body.name, possibleUrl = ctx.request.body.url @@ -360,7 +359,7 @@ async function creationEvents(request: any, app: App) { } } -async function appPostCreate(ctx: BBContext, app: App) { +async function appPostCreate(ctx: UserCtx, app: App) { const tenantId = tenancy.getTenantId() await migrations.backPopulateMigrations({ type: MigrationType.APP, @@ -391,7 +390,7 @@ async function appPostCreate(ctx: BBContext, app: App) { } } -export async function create(ctx: BBContext) { +export async function create(ctx: UserCtx) { const newApplication = await quotas.addApp(() => performAppCreate(ctx)) await appPostCreate(ctx, newApplication) await cache.bustCache(cache.CacheKey.CHECKLIST) @@ -401,7 +400,7 @@ export async function create(ctx: BBContext) { // This endpoint currently operates as a PATCH rather than a PUT // Thus name and url fields are handled only if present -export async function update(ctx: BBContext) { +export async function update(ctx: UserCtx) { const apps = (await dbCore.getAllApps({ dev: true })) as App[] // validation const name = ctx.request.body.name, @@ -421,7 +420,7 @@ export async function update(ctx: BBContext) { ctx.body = app } -export async function updateClient(ctx: BBContext) { +export async function updateClient(ctx: UserCtx) { // Get current app version const db = context.getAppDB() const application = await db.get(DocumentType.APP_METADATA) @@ -445,7 +444,7 @@ export async function updateClient(ctx: BBContext) { ctx.body = app } -export async function revertClient(ctx: BBContext) { +export async function revertClient(ctx: UserCtx) { // Check app can be reverted const db = context.getAppDB() const application = await db.get(DocumentType.APP_METADATA) @@ -471,7 +470,7 @@ export async function revertClient(ctx: BBContext) { ctx.body = app } -const unpublishApp = async (ctx: any) => { +async function unpublishApp(ctx: UserCtx) { let appId = ctx.params.appId appId = dbCore.getProdAppID(appId) @@ -487,7 +486,7 @@ const unpublishApp = async (ctx: any) => { return result } -async function destroyApp(ctx: BBContext) { +async function destroyApp(ctx: UserCtx) { let appId = ctx.params.appId appId = dbCore.getProdAppID(appId) const devAppId = dbCore.getDevAppID(appId) @@ -515,12 +514,12 @@ async function destroyApp(ctx: BBContext) { return result } -async function preDestroyApp(ctx: BBContext) { +async function preDestroyApp(ctx: UserCtx) { const { rows } = await getUniqueRows([ctx.params.appId]) ctx.rowCount = rows.length } -async function postDestroyApp(ctx: BBContext) { +async function postDestroyApp(ctx: UserCtx) { const rowCount = ctx.rowCount await groups.cleanupApp(ctx.params.appId) if (rowCount) { @@ -528,7 +527,7 @@ async function postDestroyApp(ctx: BBContext) { } } -export async function destroy(ctx: BBContext) { +export async function destroy(ctx: UserCtx) { await preDestroyApp(ctx) const result = await destroyApp(ctx) await postDestroyApp(ctx) @@ -536,7 +535,7 @@ export async function destroy(ctx: BBContext) { ctx.body = result } -export const unpublish = async (ctx: BBContext) => { +export async function unpublish(ctx: UserCtx) { const prodAppId = dbCore.getProdAppID(ctx.params.appId) const dbExists = await dbCore.dbExists(prodAppId) @@ -551,7 +550,7 @@ export const unpublish = async (ctx: BBContext) => { ctx.status = 204 } -export async function sync(ctx: BBContext) { +export async function sync(ctx: UserCtx) { const appId = ctx.params.appId try { ctx.body = await sdk.applications.syncApp(appId) diff --git a/packages/server/src/sdk/users/utils.ts b/packages/server/src/sdk/users/utils.ts index f0e0e67824..5c369754a1 100644 --- a/packages/server/src/sdk/users/utils.ts +++ b/packages/server/src/sdk/users/utils.ts @@ -9,7 +9,10 @@ import { isEqual } from "lodash" export function combineMetadataAndUser(user: any, metadata: any) { // skip users with no access - if (user.roleId === rolesCore.BUILTIN_ROLE_IDS.PUBLIC) { + if ( + user.roleId == null || + user.roleId === rolesCore.BUILTIN_ROLE_IDS.PUBLIC + ) { return null } delete user._rev diff --git a/packages/server/src/utilities/global.ts b/packages/server/src/utilities/global.ts index 45e58c8419..a75fcc0b30 100644 --- a/packages/server/src/utilities/global.ts +++ b/packages/server/src/utilities/global.ts @@ -8,7 +8,7 @@ import { } from "@budibase/backend-core" import env from "../environment" import { groups } from "@budibase/pro" -import { BBContext, ContextUser, User } from "@budibase/types" +import { UserCtx, ContextUser, User, UserGroup } from "@budibase/types" export function updateAppRole( user: ContextUser, @@ -43,33 +43,40 @@ export function updateAppRole( async function checkGroupRoles( user: ContextUser, - { appId }: { appId?: string } = {} + opts: { appId?: string; groups?: UserGroup[] } = {} ) { if (user.roleId && user.roleId !== roles.BUILTIN_ROLE_IDS.PUBLIC) { return user } - if (appId) { - user.roleId = await groups.getGroupRoleId(user as User, appId) + if (opts.appId) { + user.roleId = await groups.getGroupRoleId(user as User, opts.appId, { + groups: opts.groups, + }) + } + // final fallback, simply couldn't find a role - user must be public + if (!user.roleId) { + user.roleId = roles.BUILTIN_ROLE_IDS.PUBLIC } return user } async function processUser( user: ContextUser, - { appId }: { appId?: string } = {} + opts: { appId?: string; groups?: UserGroup[] } = {} ) { if (user) { delete user.password } - user = await updateAppRole(user, { appId }) + const appId = opts.appId || context.getAppId() + user = updateAppRole(user, { appId }) if (!user.roleId && user?.userGroups?.length) { - user = await checkGroupRoles(user, { appId }) + user = await checkGroupRoles(user, { appId, groups: opts?.groups }) } return user } -export async function getCachedSelf(ctx: BBContext, appId: string) { +export async function getCachedSelf(ctx: UserCtx, appId: string) { // this has to be tenant aware, can't depend on the context to find it out // running some middlewares before the tenancy causes context to break const user = await cache.user.getUser(ctx.user?._id!) @@ -90,6 +97,7 @@ export async function getGlobalUser(userId: string) { export async function getGlobalUsers(users?: ContextUser[]) { const appId = context.getAppId() const db = tenancy.getGlobalDB() + const allGroups = await groups.fetch() let globalUsers if (users) { const globalIds = users.map(user => @@ -118,7 +126,11 @@ export async function getGlobalUsers(users?: ContextUser[]) { return globalUsers } - return globalUsers.map(user => updateAppRole(user)) + // pass in the groups, meaning we don't actually need to retrieve them for + // each user individually + return Promise.all( + globalUsers.map(user => processUser(user, { groups: allGroups })) + ) } export async function getGlobalUsersFromMetadata(users: ContextUser[]) {