Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for GitHub teams to multi-approvers #396

Closed
wants to merge 2 commits into from
Closed

Add support for GitHub teams to multi-approvers #396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7530074
Add support for GitHub teams to multi-approvers
ian-shafer Feb 6, 2025
64300d5
Fix comment
ian-shafer Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 109 additions & 38 deletions .github/workflows/multi-approvers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't really write a lot of js so we don't have a linter yet, but there shouldn't be an extra newline here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will address this in a different PR where I will use an Action instead of a Workflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a follow-up issue.

/** 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;
Comment on lines +11 to +39
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super weird and would be a lot clearer with a single for loop using the quick continue pattern.

I also don't really understand why we're putting things into a map, then returning the value? It seems like a lot of unnecessary filtering and iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will address this in a different PR where I will use an Action instead of a Workflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a follow-up issue.

}
}

// 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;
}
Comment on lines +82 to +91
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really log the error (this.core.debug) or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will address this in a different PR where I will use an Action instead of a Workflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a follow-up issue.

}
}

/** 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);
Comment on lines +102 to +104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double split is inefficient. This will also potentially be undefined if the orgTeam doesn't contain a slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will address this in a different PR where I will use an Action instead of a Workflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is a bug that will cause a runtime panic and/or unexpected behavior. It needs to be fixed prior to merging. We do not merge known-bugs into HEAD.

}
if (membersPath) {
return new JsonMembers(membersPath);
}
throw new Error('Neither orgTeam nor membersPath is set.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if both are provided? I would expect that to be an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the README I said that at least one had to be set and that if multiple are set then an undefined (yet consistent) input would be chosen.

But I agree with you. It'll be less error-prone to only allow one.

I'll address this in a different PR where I will use an Action instead of a Workflow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a follow-up issue. We need exhaustive behavior with unit tests to verify the expected behavior.

})();

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;
}

Expand All @@ -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.`);
}
}

Expand All @@ -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();

Expand Down
98 changes: 90 additions & 8 deletions .github/workflows/multi-approvers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,16 @@ on:
# 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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to configure organization as well? We would like to configure this GitHub action at the organization level.

@meltsufin @suztomo

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I proposed this above. I'll chat with my team to see if this is a possibility

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sakthivelmanii what is the use case? You cannot assign CODEOWNERS teams outside of the organization, so I don't understand the use case for why you'd want to query teams outside of the org.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @sakthivelmanii is asking for all members of a GH org (/orgs/{org}/members) to be treated as internal reviewers. @sakthivelmanii is this correct?

description: 'The GitHub org and team slug that should be used for determining external vs. internal users. Format is ${org}/${team-slug}.'
Comment on lines -24 to +33
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we really need to simplify these inputs. Why do we need so many?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need at least two to support GH teams and a members JSON file.

  1. GH team descriptor
  2. JSON URL

For the JSON URL, it's nice to allow a path to the local repo for manual testing / development / maintenance. This isn't required, but it alleviates the need to find a public place to put a JSON file when testing in a private repo like abcxyz/actions-testsing.

Also note, the JSON members files is nice because it does not require a github-token-minter (Minty) setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to allow specifying a members JSON at all? That seems useful for local testing, but should never be used in a real scenario. We would never actually want someone manually managing the list of internal members in the organization, since that represents a substantial risk of being out of sync and creating insider risk.

I am strongly opposed to complicating a public API surface just for testing. These kinds of things should be unit tests anywhere (where we fake the response from the GitHub API).

type: 'string'
multi-approvers-js-url:
description: 'The URL to multi-approvers.js. This should typically not need to be set.'
Expand All @@ -30,8 +39,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:
Expand All @@ -40,8 +54,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
Comment on lines +60 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't access inputs directly in a script (it's a shell injection vuln). Set inputs as envvars and access the envvars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Moving this to an Action from a Workflow means that I'll test inputs in the JS instead of a Workflow step so this should be moot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix before merging, regardless of the future. We cannot have known security exploits sitting in code at HEAD.

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"

Expand All @@ -58,6 +85,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: |-
Expand All @@ -76,19 +123,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"
}
}

Comment on lines +126 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be done from the calling workflow. The minty instance referenced from the variables specified here is the abcxyz instance and a caller from another org will have no way to change that, rendering this somewhat unusable.

The current WIF pool is deployed with an assertion that prevents requests from all other orgs which would block your mint token request as well.

Usage of this workflow should look something like:

-> auth
-> minty
-> multi-approvers (passing minty token in)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time digging into this. We're not currently able to pass secrets from one workflow job to another. Some related facts:

The minty token is marked as a secret here.

This is actually supported (see Example: Masking and passing a secret between jobs or workflows), but it's very cumbersome and probably more than we want to require for this. It requires setting up a secrets store like Vault.

We've only used the Minty steps and the generated token within the same job. I was trying to do it across jobs because the multi-approvers workflow is a reusable workflow and therefore it needs to be called in it's own separate job (see docs).

We could look into turning the multi-approvers reusable workflow into an action or multiple actions. I'm not sure if this would work.

Would it be possible to just pass the required values for the two minty actions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller of the workflow needs to setup/configure Minty and pass in the resulting GITHUB_TOKEN to this action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Brad and I talked and I'm going to turn the multi-approvers workflow into an action. This will allow the Minty actions and multi-approvers to run in the same job and removes the need to pass a secret across jobs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really prefer to do that ~now before we have people start depending on the workflow version. I also think, eventually, we'll want to move this into it's own repo like we did with terraform-linter (but that can be a follow-up).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry I didn't make that clear. This PR will not be merged. I'll close this PR now to make it clear.

RE moving this into its own repo. Why not do that now? I'm not sure how much work it is to setup a new repo, but I'd prefer to do it now to avoid migration costs in the future (all clients will have to migrate to use the new action).

- 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,
Expand All @@ -104,7 +187,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 }}'
Expand All @@ -117,5 +200,4 @@ jobs:
repoOwner,
workflowRef,
github,
core,
});
Loading
Loading