diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index a0d111bd710f..c51e6c992da1 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2447,44 +2447,69 @@ it.effect("refuses line comments on a host that takes only a summary", () => }), ); -it.effect( - "refuses a review with neither a summary nor a comment, but lets an approval through", - () => - Effect.gen(function* () { - let approved = false; - const service = yield* makeService({ - projects: [ - project({ - id: "p1", - title: "t3code", - workspaceRoot: "/a", - repository: "pingdotgg/t3code", - }), - ], - providers: [ - fakeProvider("github", { - submitReview: () => { - approved = true; - return Effect.void; - }, - }), - ], - }); - const reference = { - projectId: "p1" as ProjectId, - repository: "pingdotgg/t3code", - number: 1, - }; +it.effect("allows comments and formal review decisions through the provider", () => + Effect.gen(function* () { + let permissionReads = 0; + let submissions = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "t3code", + workspaceRoot: "/a", + repository: "pingdotgg/t3code", + }), + ], + providers: [ + fakeProvider("github", { + getViewerPermissions: () => { + permissionReads += 1; + return Effect.succeed({ + actions: ["merge", "ready", "draft", "close", "reopen"], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }); + }, + submitReview: () => { + submissions += 1; + return Effect.void; + }, + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository: "pingdotgg/t3code", + number: 1, + }; - const error = yield* Effect.flip( - service.submitReview({ ...reference, verdict: "comment", body: " ", comments: [] }), - ); - assert.strictEqual(error._tag, "PullRequestOperationError"); + const error = yield* Effect.flip( + service.submitReview({ ...reference, verdict: "comment", body: " ", comments: [] }), + ); + assert.strictEqual(error._tag, "PullRequestOperationError"); - // An approval is a verdict in itself, so it needs no words. - yield* service.submitReview({ ...reference, verdict: "approve", body: "", comments: [] }); - assert.isTrue(approved); - }), + for (const verdict of ["approve", "request-changes"] as const) { + yield* service.submitReview({ + ...reference, + verdict, + body: verdict === "approve" ? "" : "Please address these findings.", + comments: [], + }); + } + assert.strictEqual(permissionReads, 2); + assert.strictEqual(submissions, 2); + + yield* service.submitReview({ + ...reference, + verdict: "comment", + body: "Looks good to discuss", + comments: [], + }); + assert.strictEqual(permissionReads, 3); + assert.strictEqual(submissions, 3); + }), ); it.effect("refuses to resolve a conversation on a host that cannot", () => @@ -2890,45 +2915,340 @@ it.effect("asks another checkout who is signed in when the first one cannot answ }), ); -it.effect("refuses to ask for a review on a host that cannot, before any call is made", () => +it.effect( + "refuses human, team, and ambiguous automation reviewer mutations before the adapter", + () => + Effect.gen(function* () { + let asked = false; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + ], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + review: FULL_REVIEW, + reviewers: { request: false, listCandidates: false }, + }, + getViewerPermissions: () => { + asked = true; + return Effect.die("must not be called"); + }, + setReviewerRequest: () => Effect.die("must not be called"), + }), + ], + }); + + const error = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "octocat", kind: "user" }], + requested: true, + }), + ); + + assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.include(error.message, "can request only nullStack65"); + assert.isFalse(asked); + + const removalError = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "reviewers", kind: "team" }], + expectedAccountId: "112618179", + requested: false, + }), + ); + assert.strictEqual(removalError._tag, "PullRequestOperationError"); + assert.include(removalError.message, "can request only nullStack65"); + assert.isFalse(asked); + + const botError = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "dependabot[bot]", kind: "user" }], + expectedAccountId: "112618179", + requested: true, + }), + ); + assert.strictEqual(botError._tag, "PullRequestOperationError"); + assert.include(botError.message, "can request only nullStack65"); + assert.isFalse(asked); + }), +); + +it.effect("allows a verified owner reviewer mutation through the existing provider path", () => Effect.gen(function* () { - let asked = false; + const sent: Array<{ + reviewers: ReadonlyArray<{ id: string; kind: "user" | "team" }>; + requested: boolean; + }> = []; const service = yield* makeService({ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], providers: [ fakeProvider("github", { - capabilities: { - diff: true, - comment: true, - actions: ["merge"], - mergeMethods: ["merge"], - search: true, - reactions: true, - review: FULL_REVIEW, - reviewers: { request: false, listCandidates: false }, + withVerifiedCredential: (_input, use) => + use({ + accountId: "112618179", + viewer: "nullStack65", + credentialFingerprint: "owner-credential", + }), + getViewerPermissions: () => + Effect.succeed({ + actions: ["ready", "draft", "close", "reopen"], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }), + setReviewerRequest: (input) => { + sent.push({ reviewers: input.reviewers, requested: input.requested }); + return Effect.void; }, + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + }; + + yield* service.requestReviewers({ + ...reference, + reviewers: [{ id: "nullStack65", kind: "user" }], + requested: true, + }); + + yield* service.requestReviewers({ + ...reference, + expectedAccountId: "112618179", + reviewers: [ + { id: "octocat", kind: "user" }, + { id: "reviewers", kind: "team" }, + ], + requested: false, + }); + + yield* service.withRoutingCredential( + { ...reference, host: "github.com", expectedAccountId: "112618179" }, + service.requestReviewers({ + ...reference, + host: "github.com", + expectedAccountId: "112618179", + reviewers: [{ id: "nullStack65", kind: "user" }], + requested: true, + }), + ); + + assert.deepStrictEqual(sent, [ + { reviewers: [{ id: "nullStack65", kind: "user" }], requested: true }, + { + reviewers: [ + { id: "octocat", kind: "user" }, + { id: "reviewers", kind: "team" }, + ], + requested: false, + }, + { reviewers: [{ id: "nullStack65", kind: "user" }], requested: true }, + ]); + }), +); + +it.effect("refuses reviewer routing without the verified owner identity", () => + Effect.gen(function* () { + let mutations = 0; + let permissionReads = 0; + const base = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "nullStack65", kind: "user" as const }], + requested: true, + }; + const wrongAccount = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "7", viewer: "someone-else", credentialFingerprint: "other" }), getViewerPermissions: () => { - asked = true; - return Effect.die("must not be called"); + permissionReads++; + return Effect.succeed({ + actions: [], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }); + }, + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const wrongAccountError = yield* Effect.flip(wrongAccount.requestReviewers(base)); + assert.strictEqual(wrongAccountError._tag, "PullRequestOperationError"); + assert.strictEqual(permissionReads, 0); + assert.strictEqual(mutations, 0); + + const noVerifier = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + setReviewerRequest: () => { + mutations++; + return Effect.void; }, - setReviewerRequest: () => Effect.die("must not be called"), }), ], }); + const noVerifierError = yield* Effect.flip(noVerifier.requestReviewers(base)); + assert.strictEqual(noVerifierError._tag, "PullRequestOperationError"); + assert.strictEqual(mutations, 0); + const explicitMismatch = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const mismatchError = yield* Effect.flip( + explicitMismatch.requestReviewers({ ...base, expectedAccountId: "7" }), + ); + assert.strictEqual(mismatchError._tag, "PullRequestOperationError"); + assert.strictEqual(mutations, 0); + }), +); + +it.effect("does not treat the owner's numeric GitHub ID as authority on enterprise hosts", () => + Effect.gen(function* () { + let verified = 0; + let mutations = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "enterprise", + workspaceRoot: "/a", + repository: "acme/web", + host: "github.enterprise.test", + }), + ], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => { + verified++; + return use({ + accountId: "112618179", + viewer: "nullStack65", + credentialFingerprint: "owner", + }); + }, + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); const error = yield* Effect.flip( service.requestReviewers({ projectId: "p1" as ProjectId, repository: "acme/web", number: 1, - reviewers: [{ id: "octocat", kind: "user" }], + reviewers: [{ id: "nullStack65", kind: "user" }], requested: true, }), ); - assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "cannot ask somebody for a review."); - assert.isFalse(asked); + assert.strictEqual(verified, 0); + assert.strictEqual(mutations, 0); + }), +); + +it.effect("preserves write permission and host capability denials for verified owner routing", () => + Effect.gen(function* () { + let mutations = 0; + const deniedPermission = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + getViewerPermissions: () => + Effect.succeed({ + actions: [], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: false, + }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "nullStack65", kind: "user" as const }], + requested: true, + }; + const permissionError = yield* Effect.flip(deniedPermission.requestReviewers(reference)); + assert.include(permissionError.message, "write access"); + assert.strictEqual(mutations, 0); + + const deniedCapability = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + review: FULL_REVIEW, + reviewers: { request: false, listCandidates: false }, + }, + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const capabilityError = yield* Effect.flip(deniedCapability.requestReviewers(reference)); + assert.include(capabilityError.message, "This host cannot ask somebody"); + assert.strictEqual(mutations, 0); }), ); @@ -3002,7 +3322,7 @@ it.effect("refuses a review request this viewer may not make, and says what acce ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "You need write access on this repository to ask for a review."); + assert.include(error.message, "can request only nullStack65"); assert.isFalse(sent); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 6b4b2bd8bfd5..137c7ccffa5e 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -161,6 +161,7 @@ const VIEWER_CACHE_CAPACITY = 32; export type PullRequestError = PullRequestUnavailableError | PullRequestOperationError; const routingCredential = Context.Reference<{ + readonly accountId: string; readonly credentialFingerprint: string; readonly viewer: string; } | null>("t3/PullRequestService/routingCredential", { defaultValue: () => null }); @@ -305,6 +306,10 @@ const ACTION_ACCESS_REFUSALS: Record = { */ const REVIEWER_REQUEST_REFUSAL = "You need write access on this repository to ask for a review."; const LABEL_CHANGE_REFUSAL = "You need triage access on this repository to change its labels."; +const OWNER_GITHUB_ACCOUNT_ID = "112618179"; +const OWNER_GITHUB_LOGIN = "nullstack65"; +const AMBIGUOUS_REVIEWER_REQUEST_REFUSAL = + "T3 can request only nullStack65 as a reviewer. The verified owner can remove pending reviewer assignments."; /** A project this page can read: its remote is on a host with an implementation. */ export interface SupportedProject { @@ -2314,40 +2319,78 @@ export const make = Effect.gen(function* () { ); const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => - requireProject(input).pipe( - Effect.flatMap((project): Effect.Effect => { + Effect.gen(function* () { + const invalid = () => + new PullRequestOperationError({ + operation: "requestReviewers", + detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, + }); + const reviewer = input.reviewers.length === 1 ? input.reviewers[0] : undefined; + if ( + input.reviewers.length === 0 || + (input.requested && + (reviewer?.kind !== "user" || reviewer.id.toLowerCase() !== OWNER_GITHUB_LOGIN)) + ) { + return yield* invalid(); + } + if ( + input.expectedAccountId !== undefined && + input.expectedAccountId !== OWNER_GITHUB_ACCOUNT_ID + ) { + return yield* invalid(); + } + + const project = yield* requireProject(input); + if (project.api.kind !== "github" || project.host.toLowerCase() !== "github.com") { + return yield* invalid(); + } + const api = registry.get("github"); + if (api?.withVerifiedCredential === undefined) return yield* invalid(); + + const perform = Effect.gen(function* () { if (!project.api.capabilities.reviewers.request) { - return Effect.fail( - new PullRequestOperationError({ - operation: "requestReviewers", - detail: "This host cannot ask somebody for a review.", - }), - ); + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: "This host cannot ask somebody for a review.", + }); } - return viewerPermissionsOf(project, input, "requestReviewers").pipe( - Effect.flatMap((viewer): Effect.Effect => { - if (!viewer.requestReviewers) { - return Effect.fail( - new PullRequestOperationError({ - operation: "requestReviewers", - detail: REVIEWER_REQUEST_REFUSAL, - }), - ); - } - return project.api - .setReviewerRequest({ - cwd: project.project.workspaceRoot, - repository: project.repository, - host: project.host, - number: input.number, - reviewers: input.reviewers, - requested: input.requested, - }) - .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); - }), - ); - }), - ); + const viewer = yield* viewerPermissionsOf(project, input, "requestReviewers"); + if (!viewer.requestReviewers) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: REVIEWER_REQUEST_REFUSAL, + }); + } + yield* project.api + .setReviewerRequest({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + reviewers: input.reviewers, + requested: input.requested, + }) + .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); + }); + + const credential = yield* routingCredential; + if (credential !== null) { + if (credential.accountId !== OWNER_GITHUB_ACCOUNT_ID) return yield* invalid(); + return yield* perform; + } + const result = yield* api + .withVerifiedCredential( + { cwd: project.project.workspaceRoot, host: project.host }, + (identity) => + identity.accountId === OWNER_GITHUB_ACCOUNT_ID && + (input.expectedAccountId === undefined || + identity.accountId === input.expectedAccountId) + ? perform.pipe(Effect.provideService(routingCredential, identity), Effect.result) + : Effect.fail(invalid()), + ) + .pipe(Effect.catchTag("PullRequestProviderError", () => Effect.fail(invalid()))); + return yield* Effect.fromResult(result); + }); /** * The labels, like the reviewer candidates, are wanted only by somebody about to change them,