fix: adding validation for study update route (#215)

* fix: adding validation for study update route * doc: updating changelog * fix: test code cleanup
awslabs · Nov 23, 2020 · 9bc2d90 · 9bc2d90
1 parent 06b2780
commit 9bc2d90
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,27 @@
 
 All notable changes to this project will be documented in this file.
 
+## [1.4.2] - 2020-11-23
+
+### Added 
+- fix: Fix a bug on the update study API
+
+We recommend to apply this patch as soon as possible
+
+## [1.4.1] - 2020-11-18
+
+### Added 
+- fix: Handling policy names for windows envs
+- fix: Fix a bug on the create study API
+
+We recommend to apply this patch as soon as possible
+
+## [1.4.0] - 2020-11-13
+
+### Added 
+- feat: Study Read/Write and Permission propagation (Goofys)
+- feat: Read/Only study mounts on AWS Service Catalog based EC2 Windows workspaces
+
 ## [1.3.2] - 2020-10-23
 
 ### Added 

diff --git a/.../addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js b/.../addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js
@@ -794,7 +794,7 @@ class EnvironmentMountService extends Service {
       const requestedStudyIds = studyInfo.map(study => study.id);
 
       // Retrieve and verify user's study permissions
-      const studyPermissionService = await this.service('studyPermissionService');
+      const [studyPermissionService, studyService] = await this.service(['studyPermissionService', 'studyService']);
       const storedPermissions = await studyPermissionService.getRequestorPermissions(requestContext);
 
       // If there are no stored permissions, use an empty permissions object
@@ -806,10 +806,10 @@ class EnvironmentMountService extends Service {
       );
 
       // Determine whether any forbidden studies were requested
-      const allowedStudies = permissions.adminAccess.concat(permissions.readonlyAccess);
+      const allowedStudies = studyService.getAllowedStudies(permissions);
       const forbiddenStudies = _.difference(requestedStudyIds, allowedStudies);
 
-      if (forbiddenStudies.length) {
+      if (!_.isEmpty(forbiddenStudies)) {
         throw new Error(`Studies not found: ${forbiddenStudies.join(',')}`);
       }
     }

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
@@ -222,7 +222,7 @@ describe('studyService', () => {
     it('should get the correct allowed studies ONLY (admin, R/O, R/W)', async () => {
       // BUILD, OPERATE and CHECK
       expect(
-        service._getAllowedStudies({
+        service.getAllowedStudies({
           adminAccess: ['studyA'],
           readonlyAccess: ['studyB'],
           readwriteAccess: ['studyC'],
@@ -251,6 +251,46 @@ describe('studyService', () => {
       );
     });
 
+    it('should fail to update resource list of non-Open Data study', async () => {
+      // BUILD
+      const dataIpt = {
+        id: 'newOpenStudy',
+        category: 'Organization',
+        resources: [{ arn: 'arn:aws:s3:::someRandomStudyArn' }],
+      };
+      service.audit = jest.fn();
+
+      // OPERATE
+      await expect(
+        service.update(
+          { principal: { userRole: 'researcher' }, principalIdentifier: { uid: 'someRandomUserUid' } },
+          dataIpt,
+        ),
+      ).rejects.toThrow({
+        message: 'Resources can only be updated for Open Data study category',
+      });
+    });
+
+    it('should fail to update Open Data study by non-system user', async () => {
+      // BUILD
+      const dataIpt = {
+        id: 'newOpenStudy',
+        category: 'Open Data',
+        resources: [{ arn: 'arn:aws:s3:::someRandomStudyArn' }],
+      };
+      service.audit = jest.fn();
+
+      // OPERATE
+      await expect(
+        service.update(
+          { principal: { userRole: 'admin' }, principalIdentifier: { uid: 'someRandomUserUid' } },
+          dataIpt,
+        ),
+      ).rejects.toThrow({
+        message: 'Only the system can update Open Data studies.',
+      });
+    });
+
     it('should try to create the study successfully', async () => {
       // BUILD
       const dataIpt = {

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
@@ -171,6 +171,14 @@ class StudyService extends Service {
   async update(requestContext, rawData) {
     const [validationService] = await this.service(['jsonSchemaValidationService']);
 
+    if (rawData.category === 'Open Data' && !isSystem(requestContext)) {
+      throw this.boom.badRequest('Only the system can update Open Data studies.', true);
+    }
+
+    if (rawData.category !== 'Open Data' && !_.isEmpty(rawData.resources)) {
+      throw this.boom.badRequest('Resources can only be updated for Open Data study category', true);
+    }
+
     // Validate input
     await validationService.ensureValid(rawData, updateSchema);
 
@@ -243,7 +251,7 @@ class StudyService extends Service {
     return result;
   }
 
-  _getAllowedStudies(permissions = []) {
+  getAllowedStudies(permissions = []) {
     const adminAccess = permissions.adminAccess || [];
     const readonlyAccess = permissions.readonlyAccess || [];
     const readwriteAccess = permissions.readwriteAccess || [];
@@ -269,7 +277,7 @@ class StudyService extends Service {
         const permissions = await this.studyPermissionService.getRequestorPermissions(requestContext);
         if (permissions) {
           // We can't give duplicate keys to the batch get, so ensure that allowedStudies is unique
-          const allowedStudies = this._getAllowedStudies(permissions);
+          const allowedStudies = this.getAllowedStudies(permissions);
           if (allowedStudies.length) {
             const rawResult = await this._getter()
               .keys(allowedStudies.map(studyId => ({ id: studyId })))