diff --git a/.env.example b/.env.example index 80a254b1..eb79cc14 100644 --- a/.env.example +++ b/.env.example @@ -2,33 +2,25 @@ # Follow the instructions in README.md as how to add your personal # tokens and secrets in this file -# GitHub App ID -GH_APP_IDENTIFIER='' -# Github App Webhook Secret to verify requests come from GitHub -GH_WEBHOOK_SECRET='' -# GitHub App Secret Key -# NOTE: this *must* be on a single line, otherwise it will break -# reading the secret key. -GH_APP_SECRET_KEY='-----BEGIN RSA PRIVATE KEY----------END RSA PRIVATE KEY-----' -# Personal (ie, for your own GitHub account) user access token with the `admin:org`, -# `admin:org_hook`, `admin: repo_hook`, `project`, `public_repo`, `repo:status`, and -# `repo_deployment` permissions enabled. +# GitHub GH_USER_TOKEN='' +GH_WEBHOOK_SECRET='' -# Slack signing secret to verify requests come from Slack -SLACK_SIGNING_SECRET= -# Slack Bot user access token -> "Bot User OAuth Token" -SLACK_BOT_USER_ACCESS_TOKEN= +# GitHub Apps +GH_APP_1_ORG_SLUG='' +GH_APP_1_IDENTIFIER='' +GH_APP_1_SECRET_KEY='-----BEGIN RSA PRIVATE KEY----------END RSA PRIVATE KEY-----' +GH_APP_1_ISSUES_PROJECT_NODE_ID='' +GH_APP_1_PRODUCT_AREA_FIELD_ID='' +GH_APP_1_STATUS_FIELD_ID='' +GH_APP_1_RESPONSE_DUE_DATE_FIELD_ID='' + +# Slack +SLACK_SIGNING_SECRET='' +SLACK_BOT_USER_ACCESS_TOKEN='' # The name of the personal repository in your personal organization being used for development. PERSONAL_TEST_REPO='eng-pipes-dev' -# Application-specific configuration -# Config for the "GitHub Issues Someone Else Cares About" project board -ISSUES_PROJECT_NODE_ID='' -STATUS_FIELD_ID='' -PRODUCT_AREA_FIELD_ID='' -RESPONSE_DUE_DATE_FIELD_ID='' - # Silence some GCP noise DRY_RUN=true diff --git a/.env.test b/.env.test index c064f241..c8e9cd32 100644 --- a/.env.test +++ b/.env.test @@ -1,9 +1,25 @@ # GitHub FORCE_USER_TOKEN_GITHUB_CLIENT="false" GH_USER_TOKEN="ghp_BLAHBLAHBLAH" -GH_APP_IDENTIFIER="1234" GH_WEBHOOK_SECRET="webhooksecret" -GH_APP_SECRET_KEY="top \nsecret\n key" + +# some functionality is org-agnostic +GH_APP_1_ORG_SLUG="Enterprise" +GH_APP_1_IDENTIFIER="1234" +GH_APP_1_SECRET_KEY="top \nsecret\n key" +GH_APP_1_ISSUES_PROJECT_NODE_ID="PVT_blahblah" +GH_APP_1_PRODUCT_AREA_FIELD_ID="PVTSSF_flooflah" +GH_APP_1_STATUS_FIELD_ID="PVTSSF_zimzimzim" +GH_APP_1_RESPONSE_DUE_DATE_FIELD_ID="PVTF_zamzamzam" + +# other functionality is hardwired for getsentry +GH_APP_2_ORG_SLUG="getsentry" +GH_APP_2_IDENTIFIER="7890" +GH_APP_2_SECRET_KEY="super \nsecret\n key" +GH_APP_2_ISSUES_PROJECT_NODE_ID="PVT_flahflah" +GH_APP_2_PRODUCT_AREA_FIELD_ID="PVTSSF_moomah" +GH_APP_2_STATUS_FIELD_ID="PVTSSF_mizmizmi" +GH_APP_2_RESPONSE_DUE_DATE_FIELD_ID="PVTF_mazmaza" # Slack SLACK_SIGNING_SECRET="slacksigningsecret" diff --git a/README.md b/README.md index 6565cca2..0ea35fb1 100644 --- a/README.md +++ b/README.md @@ -47,20 +47,43 @@ Under Sentry's [webhooks](https://github.com/organizations/getsentry/settings/ho ### Setup Secrets The following secrets are configured in GitHub for this app to function and to deploy to Google. -You can grab GitHub secrets in their respective configuration pages: [GitHub App](https://github.com/organizations/getsentry/settings/apps/getsantry) +You can grab GitHub secrets in their respective configuration pages: [getsantry](https://github.com/organizations/getsentry/settings/apps/getsantry), [covecod](https://github.com/organizations/codecov/settings/apps/covecod). #### Local Secrets (required to run yarn dev) -You will also need to set up some of these environment variables if you want to test this locally, e.g. using `direnv` or something similar +You will also need to set up some of these environment variables if you want to test this locally, e.g. using `direnv` or something similar. See below for more instructions on [setting up a local environment](#configuring-a-test-environment). + +##### GitHub + +- `GH_WEBHOOK_SECRET` - GitHub webhook secret to confirm that webhooks come + from GitHub +- `GH_USER_TOKEN` - Personal (ie, for your own GitHub account) user access + token with the `read:org` and `read:user` permissions enabled. + +##### GitHub Apps + +You can configure multiple GitHub Apps to send events to one `eng-pipes` +installation. The config values are grouped together by number, so +`GH_APP_1_*`, `GH_APP_2_*`, etc. + +- `GH_APP_1_ORG_SLUG` - Slug for the org where the GitHub app is installed +- `GH_APP_1_IDENTIFIER` - GitHub App identifier +- `GH_APP_1_SECRET_KEY` - GitHub App private key - NOTE: this *must* be on a single line, otherwise it will break reading the secret key (however, it can have literal `\n`s in it, which will be replaced with newlines) +- `GH_APP_1_ISSUES_PROJECT_NODE_ID` - node ID of the "GitHub Issues Someone Else Cares About" project board in the org +- `GH_APP_1_PRODUCT_AREA_FIELD_ID` - id of the "Product Area" field in the board +- `GH_APP_1_STATUS_FIELD_ID` - id of the "Status" field in the board +- `GH_APP_1_RESPONSE_DUE_DATE_FIELD_ID` - id of the "Response Due" field in the board + +##### Slack -- `GH_APP_IDENTIFIER` - GitHub App identifier -- `GH_APP_SECRET_KEY` - GitHub App private key -- `GH_WEBHOOK_SECRET` - GitHub webhook secret to confirm that webhooks come from GitHub -- `SENTRY_WEBPACK_WEBHOOK_SECRET` - Webhook secret that needs to match secret from CI. Records webpack asset sizes. - `SLACK_SIGNING_SECRET` - Slack webhook secret to verify payload - `SLACK_BOT_USER_ACCESS_TOKEN` - The Slack Bot User OAuth Access Token from the `Oauth & Permissions` section of your Slack app -Optional database configuration +##### CI + +- `SENTRY_WEBPACK_WEBHOOK_SECRET` - Webhook secret that needs to match secret from CI. Records webpack asset sizes. + +##### Database (Optional) - `DB_USER` - Database user - `DB_PASSWORD` - Database password @@ -128,7 +151,7 @@ NOTE: These steps will cover more aspects over time. For now it focuses on testi - In that organization, create a [new GitHub App](https://github.com/settings/apps/new) - Set the webhook to your ngrok tunnel with the GH route (e.g. `/webhooks/github`) - - Place the secrets in your `.env` (see [Setup Secrets](#setup-secrets) below) + - Place the secrets in your `.env` (see [Setup Secrets](#setup-secrets) above) - Go to the `Permissions & events` sidebar menu entry of the GitHub app configuration, and grant maximum non-`Admin` access (`Read and write` where possible, `Read only` everywhere else) for every line in `Repository permissions` (NOTE: We use a more constrained permission-set in production, but for initial setup enabling maximal permissions is fine; permissions can be whittled down later as needed.) - For `Organization permissions`, grant `Read and write` for `Members` and `Projects` - In the `Subscribe to events` section, check every possible box diff --git a/src/api/github/getClient.test.ts b/src/api/github/getClient.test.ts index c0648df5..e909b957 100644 --- a/src/api/github/getClient.test.ts +++ b/src/api/github/getClient.test.ts @@ -1,5 +1,6 @@ import { createAppAuth } from '@octokit/auth-app'; +import { GETSENTRY_ORG } from '@/config'; import { ClientType } from '@api/github/clientType'; import { getClient } from '@api/github/getClient'; import { OctokitWithRetries as octokitClass } from '@api/github/octokitWithRetries'; @@ -23,10 +24,10 @@ describe('getClient(ClientType.User)', function () { }); }); -describe("getClient(ClientType.App, 'getsentry')", function () { +describe("getClient(ClientType.App, 'Enterprise')", function () { beforeAll(async function () { octokitClass.mockClear(); - await getClient(ClientType.App, 'getsentry'); + await getClient(ClientType.App, 'Enterprise'); }); it('is instantiated twice', async function () { @@ -43,6 +44,21 @@ describe("getClient(ClientType.App, 'getsentry')", function () { auth: { appId: 1234, privateKey: 'top \nsecret\n key', + installationId: 'installation-Enterprise', + }, + }); + }); +}); + +describe('getClient(ClientType.App, GETSENTRY_ORG)', function () { + it('also knows about getsentry', async function () { + octokitClass.mockClear(); + const actual = await getClient(ClientType.App, GETSENTRY_ORG); + expect(octokitClass).toHaveBeenLastCalledWith({ + authStrategy: createAppAuth, + auth: { + appId: 7890, + privateKey: 'super \nsecret\n key', installationId: 'installation-getsentry', }, }); diff --git a/src/api/github/getClient.ts b/src/api/github/getClient.ts index f2f77e9b..b133aa1c 100644 --- a/src/api/github/getClient.ts +++ b/src/api/github/getClient.ts @@ -44,11 +44,10 @@ export async function getClient(type: ClientType, org?: string | null) { ); } - const app = GH_APPS.get('__tmp_org_placeholder__'); - let client = _CLIENTS_BY_ORG.get(org); if (client === undefined) { // Bootstrap with a client not bound to an org. + const app = GH_APPS.get(org); const appClient = _getAppClient(app.auth); // Use the unbound client to hydrate a client bound to an org. diff --git a/src/brain/ghaCancel/index.test.ts b/src/brain/ghaCancel/index.test.ts index 0ac5368e..06275c41 100644 --- a/src/brain/ghaCancel/index.test.ts +++ b/src/brain/ghaCancel/index.test.ts @@ -20,7 +20,7 @@ describe('gha-test', function () { }); beforeEach(async function () { await db('users').delete(); - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, 'Enterprise'); fastify = await buildServer(false); ghaCancel(); // @ts-ignore diff --git a/src/brain/githubMetrics/index.test.ts b/src/brain/githubMetrics/index.test.ts index feeb3a1e..630f4a71 100644 --- a/src/brain/githubMetrics/index.test.ts +++ b/src/brain/githubMetrics/index.test.ts @@ -66,7 +66,7 @@ describe('github webhook', function () { beforeEach(async function () { fastify = await buildServer(false); - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, 'Enterprise'); metrics(); }); diff --git a/src/brain/issueLabelHandler/followups.ts b/src/brain/issueLabelHandler/followups.ts index ae7a7c39..94f99100 100644 --- a/src/brain/issueLabelHandler/followups.ts +++ b/src/brain/issueLabelHandler/followups.ts @@ -77,8 +77,7 @@ export async function updateCommunityFollowups({ return; } - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); const repo = payload.repository.name; const issueNumber = payload.issue.number; @@ -88,16 +87,16 @@ export async function updateCommunityFollowups({ if (isWaitingForCommunityLabelOnIssue) { await octokit.issues.removeLabel({ - owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, name: WAITING_FOR_COMMUNITY_LABEL, }); } await octokit.issues.addLabels({ - owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, labels: [WAITING_FOR_PRODUCT_OWNER_LABEL], }); @@ -142,8 +141,7 @@ export async function ensureOneWaitingForLabel({ } const { issue, label } = payload; - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); const repo = payload.repository.name; const issueNumber = payload.issue.number; // Here label will never be undefined, ts is erroring here but is handled in the shouldSkip above @@ -156,8 +154,8 @@ export async function ensureOneWaitingForLabel({ if (labelToRemove != null) { await octokit.issues.removeLabel({ - owner: owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, name: labelToRemove, }); diff --git a/src/brain/issueLabelHandler/index.test.ts b/src/brain/issueLabelHandler/index.test.ts index 596c4ce5..4b99dd1c 100644 --- a/src/brain/issueLabelHandler/index.test.ts +++ b/src/brain/issueLabelHandler/index.test.ts @@ -24,7 +24,7 @@ import { issueLabelHandler } from '.'; describe('issueLabelHandler', function () { let fastify: Fastify; let octokit; - const app = GH_APPS.get('__tmp_org_placeholder__'); + const app = GH_APPS.get('Enterprise'); const errors = jest.fn(); let say, respond, client, ack; let calculateSLOViolationRouteSpy, calculateSLOViolationTriageSpy; diff --git a/src/brain/issueLabelHandler/route.ts b/src/brain/issueLabelHandler/route.ts index 824f3129..71a5d244 100644 --- a/src/brain/issueLabelHandler/route.ts +++ b/src/brain/issueLabelHandler/route.ts @@ -81,18 +81,17 @@ export async function markWaitingForSupport({ const issueNumber = payload.issue.number; // New issues get a Waiting for: Support label. - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); await octokit.issues.addLabels({ - owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, labels: [WAITING_FOR_SUPPORT_LABEL], }); await octokit.issues.createComment({ - owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, body: `Assigning to @${GETSENTRY_ORG}/support for [routing](https://open.sentry.io/triage/#2-route) ⏲️`, }); @@ -137,8 +136,7 @@ export async function markNotWaitingForSupport({ const { issue, label } = payload; const productAreaLabel = label; const productAreaLabelName = productAreaLabel?.name || PRODUCT_AREA_UNKNOWN; - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); const labelsToRemove: string[] = []; const labelNames = issue?.labels?.map((label) => label.name) || []; const isBeingRoutedBySupport = labelNames.includes(WAITING_FOR_SUPPORT_LABEL); @@ -153,7 +151,7 @@ export async function markNotWaitingForSupport({ for (const label of labelsToRemove) { try { await octokit.issues.removeLabel({ - owner: owner, + owner: app.org, repo: payload.repository.name, issue_number: payload.issue.number, name: label, @@ -175,7 +173,7 @@ export async function markNotWaitingForSupport({ // Only retriage issues if support is routing if (isBeingRoutedBySupport) { await octokit.issues.addLabels({ - owner: owner, + owner: app.org, repo: payload.repository.name, issue_number: payload.issue.number, labels: [WAITING_FOR_PRODUCT_OWNER_LABEL], @@ -185,7 +183,7 @@ export async function markNotWaitingForSupport({ const comment = await routeIssue(octokit, productAreaLabelName); await octokit.issues.createComment({ - owner, + owner: app.org, repo: payload.repository.name, issue_number: payload.issue.number, body: comment, diff --git a/src/brain/issueLabelHandler/triage.ts b/src/brain/issueLabelHandler/triage.ts index cccf02da..1234f58c 100644 --- a/src/brain/issueLabelHandler/triage.ts +++ b/src/brain/issueLabelHandler/triage.ts @@ -9,11 +9,11 @@ import { import { ClientType } from '@api/github/clientType'; import { getClient } from '@api/github/getClient'; import { + addIssueToGlobalIssuesProject, isNotFromAnExternalOrGTMUser, modifyProjectIssueField, shouldSkip, } from '@utils/githubEventHelpers'; -import { addIssueToGlobalIssuesProject } from '@utils/githubEventHelpers'; import { isFromABot } from '@utils/isFromABot'; const REPOS_TO_TRACK_FOR_TRIAGE = new Set(SENTRY_SDK_REPOS); @@ -60,14 +60,13 @@ export async function markWaitingForProductOwner({ } // New issues get an Untriaged label. - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); const repo = payload.repository.name; const issueNumber = payload.issue.number; await octokit.issues.addLabels({ - owner, - repo: repo, + owner: app.org, + repo, issue_number: issueNumber, labels: [WAITING_FOR_PRODUCT_OWNER_LABEL], }); @@ -114,11 +113,10 @@ export async function markNotWaitingForProductOwner({ } // Remove Untriaged label when triaged. - const owner = payload.repository.owner.login; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); try { await octokit.issues.removeLabel({ - owner: owner, + owner: app.org, repo: payload.repository.name, issue_number: payload.issue.number, name: WAITING_FOR_PRODUCT_OWNER_LABEL, diff --git a/src/brain/notifyOnGoCDStageEvent/index.test.ts b/src/brain/notifyOnGoCDStageEvent/index.test.ts index a3a410fe..fd0bfb9f 100644 --- a/src/brain/notifyOnGoCDStageEvent/index.test.ts +++ b/src/brain/notifyOnGoCDStageEvent/index.test.ts @@ -98,7 +98,7 @@ describe('notifyOnGoCDStageEvent', function () { await pleaseDeployNotifier(); await notifyOnGoCDStageEvent(); - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, 'Enterprise'); octokit.paginate.mockImplementation(() => { return [{ name: 'only frontend changes', conclusion: 'success' }]; }); diff --git a/src/brain/pleaseDeployNotifier/index.test.ts b/src/brain/pleaseDeployNotifier/index.test.ts index 1f464aa4..96e4374f 100644 --- a/src/brain/pleaseDeployNotifier/index.test.ts +++ b/src/brain/pleaseDeployNotifier/index.test.ts @@ -37,7 +37,7 @@ describe('pleaseDeployNotifier', function () { jest.spyOn(actions, 'actionViewUndeployedCommits'); pleaseDeployNotifier(); - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, 'Enterprise'); octokit.repos.getCommit.mockImplementation(({ repo, ref }) => { const defaultPayload = require('@test/payloads/github/commit').default; if (repo === 'sentry') { diff --git a/src/brain/projectsHandler/index.test.ts b/src/brain/projectsHandler/index.test.ts index d1c08159..e3496c89 100644 --- a/src/brain/projectsHandler/index.test.ts +++ b/src/brain/projectsHandler/index.test.ts @@ -14,7 +14,7 @@ import { projectsHandler } from '.'; describe('projectsHandler', function () { let fastify: Fastify; let octokit; - const app = GH_APPS.get('__tmp_org_placeholder__'); + const app = GH_APPS.get('Enterprise'); const errors = jest.fn(); beforeAll(async function () { @@ -38,7 +38,7 @@ describe('projectsHandler', function () { beforeEach(async function () { fastify = await buildServer(false); await projectsHandler(); - octokit = await getClient(ClientType.App, 'test-org'); + octokit = await getClient(ClientType.App, 'Enterprise'); }); afterEach(async function () { @@ -59,7 +59,7 @@ describe('projectsHandler', function () { fieldNodeId?: string ) { const projectPayload = { - organization: { login: 'test-org' }, + organization: { login: 'Enterprise' }, projects_v2_item: { project_node_id: projectNodeId || 'test-project-node-id', node_id: 'test-node-id', diff --git a/src/brain/projectsHandler/project.ts b/src/brain/projectsHandler/project.ts index 9e55a4bd..af20fb83 100644 --- a/src/brain/projectsHandler/project.ts +++ b/src/brain/projectsHandler/project.ts @@ -4,10 +4,10 @@ import * as Sentry from '@sentry/node'; import { GH_APPS, PRODUCT_AREA_LABEL_PREFIX } from '@/config'; import { ClientType } from '@api/github/clientType'; import { getClient } from '@api/github/getClient'; -import { shouldSkip } from '@utils/githubEventHelpers'; import { getIssueDetailsFromNodeId, getKeyValueFromProjectField, + shouldSkip, } from '@utils/githubEventHelpers'; function isNotInAProjectWeCareAbout(payload, app) { @@ -64,8 +64,7 @@ export async function syncLabelsWithProjectField({ return; } - const owner = payload?.organization?.login || ''; - const octokit = await getClient(ClientType.App, owner); + const octokit = await getClient(ClientType.App, app.org); const fieldName = getFieldName(payload, app); const fieldValue = await getKeyValueFromProjectField( payload.projects_v2_item.node_id, @@ -84,7 +83,7 @@ export async function syncLabelsWithProjectField({ ); await octokit.issues.addLabels({ - owner, + owner: app.org, repo: issueInfo.repo, issue_number: issueInfo.number, labels: [ diff --git a/src/brain/requiredChecks/getAnnotations.test.ts b/src/brain/requiredChecks/getAnnotations.test.ts index 06c5a639..fa6b919a 100644 --- a/src/brain/requiredChecks/getAnnotations.test.ts +++ b/src/brain/requiredChecks/getAnnotations.test.ts @@ -1,3 +1,4 @@ +import { GETSENTRY_ORG } from '@/config'; import { ClientType } from '@api/github/clientType'; import { getClient } from '@api/github/getClient'; @@ -7,7 +8,7 @@ describe('getAnnotations', function () { let octokit; beforeAll(async function () { - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, GETSENTRY_ORG); }); beforeEach(function () { diff --git a/src/brain/requiredChecks/index.test.ts b/src/brain/requiredChecks/index.test.ts index 501e135d..4bb1701e 100644 --- a/src/brain/requiredChecks/index.test.ts +++ b/src/brain/requiredChecks/index.test.ts @@ -22,6 +22,7 @@ import { createGitHubEvent } from '@test/utils/github'; import { buildServer } from '@/buildServer'; import { BuildStatus, + GETSENTRY_ORG, REQUIRED_CHECK_CHANNEL, REQUIRED_CHECK_NAME, } from '@/config'; @@ -66,7 +67,7 @@ describe('requiredChecks', function () { beforeEach(async function () { fastify = await buildServer(false); await requiredChecks(); - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, GETSENTRY_ORG); octokit.repos.getCommit.mockImplementation(({ repo, ref }) => { const defaultPayload = require('@test/payloads/github/commit').default; diff --git a/src/config/index.test.ts b/src/config/index.test.ts new file mode 100644 index 00000000..9451c435 --- /dev/null +++ b/src/config/index.test.ts @@ -0,0 +1,47 @@ +import { GH_APPS } from '@/config'; + +describe('GH_APPS', function () { + it('get errors out for unknown org', function () { + expect(() => GH_APPS.get('CheezWhiz')).toThrow( + "No app is registered for 'CheezWhiz'", + '' + ); + }); + + it.each([ + ['bad payload', {}, "Could not find an org in 'undefined' or 'undefined'."], + [ + 'bad payload.organization', + { organization: {} }, + "Could not find an org in '{}' or 'undefined'.", + ], + [ + 'unknown org in payload.organization', + { organization: { login: 'foo' } }, + "No app is registered for 'foo'.", + ], + [ + 'bad payload.repository', + { repository: {} }, + "Could not find an org in 'undefined' or 'undefined'.", + ], + [ + 'bad payload.repository.owner', + { repository: { owner: {} } }, + "Could not find an org in 'undefined' or '{}'.", + ], + + [ + 'user as payload.repository.owner', + { repository: { owner: { login: 'foo', type: 'User' } } }, + 'Could not find an org in \'undefined\' or \'{\n "login": "foo",\n "type": "User"\n}\'.', + ], + [ + 'unknown org in payload.repository.owner', + { repository: { owner: { login: 'foo', type: 'Organization' } } }, + "No app is registered for 'foo'.", + ], + ])('getForPayload errors out for %s', (description, payload, error) => { + expect(() => GH_APPS.getForPayload(payload)).toThrow(error); + }); +}); diff --git a/src/config/loadGitHubAppsFromEnvironment.test.ts b/src/config/loadGitHubAppsFromEnvironment.test.ts index be683130..602cdcf1 100644 --- a/src/config/loadGitHubAppsFromEnvironment.test.ts +++ b/src/config/loadGitHubAppsFromEnvironment.test.ts @@ -5,10 +5,10 @@ describe('loadGitHubAppsFromEnvironment ', function () { const expected = { apps: new Map([ [ - '__tmp_org_placeholder__', + 'Enterprise', { num: 1, - org: '__tmp_org_placeholder__', + org: 'Enterprise', auth: { appId: 42, privateKey: 'cheese', @@ -25,12 +25,13 @@ describe('loadGitHubAppsFromEnvironment ', function () { ]), }; const actual = loadGitHubAppsFromEnvironment({ - GH_APP_IDENTIFIER: '42', - GH_APP_SECRET_KEY: 'cheese', - ISSUES_PROJECT_NODE_ID: 'bread', - PRODUCT_AREA_FIELD_ID: 'wine', - STATUS_FIELD_ID: 'beer', - RESPONSE_DUE_DATE_FIELD_ID: 'olives', + GH_APP_1_ORG_SLUG: 'Enterprise', + GH_APP_1_IDENTIFIER: '42', + GH_APP_1_SECRET_KEY: 'cheese', + GH_APP_1_ISSUES_PROJECT_NODE_ID: 'bread', + GH_APP_1_PRODUCT_AREA_FIELD_ID: 'wine', + GH_APP_1_STATUS_FIELD_ID: 'beer', + GH_APP_1_RESPONSE_DUE_DATE_FIELD_ID: 'olives', }); expect(actual).toEqual(expected); }); @@ -40,9 +41,14 @@ describe('loadGitHubAppsFromEnvironment ', function () { RANDOM: 'garbage', AND_EXTRA: 'stuff', - GH_APP_IDENTIFIER: '42', - GH_APP_SECRET_KEY: 'cheese', - }).get('__tmp_org_placeholder__').auth.appId; + GH_APP_1_ORG_SLUG: 'Enterprise', + GH_APP_1_IDENTIFIER: '42', + GH_APP_1_SECRET_KEY: 'cheese', + GH_APP_1_ISSUES_PROJECT_NODE_ID: 'bread', + GH_APP_1_PRODUCT_AREA_FIELD_ID: 'wine', + GH_APP_1_STATUS_FIELD_ID: 'beer', + GH_APP_1_RESPONSE_DUE_DATE_FIELD_ID: 'olives', + }).get('Enterprise').auth.appId; expect(actual).toEqual(42); }); @@ -54,33 +60,18 @@ describe('loadGitHubAppsFromEnvironment ', function () { expect(actual).toEqual({ apps: new Map() }); }); - it('loads defaults for project board if auth is present', async function () { - const expected = { - apps: new Map([ - [ - '__tmp_org_placeholder__', - { - num: 1, - org: '__tmp_org_placeholder__', - auth: { - appId: 42, - privateKey: 'cheese', - installationId: undefined, - }, - project: { - node_id: 'PVT_kwDOABVQ184AOGW8', - product_area_field_id: 'PVTSSF_lADOABVQ184AOGW8zgJEBno', - status_field_id: 'PVTSSF_lADOABVQ184AOGW8zgI_7g0', - response_due_date_field_id: 'PVTF_lADOABVQ184AOGW8zgLLxGg', - }, - }, - ], - ]), + it('errors out on partial config', async function () { + const t = () => { + loadGitHubAppsFromEnvironment({ + GH_APP_1_ORG_SLUG: 'Enterprise', + GH_APP_1_IDENTIFIER: '42', + GH_APP_1_SECRET_KEY: 'cheese', + GH_APP_1_ISSUES_PROJECT_NODE_ID: '', // empty still fails + }); }; - const actual = loadGitHubAppsFromEnvironment({ - GH_APP_IDENTIFIER: '42', - GH_APP_SECRET_KEY: 'cheese', - }); - expect(actual).toEqual(expected); + expect(t).toThrow(Error); + expect(t).toThrow( + 'Config missing: {"1":["project.node_id","project.product_area_field_id","project.status_field_id","project.response_due_date_field_id"]}' + ); }); }); diff --git a/src/config/loadGitHubAppsFromEnvironment.ts b/src/config/loadGitHubAppsFromEnvironment.ts index 639701a5..0db1b722 100644 --- a/src/config/loadGitHubAppsFromEnvironment.ts +++ b/src/config/loadGitHubAppsFromEnvironment.ts @@ -13,6 +13,12 @@ class GitHubAppConfig { org: any; auth: any; project: any; + + constructor(num) { + this.num = num; + this.auth = {}; + this.project = {}; + } } class GitHubAppConfigs { @@ -22,14 +28,40 @@ class GitHubAppConfigs { this.configs = new Map(); } - getOrCreate(num: number): object | undefined { + getOrCreate(num: number): object { if (!this.configs.has(num)) { - const config = new GitHubAppConfig(); - config.num = num; + const config = new GitHubAppConfig(num); this.configs.set(num, config); } return this.configs.get(num); } + + validateAll() { + const allErrors = new Object(); + for (const [n, config] of this.configs) { + const errors = new Array(); + [ + 'auth.appId', + 'auth.privateKey', + 'project.node_id', + 'project.product_area_field_id', + 'project.status_field_id', + 'project.response_due_date_field_id', + ].forEach((group_key) => { + const [group, key] = group_key.split('.'); + if (!config[group][key]) { + errors.push(`${group}.${key}`); + } + }); + + if (errors.length) { + allErrors[n] = errors; + } + } + if (Object.keys(allErrors).length) { + throw new Error(`Config missing: ${JSON.stringify(allErrors)}`); + } + } } // App - strongly typed and permanent, these are used throughout the codebase @@ -69,11 +101,26 @@ export class GitHubApps { } getForPayload(gitHubEventPayload) { - // Soon we aim to support multiple orgs! - const org = '__tmp_org_placeholder__'; // payload?.organization?.login; - if (!org) { + // Org slug is differently accessed in org-scoped APIs vs. repo-scoped APIs + let org: string; + if (gitHubEventPayload.organization?.login) { + org = gitHubEventPayload.organization.login; + } else if ( + gitHubEventPayload.repository?.owner?.login && + gitHubEventPayload.repository?.owner?.type === 'Organization' + ) { + org = gitHubEventPayload.repository.owner.login; + } else { throw new Error( - `Could not find an org in ${JSON.stringify(gitHubEventPayload)}.` + `Could not find an org in '${JSON.stringify( + gitHubEventPayload.organization, + null, + 2 + )}' or '${JSON.stringify( + gitHubEventPayload.repository?.owner, + null, + 2 + )}'.` ); } return this.get(org); @@ -84,31 +131,51 @@ export class GitHubApps { export function loadGitHubAppsFromEnvironment(env) { const configs = new GitHubAppConfigs(); - let config; - - if (env.GH_APP_IDENTIFIER && env.GH_APP_SECRET_KEY) { - // Collect config by (proleptic) envvar number. Once we have GH_APP_1_FOO - // this will make more sense. We'll collect stuff in config and then - // instantiate a GitHubApp once all config has been collected for each - // (once we've made a full pass through process.env). - - config = configs.getOrCreate(1); - config.org = '__tmp_org_placeholder__'; - config.auth = { - appId: Number(env.GH_APP_IDENTIFIER), - privateKey: env.GH_APP_SECRET_KEY.replace(/\\n/g, '\n'), - }; - config.project = { - node_id: env.ISSUES_PROJECT_NODE_ID || 'PVT_kwDOABVQ184AOGW8', - product_area_field_id: - env.PRODUCT_AREA_FIELD_ID || 'PVTSSF_lADOABVQ184AOGW8zgJEBno', - status_field_id: env.STATUS_FIELD_ID || 'PVTSSF_lADOABVQ184AOGW8zgI_7g0', - response_due_date_field_id: - env.RESPONSE_DUE_DATE_FIELD_ID || 'PVTF_lADOABVQ184AOGW8zgLLxGg', - }; + + for (const [envvar, value] of Object.entries(env)) { + // Find app configuration in env, grouping by number (GH_APP_1_FOO). + + const m = envvar.match(/^GH_APP_(\d+)_([A-Z_]+)$/); + if (m === null) { + continue; + } + + const n = parseInt(m[1], 10); + const noop = (x) => x; + const path_mod = new Map([ + ['ORG_SLUG', ['org', noop]], + ['IDENTIFIER', ['auth.appId', (x) => Number(x)]], + ['SECRET_KEY', ['auth.privateKey', (x) => x.replace(/\\n/g, '\n')]], + ['ISSUES_PROJECT_NODE_ID', ['project.node_id', noop]], + ['PRODUCT_AREA_FIELD_ID', ['project.product_area_field_id', noop]], + ['STATUS_FIELD_ID', ['project.status_field_id', noop]], + [ + 'RESPONSE_DUE_DATE_FIELD_ID', + ['project.response_due_date_field_id', noop], + ], + ]).get(m[2]); + + if (path_mod === undefined) { + continue; + } + + const path = path_mod[0] as string; + const mod = path_mod[1] as (x: any) => any; + const [first, second] = path.split('.'); + + const config = configs.getOrCreate(n); + if (second) { + config[first][second] = mod(value); + } else { + config[first] = mod(value); + } } - // Now convert them to (strongly-typed) apps now that we know the info is - // clean. + // Once all configs are in hand, validate them once so we can see all errors + // at once (vs. config whack-a-mole). + configs.validateAll(); + + // Convert configs to (strongly-typed) apps now that we know all values are + // present. return new GitHubApps(configs); } diff --git a/src/utils/db/getFailureMessages.test.ts b/src/utils/db/getFailureMessages.test.ts index 6e5c1417..6b6b65cb 100644 --- a/src/utils/db/getFailureMessages.test.ts +++ b/src/utils/db/getFailureMessages.test.ts @@ -1,3 +1,4 @@ +import { GETSENTRY_ORG } from '@/config'; import { SlackMessage } from '@/config/slackMessage'; import { ClientType } from '@api/github/clientType'; import { getClient } from '@api/github/getClient'; @@ -13,7 +14,7 @@ describe('getFailureMessages', function () { }); beforeEach(async function () { - octokit = await getClient(ClientType.App, 'getsentry'); + octokit = await getClient(ClientType.App, GETSENTRY_ORG); }); afterAll(async function () { diff --git a/src/utils/githubEventHelpers.test.ts b/src/utils/githubEventHelpers.test.ts index 6a29121d..e2bf4f81 100644 --- a/src/utils/githubEventHelpers.test.ts +++ b/src/utils/githubEventHelpers.test.ts @@ -3,7 +3,7 @@ import { GH_APPS } from '@/config'; import * as githubEventHelpers from './githubEventHelpers'; describe('githubEventHelpers', function () { - const app = GH_APPS.get('__tmp_org_placeholder__'); + const app = GH_APPS.get('Enterprise'); it('addIssueToGlobalIssuesProject should return project item id from project', async function () { const octokit = { diff --git a/src/webhooks/pubsub/index.test.ts b/src/webhooks/pubsub/index.test.ts new file mode 100644 index 00000000..f637adb2 --- /dev/null +++ b/src/webhooks/pubsub/index.test.ts @@ -0,0 +1,61 @@ +import { Reply } from 'fastify'; + +import { buildServer } from '@/buildServer'; +import { GETSENTRY_ORG } from '@/config'; +import { Fastify } from '@/types'; + +import { pubSubHandler } from './'; + +describe('projectsHandler', function () { + let fastify: Fastify; + + beforeEach(async function () { + fastify = await buildServer(false); + }); + + afterEach(async function () { + fastify.close(); + jest.clearAllMocks(); + }); + + async function callWith(name: string) { + const payload = Buffer(JSON.stringify({ name })).toString('base64'); + const request = { body: { message: { data: payload } } }; + const reply = { code: jest.fn(), send: jest.fn() }; + const cheeser = jest.fn(); + const breader = jest.fn(); + const funcMap = new Map([ + ['cheese', cheeser], + ['bread', breader], + ]); + await pubSubHandler(request, reply, funcMap); + return [reply.code.mock.calls[0][0], cheeser, breader]; + } + + describe('pubSubHandler', function () { + it('basically works', async function () { + const [code, cheeser, breader] = await callWith('cheese'); + expect(code).toBe(204); + expect(cheeser).toHaveBeenCalled(); + expect(breader).not.toHaveBeenCalled(); + }); + it('handles a different task', async function () { + const [code, cheeser, breader] = await callWith('bread'); + expect(code).toBe(204); + expect(cheeser).not.toHaveBeenCalled(); + expect(breader).toHaveBeenCalled(); + }); + it('sends 400 for unknown tasks', async function () { + const [code, cheeser, breader] = await callWith('wine'); + expect(code).toBe(400); + expect(cheeser).not.toHaveBeenCalled(); + expect(breader).not.toHaveBeenCalled(); + }); + it('is called twice, once for each app', async function () { + const [code, cheeser, breader] = await callWith('cheese'); + expect(cheeser.mock.calls.length).toBe(2); + expect(cheeser.mock.calls[0][0].org).toBe('Enterprise'); + expect(cheeser.mock.calls[1][0].org).toBe(GETSENTRY_ORG); + }); + }); +}); diff --git a/src/webhooks/pubsub/index.ts b/src/webhooks/pubsub/index.ts index 5b147291..3b50fc4e 100644 --- a/src/webhooks/pubsub/index.ts +++ b/src/webhooks/pubsub/index.ts @@ -1,14 +1,26 @@ +import { Octokit } from '@octokit/rest'; import * as Sentry from '@sentry/node'; import { FastifyReply, FastifyRequest } from 'fastify'; import moment from 'moment-timezone'; import { ClientType } from '@/api/github/clientType'; -import { GETSENTRY_ORG, GH_APPS } from '@/config'; -import { notifyProductOwnersForUntriagedIssues } from '@/webhooks/pubsub/slackNotifications'; +import { GH_APPS } from '@/config'; +import { GitHubApp } from '@/config/loadGitHubAppsFromEnvironment'; import { getClient } from '@api/github/getClient'; +import { notifyProductOwnersForUntriagedIssues } from './slackNotifications'; import { triggerStaleBot } from './stalebot'; +type FunctionMap = Map< + string, + (x: GitHubApp, y: Octokit, z: moment.Moment) => any +>; + +const DEFAULT_FUNCTION_MAP = new Map([ + ['stale-triage-notifier', notifyProductOwnersForUntriagedIssues], + ['stale-bot', triggerStaleBot], +]); + type PubSubPayload = { name: string; }; @@ -35,7 +47,8 @@ export const opts = { export const pubSubHandler = async ( request: FastifyRequest<{ Body: { message: { data: string } } }>, - reply: FastifyReply + reply: FastifyReply, + _funcMap: FunctionMap = DEFAULT_FUNCTION_MAP // for testing ) => { const tx = Sentry.startTransaction({ op: 'webhooks', @@ -45,30 +58,25 @@ export const pubSubHandler = async ( Buffer.from(request.body.message.data, 'base64').toString().trim() ); - let app, - octokit, - now, - code = 204; - let func = new Map([ - ['stale-triage-notifier', notifyProductOwnersForUntriagedIssues], - ['stale-bot', triggerStaleBot], - ]).get(payload.name); + const func = _funcMap.get(payload.name); - // Performing the following check seems to suppress GitHub's dynamic method - // call security warning. + // `if (func)` is not enough to fool CodeQL. // https://codeql.github.com/codeql-query-help/javascript/js-unvalidated-dynamic-method-call/ + // https://github.com/github/codeql/tree/main/javascript/ql/src/Security/CWE-754/examples + if (typeof func === 'function') { - app = GH_APPS.get('__tmp_org_placeholder__'); - octokit = await getClient(ClientType.App, GETSENTRY_ORG); - now = moment().utc(); + reply.code(204); + reply.send(); // before real work to avoid blocking + const now = moment().utc(); + for (const [org, app] of GH_APPS.apps) { + const octokit = await getClient(ClientType.App, org); + await func(app, octokit, now); + } } else { - func = async () => {}; // no-op - code = 400; + reply.code(400); + reply.send(); } - reply.code(code); - reply.send(); // Respond early to not block the webhook sender - await func(app, octokit, now); tx.finish(); }; diff --git a/src/webhooks/pubsub/slackNotifications.test.ts b/src/webhooks/pubsub/slackNotifications.test.ts index 961965a7..57dbe6d0 100644 --- a/src/webhooks/pubsub/slackNotifications.test.ts +++ b/src/webhooks/pubsub/slackNotifications.test.ts @@ -13,7 +13,7 @@ import { } from './slackNotifications'; describe('Triage Notification Tests', function () { - const app = GH_APPS.get('__tmp_org_placeholder__'); + const app = GH_APPS.get('Enterprise'); beforeAll(async function () { await db.migrate.latest(); diff --git a/src/webhooks/pubsub/slackNotifications.ts b/src/webhooks/pubsub/slackNotifications.ts index 28fc7b3a..3c3d1bd2 100644 --- a/src/webhooks/pubsub/slackNotifications.ts +++ b/src/webhooks/pubsub/slackNotifications.ts @@ -407,7 +407,7 @@ export const notifyProductOwnersForUntriagedIssues = async ( repo: string ): Promise => { const untriagedIssues = await octokit.paginate(octokit.issues.listForRepo, { - owner: GETSENTRY_ORG, + owner: app.org, repo, state: 'open', labels: WAITING_FOR_PRODUCT_OWNER_LABEL, diff --git a/src/webhooks/pubsub/stalebot.test.ts b/src/webhooks/pubsub/stalebot.test.ts index 254ad852..de188691 100644 --- a/src/webhooks/pubsub/stalebot.test.ts +++ b/src/webhooks/pubsub/stalebot.test.ts @@ -17,7 +17,7 @@ jest.mock('@/config', () => { }); describe('Stalebot Tests', function () { - const app = GH_APPS.get('__tmp_org_placeholder__'); + const app = GH_APPS.get('Enterprise'); const issueInfo = { labels: [], diff --git a/src/webhooks/pubsub/stalebot.ts b/src/webhooks/pubsub/stalebot.ts index 6ffe0f4e..bae637c2 100644 --- a/src/webhooks/pubsub/stalebot.ts +++ b/src/webhooks/pubsub/stalebot.ts @@ -85,7 +85,7 @@ export const triggerStaleBot = async ( // Get all open issues and pull requests that are Waiting for Community REPOS_TO_TRACK_FOR_STALEBOT.forEach(async (repo: string) => { const issues = await octokit.paginate(octokit.issues.listForRepo, { - owner: GETSENTRY_ORG, + owner: app.org, repo, state: 'open', labels: WAITING_FOR_COMMUNITY_LABEL,