Restrict stack merge approval overrides to repo admins
swadhinbiswas committed 8 months ago
6569299
1 parent cb06ef7
commit 6569299
3 files changed
+34 -4
Showing 3 changed files+34-4
▼src/lib/openapi.ts
+1-1
| @@ -534,7 +534,7 @@ export const openApiSpec = { | ||
534 | 534 | post: { |
535 | 535 | tags: ["Stacks"], |
536 | 536 | summary: "Queue merge for all PRs in a stack", |
537 | - description: "Attempts to merge an entire stack in order by enqueuing each eligible PR into the merge queue.", | |
537 | + description: "Attempts to merge an entire stack in order by enqueuing each eligible PR into the merge queue. `skipApprovalCheck` requires repository admin privileges.", | |
538 | 538 | security: [{ bearerAuth: [] }], |
539 | 539 | parameters: [ |
540 | 540 | { name: "owner", in: "path", required: true, schema: { type: "string" } }, |
541 | 541 | |
▼src/pages/api/repos/[owner]/[repo]/stacks/[stackId]/merge.ts
+7-2
| @@ -3,7 +3,7 @@ import type { NodePgDatabase } from "drizzle-orm/node-postgres"; | ||
3 | 3 | import { and, eq } from "drizzle-orm"; |
4 | 4 | import { z } from "zod"; |
5 | 5 | import { getDatabase, schema } from "@/db"; |
6 | -import { canWriteRepo } from "@/lib/permissions"; | |
6 | +import { canAdminRepo, canWriteRepo } from "@/lib/permissions"; | |
7 | 7 | import { withErrorHandler } from "@/lib/errors"; |
8 | 8 | import { badRequest, notFound, success, unauthorized, forbidden } from "@/lib/api"; |
9 | 9 | import { bulkMergeStack } from "@/lib/bulk-merge"; |
| @@ -48,6 +48,12 @@ export const POST: APIRoute = withErrorHandler(async ({ params, locals, request | ||
48 | 48 | if (!parsed.success) { |
49 | 49 | return badRequest(parsed.error.issues[0]?.message || "Invalid merge payload"); |
50 | 50 | } |
51 | + if (parsed.data.skipApprovalCheck) { | |
52 | + const isRepoAdmin = await canAdminRepo(user.id, repo); | |
53 | + if (!isRepoAdmin) { | |
54 | + return forbidden("Only repository admins can skip approval checks"); | |
55 | + } | |
56 | + } | |
51 | 57 | |
52 | 58 | const result = await bulkMergeStack(stackId, user.id, { |
53 | 59 | mergeMethod: parsed.data.mergeMethod, |
| @@ -56,4 +62,3 @@ export const POST: APIRoute = withErrorHandler(async ({ params, locals, request | ||
56 | 62 | |
57 | 63 | return success(result); |
58 | 64 | }); |
59 | - | |
60 | 65 | |
▼tests/integration/stack-merge-route.test.ts
+26-1
| @@ -2,10 +2,12 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; | ||
2 | 2 | |
3 | 3 | const { |
4 | 4 | canWriteRepoMock, |
5 | + canAdminRepoMock, | |
5 | 6 | bulkMergeStackMock, |
6 | 7 | fakeSchema, |
7 | 8 | } = vi.hoisted(() => ({ |
8 | 9 | canWriteRepoMock: vi.fn(async () => true), |
10 | + canAdminRepoMock: vi.fn(async () => false), | |
9 | 11 | bulkMergeStackMock: vi.fn(async () => ({ |
10 | 12 | success: true, |
11 | 13 | merged: [{ prId: "pr-1", prNumber: 1 }], |
| @@ -28,6 +30,7 @@ vi.mock("@/db", () => ({ | ||
28 | 30 | |
29 | 31 | vi.mock("@/lib/permissions", () => ({ |
30 | 32 | canWriteRepo: canWriteRepoMock, |
33 | + canAdminRepo: canAdminRepoMock, | |
31 | 34 | })); |
32 | 35 | |
33 | 36 | vi.mock("@/lib/bulk-merge", () => ({ |
| @@ -60,6 +63,8 @@ describe("stack merge route", () => { | ||
60 | 63 | beforeEach(() => { |
61 | 64 | mockDb = makeDb(); |
62 | 65 | canWriteRepoMock.mockResolvedValue(true); |
66 | + canAdminRepoMock.mockResolvedValue(false); | |
67 | + bulkMergeStackMock.mockClear(); | |
63 | 68 | bulkMergeStackMock.mockResolvedValue({ |
64 | 69 | success: true, |
65 | 70 | merged: [{ prId: "pr-1", prNumber: 1 }], |
| @@ -85,6 +90,8 @@ describe("stack merge route", () => { | ||
85 | 90 | }); |
86 | 91 | |
87 | 92 | it("calls bulk merge with merge options", async () => { |
93 | + canAdminRepoMock.mockResolvedValue(true); | |
94 | + | |
88 | 95 | const response = await mergeStackPost({ |
89 | 96 | params: { owner: "owner-1", repo: "demo", stackId: "stack-1" }, |
90 | 97 | locals: { user: { id: "user-1" } }, |
| @@ -103,5 +110,23 @@ describe("stack merge route", () => { | ||
103 | 110 | skipApprovalCheck: true, |
104 | 111 | }); |
105 | 112 | }); |
106 | -}); | |
107 | 113 | |
114 | + it("forbids skipApprovalCheck for non-admin writers", async () => { | |
115 | + canAdminRepoMock.mockResolvedValue(false); | |
116 | + | |
117 | + const response = await mergeStackPost({ | |
118 | + params: { owner: "owner-1", repo: "demo", stackId: "stack-1" }, | |
119 | + locals: { user: { id: "user-1" } }, | |
120 | + request: new Request("http://localhost/api/repos/owner-1/demo/stacks/stack-1/merge", { | |
121 | + method: "POST", | |
122 | + headers: { "content-type": "application/json" }, | |
123 | + body: JSON.stringify({ skipApprovalCheck: true }), | |
124 | + }), | |
125 | + } as any); | |
126 | + | |
127 | + const body = await readJson(response); | |
128 | + expect(response.status).toBe(403); | |
129 | + expect(body?.error?.code).toBe("FORBIDDEN"); | |
130 | + expect(bulkMergeStackMock).not.toHaveBeenCalled(); | |
131 | + }); | |
132 | +}); | |
108 | 133 | |