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

feat: Add webhook support for storage mode attachments #1713

Merged
merged 3 commits into from
Jun 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/app/models/__tests__/submission.server.model.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ describe('Submission Model', () => {
isRetryEnabled: true,
webhookView: {
data: {
attachmentDownloadUrls: {},
formId: String(form._id),
submissionId: String(submission._id),
encryptedContent: MOCK_ENCRYPTED_CONTENT,
Expand Down Expand Up @@ -120,6 +121,7 @@ describe('Submission Model', () => {
isRetryEnabled: false,
webhookView: {
data: {
attachmentDownloadUrls: {},
formId: String(form._id),
submissionId: String(submission._id),
encryptedContent: MOCK_ENCRYPTED_CONTENT,
Expand Down Expand Up @@ -155,6 +157,7 @@ describe('Submission Model', () => {
isRetryEnabled: false,
webhookView: {
data: {
attachmentDownloadUrls: {},
formId: String(form._id),
submissionId: String(submission._id),
encryptedContent: MOCK_ENCRYPTED_CONTENT,
Expand Down Expand Up @@ -268,6 +271,7 @@ describe('Submission Model', () => {
created: expect.any(Date),
encryptedContent: MOCK_ENCRYPTED_CONTENT,
verifiedContent: undefined,
attachmentDownloadUrls: {},
version: 1,
},
})
Expand All @@ -294,6 +298,7 @@ describe('Submission Model', () => {
// Assert
expect(actualWebhookView).toEqual({
data: {
attachmentDownloadUrls: {},
formId: expect.any(String),
submissionId: expect.any(String),
created: expect.any(Date),
Expand Down Expand Up @@ -336,6 +341,7 @@ describe('Submission Model', () => {
// Assert
expect(actualWebhookView).toEqual({
data: {
attachmentDownloadUrls: {},
formId: expect.any(String),
submissionId: expect.any(String),
created: expect.any(Date),
Expand Down
23 changes: 1 addition & 22 deletions src/app/models/field/attachmentField.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
import { Document, Schema } from 'mongoose'

import {
AttachmentSize,
IAttachmentField,
IFormSchema,
ResponseMode,
} from '../../../types'
import { AttachmentSize, IAttachmentField, IFormSchema } from '../../../types'

// Manual override since mongoose types don't have generics yet.
interface IAttachmentFieldSchema extends IAttachmentField, Document {
Expand All @@ -24,22 +19,6 @@ const createAttachmentFieldSchema = () => {
},
})

// Prevent attachments from being saved on a webhooked form.
AttachmentFieldSchema.pre<IAttachmentFieldSchema>(
'validate',
function (next) {
const { webhook, responseMode } = this.parent()

if (responseMode === ResponseMode.Encrypt && webhook?.url) {
return next(
Error('Attachments are not allowed when a form has a webhook url'),
)
}

return next()
},
)

return AttachmentFieldSchema
}

Expand Down
5 changes: 5 additions & 0 deletions src/app/models/submission.server.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,18 @@ EncryptSubmissionSchema.methods.getWebhookView = function (
const formId = this.populated('form')
? String(this.form._id)
: String(this.form)
const attachmentRecords = Object.fromEntries(
this.attachmentMetadata ?? new Map(),
)

const webhookData: WebhookData = {
formId,
submissionId: String(this._id),
encryptedContent: this.encryptedContent,
verifiedContent: this.verifiedContent,
version: this.version,
created: this.created,
attachmentDownloadUrls: attachmentRecords,
}

return {
Expand Down
3 changes: 3 additions & 0 deletions src/app/modules/webhook/__tests__/webhook.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ describe('webhook.service', () => {
formId: MOCK_FORM_ID,
submissionId: MOCK_SUBMISSION_ID,
verifiedContent: 'mockVerifiedContent',
attachmentDownloadUrls: {
'some-field-id': 'https://mock.s3.url/some/s3/url/timeout=3600',
},
version: 1,
},
}
Expand Down
14 changes: 14 additions & 0 deletions src/app/modules/webhook/webhook.errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,20 @@ export class WebhookValidationError extends ApplicationError {
}
}

/**
* Webhook failed to generate S3 presigned URLs for attachments
*/
export class WebhookFailedWithPresignedUrlGenerationError extends ApplicationError {
meta: {
originalError: unknown
}

constructor(error: unknown, message = 'Presigned Url Generation failed') {
super(message)
this.meta = { originalError: error }
}
}

/**
* Webhook returned non-200 status, but error is not instance of AxiosError
*/
Expand Down
172 changes: 108 additions & 64 deletions src/app/modules/webhook/webhook.service.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import axios from 'axios'
import Bluebird from 'bluebird'
import { get } from 'lodash'
import mongoose from 'mongoose'
import { errAsync, okAsync, ResultAsync } from 'neverthrow'
Expand All @@ -9,6 +10,7 @@ import {
IWebhookResponse,
WebhookView,
} from '../../../types'
import { aws as AwsConfig } from '../../config/config'
import formsgSdk from '../../config/formsg-sdk'
import { createLoggerWithLabel } from '../../config/logger'
import { getEncryptSubmissionModel } from '../../models/submission.server.model'
Expand All @@ -18,6 +20,7 @@ import { SubmissionNotFoundError } from '../submission/submission.errors'

import {
WebhookFailedWithAxiosError,
WebhookFailedWithPresignedUrlGenerationError,
WebhookFailedWithUnknownError,
WebhookPushToQueueError,
WebhookValidationError,
Expand Down Expand Up @@ -72,10 +75,35 @@ export const saveWebhookRecord = (
})
}

const createWebhookSubmissionView = (
submissionWebhookView: WebhookView,
): Promise<WebhookView> => {
// Generate S3 signed urls
const signedUrlPromises: Record<string, Promise<string>> = {}
for (const key in submissionWebhookView.data.attachmentDownloadUrls) {
signedUrlPromises[key] = AwsConfig.s3.getSignedUrlPromise('getObject', {
Bucket: AwsConfig.attachmentS3Bucket,
Key: submissionWebhookView.data.attachmentDownloadUrls[key],
Expires: 60 * 60, // one hour expiry
})
}

return Bluebird.props(signedUrlPromises).then((signedUrls) => {
submissionWebhookView.data.attachmentDownloadUrls = signedUrls
return submissionWebhookView
})
}

export const sendWebhook = (
webhookView: WebhookView,
webhookUrl: string,
): ResultAsync<IWebhookResponse, WebhookValidationError> => {
): ResultAsync<
IWebhookResponse,
| WebhookValidationError
| WebhookFailedWithAxiosError
| WebhookFailedWithPresignedUrlGenerationError
| WebhookFailedWithUnknownError
> => {
const now = Date.now()
const { submissionId, formId } = webhookView.data

Expand Down Expand Up @@ -104,77 +132,93 @@ export const sendWebhook = (
return error instanceof WebhookValidationError
? error
: new WebhookValidationError()
})
.andThen(() =>
ResultAsync.fromPromise(
axios.post<unknown>(webhookUrl, webhookView, {
headers: {
'X-FormSG-Signature': formsgSdk.webhooks.constructHeader({
epoch: now,
submissionId,
formId,
signature,
}),
},
maxRedirects: 0,
// Timeout after 10 seconds to allow for cold starts in receiver,
// e.g. Lambdas
timeout: 10 * 1000,
}),
(error) => {
logger.error({
message: 'Webhook POST failed',
meta: {
...logMeta,
isAxiosError: axios.isAxiosError(error),
status: get(error, 'response.status'),
},
error,
})
if (axios.isAxiosError(error)) {
return new WebhookFailedWithAxiosError(error)
}
return new WebhookFailedWithUnknownError(error)
},
),
}).andThen(() => {
return ResultAsync.fromPromise(
createWebhookSubmissionView(webhookView),
(error) => {
logger.error({
message: 'S3 attachment presigned URL generation failed',
meta: logMeta,
error,
})
return new WebhookFailedWithPresignedUrlGenerationError(error)
},
)
.map((response) => {
// Capture response for logging purposes
logger.info({
message: 'Webhook POST succeeded',
meta: {
...logMeta,
status: get(response, 'status'),
},
.andThen((submissionWebhookView) =>
ResultAsync.fromPromise(
axios.post<unknown>(webhookUrl, submissionWebhookView, {
headers: {
'X-FormSG-Signature': formsgSdk.webhooks.constructHeader({
epoch: now,
submissionId,
formId,
signature,
}),
},
maxRedirects: 0,
// Timeout after 10 seconds to allow for cold starts in receiver,
// e.g. Lambdas
timeout: 10 * 1000,
}),
(error) => {
logger.error({
message: 'Webhook POST failed',
meta: {
...logMeta,
isAxiosError: axios.isAxiosError(error),
status: get(error, 'response.status'),
},
error,
})
if (axios.isAxiosError(error)) {
return new WebhookFailedWithAxiosError(error)
}
return new WebhookFailedWithUnknownError(error)
},
),
)
.map((response) => {
// Capture response for logging purposes
logger.info({
message: 'Webhook POST succeeded',
meta: {
...logMeta,
status: get(response, 'status'),
},
})
return {
signature,
webhookUrl,
response: formatWebhookResponse(response),
}
})
return {
signature,
webhookUrl,
response: formatWebhookResponse(response),
}
})
.orElse((error) => {
// Webhook was not posted
if (error instanceof WebhookValidationError) return errAsync(error)
.orElse((error) => {
// Webhook was not posted
if (error instanceof WebhookValidationError) return errAsync(error)

// S3 pre-signed URL generation failed
if (error instanceof WebhookFailedWithPresignedUrlGenerationError)
return errAsync(error)

// Webhook was posted but failed
if (error instanceof WebhookFailedWithUnknownError) {
return okAsync({
signature,
webhookUrl,
// Not Axios error so no guarantee of having response.
// Hence allow formatting function to return default shape.
response: formatWebhookResponse(),
})
}

// Webhook was posted but failed
if (error instanceof WebhookFailedWithUnknownError) {
const axiosError = error.meta.originalError
return okAsync({
signature,
webhookUrl,
// Not Axios error so no guarantee of having response.
// Hence allow formatting function to return default shape.
response: formatWebhookResponse(),
response: formatWebhookResponse(axiosError.response),
})
}

const axiosError = error.meta.originalError
return okAsync({
signature,
webhookUrl,
response: formatWebhookResponse(axiosError.response),
})
})
})
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,15 +379,6 @@
<span class="field-optional">(optional)</span>
</label>
</div>
<div
ng-if="doesFormContainAttachments()"
class="alert-custom alert-info spcp-warning"
>
<i class="bx bx-info-circle bx-md icon-spacing"></i>
<span class="alert-msg"
>Webhook is not available for forms with attachment fields.</span
>
</div>
<div class="row feature-container webhook-feature-container">
<div
class="settings-save col-xs-12"
Expand All @@ -401,7 +392,7 @@
ng-model="tempForm.webhook.url"
placeholder="https://your-webhook.com/url"
ng-required="false"
ng-disabled="isFormPublic() || doesFormContainAttachments()"
frankchn marked this conversation as resolved.
Show resolved Hide resolved
ng-disabled="isFormPublic()"
autocomplete="off"
ng-keyup="$event.keyCode === 13 && settingsForm.webhookUrl.$valid && saveWebhookUrl()"
ng-blur="saveWebhookUrl()"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,9 +337,8 @@ function editFormController(

$scope.isStorageForm = $scope.myform.responseMode === responseModeEnum.ENCRYPT

// Disable attachment fields when we have webhooks
$scope.isDisabledField = function (fieldType) {
return fieldType.name === 'attachment' && $scope.myform.webhook.url !== ''
$scope.isDisabledField = function () {
return false
Comment on lines -340 to +341
Copy link
Contributor

@liangyuanruo liangyuanruo May 18, 2021

Choose a reason for hiding this comment

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

can we simply remove this function entirely if it's returning a constant?

Copy link
Contributor Author

@frankchn frankchn May 18, 2021

Choose a reason for hiding this comment

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

Sure, but I'll do it in a separate PR? It is used in a couple of places that are not directly relevant to this.

}

/**
Expand Down
Loading