Skip to content

Allows closing PR/issues by an existence check #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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]`
Expand Down
35 changes: 27 additions & 8 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Expand All @@ -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")
}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -301,9 +318,11 @@ async function createOrAddLabel(octokit, repoDeets, labelConfig) {
})
}

// For tests
module.exports = {
getFilesNotOwnedByCodeOwner,
findCodeOwnersForChangedFiles
findCodeOwnersForChangedFiles,
githubLoginIsInCodeowners
}

// @ts-ignore
Expand Down
20 changes: 19 additions & 1 deletion index.test.js
Original file line number Diff line number Diff line change
@@ -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");
Expand Down Expand Up @@ -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);
});
})