Skip to content
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

Audit application installations #27

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

- Fixed #16 - New users were causing an error in findPromotions
- Fixed #20 - Added full JSDoc coverage across the code
- Fixed #23 - Added auditing of application installations

## 0.1.0

Expand Down
41 changes: 41 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,47 @@ does have the side effect that, if an internal username is connected to multiple
GitHub usernames, as demonstrated above, all of those GitHub usernames will be
made admins of the org.

### [[applications]]

Each `[[applications]]` section represents one org-level application
installation. These entries are used to ensure that no applications get added to
or removed from the org unexpectedly. You can also ensure that they only have
the permissions you are expecting, and receive the events you are expecting.

Similar to the `[[teams]]` above, applications are specified as an [array of
tables]. In addition, the applications use a sub-table for the permissions,
making a single application specification look like the following:

```toml
[[applications]]
appId = 42
appSlug = "infinite-improbability-drive"
repositorySelection = "selected"
Comment on lines +140 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

as a consumer of this library, where do I find the values to use for these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm yeah good question. The app id and slug I was only able to find after actually querying our org. I honestly don't know of a better way to find them, so that may have to be our recommendation for now: run the tool and plug the data into your config. Which gives me a great idea for a future feature: orglinter --generate-config It could query the org and produce a valid config file based on its current state.

Oh as for the repositorySelection I can document that "selected" and "all" are the only values. I think. I haven't been able to find solid documentation on that either. The REST API docs are terrible!

events = ["issues", "discussion", "pull_request"]

[applications.permissions]
# This applies to the application directly above, infinite-improbability-drive
checks = "read"
issues = "read"
metadata = "read"
deployments = "write"
```

Given the flexibility of TOML, you can also specify the `permissions` table
in-line, depending on your preference. That would look like this:

```toml
[[applications]]
appId = 42
appSlug = "infinite-improbability-drive"
repositorySelection = "selected"
events = ["issues", "discussion", "pull_request"]
permissions = {checks = "read", issues = "read", metadata = "read", deployments = "write"}
```

Both of the above are wholly equivalent; it's simply a matter of preference
which way you would like to specify them.
Copy link
Contributor

Choose a reason for hiding this comment

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

for folks that might not be so intimately familiar with toml, I'd recommend giving an example of how you might define multiple applications

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, that's a good call. I do have a link to the TOML documentation which does have examples, but it's likely people won't click through. I also want to avoid making this document into a TOML tutorial, or too long. I think I'll add a second application to the example.toml.


## Development

### Running Tests
Expand Down
13 changes: 13 additions & 0 deletions example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,3 +19,16 @@ name = "Heart of Gold Crew"
default_org_role = "ADMIN"
privacy = "SECRET"
members = ["mandroid", "tmcmillan", "zbeeblebrox"]

[[applications]]
appId = 42
appSlug = "infinite-improbability-drive"
repositorySelection = "selected"
events = ["issues", "discussion", "pull_request"]

[applications.permissions]
# This applies to the application directly above, infinite-improbability-drive
checks = "read"
issues = "read"
metadata = "read"
deployments = "write"
68 changes: 67 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"eslint-plugin-mocha": "^6.3.0",
"eslint-plugin-node": "^11.1.0",
"mocha": "^8.2.1",
"nock": "^13.0.7",
"nyc": "^15.1.0",
"sinon": "^9.2.4"
},
Expand Down
32 changes: 30 additions & 2 deletions src/lib/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

const fs = require('fs').promises;
const { graphql } = require('@octokit/graphql');
const { request } = require('@octokit/request');
const TOML = require('@iarna/toml');
const typedefs = require('./typedefs');

Expand Down Expand Up @@ -94,12 +95,39 @@ async function retrieveOrgInfo(orgName, token) {
// eslint-disable-next-line id-length
requiresTwoFactorAuthentication: organization.requiresTwoFactorAuthentication,
twitterUsername: organization.twitterUsername,
websiteUrl: organization.websiteUrl
websiteUrl: organization.websiteUrl,
applications: await retrieveOrgApplications(orgName, token)
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO don't do inline awaits like this in object construction.

Also, given #26, you might start to think about how the different rulesets will need to define which information they'll need to function, how to make sure you're only running each data-fetch once, and how the data gets provided to the different rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not an inline await? Is that more of a preference/style thing, or for technical reasons?

As far as #26 though yeah I should probably make this line applications: [], and move this out to a separate later line of result.applications = await .... Then later there can be something like if (config.checkApplications) result.applications = await ....

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not an inline await

There's not a technical reason, it works, but it can be hard to debug and hard to reason about imagine code like:

const stuff = {
  foo: await foo(),
  bar: await bar(),
  fizzyLiftingDrinks: fizzyLiftingDrinks(),
  fizzbuzz: await fizzbuzz()
};

it can be hard to debug if there are issues. its hard to reason about the ordering of the promises and from looking at it it looks at first glance that it might be run in parallel, but that wouldn't be the case. Is the lack of await on the fizzyLiftingDrinks intentional or a bug?

Pulling the awaits out make it easier to reason about and the order more apparent and you could force it to be parallel if you wanted.

};
console.log(`${Object.keys(result.members).length} total members retrieved.`);
return result;
}

/**
* Retrieve a list of all applications installed to the requested org
*
* @param {string} orgName - The login name of the org to retrieve applications for
* @param {string} token - A personal access token for interacting with the API
* @returns {typedefs.AppSet} - A list of all applications installed on the organization
*/
async function retrieveOrgApplications(orgName, token) {
const response = await request('GET /orgs/{org}/installations', {
headers: { authorization: `token ${token}` },
org: orgName
});
const result = response.data.installations.reduce((apps, app) => {
apps.push({
appId: app.app_id,
appSlug: app.app_slug,
repositorySelection: app.repository_selection,
permissions: app.permissions,
events: app.events
});
return apps;
}, []);
console.log(`Loaded ${result.length} application installations for ${orgName}.`);
return result;
}

/**
* Load an org's expected configuration from a TOML config file
*
Expand Down Expand Up @@ -132,4 +160,4 @@ async function loadMembershipConfig(fileName) {
return config;
}

module.exports = { retrieveOrgInfo, loadMembershipConfig };
module.exports = { retrieveOrgInfo, retrieveOrgApplications, loadMembershipConfig };
17 changes: 17 additions & 0 deletions src/lib/typedefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,23 @@
* @typedef {object.<string, MemberRecord>} MemberSet
*/

/**
* A normalized application installation record retrieved from the GitHub REST API
*
* @typedef {object} AppRecord
* @property {number} appId - The global database identifier for the application
* @property {string} appSlug - A unique slug for identifying the application
* @property {string} repositorySelection - Whether this is installed for "all" or "selected" repositories
* @property {object.<string, string>} permissions - The set of permissions granted to this application
* @property {Array.<string>} events - A list of events that are received by this application
*/

/**
* A set of application installation records retrieved from the GitHub REST API
*
* @typedef {Array.<AppRecord>} AppSet
*/

/**
* A normalized org record retrieved from the GitHub GraphQL API
*
Expand Down
70 changes: 70 additions & 0 deletions test/loaders.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
'use strict';

const assume = require('assume');
const fs = require('fs').promises;
const loaders = require('../src/lib/loaders');
const nock = require('nock');
const path = require('path');
const sinon = require('sinon');


describe('Loaders', function () {

before(function () {
sinon.stub(console, 'log');
});

after(function () {
sinon.restore();
});
Comment on lines +13 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO make these be beforeEach and afterEach that way you're not leaking state between tests via sinon.

before/after - before/after the scope they exist in (in this case before & after everything within the describe, i.e. these will run once).
beforeEach/afterEach - before/after each test in the scope (i.e. these will run once per test within this describe)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah probably a good call. My thought here was mostly to just stop console logs from happening and since I am not actually checking these logs I only wanted just a single call as opposed to many. But I could see wanting to check the log values later on.


describe('retrieveOrgApplications', function () {
let scope;

before(async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

same with these, make it beforeEach/afterEach

const response = JSON.parse(await fs.readFile(path.resolve(__dirname, 'responses/installations.json')));
scope = nock(
'https://api.github.com/',
{ reqheaders: { authorization: 'token 12345' } }
).get('/orgs/foo/installations').reply(200, response).persist();
});

after(function () {
nock.cleanAll();
});

it('calls the proper REST endpoint', async function () {
await loaders.retrieveOrgApplications('foo', '12345');
scope.done();
});

it('returns only the relevant information', async function () {
const apps = await loaders.retrieveOrgApplications('foo', '12345');

assume(apps).eqls([
{
appId: 12,
appSlug: 'heart-of-gold',
repositorySelection: 'all',
permissions: {
foo: 'read',
bar: 'write',
baz: 'read'
},
events: ['push', 'release']
},
{
appId: 13,
appSlug: 'infinite-improbability-drive',
repositorySelection: 'selected',
permissions: {
foo: 'write',
bar: 'read',
baz: 'write'
},
events: ['issue', 'discussion', 'pull_request']
}
]);
});
});
});
Loading