Skip to content

Commit 2f18650

Browse files
author
Vikhyath Mondreti
committed
fix(workspace-sync): preserve activity attribution and update boundary tests
1 parent f0529ef commit 2f18650

10 files changed

Lines changed: 197 additions & 41 deletions

File tree

‎apps/docs/lib/openapi-download.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@ describe('OpenAPI download', () => {
3333
const tags = document.tags as Array<{ name: string }>
3434

3535
expect(document.openapi).toBe('3.1.0')
36-
expect(Object.keys(paths)).toHaveLength(133)
36+
expect(Object.keys(paths)).toHaveLength(152)
3737
expect(tags.map((tag) => tag.name)).toEqual([
38+
'Workspace Sync',
3839
'Workflows',
3940
'Workflow Runs',
4041
'Logs',

‎apps/docs/openapi-v2-workflows.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15727,7 +15727,7 @@
1572715727
"type": "null"
1572815728
}
1572915729
],
15730-
"description": "Opaque continuation cursor, or null when the result is complete."
15730+
"description": "Opaque cursor for the next page. Send it back as `cursor`; null means there is nothing further to fetch. Never construct one yourself."
1573115731
},
1573215732
"truncated": {
1573315733
"type": "boolean",

‎apps/sim/app/api/workspaces/[id]/fork/excluded-workflows/route.test.ts‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,29 @@ import {
1111
} from '@sim/testing'
1212
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
1313

14-
const { mockAssertWorkspaceAdminAccess, mockCaptureServerEvent } = vi.hoisted(() => ({
15-
mockAssertWorkspaceAdminAccess: vi.fn(),
16-
mockCaptureServerEvent: vi.fn(),
17-
}))
14+
const { mockAuthorizeWorkspaceOperation, mockCaptureServerEvent, mockAssertForkingEnabled } =
15+
vi.hoisted(() => ({
16+
mockAuthorizeWorkspaceOperation: vi.fn(),
17+
mockCaptureServerEvent: vi.fn(),
18+
mockAssertForkingEnabled: vi.fn(),
19+
}))
1820

1921
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({
20-
assertWorkspaceAdminAccess: mockAssertWorkspaceAdminAccess,
22+
assertForkingEnabled: mockAssertForkingEnabled,
23+
ForkError: class extends Error {},
24+
}))
25+
26+
vi.mock('@/lib/core/application/workspace-authorization', () => ({
27+
authorizeWorkspaceOperation: mockAuthorizeWorkspaceOperation,
28+
requireAllowedWorkspacePrincipal: vi.fn(),
29+
}))
30+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
31+
getWorkspaceWithOwner: vi.fn(async (id: string) => ({
32+
id,
33+
name: 'My Workspace',
34+
organizationId: null,
35+
allowPersonalApiKeys: true,
36+
})),
2137
}))
2238

2339
vi.mock('@sim/audit', () => auditMock)
@@ -42,8 +58,8 @@ describe('fork excluded-workflows route', () => {
4258
beforeEach(() => {
4359
vi.clearAllMocks()
4460
resetDbChainMock()
45-
mockGetSession.mockResolvedValue({ user: { id: ADMIN_ID } })
46-
mockAssertWorkspaceAdminAccess.mockResolvedValue({ id: WORKSPACE_ID, name: 'My Workspace' })
61+
mockGetSession.mockResolvedValue({ user: { id: ADMIN_ID }, session: { id: 'session-1' } })
62+
mockAuthorizeWorkspaceOperation.mockResolvedValue(undefined)
4763
mockUpdateReturning([])
4864
})
4965

@@ -60,7 +76,7 @@ describe('fork excluded-workflows route', () => {
6076
)
6177

6278
expect(res.status).toBe(401)
63-
expect(mockAssertWorkspaceAdminAccess).not.toHaveBeenCalled()
79+
expect(mockAuthorizeWorkspaceOperation).not.toHaveBeenCalled()
6480
})
6581

6682
it('rejects an empty workflowIds batch', async () => {
@@ -81,7 +97,16 @@ describe('fork excluded-workflows route', () => {
8197
routeContext
8298
)
8399

84-
expect(mockAssertWorkspaceAdminAccess).toHaveBeenCalledWith(WORKSPACE_ID, ADMIN_ID)
100+
expect(mockAuthorizeWorkspaceOperation).toHaveBeenCalledWith(
101+
expect.objectContaining({ kind: 'session', userId: ADMIN_ID }),
102+
expect.objectContaining({ id: 'workspaces.fork.exclusions', minimumRole: 'admin' }),
103+
expect.objectContaining({ workspaceId: WORKSPACE_ID }),
104+
{}
105+
)
106+
expect(mockAssertForkingEnabled).toHaveBeenCalledWith(null)
107+
expect(mockAssertForkingEnabled.mock.invocationCallOrder[0]).toBeLessThan(
108+
dbChainMockFns.update.mock.invocationCallOrder[0]
109+
)
85110
})
86111

87112
it('updates the batch, reports the transition count, and records one audit entry', async () => {

‎apps/sim/app/api/workspaces/[id]/fork/lineage/route.test.ts‎

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,25 @@
33
*/
44
import { authMockFns, createMockRequest } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { OrchestrationError } from '@/lib/core/orchestration/types'
67

78
const {
8-
mockAssertWorkspaceAdminAccess,
9+
mockAuthorizeWorkspaceOperation,
910
mockGetForkParent,
1011
mockGetForkChildren,
1112
mockGetUndoableRunForTarget,
1213
mockGetEffectiveWorkspacePermission,
1314
} = vi.hoisted(() => ({
14-
mockAssertWorkspaceAdminAccess: vi.fn(),
15+
mockAuthorizeWorkspaceOperation: vi.fn(),
1516
mockGetForkParent: vi.fn(),
1617
mockGetForkChildren: vi.fn(),
1718
mockGetUndoableRunForTarget: vi.fn(),
1819
mockGetEffectiveWorkspacePermission: vi.fn(),
1920
}))
2021

2122
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({
22-
assertWorkspaceAdminAccess: mockAssertWorkspaceAdminAccess,
23+
assertForkingEnabled: vi.fn(),
24+
ForkError: class extends Error {},
2325
}))
2426

2527
vi.mock('@/ee/workspace-forking/lib/lineage/lineage', () => ({
@@ -31,7 +33,17 @@ vi.mock('@/ee/workspace-forking/lib/promote/promote-run-store', () => ({
3133
getUndoableRunForTarget: mockGetUndoableRunForTarget,
3234
}))
3335

36+
vi.mock('@/lib/core/application/workspace-authorization', () => ({
37+
authorizeWorkspaceOperation: mockAuthorizeWorkspaceOperation,
38+
requireAllowedWorkspacePrincipal: vi.fn(),
39+
}))
40+
3441
vi.mock('@/lib/workspaces/permissions/utils', () => ({
42+
getWorkspaceWithOwner: vi.fn(async (id: string) => ({
43+
id,
44+
organizationId: null,
45+
allowPersonalApiKeys: true,
46+
})),
3547
getEffectiveWorkspacePermission: mockGetEffectiveWorkspacePermission,
3648
}))
3749

@@ -55,8 +67,8 @@ const childNode = (id: string, name: string) => ({
5567
describe('fork lineage route', () => {
5668
beforeEach(() => {
5769
vi.clearAllMocks()
58-
mockGetSession.mockResolvedValue({ user: { id: VIEWER_ID } })
59-
mockAssertWorkspaceAdminAccess.mockResolvedValue({ id: WORKSPACE_ID })
70+
mockGetSession.mockResolvedValue({ user: { id: VIEWER_ID }, session: { id: 'session-1' } })
71+
mockAuthorizeWorkspaceOperation.mockResolvedValue(undefined)
6072
mockGetForkParent.mockResolvedValue(null)
6173
mockGetForkChildren.mockResolvedValue([])
6274
mockGetUndoableRunForTarget.mockResolvedValue(null)
@@ -69,13 +81,34 @@ describe('fork lineage route', () => {
6981
const res = await GET(createMockRequest('GET'), routeContext)
7082

7183
expect(res.status).toBe(401)
72-
expect(mockAssertWorkspaceAdminAccess).not.toHaveBeenCalled()
84+
expect(mockAuthorizeWorkspaceOperation).not.toHaveBeenCalled()
7385
})
7486

7587
it('requires admin on the current workspace before loading lineage', async () => {
7688
await GET(createMockRequest('GET'), routeContext)
7789

78-
expect(mockAssertWorkspaceAdminAccess).toHaveBeenCalledWith(WORKSPACE_ID, VIEWER_ID)
90+
expect(mockAuthorizeWorkspaceOperation).toHaveBeenCalledWith(
91+
expect.objectContaining({ kind: 'session', userId: VIEWER_ID }),
92+
expect.objectContaining({ id: 'workspaces.fork.discover', minimumRole: 'admin' }),
93+
expect.objectContaining({ workspaceId: WORKSPACE_ID }),
94+
{}
95+
)
96+
expect(mockAuthorizeWorkspaceOperation.mock.invocationCallOrder[0]).toBeLessThan(
97+
mockGetForkParent.mock.invocationCallOrder[0]
98+
)
99+
})
100+
101+
it('does not read lineage when current workspace authorization is refused', async () => {
102+
mockAuthorizeWorkspaceOperation.mockRejectedValue(
103+
new OrchestrationError('forbidden', 'Admin access required')
104+
)
105+
106+
const response = await GET(createMockRequest('GET'), routeContext)
107+
108+
expect(response.status).toBe(403)
109+
expect(mockGetForkParent).not.toHaveBeenCalled()
110+
expect(mockGetForkChildren).not.toHaveBeenCalled()
111+
expect(mockGetUndoableRunForTarget).not.toHaveBeenCalled()
79112
})
80113

81114
it('marks accessible and inaccessible nodes via the canonical permission resolver', async () => {

‎apps/sim/app/api/workspaces/[id]/fork/promote/route.test.ts‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@
88
*
99
* @vitest-environment node
1010
*/
11+
import { user } from '@sim/db/schema'
1112
import { auditMock, authMockFns, createMockRequest, type MockUser } from '@sim/testing'
13+
import { queueTableRows, resetDbChainMock } from '@sim/testing/mocks/database.mock'
1214
import { beforeEach, describe, expect, it, vi } from 'vitest'
1315
import { FolderCollectionFullError } from '@/lib/folders/errors'
1416

15-
const { mockLogger, mockPromoteFork, mockAssertCanPromote } = vi.hoisted(() => ({
17+
const { mockLogger, mockPromoteFork, mockAuthorizeWorkspaceOperation } = vi.hoisted(() => ({
1618
mockLogger: {
1719
info: vi.fn(),
1820
warn: vi.fn(),
@@ -23,7 +25,7 @@ const { mockLogger, mockPromoteFork, mockAssertCanPromote } = vi.hoisted(() => (
2325
child: vi.fn(),
2426
},
2527
mockPromoteFork: vi.fn(),
26-
mockAssertCanPromote: vi.fn(),
28+
mockAuthorizeWorkspaceOperation: vi.fn(),
2729
}))
2830

2931
vi.mock('@sim/audit', () => auditMock)
@@ -34,7 +36,27 @@ vi.mock('@sim/logger', () => ({
3436
}))
3537
vi.mock('@/ee/workspace-forking/lib/promote/promote', () => ({ promoteFork: mockPromoteFork }))
3638
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({
37-
assertCanPromote: mockAssertCanPromote,
39+
assertForkingEnabled: vi.fn(),
40+
ForkError: class extends Error {},
41+
}))
42+
43+
vi.mock('@/lib/core/application/workspace-authorization', () => ({
44+
authorizeWorkspaceOperation: mockAuthorizeWorkspaceOperation,
45+
requireAllowedWorkspacePrincipal: vi.fn(),
46+
}))
47+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
48+
getWorkspaceWithOwner: vi.fn(async (id: string) => ({
49+
id,
50+
name: id === 'ws-child' ? 'Child' : 'Parent',
51+
organizationId: null,
52+
allowPersonalApiKeys: true,
53+
})),
54+
}))
55+
vi.mock('@/ee/workspace-forking/lib/lineage/lineage', () => ({
56+
resolveForkEdge: vi.fn(async () => ({
57+
childWorkspaceId: 'ws-child',
58+
parentWorkspaceId: 'ws-parent',
59+
})),
3860
}))
3961

4062
import { POST } from '@/app/api/workspaces/[id]/fork/promote/route'
@@ -58,19 +80,15 @@ function promoteRequest() {
5880
describe('POST /api/workspaces/[id]/fork/promote', () => {
5981
beforeEach(() => {
6082
vi.clearAllMocks()
61-
authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER })
62-
mockAssertCanPromote.mockResolvedValue({
63-
edge: { childWorkspaceId: WORKSPACE_ID },
64-
sourceWorkspaceId: WORKSPACE_ID,
65-
targetWorkspaceId: 'ws-parent',
66-
source: { name: 'Child' },
67-
target: { name: 'Parent' },
68-
})
83+
authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER, session: { id: 'session-1' } })
84+
resetDbChainMock()
85+
queueTableRows(user, [{ name: TEST_USER.name }])
86+
mockAuthorizeWorkspaceOperation.mockResolvedValue(undefined)
6987
})
7088

7189
/**
72-
* The sync's Activity row is recorded by the use case, not here, so the route's job is to
73-
* hand it the one thing only the route knows: the display name of the edge's other side.
90+
* The shared application use case resolves the other side's name and the actor attribution
91+
* before the manager records the sync activity.
7492
*/
7593
it('names the other side of the edge for promoteFork to record the sync', async () => {
7694
mockPromoteFork.mockResolvedValue({
@@ -80,6 +98,7 @@ describe('POST /api/workspaces/[id]/fork/promote', () => {
8098
archived: 0,
8199
redeployed: 1,
82100
deployFailed: 0,
101+
deployWarnings: [],
83102
unmappedRequired: [],
84103
blockers: [],
85104
blocked: null,

‎apps/sim/app/api/workspaces/[id]/fork/route.test.ts‎

Lines changed: 41 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@
88
*
99
* @vitest-environment node
1010
*/
11+
import { user } from '@sim/db/schema'
1112
import { auditMock, authMockFns, createMockRequest, type MockUser } from '@sim/testing'
13+
import { queueTableRows, resetDbChainMock } from '@sim/testing/mocks/database.mock'
1214
import { beforeEach, describe, expect, it, vi } from 'vitest'
1315
import { FolderCollectionFullError } from '@/lib/folders/errors'
1416

15-
const { mockLogger, mockCreateFork, mockAssertCanFork } = vi.hoisted(() => ({
17+
const { mockLogger, mockCreateFork, mockAuthorizeWorkspaceOperation } = vi.hoisted(() => ({
1618
mockLogger: {
1719
info: vi.fn(),
1820
warn: vi.fn(),
@@ -23,7 +25,7 @@ const { mockLogger, mockCreateFork, mockAssertCanFork } = vi.hoisted(() => ({
2325
child: vi.fn(),
2426
},
2527
mockCreateFork: vi.fn(),
26-
mockAssertCanFork: vi.fn(),
28+
mockAuthorizeWorkspaceOperation: vi.fn(),
2729
}))
2830

2931
vi.mock('@sim/audit', () => auditMock)
@@ -33,7 +35,25 @@ vi.mock('@sim/logger', () => ({
3335
getRequestContext: () => undefined,
3436
}))
3537
vi.mock('@/ee/workspace-forking/lib/create-fork', () => ({ createFork: mockCreateFork }))
36-
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({ assertCanFork: mockAssertCanFork }))
38+
vi.mock('@/ee/workspace-forking/lib/lineage/authz', () => ({
39+
assertForkingEnabled: vi.fn(),
40+
ForkError: class extends Error {},
41+
}))
42+
vi.mock('@/lib/core/application/workspace-authorization', () => ({
43+
authorizeWorkspaceOperation: mockAuthorizeWorkspaceOperation,
44+
requireAllowedWorkspacePrincipal: vi.fn(),
45+
}))
46+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
47+
getWorkspaceWithOwner: vi.fn(async (id: string) => ({
48+
id,
49+
name: 'Source',
50+
organizationId: null,
51+
allowPersonalApiKeys: true,
52+
})),
53+
}))
54+
vi.mock('@/lib/workspaces/policy', () => ({
55+
getWorkspaceCreationPolicy: vi.fn(async () => ({ canCreate: true })),
56+
}))
3757

3858
import { POST } from '@/app/api/workspaces/[id]/fork/route'
3959

@@ -56,11 +76,10 @@ function forkRequest() {
5676
describe('POST /api/workspaces/[id]/fork', () => {
5777
beforeEach(() => {
5878
vi.clearAllMocks()
59-
authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER })
60-
mockAssertCanFork.mockResolvedValue({
61-
source: { id: SOURCE_WORKSPACE_ID, name: 'Source' },
62-
policy: {},
63-
})
79+
authMockFns.mockGetSession.mockResolvedValue({ user: TEST_USER, session: { id: 'session-1' } })
80+
resetDbChainMock()
81+
queueTableRows(user, [{ name: TEST_USER.name }])
82+
mockAuthorizeWorkspaceOperation.mockResolvedValue(undefined)
6483
})
6584

6685
it('renders a full-folder-tree refusal as an actionable 409', async () => {
@@ -100,12 +119,25 @@ describe('POST /api/workspaces/[id]/fork', () => {
100119

101120
it('still returns the created fork when the copy succeeds', async () => {
102121
mockCreateFork.mockResolvedValue({
103-
workspace: { id: 'ws-child', name: 'Child' },
122+
workspace: {
123+
id: 'ws-child',
124+
name: 'Child',
125+
ownerId: TEST_USER.id,
126+
organizationId: null,
127+
workspaceMode: 'personal',
128+
},
104129
workflowsCopied: 2,
105130
})
106131

107132
const response = await POST(forkRequest(), routeContext)
108133

109134
expect(response.status).toBe(201)
135+
expect(mockCreateFork).toHaveBeenCalledWith(
136+
expect.objectContaining({
137+
source: expect.objectContaining({ id: SOURCE_WORKSPACE_ID }),
138+
userId: TEST_USER.id,
139+
actorName: TEST_USER.name,
140+
})
141+
)
110142
})
111143
})

0 commit comments

Comments
 (0)