From 97b44bad9451c1fcb466c2040c19c7e3f80f4310 Mon Sep 17 00:00:00 2001 From: Chuck Driesler Date: Tue, 6 Aug 2024 11:25:19 +0100 Subject: [PATCH] fix(workspaces): backend validation on workspace settings fields (#2584) * fix(workspaces): backend validation on workspace settings fields * chore(workspaces): move authz to resolver level --- .../workspaces/graph/resolvers/workspaces.ts | 11 +++- .../modules/workspaces/services/management.ts | 32 ++++------ .../integration/workspaces.graph.spec.ts | 64 +++++++++++++++---- 3 files changed, 72 insertions(+), 35 deletions(-) diff --git a/packages/server/modules/workspaces/graph/resolvers/workspaces.ts b/packages/server/modules/workspaces/graph/resolvers/workspaces.ts index 96109fa03..6ca29eb72 100644 --- a/packages/server/modules/workspaces/graph/resolvers/workspaces.ts +++ b/packages/server/modules/workspaces/graph/resolvers/workspaces.ts @@ -153,6 +153,13 @@ export = FF_WORKSPACES_MODULE_ENABLED update: async (_parent, args, context) => { const { id: workspaceId, ...workspaceInput } = args.input + await authorizeResolver( + context.userId!, + workspaceId, + Roles.Workspace.Admin, + context.resourceAccessRules + ) + const updateWorkspace = updateWorkspaceFactory({ getWorkspace: getWorkspaceFactory({ db }), upsertWorkspace: upsertWorkspaceFactory({ db }), @@ -161,9 +168,7 @@ export = FF_WORKSPACES_MODULE_ENABLED const workspace = await updateWorkspace({ workspaceId, - workspaceInput, - workspaceUpdaterId: context.userId!, - updaterResourceAccessLimits: context.resourceAccessRules + workspaceInput }) return workspace diff --git a/packages/server/modules/workspaces/services/management.ts b/packages/server/modules/workspaces/services/management.ts index ce57e1aec..1e1d141ce 100644 --- a/packages/server/modules/workspaces/services/management.ts +++ b/packages/server/modules/workspaces/services/management.ts @@ -29,7 +29,6 @@ import { import { queryAllWorkspaceProjectsFactory } from '@/modules/workspaces/services/projects' import { EventBus } from '@/modules/shared/services/eventBus' import { removeNullOrUndefinedKeys } from '@speckle/shared' -import { authorizeResolver } from '@/modules/shared' import { isNewResourceAllowed } from '@/modules/core/helpers/token' import { TokenResourceIdentifier, @@ -37,6 +36,7 @@ import { } from '@/modules/core/domain/tokens/types' import { ForbiddenError } from '@/modules/shared/errors' import { validateImageString } from '@/modules/workspaces/helpers/images' +import { isEmpty } from 'lodash' type WorkspaceCreateArgs = { userId: string @@ -96,15 +96,12 @@ export const createWorkspaceFactory = } type WorkspaceUpdateArgs = { - /** Id of user performing the operation */ - workspaceUpdaterId: string workspaceId: string workspaceInput: { name?: string | null description?: string | null logo?: string | null } - updaterResourceAccessLimits: MaybeNullOrUndefined } export const updateWorkspaceFactory = @@ -117,27 +114,20 @@ export const updateWorkspaceFactory = upsertWorkspace: UpsertWorkspace emitWorkspaceEvent: EventBus['emit'] }) => - async ({ - workspaceUpdaterId, - workspaceId, - workspaceInput, - updaterResourceAccessLimits - }: WorkspaceUpdateArgs): Promise => { - await authorizeResolver( - workspaceUpdaterId, - workspaceId, - Roles.Workspace.Admin, - updaterResourceAccessLimits - ) + async ({ workspaceId, workspaceInput }: WorkspaceUpdateArgs): Promise => { + // Get existing workspace to merge with incoming changes + const currentWorkspace = await getWorkspace({ workspaceId }) + if (!currentWorkspace) { + throw new WorkspaceNotFoundError() + } + // Validate incoming changes if (!!workspaceInput.logo) { validateImageString(workspaceInput.logo) } - - const currentWorkspace = await getWorkspace({ workspaceId }) - - if (!currentWorkspace) { - throw new WorkspaceNotFoundError() + if (isEmpty(workspaceInput.name)) { + // Do not allow setting an empty name (empty descriptions allowed) + delete workspaceInput.name } const workspace = { diff --git a/packages/server/modules/workspaces/tests/integration/workspaces.graph.spec.ts b/packages/server/modules/workspaces/tests/integration/workspaces.graph.spec.ts index f15208202..c846ab7f2 100644 --- a/packages/server/modules/workspaces/tests/integration/workspaces.graph.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/workspaces.graph.spec.ts @@ -20,6 +20,10 @@ import { import { Workspace } from '@/modules/workspacesCore/domain/types' import { beforeEachContext } from '@/test/hooks' import { AllScopes } from '@/modules/core/helpers/mainConstants' +import { + BasicTestWorkspace, + createTestWorkspace +} from '@/modules/workspaces/tests/helpers/creation' describe('Workspaces GQL CRUD', () => { let apollo: TestApolloServer @@ -114,27 +118,65 @@ describe('Workspaces GQL CRUD', () => { }) describe('mutation workspaceMutations.update', () => { - it('should update a workspace', async () => { - const createRes = await apollo.execute(CreateWorkspaceDocument, { - input: { name: cryptoRandomString({ length: 6 }) } - }) + const workspace: BasicTestWorkspace = { + id: '', + ownerId: '', + name: cryptoRandomString({ length: 6 }), + description: cryptoRandomString({ length: 12 }) + } + beforeEach(async () => { + await createTestWorkspace(workspace, testUser) + }) + + it('should update a workspace', async () => { const workspaceName = cryptoRandomString({ length: 6 }) - await apollo.execute(UpdateWorkspaceDocument, { + const updateRes = await apollo.execute(UpdateWorkspaceDocument, { input: { - id: createRes.data!.workspaceMutations.create.id, + id: workspace.id, name: workspaceName } }) - const getRes = await apollo.execute(GetWorkspaceDocument, { - workspaceId: createRes.data!.workspaceMutations.create.id + const { data } = await apollo.execute(GetWorkspaceDocument, { + workspaceId: workspace.id }) - expect(createRes).to.not.haveGraphQLErrors() - expect(getRes).to.not.haveGraphQLErrors() - expect(getRes.data?.workspace.name).to.equal(workspaceName) + expect(updateRes).to.not.haveGraphQLErrors() + expect(data?.workspace.name).to.equal(workspaceName) + }) + + it('should not allow workspace name to be empty', async () => { + const updateRes = await apollo.execute(UpdateWorkspaceDocument, { + input: { + id: workspace.id, + name: '' + } + }) + + const { data } = await apollo.execute(GetWorkspaceDocument, { + workspaceId: workspace.id + }) + + expect(updateRes).to.not.haveGraphQLErrors() + expect(data?.workspace.name).to.equal(workspace.name) + }) + + it('should allow workspace description to be empty', async () => { + const updateRes = await apollo.execute(UpdateWorkspaceDocument, { + input: { + id: workspace.id, + description: '' + } + }) + + const { data } = await apollo.execute(GetWorkspaceDocument, { + workspaceId: workspace.id + }) + + expect(updateRes).to.not.haveGraphQLErrors() + expect(data?.workspace.description).to.equal('') }) }) })