Add support for GitHub teams to multi-approvers #396

ian-shafer · 2025-02-06T18:50:18Z

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:

members-url
local-members-path
team

See the README for more on each of these inputs.

Fixes #364 and #361

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.

bradegler · 2025-02-06T19:00:09Z

.github/workflows/multi-approvers.yml

+      - 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"
+              }
+            }
+


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)

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?

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

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.

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).

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).

sethvargo · 2025-02-07T01:14:59Z

.github/workflows/multi-approvers.js

-      }
+/** Implements approvedCount using the contains method. */
+class AbstractMembers {
+


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.

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

Please create a follow-up issue.

.github/workflows/multi-approvers.js

sethvargo · 2025-02-07T01:17:06Z

.github/workflows/multi-approvers.js

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


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.

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

Please create a follow-up issue.

sethvargo · 2025-02-07T01:18:14Z

.github/workflows/multi-approvers.js

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


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

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

Please create a follow-up issue.

sethvargo · 2025-02-07T01:19:08Z

.github/workflows/multi-approvers.js

+      const teamOrg = orgTeam.split('/')[0];
+      const teamSlug = orgTeam.split('/')[1];
+      return new TeamMembers(teamOrg, teamSlug, github);


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

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

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.

sethvargo · 2025-02-07T01:19:24Z

.github/workflows/multi-approvers.js

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


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

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.

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

sethvargo · 2025-02-07T01:21:20Z

.github/workflows/multi-approvers.yml

-        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}.'


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

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

GH team descriptor

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.

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).

sethvargo · 2025-02-07T01:21:55Z

.github/workflows/multi-approvers.yml

+          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


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

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.

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

sethvargo · 2025-02-07T01:23:56Z

.github/workflows/multi-approvers.yml

+      - 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"
+              }
+            }
+


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

suztomo · 2025-02-09T04:39:34Z

README.md

+##### Teams
+
+If you use the `team` input, you will have to setup
+[github-token-minter](https://github.com/abcxyz/github-token-minter)  to provide


Am I right about the requirements below?

The usage of teams here needs github-token-minter in each of the repository.

github-token-minter needs Terraform (https://github.com/abcxyz/github-token-minter?tab=readme-ov-file#deploy-the-service)?

I also see this code though:

# Use the Minty token if it was generated, otherwise use the default. github-token: '${{ steps.mint-token.outputs.token || github.token }}'

Yes, use of the team input will require github-token-minter. You won't need one per repo, but you will need one somewhere. @bradegler can speak more to this.

@bradegler Does this require Google teams need to setup Terraform?

suztomo · 2025-02-09T04:41:23Z

README.md

+      if: |-
+        # Check that the request is coming from the expected repository.
+        # The ID should be replaced with yours.
+        assertion.repository_id == '12345678' &&


Can we use repository name, such as "google-cloud-java" for https://github.com/googleapis/google-cloud-java? (I've never referenced a GitHub repository with a number. Is there a benefit in this?)

Repository names are mutable and subject to name squatting attacks. Using the repository id is the correct way to ensure the request is coming from the repository that it is expected to come from.

Great. That's worth mentioning here.

suztomo · 2025-02-09T04:43:08Z

README.md

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


Can we specify multiple teams? In the repositories we (Cloud SDK) maintain, it's norm to have the service team and the Cloud SDK team maintain a repository together (e.g., java-storage)

@suztomo We really just need one team of all Googlers who are members of the organization. More granular approvals can be achieved with a CODEOWNERS file where you can specify multiple teams.

Yes, one organization team, if it exists, suffices the needs.

Would it make sense to allow for an org input where internal approvers are members of the org instead of a team?

Yes, organization membership makes sense to me. It can tell the organization already for the repository URL.

sakthivelmanii · 2025-02-10T10:31:56Z

.github/workflows/multi-approvers.yml

+      local-members-path:
+        description: 'Path to JSON members file. See README for more.'
+        type: 'string'
+      team:


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

@meltsufin @suztomo

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

@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.

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?

ian-shafer · 2025-02-10T16:26:27Z

FYI, I will eventually close this PR in favor of another where we use an Action instead of a Workflow for multi-approvers logic.

sethvargo · 2025-02-10T23:33:47Z

.github/workflows/multi-approvers.js

-      }
+/** Implements approvedCount using the contains method. */
+class AbstractMembers {
+


Please create a follow-up issue.

sethvargo · 2025-02-10T23:33:56Z

.github/workflows/multi-approvers.js

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


Please create a follow-up issue.

sethvargo · 2025-02-10T23:34:06Z

.github/workflows/multi-approvers.js

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


Please create a follow-up issue.

sethvargo · 2025-02-10T23:34:34Z

.github/workflows/multi-approvers.js

+      const teamOrg = orgTeam.split('/')[0];
+      const teamSlug = orgTeam.split('/')[1];
+      return new TeamMembers(teamOrg, teamSlug, github);


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.

sethvargo · 2025-02-10T23:35:20Z

.github/workflows/multi-approvers.js

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


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

sethvargo · 2025-02-10T23:36:12Z

.github/workflows/multi-approvers.yml

+      local-members-path:
+        description: 'Path to JSON members file. See README for more.'
+        type: 'string'
+      team:


@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.

sethvargo · 2025-02-10T23:38:48Z

.github/workflows/multi-approvers.yml

-        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}.'


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).

sethvargo · 2025-02-10T23:39:31Z

.github/workflows/multi-approvers.yml

+          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


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

ian-shafer requested review from a team as code owners February 6, 2025 18:50

ian-shafer requested review from chloechien, pjh and mikelalcon February 6, 2025 18:50

Fix comment

64300d5

bradegler requested changes Feb 6, 2025

View reviewed changes

sethvargo reviewed Feb 7, 2025

View reviewed changes

suztomo reviewed Feb 9, 2025

View reviewed changes

suztomo mentioned this pull request Feb 10, 2025

multi-approvers.yml requires revealing concealed organization members #361

Open

sakthivelmanii reviewed Feb 10, 2025

View reviewed changes

sethvargo requested changes Feb 10, 2025

View reviewed changes

ian-shafer closed this Feb 11, 2025

Add support for GitHub teams to multi-approvers #396

Add support for GitHub teams to multi-approvers #396

Conversation

ian-shafer commented Feb 6, 2025 • edited Loading

Teams Support

New Inputs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suztomo Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suztomo Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shafer commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shafer commented Feb 6, 2025 •

edited

Loading

suztomo Feb 9, 2025 •

edited

Loading

suztomo Feb 10, 2025 •

edited

Loading