Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for email notifications with GitLab #388

Closed
wants to merge 12 commits into from
61 changes: 0 additions & 61 deletions controllers/handlePR.js

This file was deleted.

302 changes: 302 additions & 0 deletions controllers/webhook.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,302 @@
'use strict'

const bufferEq = require('buffer-equal-constant-time')
const path = require('path')
const config = require(path.join(__dirname, '/../config'))
Comment on lines +4 to +5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const path = require('path')
const config = require(path.join(__dirname, '/../config'))
const config = require('../config')

nit: I don't think the path join is needed here

const crypto = require('crypto')

const gitFactory = require('../lib/GitServiceFactory')
const Staticman = require('../lib/Staticman')

/**
* Express handler for webhook requests/notifications generated by the backing git service.
*/
module.exports = async (req, res, next) => {
let errorsRaised = []

let service = null
let staticman = null
if (_calcIsVersion1WhenGitHubAssumed(req)) {
service = 'github'
} else {
/*
* In versions of the webhook endpoint beyond v1, we have the parameters necessary to
* instantiate a Staticman instance right away.
*/
service = req.params.service
staticman = await new Staticman(req.params)
staticman.setConfigPath()
}

switch (service) {
case 'github':
await _handleWebhookGitHub(req, service, staticman).catch((errors) => {
errorsRaised = errorsRaised.concat(errors)
})
break
case 'gitlab':
await _handleWebhookGitLab(req, service, staticman).catch((errors) => {
errorsRaised = errorsRaised.concat(errors)
})
break
default:
errorsRaised.push('Unexpected service specified.')
}

if (errorsRaised.length > 0) {
res.status(400).send({
errors: JSON.stringify(errorsRaised)
})
} else {
res.status(200).send({
success: true
})
}
}

const _handleWebhookGitHub = async function (req, service, staticman) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's still a lot of nested branches here. Same applies to Gitlab handler function. Right now the function first reads the event type, then verifies the authenticity, then calls the webhook event handler. I think it would be better to break this up and change the order around. Here's some pseudocode of how I imagine this working:

const _handleWebhookGithHub() {
    let isAuthenticated = _requiresAuthentication() ? authenticateRequest() : true;
    if (!isAuthenticated) {
      return Promise.reject('Unable to authenticate webhook request')
    }

    const event = req.headers['x-github-event']
    switch(event) {
      case 'pull-request':
        _handleMergeRequest()
        break;
      default:
        errorsRaised.push(`No handler found for webhook event ${event}`)
    }
}

let errorsRaised = []

const event = req.headers['x-github-event']
if (!event) {
errorsRaised.push('No event found in the request')
} else {
if (event === 'pull_request') {
let webhookSecretExpected = null
if (staticman) {
// Webhook request authentication is NOT supported in v1 of the endpoint.
await staticman.getSiteConfig().then((siteConfig) => {
webhookSecretExpected = siteConfig.get('githubWebhookSecret')
})
}

let reqAuthenticated = true
if (webhookSecretExpected) {
reqAuthenticated = false
const webhookSecretSent = req.headers['x-hub-signature']
if (!webhookSecretSent) {
// This could be worth logging... unless the endpoint gets hammered with spam.
errorsRaised.push('No secret found in the webhook request')
} else if (_verifyGitHubSignature(webhookSecretExpected, JSON.stringify(req.body), webhookSecretSent)) {
reqAuthenticated = true
} else {
// This could be worth logging... unless the endpoint gets hammered with spam.
errorsRaised.push('Unable to verify authenticity of request')
}
}

if (reqAuthenticated) {
await _handleMergeRequest(req.params, service, req.body, staticman).catch((errors) => {
errorsRaised = errors
})
}
}
}

if (errorsRaised.length > 0) {
return Promise.reject(errorsRaised)
}
}

const _handleWebhookGitLab = async function (req, service, staticman) {
let errorsRaised = []

const event = req.headers['x-gitlab-event']
if (!event) {
errorsRaised.push('No event found in the request')
} else {
if (event === 'Merge Request Hook') {
let webhookSecretExpected = null
if (staticman) {
// Webhook request authentication is NOT supported in v1 of the endpoint.
await staticman.getSiteConfig().then((siteConfig) => {
webhookSecretExpected = siteConfig.get('gitlabWebhookSecret')
})
}

let reqAuthenticated = true
if (webhookSecretExpected) {
reqAuthenticated = false
const webhookSecretSent = req.headers['x-gitlab-token']
if (!webhookSecretSent) {
// This could be worth logging... unless the endpoint gets hammered with spam.
errorsRaised.push('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 {
// This could be worth logging... unless the endpoint gets hammered with spam.
errorsRaised.push('Unable to verify authenticity of request')
}
}

if (reqAuthenticated) {
await _handleMergeRequest(req.params, service, req.body, staticman).catch((errors) => {
errorsRaised = errors
})
}
}
}

if (errorsRaised.length > 0) {
return Promise.reject(errorsRaised)
}
}

const _calcIsVersion1WhenGitHubAssumed = function (req) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const _calcIsVersion1WhenGitHubAssumed = function (req) {
const _shouldUseDefaultService = function (req) {

nit: I think this would be a slightly more accurate name

const service = req.params.service
const version = req.params.version
return (!service && version === '1')
}

const _verifyGitHubSignature = function (secret, data, signature) {
const signedData = 'sha1=' + crypto.createHmac('sha1', secret).update(data).digest('hex')
return bufferEq(Buffer.from(signature), Buffer.from(signedData))
}

const _handleMergeRequest = async function (params, service, data, staticman) {
const errors = []

const ua = config.get('analytics.uaTrackingId')
? require('universal-analytics')(config.get('analytics.uaTrackingId'))
: null

const version = params.version
const username = params.username
const repository = params.repository
const branch = params.branch

let gitService = null
let mergeReqNbr = null
if (service === 'github') {
gitService = await _buildGitHubService(version, service, username, repository, branch, data)
mergeReqNbr = data.number
} else if (service === 'gitlab') {
gitService = await gitFactory.create('gitlab', {
version: version,
username: username,
repository: repository,
branch: branch
})
mergeReqNbr = data.object_attributes.iid
} else {
errors.push('Unable to determine service.')
return Promise.reject(errors)
}
Comment on lines +167 to +188
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const version = params.version
const username = params.username
const repository = params.repository
const branch = params.branch
let gitService = null
let mergeReqNbr = null
if (service === 'github') {
gitService = await _buildGitHubService(version, service, username, repository, branch, data)
mergeReqNbr = data.number
} else if (service === 'gitlab') {
gitService = await gitFactory.create('gitlab', {
version: version,
username: username,
repository: repository,
branch: branch
})
mergeReqNbr = data.object_attributes.iid
} else {
errors.push('Unable to determine service.')
return Promise.reject(errors)
}
const version = params.version
const username = params.username
const repository = params.repository
const branch = params.branch
let gitService = await _buildGitService(version, service, username, repository, branch, data)
let mergeReqNbr = getMergeRequestNumber(service);
------------------------------------------------------------------------------------------
const _buildGitService = function (version, service, username, repository, data) {
// Required for v1 webhook backwards compatibility
username = username ? username : data.repository.owner.login
repository = repository ? repository : data.repository.name
branch = branch ? branch : data.pull_request.base.ref
const serviceConfig = {
version,
username,
repository,
branch
}
return await gitFactory.create(service, serviceConfig);
}
const _getMergeRequestNumber = function (service, data) {
return service === 'github' ? data.number : data.object_attributes.iid
}

We can do something like above and make a helper function to abstract out the git service creation from webhook handler. This way we don't have to manually add cases to the conditional in the event another service is eventually supported.


if (mergeReqNbr === null || typeof mergeReqNbr === 'undefined') {
errors.push('No pull/merge request number found.')
return Promise.reject(errors)
}

let review = await gitService.getReview(mergeReqNbr).catch((error) => {
const msg = `Failed to retrieve merge request ${mergeReqNbr} - ${error}`
console.error(msg)
errors.push(msg)
return Promise.reject(errors)
})

if (_calcIsMergeRequestStaticmanGenerated(review)) {
if (_calcIsMergeRequestAccepted(review)) {
Comment on lines +202 to +203
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (_calcIsMergeRequestStaticmanGenerated(review)) {
if (_calcIsMergeRequestAccepted(review)) {
if (_calcIsMergeRequestStaticmanGenerated(review) && _calcIsMergeRequestAccepted(review)) {

Nesting isn't necessary here

await _createNotifyMailingList(review, staticman, ua).catch((error) => {
errors.push(error.message)
})

if (_calcIsMergeRequestBranchRequiresManualDelete(service)) {
await _deleteMergeRequestBranch(gitService, review, ua).catch((error) => {
errors.push(error)
})
}
}
}

if (errors.length > 0) {
return Promise.reject(errors)
}
}

const _buildGitHubService = function (version, service, username, repository, branch, data) {
/*
* In v1 of the endpoint, the service, username, repository, and branch parameters were
* ommitted. As such, if not provided in the webhook request URL, pull them from the webhook
* payload.
*/
if (username === null || typeof username === 'undefined') {
username = data.repository.owner.login
}
if (repository === null || typeof repository === 'undefined') {
repository = data.repository.name
}
if (branch === null || typeof branch === 'undefined') {
branch = data.pull_request.base.ref
}

return gitFactory.create(service, {
version: version,
username: username,
repository: repository,
branch: branch
})
}

const _calcIsMergeRequestAccepted = function (review) {
return (review.state === 'merged' || review.state === 'closed')
}

const _calcIsMergeRequestStaticmanGenerated = function (review) {
return (review.sourceBranch.indexOf('staticman_') > -1)
}

const _calcIsMergeRequestBranchRequiresManualDelete = function (service) {
return (service === 'github')
}
Comment on lines +245 to +255
Copy link
Collaborator

Choose a reason for hiding this comment

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

'calc' in these function names doesn't add any info. Consider these two functions:

  • _calcIsMergeRequestAccepted()
  • _isMergeRequestAccepted()

We should prefer the shorter of the two since it conveys the same meaning.

Copy link
Author

Choose a reason for hiding this comment

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

I disagree. When you prefix a function with "is", you are implying a simple getter with no logic involved. That is not the case here, hence the addition of the "calc". If it's important to you, though, I can change it.


const _createNotifyMailingList = async function (review, staticman, ua) {
const bodyMatch = review.body.match(/(?:.*?)<!--staticman_notification:(.+?)-->(?:.*?)/i)
if (_calcIsNotificationsEnabledWhenMergeRequestCreated(bodyMatch)) {
try {
const parsedBody = JSON.parse(bodyMatch[1])
if (staticman === null) {
staticman = await new Staticman(parsedBody.parameters)
staticman.setConfigPath(parsedBody.configPath)
}

await staticman.processMerge(parsedBody.fields, parsedBody.extendedFields, parsedBody.options).then(msg => {
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)
}
}
}

const _calcIsNotificationsEnabledWhenMergeRequestCreated = function (bodyMatch) {
return (bodyMatch && (bodyMatch.length === 2))
}

const _deleteMergeRequestBranch = async function (gitService, review, ua) {
try {
// This will throw the error 'Reference does not exist' if the branch has already been deleted.
await gitService.deleteBranch(review.sourceBranch)
if (ua) {
ua.event('Hooks', 'Delete branch').send()
}
} catch (err) {
if (ua) {
ua.event('Hooks', 'Delete branch error').send()
}

const msg = `Failed to delete merge branch ${review.sourceBranch} - ${err}`
console.error(msg)
return Promise.reject(msg)
}
}
3 changes: 2 additions & 1 deletion lib/GitHub.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ class GitHub extends GitService {
const isAppAuth = config.get('githubAppID') &&
config.get('githubPrivateKey')
const isLegacyAuth = config.get('githubToken') &&
['1', '2'].includes(options.version)
// Staticman version may not always be available to the caller. As such, allow for blank.
['1', '2', ''].includes(options.version)

let authToken

Expand Down
8 changes: 1 addition & 7 deletions lib/Staticman.js
Original file line number Diff line number Diff line change
Expand Up @@ -573,18 +573,12 @@ class Staticman {
}

processMerge (fields, options) {
this.fields = Object.assign({}, fields)
this.options = Object.assign({}, options)

return this.getSiteConfig().then(config => {
const subscriptions = this._initialiseSubscriptions()

return subscriptions.send(options.parent, fields, options, this.siteConfig)
}).catch(err => {
return Promise.reject(errorHandler('ERROR_PROCESSING_MERGE', {
err,
instance: this
}))
return Promise.reject(err)
})
}

Expand Down
Loading