From 12d8ebd885df92ab20aa382910289c24ade21e68 Mon Sep 17 00:00:00 2001 From: Nathan Brown Date: Tue, 19 Mar 2024 13:03:46 +0000 Subject: [PATCH] feat(cb2-11294): move get zip to use signed URLs (#67) --- src/domain/getZip.ts | 13 ++-- src/handler.ts | 12 ++-- src/infrastructure/s3/s3ZipService.ts | 19 ++---- tests/unit/domain/getZip.test.ts | 62 +++++------------- .../infrastructure/s3/s3ZipService.test.ts | 63 +++++++------------ 5 files changed, 54 insertions(+), 115 deletions(-) diff --git a/src/domain/getZip.ts b/src/domain/getZip.ts index 054aa9e..9ca3ff5 100644 --- a/src/domain/getZip.ts +++ b/src/domain/getZip.ts @@ -1,13 +1,13 @@ /* eslint-disable @typescript-eslint/no-unsafe-argument, @typescript-eslint/no-base-to-string */ -import { S3, AWSError } from 'aws-sdk'; import { APIGatewayProxyResult } from 'aws-lambda'; -import getObjectFromS3 from '../infrastructure/s3/s3ZipService'; -import NoBodyError from '../errors/NoBodyError'; -import MissingBucketNameError from '../errors/MissingBucketNameError'; +import { AWSError, S3 } from 'aws-sdk'; +import FileNameError from '../errors/FileNameError'; import IncorrectFileTypeError from '../errors/IncorrectFileTypeError'; +import MissingBucketNameError from '../errors/MissingBucketNameError'; import MissingFolderNameError from '../errors/MissingFolderNameError'; -import FileNameError from '../errors/FileNameError'; +import NoBodyError from '../errors/NoBodyError'; +import getObjectFromS3 from '../infrastructure/s3/s3ZipService'; import ZipDetails from '../interfaces/ZipDetails'; function isAWSError(error: Error | AWSError): error is AWSError { @@ -35,11 +35,12 @@ export default async ( throw new FileNameError(); } + // This is actually a unsigned URL that can be used to retrieve the file contents const file = await getObjectFromS3(s3, bucketName, folder, event.adrDocumentId); const response = file.toString('base64'); const headers = { - 'Content-type': 'application/zip', + 'Content-type': 'application/json', 'Access-Control-Allow-Origin': '*', 'Access-Control-Allow-Credentials': true, 'X-Content-Type-Options': 'nosniff', diff --git a/src/handler.ts b/src/handler.ts index 2513823..af3218b 100644 --- a/src/handler.ts +++ b/src/handler.ts @@ -3,9 +3,8 @@ import serverless from 'serverless-http'; import { Context, APIGatewayEvent, APIGatewayProxyStructuredResultV2 } from 'aws-lambda'; import { app } from './infrastructure/api'; -const handler = async (event: APIGatewayEvent, context: Context): Promise => { - return serverless(app, { - /** +const handler = async (event: APIGatewayEvent, context: Context): Promise => serverless(app, { + /** * We proxy requests from / as is handled in APIG when we deploy. * With with serverless-offline we proxy requests from /v from the client - * The package.json version as single source of truth to be the app basePath with stage @@ -28,9 +27,8 @@ const handler = async (event: APIGatewayEvent, context: Context): Promise/ */ - // basePath: `${AWS_PROVIDER_STAGE}/${MAJOR_VERSION}`, - basePath: 'v1', - })(event, context); -}; + // basePath: `${AWS_PROVIDER_STAGE}/${MAJOR_VERSION}`, + basePath: 'v1', +})(event, context); export { handler }; diff --git a/src/infrastructure/s3/s3ZipService.ts b/src/infrastructure/s3/s3ZipService.ts index a31b56e..ec5187e 100644 --- a/src/infrastructure/s3/s3ZipService.ts +++ b/src/infrastructure/s3/s3ZipService.ts @@ -1,5 +1,4 @@ import { S3 } from 'aws-sdk'; -import IncorrectFileTypeError from '../../errors/IncorrectFileTypeError'; import NoBodyError from '../../errors/NoBodyError'; export default async ( @@ -13,21 +12,11 @@ export default async ( console.info(`Bucket name: ${bucket}`); console.info(`Item key: ${key}`); - const response = await s3 - .getObject({ - Bucket: bucket, - Key: key, - }) - .promise(); + const params = { Bucket: bucket, Key: key }; + const response = await s3.getSignedUrlPromise('getObject', params); - if (response.ContentType !== 'application/octet-stream' && response.ContentType !== 'application/zip') { - // TODO: Cover this in case we encrypt the zip as octet stream or some other form - console.error(`Incorrect content-type: ${response.ContentType}`); - throw new IncorrectFileTypeError(); - } - - if (response.Body) { - return response.Body; + if (response) { + return response; } throw new NoBodyError(); diff --git a/tests/unit/domain/getZip.test.ts b/tests/unit/domain/getZip.test.ts index 720e7df..d63f5cb 100644 --- a/tests/unit/domain/getZip.test.ts +++ b/tests/unit/domain/getZip.test.ts @@ -1,10 +1,9 @@ import { S3 } from 'aws-sdk'; -import MissingBucketNameError from '../../../src/errors/MissingBucketNameError'; -import NoBodyError from '../../../src/errors/NoBodyError'; -import IncorrectFileTypeError from '../../../src/errors/IncorrectFileTypeError'; -import MissingFolderNameError from '../../../src/errors/MissingFolderNameError'; import getZip from '../../../src/domain/getZip'; import FileNameError from '../../../src/errors/FileNameError'; +import MissingBucketNameError from '../../../src/errors/MissingBucketNameError'; +import MissingFolderNameError from '../../../src/errors/MissingFolderNameError'; +import NoBodyError from '../../../src/errors/NoBodyError'; import ZipDetails from '../../../src/interfaces/ZipDetails'; describe('getZip', () => { @@ -37,10 +36,9 @@ describe('getZip', () => { it('returns an internal server error if there is no Body in the S3 request', async () => { const mockS3 = ({} as unknown) as S3; - const mockPromise = jest.fn().mockReturnValue(Promise.resolve({ ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue(undefined); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', @@ -52,31 +50,11 @@ describe('getZip', () => { expect(response.body).toEqual(error.message); }); - it('returns an 404 if the stored file is not a ZIP', async () => { - const mockS3 = ({} as unknown) as S3; - const mockPromise = jest - .fn() - .mockReturnValue(Promise.resolve({ Body: 'This is an image', ContentType: 'image/jpg' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); - - mockS3.getObject = mockGetObject; - - const event: ZipDetails = { - adrDocumentId: '1234', - }; - const response = await getZip(event, mockS3, 'bucket', 'folder', 'test'); - const error = new IncorrectFileTypeError(); - - expect(response.statusCode).toBe(404); - expect(response.body).toEqual(error.message); - }); - it('returns a not found error if the file is not found', async () => { const mockS3 = ({} as unknown) as S3; - const mockPromise = jest.fn().mockReturnValue(Promise.reject(({ code: 'NoSuchKey' } as unknown) as Error)); // eslint-disable-line prefer-promise-reject-errors - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockRejectedValue({ code: 'NoSuchKey' } as unknown as Error); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', @@ -89,10 +67,9 @@ describe('getZip', () => { it('returns an internal server error if the S3 get fails for any other reason', async () => { const mockS3 = ({} as unknown) as S3; - const mockPromise = jest.fn().mockReturnValue(Promise.reject(({ code: 'Generic Error' } as unknown) as Error)); // eslint-disable-line prefer-promise-reject-errors - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockRejectedValue(({ code: 'Generic Error' } as unknown as Error)); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', @@ -105,12 +82,9 @@ describe('getZip', () => { it('returns a successful response if everything works', async () => { const mockS3 = ({} as unknown) as S3; - const mockPromise = jest - .fn() - .mockReturnValue(Promise.resolve({ Body: 'Certificate Content', ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue('Certificate Content'); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', @@ -124,12 +98,9 @@ describe('getZip', () => { it('base64 encodes the response', async () => { const mockS3 = ({} as unknown) as S3; const body = Buffer.from('Certificate Content'); - const mockPromise = jest - .fn() - .mockReturnValue(Promise.resolve({ Body: body, ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue(body); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', @@ -142,12 +113,9 @@ describe('getZip', () => { it('ignores the folder check if the current environment is "local". Required for local testing', async () => { const mockS3 = ({} as unknown) as S3; - const mockPromise = jest - .fn() - .mockReturnValue(Promise.resolve({ Body: 'Zip Content', ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue('Zip Content'); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; const event: ZipDetails = { adrDocumentId: '1234', diff --git a/tests/unit/infrastructure/s3/s3ZipService.test.ts b/tests/unit/infrastructure/s3/s3ZipService.test.ts index 3d5d81b..d3e0518 100644 --- a/tests/unit/infrastructure/s3/s3ZipService.test.ts +++ b/tests/unit/infrastructure/s3/s3ZipService.test.ts @@ -6,61 +6,61 @@ describe('S3 Zip Service', () => { jest.resetAllMocks().restoreAllMocks(); }); - it('passes the expected key to getObject if folder is defined', async () => { + it('passes the expected key to getSignedUrl if folder is defined', async () => { const mockS3 = ({} as unknown) as S3; const bucket = 'bucket'; const folder = 'folder'; const adrDocumentId = '1234'; const mockPromise = jest .fn() - .mockResolvedValue({ Body: 'Success!', ContentType: 'application/octet-stream' }); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + .mockResolvedValue({ Body: 'Success!', ContentType: 'application/json' }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue({ promise: mockPromise }); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; await getFromS3(mockS3, bucket, folder, adrDocumentId).catch(() => {}); - const firstCall = mockGetObject.mock.calls[0] as S3.GetObjectRequest[]; + const firstCall = mockGetSignedUrlPromise.mock.calls[0] as S3.GetObjectRequest[]; const firstArg = firstCall[0]; - expect(firstArg.Key).toBe(`${folder}/adr-documents/${adrDocumentId}.zip`); + expect(firstArg).toBe('getObject'); }); - it('passes the expected key to getObject if folder is undefined', async () => { + it('passes the expected key to getSignedUrl if folder is undefined', async () => { const mockS3 = ({} as unknown) as S3; const bucket = 'bucket'; const folder: string | undefined = undefined; const adrDocumentId = '1234'; const mockPromise = jest .fn() - .mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + .mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'application/json' })); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue({ promise: mockPromise }); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; await getFromS3(mockS3, bucket, folder, adrDocumentId).catch(() => {}); - const firstCall = mockGetObject.mock.calls[0] as S3.GetObjectRequest[]; + const firstCall = mockGetSignedUrlPromise.mock.calls[0] as S3.GetObjectRequest[]; const firstArg = firstCall[0]; - expect(firstArg.Key).toBe(`/adr-documents/${adrDocumentId}.zip`); + expect(firstArg).toBe('getObject'); }); - it('passes the bucket to getObject', () => { + it('passes the bucket to getSignedUrl', () => { const mockS3 = ({} as unknown) as S3; const bucket = 'bucket'; const folder = 'folder'; const adrDocumentId = '1234'; const mockPromise = jest .fn() - .mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + .mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'application/json' })); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue({ promise: mockPromise }); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; getFromS3(mockS3, bucket, folder, adrDocumentId).catch(() => {}); - const firstCall = mockGetObject.mock.calls[0] as S3.GetObjectRequest[]; - const firstArg = firstCall[0]; + const firstCall = mockGetSignedUrlPromise.mock.calls[0] as S3.GetObjectRequest[]; + const secondArg = firstCall[1]; - expect(firstArg.Bucket).toEqual(bucket); + expect(secondArg.Bucket).toEqual(bucket); }); it('returns the expected output', async () => { @@ -68,38 +68,21 @@ describe('S3 Zip Service', () => { const bucket = 'bucket'; const folder = 'folder'; const adrDocumentId = '1234'; - const mockPromise = jest - .fn() - .mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue('Success!'); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; expect(await getFromS3(mockS3, bucket, folder, adrDocumentId)).toBe('Success!'); }); - it('throws an error if the response is not a ZIP', async () => { - const mockS3 = ({} as unknown) as S3; - const bucket = 'bucket'; - const folder = 'folder'; - const adrDocumentId = '1234'; - const mockPromise = jest.fn().mockReturnValue(Promise.resolve({ Body: 'Success!', ContentType: 'image/jpg' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); - - mockS3.getObject = mockGetObject; - - await expect(getFromS3(mockS3, bucket, folder, adrDocumentId)).rejects.toThrow(); - }); - it('throws an error if there is no body in the response', async () => { const mockS3 = ({} as unknown) as S3; const bucket = 'bucket'; const folder = 'folder'; const adrDocumentId = '1234'; - const mockPromise = jest.fn().mockReturnValue(Promise.resolve({ ContentType: 'application/octet-stream' })); - const mockGetObject = jest.fn().mockReturnValue({ promise: mockPromise }); + const mockGetSignedUrlPromise = jest.fn().mockReturnValue(undefined); - mockS3.getObject = mockGetObject; + mockS3.getSignedUrlPromise = mockGetSignedUrlPromise; await expect(getFromS3(mockS3, bucket, folder, adrDocumentId)).rejects.toThrow(); });