feat(owners): support GitHub groups in OWNERS

__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<string, unknown>) {
+    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 @<org>/<team-slug>',
+      '::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])
+  })
+})

__tests__/utils/owners.test.ts

@@ -1,10 +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', () => {
@@ -186,3 +190,119 @@ describe('effectiveOwners', () => {
     expect(effectiveOwners('README.md', new Map())).toBeUndefined()
   })
 })
+
+describe('expandGroups', () => {
+  const context = { repo: { owner: 'MyOrg', repo: 'r' } } as never
+
+  function client(listed: Record<string, string[]>) {
+    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'])
+  })
+
+  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 @<org>/<team-slug>')
+  })
+
+  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<typeof vi.fn> }).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<typeof vi.fn> }).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)
+  })
+})

dist/index.js

@@ -41870,6 +41870,100 @@ 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();
+// 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, 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
+ * @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) {
+            if (group.org === owner) {
+                groups.add(group.slug);
+            }
+            else {
+                warnOnce(entry, `OWNERS: ignoring group ${entry}: only groups of ${owner} are supported`);
+            }
+        }
+        else if (entry.startsWith('@')) {
+            warnOnce(entry, `OWNERS: ignoring ${entry}: a group is written @<org>/<team-slug>`);
+        }
+        else {
+            result.add(entry);
+        }
+    }
+    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;
+}
+/**
+ * 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) {
+        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 +42129,8 @@ function branchHasOwners(octokit, context, branch) {
 function resetOwnersCaches() {
     hasOwnersCache.clear();
     treeCache.clear();
+    groupCache.clear();
+    warnedEntries.clear();
 }
 function memoized(cache, key, compute) {
     let pending = cache.get(key);
@@ -42138,7 +42234,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 +42516,7 @@ async function assertRootOwner(octokit, context, role, username) {
         return false;
     }
     const owners = parseOwners('OWNERS', contents);
-    if (!owners[role].includes(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;
@@ -42465,7 +42561,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 ownersInclude(octokit, context, [...owners.reviewers, ...owners.approvers], login);
 }
 /**
  * Retrieve the contents of the OWNERS file at the root of the repository.