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

TechDebt: Update Multer middleware and OpenAPI spec for file imports #1326

Merged
merged 23 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6fd58f4
feat: updated multer middleware + modified file import endpoints
MacQSL Jul 18, 2024
ce0b754
chore: dropped deprecated tests
MacQSL Jul 18, 2024
c2292e9
doc: updated OpenAPI spec descriptions
MacQSL Jul 18, 2024
6653168
doc: updated docs in app.ts for multer middleware
MacQSL Jul 18, 2024
51fdb45
doc: updated documentation in app.ts for Multer files
MacQSL Jul 19, 2024
7fa0000
fix: dropped text/plain from attachment/report upload specs
MacQSL Jul 19, 2024
7bdf859
fix: created shared schemas for csv / generic file types
MacQSL Jul 19, 2024
399cce7
fix: removed manual checks for existing files
MacQSL Jul 19, 2024
50816ff
fix: updated file schema
MacQSL Jul 19, 2024
ffc752d
doc: updated documentation for fileSchema
MacQSL Jul 19, 2024
98cbf73
feat: updated typing for express requests for file imports
MacQSL Jul 19, 2024
4c2e48c
feat: created getter for getting file from request
MacQSL Jul 19, 2024
a6ce3f2
fix: fixed merge conflicts with dev
MacQSL Jul 22, 2024
8679947
doc: updated documentation for express.d.ts
MacQSL Jul 22, 2024
31d435a
doc: updated documentation for express.d.ts
MacQSL Jul 22, 2024
eac48a8
Merge branch 'dev' into techdebt-import-media-spec
MacQSL Jul 23, 2024
bf001de
doc: updated app.ts -> multerRequestHandler
MacQSL Jul 23, 2024
dc7f6f4
Fix css for file upload progress bar
NickPhura Jul 23, 2024
211e84d
ignore-skip
MacQSL Jul 23, 2024
8015362
ignore-skip
MacQSL Jul 23, 2024
0f2bd28
fix: updated APP_PORT -> APP_PORT_DEFAULT
MacQSL Jul 23, 2024
97b8144
fix: updated app.dc.yaml to include REACT_APP_PORT
MacQSL Jul 23, 2024
4145142
ignore-skip
MacQSL Jul 23, 2024
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
20 changes: 15 additions & 5 deletions api/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,26 @@ const openAPIFramework = initialize({
limits: { fileSize: MAX_UPLOAD_FILE_SIZE }
}).array('media', MAX_UPLOAD_NUM_FILES);

/**
* Multer transforms and moves the incoming files from `req.body.media` --> `req.files`.
*
* OpenAPI only allows validation on specific parts of the request object (requestBody / parameters...) this excludes the contents of `req.files`.
* To get around this we re-assign `req.body.media` to the Multer transformed files stored in `req.files`.
*
* Files can be accessed via `req.body.media` OR `req.files`.
*
* @see https://www.npmjs.com/package/express-openapi#argsconsumesmiddleware
*/
multerRequestHandler(req, res, (error?: any) => {
if (error) {
return next(error);
}

if (req.files && req.files.length) {
// Set original request file field to empty string to satisfy OpenAPI validation
// See: https://www.npmjs.com/package/express-openapi#argsconsumesmiddleware
(req.files as Express.Multer.File[]).forEach((file) => (req.body[file.fieldname] = ''));
}
// Ensure `req.files` or `req.body.media` is always set to an array
const multerFiles = req.files ?? [];

req.files = multerFiles;
req.body.media = multerFiles;

return next();
});
Expand Down
64 changes: 64 additions & 0 deletions api/src/openapi/schemas/file.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { OpenAPIV3 } from 'openapi-types';

/**
* Shared file schema for all file formats.
*
* @see MIME types: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types
*
*/
export const fileSchema: OpenAPIV3.SchemaObject = {
type: 'object',
description: 'Schema defining the structure of a file object uploaded and transformed via Multer middleware.',
required: ['fieldname', 'originalname', 'mimetype', 'buffer'],
properties: {
fieldname: {
description: 'Property name of incomming file upload. Multer expects `media`.',
type: 'string',
enum: ['media']
},
originalname: {
description: 'Original name of the file with its extension as provided by the uploader.',
type: 'string',
example: 'import-file.txt'
},
encoding: {
description: 'Encoding of the file content.',
type: 'string',
example: '7bit'
},
mimetype: {
description: 'MIME type of the file, such as `text/csv` for CSV files.',
type: 'string',
example: 'text/csv'
},
buffer: {
description: 'The content of the file transformed into a Buffer object for further processing.',
type: 'object',
format: 'buffer'
},
size: {
description: 'Size of the file in kilobytes (KB). Minimum value is 1 KB.',
type: 'integer',
minimum: 1
}
}
};

/**
* CSV file schema.
*
* Note: This could be extended further to have a regex that validates the `originalname` ends with `.csv`
* but I think the enum value for `mimetype` is enough for our needs.
*
*/
export const csvFileSchema: OpenAPIV3.SchemaObject = {
...fileSchema,
properties: {
...fileSchema.properties,
mimetype: {
description: 'CSV File type.',
type: 'string',
enum: ['text/csv']
}
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -38,41 +38,6 @@ describe('uploadMedia', () => {
}
} as any;

it('should throw an error when files are missing', async () => {
sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);

try {
const result = upload.uploadMedia();

await result({ ...mockReq, files: [] }, null as unknown as any, null as unknown as any);
expect.fail();
} catch (actualError) {
expect((actualError as HTTPError).status).to.equal(400);
expect((actualError as HTTPError).message).to.equal('Missing upload data');
}
});

it('should throw an error when file format incorrect', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
return 20;
}
});

sinon.stub(file_utils, 'scanFileForVirus').resolves(false);

try {
const result = upload.uploadMedia();

await result(mockReq, null as unknown as any, null as unknown as any);
expect.fail();
} catch (actualError) {
expect((actualError as HTTPError).status).to.equal(400);
expect((actualError as HTTPError).message).to.equal('Malicious content detected, upload cancelled');
}
});

it('should throw an error if failure occurs', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
Expand Down
18 changes: 8 additions & 10 deletions api/src/paths/project/{projectId}/attachments/report/upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import { Operation } from 'express-openapi';
import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../constants/roles';
import { getDBConnection } from '../../../../../database/db';
import { HTTP400 } from '../../../../../errors/http-error';
import { fileSchema } from '../../../../../openapi/schemas/file';
import { authorizeRequestHandler } from '../../../../../request-handlers/security/authorization';
import { AttachmentService } from '../../../../../services/attachment-service';
import { scanFileForVirus, uploadFileToS3 } from '../../../../../utils/file-utils';
import { getLogger } from '../../../../../utils/logger';
import { getFileFromRequest } from '../../../../../utils/request';

const defaultLog = getLogger('/api/project/{projectId}/attachments/report/upload');

Expand Down Expand Up @@ -57,8 +59,11 @@ POST.apiDoc = {
required: ['media', 'attachmentMeta'],
properties: {
media: {
type: 'string',
format: 'binary'
description: 'Attachment report upload file.',
type: 'array',
minItems: 1,
maxItems: 1,
items: fileSchema
},
attachmentMeta: {
type: 'object',
Expand Down Expand Up @@ -140,14 +145,7 @@ POST.apiDoc = {
*/
export function uploadMedia(): RequestHandler {
return async (req, res) => {
const rawMediaArray: Express.Multer.File[] = req.files as Express.Multer.File[];

if (!rawMediaArray || !rawMediaArray.length) {
// no media objects included, skipping media upload step
throw new HTTP400('Missing upload data');
}

const rawMediaFile: Express.Multer.File = rawMediaArray[0];
const rawMediaFile = getFileFromRequest(req);

defaultLog.debug({
label: 'uploadMedia',
Expand Down
35 changes: 0 additions & 35 deletions api/src/paths/project/{projectId}/attachments/upload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,41 +36,6 @@ describe('uploadMedia', () => {
body: {}
} as any;

it('should throw an error when files are missing', async () => {
sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);

try {
const result = upload.uploadMedia();

await result({ ...mockReq, files: [] }, null as unknown as any, null as unknown as any);
expect.fail();
} catch (actualError) {
expect((actualError as HTTPError).status).to.equal(400);
expect((actualError as HTTPError).message).to.equal('Missing upload data');
}
});

it('should throw an error when file format incorrect', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
systemUserId: () => {
return 20;
}
});

sinon.stub(file_utils, 'scanFileForVirus').resolves(false);

try {
const result = upload.uploadMedia();

await result(mockReq, null as unknown as any, null as unknown as any);
expect.fail();
} catch (actualError) {
expect((actualError as HTTPError).status).to.equal(400);
expect((actualError as HTTPError).message).to.equal('Malicious content detected, upload cancelled');
}
});

it('should throw an error if failure occurs', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
Expand Down
18 changes: 8 additions & 10 deletions api/src/paths/project/{projectId}/attachments/upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import { ATTACHMENT_TYPE } from '../../../../constants/attachments';
import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../constants/roles';
import { getDBConnection } from '../../../../database/db';
import { HTTP400 } from '../../../../errors/http-error';
import { fileSchema } from '../../../../openapi/schemas/file';
import { authorizeRequestHandler } from '../../../../request-handlers/security/authorization';
import { AttachmentService } from '../../../../services/attachment-service';
import { scanFileForVirus, uploadFileToS3 } from '../../../../utils/file-utils';
import { getLogger } from '../../../../utils/logger';
import { getFileFromRequest } from '../../../../utils/request';

const defaultLog = getLogger('/api/project/{projectId}/attachments/upload');

Expand Down Expand Up @@ -58,8 +60,11 @@ POST.apiDoc = {
required: ['media'],
properties: {
media: {
type: 'string',
format: 'binary'
description: 'Attachment import file.',
type: 'array',
minItems: 1,
maxItems: 1,
items: fileSchema
}
}
}
Expand Down Expand Up @@ -115,14 +120,7 @@ POST.apiDoc = {
*/
export function uploadMedia(): RequestHandler {
return async (req, res) => {
const rawMediaArray: Express.Multer.File[] = req.files as Express.Multer.File[];

if (!rawMediaArray || !rawMediaArray.length) {
// no media objects included, skipping media upload step
throw new HTTP400('Missing upload data');
}

const rawMediaFile: Express.Multer.File = rawMediaArray[0];
const rawMediaFile = getFileFromRequest(req);

defaultLog.debug({
label: 'uploadMedia',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,6 @@ describe('uploadMedia', () => {

const mockBctwResponse = { totalKeyxFiles: 2, newRecords: 1, existingRecords: 1 };

it('should throw an error when files are missing', async () => {
sinon.stub(db, 'getDBConnection').returns(dbConnectionObj);

try {
const result = upload.uploadKeyxMedia();

await result({ ...mockReq, files: [] }, null as unknown as any, null as unknown as any);
expect.fail();
} catch (actualError) {
expect((actualError as HTTPError).status).to.equal(400);
expect((actualError as HTTPError).message).to.equal('Missing upload data');
}
});

it('should throw an error when file has malicious content', async () => {
sinon.stub(db, 'getDBConnection').returns({
...dbConnectionObj,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import { ATTACHMENT_TYPE } from '../../../../../../../constants/attachments';
import { PROJECT_PERMISSION, SYSTEM_ROLE } from '../../../../../../../constants/roles';
import { getDBConnection } from '../../../../../../../database/db';
import { HTTP400 } from '../../../../../../../errors/http-error';
import { fileSchema } from '../../../../../../../openapi/schemas/file';
import { authorizeRequestHandler } from '../../../../../../../request-handlers/security/authorization';
import { AttachmentService } from '../../../../../../../services/attachment-service';
import { BctwService, IBctwUser } from '../../../../../../../services/bctw-service';
import { scanFileForVirus, uploadFileToS3 } from '../../../../../../../utils/file-utils';
import { getLogger } from '../../../../../../../utils/logger';
import { checkFileForKeyx } from '../../../../../../../utils/media/media-utils';
import { getFileFromRequest } from '../../../../../../../utils/request';

const defaultLog = getLogger('/api/project/{projectId}/survey/{surveyId}/attachments/keyx/upload');

Expand Down Expand Up @@ -70,8 +72,11 @@ POST.apiDoc = {
required: ['media'],
properties: {
media: {
type: 'string',
format: 'binary'
description: 'Keyx import file.',
type: 'array',
minItems: 1,
maxItems: 1,
items: fileSchema
}
}
}
Expand Down Expand Up @@ -131,14 +136,7 @@ POST.apiDoc = {
*/
export function uploadKeyxMedia(): RequestHandler {
return async (req, res) => {
const rawMediaArray: Express.Multer.File[] = req.files as Express.Multer.File[];

if (!rawMediaArray?.length) {
// no media objects included, skipping media upload step
throw new HTTP400('Missing upload data');
}

const rawMediaFile: Express.Multer.File = rawMediaArray[0];
const rawMediaFile = getFileFromRequest(req);

defaultLog.debug({
label: 'uploadKeyxMedia',
Expand Down
Loading
Loading