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

Allows closing PR/issues by an existence check

Author: orta

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