Skip to content

Commit 59addad

Browse files
BunsDevCopilot
BunsDev
and
Copilot
authored
fix(onboarding): restrict bootstrap admin promotion (#36)
* fix(onboarding): restrict bootstrap admin promotion * fix(onboarding): serialize bootstrap admin checks and gate fresh use-case insert --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 1eb1874 commit 59addad

3 files changed

Lines changed: 307 additions & 45 deletions

File tree

‎apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts‎

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest'
2+
3+
type AnyHandler = (args: { data: Record<string, unknown> }) => Promise<unknown>
4+
5+
vi.mock('@tanstack/react-start', () => ({
6+
createServerFn: () => ({
7+
inputValidator: () => ({
8+
handler: (fn: AnyHandler) => fn,
9+
}),
10+
handler: (fn: AnyHandler) => fn,
11+
}),
12+
}))
13+
14+
const hoisted = vi.hoisted(() => ({
15+
mockGetSession: vi.fn(),
16+
mockGetSettings: vi.fn(),
17+
mockAssertNotManaged: vi.fn(),
18+
mockInvalidateSettingsCache: vi.fn(),
19+
mockPrincipalFindFirst: vi.fn(),
20+
mockTxExecute: vi.fn(),
21+
mockUpdateSet: vi.fn(),
22+
mockUpdateWhere: vi.fn(),
23+
mockInsertValues: vi.fn(),
24+
mockTransaction: vi.fn(),
25+
mockEq: vi.fn((column: string, value: unknown) => ({ op: 'eq', column, value })),
26+
mockAnd: vi.fn((...conditions: unknown[]) => ({ op: 'and', conditions })),
27+
mockNe: vi.fn((column: string, value: unknown) => ({ op: 'ne', column, value })),
28+
mockGenerateId: vi.fn(),
29+
}))
30+
31+
vi.mock('@/lib/server/auth/session', () => ({
32+
getSession: hoisted.mockGetSession,
33+
}))
34+
35+
vi.mock('../workspace', () => ({
36+
getSettings: hoisted.mockGetSettings,
37+
}))
38+
39+
vi.mock('@/lib/server/config-file/managed-guard', () => ({
40+
assertNotManaged: hoisted.mockAssertNotManaged,
41+
}))
42+
43+
vi.mock('@/lib/server/domains/settings/settings.helpers', () => ({
44+
invalidateSettingsCache: hoisted.mockInvalidateSettingsCache,
45+
}))
46+
47+
vi.mock('@opencoven-feedback/ids', () => ({
48+
generateId: hoisted.mockGenerateId,
49+
}))
50+
51+
vi.mock('@/lib/server/domains/principals/principal.service', () => ({
52+
syncPrincipalProfile: vi.fn(),
53+
}))
54+
55+
vi.mock('@/lib/server/domains/boards/board.service', () => ({
56+
listBoards: vi.fn(),
57+
}))
58+
59+
vi.mock('@/lib/server/domains/settings', () => ({
60+
DEFAULT_AUTH_CONFIG: {},
61+
DEFAULT_PORTAL_CONFIG: {},
62+
}))
63+
64+
vi.mock('@/lib/server/config-file/managed-paths', () => ({
65+
isPathManaged: vi.fn(() => false),
66+
}))
67+
68+
vi.mock('@/lib/server/db', () => ({
69+
USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'],
70+
DEFAULT_STATUSES: [],
71+
settings: { id: 'settings.id' },
72+
principal: {
73+
id: 'principal.id',
74+
userId: 'principal.userId',
75+
role: 'principal.role',
76+
type: 'principal.type',
77+
},
78+
user: { id: 'user.id' },
79+
postStatuses: {},
80+
eq: hoisted.mockEq,
81+
and: hoisted.mockAnd,
82+
ne: hoisted.mockNe,
83+
sql: (strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values }),
84+
db: {
85+
query: {
86+
principal: {
87+
findFirst: hoisted.mockPrincipalFindFirst,
88+
},
89+
postStatuses: {
90+
findFirst: vi.fn(),
91+
},
92+
},
93+
update: vi.fn(() => ({
94+
set: hoisted.mockUpdateSet.mockImplementation(() => ({
95+
where: hoisted.mockUpdateWhere.mockResolvedValue(undefined),
96+
returning: vi.fn().mockResolvedValue([]),
97+
})),
98+
})),
99+
insert: vi.fn(() => ({
100+
values: hoisted.mockInsertValues.mockResolvedValue(undefined),
101+
})),
102+
transaction: hoisted.mockTransaction.mockImplementation(async (fn: (tx: unknown) => unknown) =>
103+
fn({
104+
execute: hoisted.mockTxExecute.mockResolvedValue(undefined),
105+
query: {
106+
principal: {
107+
findFirst: hoisted.mockPrincipalFindFirst,
108+
},
109+
},
110+
update: vi.fn(() => ({
111+
set: hoisted.mockUpdateSet.mockImplementation(() => ({
112+
where: hoisted.mockUpdateWhere.mockResolvedValue(undefined),
113+
})),
114+
})),
115+
insert: vi.fn(() => ({
116+
values: hoisted.mockInsertValues.mockResolvedValue(undefined),
117+
})),
118+
})
119+
),
120+
},
121+
}))
122+
123+
const { saveUseCaseFn } = await import('../onboarding')
124+
125+
const incompleteSettings = {
126+
id: 'workspace_existing',
127+
setupState: JSON.stringify({
128+
version: 1,
129+
steps: { core: true, workspace: false, boards: false },
130+
}),
131+
}
132+
133+
describe('saveUseCaseFn onboarding admin promotion', () => {
134+
beforeEach(() => {
135+
vi.clearAllMocks()
136+
hoisted.mockGetSession.mockResolvedValue({ user: { id: 'user_attacker' } })
137+
hoisted.mockGetSettings.mockResolvedValue(incompleteSettings)
138+
hoisted.mockAssertNotManaged.mockResolvedValue(undefined)
139+
hoisted.mockInvalidateSettingsCache.mockResolvedValue(undefined)
140+
hoisted.mockGenerateId.mockReturnValue('principal_new_admin')
141+
})
142+
143+
it('does not promote an existing non-admin principal when a human admin already exists', async () => {
144+
hoisted.mockPrincipalFindFirst
145+
.mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' })
146+
.mockResolvedValueOnce({
147+
id: 'principal_admin',
148+
userId: 'user_admin',
149+
role: 'admin',
150+
type: 'user',
151+
})
152+
153+
await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow(
154+
'Only admin can complete setup'
155+
)
156+
157+
expect(hoisted.mockUpdateSet).not.toHaveBeenCalled()
158+
expect(hoisted.mockInsertValues).not.toHaveBeenCalled()
159+
})
160+
161+
it('does not promote a non-admin principal when another human principal already exists', async () => {
162+
hoisted.mockPrincipalFindFirst
163+
.mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' })
164+
.mockResolvedValueOnce(null)
165+
.mockResolvedValueOnce({ id: 'principal_other', userId: 'user_other', role: 'user' })
166+
167+
await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow(
168+
'Only admin can complete setup'
169+
)
170+
171+
expect(hoisted.mockUpdateSet).not.toHaveBeenCalled()
172+
expect(hoisted.mockInsertValues).not.toHaveBeenCalled()
173+
})
174+
175+
it('still bootstraps an admin principal when no other human principal exists', async () => {
176+
hoisted.mockPrincipalFindFirst
177+
.mockResolvedValueOnce(null)
178+
.mockResolvedValueOnce(null)
179+
.mockResolvedValueOnce(null)
180+
181+
await saveUseCaseFn({ data: { useCase: 'saas' } })
182+
183+
expect(hoisted.mockInsertValues).toHaveBeenCalledWith(
184+
expect.objectContaining({
185+
id: 'principal_new_admin',
186+
userId: 'user_attacker',
187+
role: 'admin',
188+
})
189+
)
190+
expect(hoisted.mockUpdateSet).toHaveBeenCalledWith({
191+
setupState: JSON.stringify({
192+
version: 1,
193+
steps: { core: true, workspace: false, boards: false },
194+
useCase: 'saas',
195+
}),
196+
})
197+
expect(hoisted.mockInvalidateSettingsCache).toHaveBeenCalledOnce()
198+
})
199+
200+
it('rejects before inserting settings on fresh install when another human principal exists', async () => {
201+
hoisted.mockGetSettings.mockResolvedValue(null)
202+
hoisted.mockPrincipalFindFirst
203+
.mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' })
204+
.mockResolvedValueOnce(null)
205+
.mockResolvedValueOnce({ id: 'principal_other', userId: 'user_other', role: 'user' })
206+
207+
await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow(
208+
'Only admin can complete setup'
209+
)
210+
211+
expect(hoisted.mockInsertValues).not.toHaveBeenCalled()
212+
})
213+
})

‎apps/web/src/lib/server/functions/__tests__/onboarding.test.ts‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ const { state, principalTable, settingsTable, postStatusesTable, userTable } = v
2424
statusesExist: true,
2525
},
2626
principalTable: {
27+
id: 'principal.id',
2728
userId: 'principal.userId',
2829
role: 'principal.role',
2930
type: 'principal.type',
@@ -87,23 +88,20 @@ function matchesWhere(row: Record<string, unknown>, where: unknown): boolean {
8788
if (clause.op === 'and') {
8889
return clause.conditions?.every((condition) => matchesWhere(row, condition)) ?? false
8990
}
91+
if (clause.op === 'ne') {
92+
if (clause.col === principalTable.id) return row.id !== clause.value
93+
return false
94+
}
9095
if (clause.col === principalTable.userId) return row.userId === clause.value
96+
if (clause.col === principalTable.id) return row.id === clause.value
9197
if (clause.col === principalTable.role) return row.role === clause.value
9298
if (clause.col === principalTable.type) return row.type === clause.value
9399
if (clause.col === settingsTable.id) return row.id === clause.value
94100
return false
95101
}
96102

97-
vi.mock('@/lib/server/db', () => ({
98-
USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'],
99-
DEFAULT_STATUSES: [],
100-
principal: principalTable,
101-
settings: settingsTable,
102-
postStatuses: postStatusesTable,
103-
user: userTable,
104-
eq: (col: string, value: unknown) => ({ op: 'eq', col, value }),
105-
and: (...conditions: unknown[]) => ({ op: 'and', conditions }),
106-
db: {
103+
vi.mock('@/lib/server/db', () => {
104+
const dbMock = {
107105
query: {
108106
principal: {
109107
findFirst: async ({ where }: { where: unknown }) =>
@@ -152,8 +150,24 @@ vi.mock('@/lib/server/db', () => ({
152150
},
153151
}),
154152
}),
155-
},
156-
}))
153+
execute: async () => undefined,
154+
transaction: async (fn: (tx: unknown) => unknown) => fn(dbMock),
155+
}
156+
157+
return {
158+
USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'],
159+
DEFAULT_STATUSES: [],
160+
principal: principalTable,
161+
settings: settingsTable,
162+
postStatuses: postStatusesTable,
163+
user: userTable,
164+
eq: (col: string, value: unknown) => ({ op: 'eq', col, value }),
165+
and: (...conditions: unknown[]) => ({ op: 'and', conditions }),
166+
ne: (col: string, value: unknown) => ({ op: 'ne', col, value }),
167+
sql: (strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values }),
168+
db: dbMock,
169+
}
170+
})
157171

158172
beforeEach(() => {
159173
state.sessionUserId = 'user_attacker'

0 commit comments

Comments
 (0)