Merge pull request #27 from OSS-Docs-Tools/allow_closing

Allows closing PR/issues by an existence check

README.md

@@ -59,6 +59,14 @@ Then you should be good to go.
 
 We force the use of [`pull_request_target`](https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/) as a workflow event to ensure that someone cannot change the CODEOWNER files at the same time as having that change be used to validate if they can merge.
 
+### Issue / PR closing
+
+Merging a PR has strict security requirements, but closing a PR or Issue can have a weaker one. Anyone with a GitHub login in the CODEOWNER has the ability to close any PR / Issue via a comment/review which includes:
+
+```
+@github-actions close
+```
+
 ### Extras
 
 You can use this label to set labels for specific sections of the codebase, by having square brackets to indicate labels to make: `[label]`

index.js

@@ -7,7 +7,7 @@ const {readFileSync} = require("fs");
 
 // Effectively the main function
 async function run() {
- core.info("Running version 1.5.4")
+ core.info("Running version 1.6.0")
 
  // Tell folks they can merge
  if (context.eventName === "pull_request_target") {
@@ -20,7 +20,7 @@ async function run() {
  if (bodyLower.includes("lgtm")) {
  new Actor().mergeIfHasAccess();
  } else if (bodyLower.includes("@github-actions close")) {
- new Actor().closeIfHasAccess();
+ new Actor().closePROrIssueIfInCodeowners();
  } else {
  console.log("Doing nothing because the body does not include a command")
  }
@@ -125,6 +125,7 @@ class Actor {
  this.octokit = getOctokit(process.env.GITHUB_TOKEN)
  this.thisRepo = { owner: context.repo.owner, repo: context.repo.repo }
  this.issue = context.payload.issue || context.payload.pull_request
+ /** @type {string} - GitHub login */
  this.sender = context.payload.sender.login
  }
 
@@ -195,11 +196,11 @@ class Actor {
  }
  }
 
- async closeIfHasAccess() {
- const prInfo = await this.getTargetPRIfHasAccess()
- if (!prInfo) {
-  return
- }
+ async closePROrIssueIfInCodeowners() { 
+ // Because closing a PR/issue does not mutate the repo, we can use a weaker
+ // authentication method: basically is the person in the codeowners? Then they can close
+ // an issue or PR. 
+ if (!githubLoginIsInCodeowners(this.sender, this.cwd)) return
 
  const { octokit, thisRepo, issue, sender } = this;
 
@@ -230,6 +231,22 @@ function getFilesNotOwnedByCodeOwner(owner, files, cwd) {
  return filesWhichArentOwned
 }
 
+
+/**
+ * This is a reasonable security measure for proving an account is specified in the codeowners
+ * but _SHOULD NOT_ be used for authentication for something which mutates the repo,
+ * 
+ * @param {string} login
+ * @param {string} cwd
+ */
+ function githubLoginIsInCodeowners(login, cwd) {
+ const codeowners = new Codeowners(cwd);
+ const contents = readFileSync(codeowners.codeownersFilePath, "utf8").toLowerCase()
+
+ return contents.includes("@" + login.toLowerCase() + " ") || contents.includes("@" + login.toLowerCase() + "\n")
+}
+
+
 /**
  *
  * @param {string[]} files
@@ -301,9 +318,11 @@ async function createOrAddLabel(octokit, repoDeets, labelConfig) {
  })
 }
 
+// For tests
 module.exports = {
  getFilesNotOwnedByCodeOwner,
- findCodeOwnersForChangedFiles
+ findCodeOwnersForChangedFiles,
+ githubLoginIsInCodeowners
 }
 
 // @ts-ignore

index.test.js

@@ -1,4 +1,4 @@
-const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles } = require(".");
+const { getFilesNotOwnedByCodeOwner, findCodeOwnersForChangedFiles, githubLoginIsInCodeowners } = require(".");
 
 test("determine who owns a set of files", () => {
  const noFiles = findCodeOwnersForChangedFiles(["root-codeowners/one.two.js"], "./test-code-owners-repo");
@@ -35,3 +35,21 @@ test("deciding if someone has access to merge", () => {
  expect(filesNotInCodeowners).toEqual(["random-path/file.ts"]);
 });
 
+describe(githubLoginIsInCodeowners, () => {
+ test("allows folks found in the codeowners", () => {
+ const ortaIn = githubLoginIsInCodeowners("orta", ".");
+ expect(ortaIn).toEqual(true);
+ });
+ test("ignores case", () => {
+ const ortaIn = githubLoginIsInCodeowners("OrTa", ".");
+ expect(ortaIn).toEqual(true);
+ });
+ test("denies other accounts", () => {
+ const noDogMan = githubLoginIsInCodeowners("dogman", ".");
+ expect(noDogMan).toEqual(false);
+ });
+ test("denies subsets of existing accounts", () => {
+ const noOrt = githubLoginIsInCodeowners("ort", ".");
+ expect(noOrt).toEqual(false);
+ });
+})