From f20c32b319a7d3cef93f4d82a2b1c76a23997bfa Mon Sep 17 00:00:00 2001 From: Jeffrey Sica Date: Thu, 10 Sep 2026 20:27:23 -0500 Subject: [PATCH 1/3] feat(owners): resolve OWNERS files across directories from a git tree Adds src/utils/owners.ts: parseOwners (approvers/reviewers lists, options.no_parent_owners, emeritus and unknown keys tolerated, filters noted and ignored, logins lowercased), ownersDir, effectiveOwners (walk from the file's directory to the root taking the union, stopping at no_parent_owners) and loadOwnersTree (recursive git tree at a ref, fetching only the OWNERS blobs in ancestor directories of the paths of interest; a truncated tree falls back to probing each candidate path with the contents API). Not wired into authorization yet, so dist/ is unchanged. Signed-off-by: Jeffrey Sica --- __tests__/utils/owners.test.ts | 170 ++++++++++++++++++++++ src/utils/owners.ts | 250 +++++++++++++++++++++++++++++++++ 2 files changed, 420 insertions(+) create mode 100644 __tests__/utils/owners.test.ts create mode 100644 src/utils/owners.ts diff --git a/__tests__/utils/owners.test.ts b/__tests__/utils/owners.test.ts new file mode 100644 index 0000000..df20d7d --- /dev/null +++ b/__tests__/utils/owners.test.ts @@ -0,0 +1,170 @@ +import * as core from '@actions/core' +import { describe, expect, it, vi } from 'vitest' + +import { + effectiveOwners, + ownersDir, + parseOwners, +} from '../../src/utils/owners' + +describe('parseOwners', () => { + it('reads approvers and reviewers', () => { + const owners = parseOwners('OWNERS', 'approvers:\n- alice\nreviewers:\n- bob\n- carol\n') + + expect(owners).toEqual({ + path: 'OWNERS', + approvers: ['alice'], + reviewers: ['bob', 'carol'], + noParentOwners: false, + }) + }) + + it('defaults a missing role to an empty list', () => { + const owners = parseOwners('OWNERS', 'approvers:\n- alice\n') + + expect(owners.reviewers).toEqual([]) + }) + + it('treats an empty role and an empty file as no members', () => { + expect(parseOwners('OWNERS', 'approvers:\n').approvers).toEqual([]) + expect(parseOwners('OWNERS', '')).toMatchObject({ approvers: [], reviewers: [] }) + }) + + it('lowercases logins', () => { + const owners = parseOwners('OWNERS', 'approvers:\n- Alice\nreviewers:\n- BOB\n') + + expect(owners.approvers).toEqual(['alice']) + expect(owners.reviewers).toEqual(['bob']) + }) + + it('ignores emeritus roles and unknown keys', () => { + const owners = parseOwners( + 'OWNERS', + [ + 'approvers:', + '- alice', + 'emeritus_approvers:', + '- zed', + 'emeritus_reviewers:', + '- yan', + 'labels:', + '- sig/foo', + 'something_else: 42', + ].join('\n'), + ) + + expect(owners.approvers).toEqual(['alice']) + expect(owners.reviewers).toEqual([]) + }) + + it('notes that filters are ignored', () => { + const debug = vi.spyOn(core, 'debug') + + const owners = parseOwners( + 'sdk/OWNERS', + 'approvers:\n- alice\nfilters:\n ".*":\n approvers:\n - bob\n', + ) + + expect(owners.approvers).toEqual(['alice']) + expect(debug).toHaveBeenCalledWith( + 'OWNERS at sdk/OWNERS: filters are not supported; ignoring', + ) + }) + + it('reads options.no_parent_owners', () => { + expect( + parseOwners('olm/OWNERS', 'options:\n no_parent_owners: true\napprovers:\n- carol\n').noParentOwners, + ).toBe(true) + expect( + parseOwners('olm/OWNERS', 'options:\n no_parent_owners: false\n').noParentOwners, + ).toBe(false) + expect( + parseOwners('olm/OWNERS', 'options: {}\n').noParentOwners, + ).toBe(false) + }) + + it.each([ + ['approvers: alice\n', 'approvers'], + ['reviewers:\n alice: true\n', 'reviewers'], + ['approvers:\n- alice\n- 7\n', 'approvers'], + ])('rejects a role that is not a list of strings: %j', (contents, role) => { + expect(() => parseOwners('sdk/OWNERS', contents)).toThrow( + `OWNERS at sdk/OWNERS: ${role} must be a list of GitHub usernames`, + ) + }) + + it('treats a non-mapping document as empty', () => { + expect(parseOwners('OWNERS', '- alice\n')).toMatchObject({ approvers: [], reviewers: [] }) + expect(parseOwners('OWNERS', 'just text\n')).toMatchObject({ approvers: [], reviewers: [] }) + }) +}) + +describe('ownersDir', () => { + it.each([ + ['OWNERS', ''], + ['sdk/OWNERS', 'sdk'], + ['a/b/c/OWNERS', 'a/b/c'], + ['README.md', ''], + ['sdk/x.go', 'sdk'], + ['sdk', ''], + ])('%s -> %j', (path, dir) => { + expect(ownersDir(path)).toBe(dir) + }) +}) + +describe('effectiveOwners', () => { + const root = parseOwners('OWNERS', 'approvers:\n- alice\nreviewers:\n- rita\n') + const sdk = parseOwners('sdk/OWNERS', 'approvers:\n- bob\nreviewers:\n- ryan\n') + const olm = parseOwners('olm/OWNERS', 'options:\n no_parent_owners: true\napprovers:\n- carol\n') + const deep = parseOwners('sdk/internal/OWNERS', 'approvers:\n- dave\n') + + it('unions the file with its parents, nearest first', () => { + const owners = new Map([['', root], ['sdk', sdk], ['sdk/internal', deep]]) + + const set = effectiveOwners('sdk/internal/x.go', owners) + + expect(set).toBeDefined() + expect([...set!.approvers]).toEqual(['dave', 'bob', 'alice']) + expect([...set!.reviewers]).toEqual(['ryan', 'rita']) + expect(set!.sources).toEqual(['sdk/internal/OWNERS', 'sdk/OWNERS', 'OWNERS']) + }) + + it('does not apply a sibling directory', () => { + const owners = new Map([['', root], ['sdk', sdk]]) + + const set = effectiveOwners('docs/x.md', owners) + + expect([...set!.approvers]).toEqual(['alice']) + expect(set!.sources).toEqual(['OWNERS']) + }) + + it('stops at a file with no_parent_owners', () => { + const owners = new Map([['', root], ['olm', olm]]) + + const set = effectiveOwners('olm/y.go', owners) + + expect([...set!.approvers]).toEqual(['carol']) + expect([...set!.reviewers]).toEqual([]) + expect(set!.sources).toEqual(['olm/OWNERS']) + }) + + it('is undefined when no OWNERS covers the file', () => { + const owners = new Map([['sdk', sdk]]) + + expect(effectiveOwners('README.md', owners)).toBeUndefined() + expect(effectiveOwners('docs/x.md', owners)).toBeUndefined() + }) + + it('lets a root-only OWNERS cover a deep path', () => { + const owners = new Map([['', root]]) + + const set = effectiveOwners('a/b/c/d.txt', owners) + + expect([...set!.approvers]).toEqual(['alice']) + expect(set!.sources).toEqual(['OWNERS']) + }) + + it('is undefined for an empty map', () => { + expect(effectiveOwners('README.md', new Map())).toBeUndefined() + }) +}) diff --git a/src/utils/owners.ts b/src/utils/owners.ts new file mode 100644 index 0000000..c051dcc --- /dev/null +++ b/src/utils/owners.ts @@ -0,0 +1,250 @@ +import type { Octokit } from '@octokit/rest' +import type { Context } from './context' +import { Buffer } from 'node:buffer' + +import * as core from '@actions/core' +import * as yaml from 'js-yaml' + +export type OwnersRole = 'approvers' | 'reviewers' + +export interface OwnersFile { + path: string + approvers: string[] + reviewers: string[] + noParentOwners: boolean +} + +export interface OwnersSet { + approvers: Set + reviewers: Set + sources: string[] +} + +export interface OwnersTree { + owners: Map + hasOwners: boolean +} + +/** + * Parse the contents of an OWNERS file. Logins are lowercased because GitHub + * logins are case-insensitive. + * + * @param path - the path of the OWNERS file, used in error messages + * @param contents - the yaml contents + */ +export function parseOwners(path: string, contents: string): OwnersFile { + const loaded: unknown = contents.trim() === '' ? {} : yaml.load(contents) + const doc: Record + = typeof loaded === 'object' && loaded !== null && !Array.isArray(loaded) + ? (loaded as Record) + : {} + + if ('filters' in doc) { + core.debug(`OWNERS at ${path}: filters are not supported; ignoring`) + } + + const options = doc.options + const noParentOwners + = typeof options === 'object' + && options !== null + && (options as Record).no_parent_owners === true + + return { + path, + approvers: roleList(path, doc, 'approvers'), + reviewers: roleList(path, doc, 'reviewers'), + noParentOwners, + } +} + +function roleList( + path: string, + doc: Record, + role: OwnersRole, +): string[] { + const value = doc[role] + if (value === undefined || value === null) { + return [] + } + + if (!Array.isArray(value) || !value.every(v => typeof v === 'string')) { + throw new Error( + `OWNERS at ${path}: ${role} must be a list of GitHub usernames`, + ) + } + + return value.map(v => v.toLowerCase()) +} + +/** + * The directory that contains a path: 'sdk/OWNERS' is 'sdk', 'OWNERS' is '' + * + * @param path - a repository relative path + */ +export function ownersDir(path: string): string { + const slash = path.lastIndexOf('/') + return slash === -1 ? '' : path.slice(0, slash) +} + +/** + * Resolve the OWNERS that apply to a file: walk from its directory up to the + * root, taking the union of every OWNERS file on the way. A file with + * options.no_parent_owners stops the walk. + * + * @param file - the changed file + * @param owners - OWNERS files keyed by directory + * @returns undefined when no OWNERS file covers the file + */ +export function effectiveOwners( + file: string, + owners: Map, +): OwnersSet | undefined { + const approvers = new Set() + const reviewers = new Set() + const sources: string[] = [] + + let dir = ownersDir(file) + for (;;) { + const found = owners.get(dir) + if (found !== undefined) { + found.approvers.forEach(a => approvers.add(a)) + found.reviewers.forEach(r => reviewers.add(r)) + sources.push(found.path) + if (found.noParentOwners) { + break + } + } + + if (dir === '') { + break + } + dir = ownersDir(dir) + } + + if (sources.length === 0) { + return undefined + } + + return { approvers, reviewers, sources } +} + +function ancestorDirs(paths: string[]): Set { + const dirs = new Set(['']) + for (const path of paths) { + for (let dir = ownersDir(path); dir !== ''; dir = ownersDir(dir)) { + dirs.add(dir) + } + } + return dirs +} + +function isOwnersPath(path: string): boolean { + return path === 'OWNERS' || path.endsWith('/OWNERS') +} + +function decode(data: unknown, path: string): string { + const file = data as { content?: string, encoding?: string } + if (!file.content || !file.encoding) { + throw new Error(`invalid OWNERS file returned from GitHub API for ${path}`) + } + return Buffer.from(file.content, file.encoding as BufferEncoding).toString() +} + +/** + * Load the OWNERS files at ref that can apply to the given paths. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param ref - the commit to read OWNERS files from + * @param pathsOfInterest - the changed files; only OWNERS in their ancestor directories are fetched + */ +export async function loadOwnersTree( + octokit: Octokit, + context: Context, + ref: string, + pathsOfInterest: string[], +): Promise { + const dirs = ancestorDirs(pathsOfInterest) + + let tree + try { + const response = await octokit.git.getTree({ + ...context.repo, + tree_sha: ref, + recursive: 'true', + }) + tree = response.data + } + catch (e) { + throw new Error(`error loading OWNERS files at ${ref}: ${e}`) + } + + if (tree.truncated) { + // a truncated listing may have dropped OWNERS entries, so ask for each candidate path directly + core.debug(`tree at ${ref} is truncated; probing for OWNERS files`) + return probeOwners(octokit, context, ref, dirs) + } + + const entries = tree.tree.filter( + entry => + entry.type === 'blob' + && entry.path !== undefined + && entry.sha !== undefined + && isOwnersPath(entry.path), + ) as { path: string, sha: string }[] + + const wanted = entries.filter(entry => dirs.has(ownersDir(entry.path))) + + let files: OwnersFile[] + try { + files = await Promise.all( + wanted.map(async (entry) => { + const blob = await octokit.git.getBlob({ + ...context.repo, + file_sha: entry.sha, + }) + return parseOwners(entry.path, decode(blob.data, entry.path)) + }), + ) + } + catch (e) { + throw new Error(`error loading OWNERS files at ${ref}: ${e}`) + } + + return { + owners: new Map(files.map(file => [ownersDir(file.path), file])), + hasOwners: entries.length > 0, + } +} + +async function probeOwners( + octokit: Octokit, + context: Context, + ref: string, + dirs: Set, +): Promise { + const owners = new Map() + + for (const dir of dirs) { + const path = dir === '' ? 'OWNERS' : `${dir}/OWNERS` + let data + try { + const response = await octokit.repos.getContent({ + ...context.repo, + path, + ref, + }) + data = response.data + } + catch (e) { + if (typeof e === 'object' && e && 'status' in e && e.status === 404) { + continue + } + throw new Error(`error loading OWNERS files at ${ref}: ${e}`) + } + + owners.set(dir, parseOwners(path, decode(data, path))) + } + + return { owners, hasOwners: owners.size > 0 } +} From bf147d8478bb06738ebb742c6a73e4c1546fffe2 Mon Sep 17 00:00:00 2001 From: Jeffrey Sica Date: Thu, 10 Sep 2026 20:35:08 -0500 Subject: [PATCH 2/3] feat: authorize /lgtm and /approve against directory-scoped OWNERS on the PR base On a pull request, assertAuthorizedByOwnersOrMembership now resolves the OWNERS files covering each changed file (Prow inheritance, including options.no_parent_owners and both sides of a rename) from the PR's base commit, so a PR cannot grant itself approvers: - /approve: the commenter must be an approver for every changed file; the denial names the first uncovered file and the OWNERS consulted. - /lgtm: the commenter must be a reviewer or approver for at least one changed file. - A changed file with no covering OWNERS is an error naming the file. - The org-member/collaborator fallback applies only when the base tree has no OWNERS file at all. Issues keep using the root OWNERS of the default branch via the contents API with unchanged messages; the tree/blob machinery is PR-only. Logins are compared case-insensitively and a role that is not a list is rejected on both paths. Closes #65 Signed-off-by: Jeffrey Sica --- __tests__/bundle/bundle.test.ts | 84 ++++++ __tests__/issueCommentTest/approve.test.ts | 57 ++++ __tests__/label/lgtm.test.ts | 53 ++++ __tests__/utils/ownersAuth.test.ts | 309 +++++++++++++++++++++ __tests__/utils/ownersFixtures.ts | 106 +++++++ dist/index.js | 272 ++++++++++++++++-- src/utils/auth.ts | 138 ++++++--- 7 files changed, 954 insertions(+), 65 deletions(-) create mode 100644 __tests__/utils/ownersAuth.test.ts create mode 100644 __tests__/utils/ownersFixtures.ts diff --git a/__tests__/bundle/bundle.test.ts b/__tests__/bundle/bundle.test.ts index 2300e42..65668e5 100644 --- a/__tests__/bundle/bundle.test.ts +++ b/__tests__/bundle/bundle.test.ts @@ -1,4 +1,5 @@ import type { FakeGithub } from './fakeGithub' +import { Buffer } from 'node:buffer' import { spawnSync } from 'node:child_process' import fs from 'node:fs' import process from 'node:process' @@ -8,6 +9,7 @@ import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' import pullReqListPulls from '../fixtures/pullReq/pullReqListPulls.json' import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import { blobSha, prCommentEvent } from '../utils/ownersFixtures' import { start } from './fakeGithub' import { bundlePath, runBundle } from './runBundle' @@ -253,6 +255,88 @@ describe('dist/index.js', () => { expect(comments[0].body).toEqual({ body: 'you cannot LGTM your own PR.' }) }) + describe('issue_comment /approve on a pull request', () => { + const ownersFiles: Record = { + 'OWNERS': 'approvers:\n- alice\n', + 'sdk/OWNERS': 'approvers:\n- bob\n', + 'olm/OWNERS': 'options:\n no_parent_owners: true\napprovers:\n- carol\n', + } + + function routeOwners(files: string[]) { + gh.route('GET', `${repo}/pulls/1`, { status: 200, body: { base: { sha: 'basesha' } } }) + gh.route('GET', `${repo}/pulls/1/files`, { + status: 200, + body: files.map(filename => ({ filename, status: 'modified' })), + }) + gh.route('GET', `${repo}/git/trees/basesha`, { + status: 200, + body: { + sha: 'basesha', + truncated: false, + tree: Object.keys(ownersFiles).map(path => ({ path, type: 'blob', sha: blobSha(path) })), + }, + }) + for (const [path, contents] of Object.entries(ownersFiles)) { + gh.route('GET', `${repo}/git/blobs/${blobSha(path)}`, { + status: 200, + body: { encoding: 'base64', content: Buffer.from(contents).toString('base64') }, + }) + } + gh.route('POST', `${repo}/pulls/1/reviews`, { status: 200, body: {} }) + gh.route('POST', `${repo}/issues/1/comments`, { status: 201, body: {} }) + } + + it('approves when a nested approver covers every changed file', async () => { + routeOwners(['sdk/x.go', 'sdk/internal/y.go']) + + const result = await runBundle({ + eventName: 'issue_comment', + payload: prCommentEvent('/approve', 'bob'), + inputs: { ...token, 'prow-commands': '/approve' }, + apiUrl: gh.url, + }) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + const reviews = gh.requestsMatching('POST', /\/pulls\/1\/reviews$/) + expect(reviews).toHaveLength(1) + expect(reviews[0].body).toEqual({ event: 'APPROVE', comments: [] }) + expect(gh.requestsMatching('POST', /\/issues\/1\/comments$/)).toEqual([]) + const calls = gh.requests.map(r => `${r.method} ${r.path}`) + expect(calls.slice(0, 3)).toEqual([ + `GET ${repo}/pulls/1`, + `GET ${repo}/pulls/1/files?per_page=100`, + `GET ${repo}/git/trees/basesha?recursive=true`, + ]) + // the blobs are fetched concurrently, so their order is not fixed + expect(calls.slice(3, 5).sort()).toEqual([ + `GET ${repo}/git/blobs/${blobSha('OWNERS')}`, + `GET ${repo}/git/blobs/${blobSha('sdk/OWNERS')}`, + ]) + expect(calls.slice(5)).toEqual([`POST ${repo}/pulls/1/reviews`]) + }) + + it('refuses with a comment naming the file outside the approver\'s directory', async () => { + routeOwners(['sdk/x.go', 'olm/y.go']) + + const result = await runBundle({ + eventName: 'issue_comment', + payload: prCommentEvent('/approve', 'bob'), + inputs: { ...token, 'prow-commands': '/approve' }, + apiUrl: gh.url, + }) + + const wantErr = 'bob is not an approver for olm/y.go (OWNERS: olm/OWNERS)' + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes(wantErr))).toBe(true) + expect(gh.requestsMatching('POST', /\/pulls\/1\/reviews$/)).toEqual([]) + const comments = gh.requestsMatching('POST', /\/issues\/1\/comments$/) + expect(comments).toHaveLength(1) + expect(comments[0].body).toEqual({ body: `Cannot approve the pull request: Error: ${wantErr}` }) + expect(gh.requestsMatching('GET', /\/contents\//)).toEqual([]) + }) + }) + it('issue_comment /remove fails the action when the api returns 500', async () => { gh.route('GET', `${repo}/collaborators/Codertocat`, { status: 204 }) gh.route('GET', `${repo}/issues/1`, { status: 200, body: { labels: [{ name: 'foo' }] } }) diff --git a/__tests__/issueCommentTest/approve.test.ts b/__tests__/issueCommentTest/approve.test.ts index 796b97c..c2b3466 100644 --- a/__tests__/issueCommentTest/approve.test.ts +++ b/__tests__/issueCommentTest/approve.test.ts @@ -10,6 +10,7 @@ import issueCommentEventAssign from '../fixtures/issues/assign/issueCommentEvent import pullReqListReviews from '../fixtures/pullReq/pullReqListReviews.json' import * as utils from '../testUtils' +import { prCommentEvent, prHandlers } from '../utils/ownersFixtures' const server = setupServer() beforeAll(() => @@ -177,6 +178,62 @@ reviewers: }) }) + it('approves a PR when the commenter is an approver for every changed file', async () => { + const commentContext = new utils.MockContext(prCommentEvent('/approve', 'bob')) + + const observeReq = new utils.ObserveRequest() + server.use( + http.post( + `${utils.api}/repos/Codertocat/Hello-World/pulls/1/reviews`, + utils.mockResponse(200, null, observeReq), + ), + ...prHandlers( + { 'OWNERS': 'approvers:\n- alice\n', 'sdk/OWNERS': 'approvers:\n- bob\n' }, + ['sdk/x.go', 'sdk/internal/y.go'], + ), + ) + + const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + await handleIssueComment(commentContext) + await observeReq.called() + expect(await observeReq.body()).toMatchObject({ event: 'APPROVE' }) + expect(setFailed).not.toHaveBeenCalled() + }) + + it('fails a PR /approve naming the file the commenter does not own', async () => { + const commentContext = new utils.MockContext(prCommentEvent('/approve', 'bob')) + + const wantErr = 'bob is not an approver for olm/y.go (OWNERS: olm/OWNERS)' + + const observeReview = new utils.ObserveRequest() + const observeComment = new utils.ObserveRequest() + server.use( + http.post( + `${utils.api}/repos/Codertocat/Hello-World/pulls/1/reviews`, + utils.mockResponse(200, null, observeReview), + ), + http.post( + `${utils.api}/repos/Codertocat/Hello-World/issues/1/comments`, + utils.mockResponse(200, null, observeComment), + ), + ...prHandlers( + { + 'OWNERS': 'approvers:\n- alice\n', + 'sdk/OWNERS': 'approvers:\n- bob\n', + 'olm/OWNERS': 'options:\n no_parent_owners: true\napprovers:\n- carol\n', + }, + ['sdk/x.go', 'olm/y.go'], + ), + ) + + const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + await handleIssueComment(commentContext) + await observeComment.called() + expect(await observeComment.body().then(body => body.body)).toContain(wantErr) + await expect(observeReview.notCalled()).resolves.toBe('not called') + expect(setFailed).toHaveBeenCalledWith(expect.stringContaining(wantErr)) + }) + it('removes approval with the /approve cancel command if approver in OWNERS file', async () => { const owners = Buffer.from( ` diff --git a/__tests__/label/lgtm.test.ts b/__tests__/label/lgtm.test.ts index 5616b54..9b17060 100644 --- a/__tests__/label/lgtm.test.ts +++ b/__tests__/label/lgtm.test.ts @@ -12,6 +12,7 @@ import issuePayload from '../fixtures/issues/issue.json' import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import * as utils from '../testUtils' +import { prCommentEvent, prHandlers } from '../utils/ownersFixtures' const server = setupServer() beforeAll(() => @@ -659,6 +660,58 @@ reviewers: }) }) + it('adds label on a PR when the commenter reviews any changed file', async () => { + const commentContext = new utils.MockContext(prCommentEvent('/lgtm', 'ryan')) + + const observeReq = new utils.ObserveRequest() + server.use( + http.post( + `${utils.api}/repos/Codertocat/Hello-World/issues/1/labels`, + utils.mockResponse(200, null, observeReq), + ), + ...prHandlers( + { 'OWNERS': 'approvers:\n- alice\n', 'sdk/OWNERS': 'reviewers:\n- ryan\n' }, + ['sdk/x.go', 'docs/y.md'], + ), + ) + + const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + await handleIssueComment(commentContext) + await observeReq.called() + expect(await observeReq.body()).toMatchObject({ labels: ['lgtm'] }) + expect(setFailed).not.toHaveBeenCalled() + }) + + it('fails on a PR when the commenter reviews none of the changed files', async () => { + const commentContext = new utils.MockContext(prCommentEvent('/lgtm', 'ryan')) + + const wantErr = 'ryan is not a reviewer or approver for any changed file' + + const observeAdd = new utils.ObserveRequest() + const observeComment = new utils.ObserveRequest() + server.use( + http.post( + `${utils.api}/repos/Codertocat/Hello-World/issues/1/labels`, + utils.mockResponse(200, null, observeAdd), + ), + http.post( + `${utils.api}/repos/Codertocat/Hello-World/issues/1/comments`, + utils.mockResponse(200, null, observeComment), + ), + ...prHandlers( + { 'OWNERS': 'approvers:\n- alice\n', 'sdk/OWNERS': 'reviewers:\n- ryan\n' }, + ['docs/y.md'], + ), + ) + + const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) + await handleIssueComment(commentContext) + await observeComment.called() + expect(await observeComment.body().then(body => body.body)).toContain(wantErr) + await expect(observeAdd.notCalled()).resolves.toBe('not called') + expect(setFailed).toHaveBeenCalledWith(expect.stringContaining(wantErr)) + }) + it('still rejects /lgtm cancel from a non-author who is not a reviewer', async () => { issueCommentEvent.comment.body = '/lgtm cancel' const commentContext = new utils.MockContext(issueCommentEvent) diff --git a/__tests__/utils/ownersAuth.test.ts b/__tests__/utils/ownersAuth.test.ts new file mode 100644 index 0000000..58fda36 --- /dev/null +++ b/__tests__/utils/ownersAuth.test.ts @@ -0,0 +1,309 @@ +import { Octokit } from '@octokit/rest' +import { http } from 'msw' +import { setupServer } from 'msw/node' +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest' + +import { assertAuthorizedByOwnersOrMembership } from '../../src/utils/auth' +import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' +import * as utils from '../testUtils' +import { + baseSha, + blobSha, + changedFiles, + contentsResponse, + filesHandler, + prCommentEvent, + prHandlers, + pullHandler, + repo, + treeHandlers, +} from './ownersFixtures' + +const server = setupServer() +beforeAll(() => { + utils.setupActionsEnv() + server.listen({ + onUnhandledRequest: 'error', + }) +}) +afterEach(() => server.resetHandlers()) +afterAll(() => server.close()) + +const octokit = new Octokit({ auth: 'some-token' }) +const prContext = new utils.MockContext(prCommentEvent('/approve')) + +const rootOwners = 'approvers:\n- alice\nreviewers:\n- rita\n' +const sdkOwners = 'approvers:\n- bob\nreviewers:\n- ryan\n' +const olmOwners = 'options:\n no_parent_owners: true\napprovers:\n- carol\n' + +function authorize(role: 'approvers' | 'reviewers', username: string, context = prContext) { + return assertAuthorizedByOwnersOrMembership(octokit, context, role, username) +} + +describe('assertAuthorizedByOwnersOrMembership on a pull request', () => { + it('lets a root approver approve (parity with a single OWNERS file)', async () => { + server.use(...prHandlers({ OWNERS: rootOwners }, ['src/file1.txt'])) + + await expect(authorize('approvers', 'alice')).resolves.toBeUndefined() + }) + + it('denies a user who is in neither role of the root OWNERS', async () => { + server.use(...prHandlers({ OWNERS: rootOwners }, ['src/file1.txt'])) + + await expect(authorize('approvers', 'rita')).rejects.toThrow( + 'rita is not an approver for src/file1.txt (OWNERS: OWNERS)', + ) + await expect(authorize('reviewers', 'nobody')).rejects.toThrow( + 'nobody is not a reviewer or approver for any changed file', + ) + }) + + it('inherits approvers from parent directories', async () => { + const owners = { 'OWNERS': rootOwners, 'sdk/OWNERS': sdkOwners } + + server.use(...prHandlers(owners, ['sdk/x.go'])) + await expect(authorize('approvers', 'alice')).resolves.toBeUndefined() + + server.use(...prHandlers(owners, ['sdk/x.go'])) + await expect(authorize('approvers', 'bob')).resolves.toBeUndefined() + }) + + it('stops inheriting at options.no_parent_owners', async () => { + const owners = { 'OWNERS': rootOwners, 'olm/OWNERS': olmOwners } + + server.use(...prHandlers(owners, ['olm/y.go'])) + await expect(authorize('approvers', 'alice')).rejects.toThrow( + 'alice is not an approver for olm/y.go (OWNERS: olm/OWNERS)', + ) + + server.use(...prHandlers(owners, ['olm/y.go'])) + await expect(authorize('approvers', 'carol')).resolves.toBeUndefined() + }) + + it('requires an approver to cover every changed file', async () => { + const owners = { 'OWNERS': rootOwners, 'sdk/OWNERS': sdkOwners, 'olm/OWNERS': olmOwners } + const files = ['sdk/x.go', 'olm/y.go'] + + server.use(...prHandlers(owners, files)) + await expect(authorize('approvers', 'bob')).rejects.toThrow( + 'bob is not an approver for olm/y.go (OWNERS: olm/OWNERS)', + ) + + const both = { ...owners, 'olm/OWNERS': `${olmOwners}- bob\n` } + server.use(...prHandlers(both, files)) + await expect(authorize('approvers', 'bob')).resolves.toBeUndefined() + }) + + it('lets a reviewer of any changed file lgtm', async () => { + const owners = { 'OWNERS': rootOwners, 'sdk/OWNERS': sdkOwners, 'olm/OWNERS': olmOwners } + const files = ['sdk/x.go', 'olm/y.go'] + + server.use(...prHandlers(owners, files)) + await expect(authorize('reviewers', 'bob')).resolves.toBeUndefined() + + server.use(...prHandlers(owners, files)) + await expect(authorize('reviewers', 'ryan')).resolves.toBeUndefined() + + server.use(...prHandlers(owners, files)) + await expect(authorize('reviewers', 'nobody')).rejects.toThrow( + 'nobody is not a reviewer or approver for any changed file', + ) + }) + + it('fails closed when a changed file has no covering OWNERS', async () => { + server.use(...prHandlers({ 'sdk/OWNERS': sdkOwners }, ['README.md'])) + + await expect(authorize('approvers', 'bob')).rejects.toThrow( + 'no OWNERS file covers README.md', + ) + }) + + it('falls back to membership when the tree has no OWNERS files', async () => { + server.use( + ...prHandlers({}, ['src/file1.txt']), + http.get(`${utils.api}/orgs/Codertocat/members/alice`, utils.mockResponse(204)), + http.get(`${repo}/collaborators/alice`, utils.mockResponse(404)), + ) + await expect(authorize('approvers', 'alice')).resolves.toBeUndefined() + + server.use( + ...prHandlers({}, ['src/file1.txt']), + http.get(`${utils.api}/orgs/Codertocat/members/alice`, utils.mockResponse(404)), + http.get(`${repo}/collaborators/alice`, utils.mockResponse(404)), + ) + await expect(authorize('approvers', 'alice')).rejects.toThrow( + 'alice is not a org member or collaborator', + ) + }) + + it('reads OWNERS from the base branch, ignoring an OWNERS edited by the PR', async () => { + const observeContents = new utils.ObserveRequest() + server.use( + http.get(`${repo}/contents/OWNERS`, utils.mockResponse(500, null, observeContents)), + ...prHandlers({ OWNERS: rootOwners }, ['OWNERS', 'src/file1.txt']), + ) + + await expect(authorize('approvers', 'mallory')).rejects.toThrow( + 'mallory is not an approver for OWNERS (OWNERS: OWNERS)', + ) + await expect(observeContents.notCalled()).resolves.toBe('not called') + }) + + it('compares logins case-insensitively', async () => { + server.use(...prHandlers({ OWNERS: 'approvers:\n- Alice\n' }, ['src/file1.txt'])) + + await expect(authorize('approvers', 'aLICE')).resolves.toBeUndefined() + }) + + it('uses the root OWNERS on an issue without touching pulls or trees', async () => { + const issueContext = new utils.MockContext(issueCommentEvent) + const observePull = new utils.ObserveRequest() + const observeTree = new utils.ObserveRequest() + server.use( + http.get(`${repo}/contents/OWNERS`, utils.mockResponse(200, contentsResponse('OWNERS', rootOwners))), + pullHandler(observePull), + ...treeHandlers({ OWNERS: rootOwners }, { observeTree }), + ) + + await expect(authorize('approvers', 'alice', issueContext)).resolves.toBeUndefined() + await expect(observePull.notCalled()).resolves.toBe('not called') + await expect(observeTree.notCalled()).resolves.toBe('not called') + }) + + it('probes ancestor directories when the tree is truncated', async () => { + const observeSdk = new utils.ObserveRequest() + const observeRoot = new utils.ObserveRequest() + const observeBlob = new utils.ObserveRequest() + server.use( + http.get(`${repo}/git/blobs/${blobSha('OWNERS')}`, utils.mockResponse(500, null, observeBlob)), + http.get(`${repo}/contents/sdk%2FOWNERS`, utils.mockResponse(200, contentsResponse('sdk/OWNERS', sdkOwners), observeSdk)), + http.get(`${repo}/contents/OWNERS`, utils.mockResponse(200, contentsResponse('OWNERS', rootOwners), observeRoot)), + pullHandler(), + filesHandler(changedFiles('sdk/x.go')), + ...treeHandlers({ 'OWNERS': rootOwners, 'sdk/OWNERS': sdkOwners }, { truncated: true }), + ) + + await expect(authorize('approvers', 'bob')).resolves.toBeUndefined() + await observeSdk.called() + await observeRoot.called() + expect(new URL(observeSdk.ref!.url).searchParams.get('ref')).toBe(baseSha) + expect(new URL(observeRoot.ref!.url).searchParams.get('ref')).toBe(baseSha) + await expect(observeBlob.notCalled()).resolves.toBe('not called') + }) + + it('treats a 404 probe as no OWNERS in that directory and fails on other errors', async () => { + server.use( + pullHandler(), + filesHandler(changedFiles('sdk/x.go')), + ...treeHandlers({}, { truncated: true }), + http.get(`${repo}/contents/sdk%2FOWNERS`, utils.mockResponse(404)), + http.get(`${repo}/contents/OWNERS`, utils.mockResponse(200, contentsResponse('OWNERS', rootOwners))), + ) + await expect(authorize('approvers', 'alice')).resolves.toBeUndefined() + + server.use( + pullHandler(), + filesHandler(changedFiles('sdk/x.go')), + ...treeHandlers({}, { truncated: true }), + http.get(`${repo}/contents/sdk%2FOWNERS`, utils.mockResponse(500)), + ) + await expect(authorize('approvers', 'alice')).rejects.toThrow( + `error loading OWNERS files at ${baseSha}`, + ) + }) + + it('rejects a malformed role', async () => { + server.use(...prHandlers({ 'OWNERS': rootOwners, 'sdk/OWNERS': 'approvers: alice\n' }, ['sdk/x.go'])) + + await expect(authorize('approvers', 'alice')).rejects.toThrow( + 'OWNERS at sdk/OWNERS: approvers must be a list of GitHub usernames', + ) + }) + + it('requires an approver to cover both sides of a rename', async () => { + const owners = { 'old/OWNERS': 'approvers:\n- olga\n', 'new/OWNERS': 'approvers:\n- nina\n' } + const rename = { filename: 'new/a.go', previous_filename: 'old/a.go', status: 'renamed' } + + server.use(...prHandlers(owners, [rename])) + await expect(authorize('approvers', 'nina')).rejects.toThrow( + 'nina is not an approver for old/a.go (OWNERS: old/OWNERS)', + ) + + server.use(...prHandlers({ ...owners, OWNERS: 'approvers:\n- root\n' }, [rename])) + await expect(authorize('approvers', 'root')).resolves.toBeUndefined() + }) + + it('pages through the changed files', async () => { + const pages: Record = { + 1: ['sdk/x.go'], + 2: ['olm/y.go'], + } + const seen: string[] = [] + server.use( + pullHandler(), + http.get(`${repo}/pulls/1/files`, ({ request }) => { + const url = new URL(request.url) + const page = url.searchParams.get('page') ?? '1' + seen.push(`page=${page}&per_page=${url.searchParams.get('per_page')}`) + const headers: Record = { 'Content-Type': 'application/json' } + if (page === '1') { + url.searchParams.set('page', '2') + headers.Link = `<${url}>; rel="next"` + } + return new Response(JSON.stringify(changedFiles(...pages[page])), { status: 200, headers }) + }), + ...treeHandlers({ 'sdk/OWNERS': sdkOwners, 'olm/OWNERS': olmOwners }), + ) + + await expect(authorize('approvers', 'bob')).rejects.toThrow( + 'bob is not an approver for olm/y.go (OWNERS: olm/OWNERS)', + ) + expect(seen).toEqual(['page=1&per_page=100', 'page=2&per_page=100']) + }) + + it('fails when the tree cannot be fetched', async () => { + server.use( + pullHandler(), + filesHandler(changedFiles('src/file1.txt')), + http.get(`${repo}/git/trees/${baseSha}`, utils.mockResponse(500)), + ) + + await expect(authorize('approvers', 'alice')).rejects.toThrow( + `error loading OWNERS files at ${baseSha}`, + ) + }) + + it('fails when a blob cannot be fetched or is not a file', async () => { + server.use( + http.get(`${repo}/git/blobs/${blobSha('OWNERS')}`, utils.mockResponse(500)), + ...prHandlers({ OWNERS: rootOwners }, ['src/file1.txt']), + ) + await expect(authorize('approvers', 'alice')).rejects.toThrow( + `error loading OWNERS files at ${baseSha}`, + ) + + server.use( + http.get(`${repo}/git/blobs/${blobSha('OWNERS')}`, utils.mockResponse(200, { sha: 'x' })), + ...prHandlers({ OWNERS: rootOwners }, ['src/file1.txt']), + ) + await expect(authorize('approvers', 'alice')).rejects.toThrow( + 'invalid OWNERS file returned from GitHub API for OWNERS', + ) + }) + + it('requests the tree recursively and only fetches OWNERS blobs in ancestor directories', async () => { + const observeTree = new utils.ObserveRequest() + const observeDocsBlob = new utils.ObserveRequest() + server.use( + http.get(`${repo}/git/blobs/${blobSha('docs/OWNERS')}`, utils.mockResponse(500, null, observeDocsBlob)), + pullHandler(), + filesHandler(changedFiles('sdk/x.go')), + ...treeHandlers({ 'OWNERS': rootOwners, 'sdk/OWNERS': sdkOwners, 'docs/OWNERS': 'approvers:\n- doc\n' }, { observeTree }), + ) + + await expect(authorize('approvers', 'bob')).resolves.toBeUndefined() + await observeTree.called() + expect(new URL(observeTree.ref!.url).searchParams.get('recursive')).toBe('true') + await expect(observeDocsBlob.notCalled()).resolves.toBe('not called') + }) +}) diff --git a/__tests__/utils/ownersFixtures.ts b/__tests__/utils/ownersFixtures.ts new file mode 100644 index 0000000..c49344d --- /dev/null +++ b/__tests__/utils/ownersFixtures.ts @@ -0,0 +1,106 @@ +import type { HttpHandler } from 'msw' +import { Buffer } from 'node:buffer' + +import { http } from 'msw' + +import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' +import * as utils from '../testUtils' + +export const repo = `${utils.api}/repos/Codertocat/Hello-World` +export const baseSha = 'basesha' + +export function prCommentEvent(body: string, commenter = 'Codertocat', author = 'some-author') { + const event = structuredClone(issueCommentEvent) + event.comment.body = body + event.comment.user.login = commenter + event.issue.user.login = author + return { + ...event, + issue: { + ...event.issue, + pull_request: { + url: 'https://api.github.com/repos/Codertocat/Hello-World/pulls/1', + }, + }, + } +} + +export interface ChangedFile { + filename: string + previous_filename?: string + status?: string +} + +export function changedFiles(...files: (string | ChangedFile)[]): ChangedFile[] { + return files.map(f => (typeof f === 'string' ? { filename: f, status: 'modified' } : f)) +} + +export function pullHandler(observe?: utils.ObserveRequest): HttpHandler { + return http.get(`${repo}/pulls/1`, utils.mockResponse(200, { base: { sha: baseSha } }, observe)) +} + +export function filesHandler(files: ChangedFile[], observe?: utils.ObserveRequest): HttpHandler { + return http.get(`${repo}/pulls/1/files`, utils.mockResponse(200, files, observe)) +} + +export function blobSha(path: string): string { + return `blob-${path.replace(/\//g, '-')}` +} + +// git tree and blob handlers for the OWNERS files given as { 'sdk/OWNERS': yaml } +export function treeHandlers( + owners: Record, + options: { truncated?: boolean, observeTree?: utils.ObserveRequest } = {}, +): HttpHandler[] { + const tree = Object.keys(owners).map(path => ({ + path, + mode: '100644', + type: 'blob', + sha: blobSha(path), + })) + + const handlers: HttpHandler[] = [ + http.get( + `${repo}/git/trees/${baseSha}`, + utils.mockResponse( + 200, + { sha: baseSha, truncated: options.truncated ?? false, tree }, + options.observeTree, + ), + ), + ] + + for (const [path, contents] of Object.entries(owners)) { + handlers.push( + http.get( + `${repo}/git/blobs/${blobSha(path)}`, + utils.mockResponse(200, { + sha: blobSha(path), + encoding: 'base64', + content: Buffer.from(contents).toString('base64'), + }), + ), + ) + } + + return handlers +} + +// a contents API response for a probed OWNERS file (truncated-tree fallback) +export function contentsResponse(path: string, contents: string) { + return { + type: 'file', + encoding: 'base64', + size: contents.length, + name: 'OWNERS', + path, + content: Buffer.from(contents).toString('base64'), + } +} + +export function prHandlers( + owners: Record, + files: (string | ChangedFile)[], +): HttpHandler[] { + return [pullHandler(), filesHandler(changedFiles(...files)), ...treeHandlers(owners)] +} diff --git a/dist/index.js b/dist/index.js index 328fba9..249d777 100644 --- a/dist/index.js +++ b/dist/index.js @@ -43956,6 +43956,182 @@ async function hold(context = github_context) { await labelIssue(octokit, context, issueNumber, ['hold']); } +;// CONCATENATED MODULE: ./lib/utils/owners.js + + + +/** + * Parse the contents of an OWNERS file. Logins are lowercased because GitHub + * logins are case-insensitive. + * + * @param path - the path of the OWNERS file, used in error messages + * @param contents - the yaml contents + */ +function parseOwners(path, contents) { + const loaded = contents.trim() === '' ? {} : load(contents); + const doc = typeof loaded === 'object' && loaded !== null && !Array.isArray(loaded) + ? loaded + : {}; + if ('filters' in doc) { + core_debug(`OWNERS at ${path}: filters are not supported; ignoring`); + } + const options = doc.options; + const noParentOwners = typeof options === 'object' + && options !== null + && options.no_parent_owners === true; + return { + path, + approvers: roleList(path, doc, 'approvers'), + reviewers: roleList(path, doc, 'reviewers'), + noParentOwners, + }; +} +function roleList(path, doc, role) { + const value = doc[role]; + if (value === undefined || value === null) { + return []; + } + if (!Array.isArray(value) || !value.every(v => typeof v === 'string')) { + throw new Error(`OWNERS at ${path}: ${role} must be a list of GitHub usernames`); + } + return value.map(v => v.toLowerCase()); +} +/** + * The directory that contains a path: 'sdk/OWNERS' is 'sdk', 'OWNERS' is '' + * + * @param path - a repository relative path + */ +function ownersDir(path) { + const slash = path.lastIndexOf('/'); + return slash === -1 ? '' : path.slice(0, slash); +} +/** + * Resolve the OWNERS that apply to a file: walk from its directory up to the + * root, taking the union of every OWNERS file on the way. A file with + * options.no_parent_owners stops the walk. + * + * @param file - the changed file + * @param owners - OWNERS files keyed by directory + * @returns undefined when no OWNERS file covers the file + */ +function effectiveOwners(file, owners) { + const approvers = new Set(); + const reviewers = new Set(); + const sources = []; + let dir = ownersDir(file); + for (;;) { + const found = owners.get(dir); + if (found !== undefined) { + found.approvers.forEach(a => approvers.add(a)); + found.reviewers.forEach(r => reviewers.add(r)); + sources.push(found.path); + if (found.noParentOwners) { + break; + } + } + if (dir === '') { + break; + } + dir = ownersDir(dir); + } + if (sources.length === 0) { + return undefined; + } + return { approvers, reviewers, sources }; +} +function ancestorDirs(paths) { + const dirs = new Set(['']); + for (const path of paths) { + for (let dir = ownersDir(path); dir !== ''; dir = ownersDir(dir)) { + dirs.add(dir); + } + } + return dirs; +} +function isOwnersPath(path) { + return path === 'OWNERS' || path.endsWith('/OWNERS'); +} +function decode(data, path) { + const file = data; + if (!file.content || !file.encoding) { + throw new Error(`invalid OWNERS file returned from GitHub API for ${path}`); + } + return external_node_buffer_.Buffer.from(file.content, file.encoding).toString(); +} +/** + * Load the OWNERS files at ref that can apply to the given paths. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param ref - the commit to read OWNERS files from + * @param pathsOfInterest - the changed files; only OWNERS in their ancestor directories are fetched + */ +async function loadOwnersTree(octokit, context, ref, pathsOfInterest) { + const dirs = ancestorDirs(pathsOfInterest); + let tree; + try { + const response = await octokit.git.getTree({ + ...context.repo, + tree_sha: ref, + recursive: 'true', + }); + tree = response.data; + } + catch (e) { + throw new Error(`error loading OWNERS files at ${ref}: ${e}`); + } + if (tree.truncated) { + // a truncated listing may have dropped OWNERS entries, so ask for each candidate path directly + core_debug(`tree at ${ref} is truncated; probing for OWNERS files`); + return probeOwners(octokit, context, ref, dirs); + } + const entries = tree.tree.filter(entry => entry.type === 'blob' + && entry.path !== undefined + && entry.sha !== undefined + && isOwnersPath(entry.path)); + const wanted = entries.filter(entry => dirs.has(ownersDir(entry.path))); + let files; + try { + files = await Promise.all(wanted.map(async (entry) => { + const blob = await octokit.git.getBlob({ + ...context.repo, + file_sha: entry.sha, + }); + return parseOwners(entry.path, decode(blob.data, entry.path)); + })); + } + catch (e) { + throw new Error(`error loading OWNERS files at ${ref}: ${e}`); + } + return { + owners: new Map(files.map(file => [ownersDir(file.path), file])), + hasOwners: entries.length > 0, + }; +} +async function probeOwners(octokit, context, ref, dirs) { + const owners = new Map(); + for (const dir of dirs) { + const path = dir === '' ? 'OWNERS' : `${dir}/OWNERS`; + let data; + try { + const response = await octokit.repos.getContent({ + ...context.repo, + path, + ref, + }); + data = response.data; + } + catch (e) { + if (typeof e === 'object' && e && 'status' in e && e.status === 404) { + continue; + } + throw new Error(`error loading OWNERS files at ${ref}: ${e}`); + } + owners.set(dir, parseOwners(path, decode(data, path))); + } + return { owners, hasOwners: owners.size > 0 }; +} + ;// CONCATENATED MODULE: ./lib/utils/auth.js @@ -44118,20 +44294,22 @@ async function checkCommenterAuth(octokit, context, issueNum, user) { return false; } /** - * When an OWNERS file is present, use it to authorize the action - otherwise fall back to allowing organization members and collaborators - * @param role is the role to check - * @param username is the user to authorize + * When the repository has OWNERS files, use them to authorize the action, + * otherwise fall back to allowing organization members and collaborators. + * On a pull request the OWNERS covering each changed file are used + * (approvers must cover every file, reviewers at least one); on an issue the + * root OWNERS file is used. + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param role - the role to check + * @param username - the user to authorize */ async function assertAuthorizedByOwnersOrMembership(octokit, context, role, username) { core_debug('Checking if the user is authorized to interact with prow'); - const owners = await retrieveOwnersFile(octokit, context); - if (owners !== '') { - if (!isInOwnersFile(owners, role, username)) { - throw new Error(`${username} is not included in the ${role} role in the OWNERS file`); - } - } - else { + const hasOwners = context.payload.issue?.pull_request !== undefined + ? await assertPullRequestOwner(octokit, context, role, username) + : await assertRootOwner(octokit, context, role, username); + if (!hasOwners) { const isOrgMember = await checkOrgMember(octokit, context, username); const isCollaborator = await checkCollaborator(octokit, context, username); if (!isOrgMember && !isCollaborator) { @@ -44139,6 +44317,62 @@ async function assertAuthorizedByOwnersOrMembership(octokit, context, role, user } } } +/** + * Authorize against the root OWNERS file of the default branch. + * @returns false when the repository has no root OWNERS file + */ +async function assertRootOwner(octokit, context, role, username) { + const contents = await retrieveOwnersFile(octokit, context); + if (contents === '') { + return false; + } + const owners = parseOwners('OWNERS', contents); + if (!owners[role].includes(username.toLowerCase())) { + throw new Error(`${username} is not included in the ${role} role in the OWNERS file`); + } + return true; +} +/** + * Authorize against the OWNERS files covering the pull request's changed files. + * @returns false when the repository has no OWNERS files at all + */ +async function assertPullRequestOwner(octokit, context, role, username) { + const pullNumber = context.payload.issue.number; + const { data: pull } = await octokit.pulls.get({ + ...context.repo, + pull_number: pullNumber, + }); + const changed = await octokit.paginate(octokit.pulls.listFiles, { + ...context.repo, + pull_number: pullNumber, + per_page: 100, + }); + const files = [...new Set(changed.flatMap(f => f.previous_filename !== undefined ? [f.filename, f.previous_filename] : [f.filename]))]; + // OWNERS come from the base branch so a PR cannot grant itself approvers + const tree = await loadOwnersTree(octokit, context, pull.base.sha, files); + if (!tree.hasOwners) { + core_debug('No OWNERS files found'); + return false; + } + const login = username.toLowerCase(); + const covered = files.map((file) => { + const owners = effectiveOwners(file, tree.owners); + if (owners === undefined) { + throw new Error(`no OWNERS file covers ${file}`); + } + return { file, owners }; + }); + if (role === 'approvers') { + const failing = covered.find(({ owners }) => !owners.approvers.has(login)); + if (failing !== undefined) { + throw new Error(`${username} is not an approver for ${failing.file} (OWNERS: ${failing.owners.sources.join(', ')})`); + } + } + else if (!covered.some(({ owners }) => owners.reviewers.has(login) || owners.approvers.has(login))) { + throw new Error(`${username} is not a reviewer or approver for any changed file`); + } + return true; +} /** * Retrieve the contents of the OWNERS file at the root of the repository. * If the file does not exist, returns an empty string. @@ -44167,22 +44401,6 @@ async function retrieveOwnersFile(octokit, context) { core_debug(`OWNERS file contents: ${decoded}`); return decoded; } -/** - * Determine if the user has the specified role in the OWNERS file. - * @param ownersContents - the contents of the OWNERS file - * @param role - the role to check - * @param username - the user to authorize - */ -function isInOwnersFile(ownersContents, role, username) { - core_debug(`checking if ${username} is in the ${role} in the OWNERS file`); - const ownersData = load(ownersContents); - const roleMembers = ownersData[role]; - if (roleMembers !== undefined) { - return roleMembers.includes(username); - } - info(`${username} is not in the ${role} role in the OWNERS file`); - return false; -} ;// CONCATENATED MODULE: ./lib/utils/comments.js /** diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 502f1f8..e27eed8 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -1,10 +1,11 @@ import type { Octokit } from '@octokit/rest' import type { Context } from './context' +import type { OwnersRole } from './owners' import { Buffer } from 'node:buffer' import * as core from '@actions/core' -import * as yaml from 'js-yaml' +import { effectiveOwners, loadOwnersTree, parseOwners } from './owners' function getErrorDetails(error: unknown): { status: unknown, message: string } { if (typeof error === 'object' && error !== null) { @@ -228,28 +229,30 @@ export async function checkCommenterAuth( } /** - * When an OWNERS file is present, use it to authorize the action - otherwise fall back to allowing organization members and collaborators - * @param role is the role to check - * @param username is the user to authorize + * When the repository has OWNERS files, use them to authorize the action, + * otherwise fall back to allowing organization members and collaborators. + * On a pull request the OWNERS covering each changed file are used + * (approvers must cover every file, reviewers at least one); on an issue the + * root OWNERS file is used. + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param role - the role to check + * @param username - the user to authorize */ export async function assertAuthorizedByOwnersOrMembership( octokit: Octokit, context: Context, - role: string, + role: OwnersRole, username: string, ): Promise { core.debug('Checking if the user is authorized to interact with prow') - const owners = await retrieveOwnersFile(octokit, context) - if (owners !== '') { - if (!isInOwnersFile(owners, role, username)) { - throw new Error( - `${username} is not included in the ${role} role in the OWNERS file`, - ) - } - } - else { + const hasOwners + = context.payload.issue?.pull_request !== undefined + ? await assertPullRequestOwner(octokit, context, role, username) + : await assertRootOwner(octokit, context, role, username) + + if (!hasOwners) { const isOrgMember = await checkOrgMember(octokit, context, username) const isCollaborator = await checkCollaborator(octokit, context, username) @@ -259,6 +262,88 @@ export async function assertAuthorizedByOwnersOrMembership( } } +/** + * Authorize against the root OWNERS file of the default branch. + * @returns false when the repository has no root OWNERS file + */ +async function assertRootOwner( + octokit: Octokit, + context: Context, + role: OwnersRole, + username: string, +): Promise { + const contents = await retrieveOwnersFile(octokit, context) + if (contents === '') { + return false + } + + const owners = parseOwners('OWNERS', contents) + if (!owners[role].includes(username.toLowerCase())) { + throw new Error( + `${username} is not included in the ${role} role in the OWNERS file`, + ) + } + return true +} + +/** + * Authorize against the OWNERS files covering the pull request's changed files. + * @returns false when the repository has no OWNERS files at all + */ +async function assertPullRequestOwner( + octokit: Octokit, + context: Context, + role: OwnersRole, + username: string, +): Promise { + const pullNumber = context.payload.issue!.number + + const { data: pull } = await octokit.pulls.get({ + ...context.repo, + pull_number: pullNumber, + }) + const changed = await octokit.paginate(octokit.pulls.listFiles, { + ...context.repo, + pull_number: pullNumber, + per_page: 100, + }) + const files = [...new Set(changed.flatMap(f => + f.previous_filename !== undefined ? [f.filename, f.previous_filename] : [f.filename], + ))] + + // OWNERS come from the base branch so a PR cannot grant itself approvers + const tree = await loadOwnersTree(octokit, context, pull.base.sha, files) + if (!tree.hasOwners) { + core.debug('No OWNERS files found') + return false + } + + const login = username.toLowerCase() + const covered = files.map((file) => { + const owners = effectiveOwners(file, tree.owners) + if (owners === undefined) { + throw new Error(`no OWNERS file covers ${file}`) + } + return { file, owners } + }) + + if (role === 'approvers') { + const failing = covered.find(({ owners }) => !owners.approvers.has(login)) + if (failing !== undefined) { + throw new Error( + `${username} is not an approver for ${failing.file} (OWNERS: ${failing.owners.sources.join(', ')})`, + ) + } + } + else if (!covered.some(({ owners }) => owners.reviewers.has(login) || owners.approvers.has(login))) { + throw new Error( + `${username} is not a reviewer or approver for any changed file`, + ) + } + + return true +} + /** * Retrieve the contents of the OWNERS file at the root of the repository. * If the file does not exist, returns an empty string. @@ -295,26 +380,3 @@ async function retrieveOwnersFile( core.debug(`OWNERS file contents: ${decoded}`) return decoded } - -/** - * Determine if the user has the specified role in the OWNERS file. - * @param ownersContents - the contents of the OWNERS file - * @param role - the role to check - * @param username - the user to authorize - */ -function isInOwnersFile( - ownersContents: string, - role: string, - username: string, -): boolean { - core.debug(`checking if ${username} is in the ${role} in the OWNERS file`) - const ownersData: any = yaml.load(ownersContents) - - const roleMembers = ownersData[role] - if ((roleMembers as string[]) !== undefined) { - return roleMembers.includes(username) - } - - core.info(`${username} is not in the ${role} role in the OWNERS file`) - return false -} From 34ac705755a6f3d93d7dd9eeaa5eed33a041e7bf Mon Sep 17 00:00:00 2001 From: Jeffrey Sica Date: Thu, 10 Sep 2026 20:36:27 -0500 Subject: [PATCH 3/3] docs: directory-scoped OWNERS Rewrite the OWNERS section of docs/commands.md for multiple OWNERS files: resolution walk and no_parent_owners, base-branch reading and why, /approve (every changed file) versus /lgtm (at least one), the issue case, fail-closed rules, when the membership fallback applies, and the supported, ignored and unsupported keys. Point the /lgtm and /approve policy cells at the section and extend the example in docs/examples.md with a nested sdk/OWNERS. Signed-off-by: Jeffrey Sica --- docs/commands.md | 56 ++++++++++++++++++++++++++++++++++++++---------- docs/examples.md | 14 +++++++++++- 2 files changed, 58 insertions(+), 12 deletions(-) diff --git a/docs/commands.md b/docs/commands.md index 2e15da0..add0209 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -4,10 +4,10 @@ A command must start a line of the comment (leading whitespace is allowed); a co Commands | Policy | Description --- | --- | --- -`/approve` | [OWNERS](#owners) if present, otherwise Org members & Collaborators | approve all the files for the current PR -`/approve no-issue` | [OWNERS](#owners) if present, otherwise Org members & Collaborators | same as `/approve`; accepted for Prow compatibility -`/approve cancel` | [OWNERS](#owners) if present, otherwise Org member & Collaborators | removes your approval on this pull-request -`/remove-approve` | [OWNERS](#owners) if present, otherwise Org member & Collaborators | same as `/approve cancel` +`/approve` | [OWNERS](#owners) approver for **every** changed file if the repo has OWNERS files, otherwise Org members & Collaborators | approve all the files for the current PR +`/approve no-issue` | same as `/approve` | same as `/approve`; accepted for Prow compatibility +`/approve cancel` | same as `/approve` | removes your approval on this pull-request +`/remove-approve` | same as `/approve` | same as `/approve cancel` `/assign [@userA @userB @etc]` | anyone | Assign other users (or yourself if no one is specified). Target user must be Org Member, Collaborator, or have previously commented `/unassign [@userA @userB @etc]` | anyone | Unassigns specified people (or yourself if no one is specified). Target must have been already assigned. `/cc [@userA @userB @etc]` | anyone | Request review from specified people (or yourself if no one is specified). Target be an Org Member, Collaborator, or have previously commented. @@ -27,9 +27,9 @@ Label Commands | Policy | Description `/remove-area [label1 label2 ...]` | anyone | removes an area/<> label(s) if it's defined in [the `.prowlabels.yaml` file](./labeling.md) `/kind [label1 label2 ...]` | anyone | adds a kind/<> label(s) if it's defined in [the `.prowlabels.yaml` file](./labeling.md) `/remove-kind [label1 label2 ...]` | anyone | removes a kind/<> label(s) if it's defined in [the `.prowlabels.yaml` file](./labeling.md) -`/lgtm` | [OWNERS](#owners) reviewers if present, otherwise Collaborators and Org Members; **not the PR author** | adds the `lgtm` label. This is used for [automatic PR merging](./automatic-merging.md). Like Prow, you cannot LGTM your own PR; the guard also applies to issues since the label has no meaning there either -`/lgtm cancel` | [OWNERS](#owners) reviewers if present, otherwise Collaborators and Org Members, **or the PR author** | removes the `lgtm` label -`/remove-lgtm` | [OWNERS](#owners) reviewers if present, otherwise Collaborators and Org Members, **or the PR author** | same as `/lgtm cancel` +`/lgtm` | [OWNERS](#owners) reviewer or approver for **at least one** changed file if the repo has OWNERS files, otherwise Collaborators and Org Members; **not the PR author** | adds the `lgtm` label. This is used for [automatic PR merging](./automatic-merging.md). Like Prow, you cannot LGTM your own PR; the guard also applies to issues since the label has no meaning there either +`/lgtm cancel` | same as `/lgtm`, **or the PR author** | removes the `lgtm` label +`/remove-lgtm` | same as `/lgtm`, **or the PR author** | same as `/lgtm cancel` `/hold` | anyone | adds the `hold` label which prevents [automatic PR merging](./automatic-merging.md). Also see [lgtm removal on pr update](./pr-jobs.md) `/hold cancel` | anyone | removes the `hold` label `/unhold`, `/remove-hold` | anyone | same as `/hold cancel` @@ -79,13 +79,43 @@ The API key is optional; unauthenticated access is best effort and may be rate l ## OWNERS -A simplified version of [Prow's OWNERS](https://go.k8s.io/owners) file is supported. When an OWNERS file is present at the root of the repository, it is used to authorize the /lgtm and /approve commands. See an [example][owners-example] using an OWNERS file. +A simplified version of [Prow's OWNERS](https://go.k8s.io/owners) files is supported. When the repository contains any `OWNERS` file, the `/lgtm` and `/approve` commands are authorized against them; when it contains none, org members and collaborators may use both commands. See an [example][owners-example] using OWNERS files. -The `reviewers` role grants access to the /lgtm command and the approvers role grants access to the /approve command. +### Where OWNERS files live -The `approvers` role does not grant the reviewers role, a user must be in both roles to use /lgtm and /approve. +An `OWNERS` file may be placed in any directory. It applies to the files in that directory and all directories below it. Which files apply to a given path is resolved the way Prow does it: walk from the file's directory up to the repository root and take the union of every `OWNERS` file on the way (a directory's `OWNERS` plus its parents'). A root-only `OWNERS` therefore covers the whole repository. -The OWNERS file must be in YAML format. All entries are expected to be GitHub usernames; teams are not supported. +Setting `options.no_parent_owners: true` in an `OWNERS` file stops the walk there, so only that file (and any below it) applies and the parents' approvers and reviewers are not inherited. + +### Which files decide the outcome + +On a pull request the changed files are listed (for renames both the old and the new path count) and their OWNERS are read from the PR's **base** branch. The head branch is never consulted, so a pull request cannot grant itself approvers by editing an `OWNERS` file. + +- `/approve`: the commenter must be an `approver` for **every** changed file. The refusal names the first file that is not covered and the OWNERS files consulted for it. +- `/lgtm`: the commenter must be a `reviewer` or `approver` for **at least one** changed file (Prow's lgtm rule). +- On an issue there are no changed files, so the root `OWNERS` of the default branch is used as before. + +The `approvers` role does not grant `/lgtm` on its own for issues; on pull requests an approver of a changed file may also `/lgtm`. + +### Failure modes + +Authorization fails closed: + +- a changed file with no covering `OWNERS` file fails the command with an error naming the file; +- an `OWNERS` file that cannot be fetched or parsed (for example `approvers` is not a list) fails the command; +- the org-member/collaborator fallback applies only when the repository has **no** `OWNERS` file at all. + +### File format + +The OWNERS file must be in YAML format. All entries are expected to be GitHub usernames (compared case-insensitively); teams are not supported. + +Key | Meaning +--- | --- +`approvers` | list of usernames who may use `/approve` (and `/lgtm` on a pull request) +`reviewers` | list of usernames who may use `/lgtm` +`options.no_parent_owners` | `true` stops inheritance from parent directories + +`emeritus_approvers`, `emeritus_reviewers`, `labels` and `filters` are accepted but ignored (`filters` is noted in the debug log). `OWNERS_ALIASES` files and aliases are not supported. Unknown keys are tolerated. ```yaml # List of usernames who may use /lgtm @@ -99,6 +129,10 @@ approvers: - user1 - user2 - admin1 + +# Optional: do not inherit approvers and reviewers from parent directories +options: + no_parent_owners: false ``` [owners-example]: ./examples.md#review-and-approve-pull-requests diff --git a/docs/examples.md b/docs/examples.md index 902b601..b00bb8a 100644 --- a/docs/examples.md +++ b/docs/examples.md @@ -28,7 +28,7 @@ priority: ## Review and Approve Pull Requests -Below is an example of how to use an [OWNERS](./commands.md#owners) file with the Prow action. +Below is an example of how to use [OWNERS](./commands.md#owners) files with the Prow action. Add an OWNERS file to the root of the repository in the default branch. ```yaml @@ -45,6 +45,18 @@ approvers: - admin1 ``` +Optionally add more OWNERS files in subdirectories, for example `sdk/OWNERS`. Their approvers and reviewers apply to files under `sdk/` in addition to the root ones, unless `no_parent_owners` is set. + +```yaml +# sdk/OWNERS: sdk-maintainer may /approve and /lgtm changes under sdk/ +approvers: + - sdk-maintainer +reviewers: + - sdk-reviewer +``` + +A pull request that changes files under `sdk/` and elsewhere needs an approver for every changed file (`user1` or `admin1` here, since they are inherited from the root), while `/lgtm` needs a reviewer or approver of at least one changed file. OWNERS files are read from the base branch of the pull request. + Grant the default GITHUB_TOKEN permission to label issues and review pull requests. ```yaml name: Handle prow slash commands