Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

Only allowed users can execute benchmarks #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ WEBHOOK_PROXY_URL=<web hook url (like https://smee.io), not required>

Add `BASE_BRANCH=master` or whatever is appropriate.

#### Allowed users

It is possible to restrict who can execute a benchmark.

Add `ALLOWED_USERS` with comma separated list of user's github ids. Eg:

`ALLOWED_USERS=123,455,234`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not support usernames directly?

Copy link
Author

@enthusiastmartin enthusiastmartin Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be possible. One of the reasons that i went with ids was that you can change your username ( although probably not very common thing to do).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if users can change their username, we would prefer this, as it is much easier to audit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github IDs are immutable, therefore saner to rely on. You don't risk having someone hijack a username to execute the commands. I don't get why it would be harder to audit - even if that were the case, the stability makes up for it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github IDs are immutable, therefore saner to rely on. You don't risk having someone hijack a username to execute the commands. I don't get why it would be harder to audit - even if that were the case, the stability makes up for it.

imo, this is good point.

Obliviously i can change this PR and use usernames instead of ids - so let me know what would you prefer.


Github user id can be retrieved using Github API: https://api.github.com/users/your_github_user_name

If `ALLOWED_USERS` is not specified - any user can execute the benchmark.

## Permissions Needed

* Metadata: Read Only
Expand Down
24 changes: 23 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@

var { benchBranch, benchmarkRuntime } = require("./bench");
const { benchBranch, benchmarkRuntime } = require("./bench");

let allowedUsers = process.env.ALLOWED_USERS;
if (allowedUsers) {
allowedUsers = allowedUsers.split(",").map(Number).filter(item => item);
}

// Allow only selected users or if not specified - allow any.
function isAllowed(senderId) {
return allowedUsers === undefined || allowedUsers.includes(senderId);
}

module.exports = app => {
app.log(`base branch: ${process.env.BASE_BRANCH}`);
Expand All @@ -10,6 +20,18 @@ module.exports = app => {
return;
}

if (! isAllowed(context.payload.sender.id)){
app.log(`User not allowed ${context.payload.sender.id}`)
const repo = context.payload.repository.name;
const owner = context.payload.repository.owner.login;
const comment_id = context.payload.comment.id;
context.github.issues.updateComment({
owner, repo, comment_id,
body: `Denied. User is not allowed to execute benchmark.`
});
return;
}

// Capture `<action>` in `/bench <action> <extra>`
let action = commentText.split(" ").splice(1, 1).join(" ").trim();
// Capture all `<extra>` text in `/bench <action> <extra>`
Expand Down