From b2e5fdb1e1459270a8c04b57f2ac8f5415700d32 Mon Sep 17 00:00:00 2001 From: Alex Waibel Date: Sun, 29 Mar 2020 16:58:18 -0400 Subject: [PATCH 1/7] Fix incorrect error handling (#342) * Convert error handler to more modern class declaration. * Make GitHub and GitService actually throw errors rather than return them * Fix some broken unit and acceptance tests --- lib/ErrorHandler.js | 114 ++++++++++++++++++----------------- lib/GitHub.js | 4 +- lib/GitService.js | 4 +- test/acceptance/api.test.js | 99 +++++++++++++++++------------- test/helpers/sampleData.js | 7 +++ test/unit/lib/GitHub.test.js | 60 ++++++++++++++---- 6 files changed, 173 insertions(+), 115 deletions(-) diff --git a/lib/ErrorHandler.js b/lib/ErrorHandler.js index cfc45405..cc5734de 100644 --- a/lib/ErrorHandler.js +++ b/lib/ErrorHandler.js @@ -17,81 +17,83 @@ class ApiError { } } -const ErrorHandler = function () { - this.ERROR_MESSAGES = { - 'missing-input-secret': 'reCAPTCHA: The secret parameter is missing', - 'invalid-input-secret': 'reCAPTCHA: The secret parameter is invalid or malformed', - 'missing-input-response': 'reCAPTCHA: The response parameter is missing', - 'invalid-input-response': 'reCAPTCHA: The response parameter is invalid or malformed', - 'RECAPTCHA_MISSING_CREDENTIALS': 'Missing reCAPTCHA API credentials', - 'RECAPTCHA_FAILED_DECRYPT': 'Could not decrypt reCAPTCHA secret', - 'RECAPTCHA_CONFIG_MISMATCH': 'reCAPTCHA options do not match Staticman config', - 'PARSING_ERROR': 'Error whilst parsing config file', - 'GITHUB_AUTH_TOKEN_MISSING': 'The site requires a valid GitHub authentication token to be supplied in the `options[github-token]` field', - 'MISSING_CONFIG_BLOCK': 'Error whilst parsing Staticman config file' +class ErrorHandler { + constructor () { + this.ERROR_MESSAGES = { + 'missing-input-secret': 'reCAPTCHA: The secret parameter is missing', + 'invalid-input-secret': 'reCAPTCHA: The secret parameter is invalid or malformed', + 'missing-input-response': 'reCAPTCHA: The response parameter is missing', + 'invalid-input-response': 'reCAPTCHA: The response parameter is invalid or malformed', + 'RECAPTCHA_MISSING_CREDENTIALS': 'Missing reCAPTCHA API credentials', + 'RECAPTCHA_FAILED_DECRYPT': 'Could not decrypt reCAPTCHA secret', + 'RECAPTCHA_CONFIG_MISMATCH': 'reCAPTCHA options do not match Staticman config', + 'PARSING_ERROR': 'Error whilst parsing config file', + 'GITHUB_AUTH_TOKEN_MISSING': 'The site requires a valid GitHub authentication token to be supplied in the `options[github-token]` field', + 'MISSING_CONFIG_BLOCK': 'Error whilst parsing Staticman config file' + } + + this.ERROR_CODE_ALIASES = { + 'missing-input-secret': 'RECAPTCHA_MISSING_INPUT_SECRET', + 'invalid-input-secret': 'RECAPTCHA_INVALID_INPUT_SECRET', + 'missing-input-response': 'RECAPTCHA_MISSING_INPUT_RESPONSE', + 'invalid-input-response': 'RECAPTCHA_INVALID_INPUT_RESPONSE' + } } - this.ERROR_CODE_ALIASES = { - 'missing-input-secret': 'RECAPTCHA_MISSING_INPUT_SECRET', - 'invalid-input-secret': 'RECAPTCHA_INVALID_INPUT_SECRET', - 'missing-input-response': 'RECAPTCHA_MISSING_INPUT_RESPONSE', - 'invalid-input-response': 'RECAPTCHA_INVALID_INPUT_RESPONSE' + getErrorCode (error) { + return this.ERROR_CODE_ALIASES[error] || error } -} -ErrorHandler.prototype.getErrorCode = function (error) { - return this.ERROR_CODE_ALIASES[error] || error -} + getMessage (error) { + return this.ERROR_MESSAGES[error] + } -ErrorHandler.prototype.getMessage = function (error) { - return this.ERROR_MESSAGES[error] -} + log (err, instance) { + let parameters = {} + let prefix = '' -ErrorHandler.prototype.log = function (err, instance) { - let parameters = {} - let prefix = '' + if (instance) { + parameters = instance.getParameters() - if (instance) { - parameters = instance.getParameters() + prefix += `${parameters.username}/${parameters.repository}` + } - prefix += `${parameters.username}/${parameters.repository}` + console.log(`${prefix}`, err) } - console.log(`${prefix}`, err) -} + _save (errorCode, data = {}) { + const {err} = data -ErrorHandler.prototype._save = function (errorCode, data = {}) { - const {err} = data + if (err) { + err._smErrorCode = err._smErrorCode || errorCode - if (err) { - err._smErrorCode = err._smErrorCode || errorCode + // Re-wrap API request errors as these could expose + // request/response details that the user should not + // be allowed to see e.g. access tokens. + // `request-promise` is the primary offender here, + // but we similarly do not want others to leak too. + if ( + err instanceof StatusCodeError || + err instanceof RequestError + ) { + const statusCode = err.statusCode || err.code - // Re-wrap API request errors as these could expose - // request/response details that the user should not - // be allowed to see e.g. access tokens. - // `request-promise` is the primary offender here, - // but we similarly do not want others to leak too. - if ( - err instanceof StatusCodeError || - err instanceof RequestError - ) { - const statusCode = err.statusCode || err.code + return new ApiError(err.message, statusCode, err._smErrorCode) + } - return new ApiError(err.message, statusCode, err._smErrorCode) + return err } - return err - } + let payload = { + _smErrorCode: errorCode + } - let payload = { - _smErrorCode: errorCode - } + if (data.data) { + payload.data = data.data + } - if (data.data) { - payload.data = data.data + return payload } - - return payload } const errorHandler = new ErrorHandler() diff --git a/lib/GitHub.js b/lib/GitHub.js index 73dd1b3c..67558241 100644 --- a/lib/GitHub.js +++ b/lib/GitHub.js @@ -117,7 +117,7 @@ class GitHub extends GitService { console.log(err) } - return Promise.reject(errorHandler('GITHUB_WRITING_FILE', {err})) + return Promise.reject(errorHandler('GITHUB_WRITING_FILE')) }) } @@ -182,7 +182,7 @@ class GitHub extends GitService { try { return await super.readFile(filePath, getFullResponse) } catch (err) { - return errorHandler('GITHUB_READING_FILE', {err}) + throw errorHandler('GITHUB_READING_FILE', {err}) } } diff --git a/lib/GitService.js b/lib/GitService.js index 2add1c12..1cc79663 100644 --- a/lib/GitService.js +++ b/lib/GitService.js @@ -51,7 +51,7 @@ class GitService { try { content = Buffer.from(res.content, 'base64').toString() } catch (err) { - return errorHandler('GITHUB_READING_FILE', {err}) + throw errorHandler('GITHUB_READING_FILE', {err}) } try { @@ -85,7 +85,7 @@ class GitService { errorData.data = err.message } - return errorHandler('PARSING_ERROR', errorData) + throw errorHandler('PARSING_ERROR', errorData) } } diff --git a/test/acceptance/api.test.js b/test/acceptance/api.test.js index 925981ff..36758afc 100644 --- a/test/acceptance/api.test.js +++ b/test/acceptance/api.test.js @@ -26,7 +26,7 @@ afterAll(done => { }) describe('Connect endpoint', () => { - test('accepts the invitation if one is found and replies with "OK!"', () => { + test('accepts the invitation if one is found and replies with "OK!"', async () => { const invitationId = 123 const reqListInvititations = nock('https://api.github.com', { @@ -52,15 +52,13 @@ describe('Connect endpoint', () => { .patch(`/user/repository_invitations/${invitationId}`) .reply(204) - return request('/v2/connect/johndoe/foobar') - .then(response => { - expect(reqListInvititations.isDone()).toBe(true) - expect(reqAcceptInvitation.isDone()).toBe(true) - expect(response).toBe('OK!') - }) + let response = await request('/v2/connect/johndoe/foobar') + expect(reqListInvititations.isDone()).toBe(true) + expect(reqAcceptInvitation.isDone()).toBe(true) + expect(response).toBe('OK!') }) - test('returns a 404 and an error message if a matching invitation is not found', () => { + test('returns a 404 and an error message if a matching invitation is not found', async () => { const invitationId = 123 const reqListInvititations = nock('https://api.github.com', { reqheaders: { @@ -85,18 +83,21 @@ describe('Connect endpoint', () => { .patch(`/user/repository_invitations/${invitationId}`) .reply(204) - return request('/v2/connect/johndoe/foobar') - .catch(err => { + expect.assertions(4) + + try { + await request('/v2/connect/johndoe/foobar') + } catch (err) { expect(reqListInvititations.isDone()).toBe(true) expect(reqAcceptInvitation.isDone()).toBe(false) expect(err.response.body).toBe('Invitation not found') expect(err.statusCode).toBe(404) - }) + } }) }) describe('Entry endpoint', () => { - test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong site key)', () => { + test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong site key)', async () => { const data = Object.assign({}, helpers.getParameters(), { path: 'staticman.yml' }) @@ -136,23 +137,27 @@ describe('Entry endpoint', () => { } const formData = querystring.stringify(form) - return request({ - body: formData, - method: 'POST', - uri: `/v2/entry/${data.username}/${data.repository}/${data.branch}/${data.property}`, - headers: { - 'content-type': 'application/x-www-form-urlencoded' - } - }).catch((response) => { + expect.assertions(3) + + try { + await request({ + body: formData, + method: 'POST', + uri: `/v2/entry/${data.username}/${data.repository}/${data.branch}/${data.property}`, + headers: { + 'content-type': 'application/x-www-form-urlencoded' + } + }) + } catch(response) { const error = JSON.parse(response.error) expect(error.success).toBe(false) expect(error.errorCode).toBe('RECAPTCHA_CONFIG_MISMATCH') expect(error.message).toBe('reCAPTCHA options do not match Staticman config') - }) + } }) - test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong secret)', () => { + test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong secret)', async () => { const data = Object.assign({}, helpers.getParameters(), { path: 'staticman.yml' }) @@ -192,23 +197,27 @@ describe('Entry endpoint', () => { } const formData = querystring.stringify(form) - return request({ - body: formData, - method: 'POST', - uri: '/v2/entry/johndoe/foobar/master/comments', - headers: { - 'content-type': 'application/x-www-form-urlencoded' - } - }).catch(response => { + expect.assertions(3) + + try { + await request({ + body: formData, + method: 'POST', + uri: '/v2/entry/johndoe/foobar/master/comments', + headers: { + 'content-type': 'application/x-www-form-urlencoded' + } + }) + } catch (response) { const error = JSON.parse(response.error) expect(error.success).toBe(false) expect(error.errorCode).toBe('RECAPTCHA_CONFIG_MISMATCH') expect(error.message).toBe('reCAPTCHA options do not match Staticman config') - }) + } }) - test('outputs a MISSING_CONFIG_BLOCK error if the site config is malformed', () => { + test('outputs a PARSING_ERROR error if the site config is malformed', async () => { const data = Object.assign({}, helpers.getParameters(), { path: 'staticman.yml' }) @@ -243,20 +252,24 @@ describe('Entry endpoint', () => { } const formData = querystring.stringify(form) - return request({ - body: formData, - method: 'POST', - uri: '/v2/entry/johndoe/foobar/master/comments', - headers: { - 'content-type': 'application/x-www-form-urlencoded' - } - }).catch(response => { + expect.assertions(4) + + try { + await request({ + body: formData, + method: 'POST', + uri: '/v2/entry/johndoe/foobar/master/comments', + headers: { + 'content-type': 'application/x-www-form-urlencoded' + } + }) + } catch (response) { const error = JSON.parse(response.error) expect(error.success).toBe(false) - expect(error.errorCode).toBe('MISSING_CONFIG_BLOCK') - expect(error.message).toBe('Error whilst parsing Staticman config file') + expect(error.errorCode).toBe('PARSING_ERROR') + expect(error.message).toBe('Error whilst parsing config file') expect(error.rawError).toBeDefined() - }) + } }) }) diff --git a/test/helpers/sampleData.js b/test/helpers/sampleData.js index 35aab01f..7b3955ca 100644 --- a/test/helpers/sampleData.js +++ b/test/helpers/sampleData.js @@ -150,6 +150,13 @@ module.exports.config3 = `comments: siteKey: "123456789" secret: "@reCaptchaSecret@"` +module.exports.configInvalidYML = `invalid: +- x +y + foo +bar +` + module.exports.prBody1 = `Dear human, Here's a new entry for your approval. :tada: diff --git a/test/unit/lib/GitHub.test.js b/test/unit/lib/GitHub.test.js index d1e65d52..24bc5098 100644 --- a/test/unit/lib/GitHub.test.js +++ b/test/unit/lib/GitHub.test.js @@ -100,6 +100,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.readFile(filePath) } catch (err) { @@ -109,18 +111,46 @@ describe('GitHub interface', () => { expect(scope.isDone()).toBe(true) }) - test('returns an error if parsing fails for the given file', async () => { + test('returns an error if the config file cannot be read', async () => { const filePath = 'path/to/file.yml' const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.readFile(filePath) } catch (err) { - expect(err._smErrorCode).toEqual('PARSING_ERROR') + expect(err._smErrorCode).toEqual('GITHUB_READING_FILE') expect(err.message).toBeDefined() } }) + test('returns an error if the config file cannot be parsed', async () => { + const scope = nock((/api\.github\.com/), { + reqheaders: { + authorization: 'token '.concat('1q2w3e4r') + } + }) + .get('/repos/johndoe/foobar/contents/path/to/file.yml?ref=master') + .reply(200, { + content: btoa(sampleData.configInvalidYML) + }) + + const filePath = 'path/to/file.yml' + const githubInstance = await new GitHub(req.params) + + expect.assertions(3) + + try { + await githubInstance.readFile(filePath) + } catch (err) { + expect(err._smErrorCode).toEqual('PARSING_ERROR') + expect(err.message).toBeDefined() + } + + expect(scope.isDone()).toBe(true) + }) + test('reads a YAML file and returns its parsed contents', async () => { const filePath = 'path/to/file.yml' const parsedConfig = yaml.safeLoad(sampleData.config1, 'utf8') @@ -284,12 +314,12 @@ describe('GitHub interface', () => { } }) .put('/repos/johndoe/foobar/contents/path/to/file.txt') - .reply(200, { - number: 123 - }) + .replyWithError('An error') const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.writeFile( options.path, @@ -361,9 +391,11 @@ describe('GitHub interface', () => { id: 1 }) + expect.assertions(5) + const githubInstance = await new GitHub(req.params) - await githubInstance.writeFileAndSendReview( + const data = await githubInstance.writeFileAndSendReview( options.path, options.content, options.newBranch, @@ -371,12 +403,12 @@ describe('GitHub interface', () => { options.commitBody ) - setTimeout(() => { - expect(branchScope.isDone()).toBe(true) - expect(refsScope.isDone()).toBe(true) - expect(fileScope.isDone()).toBe(true) - expect(pullScope.isDone()).toBe(true) - }) + expect(data).toEqual({"id": 1}) + + expect(branchScope.isDone()).toBe(true) + expect(refsScope.isDone()).toBe(true) + expect(fileScope.isDone()).toBe(true) + expect(pullScope.isDone()).toBe(true) }) // TODO: Figure out why this works with no mocks @@ -401,6 +433,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.writeFileAndSendReview( options.path, @@ -448,6 +482,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.getCurrentUser() } catch (err) { From 7e0284017c350c6b4157a04903d47781f023face Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Tue, 13 Oct 2020 19:53:29 -0400 Subject: [PATCH 2/7] Enable sending email notifications when using GitLab (as opposed to GitHub). - Modify handlePR.js to instantiate a generic GitService instead of GitHub. - Modify GitHub.js so that a blank (Staticman) version number can resolve to GitHub token authorization (as the version number is not available when instantiating GitHub from handlePR). - Modify server.js to provide v3 service-specific "webhook" endpoints. Retain usage of the express-github-webhook module for GitHub. - Update tests and add test cases for GitLab. - Add new webhook controller. --- controllers/handlePR.js | 72 +++++++++-- controllers/webhook.js | 20 +++ lib/GitHub.js | 7 +- server.js | 37 ++++-- test/unit/controllers/handlePR.test.js | 166 +++++++++++++++++-------- test/unit/lib/GitHub.test.js | 16 +++ 6 files changed, 244 insertions(+), 74 deletions(-) create mode 100644 controllers/webhook.js diff --git a/controllers/handlePR.js b/controllers/handlePR.js index 87aeacde..a36dea03 100644 --- a/controllers/handlePR.js +++ b/controllers/handlePR.js @@ -1,7 +1,7 @@ 'use strict' const config = require('../config') -const GitHub = require('../lib/GitHub') +const gitFactory = require('../lib/GitServiceFactory') const Staticman = require('../lib/Staticman') module.exports = async (repo, data) => { @@ -9,18 +9,55 @@ module.exports = async (repo, data) => { ? require('universal-analytics')(config.get('analytics.uaTrackingId')) : null - if (!data.number) { - return + /* + * Unfortunately, all we have available to us at this point is the request body (as opposed to + * the full request). Meaning, we don't have the :service portion of the request URL available. + * As such, switch between GitHub and GitLab using the repo URL. For example: + * "url": "https://api.github.com/repos/hispanic/staticman-test" + * "url": "git@gitlab.com:ihispanic/staticman-test.git" + */ + const calcIsGitHub = function (data) { + return data.repository.url.includes('github.com') + } + const calcIsGitLab = function (data) { + return data.repository.url.includes('gitlab.com') } - const github = await new GitHub({ - username: data.repository.owner.login, - repository: data.repository.name, - version: '1' - }) + /* + * Because we don't have the full request available to us here, we can't set the branch and + * (Staticman) version options. Fortunately, they aren't critical. + */ + const unknownBranch = 'UNKNOWN' + const unknownVersion = '' + + let gitService = null + let mergeReqNbr = null + if (calcIsGitHub(data)) { + gitService = await gitFactory.create('github', { + branch: unknownBranch, + repository: data.repository.name, + username: data.repository.owner.login, + version: unknownVersion + }) + mergeReqNbr = data.number + } else if (calcIsGitLab(data)) { + gitService = await gitFactory.create('gitlab', { + branch: unknownBranch, + repository: data.repository.name, + username: data.user.username, + version: unknownVersion + }) + mergeReqNbr = data.object_attributes.iid + } else { + return null + } + + if (!mergeReqNbr) { + return + } try { - let review = await github.getReview(data.number) + let review = await gitService.getReview(mergeReqNbr) if (review.sourceBranch.indexOf('staticman_')) { return null } @@ -30,6 +67,10 @@ module.exports = async (repo, data) => { } if (review.state === 'merged') { + /* + * The "staticman_notification" comment section of the comment pull/merge request only + * exists if notifications were enabled at the time the pull/merge request was created. + */ const bodyMatch = review.body.match(/(?:.*?)(?:.*?)/i) if (bodyMatch && (bodyMatch.length === 2)) { @@ -48,7 +89,18 @@ module.exports = async (repo, data) => { if (ua) { ua.event('Hooks', 'Delete branch').send() } - return github.deleteBranch(review.sourceBranch) + + let result = null + /* + * Only necessary for GitHub, as GitLab automatically deletes the backing branch for the + * pull/merge request. For GitHub, this will throw the following error if the branch has + * already been deleted: + * "UnhandledPromiseRejectionWarning: HttpError: Reference does not exist" if branch already deleted. + */ + if (calcIsGitHub(data)) { + result = gitService.deleteBranch(review.sourceBranch) + } + return result } catch (e) { console.log(e.stack || e) diff --git a/controllers/webhook.js b/controllers/webhook.js new file mode 100644 index 00000000..95438737 --- /dev/null +++ b/controllers/webhook.js @@ -0,0 +1,20 @@ +'use strict' + +const handlePR = require('./handlePR') + +module.exports = async (req, res, next) => { + switch (req.params.service) { + case 'gitlab': + handlePR(req.params.repository, req.body) + + res.status(200).send({ + success: true + }) + + break + default: + res.status(500).send({ + error: 'Unexpected service found.' + }) + } +} diff --git a/lib/GitHub.js b/lib/GitHub.js index 67558241..16814605 100644 --- a/lib/GitHub.js +++ b/lib/GitHub.js @@ -19,7 +19,12 @@ class GitHub extends GitService { const isAppAuth = config.get('githubAppID') && config.get('githubPrivateKey') const isLegacyAuth = config.get('githubToken') && - ['1', '2'].includes(options.version) + /* + * Allowing/requiring the Staticman version to bleed-through to here is problematic, as + * it may not always be available to the caller (e.g., controllers/handlePR.js). To allow + * for this, we allow for a blank version value. + */ + ['1', '2', ''].includes(options.version) let authToken diff --git a/server.js b/server.js index 003af118..e436714e 100644 --- a/server.js +++ b/server.js @@ -13,7 +13,8 @@ class StaticmanAPI { auth: require('./controllers/auth'), handlePR: require('./controllers/handlePR'), home: require('./controllers/home'), - process: require('./controllers/process') + process: require('./controllers/process'), + webhook: require('./controllers/webhook') } this.server = express() @@ -23,7 +24,7 @@ class StaticmanAPI { // type: '*' })) - this.initialiseWebhookHandler() + this.initialiseGitHubWebhookHandler() this.initialiseCORS() this.initialiseBruteforceProtection() this.initialiseRoutes() @@ -96,6 +97,14 @@ class StaticmanAPI { this.controllers.auth ) + this.server.post( + '/v:version/webhook/:service/', + this.bruteforce.prevent, + this.requireApiVersion([3]), + this.requireService(['gitlab']), + this.controllers.webhook + ) + // Route: root this.server.get( '/', @@ -103,14 +112,26 @@ class StaticmanAPI { ) } - initialiseWebhookHandler () { - const webhookHandler = GithubWebHook({ - path: '/v1/webhook' - }) + initialiseGitHubWebhookHandler () { + /* + * The express-github-webhook module is frustrating, as it only allows for a simplistic match + * for equality against one path. No string patterns (e.g., /v1?3?/webhook(/github)?). No + * regular expressions (e.g., /\/v[13]\/webhook(?:\/github)?/). As such, create one instance + * of the module per supported path. This won't scale well as Staticman API versions are added. + */ + for (const onePath of ['/v1/webhook', '/v3/webhook/github']) { + const webhookHandler = GithubWebHook({ + path: onePath + }) - webhookHandler.on('pull_request', this.controllers.handlePR) + /* + * Frustratingly, the express-github-webhook module only passes along body.data (and the + * repository name) to the callback, not the whole request. + */ + webhookHandler.on('pull_request', this.controllers.handlePR) - this.server.use(webhookHandler) + this.server.use(webhookHandler) + } } requireApiVersion (versions) { diff --git a/test/unit/controllers/handlePR.test.js b/test/unit/controllers/handlePR.test.js index 786525bc..e51964c1 100644 --- a/test/unit/controllers/handlePR.test.js +++ b/test/unit/controllers/handlePR.test.js @@ -27,9 +27,11 @@ beforeEach(() => { }) describe('HandlePR controller', () => { - test('ignores pull requests from branches not prefixed with `staticman_`', async () => { - const pr = { - number: 123, + test.each([ + ['github'], + ['gitlab'], + ])('ignores pull requests from branches not prefixed with `staticman_` - %s', async (service) => { + let pr = { title: 'Some random PR', body: 'Unrelated review body', head: { @@ -40,35 +42,31 @@ describe('HandlePR controller', () => { }, merged: false, repository: { - name: req.params.repository, - owner: { - login: req.params.username - } + name: req.params.repository }, state: 'open' } + modifyPrDataForService(pr, service, req) const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.base.ref) - const mockGetReview = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - getReview: mockGetReview - } - }) - }) + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab) const handlePR = require('./../../../controllers/handlePR') let response = await handlePR(req.params.repository, pr) - expect(mockGetReview).toHaveBeenCalledTimes(1) + assertGetReviewCalls(service, mockGetReviewGitHub, mockGetReviewGitLab) expect(response).toBe(null) }) describe('processes notifications if the pull request has been merged', () => { - test('do nothing if PR body doesn\'t match template', async () => { - const pr = { + test.each([ + ['github'], + ['gitlab'], + ])('do nothing if PR body doesn\'t match template - %s', async (service) => { + let pr = { number: 123, title: 'Add Staticman data', body: sampleData.prBody2, @@ -87,29 +85,27 @@ describe('HandlePR controller', () => { }, state: 'open' } + modifyPrDataForService(pr, service, req) const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.base.ref) - const mockGetReview = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) const mockDeleteBranch = jest.fn() - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - getReview: mockGetReview, - deleteBranch: mockDeleteBranch - } - }) - }) + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) const handlePR = require('./../../../controllers/handlePR') await handlePR(req.params.repository, pr) - expect(mockGetReview).toHaveBeenCalledTimes(1) + assertGetReviewCalls(service, mockGetReviewGitHub, mockGetReviewGitLab) expect(mockDeleteBranch).not.toHaveBeenCalled() }) - test('abort and return an error if `processMerge` fails', async () => { - const pr = { + test.each([ + ['github'], + ['gitlab'], + ])('abort and return an error if `processMerge` fails - %s', async (service) => { + let pr = { number: 123, title: 'Add Staticman data', body: sampleData.prBody1, @@ -128,19 +124,14 @@ describe('HandlePR controller', () => { }, state: 'closed' } + modifyPrDataForService(pr, service, req) const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.base.ref) - const mockGetReview = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) const mockDeleteBranch = jest.fn() - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - getReview: mockGetReview, - deleteBranch: mockDeleteBranch - } - }) - }) + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) const errorMessage = 'some error' @@ -150,12 +141,12 @@ describe('HandlePR controller', () => { const handlePR = require('./../../../controllers/handlePR') - expect.assertions(4) + expect.assertions(5) try { await handlePR(req.params.repository, pr) } catch (e) { expect(e).toBe(errorMessage) - expect(mockGetReview).toHaveBeenCalledTimes(1) + assertGetReviewCalls(service, mockGetReviewGitHub, mockGetReviewGitLab) // expect(mockSetConfigPathFn.mock.calls.length).toBe(1) // expect(mockProcessMergeFn.mock.calls.length).toBe(1) expect(mockSetConfigPathFn).toHaveBeenCalledTimes(1) @@ -163,8 +154,11 @@ describe('HandlePR controller', () => { } }) - test('delete the branch if the pull request is closed', async () => { - const pr = { + test.each([ + ['github'], + ['gitlab'], + ])('delete the branch if the pull request is closed - %s', async (service) => { + let pr = { number: 123, title: 'Add Staticman data', body: sampleData.prBody1, @@ -183,28 +177,90 @@ describe('HandlePR controller', () => { }, state: 'closed' } + modifyPrDataForService(pr, service, req) const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.base.ref) const mockDeleteBranch = jest.fn() - const mockGetReview = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) + const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - deleteBranch: mockDeleteBranch, - getReview: mockGetReview - } - }) - }) + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) const handlePR = require('./../../../controllers/handlePR') await handlePR(req.params.repository, pr) - expect(mockGetReview).toHaveBeenCalledTimes(1) - expect(mockGetReview.mock.calls[0][0]).toEqual(123) - expect(mockDeleteBranch).toHaveBeenCalledTimes(1) + assertGetReviewCalls(service, mockGetReviewGitHub, mockGetReviewGitLab) + if (service === 'github') { + expect(mockGetReviewGitHub.mock.calls[0][0]).toEqual(123) + expect(mockDeleteBranch).toHaveBeenCalledTimes(1) + } else if (service === 'gitlab') { + expect(mockGetReviewGitLab.mock.calls[0][0]).toEqual(123) + expect(mockDeleteBranch).toHaveBeenCalledTimes(0) + } expect(mockSetConfigPathFn.mock.calls.length).toBe(1) expect(mockProcessMergeFn.mock.calls.length).toBe(1) }) }) }) + +/** + * Avoid code duplication in the above test cases. + */ +const modifyPrDataForService = function (prData, service, req) { + if (service === 'github') { + prData.number = 123 + prData.repository.url = 'https://api.github.com/repos/' + req.params.username + '/' + req.params.repository + prData.repository.owner = { + login: req.params.username + } + } else if (service === 'gitlab') { + prData.object_attributes = { + iid: 123 + } + prData.repository.url = 'git@gitlab.com:' + req.params.username + '/' + req.params.repository + '.git' + prData.user = { + username: req.params.username + } + } +} + +/** + * Avoid code duplication in the above test cases. + */ +const mockGitModules = function (mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) { + jest.mock('../../../lib/GitHub', () => { + return jest.fn().mockImplementation(() => { + let result = { + getReview: mockGetReviewGitHub, + } + if (mockDeleteBranch) { + result.deleteBranch = mockDeleteBranch + } + return result + }) + }) + jest.mock('../../../lib/GitLab', () => { + return jest.fn().mockImplementation(() => { + let result = { + getReview: mockGetReviewGitLab, + } + if (mockDeleteBranch) { + result.deleteBranch = mockDeleteBranch + } + return result + }) + }) +} + +/** + * Avoid code duplication in the above test cases. + */ +const assertGetReviewCalls = function (service, mockGetReviewGitHub, mockGetReviewGitLab) { + if (service === 'github') { + expect(mockGetReviewGitHub).toHaveBeenCalledTimes(1) + expect(mockGetReviewGitLab).toHaveBeenCalledTimes(0) + } else if (service === 'gitlab') { + expect(mockGetReviewGitHub).toHaveBeenCalledTimes(0) + expect(mockGetReviewGitLab).toHaveBeenCalledTimes(1) + } +} diff --git a/test/unit/lib/GitHub.test.js b/test/unit/lib/GitHub.test.js index 24bc5098..f00f03ef 100644 --- a/test/unit/lib/GitHub.test.js +++ b/test/unit/lib/GitHub.test.js @@ -37,6 +37,22 @@ describe('GitHub interface', () => { expect(scope.isDone()).toBe(true) }) + test('authenticates with the GitHub API using a personal access token when version blank', async () => { + const scope = nock((/api\.github\.com/), { + reqheaders: { + authorization: 'token '.concat('1q2w3e4r') + } + }) + .get('/user/repository_invitations') + .reply(200) + + let paramsWithoutVersion = Object.assign({}, req.params) + paramsWithoutVersion.version = '' + const githubInstance = await new GitHub(paramsWithoutVersion) + await githubInstance.api.repos.listInvitationsForAuthenticatedUser(); + expect(scope.isDone()).toBe(true) + }) + test('authenticates with the GitHub API using an OAuth token', async () => { const scope = nock((/api\.github\.com/), { reqheaders: { From ef25d7aac15ce99d8309e47a10344a8151c4e4a9 Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Tue, 13 Oct 2020 21:25:44 -0400 Subject: [PATCH 3/7] Remove extraneous commas. --- test/unit/controllers/handlePR.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/controllers/handlePR.test.js b/test/unit/controllers/handlePR.test.js index e51964c1..2329bd2a 100644 --- a/test/unit/controllers/handlePR.test.js +++ b/test/unit/controllers/handlePR.test.js @@ -231,7 +231,7 @@ const mockGitModules = function (mockGetReviewGitHub, mockGetReviewGitLab, mockD jest.mock('../../../lib/GitHub', () => { return jest.fn().mockImplementation(() => { let result = { - getReview: mockGetReviewGitHub, + getReview: mockGetReviewGitHub } if (mockDeleteBranch) { result.deleteBranch = mockDeleteBranch @@ -242,7 +242,7 @@ const mockGitModules = function (mockGetReviewGitHub, mockGetReviewGitLab, mockD jest.mock('../../../lib/GitLab', () => { return jest.fn().mockImplementation(() => { let result = { - getReview: mockGetReviewGitLab, + getReview: mockGetReviewGitLab } if (mockDeleteBranch) { result.deleteBranch = mockDeleteBranch From f5f52f8ef1713d11ffd704bee944807dfc7bf3d6 Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Mon, 19 Oct 2020 21:02:00 -0400 Subject: [PATCH 4/7] Flesh out the (GitLab) webhook handler with webhook event filtering, error handling, and webhook request authentication. - Add support in the Staticman JSON configuration for GitLab and GitHub webhook request authentication via "gitlabWebhookSecret" and "githubWebhookSecret" properties. - In the handlePR controller, pass real values for the "branch" to the GitFactory. - In the handlePR controller, pass a value for "username" to the GitFactory that identifies the repo owner, not the submitter of the merge/pull request. - In the handlePR controller, when appropriate, return helpful error messages instead of simply null. - In the handlePR controller, fix the "staticman_" source branch test. - In the handlePR controller, clean up the analytics events invocations, as the logic is doing more than just deleting the source branch. - Improve server.js to silence UnhandledPromiseRejectionWarning messages. - Update and add unit tests. --- config.js | 12 ++ controllers/handlePR.js | 127 +++++++++++------- controllers/webhook.js | 62 ++++++++- server.js | 18 ++- test/unit/controllers/handlePR.test.js | 158 ++++++++++++++++++++--- test/unit/controllers/webhook.test.js | 172 +++++++++++++++++++++++++ 6 files changed, 478 insertions(+), 71 deletions(-) create mode 100644 test/unit/controllers/webhook.test.js diff --git a/config.js b/config.js index 99411ba6..08c8820d 100644 --- a/config.js +++ b/config.js @@ -84,6 +84,12 @@ const schema = { default: null, env: 'GITHUB_TOKEN' }, + githubWebhookSecret: { + doc: 'Token to verify that webhook requests are from GitHub', + format: String, + default: null, + env: 'GITHUB_WEBHOOK_SECRET' + }, gitlabAccessTokenUri: { doc: 'URI for the GitLab authentication provider.', format: String, @@ -102,6 +108,12 @@ const schema = { default: null, env: 'GITLAB_TOKEN' }, + gitlabWebhookSecret: { + doc: 'Token to verify that webhook requests are from GitLab', + format: String, + default: null, + env: 'GITLAB_WEBHOOK_SECRET' + }, port: { doc: 'The port to bind the application to.', format: 'port', diff --git a/controllers/handlePR.js b/controllers/handlePR.js index a36dea03..ac34f256 100644 --- a/controllers/handlePR.js +++ b/controllers/handlePR.js @@ -13,8 +13,8 @@ module.exports = async (repo, data) => { * Unfortunately, all we have available to us at this point is the request body (as opposed to * the full request). Meaning, we don't have the :service portion of the request URL available. * As such, switch between GitHub and GitLab using the repo URL. For example: - * "url": "https://api.github.com/repos/hispanic/staticman-test" - * "url": "git@gitlab.com:ihispanic/staticman-test.git" + * "url": "https://api.github.com/repos/foo/staticman-test" + * "url": "git@gitlab.com:foo/staticman-test.git" */ const calcIsGitHub = function (data) { return data.repository.url.includes('github.com') @@ -24,73 +24,91 @@ module.exports = async (repo, data) => { } /* - * Because we don't have the full request available to us here, we can't set the branch and - * (Staticman) version options. Fortunately, they aren't critical. + * Because we don't have the full request available to us here, we can't set the (Staticman) + * version option. Fortunately, it isn't critical. */ - const unknownBranch = 'UNKNOWN' const unknownVersion = '' let gitService = null let mergeReqNbr = null if (calcIsGitHub(data)) { gitService = await gitFactory.create('github', { - branch: unknownBranch, + branch: data.pull_request.base.ref, repository: data.repository.name, username: data.repository.owner.login, version: unknownVersion }) mergeReqNbr = data.number } else if (calcIsGitLab(data)) { + const repoUrl = data.repository.url + const repoUsername = repoUrl.substring(repoUrl.indexOf(':') + 1, repoUrl.indexOf('/')) gitService = await gitFactory.create('gitlab', { - branch: unknownBranch, + branch: data.object_attributes.target_branch, repository: data.repository.name, - username: data.user.username, + username: repoUsername, version: unknownVersion }) mergeReqNbr = data.object_attributes.iid } else { - return null + return Promise.reject(new Error('Unable to determine service.')) } if (!mergeReqNbr) { - return + return Promise.reject(new Error('No pull/merge request number found.')) } - try { - let review = await gitService.getReview(mergeReqNbr) - if (review.sourceBranch.indexOf('staticman_')) { - return null - } + let review = await gitService.getReview(mergeReqNbr).catch((error) => { + return Promise.reject(new Error(error)) + }) - if (review.state !== 'merged' && review.state !== 'closed') { - return null - } + if (review.sourceBranch.indexOf('staticman_') < 0) { + /* + * Don't throw an error here, as we might receive "real" (non-bot) pull requests for files + * other than Staticman-processed comments. + */ + return null + } - if (review.state === 'merged') { - /* - * The "staticman_notification" comment section of the comment pull/merge request only - * exists if notifications were enabled at the time the pull/merge request was created. - */ - const bodyMatch = review.body.match(/(?:.*?)(?:.*?)/i) - - if (bodyMatch && (bodyMatch.length === 2)) { - try { - const parsedBody = JSON.parse(bodyMatch[1]) - const staticman = await new Staticman(parsedBody.parameters) - - staticman.setConfigPath(parsedBody.configPath) - staticman.processMerge(parsedBody.fields, parsedBody.options) - } catch (err) { - return Promise.reject(err) + if (review.state !== 'merged' && review.state !== 'closed') { + /* + * Don't throw an error here, as we'll regularly receive webhook calls whenever a pull/merge + * request is opened, not just merged/closed. + */ + return null + } + + if (review.state === 'merged') { + /* + * The "staticman_notification" comment section of the comment pull/merge request only + * exists if notifications were enabled at the time the pull/merge request was created. + */ + const bodyMatch = review.body.match(/(?:.*?)(?:.*?)/i) + + if (bodyMatch && (bodyMatch.length === 2)) { + try { + const parsedBody = JSON.parse(bodyMatch[1]) + const staticman = await new Staticman(parsedBody.parameters) + + staticman.setConfigPath(parsedBody.configPath) + + await staticman.processMerge(parsedBody.fields, parsedBody.options) + // staticman.processMerge(parsedBody.fields, parsedBody.options).then(data => { + // if (ua) { + // ua.event('Hooks', 'Create/notify mailing list').send() + // } + // }) + if (ua) { + ua.event('Hooks', 'Create/notify mailing list').send() + } + } catch (err) { + if (ua) { + ua.event('Hooks', 'Create/notify mailing list error').send() } - } - } - if (ua) { - ua.event('Hooks', 'Delete branch').send() + return Promise.reject(err) + } } - let result = null /* * Only necessary for GitHub, as GitLab automatically deletes the backing branch for the * pull/merge request. For GitHub, this will throw the following error if the branch has @@ -98,16 +116,29 @@ module.exports = async (repo, data) => { * "UnhandledPromiseRejectionWarning: HttpError: Reference does not exist" if branch already deleted. */ if (calcIsGitHub(data)) { - result = gitService.deleteBranch(review.sourceBranch) - } - return result - } catch (e) { - console.log(e.stack || e) + // await gitService.deleteBranch(review.sourceBranch).then(data => { + // if (ua) { + // ua.event('Hooks', 'Delete branch').send() + // } + // }).catch(err => { + // if (ua) { + // ua.event('Hooks', 'Delete branch error').send() + // } - if (ua) { - ua.event('Hooks', 'Delete branch error').send() - } + // return Promise.reject(err) + // }) + try { + await gitService.deleteBranch(review.sourceBranch) + if (ua) { + ua.event('Hooks', 'Delete branch').send() + } + } catch (err) { + if (ua) { + ua.event('Hooks', 'Delete branch error').send() + } - return Promise.reject(e) + return Promise.reject(err) + } + } } } diff --git a/controllers/webhook.js b/controllers/webhook.js index 95438737..ab0b27f8 100644 --- a/controllers/webhook.js +++ b/controllers/webhook.js @@ -1,20 +1,70 @@ 'use strict' +const path = require('path') +const config = require(path.join(__dirname, '/../config')) const handlePR = require('./handlePR') module.exports = async (req, res, next) => { switch (req.params.service) { case 'gitlab': - handlePR(req.params.repository, req.body) + let errorMsg = null + let event = req.headers['x-gitlab-event'] - res.status(200).send({ - success: true - }) + if (!event) { + errorMsg = 'No event found in the request' + } else { + if (event === 'Merge Request Hook') { + const webhookSecretExpected = config.get('gitlabWebhookSecret') + const webhookSecretSent = req.headers['x-gitlab-token'] + + let reqAuthenticated = true + if (webhookSecretExpected) { + reqAuthenticated = false + if (!webhookSecretSent) { + errorMsg = 'No secret found in the webhook request' + } else if (webhookSecretExpected === webhookSecretSent) { + /* + * Whereas GitHub uses the webhook secret to sign the request body, GitLab does not. + * As such, just check that the received secret equals the expected value. + */ + reqAuthenticated = true + } else { + errorMsg = 'Unable to verify authenticity of request' + } + } + + if (reqAuthenticated) { + await handlePR(req.params.repository, req.body).catch((error) => { + console.error(error.stack || error) + errorMsg = error.message + }) + // try { + // await handlePR(req.params.repository, req.body) + // } catch (error) { + // console.error(error.stack || error) + // errorMsg = error.message + // } + } + } + } + + if (errorMsg !== null) { + res.status(400).send({ + error: errorMsg + }) + } else { + res.status(200).send({ + success: true + }) + } break default: - res.status(500).send({ - error: 'Unexpected service found.' + res.status(400).send({ + /* + * We are expecting GitHub webhooks to be handled by the express-github-webhook module. + */ + error: 'Unexpected service specified.' }) } } diff --git a/server.js b/server.js index e436714e..24b80a88 100644 --- a/server.js +++ b/server.js @@ -121,14 +121,28 @@ class StaticmanAPI { */ for (const onePath of ['/v1/webhook', '/v3/webhook/github']) { const webhookHandler = GithubWebHook({ - path: onePath + path: onePath, + secret: config.get('githubWebhookSecret') }) /* + * Wrap the handlePR callback so that we can catch any errors thrown and log them. This + * also has the benefit of eliminating noisy UnhandledPromiseRejectionWarning messages. + * * Frustratingly, the express-github-webhook module only passes along body.data (and the * repository name) to the callback, not the whole request. */ - webhookHandler.on('pull_request', this.controllers.handlePR) + const handlePrWrapper = function (repo, data) { + this.controllers.handlePR(repo, data).catch((error) => { + console.error(error) + /* + * Unfortunately, the express-github-webhook module returns a 200 (success) regardless + * of any errors raised in the downstream handler. So, all we can do is log errors. + */ + }) + }.bind(this) + + webhookHandler.on('pull_request', handlePrWrapper) this.server.use(webhookHandler) } diff --git a/test/unit/controllers/handlePR.test.js b/test/unit/controllers/handlePR.test.js index 2329bd2a..aa898e79 100644 --- a/test/unit/controllers/handlePR.test.js +++ b/test/unit/controllers/handlePR.test.js @@ -18,15 +18,135 @@ jest.mock('../../../lib/Staticman', () => { beforeEach(() => { mockSetConfigPathFn = jest.fn() - mockProcessMergeFn = jest.fn() + mockProcessMergeFn = jest.fn().mockResolvedValue({ }) req = helpers.getMockRequest() - res = helpers.getMockResponse() jest.resetAllMocks() jest.resetModules() }) describe('HandlePR controller', () => { + test.each([ + ['github'], + ['gitlab'], + ])('abort and return an error if unable to determine service - %s', async (service) => { + let pr = { + title: 'Some random PR', + body: 'Unrelated review body', + head: { + ref: 'some-other-branch' + }, + pull_request: { + base: { + ref: 'master' + } + }, + merged: false, + repository: { + name: req.params.repository + }, + state: 'open' + } + modifyPrDataForService(pr, service, req) + pr.repository.url = pr.repository.url.replace(service, 'gitFoo') + + const mockDeleteBranch = jest.fn() + + const handlePR = require('./../../../controllers/handlePR') + + expect.hasAssertions() + try { + await handlePR(req.params.repository, pr) + } catch (e) { + expect(e.message).toBe('Unable to determine service.') + expect(mockDeleteBranch).toHaveBeenCalledTimes(0) + expect(mockProcessMergeFn).toHaveBeenCalledTimes(0) + } + }) + + test.each([ + ['github'], + ['gitlab'], + ])('abort and return an error if no merge/pull request number found - %s', async (service) => { + let pr = { + title: 'Some random PR', + body: 'Unrelated review body', + head: { + ref: 'some-other-branch' + }, + pull_request: { + base: { + ref: 'master' + } + }, + merged: false, + repository: { + name: req.params.repository + }, + state: 'open' + } + modifyPrDataForService(pr, service, req) + if (service === 'github') { + pr.number = null + } else if (service === 'gitlab') { + pr.object_attributes.iid = null + } + + const mockDeleteBranch = jest.fn() + + const handlePR = require('./../../../controllers/handlePR') + + expect.hasAssertions() + try { + await handlePR(req.params.repository, pr) + } catch (e) { + expect(e.message).toBe('No pull/merge request number found.') + expect(mockDeleteBranch).toHaveBeenCalledTimes(0) + expect(mockProcessMergeFn).toHaveBeenCalledTimes(0) + } + }) + + test.each([ + ['github'], + ['gitlab'], + ])('abort and return an error if "getReview" call fails - %s', async (service) => { + let pr = { + title: 'Some random PR', + body: 'Unrelated review body', + head: { + ref: 'some-other-branch' + }, + pull_request: { + base: { + ref: 'master' + } + }, + merged: false, + repository: { + name: req.params.repository + }, + state: 'open' + } + modifyPrDataForService(pr, service, req) + + const mockGetReviewGitHub = jest.fn().mockRejectedValue('Error calling getReview.') + const mockGetReviewGitLab = jest.fn().mockRejectedValue('Error calling getReview.') + const mockDeleteBranch = jest.fn() + + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) + + const handlePR = require('./../../../controllers/handlePR') + + expect.hasAssertions() + try { + await handlePR(req.params.repository, pr) + } catch (e) { + expect(e.message).toBe('Error calling getReview.') + expect(mockDeleteBranch).toHaveBeenCalledTimes(0) + expect(mockProcessMergeFn).toHaveBeenCalledTimes(0) + } + }) + test.each([ ['github'], ['gitlab'], @@ -37,8 +157,10 @@ describe('HandlePR controller', () => { head: { ref: 'some-other-branch' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: false, repository: { @@ -48,7 +170,7 @@ describe('HandlePR controller', () => { } modifyPrDataForService(pr, service, req) - const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.base.ref) + const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.pull_request.base.ref) const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) @@ -73,8 +195,10 @@ describe('HandlePR controller', () => { head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -87,7 +211,7 @@ describe('HandlePR controller', () => { } modifyPrDataForService(pr, service, req) - const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.base.ref) + const mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.pull_request.base.ref) const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) const mockDeleteBranch = jest.fn() @@ -112,8 +236,10 @@ describe('HandlePR controller', () => { head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -126,7 +252,7 @@ describe('HandlePR controller', () => { } modifyPrDataForService(pr, service, req) - const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.base.ref) + const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.pull_request.base.ref) const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) const mockDeleteBranch = jest.fn() @@ -165,8 +291,10 @@ describe('HandlePR controller', () => { head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -179,8 +307,8 @@ describe('HandlePR controller', () => { } modifyPrDataForService(pr, service, req) - const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.base.ref) - const mockDeleteBranch = jest.fn() + const mockReview = new Review(pr.title, pr.body, 'merged', pr.head.ref, pr.pull_request.base.ref) + const mockDeleteBranch = jest.fn((sourceBranch) => new Promise((resolve, reject) => resolve({ }))) const mockGetReviewGitHub = jest.fn().mockResolvedValue(mockReview) const mockGetReviewGitLab = jest.fn().mockResolvedValue(mockReview) diff --git a/test/unit/controllers/webhook.test.js b/test/unit/controllers/webhook.test.js new file mode 100644 index 00000000..6608d9dc --- /dev/null +++ b/test/unit/controllers/webhook.test.js @@ -0,0 +1,172 @@ +const config = require('./../../../config') +const helpers = require('./../../helpers') + +let req +let res + +let mockHandlePrFn = jest.fn() +// The handlePR module exposes one "naked" function. +jest.mock('../../../controllers/handlePR', () => { + return mockHandlePrFn +}); + +const webhook = require('./../../../controllers/webhook') + +beforeEach(() => { + mockHandlePrFn.mockImplementation(() => Promise.resolve('success')) + // mockHandlePrFn.mockImplementation(() => { + // return 'success' + // }) + + req = helpers.getMockRequest() + res = helpers.getMockResponse() +}) + +afterEach(() => { + // Clear any test-specific mock implementations. + mockHandlePrFn.mockClear() +}) + +describe('Webhook controller', () => { + test.each([ + ['github'] + ])('returns an error if GitHub specified', async (service) => { + req.params.service = service + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(0) + expect(res.send.mock.calls[0][0]).toEqual({ error: 'Unexpected service specified.' }) + expect(res.status.mock.calls[0][0]).toBe(400) + }) + }) + + test.each([ + ['gitlab'] + ])('abort and return an error if no event header found - %s', async (service) => { + req.params.service = service + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(0) + expect(res.send.mock.calls[0][0]).toEqual({ error: 'No event found in the request' }) + expect(res.status.mock.calls[0][0]).toBe(400) + }) + }) + + test.each([ + ['gitlab'] + ])('abort and return success if not "Merge Request Hook" event - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Some Other Hook' + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(0) + expect(res.status.mock.calls[0][0]).toBe(200) + }) + }) + + test.each([ + ['gitlab'] + ])('abort and return an error if "handlePR" call fails - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Merge Request Hook' + + /* + * Replace the mock implementation to throw an error. More info: + * https://blog.bguiz.com/2017/mocking-chained-apis-jest/ + */ + mockHandlePrFn.mockImplementation(() => Promise.reject( { message: 'Error calling handlePR.' } )) + // mockHandlePrFn.mockImplementation(() => { + // throw { message: 'Error calling handlePR.' } + // }) + + // Suppress any calls to console.error - to keep test output clean. + const consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(1) + expect(res.send.mock.calls[0][0]).toEqual({ error: 'Error calling handlePR.' }) + expect(res.status.mock.calls[0][0]).toBe(400) + + // Restore console.error + consoleSpy.mockRestore(); + }) + }) + + test.each([ + ['gitlab'] + ])('return success if "handlePR" call succeeds - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Merge Request Hook' + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(1) + expect(res.status.mock.calls[0][0]).toBe(200) + }) + }) + + test.each([ + ['gitlab'] + ])('abort and return an error if webhook secret not sent - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Merge Request Hook' + + // Inject a value for "gitlabWebhookSecret" into the JSON-sourced config. + config.set('gitlabWebhookSecret', '2a-foobar-db72') + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(0) + expect(res.send.mock.calls[0][0]).toEqual({ error: 'No secret found in the webhook request' }) + expect(res.status.mock.calls[0][0]).toBe(400) + + // Clean-up the modified JSON-sourced config. + config.set('gitlabWebhookSecret', null) + }) + }) + + test.each([ + ['gitlab'] + ])('abort and return an error if unexpected webhook secret sent - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Merge Request Hook' + req.headers['x-gitlab-token'] = '2a-different-db72' + + // Inject a value for "gitlabWebhookSecret" into the JSON-sourced config. + config.set('gitlabWebhookSecret', '2a-foobar-db72') + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(0) + expect(res.send.mock.calls[0][0]).toEqual({ error: 'Unable to verify authenticity of request' }) + expect(res.status.mock.calls[0][0]).toBe(400) + + // Clean-up the modified JSON-sourced config. + config.set('gitlabWebhookSecret', null) + }) + }) + + test.each([ + ['gitlab'] + ])('return success if expected webhook secret sent - %s', async (service) => { + req.params.service = service + req.headers['x-gitlab-event'] = 'Merge Request Hook' + req.headers['x-gitlab-token'] = '2a-foobar-db72' + + // Inject a value for "gitlabWebhookSecret" into the JSON-sourced config. + config.set('gitlabWebhookSecret', '2a-foobar-db72') + + expect.hasAssertions() + return webhook(req, res).then(response => { + expect(mockHandlePrFn).toHaveBeenCalledTimes(1) + expect(res.status.mock.calls[0][0]).toBe(200) + + // Clean-up the modified JSON-sourced config. + config.set('gitlabWebhookSecret', null) + }) + }) +}) From ad59705a073849650f037d91c07f384f822262c5 Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Tue, 20 Oct 2020 13:39:01 -0400 Subject: [PATCH 5/7] Code clean-up. - Remove commented-out code. - Add explicit error handler to allows for overriding the system/express error handler. --- controllers/handlePR.js | 16 ---------------- controllers/webhook.js | 6 ------ server.js | 8 +++++++- 3 files changed, 7 insertions(+), 23 deletions(-) diff --git a/controllers/handlePR.js b/controllers/handlePR.js index ac34f256..743d83bb 100644 --- a/controllers/handlePR.js +++ b/controllers/handlePR.js @@ -92,11 +92,6 @@ module.exports = async (repo, data) => { staticman.setConfigPath(parsedBody.configPath) await staticman.processMerge(parsedBody.fields, parsedBody.options) - // staticman.processMerge(parsedBody.fields, parsedBody.options).then(data => { - // if (ua) { - // ua.event('Hooks', 'Create/notify mailing list').send() - // } - // }) if (ua) { ua.event('Hooks', 'Create/notify mailing list').send() } @@ -116,17 +111,6 @@ module.exports = async (repo, data) => { * "UnhandledPromiseRejectionWarning: HttpError: Reference does not exist" if branch already deleted. */ if (calcIsGitHub(data)) { - // await gitService.deleteBranch(review.sourceBranch).then(data => { - // if (ua) { - // ua.event('Hooks', 'Delete branch').send() - // } - // }).catch(err => { - // if (ua) { - // ua.event('Hooks', 'Delete branch error').send() - // } - - // return Promise.reject(err) - // }) try { await gitService.deleteBranch(review.sourceBranch) if (ua) { diff --git a/controllers/webhook.js b/controllers/webhook.js index ab0b27f8..d7431287 100644 --- a/controllers/webhook.js +++ b/controllers/webhook.js @@ -38,12 +38,6 @@ module.exports = async (req, res, next) => { console.error(error.stack || error) errorMsg = error.message }) - // try { - // await handlePR(req.params.repository, req.body) - // } catch (error) { - // console.error(error.stack || error) - // errorMsg = error.message - // } } } } diff --git a/server.js b/server.js index 24b80a88..212e7d32 100644 --- a/server.js +++ b/server.js @@ -134,16 +134,22 @@ class StaticmanAPI { */ const handlePrWrapper = function (repo, data) { this.controllers.handlePR(repo, data).catch((error) => { - console.error(error) /* * Unfortunately, the express-github-webhook module returns a 200 (success) regardless * of any errors raised in the downstream handler. So, all we can do is log errors. */ + console.error(error) }) }.bind(this) webhookHandler.on('pull_request', handlePrWrapper) + /* + * Explicit handler for errors raised inside the express-github-webhook module that mimmicks + * the system/express error handler. But, allows for customization and debugging. + */ + webhookHandler.on('error', (error) => console.error(error.stack || error)) + this.server.use(webhookHandler) } } From d400a360789f2b8d762eaa06841d8d055eb4a30f Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Tue, 20 Oct 2020 13:51:16 -0400 Subject: [PATCH 6/7] Remove extraneous whitespace. --- server.js | 2 +- test/unit/controllers/webhook.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server.js b/server.js index 212e7d32..c87bff03 100644 --- a/server.js +++ b/server.js @@ -145,7 +145,7 @@ class StaticmanAPI { webhookHandler.on('pull_request', handlePrWrapper) /* - * Explicit handler for errors raised inside the express-github-webhook module that mimmicks + * Explicit handler for errors raised inside the express-github-webhook module that mimmicks * the system/express error handler. But, allows for customization and debugging. */ webhookHandler.on('error', (error) => console.error(error.stack || error)) diff --git a/test/unit/controllers/webhook.test.js b/test/unit/controllers/webhook.test.js index 6608d9dc..bbedba45 100644 --- a/test/unit/controllers/webhook.test.js +++ b/test/unit/controllers/webhook.test.js @@ -74,7 +74,7 @@ describe('Webhook controller', () => { req.headers['x-gitlab-event'] = 'Merge Request Hook' /* - * Replace the mock implementation to throw an error. More info: + * Replace the mock implementation to throw an error. More info: * https://blog.bguiz.com/2017/mocking-chained-apis-jest/ */ mockHandlePrFn.mockImplementation(() => Promise.reject( { message: 'Error calling handlePR.' } )) From 791e56a656dfe934b1618f908f192aab097b6ed4 Mon Sep 17 00:00:00 2001 From: Michael Harry Scepaniak Date: Tue, 20 Oct 2020 15:53:42 -0400 Subject: [PATCH 7/7] Improve/fix comment. --- controllers/handlePR.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/handlePR.js b/controllers/handlePR.js index 743d83bb..c337ee2e 100644 --- a/controllers/handlePR.js +++ b/controllers/handlePR.js @@ -108,7 +108,7 @@ module.exports = async (repo, data) => { * Only necessary for GitHub, as GitLab automatically deletes the backing branch for the * pull/merge request. For GitHub, this will throw the following error if the branch has * already been deleted: - * "UnhandledPromiseRejectionWarning: HttpError: Reference does not exist" if branch already deleted. + * HttpError: Reference does not exist" */ if (calcIsGitHub(data)) { try {