Skip to content

Commit bd35758

Browse files
icecrasher321icecrasher321
committed
index migration safety
1 parent 735bd6e commit bd35758

4 files changed

Lines changed: 106 additions & 21 deletions

File tree

‎apps/sim/app/api/organizations/[id]/permission-groups/[groupId]/route.ts‎

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -128,20 +128,20 @@ export const PUT = withRouteHandler(
128128
? false
129129
: group.appliesToAllWorkspaces
130130

131-
let resolvedWorkspaceIds: string[] = []
132-
if (!resolvedAppliesToAll) {
133-
const provided =
134-
updates.workspaceIds !== undefined
135-
? updates.workspaceIds
136-
: (await getGroupWorkspaces(id)).map((ws) => ws.id)
137-
resolvedWorkspaceIds = Array.from(new Set(provided))
138-
if (resolvedWorkspaceIds.length === 0) {
131+
// Resolve and validate explicitly-provided workspaceIds before the
132+
// transaction. When the request omits them for a specific-scope group
133+
// ("keep current"), they're read under the lock instead (see below) so the
134+
// conflict check and the write share one consistent snapshot.
135+
let providedWorkspaceIds: string[] | null = null
136+
if (!resolvedAppliesToAll && updates.workspaceIds !== undefined) {
137+
providedWorkspaceIds = Array.from(new Set(updates.workspaceIds))
138+
if (providedWorkspaceIds.length === 0) {
139139
return NextResponse.json(
140140
{ error: 'Select at least one workspace when the group targets specific workspaces' },
141141
{ status: 400 }
142142
)
143143
}
144-
const invalid = await findWorkspacesNotInOrganization(resolvedWorkspaceIds, organizationId)
144+
const invalid = await findWorkspacesNotInOrganization(providedWorkspaceIds, organizationId)
145145
if (invalid.length > 0) {
146146
return NextResponse.json(
147147
{ error: 'One or more selected workspaces do not belong to this organization' },
@@ -153,12 +153,27 @@ export const PUT = withRouteHandler(
153153
const now = new Date()
154154

155155
await db.transaction(async (tx) => {
156+
// For a specific-scope group the target workspaces are the request's
157+
// explicit ids, or — when omitted ("keep current") — the group's current
158+
// workspaces read under the lock so the conflict check and write share
159+
// one snapshot.
160+
let resolvedWorkspaceIds: string[] = []
161+
156162
// When the scope changes, serialize against other permission-group writes
157163
// for this org and re-check membership conflicts atomically with the
158164
// write, so a concurrent member add (or scope change) can't slip a user
159165
// into two groups that overlap on a workspace.
160166
if (scopeProvided) {
161167
await acquirePermissionGroupOrgLock(tx, organizationId)
168+
169+
if (!resolvedAppliesToAll) {
170+
resolvedWorkspaceIds =
171+
providedWorkspaceIds ?? (await getGroupWorkspaces(id, tx)).map((ws) => ws.id)
172+
if (resolvedWorkspaceIds.length === 0) {
173+
throw new Error('NO_WORKSPACES')
174+
}
175+
}
176+
162177
const members = await tx
163178
.select({ userId: permissionGroupMember.userId })
164179
.from(permissionGroupMember)
@@ -263,6 +278,12 @@ export const PUT = withRouteHandler(
263278
{ status: 409 }
264279
)
265280
}
281+
if (error instanceof Error && error.message === 'NO_WORKSPACES') {
282+
return NextResponse.json(
283+
{ error: 'Select at least one workspace when the group targets specific workspaces' },
284+
{ status: 400 }
285+
)
286+
}
266287
if (getPostgresErrorCode(error) === '23505') {
267288
const constraint = getPostgresConstraintName(error)
268289
if (constraint === PERMISSION_GROUP_CONSTRAINTS.organizationName) {

‎apps/sim/lib/api/contracts/permission-groups.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,24 @@ describe('createPermissionGroupBodySchema', () => {
5757
})
5858
expect(result.success).toBe(true)
5959
})
60+
61+
it('rejects a default group with workspaceIds (appliesToAllWorkspaces omitted)', () => {
62+
const result = createPermissionGroupBodySchema.safeParse({
63+
name: 'Baseline',
64+
isDefault: true,
65+
workspaceIds: ['ws-1'],
66+
})
67+
expect(result.success).toBe(false)
68+
})
69+
70+
it('rejects an all-workspaces group that also names specific workspaces', () => {
71+
const result = createPermissionGroupBodySchema.safeParse({
72+
name: 'Engineering',
73+
appliesToAllWorkspaces: true,
74+
workspaceIds: ['ws-1'],
75+
})
76+
expect(result.success).toBe(false)
77+
})
6078
})
6179

6280
describe('updatePermissionGroupBodySchema', () => {
@@ -88,4 +106,20 @@ describe('updatePermissionGroupBodySchema', () => {
88106
})
89107
expect(result.success).toBe(false)
90108
})
109+
110+
it('rejects workspaceIds when making the group the default', () => {
111+
const result = updatePermissionGroupBodySchema.safeParse({
112+
isDefault: true,
113+
workspaceIds: ['ws-1'],
114+
})
115+
expect(result.success).toBe(false)
116+
})
117+
118+
it('rejects workspaceIds on an all-workspaces update', () => {
119+
const result = updatePermissionGroupBodySchema.safeParse({
120+
appliesToAllWorkspaces: true,
121+
workspaceIds: ['ws-1'],
122+
})
123+
expect(result.success).toBe(false)
124+
})
91125
})

‎apps/sim/lib/api/contracts/permission-groups.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,26 @@ const workspaceIdsSchema = z.array(z.string().min(1)).max(MAX_PERMISSION_GROUP_W
119119
/**
120120
* Enforce the workspace-scope invariants shared by create and update:
121121
* - a specific-scope group (`appliesToAllWorkspaces === false`) must name at
122-
* least one workspace, and
123-
* - the organization default group must apply to all workspaces.
122+
* least one workspace,
123+
* - the organization default group must apply to all workspaces, and
124+
* - an all-workspaces or default group must not name specific workspaces
125+
* (otherwise `workspaceIds` would be silently dropped server-side).
124126
*/
125127
function refineWorkspaceScope(
126128
body: { appliesToAllWorkspaces?: boolean; workspaceIds?: string[]; isDefault?: boolean },
127129
ctx: z.RefinementCtx
128130
) {
131+
// A default group is always org-wide, and an explicit all-workspaces group has
132+
// no specific workspaces. Reject workspaceIds in either case rather than
133+
// silently dropping them when the scope resolves to all-workspaces.
134+
const allWorkspaces = body.isDefault === true || body.appliesToAllWorkspaces === true
135+
if (allWorkspaces && body.workspaceIds && body.workspaceIds.length > 0) {
136+
ctx.addIssue({
137+
code: 'custom',
138+
path: ['workspaceIds'],
139+
message: 'workspaceIds can only be set when the group targets specific workspaces',
140+
})
141+
}
129142
if (body.appliesToAllWorkspaces === false) {
130143
if (!body.workspaceIds || body.workspaceIds.length === 0) {
131144
ctx.addIssue({

‎packages/db/migrations/0238_workspace_scoped_permission_groups.sql‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,34 @@
1-
CREATE TABLE "permission_group_workspace" (
1+
-- Replay-safety: this file ends in CONCURRENTLY index ops below an embedded COMMIT,
2+
-- so a failure there replays the whole file from the top — every statement here is idempotent.
3+
CREATE TABLE IF NOT EXISTS "permission_group_workspace" (
24
"id" text PRIMARY KEY NOT NULL,
35
"permission_group_id" text NOT NULL,
46
"workspace_id" text NOT NULL,
57
"organization_id" text NOT NULL,
68
"created_at" timestamp DEFAULT now() NOT NULL
79
);
810
--> statement-breakpoint
9-
DROP INDEX "permission_group_member_organization_user_unique";--> statement-breakpoint
10-
ALTER TABLE "permission_group" ADD COLUMN "applies_to_all_workspaces" boolean DEFAULT true NOT NULL;--> statement-breakpoint
11-
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_permission_group_id_permission_group_id_fk" FOREIGN KEY ("permission_group_id") REFERENCES "public"."permission_group"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
12-
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_workspace_id_workspace_id_fk" FOREIGN KEY ("workspace_id") REFERENCES "public"."workspace"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
13-
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_organization_id_organization_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organization"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
14-
CREATE INDEX "permission_group_workspace_group_id_idx" ON "permission_group_workspace" USING btree ("permission_group_id");--> statement-breakpoint
15-
CREATE INDEX "permission_group_workspace_workspace_id_idx" ON "permission_group_workspace" USING btree ("workspace_id");--> statement-breakpoint
16-
CREATE UNIQUE INDEX "permission_group_workspace_group_workspace_unique" ON "permission_group_workspace" USING btree ("permission_group_id","workspace_id");--> statement-breakpoint
17-
CREATE INDEX "permission_group_member_organization_user_idx" ON "permission_group_member" USING btree ("organization_id","user_id");
11+
ALTER TABLE "permission_group" ADD COLUMN IF NOT EXISTS "applies_to_all_workspaces" boolean DEFAULT true NOT NULL;--> statement-breakpoint
12+
DO $$ BEGIN
13+
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_permission_group_id_permission_group_id_fk" FOREIGN KEY ("permission_group_id") REFERENCES "public"."permission_group"("id") ON DELETE cascade ON UPDATE no action;
14+
EXCEPTION WHEN duplicate_object THEN null;
15+
END $$;--> statement-breakpoint
16+
DO $$ BEGIN
17+
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_workspace_id_workspace_id_fk" FOREIGN KEY ("workspace_id") REFERENCES "public"."workspace"("id") ON DELETE cascade ON UPDATE no action;
18+
EXCEPTION WHEN duplicate_object THEN null;
19+
END $$;--> statement-breakpoint
20+
DO $$ BEGIN
21+
ALTER TABLE "permission_group_workspace" ADD CONSTRAINT "permission_group_workspace_organization_id_organization_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organization"("id") ON DELETE cascade ON UPDATE no action;
22+
EXCEPTION WHEN duplicate_object THEN null;
23+
END $$;--> statement-breakpoint
24+
CREATE INDEX IF NOT EXISTS "permission_group_workspace_group_id_idx" ON "permission_group_workspace" USING btree ("permission_group_id");--> statement-breakpoint
25+
CREATE INDEX IF NOT EXISTS "permission_group_workspace_workspace_id_idx" ON "permission_group_workspace" USING btree ("workspace_id");--> statement-breakpoint
26+
CREATE UNIQUE INDEX IF NOT EXISTS "permission_group_workspace_group_workspace_unique" ON "permission_group_workspace" USING btree ("permission_group_id","workspace_id");--> statement-breakpoint
27+
-- permission_group_member is an existing table: swap its (organization_id, user_id)
28+
-- index CONCURRENTLY so the build/drop never write-locks the relation (runner
29+
-- convention — plain CREATE/DROP INDEX takes ACCESS EXCLUSIVE for the whole op).
30+
COMMIT;--> statement-breakpoint
31+
SET lock_timeout = 0;--> statement-breakpoint
32+
CREATE INDEX CONCURRENTLY IF NOT EXISTS "permission_group_member_organization_user_idx" ON "permission_group_member" USING btree ("organization_id","user_id");--> statement-breakpoint
33+
DROP INDEX CONCURRENTLY IF EXISTS "permission_group_member_organization_user_unique";--> statement-breakpoint
34+
SET lock_timeout = '5s';

0 commit comments

Comments
 (0)