Merge pull request #161 from adhocteam/js-265-add-missing-authorizati…

…on-checks Add two missing controller authorization checks
HHS · Feb 12, 2021 · 70644cc · 70644cc
2 parents cc6e6df + 5c0bc3c
commit 70644cc
Show file tree

Hide file tree

Showing 8 changed files with 104 additions and 43 deletions.
diff --git a/src/constants.js b/src/constants.js
@@ -0,0 +1,2 @@
+// eslint-disable-next-line import/prefer-default-export
+export const DECIMAL_BASE = 10;
diff --git a/src/policies/user.js b/src/policies/user.js
@@ -10,6 +10,7 @@ export default class Users {
     const permissions = this.user.permissions.find(
       (permission) => (
         (permission.scopeId === SCOPES.READ_WRITE_REPORTS
+          || permission.scopeId === SCOPES.READ_REPORTS
           || permission.scopeId === SCOPES.APPROVE_REPORTS)
         && permission.regionId === region),
     );

diff --git a/src/routes/activityReports/handlers.js b/src/routes/activityReports/handlers.js
@@ -1,11 +1,13 @@
 import handleErrors from '../../lib/apiErrorHandler';
 import SCOPES from '../../middleware/scopeConstants';
 import ActivityReport from '../../policies/activityReport';
+import User from '../../policies/user';
 import {
   possibleRecipients, activityReportById, createOrUpdate, review,
 } from '../../services/activityReports';
 import { goalsForGrants } from '../../services/goals';
 import { userById, usersWithPermissions } from '../../services/users';
+import { DECIMAL_BASE } from '../../constants';
 
 const { APPROVE_REPORTS } = SCOPES;
 
@@ -40,6 +42,14 @@ export async function getGoals(req, res) {
  */
 export async function getApprovers(req, res) {
   const { region } = req.query;
+  const user = await userById(req.session.userId);
+  const authorization = new User(user);
+
+  if (!authorization.canViewUsersInRegion(parseInt(region, DECIMAL_BASE))) {
+    res.sendStatus(403);
+    return;
+  }
+
   try {
     const users = await usersWithPermissions([region], [APPROVE_REPORTS]);
     res.json(users);

diff --git a/src/routes/activityReports/handlers.test.js b/src/routes/activityReports/handlers.test.js
@@ -12,6 +12,7 @@ import {
 } from '../../services/activityReports';
 import { userById, usersWithPermissions } from '../../services/users';
 import ActivityReport from '../../policies/activityReport';
+import User from '../../policies/user';
 
 jest.mock('../../services/activityReports', () => ({
   activityReportById: jest.fn(),
@@ -25,6 +26,7 @@ jest.mock('../../services/users', () => ({
   usersWithPermissions: jest.fn(),
 }));
 
+jest.mock('../../policies/user');
 jest.mock('../../policies/activityReport');
 
 const mockResponse = {
@@ -242,10 +244,23 @@ describe('Activity Report handlers', () => {
 
   describe('getApprovers', () => {
     it('returns a list of approvers', async () => {
+      User.mockImplementation(() => ({
+        canViewUsersInRegion: () => true,
+      }));
       const response = [{ name: 'name', id: 1 }];
       usersWithPermissions.mockResolvedValue(response);
       await getApprovers({ ...mockRequest, query: { region: 1 } }, mockResponse);
       expect(mockResponse.json).toHaveBeenCalledWith(response);
     });
+
+    it('handles unauthorized', async () => {
+      User.mockImplementation(() => ({
+        canViewUsersInRegion: () => false,
+      }));
+      const response = [{ name: 'name', id: 1 }];
+      usersWithPermissions.mockResolvedValue(response);
+      await getApprovers({ ...mockRequest, query: { region: 1 } }, mockResponse);
+      expect(mockResponse.sendStatus).toHaveBeenCalledWith(403);
+    });
   });
 });
diff --git a/src/routes/files/handlers.js b/src/routes/files/handlers.js
@@ -4,6 +4,10 @@ import handleErrors from '../../lib/apiErrorHandler';
 import { File } from '../../models';
 import s3Uploader from '../../lib/s3Uploader';
 
+import ActivityReportPolicy from '../../policies/activityReport';
+import { activityReportById } from '../../services/activityReports';
+import { userById } from '../../services/users';
+
 const fileType = require('file-type');
 const multiparty = require('multiparty');
 
@@ -48,6 +52,7 @@ export const updateStatus = async (fileId, fileStatus) => {
 export default async function uploadHandler(req, res) {
   const form = new multiparty.Form();
   form.parse(req, async (error, fields, files) => {
+    const { reportId, attachmentType } = fields;
     if (error) {
       res.status(500).send(error);
     }
@@ -56,13 +61,21 @@ export default async function uploadHandler(req, res) {
     let fileName;
     let type;
 
+    const user = await userById(req.session.userId);
+    const report = await activityReportById(reportId);
+    const authorization = new ActivityReportPolicy(user, report);
+
+    if (!authorization.canUpdate()) {
+      res.sendStatus(403);
+      return;
+    }
+
     try {
       if (!files.file) {
         res.status(400).send({ error: 'file required' });
         return;
       }
       const { path, originalFilename, size } = files.file[0];
-      const { reportId, attachmentType } = fields;
       if (!size) {
         res.status(400).send({ error: 'fileSize required' });
       }

diff --git a/src/routes/files/handlers.test.js b/src/routes/files/handlers.test.js
@@ -8,6 +8,9 @@ import db, {
 import app from '../../app';
 import s3Uploader from '../../lib/s3Uploader';
 import SCOPES from '../../middleware/scopeConstants';
+import ActivityReportPolicy from '../../policies/activityReport';
+
+jest.mock('../../policies/activityReport');
 
 const request = require('supertest');
 
@@ -50,31 +53,35 @@ const reportObject = {
 };
 
 describe('File Upload', () => {
-  afterAll(() => {
+  let user;
+  let report;
+  let fileId;
+  beforeAll(async () => {
+    user = await User.create(mockUser, { include: [{ model: Permission, as: 'permissions' }] });
+    report = await ActivityReport.create(reportObject);
+    process.env.NODE_ENV = 'test';
+    process.env.BYPASS_AUTH = 'true';
+    process.env.CURRENT_USER_ID = 100;
+  });
+  afterAll(async () => {
+    await File.destroy({ where: {} });
+    await ActivityReport.destroy({ where: { } });
+    await User.destroy({ where: { id: user.id } });
+    process.env = ORIGINAL_ENV; // restore original env
     db.sequelize.close();
   });
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
 
   describe('File Upload Handlers happy path', () => {
-    let user;
-    let report;
-    let fileId;
-    beforeAll(async () => {
-      user = await User.create(mockUser, { include: [{ model: Permission, as: 'permissions' }] });
-      report = await ActivityReport.create(reportObject);
-      process.env.NODE_ENV = 'test';
-      process.env.BYPASS_AUTH = 'true';
-      process.env.CURRENT_USER_ID = 100;
-    });
-    afterAll(async () => {
-      await File.destroy({ where: {} });
-      await ActivityReport.destroy({ where: { } });
-      await User.destroy({ where: { id: user.id } });
-      process.env = ORIGINAL_ENV; // restore original env
-    });
     beforeEach(() => {
       s3Uploader.mockReset();
     });
     it('tests a file upload', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       await request(app)
         .post('/api/files')
         .field('reportId', report.dataValues.id)
@@ -87,6 +94,9 @@ describe('File Upload', () => {
         });
     });
     it('checks the metadata was uploaded to the database', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       const file = await File.findOne({ where: { id: fileId } });
       const uuid = file.dataValues.key.slice(0, -4);
       expect(file.dataValues.id).toBe(fileId);
@@ -98,23 +108,10 @@ describe('File Upload', () => {
   });
 
   describe('File Upload Handlers error handling', () => {
-    let user;
-    let report;
-    beforeAll(async () => {
-      user = await User.create(mockUser, { include: [{ model: Permission, as: 'permissions' }] });
-      report = await ActivityReport.create(reportObject);
-      process.env.NODE_ENV = 'test';
-      process.env.BYPASS_AUTH = 'true';
-      process.env.CURRENT_USER_ID = 100;
-    });
-    afterAll(async () => {
-      await File.destroy({ where: {} });
-      await ActivityReport.destroy({ where: { } });
-      await User.destroy({ where: { id: user.id } });
-      db.sequelize.close();
-      process.env = ORIGINAL_ENV; // restore original env
-    });
     it('tests a file upload without a report id', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       await request(app)
         .post('/api/files')
         .field('attachmentType', 'ATTACHMENT')
@@ -123,6 +120,9 @@ describe('File Upload', () => {
         .then(() => expect(s3Uploader).not.toHaveBeenCalled());
     });
     it('tests a file upload without a file', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       await request(app)
         .post('/api/files')
         .field('reportId', report.dataValues.id)
@@ -131,6 +131,9 @@ describe('File Upload', () => {
         .then(() => expect(s3Uploader).not.toHaveBeenCalled());
     });
     it('tests a file upload without a attachment', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       await request(app)
         .post('/api/files')
         .field('reportId', report.dataValues.id)
@@ -139,6 +142,9 @@ describe('File Upload', () => {
         .then(() => expect(s3Uploader).not.toHaveBeenCalled());
     });
     it('tests a file upload with an incorrect attachment value', async () => {
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => true,
+      }));
       await request(app)
         .post('/api/files')
         .field('reportId', report.dataValues.id)
@@ -147,5 +153,18 @@ describe('File Upload', () => {
         .expect(400, { error: 'incorrect attachmentType. Wanted: ATTACHMENT or RESOURCE. Got: FAKE' })
         .then(() => expect(s3Uploader).not.toHaveBeenCalled());
     });
+    it('tests an unauthorized upload', async () => {
+      jest.clearAllMocks();
+      ActivityReportPolicy.mockImplementation(() => ({
+        canUpdate: () => false,
+      }));
+      await request(app)
+        .post('/api/files')
+        .field('reportId', report.dataValues.id)
+        .field('attachmentType', 'ATTACHMENT')
+        .attach('file', `${__dirname}/testfiles/testfile.pdf`)
+        .expect(403)
+        .then(() => expect(s3Uploader).not.toHaveBeenCalled());
+    });
   });
 });
diff --git a/src/routes/users/handlers.js b/src/routes/users/handlers.js
@@ -1,15 +1,16 @@
 /* eslint-disable import/prefer-default-export */
-import Users from '../../policies/user';
+import UserPolicy from '../../policies/user';
 import SCOPES from '../../middleware/scopeConstants';
 import { userById, usersWithPermissions } from '../../services/users';
 import handleErrors from '../../lib/apiErrorHandler';
+import { DECIMAL_BASE } from '../../constants';
 
 export async function getPossibleCollaborators(req, res) {
   try {
     const user = await userById(req.session.userId);
     const { region } = req.query;
-    const authorization = new Users(user);
-    if (!authorization.canViewUsersInRegion(parseInt(region, 10))) {
+    const authorization = new UserPolicy(user);
+    if (!authorization.canViewUsersInRegion(parseInt(region, DECIMAL_BASE))) {
       res.sendStatus(403);
       return;
     }

diff --git a/src/services/activityReports.test.js b/src/services/activityReports.test.js
@@ -6,7 +6,7 @@ import {
 } from './activityReports';
 
 const mockUser = {
-  id: 100,
+  id: 1000,
   homeRegionId: 1,
   name: 'user',
 };
@@ -34,7 +34,7 @@ describe('Activity Reports DB service', () => {
   afterAll(async () => {
     await ActivityRecipient.destroy({ where: {} });
     await ActivityReport.destroy({ where: {} });
-    await User.destroy({ where: { id: 100 } });
+    await User.destroy({ where: { id: mockUser.id } });
     await NonGrantee.destroy({ where: { id: 100 } });
     await Grant.destroy({ where: { id: 100 } });
     await Grantee.destroy({ where: { id: 100 } });
@@ -61,15 +61,15 @@ describe('Activity Reports DB service', () => {
     });
 
     it('creates a new report with non-grantee recipient', async () => {
-      const beginningARCount = await ActivityReport.count();
       const report = await createOrUpdate({ ...reportObject, activityRecipientType: 'non-grantee' });
-      const endARCount = await ActivityReport.count();
-      expect(endARCount - beginningARCount).toBe(1);
       expect(report.activityRecipients[0].nonGrantee.id).toBe(100);
     });
 
     it('handles reports with collaborators', async () => {
-      const report = await createOrUpdate({ ...reportObject, collaborators: [{ id: 100 }] });
+      const report = await createOrUpdate({
+        ...reportObject,
+        collaborators: [{ id: mockUser.id }],
+      });
       expect(report.collaborators.length).toBe(1);
       expect(report.collaborators[0].name).toBe('user');
     });
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		// eslint-disable-next-line import/prefer-default-export
		export const DECIMAL_BASE = 10;