Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Various TRE bugfixes #642

Merged
merged 15 commits into from
Aug 11, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,14 @@ const addUpdateBaseAwsAccountFormFields = {
const addUpdateAwsAccountAppStreamFormFields = {
appStreamFleetDesiredInstances: {
label: 'AppStream Fleet Desired Instance',
placeholder: 'Number of users that can concurrently access a workspace through AppStream',
placeholder:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various text enhancements suggested by Jolie

'Maximum number of concurrently running AppStream sessions. Each researcher uses one AppStream session when viewing a workspace',
rules: 'required|integer',
},
appStreamDisconnectTimeoutSeconds: {
label: 'AppStreamDisconnectTimeoutSeconds',
placeholder: 'The amount of time that a streaming session remains active after users disconnect',
rules: 'required|integer',
placeholder: 'The amount of time that a streaming session remains active after users disconnect. (Minimum of 60)',
rules: ['required', 'integer', 'min:60'],
},
appStreamIdleDisconnectTimeoutSeconds: {
label: 'AppStreamIdleDisconnectTimeoutSeconds',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,14 @@ const createBaseAwsAccountFormFields = {
const createAwsAccountAppStreamFormFields = {
appStreamFleetDesiredInstances: {
label: 'AppStream Fleet Desired Instance',
placeholder: 'Number of users that can concurrently access a workspace through AppStream',
placeholder:
'Maximum number of concurrently running AppStream sessions. Each researcher uses one AppStream session when viewing a workspace',
rules: 'required|integer',
},
appStreamDisconnectTimeoutSeconds: {
label: 'AppStreamDisconnectTimeoutSeconds',
placeholder: 'The amount of time that a streaming session remains active after users disconnect',
rules: 'required|integer',
placeholder: 'The amount of time that a streaming session remains active after users disconnect. (Minimum of 60)',
rules: ['required', 'integer', 'min:60'],
},
appStreamIdleDisconnectTimeoutSeconds: {
label: 'AppStreamIdleDisconnectTimeoutSeconds',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,10 @@ class ScEnvironmentHttpConnections extends React.Component {
<List.Item>Click the &quot;Generate URL&quot; button to create the destination URL</List.Item>
<List.Item>Copy the URL and hit &quot;Connect&quot;. </List.Item>
<List.Item>Paste the URL in the new AppStream FireFox tab</List.Item>
<List.Item>
In your browser, please allow popups for this domain so we can open the AppStream page in a new tab for
you
</List.Item>
</List>
</Segment>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class ScEnvironmentRdpConnectionRow extends React.Component {
this.windowsRdpInfo = undefined;
// A flag to indicate if we are in the process of getting the windows rdp info
this.processingGetInfo = false;
// A flag to indicate if we're getting the connection url
this.processingGetConnection = false;
// Should the password be shown
this.showPassword = false;
this.processingId = undefined;
Expand Down Expand Up @@ -83,6 +85,9 @@ class ScEnvironmentRdpConnectionRow extends React.Component {
handleConnect = id =>
action(async () => {
try {
runInAction(() => {
this.processingGetConnection = true;
});
const store = this.getConnectionStore();
const urlObj = await store.createConnectionUrl(id);
const appStreamUrl = urlObj.url;
Expand All @@ -93,6 +98,7 @@ class ScEnvironmentRdpConnectionRow extends React.Component {
throw Error('AppStream URL was not returned by the API');
}
runInAction(() => {
this.processingGetConnection = false;
this.processingId = id;
});
} catch (error) {
Expand Down Expand Up @@ -219,6 +225,12 @@ class ScEnvironmentRdpConnectionRow extends React.Component {
Connect to Your Windows Instance
</List.Item>
</List>
{this.isAppStreamEnabled && (
<div className="mt3">
In your browser, please allow popups for this domain so we can open the AppStream page in a new tab for
you
</div>
)}
</Table.Cell>
</Table.Row>

Expand All @@ -231,7 +243,7 @@ class ScEnvironmentRdpConnectionRow extends React.Component {
onClick={this.handleConnect(connectionId)}
floated="right"
disabled={this.processingId}
loading={this.processingId}
loading={this.processingGetConnection}
>
Connect
</Button>
Expand Down Expand Up @@ -284,6 +296,7 @@ decorate(ScEnvironmentRdpConnectionRow, {
processingId: observable,
windowsRdpInfo: observable,
processingGetInfo: observable,
processingGetConnection: observable,
showPassword: observable,
handleGetInfo: action,
toggleShowPassword: action,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ class ScEnvironmentSshConnections extends React.Component {
Connecting from Windows via Putty
</List.Item>
</List>
<div className="mt3">
In your browser, please allow popups for this domain so we can open the AppStream page in a new tab for you
</div>
</Segment>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ describe('EnvironmentSCService', () => {
adminKeyPairName: '',
cidr: '192.168.xx.yy',
description: 'env-desc',
egressStoreIamPolicyDocument: '{}',
encryptionKeyArn: 'UltraSecureEncryptionKey',
envId: 'sampleEnvId',
envTypeConfigId: 'sampleEnvTypeConfigId',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,7 @@ class EnvironmentConfigVarsService extends Service {
adminKeyPairName,
};

if (enableEgressStore && enableEgressStore.toUpperCase() === 'TRUE') {
result.egressStoreIamPolicyDocument = JSON.stringify(egressStoreIamPolicyDocument);
}
result.egressStoreIamPolicyDocument = JSON.stringify(egressStoreIamPolicyDocument);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our workspace cfn template expect a value for this attribute, and will error out if we don't provide it. If the attribute is {} then EgressPolicy is not added. Incase where Egress is turned off, the egressStoreIamPolicyDocument will be {}


return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@
"rev",
"inWorkflow",
"status",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Jeet's change of removing CIDR from AppStream enabled accounts, we don't always pass in cidr. Therefore we need to remove it from here, or it will cause a validation error.

Copy link
Contributor

@jn1119 jn1119 Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can cidr be completely removed? I wonder why egress store needs it.

"cidr",
"createdAt",
"updatedBy",
"createdBy",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ describe('EnvTypeService', () => {
service = await container.find('envTypeConfigService');
envTypeService = await container.find('envTypeService');
s3Service = await container.find('s3Service');
const settingsService = await container.find('settings');

// skip authorization
service.assertAuthorized = jest.fn();
Expand All @@ -80,6 +81,13 @@ describe('EnvTypeService', () => {
putObject: jest.fn().mockReturnThis(),
promise: jest.fn().mockReturnThis(),
};

settingsService.getBoolean = jest.fn(settingKey => {
if (settingKey === 'isAppStreamEnabled') {
return true;
}
throw Error(`${settingKey} not found`);
});
});

describe('list function', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
* permissions and limitations under the License.
*/

/* eslint-disable no-template-curly-in-string */
const _ = require('lodash');
const Service = require('@aws-ee/base-services-container/lib/service');
const { isAllow, allowIfActive, allowIfAdmin } = require('@aws-ee/base-services/lib/authorization/authorization-utils');
Expand All @@ -24,6 +25,7 @@ const updateEnvTypeConfigSchema = createEnvTypeConfigSchema;
const settingKeys = {
envTypeConfigsBucketName: 'envTypeConfigsBucketName',
envTypeConfigsPrefix: 'envTypeConfigsPrefix',
isAppStreamEnabled: 'isAppStreamEnabled',
};

/**
Expand Down Expand Up @@ -136,14 +138,14 @@ class EnvTypeConfigService extends Service {

// Make sure the specified configuration has params mapping specified for
// all non-default CFN input params for the given env type
await this.assertNoMissingParams(requestContext, envType, config);
const updatedConfig = await this.checkAndUpdateParams(envType, config);

// Make sure the config with the same id for the same env type does not exist already
const existingConfigs = await this.getConfigsFromS3(envTypeId);
const existingConfig = _.find(existingConfigs, { id: config.id });
const existingConfig = _.find(existingConfigs, { id: updatedConfig.id });
if (existingConfig) {
throw this.boom.badRequest(
`The environment type configuration with id "${config.id}" for environment type "${envTypeId}" already exists`,
`The environment type configuration with id "${updatedConfig.id}" for environment type "${envTypeId}" already exists`,
true,
);
}
Expand All @@ -152,7 +154,7 @@ class EnvTypeConfigService extends Service {
const by = _.get(requestContext, 'principalIdentifier.uid');
const { bucket, key } = await this.getS3Coordinates(envType.id);
const now = new Date().toISOString();
const configToSave = this.fromRawToS3Object(config, {
const configToSave = this.fromRawToS3Object(updatedConfig, {
createdBy: by,
updatedBy: by,
createdAt: now,
Expand Down Expand Up @@ -208,11 +210,9 @@ class EnvTypeConfigService extends Service {
}

// Merge given config with the existing config before updating
const configToUpdate = { ...existingConfig, ...config };
let configToUpdate = { ...existingConfig, ...config };

// Make sure the specified configuration has params mapping specified for
// all non-default CFN input params for the given env type
await this.assertNoMissingParams(requestContext, envType, configToUpdate);
configToUpdate = await this.checkAndUpdateParams(envType, configToUpdate);

// Everything is good so far, time to save the given configuration to S3 now
const by = _.get(requestContext, 'principalIdentifier.uid');
Expand Down Expand Up @@ -241,6 +241,39 @@ class EnvTypeConfigService extends Service {
return savedConfig;
}

async checkAndUpdateParams(envType, config) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting default values for the env params, depending on if AppStream is turned on or not.

const isAppStreamEnabled = this.settings.getBoolean(settingKeys.isAppStreamEnabled);
const updatedConfig = { ...config };
updatedConfig.params.push({
key: 'IsAppStreamEnabled',
value: isAppStreamEnabled.toString(),
});
let params = [...updatedConfig.params];
if (isAppStreamEnabled) {
params = [
...params,
{
key: 'AccessFromCIDRBlock',
value: '',
},
// Let's automatically fill in these values for the customer
{ key: 'EgressStoreIamPolicyDocument', value: '${egressStoreIamPolicyDocument}' },
{ key: 'SolutionNamespace', value: '${solutionNamespace}' },
];
} else {
params = [
...params,
{ key: 'EgressStoreIamPolicyDocument', value: '{}' },
{ key: 'SolutionNamespace', value: '' },
];
}
updatedConfig.params = params;
// Make sure the specified configuration has params mapping specified for
// all non-default CFN input params for the given env type
await this.assertNoMissingParams(envType, updatedConfig);
return updatedConfig;
}

async delete(requestContext, envTypeId, configId) {
// ensure that the caller has permissions to delete env type config
// Perform default condition checks to make sure the user is active and is admin
Expand Down Expand Up @@ -338,7 +371,7 @@ class EnvTypeConfigService extends Service {
}
}

async assertNoMissingParams(requestContext, envType, config) {
async assertNoMissingParams(envType, config) {
// Make sure the specified configuration is complete and provides mapping
// for all non-default CFN input parameters
const cfnInputParams = envType.params || [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,25 @@ import { createForm } from '@aws-ee/base-ui/dist/helpers/form';
*/
function getCfnParamsForm(cfnParams, existingParamValues) {
const isAppStreamEnabled = process.env.REACT_APP_IS_APP_STREAM_ENABLED === 'true';
const keysToFilterOut = ['IsAppStreamEnabled', 'EgressStoreIamPolicyDocument', 'SolutionNamespace'];
if (isAppStreamEnabled) {
keysToFilterOut.push('AccessFromCIDRBlock');
}
const filteredCfnParams = cfnParams.filter(cfnParam => {
const { ParameterKey } = cfnParam;
// Include keys that are not in keysToFilterOut array
return !keysToFilterOut.includes(ParameterKey);
});

const fields = {};
_.forEach(cfnParams, ({ ParameterKey, Description, DefaultValue }) => {
filteredCfnParams.forEach(({ ParameterKey, Description, DefaultValue }) => {
const existingValue = _.get(_.find(existingParamValues, { key: ParameterKey }), 'value') || DefaultValue;
if (!isAppStreamEnabled || (isAppStreamEnabled && !(ParameterKey === 'AccessFromCIDRBlock'))) {
fields[ParameterKey] = {
label: ParameterKey,
extra: { explain: Description },
value: existingValue,
rules: 'required',
};
}
fields[ParameterKey] = {
label: ParameterKey,
extra: { explain: Description },
value: existingValue,
rules: 'required',
};
});
return createForm(fields);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,49 @@ describe('CfnParamsForm', () => {
expect(formMock.createForm).toHaveBeenCalledTimes(1);
expect(formMock.createForm).toHaveBeenCalledWith(fieldsWithCidr);
});

describe('should not return these fields if present in the CFN template: IsAppStreamEnabled, EgressStoreIamPolicyDocument, SolutionNamespace', () => {
const isAppStreamEnabledParam = {
ParameterKey: 'IsAppStreamEnabled',
Description: 'Enable AppStream',
DefaultValue: 'false',
};

const egressStoreIamPolicyDocumentParam = {
ParameterKey: 'EgressStoreIamPolicyDocument',
Description: 'Policy for egress store',
};

const solutionNamespaceParam = {
ParameterKey: 'SolutionNamespace',
Description: 'Namespace provided when onboarding your account',
};
it('AppStream Enabled', () => {
process.env.REACT_APP_IS_APP_STREAM_ENABLED = true;
runTest();
});

it('AppStream Disabled', () => {
process.env.REACT_APP_IS_APP_STREAM_ENABLED = false;
runTest();
});

function runTest() {
// BUILD
const params = [
...defaultParams,
isAppStreamEnabledParam,
egressStoreIamPolicyDocumentParam,
solutionNamespaceParam,
];
const fields = JSON.parse(JSON.stringify(defaultFields));
// OPERATE
const returnedForm = getCfnParamsForm(params, []);

// CHECK
expect(returnedForm).toBe(expectedForm);
expect(formMock.createForm).toHaveBeenCalledTimes(1);
expect(formMock.createForm).toHaveBeenCalledWith(fields);
}
});
});
5 changes: 4 additions & 1 deletion main/config/settings/example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,7 @@
# If you toggle the egress store from 'true' to 'false' and redeploy the whole solution, the backend stack deployment shold error out with following message:
# Error validating existing stack policy: Unknown logical id 'LogicalResourceId/EgressStore*' in statement {} - stack policies can only be applied to logical ids referenced in the template
# Stack policies are applied here: addons/addon-stack-policy/packages/stack-policy/lib/steps/update-cfn-stack-policy.js
# enableEgressStore: 'false'
#enableEgressStore: 'false'

# Determine whether workspace should be accessible only through AppStream
#isAppStreamEnabled: false
4 changes: 3 additions & 1 deletion scripts/app-stream/SETUP.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@ Note: Please set up your [AWS Profile](https://docs.aws.amazon.com/cli/latest/us
2. This will open a new tab in your browser. When the prompt ask for which user you would like to log in as, choose Administrator. This will take you to a Windows Desktop that you can interact with to create your AppStream image.
3. On the Windows desktop, click the `Start` button and type in `Windows Powershell`. Right click the application in the search result, and choose `Run as administrator`
4. Run the following commands

TODO: Change `feat-secure-workspace-egress` to `mainline` for the url when merging feat branch back to mainline
```
cd ~\Documents

# Pull the Image Builder script from Github
Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/mainline/scripts/app-stream/buildImage.ps1 -OutFile buildImage.ps1
Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/feat-secure-workspace-egress/scripts/app-stream/buildImage.ps1 -OutFile buildImage.ps1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sure we add a task to change it back to mainline when releasing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me add todos for these


# Execute Image builder script
.\buildImage.ps1
Expand Down
3 changes: 2 additions & 1 deletion scripts/app-stream/buildImage.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ Set-ExecutionPolicy Bypass -Scope Process -Force; [System.Net.ServicePointManage

choco install -y putty.install

Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/mainline/scripts/app-stream/ec2linux.ps1 -OutFile 'C:\Users\Public\Documents\EC2Linux.ps1'
# TODO: Change `feat-secure-workspace-egress` to `mainline` for the url when merging feat branch back to mainline
Invoke-WebRequest -Uri https://raw.githubusercontent.com/awslabs/service-workbench-on-aws/feat-secure-workspace-egress/scripts/app-stream/ec2linux.ps1 -OutFile 'C:\Users\Public\Documents\EC2Linux.ps1'

# Prepare remote desktop script with arguments
Set-Content C:\Users\public\Documents\MicrosoftRemoteDesktop.ps1 "mstsc /f /v:`"`$env:APPSTREAM_SESSION_CONTEXT`":3389"
Expand Down