diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/environment/__tests__/environment-mount-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/environment/__tests__/environment-mount-service.test.js index 51ec0538ca..cccc43927f 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/environment/__tests__/environment-mount-service.test.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/environment/__tests__/environment-mount-service.test.js @@ -634,7 +634,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: [`${studyBucket}/${studyPrefix}`], }, ], @@ -695,7 +701,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: ['StudyBucketPath_XYZ'], }, ], @@ -736,7 +748,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: ['StudyBucketPath_XYZ', `${studyBucket}/${studyPrefix}`], }, ], @@ -882,7 +900,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: ['AnotherStudyBucketPath', `${studyBucket}/${studyPrefix}`], }, ], @@ -917,7 +941,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: ['AnotherStudyBucketPath'], }, { @@ -1022,7 +1052,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: [`${studyBucket}/${studyPrefix}`], }, ], @@ -1172,7 +1208,13 @@ describe('EnvironmentMountService', () => { { Sid: 'S3StudyReadWriteAccess', Effect: 'Allow', - Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], Resource: [`${studyBucket}/${studyPrefix}`], }, { @@ -1232,4 +1274,106 @@ describe('EnvironmentMountService', () => { ); expect(IamUpdateParams.policyDoc).toMatchObject(expectedPolicy); }); + + it('should ensure permission updates when admins are removed are as expected', async () => { + // BUILD + const updateRequest = { + usersToAdd: [{ uid: 'User1-UID', permissionLevel: 'readwrite' }], + usersToRemove: [{ uid: 'User1-UID', permissionLevel: 'admin' }], + }; + const studyId = 'StudyA'; + const envsForUser = [{ studyIds: ['StudyA'] }]; + environmentScService.getActiveEnvsForUser = jest.fn().mockResolvedValue(envsForUser); + iamService.putRolePolicy = jest.fn(); + const studyBucket = 'arn:aws:s3:::xxxxxxxx-namespace-studydata'; + const studyPrefix = 'studies/Organization/SampleStudy/*'; + const inputPolicy = { + Version: '2012-10-17', + Statement: [ + { + Sid: 'studyKMSAccess', + Action: ['Permission1', 'Permission2'], + Effect: 'Allow', + Resource: 'arn:aws:kms:region:xxxxxxxx:key/someRandomString', + }, + { + Sid: 'studyListS3AccessN', + Effect: 'Allow', + Action: 's3:ListBucket', + Resource: studyBucket, + Condition: { + StringLike: { + 's3:prefix': ['AnotherStudyPrefixForThisBucket', studyPrefix], + }, + }, + }, + { + Sid: 'S3StudyReadAccess', + Effect: 'Allow', + Action: ['s3:GetObject'], + Resource: ['AnotherStudyBucketPath', `${studyBucket}/${studyPrefix}`], + }, + ], + }; + const IamUpdateParams = { + iamClient: 'sampleIamClient', + studyPathArn: `${studyBucket}/${studyPrefix}`, + policyDoc: inputPolicy, + roleName: 'sampleRoleName', + studyDataPolicyName: 'sampleStudyDataPolicy', + }; + const expectedPolicy = { + Version: '2012-10-17', + Statement: [ + { + Sid: 'studyKMSAccess', + Action: ['Permission1', 'Permission2'], + Effect: 'Allow', + Resource: 'arn:aws:kms:region:xxxxxxxx:key/someRandomString', + }, + { + Sid: 'studyListS3AccessN', + Effect: 'Allow', + Action: 's3:ListBucket', + Resource: studyBucket, + Condition: { + StringLike: { + 's3:prefix': ['AnotherStudyPrefixForThisBucket', studyPrefix], + }, + }, + }, + { + Sid: 'S3StudyReadAccess', + Effect: 'Allow', + Action: ['s3:GetObject'], + Resource: ['AnotherStudyBucketPath'], + }, + { + Sid: 'S3StudyReadWriteAccess', + Effect: 'Allow', + Action: [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ], + Resource: [`${studyBucket}/${studyPrefix}`], + }, + ], + }; + service._getIamUpdateParams = jest.fn().mockResolvedValue(IamUpdateParams); + + // OPERATE + await service.applyWorkspacePermissions(studyId, updateRequest); + + // CHECK + expect(iamService.putRolePolicy).toHaveBeenCalledWith( + IamUpdateParams.roleName, + IamUpdateParams.studyDataPolicyName, + JSON.stringify(IamUpdateParams.policyDoc), + IamUpdateParams.iamClient, + ); + expect(IamUpdateParams.policyDoc).toMatchObject(expectedPolicy); + }); }); diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js index 5807a5e5cf..c2c787b77a 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js +++ b/addons/addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js @@ -70,7 +70,9 @@ class EnvironmentMountService extends Service { const iamPolicyDocument = await this._generateIamPolicyDoc(studyInfo); return { - s3Mounts: JSON.stringify(s3Mounts.map(({ id, bucket, prefix }) => ({ id, bucket, prefix }))), + s3Mounts: JSON.stringify( + s3Mounts.map(({ id, bucket, prefix, writeable }) => ({ id, bucket, prefix, writeable })), + ), iamPolicyDocument: JSON.stringify(iamPolicyDocument), environmentInstanceFiles: this.settings.get(settingKeys.environmentInstanceFiles), s3Prefixes: s3Mounts.filter(({ category }) => category !== 'Open Data').map(mount => mount.prefix), @@ -166,11 +168,9 @@ class EnvironmentMountService extends Service { Sid: putSid, Effect: 'Allow', Principal: { AWS: [] }, - Action: ['s3:PutObject'], + Action: ['s3:AbortMultipartUpload', 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl'], Resource: [`arn:aws:s3:::${s3BucketName}/${prefix}*`], }; - // For writeable permission, PutObjectAcl is not required on the S3 bucket policy - // but is required on Workspace Role policy // Pull out existing statements if available statements.forEach(statement => { @@ -309,8 +309,10 @@ class EnvironmentMountService extends Service { const errors = []; await Promise.all( _.map(allowedUsers, async user => { + const existingStudyAdmins = _.filter(updateRequest.usersToAdd, u => u.permissionLevel === 'admin'); + const removedStudyAdmins = _.filter(updateRequest.usersToRemove, u => u.permissionLevel === 'admin'); const isStudyAdmin = !_.isEmpty( - _.filter(updateRequest.usersToAdd, u => u.permissionLevel === 'admin' && u.uid === user.uid), + _.filter([...existingStudyAdmins, ...removedStudyAdmins], u => u.uid === user.uid), ); const userOwnedEnvs = await environmentScService.getActiveEnvsForUser(user.uid); const envsWithStudy = _.filter(userOwnedEnvs, env => _.includes(env.studyIds, studyId)); @@ -533,7 +535,13 @@ class EnvironmentMountService extends Service { Effect: 'Allow', Action: statementSidToUse === readWriteStatementId - ? ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'] + ? [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ] : ['s3:GetObject'], Resource: [studyPathArn], }; @@ -771,11 +779,11 @@ class EnvironmentMountService extends Service { if (studyInfo.length) { // There might be multiple resources. In the future we may flatMap, for now... mounts = studyInfo.reduce( - (result, { id, resources, category }) => + (result, { id, resources, category, writeable }) => result.concat( resources.map(resource => { const { bucket, prefix } = parseS3Arn(resource.arn); - return { id, bucket, prefix, category }; + return { id, bucket, prefix, category, writeable }; }), ), [], @@ -823,7 +831,13 @@ class EnvironmentMountService extends Service { const readonlyStudies = _.filter(studyInfo, study => !study.writeable); if (writeableStudies.length && writeableStudies.length > 0) { - const objectLevelWriteActions = ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl']; + const objectLevelWriteActions = [ + 's3:GetObject', + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + ]; statements.push({ Sid: readWriteStatementId, Effect: 'Allow', 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 526a9417e0..2f062d91fd 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 @@ -264,11 +264,14 @@ class StudyService extends Service { // Filter by category and inject requestor's access level const studyAccessMap = {}; - ['admin', 'readwrite', 'readonly'].forEach(level => - permissions[`${level}Access`].forEach(studyId => { - studyAccessMap[studyId] = level; - }), - ); + ['admin', 'readwrite', 'readonly'].forEach(level => { + const studiesWithPermission = permissions[`${level}Access`]; + if (studiesWithPermission && studiesWithPermission.length > 0) + studiesWithPermission.forEach(studyId => { + studyAccessMap[studyId] = level; + }); + }); + result = rawResult .filter(study => study.category === category) .map(study => ({