From c819e2857c5d3f9e1c8b1a4f208b0c1643af800f Mon Sep 17 00:00:00 2001 From: Sanket Dharwadkar Date: Fri, 13 Nov 2020 17:08:47 -0500 Subject: [PATCH] feat: added delete permissions, handled add logic (#209) --- .../environment-mount-service.test.js | 116 +++++++++++++++++- .../environment/environment-mount-service.js | 32 +++-- 2 files changed, 132 insertions(+), 16 deletions(-) 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 cccc43927f..00ac3f323a 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 @@ -107,11 +107,7 @@ describe('EnvironmentMountService', () => { await service.applyWorkspacePermissions(studyId, updateRequest); // CHECK - expect(service.addPermissions).toHaveBeenCalledWith( - [{ uid: 'User1-UID', permissionLevel: 'readonly' }], - studyId, - updateRequest, - ); + expect(service.addPermissions).toHaveBeenCalledWith([{ uid: 'User1-UID', permissionLevel: 'readonly' }], studyId); expect(service.removePermissions).not.toHaveBeenCalled(); expect(service.updatePermissions).not.toHaveBeenCalled(); }); @@ -640,6 +636,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: [`${studyBucket}/${studyPrefix}`], }, @@ -707,6 +704,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: ['StudyBucketPath_XYZ'], }, @@ -754,6 +752,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: ['StudyBucketPath_XYZ', `${studyBucket}/${studyPrefix}`], }, @@ -906,6 +905,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: ['AnotherStudyBucketPath', `${studyBucket}/${studyPrefix}`], }, @@ -947,6 +947,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: ['AnotherStudyBucketPath'], }, @@ -1058,6 +1059,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: [`${studyBucket}/${studyPrefix}`], }, @@ -1170,6 +1172,108 @@ describe('EnvironmentMountService', () => { expect(IamUpdateParams.policyDoc).toMatchObject(expectedPolicy); }); + it('should ensure duplicate resources are not created for non-admins after permission addition from a bad state', async () => { + // BUILD + const updateRequest = { + usersToAdd: [{ uid: 'User1-UID', permissionLevel: 'readwrite' }], + }; + 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', + 's3:DeleteObject', + ], + 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); + }); + it('should ensure permission updates for admins are as expected', async () => { // BUILD const updateRequest = { @@ -1214,6 +1318,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: [`${studyBucket}/${studyPrefix}`], }, @@ -1357,6 +1462,7 @@ describe('EnvironmentMountService', () => { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ], Resource: [`${studyBucket}/${studyPrefix}`], }, 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 c2c787b77a..ce993fd505 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 @@ -168,7 +168,13 @@ class EnvironmentMountService extends Service { Sid: putSid, Effect: 'Allow', Principal: { AWS: [] }, - Action: ['s3:AbortMultipartUpload', 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl'], + Action: [ + 's3:AbortMultipartUpload', + 's3:ListMultipartUploadParts', + 's3:PutObject', + 's3:PutObjectAcl', + 's3:DeleteObject', + ], Resource: [`arn:aws:s3:::${s3BucketName}/${prefix}*`], }; @@ -281,7 +287,7 @@ class EnvironmentMountService extends Service { } }; - await runAndCaptureErrors(allowedUsers, users => this.addPermissions(users, studyId, updateRequest)); + await runAndCaptureErrors(allowedUsers, users => this.addPermissions(users, studyId)); await runAndCaptureErrors(disAllowedUsers, users => this.removePermissions(users, studyId, updateRequest)); await runAndCaptureErrors(permissionChangeUsers, users => this.updatePermissions(users, studyId, updateRequest)); @@ -304,16 +310,11 @@ class EnvironmentMountService extends Service { * @param {Object[]} allowedUsers - Users that newly/continue-to have access to given studyId * @param {String} studyId */ - async addPermissions(allowedUsers, studyId, updateRequest) { + async addPermissions(allowedUsers, studyId) { const [iamService, environmentScService] = await this.service(['iamService', 'environmentScService']); 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([...existingStudyAdmins, ...removedStudyAdmins], u => u.uid === user.uid), - ); const userOwnedEnvs = await environmentScService.getActiveEnvsForUser(user.uid); const envsWithStudy = _.filter(userOwnedEnvs, env => _.includes(env.studyIds, studyId)); await Promise.all( @@ -328,11 +329,15 @@ class EnvironmentMountService extends Service { } = await this._getIamUpdateParams(env, studyId); const statementSidToUse = this._getStatementSidToUse(user.permissionLevel); + let ensureRemovedPermission; + if (statementSidToUse === readOnlyStatementId) { + ensureRemovedPermission = readWriteStatementId; + } else if (statementSidToUse === readWriteStatementId) { + ensureRemovedPermission = readOnlyStatementId; + } policyDoc.Statement = this._getStatementsAfterAddition(policyDoc, studyPathArn, statementSidToUse); policyDoc.Statement = this._ensureListAccess(policyDoc, studyPathArn); - if (isStudyAdmin && user.permissionLevel === readWritePermissionLevel) { - policyDoc.Statement = this._getStatementsAfterRemoval(policyDoc, studyPathArn, readOnlyStatementId); - } + policyDoc.Statement = this._getStatementsAfterRemoval(policyDoc, studyPathArn, ensureRemovedPermission); await iamService.putRolePolicy(roleName, studyDataPolicyName, JSON.stringify(policyDoc), iamClient); } catch (error) { const envId = env.id; @@ -541,6 +546,7 @@ class EnvironmentMountService extends Service { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ] : ['s3:GetObject'], Resource: [studyPathArn], @@ -662,6 +668,9 @@ class EnvironmentMountService extends Service { } async _getWorkspacePolicy(iamClient, env) { + if (!env.outputs) { + throw new Error('Environment outputs are not ready yet. Please make sure environment is in Completed status'); + } const iamService = await this.service('iamService'); const workspaceRoleArn = _.find(env.outputs, { OutputKey: 'WorkspaceInstanceRoleArn' }).OutputValue; const roleName = workspaceRoleArn.split('role/')[1]; @@ -837,6 +846,7 @@ class EnvironmentMountService extends Service { 's3:ListMultipartUploadParts', 's3:PutObject', 's3:PutObjectAcl', + 's3:DeleteObject', ]; statements.push({ Sid: readWriteStatementId,