feat: BE endpoint to retrieve presigned URLs (#6685)

* feat: get quarantine presignedurl * test: getPutQuarantinePresignedUrls * refactor: rename endpoint and fns * feat: check if key is a mongodb object id * test: when fieldId is not valid * feat: restrict presigned post validity to 1 min * refactor: endpoint to get-s3-presigned-post-data * feat: better err handling response to client * fix: expiry in test * feat: stricter validation for ids and file size * test: file size limit and new schema * refactor: scope logger to module * feat: configureAws error logging * feat: add logging to check for bucket * revert: "feat: add logging to check for bucket" This reverts commit 0f21dc6. * revert: "feat: configureAws error logging" This reverts commit 13026af. * fix: wrap createPresignedPost with callback * chore: rm unnecessary export * refactor: createPresignedPostDataPromise util * feat: use presigned post expiry env var * fix: path to utils * fix: public-read acl for images and logos * fix: import crypto properly * refactor: shared CreatePresignedPostError in utils * refactor: expiry constant * refactor: use shared CreatePresignedPostError * fix: script to enable bucket versioning * chore: enable versioning for clean bucket * fix: uat and prod virus scanner deployment * fix: broken tests from createPresignedPost promise * fix: s3 service test CopyObjectCommand * feat: joi total file size validator + better msgs * feat: async getting of presigned post data * refactor: rename cleanedResults to okResults * docs: in controller and routes * refactor: rm Promise.all * feat: gate presigned post data endpoint w db flag * fix: joi validate total file size comment * refactor: check for total attachment size in separate step * test: /get-s3-presigned-post-data route * test: if createPresignedPost fails * test: joi validation of /submissions/storage * test: singpass & corppass logins * refactor: use getFeatureFlag in service * chore: add unit tests for getS3PresignedPostData * fix: AttachmentSizeLimitExceededError test * refactor: use getEnabledFlags for /submissions/storage - realised that getFeatureFlag is not suitable, repeated db calls * fix: set status 403 for feature disabled error --------- Co-authored-by: tshuli <[email protected]>
opengovsg · Sep 15, 2023 · b7c482d · b7c482d
1 parent 9d0da42
commit b7c482d
Show file tree

Hide file tree

Showing 28 changed files with 1,676 additions and 100 deletions.
diff --git a/.github/workflows/deploy-virus-scanner-production.yml b/.github/workflows/deploy-virus-scanner-production.yml
@@ -1,17 +1,13 @@
 name: Deploy to production
 
-concurrency:
-  group: ${{ github.ref }}
-  cancel-in-progress: true
-
 permissions:
   id-token: write
   contents: read
 
 on:
   push:
     branches:
-      - production
+      - release-al2
   # schedule builds for 12:00AM GMT+8 everyday to get latest virus definitions
   schedule:
     - cron: '0 16 * * *'

diff --git a/.github/workflows/deploy-virus-scanner-uat.yml b/.github/workflows/deploy-virus-scanner-uat.yml
@@ -1,9 +1,5 @@
 name: Deploy to uat
 
-concurrency:
-  group: ${{ github.ref }}
-  cancel-in-progress: true
-
 permissions:
   id-token: write
   contents: read

diff --git a/init-localstack.sh b/init-localstack.sh
@@ -29,7 +29,9 @@ awslocal s3 mb s3://$STATIC_ASSETS_S3_BUCKET
 
 # Buckets for virus scanner
 # Set to versioning enabled
-awslocal s3 mb s3://$VIRUS_SCANNER_QUARANTINE_S3_BUCKET --versioning-configuration Status=Enabled
-awslocal s3 mb s3://$VIRUS_SCANNER_CLEAN_S3_BUCKET --versioning-configuration Status=Enabled
+awslocal s3 mb s3://$VIRUS_SCANNER_QUARANTINE_S3_BUCKET 
+awslocal s3api put-bucket-versioning --bucket $VIRUS_SCANNER_QUARANTINE_S3_BUCKET   --versioning-configuration Status=Enabled
+awslocal s3 mb s3://$VIRUS_SCANNER_CLEAN_S3_BUCKET 
+awslocal s3api put-bucket-versioning --bucket $VIRUS_SCANNER_CLEAN_S3_BUCKET   --versioning-configuration Status=Enabled
 
 set +x
diff --git a/serverless/virus-scanner/src/__tests/s3.service.spec.ts b/serverless/virus-scanner/src/__tests/s3.service.spec.ts
@@ -5,10 +5,11 @@ import { CopyObjectCommand, DeleteObjectCommand } from '@aws-sdk/client-s3'
 import * as LoggerService from '../logger'
 import { S3Service } from '../s3.service'
 
+const VersionId = 'mockObjectVersionId'
 // Mock S3Client
 let getResult = {
   Body: 'mockBody',
-  VersionId: 'mockObjectVersionId',
+  VersionId,
 }
 jest.mock('@aws-sdk/client-s3', () => {
   return {
@@ -20,7 +21,7 @@ jest.mock('@aws-sdk/client-s3', () => {
       }
     }),
     CopyObjectCommand: jest.fn().mockImplementation(() => {
-      return
+      return { VersionId }
     }),
     DeleteObjectCommand: jest.fn().mockImplementation(() => {
       return
@@ -99,6 +100,7 @@ describe('S3Service', () => {
           sourceObjectVersionId: 'sourceObjectVersionId',
           destinationBucketName: 'destinationBucketName',
           destinationObjectKey: 'destinationObjectKey',
+          destinationVersionId: 'mockObjectVersionId',
         },
         'Moved document in s3',
       )

diff --git a/serverless/virus-scanner/src/index.ts b/serverless/virus-scanner/src/index.ts
@@ -147,8 +147,11 @@ export const handler = async (
       key: quarantineFileKey,
       versionId,
     })
+
+    let destinationVersionId: string
+
     try {
-      await s3Client.moveS3File({
+      destinationVersionId = await s3Client.moveS3File({
         sourceBucketName: quarantineBucket,
         sourceObjectKey: quarantineFileKey,
         sourceObjectVersionId: versionId,
@@ -174,13 +177,15 @@ export const handler = async (
     logger.info({
       message: 'clean file moved to clean bucket',
       cleanFileKey,
+      destinationVersionId,
     })
 
     return {
       statusCode: StatusCodes.OK,
       body: JSON.stringify({
         message: 'File scan completed',
         cleanFileKey,
+        destinationVersionId,
       }),
     }
   }

diff --git a/serverless/virus-scanner/src/s3.service.ts b/serverless/virus-scanner/src/s3.service.ts
@@ -132,7 +132,7 @@ export class S3Service {
     sourceObjectVersionId,
     destinationBucketName,
     destinationObjectKey,
-  }: MoveS3FileParams) {
+  }: MoveS3FileParams): Promise<string> {
     this.logger.info(
       {
         sourceBucketName,
@@ -145,14 +145,29 @@ export class S3Service {
     )
 
     try {
-      await this.s3Client.send(
+      const { VersionId } = await this.s3Client.send(
         new CopyObjectCommand({
           Key: destinationObjectKey,
           Bucket: destinationBucketName,
           CopySource: `${sourceBucketName}/${sourceObjectKey}?versionId=${sourceObjectVersionId}`,
         }),
       )
 
+      if (!VersionId) {
+        this.logger.error(
+          {
+            sourceBucketName,
+            sourceObjectKey,
+            sourceObjectVersionId,
+            destinationBucketName,
+            destinationObjectKey,
+          },
+          'VersionId is empty after copying object in s3',
+        )
+
+        throw new Error('VersionId is empty')
+      }
+
       await this.s3Client.send(
         new DeleteObjectCommand({
           Key: sourceObjectKey,
@@ -168,9 +183,12 @@ export class S3Service {
           sourceObjectVersionId,
           destinationBucketName,
           destinationObjectKey,
+          destinationVersionId: VersionId,
         },
         'Moved document in s3',
       )
+
+      return VersionId
     } catch (error) {
       this.logger.error(
         {

diff --git a/shared/constants/feature-flags.ts b/shared/constants/feature-flags.ts
@@ -6,4 +6,6 @@ export const featureFlags = {
   encryptionBoundaryShift: 'encryption-boundary-shift' as const,
   encryptionBoundaryShiftHardValidation:
     'encryption-boundary-shift-hard-validation' as const,
+  encryptionBoundaryShiftVirusScanner:
+    'encryption-boundary-shift-virus-scanner' as const,
 }
diff --git a/src/app/modules/feature-flags/feature-flags.service.ts b/src/app/modules/feature-flags/feature-flags.service.ts
@@ -46,7 +46,7 @@ export const getEnabledFlags = (): ResultAsync<string[], DatabaseError> => {
  * @returns boolean that represents the status of the feature flag or the fallback value
  */
 export const getFeatureFlag = (
-  flag: keyof typeof featureFlags,
+  flag: typeof featureFlags[keyof typeof featureFlags],
   options?: {
     fallbackValue?: boolean
     logMeta?: CustomLoggerParams['meta']

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts
@@ -44,6 +44,7 @@ import {
 } from 'src/app/services/mail/mail.errors'
 import MailService from 'src/app/services/mail/mail.service'
 import { TwilioCredentials } from 'src/app/services/sms/sms.types'
+import { CreatePresignedPostError } from 'src/app/utils/aws-s3'
 import { EditFieldActions } from 'src/shared/constants'
 import {
   FormFieldSchema,
@@ -91,7 +92,6 @@ import {
 import * as FormService from '../../form.service'
 import * as AdminFormController from '../admin-form.controller'
 import {
-  CreatePresignedUrlError,
   EditFieldError,
   FieldNotFoundError,
   InvalidFileTypeError,
@@ -1225,7 +1225,7 @@ describe('admin-form.controller', () => {
       })
     })
 
-    it('should return 400 when CreatePresignedUrlError is returned when creating presigned POST URL', async () => {
+    it('should return 400 when CreatePresignedPostError is returned when creating presigned POST URL', async () => {
       // Arrange
       // Mock various services to return expected results.
       MockUserService.getPopulatedUserById.mockReturnValueOnce(
@@ -1238,7 +1238,7 @@ describe('admin-form.controller', () => {
       const mockErrorString = 'creating presigned post url failed, oh no'
       const mockRes = expressHandler.mockResponse()
       MockAdminFormService.createPresignedPostUrlForImages.mockReturnValueOnce(
-        errAsync(new CreatePresignedUrlError(mockErrorString)),
+        errAsync(new CreatePresignedPostError(mockErrorString)),
       )
 
       // Act
@@ -1460,7 +1460,7 @@ describe('admin-form.controller', () => {
       })
     })
 
-    it('should return 400 when CreatePresignedUrlError is returned when creating presigned POST URL', async () => {
+    it('should return 400 when CreatePresignedPostError is returned when creating presigned POST URL', async () => {
       // Arrange
       const mockRes = expressHandler.mockResponse()
       // Mock error
@@ -1473,7 +1473,7 @@ describe('admin-form.controller', () => {
         okAsync(MOCK_FORM),
       )
       MockAdminFormService.createPresignedPostUrlForLogos.mockReturnValueOnce(
-        errAsync(new CreatePresignedUrlError(mockErrorString)),
+        errAsync(new CreatePresignedPostError(mockErrorString)),
       )
 
       // Act

diff --git a/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts b/src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts
@@ -23,6 +23,7 @@ import { MissingUserError } from 'src/app/modules/user/user.errors'
 import * as UserService from 'src/app/modules/user/user.service'
 import { SmsLimitExceededError } from 'src/app/modules/verification/verification.errors'
 import { TwilioCredentials } from 'src/app/services/sms/sms.types'
+import { CreatePresignedPostError } from 'src/app/utils/aws-s3'
 import { formatErrorRecoveryMessage } from 'src/app/utils/handle-mongo-error'
 import { EditFieldActions } from 'src/shared/constants'
 import {
@@ -64,7 +65,6 @@ import {
   TransferOwnershipError,
 } from '../../form.errors'
 import {
-  CreatePresignedUrlError,
   EditFieldError,
   FieldNotFoundError,
   InvalidCollaboratorError,
@@ -231,7 +231,7 @@ describe('admin-form.service', () => {
       )
     })
 
-    it('should return CreatePresignedUrlError when error occurs whilst creating presigned POST URL', async () => {
+    it('should return CreatePresignedPostError when error occurs whilst creating presigned POST URL', async () => {
       // Arrange
       // Mock external service failure.
       const s3Spy = jest
@@ -258,7 +258,7 @@ describe('admin-form.service', () => {
       )
       expect(actualResult.isErr()).toEqual(true)
       expect(actualResult._unsafeUnwrapErr()).toEqual(
-        new CreatePresignedUrlError('Error occurred whilst uploading file'),
+        new CreatePresignedPostError('Error occurred whilst uploading file'),
       )
     })
   })
@@ -324,7 +324,7 @@ describe('admin-form.service', () => {
       )
     })
 
-    it('should return CreatePresignedUrlError when error occurs whilst creating presigned POST URL', async () => {
+    it('should return CreatePresignedPostError when error occurs whilst creating presigned POST URL', async () => {
       // Arrange
       // Mock external service failure.
       const s3Spy = jest
@@ -351,7 +351,7 @@ describe('admin-form.service', () => {
       )
       expect(actualResult.isErr()).toEqual(true)
       expect(actualResult._unsafeUnwrapErr()).toEqual(
-        new CreatePresignedUrlError('Error occurred whilst uploading file'),
+        new CreatePresignedPostError('Error occurred whilst uploading file'),
       )
     })
   })

diff --git a/src/app/modules/form/admin-form/admin-form.errors.ts b/src/app/modules/form/admin-form/admin-form.errors.ts
@@ -6,12 +6,6 @@ export class InvalidFileTypeError extends ApplicationError {
   }
 }
 
-export class CreatePresignedUrlError extends ApplicationError {
-  constructor(message: string) {
-    super(message)
-  }
-}
-
 export class EditFieldError extends ApplicationError {
   constructor(message: string) {
     super(message)

diff --git a/src/app/modules/form/admin-form/admin-form.service.ts b/src/app/modules/form/admin-form/admin-form.service.ts
@@ -48,6 +48,10 @@ import getAgencyModel from '../../../models/agency.server.model'
 import getFormModel from '../../../models/form.server.model'
 import * as SmsService from '../../../services/sms/sms.service'
 import { twilioClientCache } from '../../../services/sms/sms.service'
+import {
+  createPresignedPostDataPromise,
+  CreatePresignedPostError,
+} from '../../../utils/aws-s3'
 import { dotifyObject } from '../../../utils/dotify-object'
 import { isVerifiableMobileField } from '../../../utils/field-validation/field-validation.guards'
 import {
@@ -87,7 +91,6 @@ import {
 } from './../../../services/sms/sms.types'
 import { PRESIGNED_POST_EXPIRY_SECS } from './admin-form.constants'
 import {
-  CreatePresignedUrlError,
   EditFieldError,
   FieldNotFoundError,
   InvalidCollaboratorError,
@@ -167,42 +170,25 @@ const createPresignedPostUrl = (
   { fileId, fileMd5Hash, fileType }: PresignedPostUrlParams,
 ): ResultAsync<
   PresignedPost,
-  InvalidFileTypeError | CreatePresignedUrlError
+  InvalidFileTypeError | CreatePresignedPostError
 > => {
   if (!VALID_UPLOAD_FILE_TYPES.includes(fileType)) {
     return errAsync(
       new InvalidFileTypeError(`"${fileType}" is not a supported file type`),
     )
   }
 
-  const presignedPostUrlPromise = new Promise<PresignedPost>(
-    (resolve, reject) => {
-      AwsConfig.s3.createPresignedPost(
-        {
-          Bucket: bucketName,
-          Expires: PRESIGNED_POST_EXPIRY_SECS,
-          Conditions: [
-            // Content length restrictions: 0 to MAX_UPLOAD_FILE_SIZE.
-            ['content-length-range', 0, MAX_UPLOAD_FILE_SIZE],
-          ],
-          Fields: {
-            acl: 'public-read',
-            key: fileId,
-            'Content-MD5': fileMd5Hash,
-            'Content-Type': fileType,
-          },
-        },
-        (err, data) => {
-          if (err) {
-            return reject(err)
-          }
-          return resolve(data)
-        },
-      )
-    },
-  )
+  const presignedPostUrlPromise = createPresignedPostDataPromise({
+    bucketName,
+    expiresSeconds: PRESIGNED_POST_EXPIRY_SECS,
+    size: MAX_UPLOAD_FILE_SIZE,
+    key: fileId,
+    acl: 'public-read',
+    fileMd5Hash,
+    fileType,
+  })
 
-  return ResultAsync.fromPromise(presignedPostUrlPromise, (error) => {
+  return presignedPostUrlPromise.mapErr((error) => {
     logger.error({
       message: 'Error encountered when creating presigned POST URL',
       meta: {
@@ -214,7 +200,7 @@ const createPresignedPostUrl = (
       error,
     })
 
-    return new CreatePresignedUrlError('Error occurred whilst uploading file')
+    return new CreatePresignedPostError('Error occurred whilst uploading file')
   })
 }
 
@@ -227,13 +213,13 @@ const createPresignedPostUrl = (
  *
  * @returns ok(presigned post url) when creation is successful
  * @returns err(InvalidFileTypeError) when given file type is not supported
- * @returns err(CreatePresignedUrlError) when errors occurs on S3 side whilst creating presigned post url.
+ * @returns err(CreatePresignedPostError) when errors occurs on S3 side whilst creating presigned post url.
  */
 export const createPresignedPostUrlForImages = (
   uploadParams: PresignedPostUrlParams,
 ): ResultAsync<
   PresignedPost,
-  InvalidFileTypeError | CreatePresignedUrlError
+  InvalidFileTypeError | CreatePresignedPostError
 > => {
   return createPresignedPostUrl(AwsConfig.imageS3Bucket, uploadParams)
 }
@@ -247,13 +233,13 @@ export const createPresignedPostUrlForImages = (
  *
  * @returns ok(presigned post url) when creation is successful
  * @returns err(InvalidFileTypeError) when given file type is not supported
- * @returns err(CreatePresignedUrlError) when errors occurs on S3 side whilst creating presigned post url.
+ * @returns err(CreatePresignedPostError) when errors occurs on S3 side whilst creating presigned post url.
  */
 export const createPresignedPostUrlForLogos = (
   uploadParams: PresignedPostUrlParams,
 ): ResultAsync<
   PresignedPost,
-  InvalidFileTypeError | CreatePresignedUrlError
+  InvalidFileTypeError | CreatePresignedPostError
 > => {
   return createPresignedPostUrl(AwsConfig.logoS3Bucket, uploadParams)
 }