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 87aeacde..c337ee2e 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,53 +9,120 @@ 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/foo/staticman-test" + * "url": "git@gitlab.com:foo/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') + } + + /* + * 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 unknownVersion = '' - const github = await new GitHub({ - username: data.repository.owner.login, - repository: data.repository.name, - version: '1' + let gitService = null + let mergeReqNbr = null + if (calcIsGitHub(data)) { + gitService = await gitFactory.create('github', { + 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: data.object_attributes.target_branch, + repository: data.repository.name, + username: repoUsername, + version: unknownVersion + }) + mergeReqNbr = data.object_attributes.iid + } else { + return Promise.reject(new Error('Unable to determine service.')) + } + + if (!mergeReqNbr) { + return Promise.reject(new Error('No pull/merge request number found.')) + } + + let review = await gitService.getReview(mergeReqNbr).catch((error) => { + return Promise.reject(new Error(error)) }) - try { - let review = await github.getReview(data.number) - if (review.sourceBranch.indexOf('staticman_')) { - 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' && review.state !== 'closed') { - return null - } + 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 (review.state === 'merged') { - 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) - if (bodyMatch && (bodyMatch.length === 2)) { - try { - const parsedBody = JSON.parse(bodyMatch[1]) - const staticman = await new Staticman(parsedBody.parameters) + staticman.setConfigPath(parsedBody.configPath) - staticman.setConfigPath(parsedBody.configPath) - staticman.processMerge(parsedBody.fields, parsedBody.options) - } catch (err) { - return Promise.reject(err) + await staticman.processMerge(parsedBody.fields, parsedBody.options) + if (ua) { + ua.event('Hooks', 'Create/notify mailing list').send() + } + } catch (err) { + if (ua) { + ua.event('Hooks', 'Create/notify mailing list error').send() } + + return Promise.reject(err) } } - if (ua) { - ua.event('Hooks', 'Delete branch').send() - } - return github.deleteBranch(review.sourceBranch) - } catch (e) { - console.log(e.stack || e) + /* + * 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: + * HttpError: Reference does not exist" + */ + if (calcIsGitHub(data)) { + 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() + } - if (ua) { - ua.event('Hooks', 'Delete branch error').send() + return Promise.reject(err) + } } - - return Promise.reject(e) } } diff --git a/controllers/webhook.js b/controllers/webhook.js new file mode 100644 index 00000000..d7431287 --- /dev/null +++ b/controllers/webhook.js @@ -0,0 +1,64 @@ +'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': + let errorMsg = null + let event = req.headers['x-gitlab-event'] + + 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 + }) + } + } + } + + if (errorMsg !== null) { + res.status(400).send({ + error: errorMsg + }) + } else { + res.status(200).send({ + success: true + }) + } + + break + default: + 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/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..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 @@ -117,7 +122,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 +187,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/server.js b/server.js index 003af118..c87bff03 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,46 @@ 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, + 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. + */ + const handlePrWrapper = function (repo, data) { + this.controllers.handlePR(repo, data).catch((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', this.controllers.handlePR) + webhookHandler.on('pull_request', handlePrWrapper) - this.server.use(webhookHandler) + /* + * 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) + } } requireApiVersion (versions) { 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/controllers/handlePR.test.js b/test/unit/controllers/handlePR.test.js index 786525bc..aa898e79 100644 --- a/test/unit/controllers/handlePR.test.js +++ b/test/unit/controllers/handlePR.test.js @@ -18,65 +18,187 @@ 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('ignores pull requests from branches not prefixed with `staticman_`', async () => { - const pr = { - number: 123, + 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' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: false, repository: { - name: req.params.repository, - owner: { - login: req.params.username + 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 mockReview = new Review(pr.title, pr.body, 'false', pr.head.ref, pr.base.ref) - const mockGetReview = jest.fn().mockResolvedValue(mockReview) + const mockDeleteBranch = jest.fn() - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - getReview: mockGetReview + 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'], + ])('ignores pull requests from branches not prefixed with `staticman_` - %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 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) + + 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, head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -87,37 +209,37 @@ 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 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() - 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, head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -128,19 +250,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 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() - jest.mock('../../../lib/GitHub', () => { - return jest.fn().mockImplementation(() => { - return { - getReview: mockGetReview, - deleteBranch: mockDeleteBranch - } - }) - }) + mockGitModules(mockGetReviewGitHub, mockGetReviewGitLab, mockDeleteBranch) const errorMessage = 'some error' @@ -150,12 +267,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,16 +280,21 @@ 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, head: { ref: 'staticman_1234567' }, - base: { - ref: 'master' + pull_request: { + base: { + ref: 'master' + } }, merged: true, repository: { @@ -183,28 +305,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 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) - 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/controllers/webhook.test.js b/test/unit/controllers/webhook.test.js new file mode 100644 index 00000000..bbedba45 --- /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) + }) + }) +}) diff --git a/test/unit/lib/GitHub.test.js b/test/unit/lib/GitHub.test.js index d1e65d52..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: { @@ -100,6 +116,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.readFile(filePath) } catch (err) { @@ -109,18 +127,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 +330,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 +407,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 +419,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 +449,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.writeFileAndSendReview( options.path, @@ -448,6 +498,8 @@ describe('GitHub interface', () => { const githubInstance = await new GitHub(req.params) + expect.assertions(2) + try { await githubInstance.getCurrentUser() } catch (err) {