Verify that PR creator is a member of the website-write team

[ajb176]

Fixes #3906

What changes did you make?

• Added a new workflow which makes up to three API calls to verify team membership, close the pull request, and add the requested comment (the latter two if necessary).
• The octokit membership API call returns a 404 error if the member is not found on the website-write team, in which case the catch block makes the API calls to close the PR and add the comment.
• If the PR author is found on the website-write team (the expected case), nothing will happen except a comment logged that the author was successfully verified.

Why did you make the changes (we will use this info to test)?

• To ensure only members of the website-write team can create pull requests.

Instructions for Reviewers:

• Checkout a new branch from the verify-pr-creator branch for testing, let's call this testbranch.
• Change the token in the YML file to a user-generated token with the same permissions as the HACKFORLA_ADMIN_TOKEN and save.
• From testbranch, check out a new branch called testcase1. In testcase1, make any minor change such as adding a comment to a file, and then save, commit and push the changes.
• Navigate to your remote repository and try to merge testcase1 into testbranch. The action should run. This should check whether your github handle belongs to the website-write team, should complete successfully without doing anything if it does. Navigate to the checks tab, click on Pull Request Verification, then Check Team Membership, drop down the githubscript and it should say 'Successfully verified!'
• Navigate back to testbranch and in the verify-pr.js file, on line 7 change prAuthor to a string such as 'abcdef', or any string that does not correspond to the username of a website-write team member. Save, commit, and push the changes.
• From this branch after edits, checkout a new branch called testcase2, make any change to testcase2, and then save, commit and push the changes.
• Go to your local repository and try to merge testcase2 into testbranch. The PR should automatically close and write a comment from your handle.

[github-actions]

Want to review this pull request? Take a look at this documentation for a step by step guide!

---

From your project repository, check out a new branch and test the changes.

git checkout -b ajb176-verify-pr-creator-3906 gh-pages
git pull https://github.com/ajb176/website.git verify-pr-creator-3906

[t-will-gillis]

availability:
eta: 6/16 e.o.d.

[moazDev1]

Availability: Weekdays 5PM - 9PM
ETA: 6/23 EOD

[t-will-gillis]

Hey @ajb176 Great job with this- I tested in my repo with both a my name and a non-hfla name. The PR authored by the non-member was closed and the message was posted on the issue. So everything works well.

I have some comments:

• I think that I could see this both ways, but here is the question: did you consider adding this as another job in the pull-request-trigger.yml workflow? It seems this job could belong there, but the counter-argument is that pr-instructions.yml is independent so why not this one?
• Regarding the job itself, there are other times that we need to check team membership and I believe that this should be pulled into a javascript file in the utils folder, check-team-membership.js or something. Then we can reuse this module in other workflows.
• Assuming that you do create the js file, owner could be defined in terms of context.repo.owner and repo in terms of context.repo.repo
• minor thing, but notice you are using the earlier version of the API on line 19 with await github.rest.teams.getMembershipForUserInOrg instead of the current version similar to lines 27 and 33., e.g. await github.request('GET /orgs/{org}/teams/{team_slug}/memberships/{username}
• (FYI we might(???) need to convert the API over to GraphQL in several months. But not worrying about that right now...)

Again, everything is looking good and thanks for working on this!

[ajb176]

Hey @t-will-gillis, thanks for the feedback.

• Using the pull-request-trigger.yml was my first thought, but that trigger also fires when the PR is closed, this job only needs to run once when the PR is opened. I'm under the impression that separate yml files should generally be used for different triggers, and technically a better name for the yml I created would be pull-request-target-open.yml, and it could be changed to that if there are any other jobs we want to add specifically when a PR is opened and access is needed. One thing that's confusing to me is I didn't catch any mention of tokens in the pr-instructions and wr-pr-instructions workflow, so I'm not sure how they obtain permission to post comments (I might just be missing something). Any idea about that?
• Nice catch with the API, I forgot about that. There's no good reason why I used both, I was using the docs here: https://octokit.github.io/rest.js/v20#usage and was looking for a method to close pull requests and couldn't find one. Because the docs here https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28 have the API calls specifically grouped for each type I found them a little easier to read. What I didn't notice was that you treat PRs just like issues and I should've been looking for an API call to update issues and not specifically PRs. I believe they use the same API endpoints, the rest calls are just a little more intuitive to use because they abstract out the HTTP and look like normal JS functions. I can switch the API calls to either syntax, there should be a matching one for each and they should function exactly the same way.

Regarding the job itself, there are other times that we need to check team membership and I believe that this should be pulled into a javascript file in the utils folder, check-team-membership.js or something. Then we can reuse this module in other workflows.

Do you mean the entire job, or just get membership API call? I'm not sure if I see much re-usability in the script here, beyond discretizing the API calls themselves which wouldn't make much sense. Can you expand on this a little?

[t-will-gillis]

Hey @ajb176

• About whether your workflow should be part of “Pull Request Trigger” or not: we currently have three PR related workflows and we could gather these plus your workflow into one big happy PR workflow, but I am leaning towards the argument that keeping these as separate workflows might be a better idea. But since we keep adding new workflows, I do think we need to have a standard naming convention. Naming your workflow as pr-verification.yml or similar works for me- since it aligns with pr-instructions.yml. Maybe the last one should be pr-set-labels.yml? I am thinking that it would be ideal to look at the files and see right away which workflows have the same triggers.
• About pr-instructions.yml and wr-pr-instructions.yml not having explicitly defined tokens, they are using default access permissions. I can’t claim that I know this topic inside and out, but here is an article. When you look at “Set up job” in an “Add Pull Request Instructions” workflow run, there is a default GITHUB_TOKEN and permissions.
• About the v20 versus version=2022-11-28, yeah I agree that they would function exactly the same way, but thought it would be cleaner to use the later version for all.
• About creating the JS file, I was meaning the entire job. You would still have the yml file- it would be similar to “Add-Missing-Labels-To-Issues” (f. “Issue Trigger”) in its organizational structure , but it would run a /utils/ file similar to get-team-members.js to check if the individual was on the team, that would trigger the existing utils/post-issue-comment.js when needed. Modularized.
• Let me know if you would like to discuss more…
• BTW on the “Issue Trigger” workflow we are checking team membership using someone else’s automation. It would be good to use our own checks so we aren't at the mercy of the person keeping their automation updated.

[ajb176]

• It seems like there isn't a clean way to group together jobs with different but similar triggers. All I could find is using some if statements to evaluate context variables, and using the value of those context variables to determine whether the job should run. Kind of a roundabout method to do something that should be pretty simple. It seems like the trigger is the important thing that determines whether jobs can be within the same workflow, but the triggers are all different now anyway, which is why I suggested keeping the current naming convention until more jobs with the same trigger are added, in which case we can re-name it to the trigger so people can know immediately whether a new job they're writing can fit there. I'm not sure what the overhead of having multiple yml files vs multiple jobs in a yml file is, maybe someone can look into that. If it's not really a concern to have a ton of yml files, what we're doing now seems fine.
• I think you're right, thanks. I'm surprised you're able to post comments with the default token.
• Yeah, no what I mean to say is they're both the current version. See here. The .rest methods are built on top of the .request methods to make them easier to use. I've seen mostly .request being used withinin yml files, the post-comment script uses the .rest method. Both are linked in the wiki, if we want to use one we should probably remove the link to the other. I would prefer to use the rest methods seeing as they're made for convenience and I see no reason not to, but I'm not hung up on either.
• I can make a util function that takes a function as a parameter, runs the membership check, and only calls the function if the check passes. My concern is just readability, it's pretty easy to see how the action works right now and I don't want to obfuscate a very simple script by splitting it up into too many different files that then import functions from more files. I can definitely move the full script to a JS file and make a utility function that checks membership though, I'll check the issue-trigger and make sure the util function can fit there.

[ajb176]

Hi @t-will-gillis

It was yes, but that's because I edited the verify-pr.js file to add a random string in place of prAuthor, so it was the expected result. In my testing the action's worked as expected. And in my tests before writing the PR I don't recall ever getting those error messages, I was getting a 403 error when the permissions weren't configured properly.

I just googled that error message and it seems like that happens when the admin boxes aren't checked. Have you tried creating a new secret with the same permissions and trying that?

If that doesn't work, how did you set up the github actions bot in your repository? That seems like the major difference, so I can try configuring it and seeing if somehow my handle posting/closing vs the actions bot is the cause of the different results. But that doesn't make too much sense given the bot has had no problem closing/commenting when the token wasn't supplied.

[t-will-gillis]

Hey @ajb176

If that doesn't work, how did you set up the github actions bot in your repository? That seems like the major difference, so I can try configuring it and seeing if somehow my handle posting/closing vs the actions bot is the cause of the different results. But that doesn't make too much sense given the bot has had no problem closing/commenting when the token wasn't supplied.

I have been experimenting in my repo and I continue having essentially the same results as I have been. I think that your observation about your handle posting/closing vs. the actions bot doing it is significant.

• When I provide the HACKFORLA_ADMIN_TOKEN with the permissions as noted in the convos above, the workflow successfully/ correctly identifies “member” vs “non-member”, but then the workflow fails to either comment on or close the PR due to: Unhandled error: HttpError: Must have admin rights to Repository. (<-- I believe this actually is the expected and desired behavior, but no matter...)

• When I remove the HACKFORLA_ADMIN_TOKEN, I believe that the defaults kick in. My default settings for the GITHUB-TOKEN are more permissive than yours- see Details in the previous comment above for the configurations I am using. These default permissions do not allow for the membership check to run correctly (they return 404 for everyone), however the defaults do allow the bot to both add comments and close the PR. (<-- Again, this matches HfLA but this makes me think that our defaults might be too permissive.)

• When I adjust the GITHUB_TOKEN / default permissions to be "read only", the workflow fails both when checking membership and when trying to update the PR.

• Btw: I think there is still the previous, open consideration that you brought up about using actions/checkout with pull_request_target. I have experimented with both pull_request and pull_request_target but get the same results.

[ajb176]

I've done a lot of testing with different permissions, and the only way the action breaks is if I uncheck the topmost repo (full control of private repositories). If I uncheck public_repo I get a 403 admin rights error, if I check everything individually but not the top repo box, I get a 404 error from the membership API call. It works for me without the admin boxes checked. I don't seem to need anything except repo access, the action works in my origin repo even without admin:org hook or admin:write. Even if it's not my handle obtained dynamically from the PR, if I hardcode another website-write member's username when calling check-membership the PR becomes verified, if I hardcode a non team member it gets a 404 and closes the PR.

Hey @ajb176 Great job with this- I tested in my repo with both a my name and a non-hfla name. The PR authored by the non-member was closed and the message was posted on the issue. So everything works well.

When you first tested the PR with everything in the yml file, it was working for you, wasn't it? Could you try creating a new branch, pulling from pr-creator-3906 again, creating a token with only repo permissions, and copy pasting that secret to the repo? I'm just not able to replicate the error unless I mess with the token permissions.

[t-will-gillis]

Could you try creating a new branch, pulling from pr-creator-3906 again, creating a token with only repo permissions, and copy pasting that secret to the repo?

I did what you suggested above and this appears to work for both cases. I don't completely understand why the workflow only runs when the token has fewer permissions, but good call regardless.

[t-will-gillis]

Hi @ajb176 I have tested this again and everything appears to be working correctly. I am ready to approve and merge this, and have a couple final requests:

• if you would, please replace the hard-coded variables with context variables (ie. context.repo.owner, etc. (and of course add context to params in isMemberOfTeam())
• I think that a negative console.log() statement in verify-pr.js (complement to the positive statement on line 10) would be helpful for reviewing logs

I know this has been a very long process, but thank you for working on this.

[ajb176]

Yeah, I can add a log statement

I mentioned this earlier, but I don't see any context.repo object. If I saw one I would've used it so testers wouldn't have to manually add their name. There's context.repository.owner whose value is just [Object] in this context and context.repository.full_name which is a string <owner>/website locally. I should be able to split that string to get the owner, but it's not very clean. Nevermind, I checked the context for the HFLA repo and it seems like it should work fine.

[ajb176]

The variables for closing and commenting are now taken from the context, seems like it's working. That was my mistake, I would've done it earlier but I had to manually print some objects to see their values, in the logs it just showed : [Object]

As for checking team membership, I think it's better to keep that 'hackforla' hard-coded if the goal is to be able to use it elsewhere. I'm not sure the same context variables will be available in different workflows.

[t-will-gillis]

Hi @ajb176 Great- thanks for making the final changes!