From 9b369167d080bad1035f5db5b96b618aa3af5fda Mon Sep 17 00:00:00 2001 From: Sanket Dharwadkar Date: Thu, 12 Nov 2020 19:22:44 -0500 Subject: [PATCH] Study Admin permission handling (#205) * feat: handle study admin permissions * feat: added study admin unit tests * feat: changes per code review * fix: test fixes * fix: remove duplicate code --- .../environment-mount-service.test.js | 483 +++++++++++++++++- .../environment/environment-mount-service.js | 57 ++- 2 files changed, 526 insertions(+), 14 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 043acca3c7..51ec0538ca 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,7 +107,11 @@ describe('EnvironmentMountService', () => { await service.applyWorkspacePermissions(studyId, updateRequest); // CHECK - expect(service.addPermissions).toHaveBeenCalledWith([{ uid: 'User1-UID', permissionLevel: 'readonly' }], studyId); + expect(service.addPermissions).toHaveBeenCalledWith( + [{ uid: 'User1-UID', permissionLevel: 'readonly' }], + studyId, + updateRequest, + ); expect(service.removePermissions).not.toHaveBeenCalled(); expect(service.updatePermissions).not.toHaveBeenCalled(); }); @@ -131,6 +135,7 @@ describe('EnvironmentMountService', () => { expect(service.removePermissions).toHaveBeenCalledWith( [{ uid: 'User2-UID', permissionLevel: 'readwrite' }], studyId, + updateRequest, ); expect(service.updatePermissions).not.toHaveBeenCalled(); }); @@ -751,4 +756,480 @@ describe('EnvironmentMountService', () => { expect(IamUpdateParams.policyDoc).toMatchObject(expectedPolicy); }); }); + + it('should ensure study admins have at least R/O access after R/O permission removal', async () => { + // BUILD + const updateRequest = { + usersToRemove: [{ uid: 'User1-UID', permissionLevel: 'readonly' }], + usersToAdd: [{ 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', `${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 study admins have at least R/O access after R/W permission removal', async () => { + // BUILD + const updateRequest = { + usersToRemove: [{ uid: 'User1-UID', permissionLevel: 'readwrite' }], + usersToAdd: [{ 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: 'S3StudyReadWriteAccess', + Effect: 'Allow', + Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + 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: 'S3StudyReadWriteAccess', + Effect: 'Allow', + Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Resource: ['AnotherStudyBucketPath'], + }, + { + Sid: 'S3StudyReadAccess', + Effect: 'Allow', + Action: ['s3:GetObject'], + 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 study admins have only one access level after R/W permission addition', async () => { + // BUILD + + const updateRequest = { + usersToAdd: [ + { uid: 'User1-UID', permissionLevel: 'admin' }, + { 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: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); + }); + + it('should ensure duplicate resources are not created for admins after R/O permission addition', async () => { + // BUILD + const updateRequest = { + usersToAdd: [ + { uid: 'User1-UID', permissionLevel: 'admin' }, + { uid: 'User1-UID', permissionLevel: 'readonly' }, + ], + }; + 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', `${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 = { + usersToAdd: [ + { uid: 'User1-UID', permissionLevel: 'admin' }, + { uid: 'User1-UID', permissionLevel: 'readonly' }, + ], + usersToRemove: [{ 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: 'S3StudyReadWriteAccess', + Effect: 'Allow', + Action: ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'], + Resource: [`${studyBucket}/${studyPrefix}`], + }, + { + Sid: 'S3StudyReadAccess', + Effect: 'Allow', + Action: ['s3:GetObject'], + Resource: ['AnotherStudyBucketPath'], + }, + ], + }; + 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', `${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 5b4491511d..5807a5e5cf 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 @@ -25,6 +25,11 @@ const settingKeys = { studyDataKmsPolicyWorkspaceSid: 'studyDataKmsPolicyWorkspaceSid', }; +const readOnlyPermissionLevel = 'readonly'; +const readWritePermissionLevel = 'readwrite'; +const readWriteStatementId = 'S3StudyReadWriteAccess'; +const readOnlyStatementId = 'S3StudyReadAccess'; + const parseS3Arn = arn => { const path = arn.slice('arn:aws:s3:::'.length); const slashIndex = path.indexOf('/'); @@ -276,8 +281,8 @@ class EnvironmentMountService extends Service { } }; - await runAndCaptureErrors(allowedUsers, users => this.addPermissions(users, studyId)); - await runAndCaptureErrors(disAllowedUsers, users => this.removePermissions(users, studyId)); + await runAndCaptureErrors(allowedUsers, users => this.addPermissions(users, studyId, updateRequest)); + await runAndCaptureErrors(disAllowedUsers, users => this.removePermissions(users, studyId, updateRequest)); await runAndCaptureErrors(permissionChangeUsers, users => this.updatePermissions(users, studyId, updateRequest)); if (!_.isEmpty(errors)) { @@ -299,11 +304,14 @@ class EnvironmentMountService extends Service { * @param {Object[]} allowedUsers - Users that newly/continue-to have access to given studyId * @param {String} studyId */ - async addPermissions(allowedUsers, studyId) { + async addPermissions(allowedUsers, studyId, updateRequest) { const [iamService, environmentScService] = await this.service(['iamService', 'environmentScService']); const errors = []; await Promise.all( _.map(allowedUsers, async user => { + const isStudyAdmin = !_.isEmpty( + _.filter(updateRequest.usersToAdd, u => u.permissionLevel === 'admin' && u.uid === user.uid), + ); const userOwnedEnvs = await environmentScService.getActiveEnvsForUser(user.uid); const envsWithStudy = _.filter(userOwnedEnvs, env => _.includes(env.studyIds, studyId)); await Promise.all( @@ -320,6 +328,9 @@ class EnvironmentMountService extends Service { const statementSidToUse = this._getStatementSidToUse(user.permissionLevel); 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); + } await iamService.putRolePolicy(roleName, studyDataPolicyName, JSON.stringify(policyDoc), iamClient); } catch (error) { const envId = env.id; @@ -342,11 +353,14 @@ class EnvironmentMountService extends Service { * @param {Object[]} disAllowedUsers - Users that lost access to given studyId * @param {String} studyId */ - async removePermissions(disAllowedUsers, studyId) { + async removePermissions(disAllowedUsers, studyId, updateRequest) { const [iamService, environmentScService] = await this.service(['iamService', 'environmentScService']); const errors = []; await Promise.all( _.map(disAllowedUsers, async user => { + const isStudyAdmin = !_.isEmpty( + _.filter(updateRequest.usersToAdd, u => u.permissionLevel === 'admin' && u.uid === user.uid), + ); const userOwnedEnvs = await environmentScService.getActiveEnvsForUser(user.uid); const envsWithStudy = _.filter(userOwnedEnvs, env => _.includes(env.studyIds, studyId)); await Promise.all( @@ -361,8 +375,12 @@ class EnvironmentMountService extends Service { } = await this._getIamUpdateParams(env, studyId); const statementSidToUse = this._getStatementSidToUse(user.permissionLevel); + // Study admin should always have R/O access (for backwards compatibility) policyDoc.Statement = this._getStatementsAfterRemoval(policyDoc, studyPathArn, statementSidToUse); policyDoc.Statement = this._removeListAccess(policyDoc, studyPathArn); + if (isStudyAdmin) { + policyDoc.Statement = this._ensureReadAccessForAdmin(policyDoc, studyPathArn); + } await iamService.putRolePolicy(roleName, studyDataPolicyName, JSON.stringify(policyDoc), iamClient); } catch (error) { const envId = env.id; @@ -433,6 +451,19 @@ class EnvironmentMountService extends Service { } } + /** + * Function that returns updated policy document after making sure study admin at least continues to have R/O access + * + * @param {Object} policyDoc - S3 studydata policy document for workspace role + * @param {String} studyPathArn + * @returns {Object[]} - the statement to update in the policy + */ + _ensureReadAccessForAdmin(policyDoc, studyPathArn) { + policyDoc.Statement = this._ensureListAccess(policyDoc, studyPathArn); + policyDoc.Statement = this._getStatementsAfterAddition(policyDoc, studyPathArn, readOnlyStatementId); + return policyDoc.Statement; + } + /** * Function that returns updated policy document with additions according to recent user-study permission change * @@ -501,7 +532,7 @@ class EnvironmentMountService extends Service { Sid: statementSidToUse, Effect: 'Allow', Action: - statementSidToUse === 'S3StudyReadWriteAccess' + statementSidToUse === readWriteStatementId ? ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl'] : ['s3:GetObject'], Resource: [studyPathArn], @@ -651,10 +682,10 @@ class EnvironmentMountService extends Service { _getStatementSidToUse(permissionLevel) { let statementSidToUse = ''; - if (permissionLevel === 'readwrite') { - statementSidToUse = 'S3StudyReadWriteAccess'; - } else if (permissionLevel === 'readonly') { - statementSidToUse = 'S3StudyReadAccess'; + if (permissionLevel === readWritePermissionLevel) { + statementSidToUse = readWriteStatementId; + } else if (permissionLevel === readOnlyPermissionLevel) { + statementSidToUse = readOnlyStatementId; } if (statementSidToUse === '') { throw new Error( @@ -791,20 +822,20 @@ class EnvironmentMountService extends Service { const writeableStudies = _.filter(studyInfo, study => study.writeable); const readonlyStudies = _.filter(studyInfo, study => !study.writeable); - if (writeableStudies.length) { + if (writeableStudies.length && writeableStudies.length > 0) { const objectLevelWriteActions = ['s3:GetObject', 's3:PutObject', 's3:PutObjectAcl']; statements.push({ - Sid: 'S3StudyReadWriteAccess', + Sid: readWriteStatementId, Effect: 'Allow', Action: objectLevelWriteActions, Resource: this._getObjectPathArns(writeableStudies), }); } - if (readonlyStudies.length) { + if (readonlyStudies.length && readonlyStudies.length > 0) { const objectLevelReadActions = ['s3:GetObject']; statements.push({ - Sid: 'S3StudyReadAccess', + Sid: readOnlyStatementId, Effect: 'Allow', Action: objectLevelReadActions, Resource: this._getObjectPathArns(readonlyStudies),