Skip to content
This repository has been archived by the owner on Dec 6, 2024. It is now read-only.

Commit

Permalink
fix: UI fixes and permissions for bucket policy (#208)
Browse files Browse the repository at this point in the history 
* fix: UI fixes and permissions for bucket policy

* feat: adding unit test
  • Loading branch information
@SanketD92
SanketD92 authored Nov 13, 2020
1 parent 9b36916 commit 423b5b2
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 21 deletions.
158 changes: 151 additions & 7 deletions ...s/packages/base-raas-services/lib/environment/__tests__/environment-mount-service.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`],
},
],
Expand Down Expand Up @@ -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'],
},
],
Expand Down Expand Up @@ -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}`],
},
],
Expand Down Expand Up @@ -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}`],
},
],
Expand Down Expand Up @@ -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'],
},
{
Expand Down Expand Up @@ -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}`],
},
],
Expand Down Expand Up @@ -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}`],
},
{
Expand Down Expand Up @@ -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);
});
});
32 changes: 23 additions & 9 deletions .../addon-base-raas/packages/base-raas-services/lib/environment/environment-mount-service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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],
};
Expand Down Expand Up @@ -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 };
}),
),
[],
Expand Down Expand Up @@ -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',
Expand Down
13 changes: 8 additions & 5 deletions addons/addon-base-raas/packages/base-raas-services/lib/study/study-service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 => ({
Expand Down

0 comments on commit 423b5b2

Please sign in to comment.