From 6e57e51b3d7f479d2def828f8aeda117f139201b Mon Sep 17 00:00:00 2001 From: Sanket Dharwadkar Date: Wed, 18 Nov 2020 14:52:25 -0500 Subject: [PATCH] fix: adding validation for enhanced study security (#211) * fix: adding validation for enhanced study security * feat: adding unit tests for study service --- .../base-raas-services/lib/helpers/is-role.js | 5 + .../lib/study/__tests__/study-service.test.js | 94 +++++++++++++++++-- .../lib/study/study-service.js | 13 ++- 3 files changed, 103 insertions(+), 9 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/helpers/is-role.js b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/is-role.js index 047f9abe3f..2fcd2b59ad 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/helpers/is-role.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/helpers/is-role.js @@ -24,10 +24,15 @@ function isAdmin(requestContext) { return isRole(requestContext, 'admin'); } +function isSystem(requestContext) { + return _.get(requestContext, 'principalIdentifier.uid') === '_system_'; +} + module.exports = { isInternalResearcher, isExternalResearcher, isInternalGuest, isExternalGuest, isAdmin, + isSystem, }; diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-service.test.js index 2b217de80e..2a6ebac422 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/study/__tests__/study-service.test.js @@ -149,7 +149,7 @@ describe('studyService', () => { }); // OPERATE try { - await service.create({ principal: { userRole: 'admin' } }, dataIpt); + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, dataIpt); expect.hasAssertions(); } catch (err) { // CHECK @@ -157,6 +157,88 @@ describe('studyService', () => { } }); + it('should fail if non-system user is trying to create Open Data study', async () => { + // BUILD + const dataIpt = { + id: 'newOpenStudy', + category: 'Open Data', + }; + + // OPERATE + try { + await service.create( + { principal: { userRole: 'admin' }, principalIdentifier: { uid: 'someRandomUserUid' } }, + dataIpt, + ); + expect.hasAssertions(); + } catch (err) { + // CHECK + expect(err.message).toEqual('Only the system can create Open Data studies.'); + } + }); + + it('should pass if system is trying to create Open Data study', async () => { + // BUILD + const dataIpt = { + id: 'newOpenStudy', + category: 'Open Data', + }; + service.audit = jest.fn(); + + // OPERATE + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, dataIpt); + + // CHECK + expect(dbService.table.update).toHaveBeenCalled(); + expect(service.audit).toHaveBeenCalledWith( + { principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, + { action: 'create-study', body: undefined }, + ); + }); + + it('should fail if non-Open Data study type has non-empty resources list', async () => { + // BUILD + const dataIpt = { + id: 'newOpenStudy', + category: 'Organization', + projectId: 'existingProjId', + resources: [{ arn: 'arn:aws:s3:::someRandomStudyArn' }], + }; + projectService.verifyUserProjectAssociation.mockImplementationOnce(() => true); + + // OPERATE + try { + await service.create( + { principal: { userRole: 'admin' }, principalIdentifier: { uid: 'someRandomUserUid' } }, + dataIpt, + ); + expect.hasAssertions(); + } catch (err) { + // CHECK + expect(err.message).toEqual('Resources can only be assigned to Open Data study category'); + } + }); + + it('should pass if Open Data study type has non-empty resources list', async () => { + // BUILD + const dataIpt = { + id: 'newOpenStudy', + category: 'Open Data', + resources: [{ arn: 'arn:aws:s3:::someRandomStudyArn' }], + }; + service.audit = jest.fn(); + + // OPERATE + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, dataIpt); + + // CHECK + expect(dbService.table.update).toHaveBeenCalled(); + expect(service.audit).toHaveBeenCalledWith( + { principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, + { action: 'create-study', body: undefined }, + ); + }); + it('should try to create the study successfully', async () => { // BUILD const dataIpt = { @@ -167,12 +249,12 @@ describe('studyService', () => { service.audit = jest.fn(); // OPERATE - await service.create({ principal: { userRole: 'admin' } }, dataIpt); + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, dataIpt); // CHECK expect(dbService.table.update).toHaveBeenCalled(); expect(service.audit).toHaveBeenCalledWith( - { principal: { userRole: 'admin' } }, + { principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, { action: 'create-study', body: undefined }, ); }); @@ -188,12 +270,12 @@ describe('studyService', () => { service.audit = jest.fn(); // OPERATE - await service.create({ principal: { userRole: 'admin' } }, dataIpt); + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, dataIpt); // CHECK expect(dbService.table.update).toHaveBeenCalled(); expect(service.audit).toHaveBeenCalledWith( - { principal: { userRole: 'admin' } }, + { principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, { action: 'create-study', body: undefined }, ); }); @@ -298,7 +380,7 @@ describe('studyService', () => { // OPERATE try { - await service.create({ principal: { userRole: 'admin' } }, ipt); + await service.create({ principal: { userRole: 'admin' }, principalIdentifier: { uid: '_system_' } }, ipt); expect.hasAssertions(); } catch (err) { // CHECK diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/study/study-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/study/study-service.js index 2f062d91fd..6d3840a13e 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/study/study-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/study/study-service.js @@ -18,7 +18,7 @@ const Service = require('@aws-ee/base-services-container/lib/service'); const { runAndCatch } = require('@aws-ee/base-services/lib/helpers/utils'); const { buildTaggingXml } = require('../helpers/aws-tags'); -const { isInternalResearcher, isAdmin } = require('../helpers/is-role'); +const { isInternalResearcher, isAdmin, isSystem } = require('../helpers/is-role'); const createSchema = require('../schema/create-study'); const updateSchema = require('../schema/update-study'); @@ -89,6 +89,9 @@ class StudyService extends Service { if (!(isInternalResearcher(requestContext) || isAdmin(requestContext))) { throw this.boom.forbidden('Only admin and internal researcher are authorized to create studies. '); } + if (rawData.category === 'Open Data' && !isSystem(requestContext)) { + throw this.boom.badRequest('Only the system can create Open Data studies.', true); + } const [validationService, projectService] = await this.service(['jsonSchemaValidationService', 'projectService']); // Validate input @@ -105,13 +108,17 @@ class StudyService extends Service { if (rawData.category !== 'Open Data') { const projectId = rawData.projectId; if (!projectId) { - throw this.boom.badRequest('Missing required projectId'); + throw this.boom.badRequest('Missing required projectId', true); } // Verify user has access to the project the new study will be associated with if (!(await projectService.verifyUserProjectAssociation(by, projectId))) { - throw this.boom.forbidden(`Not authorized to add study related to project "${projectId}"`); + throw this.boom.forbidden(`Not authorized to add study related to project "${projectId}"`, true); } await projectService.mustFind(requestContext, { id: rawData.projectId }); + // Verify user is not trying to create resources for non-Open data studies + if (!_.isEmpty(rawData.resources)) { + throw this.boom.badRequest('Resources can only be assigned to Open Data study category', true); + } } const id = rawData.id;