diff --git a/src/middleware/userAdminAccessMiddleware.js b/src/middleware/userAdminAccessMiddleware.js index 8790dd8011..5a94db2912 100644 --- a/src/middleware/userAdminAccessMiddleware.js +++ b/src/middleware/userAdminAccessMiddleware.js @@ -16,9 +16,7 @@ import handleErrors from '../lib/apiErrorHandler'; export default async function userAdminAccessMiddleware(req, res, next) { try { const { userId } = req.session; - if ((await validateUserAuthForAdmin(userId))) { - auditLogger.info(`User ${userId} successfully checked ADMIN access`); - } else { + if (!(await validateUserAuthForAdmin(userId))) { auditLogger.error(`User ${userId} attempted to access an ADMIN route without permission`); // consider sending a 404 rather than a 403 (Forbidden) to avoid confirming route return res.sendStatus(httpCodes.FORBIDDEN); diff --git a/src/routes/files/handlers.js b/src/routes/files/handlers.js index 3c94bd3291..86d725c318 100644 --- a/src/routes/files/handlers.js +++ b/src/routes/files/handlers.js @@ -9,6 +9,7 @@ import createFileMetaData, { import ActivityReportPolicy from '../../policies/activityReport'; import { activityReportById } from '../../services/activityReports'; import { userById } from '../../services/users'; +import { validateUserAuthForAdmin } from '../../services/accessValidation'; import { auditLogger } from '../../logger'; import { FILE_STATUSES } from '../../constants'; @@ -53,10 +54,10 @@ export const deleteHandler = async (req, res) => { export default async function uploadHandler(req, res) { const form = new multiparty.Form(); - form.parse(req, async (error, fields, files) => { + await form.parse(req, async (error, fields, files) => { const { reportId } = fields; if (error) { - res.status(500).send(error); + return res.status(500).send(error); } let buffer; let metadata; @@ -67,29 +68,25 @@ export default async function uploadHandler(req, res) { const report = await activityReportById(reportId); const authorization = new ActivityReportPolicy(user, report); - if (!authorization.canUpdate()) { - res.sendStatus(403); - return; + if (!(authorization.canUpdate() || (await validateUserAuthForAdmin(req.session.userId)))) { + return res.sendStatus(403); } try { if (!files.file) { - res.status(400).send({ error: 'file required' }); - return; + return res.status(400).send({ error: 'file required' }); } const { path, originalFilename, size } = files.file[0]; if (!size) { - res.status(400).send({ error: 'fileSize required' }); + return res.status(400).send({ error: 'fileSize required' }); } if (!reportId) { - res.status(400).send({ error: 'reportId required' }); - return; + return res.status(400).send({ error: 'reportId required' }); } buffer = fs.readFileSync(path); type = await fileType.fromFile(path); if (!type) { - res.status(400).send('Could not determine file type'); - return; + return res.status(400).send('Could not determine file type'); } fileName = `${uuidv4()}.${type.ext}`; metadata = await createFileMetaData( diff --git a/src/routes/files/handlers.test.js b/src/routes/files/handlers.test.js index 0b9fcb3d60..ee7fbdc259 100644 --- a/src/routes/files/handlers.test.js +++ b/src/routes/files/handlers.test.js @@ -3,7 +3,6 @@ import db, { File, ActivityReport, User, - Permission, } from '../../models'; import app from '../../app'; import { uploadFile, deleteFileFromS3, getPresignedURL } from '../../lib/s3'; @@ -12,8 +11,13 @@ import SCOPES from '../../middleware/scopeConstants'; import { REPORT_STATUSES, FILE_STATUSES } from '../../constants'; import ActivityReportPolicy from '../../policies/activityReport'; import * as Files from '../../services/files'; +import { validateUserAuthForAdmin, validateUserAuthForAccess } from '../../services/accessValidation'; jest.mock('../../policies/activityReport'); +jest.mock('../../services/accessValidation', () => ({ + validateUserAuthForAdmin: jest.fn().mockResolvedValue(false), + validateUserAuthForAccess: jest.fn().mockResolvedValue(true), +})); const request = require('supertest'); @@ -26,23 +30,6 @@ const mockUser = { hsesUserId: '2046', hsesUsername: '2046', homeRegionId: 1, - permissions: [ - { - userId: 2046, - regionId: 5, - scopeId: SCOPES.READ_WRITE_REPORTS, - }, - { - userId: 2046, - regionId: 6, - scopeId: SCOPES.READ_WRITE_REPORTS, - }, - { - userId: 2046, - regionId: 14, - scopeId: SCOPES.SITE_ACCESS, - }, - ], }; const mockSession = jest.fn(); @@ -63,7 +50,7 @@ describe('File Upload', () => { let report; let fileId; beforeAll(async () => { - user = await User.create(mockUser, { include: [{ model: Permission, as: 'permissions' }] }); + user = await User.create(mockUser); report = await ActivityReport.create(reportObject); process.env.NODE_ENV = 'test'; process.env.BYPASS_AUTH = 'true'; @@ -76,7 +63,7 @@ describe('File Upload', () => { process.env = ORIGINAL_ENV; // restore original env await db.sequelize.close(); }); - afterEach(() => { + beforeEach(() => { jest.clearAllMocks(); }); @@ -90,15 +77,35 @@ describe('File Upload', () => { canUpdate: () => true, })); uploadFile.mockResolvedValue({ key: 'key' }); - await request(app) + const response = await request(app) .post('/api/files') .field('reportId', report.dataValues.id) .attach('file', `${__dirname}/testfiles/testfile.pdf`) - .expect(200) - .then((res) => { - fileId = res.body.id; - expect(uploadFile).toHaveBeenCalled(); - }); + .expect(200); + fileId = response.body.id; + expect(uploadFile).toHaveBeenCalled(); + expect(mockAddToScanQueue).toHaveBeenCalled(); + const file = await File.findOne({ where: { id: fileId } }); + const uuid = file.dataValues.key.slice(0, -4); + expect(file.dataValues.id).toBe(fileId); + expect(file.dataValues.status).not.toBe(null); + expect(file.dataValues.originalFileName).toBe('testfile.pdf'); + expect(file.dataValues.activityReportId).toBe(report.dataValues.id); + expect(validate(uuid)).toBe(true); + }); + it('allows an admin to upload a file', async () => { + ActivityReportPolicy.mockImplementation(() => ({ + canUpdate: () => false, + })); + validateUserAuthForAdmin.mockResolvedValue(true); + uploadFile.mockResolvedValue({ key: 'key' }); + const response = await request(app) + .post('/api/files') + .field('reportId', report.dataValues.id) + .attach('file', `${__dirname}/testfiles/testfile.pdf`) + .expect(200); + fileId = response.body.id; + expect(uploadFile).toHaveBeenCalled(); expect(mockAddToScanQueue).toHaveBeenCalled(); const file = await File.findOne({ where: { id: fileId } }); const uuid = file.dataValues.key.slice(0, -4); @@ -155,9 +162,7 @@ describe('File Upload', () => { .post('/api/files') .attach('file', `${__dirname}/testfiles/testfile.pdf`) .expect(400, { error: 'reportId required' }) - .then(() => { - expect(uploadFile).not.toHaveBeenCalled(); - }); + await expect(uploadFile).not.toHaveBeenCalled(); }); it('tests a file upload without a file', async () => { ActivityReportPolicy.mockImplementation(() => ({ @@ -172,7 +177,7 @@ describe('File Upload', () => { }); }); it('tests an unauthorized upload', async () => { - jest.clearAllMocks(); + validateUserAuthForAdmin.mockResolvedValue(false); ActivityReportPolicy.mockImplementation(() => ({ canUpdate: () => false, })); diff --git a/src/services/accessValidation.js b/src/services/accessValidation.js index 51611de6fd..689cdf2d1e 100644 --- a/src/services/accessValidation.js +++ b/src/services/accessValidation.js @@ -65,7 +65,12 @@ export async function validateUserAuthForAdmin(userId) { scopeId: ADMIN, }, }); - return userPermission !== null; + if (userPermission !== null) { + logger.info(`User ${userId} successfully checked ADMIN access`); + return true; + } + logger.warn(`User ${userId} unsuccessfully checked ADMIN access`); + return false; } catch (error) { logger.error(`${JSON.stringify({ ...logContext })} - ADMIN Access error - ${error}`); throw error;