From 06a7e71b838db16072cbc2946ae3e5dd67eaf61e Mon Sep 17 00:00:00 2001 From: Charles Driesler Date: Tue, 3 Dec 2024 15:12:55 +0000 Subject: [PATCH 1/3] fix(sso): disable slug edit if sso enabled --- .../settings/workspaces/General.vue | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/frontend-2/components/settings/workspaces/General.vue b/packages/frontend-2/components/settings/workspaces/General.vue index d17117e892..e96da6afb3 100644 --- a/packages/frontend-2/components/settings/workspaces/General.vue +++ b/packages/frontend-2/components/settings/workspaces/General.vue @@ -26,14 +26,14 @@ label="Short ID" name="shortId" :help="slugHelp" - :disabled="!isAdmin || needsSsoLogin" + :disabled="disableSlugInput" show-label label-position="left" - :tooltip-text="disabledTooltipText" + :tooltip-text="disabledSlugTooltipText" read-only - :right-icon="isAdmin || needsSsoLogin ? IconEdit : undefined" - right-icon-title="Edit short ID" - @right-icon-click="openSlugEditDialog" + :right-icon="disableSlugInput ? undefined : IconEdit" + :right-icon-title="disableSlugInput ? undefined : 'Edit short ID'" + @right-icon-click="disableSlugInput ? undefined : openSlugEditDialog" />
workspaceResult.value?.workspace?.slug || '') }) @@ -319,7 +319,16 @@ const disabledTooltipText = computed(() => { return undefined }) +const disableSlugInput = computed(() => !isAdmin.value || hasSsoEnabled.value) + +const disabledSlugTooltipText = computed(() => { + return hasSsoEnabled.value + ? 'Short ID cannot be changed while SSO is enabled.' + : disabledTooltipText.value +}) + const openSlugEditDialog = () => { + if (hasSsoEnabled.value) return showEditSlugDialog.value = true } From be43973734425092084f03ceec2a58c95d817e1f Mon Sep 17 00:00:00 2001 From: Charles Driesler Date: Tue, 3 Dec 2024 15:33:03 +0000 Subject: [PATCH 2/3] fix(sso): guard on backend --- .../workspaces/graph/resolvers/workspaces.ts | 8 ++++ .../modules/workspaces/services/management.ts | 16 +++++++- .../workspaces/tests/helpers/creation.ts | 1 + .../tests/unit/services/management.spec.ts | 38 +++++++++++++++++++ 4 files changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/server/modules/workspaces/graph/resolvers/workspaces.ts b/packages/server/modules/workspaces/graph/resolvers/workspaces.ts index e9abe5fa05..e16e7bb6e1 100644 --- a/packages/server/modules/workspaces/graph/resolvers/workspaces.ts +++ b/packages/server/modules/workspaces/graph/resolvers/workspaces.ts @@ -517,6 +517,10 @@ export = FF_WORKSPACES_MODULE_ENABLED getWorkspaceBySlug: getWorkspaceBySlugFactory({ db }) }), getWorkspace: getWorkspaceWithDomainsFactory({ db }), + getWorkspaceSsoProviderRecord: getWorkspaceSsoProviderFactory({ + db, + decrypt: getDecryptor() + }), upsertWorkspace: upsertWorkspaceFactory({ db }), emitWorkspaceEvent: getEventBus().emit }) @@ -620,6 +624,10 @@ export = FF_WORKSPACES_MODULE_ENABLED getWorkspaceBySlug: getWorkspaceBySlugFactory({ db }) }), getWorkspace: getWorkspaceWithDomainsFactory({ db }), + getWorkspaceSsoProviderRecord: getWorkspaceSsoProviderFactory({ + db, + decrypt: getDecryptor() + }), upsertWorkspace: upsertWorkspaceFactory({ db }), emitWorkspaceEvent: getEventBus().emit }) diff --git a/packages/server/modules/workspaces/services/management.ts b/packages/server/modules/workspaces/services/management.ts index 98c2c3f9ba..97b3c9523d 100644 --- a/packages/server/modules/workspaces/services/management.ts +++ b/packages/server/modules/workspaces/services/management.ts @@ -64,7 +64,10 @@ import { userEmailsCompliantWithWorkspaceDomains } from '@/modules/workspaces/do import { workspaceRoles as workspaceRoleDefinitions } from '@/modules/workspaces/roles' import { blockedDomains } from '@speckle/shared' import { DeleteStreamRecord } from '@/modules/core/domain/streams/operations' -import { DeleteSsoProvider } from '@/modules/workspaces/domain/sso/operations' +import { + DeleteSsoProvider, + GetWorkspaceSsoProviderRecord +} from '@/modules/workspaces/domain/sso/operations' type WorkspaceCreateArgs = { userId: string @@ -230,11 +233,13 @@ const sanitizeInput = (input: Partial) => { export const updateWorkspaceFactory = ({ getWorkspace, + getWorkspaceSsoProviderRecord, validateSlug, upsertWorkspace, emitWorkspaceEvent }: { getWorkspace: GetWorkspaceWithDomains + getWorkspaceSsoProviderRecord: GetWorkspaceSsoProviderRecord validateSlug: ValidateWorkspaceSlug upsertWorkspace: UpsertWorkspace emitWorkspaceEvent: EventBus['emit'] @@ -253,7 +258,14 @@ export const updateWorkspaceFactory = throw new WorkspaceInvalidUpdateError() } - if (workspaceInput.slug) await validateSlug({ slug: workspaceInput.slug }) + if (workspaceInput.slug) { + const ssoProvider = await getWorkspaceSsoProviderRecord({ workspaceId }) + if (ssoProvider) + throw new WorkspaceInvalidUpdateError( + 'Cannot update workspace slug if SSO is configured.' + ) + await validateSlug({ slug: workspaceInput.slug }) + } const workspace = { ...omit(currentWorkspace, 'domains'), diff --git a/packages/server/modules/workspaces/tests/helpers/creation.ts b/packages/server/modules/workspaces/tests/helpers/creation.ts index 6b0d31356a..3a25d52eff 100644 --- a/packages/server/modules/workspaces/tests/helpers/creation.ts +++ b/packages/server/modules/workspaces/tests/helpers/creation.ts @@ -196,6 +196,7 @@ export const createTestWorkspace = async ( getWorkspaceBySlug: getWorkspaceBySlugFactory({ db }) }), getWorkspace: getWorkspaceWithDomainsFactory({ db }), + getWorkspaceSsoProviderRecord: getWorkspaceSsoProviderRecordFactory({ db }), upsertWorkspace: upsertWorkspaceFactory({ db }), emitWorkspaceEvent: (...args) => getEventBus().emit(...args) }) diff --git a/packages/server/modules/workspaces/tests/unit/services/management.spec.ts b/packages/server/modules/workspaces/tests/unit/services/management.spec.ts index 767f3f211b..db0bcf6bd2 100644 --- a/packages/server/modules/workspaces/tests/unit/services/management.spec.ts +++ b/packages/server/modules/workspaces/tests/unit/services/management.spec.ts @@ -271,6 +271,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => null, + getWorkspaceSsoProviderRecord: async () => null, validateSlug: async () => { expect.fail() }, @@ -292,6 +293,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => { expect.fail() }, @@ -315,6 +317,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => { expect.fail() }, @@ -338,6 +341,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => { expect.fail() }, @@ -361,6 +365,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => { expect.fail() }, @@ -383,6 +388,7 @@ describe('Workspace services', () => { const err = await expectToThrow(async () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => { expect.fail() }, @@ -406,6 +412,7 @@ describe('Workspace services', () => { let newWorkspaceName await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => {}, validateSlug: async () => {}, @@ -418,6 +425,36 @@ describe('Workspace services', () => { }) expect(newWorkspaceName).to.be.equal(workspace.name) }) + + it('does not allow updating the workspace slug if SSO is enabled', async () => { + const workspace = createTestWorkspaceWithDomainsData() + + const err = await expectToThrow(async () => { + await updateWorkspaceFactory({ + getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => ({ + workspaceId: 'foo', + providerId: 'bar' + }), + emitWorkspaceEvent: async () => { + expect.fail() + }, + validateSlug: async () => {}, + upsertWorkspace: async () => { + expect.fail() + } + })({ + workspaceId: workspace.id, + workspaceInput: { + slug: 'new-slug' + } + }) + }) + expect(err.message).to.be.equal( + 'Cannot update workspace slug if SSO is configured.' + ) + }) + it('updates the workspace and emits the correct event payload', async () => { const workspaceId = cryptoRandomString({ length: 10 }) const workspace = createTestWorkspaceWithDomainsData({ @@ -444,6 +481,7 @@ describe('Workspace services', () => { await updateWorkspaceFactory({ getWorkspace: async () => workspace, + getWorkspaceSsoProviderRecord: async () => null, emitWorkspaceEvent: async () => {}, validateSlug: async () => {}, upsertWorkspace: async ({ workspace }) => { From 6716916812600b3738da9ca6816e92168a1f90f2 Mon Sep 17 00:00:00 2001 From: Charles Driesler Date: Tue, 3 Dec 2024 15:44:00 +0000 Subject: [PATCH 3/3] chore(sso): more test fixes --- .../modules/workspaces/tests/integration/subs.graph.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/server/modules/workspaces/tests/integration/subs.graph.spec.ts b/packages/server/modules/workspaces/tests/integration/subs.graph.spec.ts index af501e7afc..36eeaaceea 100644 --- a/packages/server/modules/workspaces/tests/integration/subs.graph.spec.ts +++ b/packages/server/modules/workspaces/tests/integration/subs.graph.spec.ts @@ -2,6 +2,7 @@ import { db } from '@/db/knex' import { ServerInvites } from '@/modules/core/dbSchema' import { getEventBus } from '@/modules/shared/services/eventBus' import { getDefaultRegionFactory } from '@/modules/workspaces/repositories/regions' +import { getWorkspaceSsoProviderRecordFactory } from '@/modules/workspaces/repositories/sso' import { getWorkspaceBySlugFactory, getWorkspaceWithDomainsFactory, @@ -59,6 +60,7 @@ const updateWorkspace = updateWorkspaceFactory({ getWorkspaceBySlug: getWorkspaceBySlugFactory({ db }) }), getWorkspace: getWorkspaceWithDomainsFactory({ db }), + getWorkspaceSsoProviderRecord: getWorkspaceSsoProviderRecordFactory({ db }), upsertWorkspace: upsertWorkspaceFactory({ db }), emitWorkspaceEvent: getEventBus().emit })