Filter file approval visibility by path read scope
swadhinbiswas committed 8 months ago
493cf5c
1 parent 8f48521
commit 493cf5c
3 files changed
+63 -4
Showing 3 changed files+63-4
▼doc/feature_audit.md
+1-1
| @@ -36,7 +36,7 @@ | ||
36 | 36 | | Branch protection rules | ✅ | Schema + UI + API in place | |
37 | 37 | | Repository templates | ⚠️ | Template creation and listing are implemented with access-aware discovery filters (`/repos/templates?q=&owner=&visibility=`) including collaborator-visible private templates; dedicated template governance UX remains partial | |
38 | 38 | | Repository Wiki | ✅ | Implemented (Phase 8) | |
39 | -| File-level permissions | ⚠️ | Path-scoped enforcement now covers PR comment create/edit/delete, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke flows, PR rewrite operations, and PR impact scans; broader endpoint coverage still remains | | |
39 | +| File-level permissions | ⚠️ | Path-scoped enforcement now covers PR comment create/edit/delete, single-review inline comments, batch-review comment submission, suggestion-apply, partial file-approval create/revoke and read-filter flows, PR rewrite operations, and PR impact scans; broader endpoint coverage still remains | | |
40 | 40 | | Commit signing (GPG) | ✅ | GPG key management UI exists | |
41 | 41 | | Commit signing (SSH) | ✅ | SSH key management UI exists | |
42 | 42 | |
43 | 43 | |
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/file-approvals.ts
+10-2
| @@ -51,6 +51,10 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => { | ||
51 | 51 | |
52 | 52 | const repoPath = await resolveRepoPath(getRepoPath(owner, repo)); |
53 | 53 | const changedFiles = await getChangedFiles(repoPath, pr.baseBranch, pr.headBranch); |
54 | + const scopedUserId = locals.user?.id || "__anonymous__"; | |
55 | + const readScope = await checkPathPermissions(scopedUserId, repository.id, changedFiles, "read"); | |
56 | + const denied = new Set(readScope.deniedPaths || []); | |
57 | + const visibleFiles = changedFiles.filter((path) => !denied.has(path)); | |
54 | 58 | |
55 | 59 | const approvals = await db.query.fileApprovals.findMany({ |
56 | 60 | where: eq(schema.fileApprovals.pullRequestId, pr.id), |
| @@ -72,7 +76,7 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => { | ||
72 | 76 | } |
73 | 77 | } |
74 | 78 | |
75 | - const files = changedFiles.map((path) => { | |
79 | + const files = visibleFiles.map((path) => { | |
76 | 80 | const entry = approvalMap.get(path); |
77 | 81 | const stale = entry?.stale || false; |
78 | 82 | const approvers = entry?.approvers || []; |
| @@ -84,7 +88,11 @@ export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => { | ||
84 | 88 | }; |
85 | 89 | }); |
86 | 90 | |
87 | - return success({ files, allApproved: files.length > 0 && files.every((file) => file.approved) }); | |
91 | + return success({ | |
92 | + files, | |
93 | + hiddenPaths: denied.size, | |
94 | + allApproved: files.length > 0 && files.every((file) => file.approved), | |
95 | + }); | |
88 | 96 | }); |
89 | 97 | |
90 | 98 | export const POST: APIRoute = withErrorHandler(async ({ params, locals, request }) => { |
91 | 99 | |
▼tests/integration/file-approvals-path-permissions.test.ts
+52-1
| @@ -5,12 +5,14 @@ const { | ||
5 | 5 | canAdminRepoMock, |
6 | 6 | checkPathPermissionsMock, |
7 | 7 | approveFileMock, |
8 | + getChangedFilesMock, | |
8 | 9 | fakeSchema, |
9 | 10 | } = vi.hoisted(() => ({ |
10 | 11 | canReadRepoMock: vi.fn(async () => true), |
11 | 12 | canAdminRepoMock: vi.fn(async () => false), |
12 | 13 | checkPathPermissionsMock: vi.fn(async () => ({ allowed: true, deniedPaths: [] })), |
13 | 14 | approveFileMock: vi.fn(async () => ({ id: "fa-1", path: "src/app.ts" })), |
15 | + getChangedFilesMock: vi.fn(async () => ["src/app.ts", "src/secret.ts"]), | |
14 | 16 | fakeSchema: { |
15 | 17 | users: { username: {} }, |
16 | 18 | repositories: { ownerId: {}, name: {}, id: {} }, |
| @@ -39,7 +41,19 @@ vi.mock("@/lib/partial-file-approvals", () => ({ | ||
39 | 41 | approveFile: approveFileMock, |
40 | 42 | })); |
41 | 43 | |
42 | -import { POST as fileApprovalsPost } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/file-approvals"; | |
44 | +vi.mock("@/lib/git-storage", () => ({ | |
45 | + resolveRepoPath: vi.fn(async () => "/tmp/repo"), | |
46 | +})); | |
47 | + | |
48 | +vi.mock("@/lib/git", () => ({ | |
49 | + getRepoPath: vi.fn(() => "/tmp/repo"), | |
50 | + getChangedFiles: getChangedFilesMock, | |
51 | +})); | |
52 | + | |
53 | +import { | |
54 | + GET as fileApprovalsGet, | |
55 | + POST as fileApprovalsPost, | |
56 | +} from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/file-approvals"; | |
43 | 57 | import { DELETE as fileApprovalsDelete } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/file-approvals/[id]"; |
44 | 58 | |
45 | 59 | function makeDb() { |
| @@ -56,6 +70,22 @@ function makeDb() { | ||
56 | 70 | }, |
57 | 71 | fileApprovals: { |
58 | 72 | findFirst: vi.fn(async () => ({ id: "fa-1", pullRequestId: "pr-1", approvedById: "user-1", path: "src/app.ts" })), |
73 | + findMany: vi.fn(async () => ([ | |
74 | + { | |
75 | + id: "fa-1", | |
76 | + pullRequestId: "pr-1", | |
77 | + path: "src/app.ts", | |
78 | + commitSha: "sha-1", | |
79 | + approvedBy: { username: "alice" }, | |
80 | + }, | |
81 | + { | |
82 | + id: "fa-2", | |
83 | + pullRequestId: "pr-1", | |
84 | + path: "src/secret.ts", | |
85 | + commitSha: "sha-1", | |
86 | + approvedBy: { username: "bob" }, | |
87 | + }, | |
88 | + ])), | |
59 | 89 | }, |
60 | 90 | }, |
61 | 91 | delete: vi.fn(() => ({ |
| @@ -71,6 +101,27 @@ describe("file approvals path permissions", () => { | ||
71 | 101 | canReadRepoMock.mockResolvedValue(true); |
72 | 102 | canAdminRepoMock.mockResolvedValue(false); |
73 | 103 | checkPathPermissionsMock.mockResolvedValue({ allowed: true, deniedPaths: [] }); |
104 | + getChangedFilesMock.mockResolvedValue(["src/app.ts", "src/secret.ts"]); | |
105 | + }); | |
106 | + | |
107 | + it("filters hidden files from GET results when read path scope denies access", async () => { | |
108 | + checkPathPermissionsMock.mockResolvedValue({ | |
109 | + allowed: false, | |
110 | + deniedPaths: ["src/secret.ts"], | |
111 | + reason: "denied", | |
112 | + }); | |
113 | + | |
114 | + const response = await fileApprovalsGet({ | |
115 | + params: { owner: "owner-1", repo: "demo", number: "42" }, | |
116 | + locals: { user: { id: "user-1" } }, | |
117 | + } as any); | |
118 | + | |
119 | + const body = await response.json(); | |
120 | + expect(response.status).toBe(200); | |
121 | + expect(body?.data?.files).toHaveLength(1); | |
122 | + expect(body?.data?.files?.[0]?.path).toBe("src/app.ts"); | |
123 | + expect(body?.data?.hiddenPaths).toBe(1); | |
124 | + expect(checkPathPermissionsMock).toHaveBeenCalledWith("user-1", "repo-1", ["src/app.ts", "src/secret.ts"], "read"); | |
74 | 125 | }); |
75 | 126 | |
76 | 127 | it("blocks approving file when path permissions deny access", async () => { |
77 | 128 | |