From 7530074ae6ea9f3b1dbde365b68d34e0a78c0157 Mon Sep 17 00:00:00 2001 From: Ian Shafer Date: Thu, 6 Feb 2025 18:44:08 +0000 Subject: [PATCH 1/2] Add support for GitHub teams to multi-approvers This change also updates the allowed inputs to the multi-approvers workflow. Teams Support ======== Support for GitHub teams requires github-token-minter (Minty) to generate a token with elevated priviledges to access the team membership REST APIs. New Inputs ======== The original org-members-path has been deprecated. It will continue to be supported in the near future. Users can migrate from org-members-path by using the new members-url iput. Three inputs are supported: 1. members-url 2. local-members-path 3. team See the README for more on each of these inputs. --- .github/workflows/multi-approvers.js | 147 +++++++++++++++++++------- .github/workflows/multi-approvers.yml | 101 ++++++++++++++++-- README.md | 67 ++++++++++-- 3 files changed, 260 insertions(+), 55 deletions(-) diff --git a/.github/workflows/multi-approvers.js b/.github/workflows/multi-approvers.js index 0a0c9694..88c6d432 100644 --- a/.github/workflows/multi-approvers.js +++ b/.github/workflows/multi-approvers.js @@ -2,49 +2,120 @@ const APPROVED = 'APPROVED'; const COMMENTED = 'COMMENTED'; const MIN_APPROVED_COUNT = 2; -/** Returns the number of approvals from members in the given list. */ -function inOrgApprovedCount(members, submittedReviews, prLogin) { - const reviewStateByLogin = {}; - submittedReviews - // Remove the PR user. - .filter((r) => r.user.login !== prLogin) - // Only consider users in the org. - .filter((r) => members.has(r.user.login)) - // Sort chronologically ascending. Note that a reviewer can submit multiple reviews. - .sort((a, b) => new Date(a.submitted_at) - new Date(b.submitted_at)) - .forEach((r) => { - const reviewerLogin = r.user.login; - - // Set state if it does not exist. - if (!Object.hasOwn(reviewStateByLogin, reviewerLogin)) { - reviewStateByLogin[reviewerLogin] = r.state; - return; - } +/** Implements approvedCount using the contains method. */ +class AbstractMembers { + + /** Returns the number of approvals from members in the given list. */ + async approvedCount(submittedReviews, login) { + const reviewStateByLogin = {}; + submittedReviews + // Remove the PR user. + .filter((r) => r.user.login !== prLogin) + // Only consider users in the org. + .filter((r) => this.contains(r.user.login)) + // Sort chronologically ascending. Note that a reviewer can submit multiple reviews. + .sort((a, b) => new Date(a.submitted_at) - new Date(b.submitted_at)) + .forEach((r) => { + const reviewerLogin = r.user.login; + + // Set state if it does not exist. + if (!Object.hasOwn(reviewStateByLogin, reviewerLogin)) { + reviewStateByLogin[reviewerLogin] = r.state; + return; + } + + // Always update state if not approved. + if (reviewStateByLogin[reviewerLogin] !== APPROVED) { + reviewStateByLogin[reviewerLogin] = r.state; + return; + } + + // Do not update approved state for a comment. + if (reviewStateByLogin[reviewerLogin] === APPROVED && r.state !== COMMENTED) { + reviewStateByLogin[reviewerLogin] = r.state; + } + }) + + return Object.values(reviewStateByLogin).filter((s) => s === APPROVED).length; + } +} - // Always update state if not approved. - if (reviewStateByLogin[reviewerLogin] !== APPROVED) { - reviewStateByLogin[reviewerLogin] = r.state; - return; - } +/** Members backed by a JSON file. */ +class JsonMembers extends AbstractMembers { + members; - // Do not update approved state for a comment. - if (reviewStateByLogin[reviewerLogin] === APPROVED && r.state !== COMMENTED) { - reviewStateByLogin[reviewerLogin] = r.state; - } - }) + constructor(membersPath) { + super(); + this.members = require(membersPath).reduce((acc, v) => acc.set(v.login, v), new Map()); + } - return Object.values(reviewStateByLogin).filter((s) => s === APPROVED).length; + async contains(login) { + return this.members.has(login); + } +} + +/** Members backed by a GitHub team. */ +class TeamMembers extends AbstractMembers { + static ALLOWED_ROLES = ["maintainer", "member"]; + static ACTIVE = "active"; + + org; + teamSlug; + github; + + constructor(org, teamSlug, github) { + super(); + this.org = org; + this.teamSlug = teamSlug; + this.github = github; + } + + async contains(login) { + try { + const response = await this.github.rest.teams.getMembershipForUserInOrg({ + org: this.org, + team_slug: this.teamSlug, + username: login, + }); + return TeamMembers.ALLOWED_ROLES.indexOf(response.data.role) >= 0 && + response.data.state === TeamMembers.ACTIVE; + } catch (error) { + if (error.status === 404) { + // We can get here for two reasons: + // 1) The user is not a member + // 2) The team does not exist + // Either way, it's safe to return false here. + return false; + } + throw error; + } + } } /** Checks that approval requirements are satisfied. */ -async function onPullRequest({orgMembersPath, prNumber, repoName, repoOwner, github, core}) { - const members = require(orgMembersPath).reduce((acc, v) => acc.set(v.login, v), new Map()); +async function onPullRequest({orgTeam, membersPath, prNumber, repoName, repoOwner, github, core}) { + const members = (function() { + if (orgTeam) { + if (orgTeam.indexOf('/') <= 0) { + throw new Error(`Malformed team [${orgTeam}]. Team must be in the format \${org}/\${team-slug}.`); + } + const teamOrg = orgTeam.split('/')[0]; + const teamSlug = orgTeam.split('/')[1]; + return new TeamMembers(teamOrg, teamSlug, github); + } + if (membersPath) { + return new JsonMembers(membersPath); + } + throw new Error('Neither orgTeam nor membersPath is set.'); + })(); + const prResponse = await github.rest.pulls.get({owner: repoOwner, repo: repoName, pull_number: prNumber}); const prLogin = prResponse.data.user.login; - if (members.has(prLogin)) { - // Do nothing if the pull request owner is a member of the org. - core.info(`Pull request login ${prLogin} is a member of the org, therefore no special approval rules apply.`); + const isInternalPr = await members.contains(prLogin); + if (isInternalPr) { + // Do nothing if the pull request owner is an internal user. + core.info(`Pull request login ${prLogin} is an internal member, therefore no special approval rules apply.`); return; } @@ -54,12 +125,12 @@ async function onPullRequest({orgMembersPath, prNumber, repoName, repoOwner, git pull_number: prNumber, }); - const approvedCount = inOrgApprovedCount(members, submittedReviews, prLogin); + const approvedCount = await members.approvedCount(submittedReviews, prLogin); - core.info(`Found ${approvedCount} ${APPROVED} reviews.`); + core.info(`Found ${approvedCount} ${APPROVED} internal reviews.`); if (approvedCount < MIN_APPROVED_COUNT) { - core.setFailed(`This pull request has ${approvedCount} of ${MIN_APPROVED_COUNT} required approvals from members of the org.`); + core.setFailed(`This pull request has ${approvedCount} of ${MIN_APPROVED_COUNT} required internal approvals.`); } } @@ -69,7 +140,7 @@ async function onPullRequest({orgMembersPath, prNumber, repoName, repoOwner, git * This is required because GitHub treats checks made by pull_request and * pull_request_review as different status checks. */ -async function onPullRequestReview({workflowRef, repoName, repoOwner, branch, prNumber, github, core}) { +async function onPullRequestReview({workflowRef, repoName, repoOwner, branch, prNumber, github}) { // Get the filename of the workflow. const workflowFilename = workflowRef.split('@')[0].split('/').pop(); diff --git a/.github/workflows/multi-approvers.yml b/.github/workflows/multi-approvers.yml index e7d131bb..55da9e35 100644 --- a/.github/workflows/multi-approvers.yml +++ b/.github/workflows/multi-approvers.yml @@ -17,11 +17,19 @@ name: 'multi-approvers' on: workflow_call: inputs: - # Path to a JSON file containing the members of the org. This should be - # the full path that should work with the https://raw.githubusercontent.com/ prefix. + # Path to a JSON file containing the members of the org. This should be # the full path that should work with the https://raw.githubusercontent.com/ prefix. # See the README for more. org-members-path: - required: true + description: 'DEPRECATED: Use members-url with https://raw.githubusercontent.com/ prepeneded instead.' + type: 'string' + members-url: + description: 'Full URL to JSON members file. URL must be publicly accessible.' + type: 'string' + local-members-path: + description: 'Path to JSON members file. See README for more.' + type: 'string' + team: + description: 'The GitHub org and team slug that should be used for determining external vs. internal users. Format is ${org}/${team-slug}.' type: 'string' multi-approvers-js-url: description: 'The URL to multi-approvers.js. This should typically not need to be set.' @@ -30,8 +38,13 @@ on: permissions: + # Needed to re-run approver checks (see the step named "Re-run approver checks"). actions: 'write' + # Needed to support the local-members-path input. contents: 'read' + # Needed to support elevated permissions from Minty. + id-token: 'write' + # Need to read the pull request to get author and org data. pull-requests: 'read' jobs: @@ -40,8 +53,21 @@ jobs: contains(fromJSON('["pull_request", "pull_request_review"]'), github.event_name) runs-on: 'ubuntu-latest' steps: - - name: 'Download members.json' - id: 'download-members-json' + - name: 'Check inputs' + shell: 'bash' + run: |- + if [[ -z "${{ inputs.org-members-path }}" && + -z "${{ inputs.members-url }}" && + -z "${{ inputs.local-members-path }}" && + -z "${{ inputs.team }}" ]]; then + echo "Invalid inputs. One of org-members-path, members-url, local-members-path, or team must be set." 1>&2 + exit 1 + fi + + - name: 'Download members.json from org-members-path' + id: 'download-members-json-path' + if: |- + ${{ inputs.org-members-path != '' }} run: |- MEMBERS_JSON="${RUNNER_TEMP}/${GITHUB_SHA:0:7}.members.json" @@ -58,6 +84,26 @@ jobs: echo "::notice::Downloaded members.json to ${MEMBERS_JSON}" echo "output-file=${MEMBERS_JSON}" >> "${GITHUB_OUTPUT}" + - name: 'Download members.json from members-url' + id: 'download-members-json-url' + if: |- + ${{ inputs.members-url != '' }} + run: |- + MEMBERS_JSON="${RUNNER_TEMP}/${GITHUB_SHA:0:7}.members.json" + + # Download the file, passing in authentication to get a higher rate + # limit: https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions + curl "${{ inputs.members-url }}" \ + --silent \ + --fail \ + --location \ + --header "Authorization: Token ${{ github.token }}" \ + --output "${MEMBERS_JSON}" + + # Save the result to an output. + echo "::notice::Downloaded members.json to ${MEMBERS_JSON}" + echo "output-file=${MEMBERS_JSON}" >> "${GITHUB_OUTPUT}" + - name: 'Download multi-approvers.js' id: 'download-multi-approvers-js' run: |- @@ -76,19 +122,55 @@ jobs: echo "::notice::Downloaded multi-approvers.js to ${MULTI_APPROVERS_JS}" echo "output-file=${MULTI_APPROVERS_JS}" >> "${GITHUB_OUTPUT}" + - name: 'Authenticate to Google Cloud' + id: 'minty-auth' + if: |- + ${{ inputs.team != '' }} + uses: 'google-github-actions/auth@6fc4af4b145ae7821d527454aa9bd537d1f2dc5f' # ratchet:google-github-actions/auth@v2 + with: + create_credentials_file: false + export_environment_variables: false + workload_identity_provider: '${{ vars.TOKEN_MINTER_WIF_PROVIDER }}' + service_account: '${{ vars.TOKEN_MINTER_WIF_SERVICE_ACCOUNT }}' + token_format: 'id_token' + id_token_audience: '${{ vars.TOKEN_MINTER_SERVICE_AUDIENCE }}' + id_token_include_email: true + + - name: 'Mint Token' + id: 'mint-token' + if: |- + ${{ inputs.team != '' }} + uses: 'abcxyz/github-token-minter/.github/actions/minty@main' # ratchet:exclude + with: + id_token: '${{ steps.minty-auth.outputs.id_token }}' + service_url: '${{ vars.TOKEN_MINTER_SERVICE_URL }}' + requested_permissions: |- + { + "scope": "actions-testing-multi-approvers", + "repositories": ["actions-testing"], + "permissions": { + "members": "read", + "pull_requests": "read" + } + } + - name: 'Check approver requirements' uses: 'actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea' # ratchet:actions/github-script@v7 with: retries: 3 + # Use the Minty token if it was generated, otherwise use the default. + github-token: '${{ steps.mint-token.outputs.token || github.token }}' script: |- - const orgMembersPath = '${{ steps.download-members-json.outputs.output-file }}'; - // Warning: this should not be quoted, otherwise comparisons will not work in JS. + const membersPath = '${{ inputs.local-members-path || steps.download-members-json-path.outputs.output-file || steps.download-members-json-url.outputs.output-file }}'; + const orgTeam = '${{ inputs.team }}'; + // Warning: this should not be quoted to keep it as a number. const prNumber = ${{ github.event.pull_request.number }} const repoName = '${{ github.event.repository.name }}' const repoOwner = '${{ github.event.repository.owner.login }}' const {onPullRequest} = require('${{ steps.download-multi-approvers-js.outputs.output-file }}'); await onPullRequest({ - orgMembersPath, + orgTeam, + membersPath, prNumber, repoName, repoOwner, @@ -104,7 +186,7 @@ jobs: retries: 3 script: |- const branch = '${{ github.event.pull_request.head.ref }}' - // Warning: this should not be quoted, otherwise comparisons will not work in JS. + // Warning: this should not be quoted to keep it as a number. const prNumber = ${{ github.event.pull_request.number }} const repoName = '${{ github.event.repository.name }}' const repoOwner = '${{ github.event.repository.owner.login }}' @@ -117,5 +199,4 @@ jobs: repoOwner, workflowRef, github, - core, }); diff --git a/README.md b/README.md index d4d5947b..dfc744af 100644 --- a/README.md +++ b/README.md @@ -180,12 +180,15 @@ as a required status check. #### multi-approvers.yml -Use this workflow to require two in-org approvers for pull requests sent from an -out-of-org user. This prevents in-org users from creating "sock puppet" accounts -and approving their own pull requests with their in-org account. +Use this workflow to require two "internal" approvers for pull requests sent +from an "external" user. Internal users are defined by the JSON members file or +GitHub team specified in the inputs to this workflow. External users are all +users who are not internal. -This workflow requires one input: `org-members-path`. This is a JSON formatted -file with the following schema: +This workflow prevents internal users from creating "sock puppet" accounts +and approving their own pull requests with their internal account. + +If a JSON members file is supplied, it must be in the following format: ```json [ @@ -194,7 +197,7 @@ file with the following schema: ] ``` -This file can be generated using the following command: +This JSON file can be generated using the following command: ```bash gh api \ @@ -205,6 +208,55 @@ gh api \ jq '[.[] | {login}]' ``` +This generates the file from all users in a GitHub organization. The same +concept could also be used for a GitHub team. Of course, the JSON file can be +generated by any means, as long as it follow the expected format. + +##### Inputs + +One of `team`, `local-members-path`, or `members-url` must be set. If multiple +of these inputs are set, only one will be used. Which one will be used is +undefined, but deterministic. + +**team**: This is actually the org and team formateted as ${org}/${team-slug} +e.g. abcxyz/seahawks-2013. + +**local-members-path**: JSON file that exists in the repository that is calling +the workflow. + +**members-url**: Full URL to a members JSON file. If this is in a public GitHub +repository, the URL should be similar to +https://raw.githubusercontent.com/${owner}/${repo}/${ref}/path/to/members.json. + +##### Teams + +If you use the `team` input, you will have to setup +[github-token-minter](https://github.com/abcxyz/github-token-minter) to provide +a token with elevated priviledges (necessary to query a user's membership in a +team). The github-token-minter scope configuration should be as follows: + +```yaml +scope: + your-name: # Set this to whatever you want. + rule: + if: |- + # Check that the request is coming from the expected repository. + # The ID should be replaced with yours. + assertion.repository_id == '12345678' && + # Check that the request is coming from the expected workflow. + # This should be used as-is. + assertion.job_workflow_ref.startsWith("abcxyz/pkg/.github/workflows/multi-approvers.yml") + repositories: + # Set this to your repository. + - 'actions-testing' + permissions: + # These two permissions are required. + members: 'read' + pull_requests: 'read' +``` + +Here's an example of using this workflow from your repository: + ```yaml name: 'multi-approvers' @@ -226,6 +278,7 @@ on: permissions: actions: 'write' contents: 'read' + id-token: 'write' pull-requests: 'read' concurrency: @@ -236,7 +289,7 @@ jobs: multi-approvers: uses: 'abcxyz/pkg/.github/workflows/multi-approvers.yml@main' with: - org-members-path: 'abcxyz/pkg/main/.github/workflows/members.json' + members-url: 'https://raw.githubusercontent.com/abcxyz/pkg/main/path/to/members.json' ``` Note: the `org-members-path` should be the full path to the JSON file without From 64300d563b6995becc4118fdb90be0079e8124ba Mon Sep 17 00:00:00 2001 From: Ian Shafer Date: Thu, 6 Feb 2025 18:57:04 +0000 Subject: [PATCH 2/2] Fix comment --- .github/workflows/multi-approvers.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/multi-approvers.yml b/.github/workflows/multi-approvers.yml index 55da9e35..f73eb57a 100644 --- a/.github/workflows/multi-approvers.yml +++ b/.github/workflows/multi-approvers.yml @@ -17,7 +17,8 @@ name: 'multi-approvers' on: workflow_call: inputs: - # Path to a JSON file containing the members of the org. This should be # the full path that should work with the https://raw.githubusercontent.com/ prefix. + # Path to a JSON file containing the members of the org. This should be + # the full path that should work with the https://raw.githubusercontent.com/ prefix. # See the README for more. org-members-path: description: 'DEPRECATED: Use members-url with https://raw.githubusercontent.com/ prepeneded instead.'