multi-approvers.yml requires revealing concealed organization members

[meltsufin]

The members.json configuration for multi-approvers.yml workflow requires all organization members to be made public.
This is not desirable if there are good reasons for certain members to be private. On the other hand, if the workflow is meant to be use only with public organization members, then it should be able to fetch that list without any credentials using https://api.github.com/orgs/ORG/members endpoint and not require members.json.

[ian-shafer]

This is a known issue with this workflow that we plan on addressing in the near future. FYI, we would have started with simply accessing the orgs REST API, but the token granted to workflows does not allow read access to that part of the REST API.

[mattrandallbecker]

I think this is our reasonable first pass as it allows someone to curate the configuration list down to only those that should not require two reviews when making a PR. You can remove any names from it you don't want to be public when creating the configuration, you could even remove Googlers at the cost of making their PRs require two approvals.

In the future we'll create a new workflow that only takes a team name as input, and uses this API to validate the author is within that team. That will obfuscate who is actually in the team, although one could figure it out by seeing which PRs only require one approval. That workflow however will need a PAT token, and so rolling that out will be more complex.

[meltsufin]

It's good to know that there are plans to make this easier in the future. For now, I think we'll curate the list manually. Since we have many repositories in our organization, an improvement could be the ability to read the members.json from another repository from within the GitHub org.

[suztomo]

@ian-shafer Would you elaborate more about why multi-approvers.yml needs organization membership? The GitHub repository permissions in my team are managed in repository-level by teams, not by organization membership. The people with the "Write" or "Maintain" role carry the risk of leveraging "sock puppet" account.

For example, even though I belong to the googleapis organization, I cannot merge a pull request in https://github.com/googleapis/nodejs-bigtable, which I don't have the roles to merge a pull request by myself.

[meltsufin]

We should probably disable non-members of the organization from being added as collaborators to any of the repos.

[ian-shafer]

Would you elaborate more about why multi-approvers.yml needs organization membership?

@suztomo I was probably wrong about that. Based on feedback (including yours) the next step for this workflow is to add support to query team membership at runtime. We could also do this for orgs (as I suggested above), but that does not sound as useful.

The good news, is that this v0 version can support that, if you don't mind checking in a JSON list of your team.

[suztomo]

Thank you. I'm looking forward to the update.

FYI1: There's a field in API response that explains the relationship between the author and the repository. Such as maintainer, collaborator, admin, etc. See "author_association": "COLLABORATOR" in https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28

Additionally, you may want to ensure the multi-approvers.yml design works with the CODEOWNERS file, in addition to repository-level permission. As I haven't seen how multi-approvers.yml works, I'm not saying current design is not working with it.

[ian-shafer]

Thanks for the ideas, @suztomo. Can you share more about the author_association field and how we could use it?

Also, we don't currently take into account the CODEOWNERS files. What are you thinking here?

[suztomo]

Sorry, my bad. Please forget about the FYI items. I think it's better for me to learn how multi-approvers.yml works first (Mike is setting up it in one of our repositories) before giving ideas.

[suztomo]

I think it's better for me to learn how multi-approvers.yml works first

Memo: (I think I now I know what multi-approvers check does.) I thought that the "author_association" fields might work to determine the followings:

• author_association in the pull request can determine whether the pull request is coming from outside the maintainers.

  ~/Documents/hitomemo $ gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/abcxyz/pkg/pulls/396        
  {
    "url": "https://api.github.com/repos/abcxyz/pkg/pulls/396",
  ...
    "author_association": "CONTRIBUTOR",
  

• The author_association in the reviews (https://docs.github.com/en/rest/pulls/reviews?apiVersion=2022-11-28#list-reviews-for-a-pull-request) can tell whether the approvers are the maintainers

  ~/Documents/hitomemo $ gh api \
    -H "Accept: application/vnd.github+json" \
    -H "X-GitHub-Api-Version: 2022-11-28" \
    /repos/googleapis/java-shared-config/pulls/984/reviews
  [
    {
      "id": 2599300289,
      "node_id": "PRR_kwDODCCUjM6a7izB",
      "user": {
        "login": "ldetmer",
  ...
    "state": "APPROVED",
  ...
    "author_association": "MEMBER",
  

It's as if counting the green checkmarks in the pull requests:

However, I learned that the value of the author_association field depends on the visibility of the member (https://github.com/orgs/community/discussions/18690). A Googler with the private organization visibility cannot be distinguished from an external contributor who has committed the code in the repository in the past. For example the author of /repos/abcxyz/pkg/pulls/396 is CONTRIBUTOR in the API output above.