From 05f387b006f7fcbd2ea9b8c85e25c4ddf77941dc Mon Sep 17 00:00:00 2001 From: mrbobbytables Date: Thu, 15 Oct 2026 14:01:39 +0000 Subject: [PATCH 1/3] feat(owners): support GitHub groups in OWNERS approvers and reviewers List a team of the repository's org as @org/team; its members are resolved through the teams API. Other orgs' groups are ignored. Hive-Run: cncf/prow-github-actions#485 Hive-Plan: owners-github-groups Hive-Spec: owners#github-groups Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: mrbobbytables --- __tests__/utils/owners.test.ts | 29 +++++++++++++ dist/index.js | 71 ++++++++++++++++++++++++++++-- docs/commands.md | 12 ++++++ src/utils/auth.ts | 6 +-- src/utils/owners.ts | 79 ++++++++++++++++++++++++++++++++++ src/utils/pullRequestOwners.ts | 5 ++- 6 files changed, 194 insertions(+), 8 deletions(-) diff --git a/__tests__/utils/owners.test.ts b/__tests__/utils/owners.test.ts index 58e3f5ae..addc31d9 100644 --- a/__tests__/utils/owners.test.ts +++ b/__tests__/utils/owners.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it, vi } from 'vitest' import { effectiveOwners, + expandGroups, ownersDir, parseOwners, } from '../../src/utils/owners' @@ -186,3 +187,31 @@ describe('effectiveOwners', () => { expect(effectiveOwners('README.md', new Map())).toBeUndefined() }) }) + +describe('expandGroups', () => { + const context = { repo: { owner: 'MyOrg', repo: 'r' } } as never + + function client(listed: Record) { + const paginate = vi.fn(async (_fn: unknown, params: { team_slug: string }) => { + const members = listed[params.team_slug] + if (members === undefined) { + throw new Error('404') + } + return members.map(login => ({ login })) + }) + return { paginate, teams: { listMembersInOrg: vi.fn() } } as never + } + + it('replaces same-org groups with their members and keeps plain logins', async () => { + const result = await expandGroups(client({ maintainers: ['Bob', 'carol'] }), context, ['alice', '@myorg/maintainers']) + + expect([...result].sort()).toEqual(['alice', 'bob', 'carol']) + }) + + it('ignores groups of another org and groups it cannot read', async () => { + const result = await expandGroups(client({}), context, ['alice', '@other/team', '@myorg/missing']) + + expect([...result]).toEqual(['alice']) + expect(core.warning).toHaveBeenCalledTimes(2) + }) +}) diff --git a/dist/index.js b/dist/index.js index 4319e8ef..03d0ae26 100644 --- a/dist/index.js +++ b/dist/index.js @@ -41870,6 +41870,70 @@ function stringList(path, doc, key, what) { } return value; } +/** + * Parse an `@org/team-slug` entry of an approvers or reviewers list. + * + * @param entry - a lowercased OWNERS entry + * @returns undefined when the entry is a plain login + */ +function parseGroup(entry) { + const match = /^@([^/\s]+)\/([^/\s]+)$/.exec(entry); + return match === null ? undefined : { org: match[1], slug: match[2] }; +} +const groupCache = new Map(); +/** + * Replace the `@org/team` entries of a list of OWNERS entries with the logins of + * the team's members (transitive, as the teams API returns them). A group of + * another organization, or one that cannot be read, contributes nobody. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param entries - lowercased OWNERS entries + */ +async function expandGroups(octokit, context, entries) { + const result = new Set(); + const owner = context.repo.owner.toLowerCase(); + const groups = new Set(); + for (const entry of entries) { + const group = parseGroup(entry); + if (group === undefined) { + result.add(entry); + } + else if (group.org !== owner) { + warning(`OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`); + } + else { + groups.add(group.slug); + } + } + const members = await Promise.all([...groups].map(slug => memoized(groupCache, `${owner}/${slug}`, async () => { + try { + const listed = await octokit.paginate(octokit.teams.listMembersInOrg, { + org: context.repo.owner, + team_slug: slug, + per_page: 100, + }); + return listed.map(member => member.login.toLowerCase()); + } + catch (e) { + warning(`OWNERS: could not read the members of @${owner}/${slug}: ${e}`); + return []; + } + }))); + members.flat().forEach(login => result.add(login)); + return result; +} +/** an OwnersSet whose approvers and reviewers hold logins only */ +async function expandOwnersSet(octokit, context, set) { + if (set === undefined) { + return undefined; + } + const [approvers, reviewers] = await Promise.all([ + expandGroups(octokit, context, set.approvers), + expandGroups(octokit, context, set.reviewers), + ]); + return { ...set, approvers, reviewers }; +} /** * The directory that contains a path: 'sdk/OWNERS' is 'sdk', 'OWNERS' is '' * @@ -42035,6 +42099,7 @@ function branchHasOwners(octokit, context, branch) { function resetOwnersCaches() { hasOwnersCache.clear(); treeCache.clear(); + groupCache.clear(); } function memoized(cache, key, compute) { let pending = cache.get(key); @@ -42138,7 +42203,7 @@ async function pullRequestOwners_load(octokit, context, pullNumber) { return pull.base.sha; }); const tree = await loadOwnersTree(octokit, context, baseSha, files); - const perFile = new Map(files.map(file => [file, effectiveOwners(file, tree.owners)])); + const perFile = new Map(await Promise.all(files.map(async (file) => [file, await expandOwnersSet(octokit, context, effectiveOwners(file, tree.owners))]))); return { number: pullNumber, baseSha, @@ -42420,7 +42485,7 @@ async function assertRootOwner(octokit, context, role, username) { return false; } const owners = parseOwners('OWNERS', contents); - if (!owners[role].includes(username.toLowerCase())) { + if (!(await expandGroups(octokit, context, owners[role])).has(username.toLowerCase())) { throw new Error(`${username} is not included in the ${role} role in the OWNERS file`); } return true; @@ -42465,7 +42530,7 @@ async function rootOwnersIncludes(octokit, context, user) { } const owners = parseOwners('OWNERS', contents); const login = user.toLowerCase(); - return owners.reviewers.includes(login) || owners.approvers.includes(login); + return (await expandGroups(octokit, context, [...owners.reviewers, ...owners.approvers])).has(login); } /** * Retrieve the contents of the OWNERS file at the root of the repository. diff --git a/docs/commands.md b/docs/commands.md index b18bb8e4..49e96281 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -181,6 +181,18 @@ An `OWNERS` file may be placed in any directory. It applies to the files in that 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. +### GitHub groups + +`approvers` and `reviewers` may list a GitHub team of the repository's own organization as `@/` (quote it in YAML, since a plain `@` cannot start a scalar): + +```yaml +approvers: + - user1 + - '@myorg/maintainers' +``` + +The team's members (child teams included, as the teams API returns them) are looked up once per team per run and treated as if listed individually. A team of another organization is ignored with a warning, and so is a team the token cannot read: it needs `members: read` (a token that can read org teams). Groups are matched when OWNERS are resolved, so changes to the team apply on the next event. + ### 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 **current tip of the pull request's base branch**, never from the pull request itself: the head branch is never consulted, so a pull request cannot grant itself approvers by editing an `OWNERS` file, and a pull request opened before the OWNERS files existed picks them up without a rebase (GitHub's `base.sha` is a snapshot from the PR's last update; the bot reads the branch instead, one `GET /branches/{base}` per branch per run). The `[APPROVALNOTIFIER]` links point at that tip. diff --git a/src/utils/auth.ts b/src/utils/auth.ts index db993139..f8e37f83 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -7,7 +7,7 @@ import { Buffer } from 'node:buffer' import * as core from '@actions/core' import { defaultAuthorization, loadProwConfig, resolveAuthorization } from './config' -import { parseOwners } from './owners' +import { expandGroups, parseOwners } from './owners' import { loadPullRequestOwners } from './pullRequestOwners' function getErrorDetails(error: unknown): { status: unknown, message: string } { @@ -356,7 +356,7 @@ async function assertRootOwner( } const owners = parseOwners('OWNERS', contents) - if (!owners[role].includes(username.toLowerCase())) { + if (!(await expandGroups(octokit, context, owners[role])).has(username.toLowerCase())) { throw new Error( `${username} is not included in the ${role} role in the OWNERS file`, ) @@ -420,7 +420,7 @@ async function rootOwnersIncludes( const owners = parseOwners('OWNERS', contents) const login = user.toLowerCase() - return owners.reviewers.includes(login) || owners.approvers.includes(login) + return (await expandGroups(octokit, context, [...owners.reviewers, ...owners.approvers])).has(login) } /** diff --git a/src/utils/owners.ts b/src/utils/owners.ts index 52c8306a..6428b2d0 100644 --- a/src/utils/owners.ts +++ b/src/utils/owners.ts @@ -89,6 +89,84 @@ function stringList( return value } +/** + * Parse an `@org/team-slug` entry of an approvers or reviewers list. + * + * @param entry - a lowercased OWNERS entry + * @returns undefined when the entry is a plain login + */ +export function parseGroup(entry: string): { org: string, slug: string } | undefined { + const match = /^@([^/\s]+)\/([^/\s]+)$/.exec(entry) + return match === null ? undefined : { org: match[1]!, slug: match[2]! } +} + +const groupCache = new Map>() + +/** + * Replace the `@org/team` entries of a list of OWNERS entries with the logins of + * the team's members (transitive, as the teams API returns them). A group of + * another organization, or one that cannot be read, contributes nobody. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param entries - lowercased OWNERS entries + */ +export async function expandGroups( + octokit: Octokit, + context: Context, + entries: Iterable, +): Promise> { + const result = new Set() + const owner = context.repo.owner.toLowerCase() + const groups = new Set() + for (const entry of entries) { + const group = parseGroup(entry) + if (group === undefined) { + result.add(entry) + } + else if (group.org !== owner) { + core.warning(`OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`) + } + else { + groups.add(group.slug) + } + } + + const members = await Promise.all([...groups].map(slug => + memoized(groupCache, `${owner}/${slug}`, async () => { + try { + const listed = await octokit.paginate(octokit.teams.listMembersInOrg, { + org: context.repo.owner, + team_slug: slug, + per_page: 100, + }) + return listed.map(member => member.login.toLowerCase()) + } + catch (e) { + core.warning(`OWNERS: could not read the members of @${owner}/${slug}: ${e}`) + return [] + } + }))) + members.flat().forEach(login => result.add(login)) + return result +} + +/** an OwnersSet whose approvers and reviewers hold logins only */ +export async function expandOwnersSet( + octokit: Octokit, + context: Context, + set: OwnersSet | undefined, +): Promise { + if (set === undefined) { + return undefined + } + const [approvers, reviewers] = await Promise.all([ + expandGroups(octokit, context, set.approvers), + expandGroups(octokit, context, set.reviewers), + ]) + return { ...set, approvers, reviewers } +} + /** * The directory that contains a path: 'sdk/OWNERS' is 'sdk', 'OWNERS' is '' * @@ -301,6 +379,7 @@ export function branchHasOwners(octokit: Octokit, context: Context, branch: stri export function resetOwnersCaches(): void { hasOwnersCache.clear() treeCache.clear() + groupCache.clear() } function memoized(cache: Map>, key: string, compute: () => Promise): Promise { diff --git a/src/utils/pullRequestOwners.ts b/src/utils/pullRequestOwners.ts index b4dbfc9b..a50c2cc7 100644 --- a/src/utils/pullRequestOwners.ts +++ b/src/utils/pullRequestOwners.ts @@ -4,7 +4,7 @@ import type { OwnersSet, OwnersTree } from './owners' import * as core from '@actions/core' -import { effectiveOwners, loadOwnersTree } from './owners' +import { effectiveOwners, expandOwnersSet, loadOwnersTree } from './owners' /** a pull request and the OWNERS covering its changed files; every login is lowercased */ export interface PullRequestOwners { @@ -83,7 +83,8 @@ async function load( return pull.base.sha }) const tree = await loadOwnersTree(octokit, context, baseSha, files) - const perFile = new Map(files.map(file => [file, effectiveOwners(file, tree.owners)])) + const perFile = new Map(await Promise.all(files.map(async file => + [file, await expandOwnersSet(octokit, context, effectiveOwners(file, tree.owners))] as const))) return { number: pullNumber, From d58e18fd641aa0b83bc4fb3e97ebbd64e3cca68a Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Thu, 15 Oct 2026 10:05:47 -0500 Subject: [PATCH 2/3] fix(owners): warn once per group entry, skip team reads for direct listings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - expandGroups warned on every call, and the per-file expansion calls it twice per changed file, so one foreign-org group in a root OWNERS file produced a warning per file per role. Warn once per entry per run. - An `@…` entry that is not `@/` used to be kept as a literal login that can never match; drop it with a warning. - ownersInclude answers a directly listed login before reading any team, so the root-OWNERS authorization path makes no teams-API call in the common case. - Docs: the default GITHUB_TOKEN has no organization permissions, so groups need the `token` secret with Members: read; say so in commands, installing and the day-one surprises table, and point the OWNERS_ALIASES note at groups. - Tests: unit cases for the dedupe, the malformed entry, expandOwnersSet and ownersInclude; a bundle file driving all four arms through dist/index.js. dist/ repacked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- __tests__/bundle/ownersGroups.test.ts | 121 ++++++++++++++++++++++++++ __tests__/utils/owners.test.ts | 93 +++++++++++++++++++- dist/index.js | 66 ++++++++++---- docs/commands.md | 4 +- docs/installing.md | 2 +- docs/prow-for-your-org.md | 6 +- src/utils/auth.ts | 6 +- src/utils/owners.ts | 50 +++++++++-- 8 files changed, 316 insertions(+), 32 deletions(-) create mode 100644 __tests__/bundle/ownersGroups.test.ts diff --git a/__tests__/bundle/ownersGroups.test.ts b/__tests__/bundle/ownersGroups.test.ts new file mode 100644 index 00000000..c1a07f60 --- /dev/null +++ b/__tests__/bundle/ownersGroups.test.ts @@ -0,0 +1,121 @@ +import type { FakeGithub } from './fakeGithub' +import { Buffer } from 'node:buffer' +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' + +import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' +import { prCommentEvent } from '../utils/ownersData' +import { start } from './fakeGithub' +import { comment, helpersFor, repo, token } from './helpers' +import { runBundle } from './runBundle' + +vi.setConfig({ testTimeout: 30_000 }) + +// `@org/team` entries of OWNERS files (src/utils/owners.ts: expandGroups, ownersInclude), driven through +// dist/index.js: the team is read once per run however many files and roles list it, a login listed +// directly never reads a team, and a group of another org or an unreadable team warns once and admits nobody +describe('dist/index.js OWNERS groups', () => { + const teamRead = `GET /orgs/Codertocat/teams/reviewers/members?per_page=100` + const rootOwnersRead = `GET ${repo}/contents/OWNERS` + const labelsRead = `GET ${repo}/labels?per_page=100` + const labelsPost = `POST ${repo}/issues/1/labels` + const commentPost = `POST ${repo}/issues/1/comments` + let gh: FakeGithub + const { calls, expectCommandThenConfig, routeOwners } = helpersFor(() => gh) + + beforeAll(async () => { + gh = await start() + }) + beforeEach(() => gh.mergeQueueFallback({ pullRequestId: 'PR_none', headOid: pullReqOpenedEvent.pull_request.head.sha, enabled: false })) + afterEach(() => gh.reset()) + afterAll(() => gh.close()) + + function routeRootOwners(contents: string) { + gh.route('GET', `${repo}/contents/OWNERS`, { + status: 200, + body: { encoding: 'base64', content: Buffer.from(contents).toString('base64') }, + }) + } + + function routeTeam(members: string[]) { + gh.route('GET', '/orgs/Codertocat/teams/reviewers/members', { status: 200, body: members.map(login => ({ login })) }) + } + + function routeLabelWrite() { + gh.route('GET', `${repo}/labels`, { status: 200, body: [{ name: 'lgtm' }] }) + gh.route('POST', `${repo}/issues/1/labels`, { status: 200, body: [] }) + } + + function lgtm(payload: Record) { + return runBundle({ eventName: 'issue_comment', payload, inputs: { ...token, 'prow-commands': '/lgtm' }, apiUrl: gh.url }) + } + + function warnings(stdout: string) { + return stdout.split('\n').filter(line => line.startsWith('::warning::')) + } + + it('on a pull request, a reviewer listed only through a team is authorized; the team is read once, other-org and malformed entries warn once', async () => { + routeOwners( + { + 'OWNERS': 'approvers:\n- \'@bob\'\nreviewers:\n- \'@other/team\'\n', + 'sdk/OWNERS': 'reviewers:\n- \'@codertocat/reviewers\'\n', + }, + ['sdk/a.go', 'sdk/b.go'], + { user: { login: 'some-author' } }, + ) + routeTeam(['Alice', 'carol']) + gh.route('POST', `${repo}/statuses/headsha`, { status: 201, body: {} }) + routeLabelWrite() + + const result = await lgtm(prCommentEvent('/lgtm', 'alice')) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual([{ labels: ['lgtm'] }]) + expect(gh.requestsMatching('GET', /\/orgs\/Codertocat\/members\/|\/collaborators\//)).toEqual([]) + expect(calls().filter(c => c === teamRead)).toHaveLength(1) + expect(warnings(result.stdout)).toEqual([ + '::warning::OWNERS: ignoring @bob: a group is written @/', + '::warning::OWNERS: ignoring group @other/team: only groups of codertocat are supported', + ]) + }) + + it('on an issue, a commenter listed through a team in the root OWNERS file is authorized by the team read', async () => { + routeRootOwners('reviewers:\n- \'@codertocat/reviewers\'\n') + routeTeam(['codertocat']) + routeLabelWrite() + + const result = await lgtm(comment('/lgtm', 'some-author')) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/).map(r => r.body)).toEqual([{ labels: ['lgtm'] }]) + expectCommandThenConfig([rootOwnersRead, teamRead, labelsRead, labelsPost]) + }) + + it('on an issue, a commenter listed directly next to a team is authorized without reading the team', async () => { + routeRootOwners('reviewers:\n- codertocat\n- \'@codertocat/reviewers\'\n') + routeLabelWrite() + + const result = await lgtm(comment('/lgtm', 'some-author')) + + expect(result.status, result.stdout).toBe(0) + expect(result.errors).toEqual([]) + expect(gh.requestsMatching('GET', /\/teams\//)).toEqual([]) + expectCommandThenConfig([rootOwnersRead, labelsRead, labelsPost]) + }) + + it('a team the token cannot read admits nobody: refused with one warning', async () => { + routeRootOwners('reviewers:\n- \'@codertocat/reviewers\'\n') + gh.route('GET', '/orgs/Codertocat/teams/reviewers/members', { status: 403, body: { message: 'Resource not accessible by integration' } }) + gh.route('POST', `${repo}/issues/1/comments`, { status: 201, body: {} }) + + const result = await lgtm(comment('/lgtm', 'some-author')) + + expect(result.status, result.stdout).toBe(1) + expect(result.errors.some(e => e.includes('Codertocat is not included in the reviewers role in the OWNERS file'))).toBe(true) + expect(gh.requestsMatching('POST', /\/issues\/1\/labels$/)).toEqual([]) + expect(warnings(result.stdout)).toHaveLength(1) + expect(warnings(result.stdout)[0]).toMatch(/^::warning::OWNERS: could not read the members of @codertocat\/reviewers: /) + expectCommandThenConfig([rootOwnersRead, teamRead, commentPost]) + }) +}) diff --git a/__tests__/utils/owners.test.ts b/__tests__/utils/owners.test.ts index addc31d9..c3c1a241 100644 --- a/__tests__/utils/owners.test.ts +++ b/__tests__/utils/owners.test.ts @@ -1,11 +1,14 @@ import * as core from '@actions/core' -import { describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { effectiveOwners, expandGroups, + expandOwnersSet, ownersDir, + ownersInclude, parseOwners, + resetOwnersCaches, } from '../../src/utils/owners' describe('parseOwners', () => { @@ -208,10 +211,98 @@ describe('expandGroups', () => { expect([...result].sort()).toEqual(['alice', 'bob', 'carol']) }) + beforeEach(() => resetOwnersCaches()) + it('ignores groups of another org and groups it cannot read', async () => { const result = await expandGroups(client({}), context, ['alice', '@other/team', '@myorg/missing']) expect([...result]).toEqual(['alice']) expect(core.warning).toHaveBeenCalledTimes(2) }) + + it('drops an @ entry that is not @org/team-slug, with a warning', async () => { + const result = await expandGroups(client({}), context, ['alice', '@bob', '@myorg/a/b']) + + expect([...result]).toEqual(['alice']) + expect(core.warning).toHaveBeenCalledTimes(2) + expect(core.warning).toHaveBeenCalledWith('OWNERS: ignoring @bob: a group is written @/') + }) + + it('warns once per entry per run, however many files list it', async () => { + const octokit = client({}) + await expandGroups(octokit, context, ['@other/team', '@myorg/missing']) + await expandGroups(octokit, context, ['@other/team', '@myorg/missing']) + await expandGroups(octokit, context, ['@other/team']) + + expect(core.warning).toHaveBeenCalledTimes(2) + expect((octokit as { paginate: ReturnType }).paginate).toHaveBeenCalledTimes(1) + }) + + it('reads each team once per run', async () => { + const octokit = client({ maintainers: ['bob'] }) + const [a, b] = await Promise.all([ + expandGroups(octokit, context, ['@myorg/maintainers']), + expandGroups(octokit, context, ['@myorg/maintainers', 'alice']), + ]) + + expect([...a]).toEqual(['bob']) + expect([...b].sort()).toEqual(['alice', 'bob']) + expect((octokit as { paginate: ReturnType }).paginate).toHaveBeenCalledTimes(1) + }) +}) + +describe('expandOwnersSet', () => { + const context = { repo: { owner: 'myorg', repo: 'r' } } as never + const octokit = { + paginate: vi.fn(async () => [{ login: 'Bob' }]), + teams: { listMembersInOrg: vi.fn() }, + } as never + + it('expands both roles and keeps labels and sources', async () => { + const set = await expandOwnersSet(octokit, context, { + approvers: new Set(['@myorg/maintainers']), + reviewers: new Set(['alice', '@myorg/maintainers']), + labels: new Set(['area/sdk']), + sources: ['sdk/OWNERS'], + }) + + expect(set).toEqual({ + approvers: new Set(['bob']), + reviewers: new Set(['alice', 'bob']), + labels: new Set(['area/sdk']), + sources: ['sdk/OWNERS'], + }) + }) + + it('passes undefined through', async () => { + expect(await expandOwnersSet(octokit, context, undefined)).toBeUndefined() + }) +}) + +describe('ownersInclude', () => { + const context = { repo: { owner: 'myorg', repo: 'r' } } as never + + function client(members: string[]) { + return { + paginate: vi.fn(async () => members.map(login => ({ login }))), + teams: { listMembersInOrg: vi.fn() }, + } + } + + beforeEach(() => resetOwnersCaches()) + + it('answers a direct listing without reading any team', async () => { + const octokit = client(['bob']) + + expect(await ownersInclude(octokit as never, context, ['alice', '@myorg/maintainers'], 'alice')).toBe(true) + expect(octokit.paginate).not.toHaveBeenCalled() + }) + + it('reads the teams when the login is not listed directly', async () => { + const octokit = client(['bob']) + + expect(await ownersInclude(octokit as never, context, ['alice', '@myorg/maintainers'], 'bob')).toBe(true) + expect(await ownersInclude(octokit as never, context, ['alice', '@myorg/maintainers'], 'carol')).toBe(false) + expect(octokit.paginate).toHaveBeenCalledTimes(1) + }) }) diff --git a/dist/index.js b/dist/index.js index 03d0ae26..c75804cc 100644 --- a/dist/index.js +++ b/dist/index.js @@ -37192,7 +37192,7 @@ function getOctokit(token, options, ...additionalPlugins) { // EXTERNAL MODULE: external "node:buffer" var external_node_buffer_ = __nccwpck_require__(4573); ;// CONCATENATED MODULE: ./node_modules/js-yaml/dist/js-yaml.mjs -/*! js-yaml 5.4.3 https://github.com/nodeca/js-yaml @license MIT */ +/*! js-yaml 5.4.2 https://github.com/nodeca/js-yaml @license MIT */ //#region src/tag.ts /** * Returned by a scalar resolver when the source does not match its tag. @@ -39327,7 +39327,7 @@ function readBlockScalar(state, parentIndent, props) { state.position = linePosition + column; throwError(state, "tab characters must not be used in indentation"); } - if (column >= parentIndent && column < maxLeadingIndent) { + if (column < maxLeadingIndent) { state.position = linePosition + column; throwError(state, "bad indentation of a mapping entry"); } @@ -39537,16 +39537,13 @@ function readBlockMapping(state, nodeIndent, flowIndent, props) { ch = state.input.charCodeAt(++state.position); if (!isWsOrEolOrEnd(ch)) throwError(state, "a whitespace character is expected after the key-value separator within a block mapping"); if (!mappingOpened) { - state.events.splice(beforeKey.eventsLength, 0, { - type: EVENT_ID.MAPPING, - start: beforeKey.position, - anchorStart: props.anchorStart, - anchorEnd: props.anchorEnd, - tagStart: props.tagStart, - tagEnd: props.tagEnd, - style: COLLECTION_STYLE.BLOCK - }); + restoreState(state, beforeKey); + addMappingEvent(state, beforeKey.position, props.anchorStart, props.anchorEnd, props.tagStart, props.tagEnd, COLLECTION_STYLE.BLOCK); mappingOpened = true; + parseNode(state, flowIndent, CONTEXT_FLOW_OUT, false, true); + ch = state.input.charCodeAt(state.position); + while (isWhiteSpace(ch)) ch = state.input.charCodeAt(++state.position); + state.position++; } detected = true; atExplicitKey = false; @@ -41881,10 +41878,19 @@ function parseGroup(entry) { return match === null ? undefined : { org: match[1], slug: match[2] }; } const groupCache = new Map(); +// entries already warned about: the same OWNERS entry is expanded once per changed file per role +const warnedEntries = new Set(); +function warnOnce(entry, message) { + if (!warnedEntries.has(entry)) { + warnedEntries.add(entry); + warning(message); + } +} /** * Replace the `@org/team` entries of a list of OWNERS entries with the logins of * the team's members (transitive, as the teams API returns them). A group of - * another organization, or one that cannot be read, contributes nobody. + * another organization, one that cannot be read, or a malformed `@…` entry + * contributes nobody and is warned about once per run. * * @param octokit - a hydrated github client * @param context - the github actions event context @@ -41896,14 +41902,19 @@ async function expandGroups(octokit, context, entries) { const groups = new Set(); for (const entry of entries) { const group = parseGroup(entry); - if (group === undefined) { - result.add(entry); + if (group !== undefined) { + if (group.org === owner) { + groups.add(group.slug); + } + else { + warnOnce(entry, `OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`); + } } - else if (group.org !== owner) { - warning(`OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`); + else if (entry.startsWith('@')) { + warnOnce(entry, `OWNERS: ignoring ${entry}: a group is written @/`); } else { - groups.add(group.slug); + result.add(entry); } } const members = await Promise.all([...groups].map(slug => memoized(groupCache, `${owner}/${slug}`, async () => { @@ -41923,6 +41934,22 @@ async function expandGroups(octokit, context, entries) { members.flat().forEach(login => result.add(login)); return result; } +/** + * Whether a list of OWNERS entries names a login, directly or through one of + * its groups. A direct listing answers without reading any team. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param entries - lowercased OWNERS entries + * @param login - a lowercased login + */ +async function ownersInclude(octokit, context, entries, login) { + const list = [...entries]; + if (list.includes(login)) { + return true; + } + return (await expandGroups(octokit, context, list)).has(login); +} /** an OwnersSet whose approvers and reviewers hold logins only */ async function expandOwnersSet(octokit, context, set) { if (set === undefined) { @@ -42100,6 +42127,7 @@ function resetOwnersCaches() { hasOwnersCache.clear(); treeCache.clear(); groupCache.clear(); + warnedEntries.clear(); } function memoized(cache, key, compute) { let pending = cache.get(key); @@ -42485,7 +42513,7 @@ async function assertRootOwner(octokit, context, role, username) { return false; } const owners = parseOwners('OWNERS', contents); - if (!(await expandGroups(octokit, context, owners[role])).has(username.toLowerCase())) { + if (!(await ownersInclude(octokit, context, owners[role], username.toLowerCase()))) { throw new Error(`${username} is not included in the ${role} role in the OWNERS file`); } return true; @@ -42530,7 +42558,7 @@ async function rootOwnersIncludes(octokit, context, user) { } const owners = parseOwners('OWNERS', contents); const login = user.toLowerCase(); - return (await expandGroups(octokit, context, [...owners.reviewers, ...owners.approvers])).has(login); + return ownersInclude(octokit, context, [...owners.reviewers, ...owners.approvers], login); } /** * Retrieve the contents of the OWNERS file at the root of the repository. diff --git a/docs/commands.md b/docs/commands.md index 49e96281..db500324 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -191,7 +191,9 @@ approvers: - '@myorg/maintainers' ``` -The team's members (child teams included, as the teams API returns them) are looked up once per team per run and treated as if listed individually. A team of another organization is ignored with a warning, and so is a team the token cannot read: it needs `members: read` (a token that can read org teams). Groups are matched when OWNERS are resolved, so changes to the team apply on the next event. +The team's members (child teams included, as the teams API returns them) are looked up once per team per run and treated as if listed individually. A team of another organization is ignored, and so is an `@…` entry that is not `@/`; both are warned about once per run. Groups are matched when OWNERS are resolved, so changes to the team apply on the next event. + +Reading a team's members needs a token with the organization's *Members: read* permission (classic PAT: `read:org`; GitHub App: *Organization permissions → Members: read*). **The workflow's default `GITHUB_TOKEN` does not have it**: with that token every group warns `could not read the members of @org/team` and contributes nobody, so a user listed only through a group is refused. Pass such a token via the `token` secret ([installing](./installing.md#inputs-and-secrets)). A login listed directly is matched without reading any team. ### Which files decide the outcome diff --git a/docs/installing.md b/docs/installing.md index a0a15f0b..a28eb374 100644 --- a/docs/installing.md +++ b/docs/installing.md @@ -185,7 +185,7 @@ Input | Default | Meaning Secret | Default | Meaning --- | --- | --- -`token` | the caller's `github.token` | A token that can read a private `/.project`, a bot user's token so merges trigger `push` workflows ([events](./events.md#check_suite-and-status)), or one with the `workflows` scope so fork pull requests whose merge involves workflow files can be merged ([automatic merging](./automatic-merging.md#fork-pull-requests-and-workflow-files)). +`token` | the caller's `github.token` | A token that can read a private `/.project`, a bot user's token so merges trigger `push` workflows ([events](./events.md#check_suite-and-status)), one with the `workflows` scope so fork pull requests whose merge involves workflow files can be merged ([automatic merging](./automatic-merging.md#fork-pull-requests-and-workflow-files)), or one with the organization's *Members: read* permission so OWNERS files may list teams ([commands](./commands.md#github-groups)). `cat-api-key` | — | The [thecatapi.com](https://thecatapi.com) key for [`/meow`](./commands.md#enabling-meow). ```yaml diff --git a/docs/prow-for-your-org.md b/docs/prow-for-your-org.md index fa37cfe1..ed3ec55d 100644 --- a/docs/prow-for-your-org.md +++ b/docs/prow-for-your-org.md @@ -188,7 +188,10 @@ options: Those are all the keys supported: `approvers`, `reviewers`, `labels`, `options.no_parent_owners`. `OWNERS_ALIASES`, `filters` and `emeritus_approvers` are not -supported. [commands](./commands.md#owners) has the full format and the failure modes. +supported; in place of aliases, `approvers` and `reviewers` may list a GitHub team of your own +organization as `'@/'`, which needs a `token` that can read the organization's +members ([commands](./commands.md#github-groups)). [commands](./commands.md#owners) has the full +format and the failure modes. What changes the moment a pull request's base branch (your default branch, for most) carries any OWNERS file, open pull requests included: @@ -231,6 +234,7 @@ lines. | Branch protection or rulesets require reviews/checks the `GITHUB_TOKEN` cannot satisfy | `mergeable_state` is `blocked`; the event path skips the merge and logs the state | Lower the requirement, or let a human's review satisfy it ([automatic merging](./automatic-merging.md#event-driven-merging)) | | A merge made with `GITHUB_TOKEN` fires no `push` workflows | post-merge automation does not run | pass a PAT or GitHub App token via the `token` secret ([installing](./installing.md#inputs-and-secrets)) | | `/.project` is private | a consumer's `GITHUB_TOKEN` cannot read it; the org tier silently falls through to `/.github` | keep the org config in `.github`, or pass a `token` that can read `.project` ([installing](./installing.md#the-project-tier)) | +| An OWNERS file lists a team (`'@org/team'`) and its members are refused | `GITHUB_TOKEN` has no organization permissions, so the team cannot be read: one `could not read the members of @org/team` warning, and the group contributes nobody ([commands](./commands.md#github-groups)) | pass a `token` with the organization's *Members: read* permission (PAT `read:org`), or list the logins directly | | Fork PRs | the template uses `pull_request_target`, so forks get labels and merges; safe because nothing checks out PR code ([events](./events.md#pull_request_target-and-the-reusable-workflow)) | nothing | | A fork PR passes the gate but will not merge: `403 Resource not accessible by integration` | the merge involves workflow files the fork's branch lacks (or changes); `GITHUB_TOKEN` cannot get the `workflows` permission. The bot comments once and skips the PR ([fork pull requests and workflow files](./automatic-merging.md#fork-pull-requests-and-workflow-files)) | the author rebases onto the base branch; or pass a `token` with the `workflows` scope | | zizmor (`dangerous-triggers`) or a hash-pin org policy rejects `pull_request_target` | `pull_request` gives fork PRs a read-only token | use the [`pull_request` template](./installing.md#without-pull_request_target): fork PRs are handled by the scheduled `sweep` within minutes, comments stay instant | diff --git a/src/utils/auth.ts b/src/utils/auth.ts index f8e37f83..f5d5d1f9 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -7,7 +7,7 @@ import { Buffer } from 'node:buffer' import * as core from '@actions/core' import { defaultAuthorization, loadProwConfig, resolveAuthorization } from './config' -import { expandGroups, parseOwners } from './owners' +import { ownersInclude, parseOwners } from './owners' import { loadPullRequestOwners } from './pullRequestOwners' function getErrorDetails(error: unknown): { status: unknown, message: string } { @@ -356,7 +356,7 @@ async function assertRootOwner( } const owners = parseOwners('OWNERS', contents) - if (!(await expandGroups(octokit, context, owners[role])).has(username.toLowerCase())) { + if (!(await ownersInclude(octokit, context, owners[role], username.toLowerCase()))) { throw new Error( `${username} is not included in the ${role} role in the OWNERS file`, ) @@ -420,7 +420,7 @@ async function rootOwnersIncludes( const owners = parseOwners('OWNERS', contents) const login = user.toLowerCase() - return (await expandGroups(octokit, context, [...owners.reviewers, ...owners.approvers])).has(login) + return ownersInclude(octokit, context, [...owners.reviewers, ...owners.approvers], login) } /** diff --git a/src/utils/owners.ts b/src/utils/owners.ts index 6428b2d0..35a3ad19 100644 --- a/src/utils/owners.ts +++ b/src/utils/owners.ts @@ -101,11 +101,21 @@ export function parseGroup(entry: string): { org: string, slug: string } | undef } const groupCache = new Map>() +// entries already warned about: the same OWNERS entry is expanded once per changed file per role +const warnedEntries = new Set() + +function warnOnce(entry: string, message: string): void { + if (!warnedEntries.has(entry)) { + warnedEntries.add(entry) + core.warning(message) + } +} /** * Replace the `@org/team` entries of a list of OWNERS entries with the logins of * the team's members (transitive, as the teams API returns them). A group of - * another organization, or one that cannot be read, contributes nobody. + * another organization, one that cannot be read, or a malformed `@…` entry + * contributes nobody and is warned about once per run. * * @param octokit - a hydrated github client * @param context - the github actions event context @@ -121,14 +131,19 @@ export async function expandGroups( const groups = new Set() for (const entry of entries) { const group = parseGroup(entry) - if (group === undefined) { - result.add(entry) + if (group !== undefined) { + if (group.org === owner) { + groups.add(group.slug) + } + else { + warnOnce(entry, `OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`) + } } - else if (group.org !== owner) { - core.warning(`OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`) + else if (entry.startsWith('@')) { + warnOnce(entry, `OWNERS: ignoring ${entry}: a group is written @/`) } else { - groups.add(group.slug) + result.add(entry) } } @@ -151,6 +166,28 @@ export async function expandGroups( return result } +/** + * Whether a list of OWNERS entries names a login, directly or through one of + * its groups. A direct listing answers without reading any team. + * + * @param octokit - a hydrated github client + * @param context - the github actions event context + * @param entries - lowercased OWNERS entries + * @param login - a lowercased login + */ +export async function ownersInclude( + octokit: Octokit, + context: Context, + entries: Iterable, + login: string, +): Promise { + const list = [...entries] + if (list.includes(login)) { + return true + } + return (await expandGroups(octokit, context, list)).has(login) +} + /** an OwnersSet whose approvers and reviewers hold logins only */ export async function expandOwnersSet( octokit: Octokit, @@ -380,6 +417,7 @@ export function resetOwnersCaches(): void { hasOwnersCache.clear() treeCache.clear() groupCache.clear() + warnedEntries.clear() } function memoized(cache: Map>, key: string, compute: () => Promise): Promise { From 9a6475c13a10183f0f358bd23d0f919397d7f77a Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Thu, 15 Oct 2026 10:13:57 -0500 Subject: [PATCH 3/3] chore: repack dist/ against the locked js-yaml 5.4.3 The previous repack came from a node_modules that still held js-yaml 5.4.2; CI's fresh npm ci bundles 5.4.3 (#440), so the drift gate failed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- dist/index.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/dist/index.js b/dist/index.js index c75804cc..67156fa9 100644 --- a/dist/index.js +++ b/dist/index.js @@ -37192,7 +37192,7 @@ function getOctokit(token, options, ...additionalPlugins) { // EXTERNAL MODULE: external "node:buffer" var external_node_buffer_ = __nccwpck_require__(4573); ;// CONCATENATED MODULE: ./node_modules/js-yaml/dist/js-yaml.mjs -/*! js-yaml 5.4.2 https://github.com/nodeca/js-yaml @license MIT */ +/*! js-yaml 5.4.3 https://github.com/nodeca/js-yaml @license MIT */ //#region src/tag.ts /** * Returned by a scalar resolver when the source does not match its tag. @@ -39327,7 +39327,7 @@ function readBlockScalar(state, parentIndent, props) { state.position = linePosition + column; throwError(state, "tab characters must not be used in indentation"); } - if (column < maxLeadingIndent) { + if (column >= parentIndent && column < maxLeadingIndent) { state.position = linePosition + column; throwError(state, "bad indentation of a mapping entry"); } @@ -39537,13 +39537,16 @@ function readBlockMapping(state, nodeIndent, flowIndent, props) { ch = state.input.charCodeAt(++state.position); if (!isWsOrEolOrEnd(ch)) throwError(state, "a whitespace character is expected after the key-value separator within a block mapping"); if (!mappingOpened) { - restoreState(state, beforeKey); - addMappingEvent(state, beforeKey.position, props.anchorStart, props.anchorEnd, props.tagStart, props.tagEnd, COLLECTION_STYLE.BLOCK); + state.events.splice(beforeKey.eventsLength, 0, { + type: EVENT_ID.MAPPING, + start: beforeKey.position, + anchorStart: props.anchorStart, + anchorEnd: props.anchorEnd, + tagStart: props.tagStart, + tagEnd: props.tagEnd, + style: COLLECTION_STYLE.BLOCK + }); mappingOpened = true; - parseNode(state, flowIndent, CONTEXT_FLOW_OUT, false, true); - ch = state.input.charCodeAt(state.position); - while (isWhiteSpace(ch)) ch = state.input.charCodeAt(++state.position); - state.position++; } detected = true; atExplicitKey = false;