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

Create sections for Secrets sync destination fields for create/edit view #27538

Merged
merged 18 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/27538.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ui: Creates separate section for updating sensitive creds for Secrets sync create/edit view.
```
5 changes: 4 additions & 1 deletion ui/app/models/sync/destinations/aws-sm.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import SyncDestinationModel from '../destination';
import { attr } from '@ember-data/model';
import { withFormFields } from 'vault/decorators/model-form-fields';

// displayFields are used on the destination details view
const displayFields = [
// connection details
'name',
Expand All @@ -20,11 +21,13 @@ const displayFields = [
'secretNameTemplate',
'customTags',
];
// formFieldGroups are used on the create-edit destination view
const formFieldGroups = [
{
default: ['name', 'region', 'roleArn', 'externalId', 'granularity', 'secretNameTemplate', 'customTags'],
default: ['name', 'region', 'roleArn', 'externalId'],
},
{ Credentials: ['accessKeyId', 'secretAccessKey'] },
{ 'Advanced configuration': ['granularity', 'secretNameTemplate', 'customTags'] },
];
@withFormFields(displayFields, formFieldGroups)
export default class SyncDestinationsAwsSecretsManagerModel extends SyncDestinationModel {
Expand Down
15 changes: 4 additions & 11 deletions ui/app/models/sync/destinations/azure-kv.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import SyncDestinationModel from '../destination';
import { attr } from '@ember-data/model';
import { withFormFields } from 'vault/decorators/model-form-fields';

// displayFields are used on the destination details view
const displayFields = [
// connection details
'name',
Expand All @@ -20,20 +20,13 @@ const displayFields = [
'secretNameTemplate',
'customTags',
];
// formFieldGroups are used on the create-edit destination view
const formFieldGroups = [
{
default: [
'name',
'keyVaultUri',
'tenantId',
'cloud',
'clientId',
'granularity',
'secretNameTemplate',
'customTags',
],
default: ['name', 'keyVaultUri', 'tenantId', 'cloud', 'clientId'],
},
{ Credentials: ['clientSecret'] },
{ 'Advanced configuration': ['granularity', 'secretNameTemplate', 'customTags'] },
];
@withFormFields(displayFields, formFieldGroups)
export default class SyncDestinationsAzureKeyVaultModel extends SyncDestinationModel {
Expand Down
6 changes: 4 additions & 2 deletions ui/app/models/sync/destinations/gcp-sm.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import SyncDestinationModel from '../destination';
import { attr } from '@ember-data/model';
import { withFormFields } from 'vault/decorators/model-form-fields';

// displayFields are used on the destination details view
const displayFields = [
// connection details
'name',
Expand All @@ -17,9 +17,11 @@ const displayFields = [
'secretNameTemplate',
'customTags',
];
// formFieldGroups are used on the create-edit destination view
const formFieldGroups = [
{ default: ['name', 'projectId', 'granularity', 'secretNameTemplate', 'customTags'] },
{ default: ['name', 'projectId'] },
{ Credentials: ['credentials'] },
{ 'Advanced configuration': ['granularity', 'secretNameTemplate', 'customTags'] },
];
@withFormFields(displayFields, formFieldGroups)
export default class SyncDestinationsGoogleCloudSecretManagerModel extends SyncDestinationModel {
Expand Down
7 changes: 5 additions & 2 deletions ui/app/models/sync/destinations/gh.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import SyncDestinationModel from '../destination';
import { attr } from '@ember-data/model';
import { withFormFields } from 'vault/decorators/model-form-fields';

// displayFields are used on the destination details view
const displayFields = [
// connection details
'name',
Expand All @@ -17,9 +17,12 @@ const displayFields = [
'granularity',
'secretNameTemplate',
];

// formFieldGroups are used on the create-edit destination view
const formFieldGroups = [
{ default: ['name', 'repositoryOwner', 'repositoryName', 'granularity', 'secretNameTemplate'] },
{ default: ['name', 'repositoryOwner', 'repositoryName'] },
{ Credentials: ['accessToken'] },
{ 'Advanced configuration': ['granularity', 'secretNameTemplate'] },
];

@withFormFields(displayFields, formFieldGroups)
Expand Down
6 changes: 4 additions & 2 deletions ui/app/models/sync/destinations/vercel-project.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const validations = {
// getter/setter for the deploymentEnvironments model attribute
deploymentEnvironmentsArray: [{ type: 'presence', message: 'At least one environment is required.' }],
};

// displayFields are used on the destination details view
const displayFields = [
// connection details
'name',
Expand All @@ -33,9 +33,11 @@ const displayFields = [
'granularity',
'secretNameTemplate',
];
// formFieldGroups are used on the create-edit destination view
const formFieldGroups = [
{ default: ['name', 'projectId', 'teamId', 'deploymentEnvironments', 'granularity', 'secretNameTemplate'] },
{ default: ['name', 'projectId', 'teamId', 'deploymentEnvironments'] },
{ Credentials: ['accessToken'] },
{ 'Advanced configuration': ['granularity', 'secretNameTemplate'] },
];
@withModelValidations(validations)
@withFormFields(displayFields, formFieldGroups)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,34 @@

{{#each @destination.formFieldGroups as |fieldGroup|}}
{{#each-in fieldGroup as |group fields|}}
{{#if (and (eq group "Credentials") (not @destination.isNew))}}
{{#if (not-eq group "default")}}
<hr class="has-top-margin-xl has-bottom-margin-l has-background-gray-200" />
<Hds::Text::Display @tag="h2" @size="400" @weight="bold">Credentials</Hds::Text::Display>
<Hds::Text::Body @tag="p" @size="100" @color="faint" class="has-bottom-margin-m">
Connection credentials are sensitive information and the value cannot be read. Enable the input to update.
<Hds::Text::Display @tag="h2" @size="400" @weight="bold">{{group}}</Hds::Text::Display>
<Hds::Text::Body
@tag="p"
@size="100"
@color="faint"
class="has-bottom-margin-m"
data-test-destination-subText={{group}}
>
{{(this.groupSubtext group @destination.isNew)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{(this.groupSubtext group @destination.isNew)}}
{{this.groupSubtext group @destination.isNew}}

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of passing in isNew here can we just access this.args.destination.isNew in the component helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that and then realized I was hitting issues because this is within an each iteration, so it's easier to access the destination by passing it in like this.

</Hds::Text::Body>
{{#each fields as |attr|}}
{{/if}}
{{! CREATE fields }}
{{#each fields as |attr|}}
{{#if (and (eq group "Credentials") (not @destination.isNew))}}
<EnableInput data-test-enable-field={{attr.name}} class="field" @attr={{attr}}>
<FormField @attr={{attr}} @model={{@destination}} @modelValidations={{this.modelValidations}} />
</EnableInput>
{{/each}}
{{else}}
{{#each fields as |attr|}}
{{else}}
<FormField
@attr={{attr}}
@model={{@destination}}
@modelValidations={{this.modelValidations}}
@onKeyUp={{this.updateWarningValidation}}
/>
{{/each}}
{{/if}}
{{/if}}
{{/each}}
{{/each-in}}
{{/each}}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,20 @@ export default class DestinationsCreateForm extends Component<Args> {
};
}

groupSubtext(group: string, isNew: boolean) {
const dynamicText = isNew
? 'used to authenticate with the destination'
: 'and the value cannot be read. Enable the input to update';
Comment on lines +59 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
groupSubtext(group: string, isNew: boolean) {
const dynamicText = isNew
? 'used to authenticate with the destination'
: 'and the value cannot be read. Enable the input to update';
groupSubtext(group: string) {
const dynamicText = this.args.destination.isNew
? 'used to authenticate with the destination'
: 'and the value cannot be read. Enable the input to update';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went this exact same route, but it fails to read the args. It might be because I'm iterating over via a each and then each-in and then an each again, but passing in via the template solved the problem
image

Copy link
Contributor

Choose a reason for hiding this comment

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

strange! it must be how the template helper functions compile 🤔

switch (group) {
case 'Advanced configuration':
return 'Configuration options for the destination.';
case 'Credentials':
return `Connection credentials are sensitive information ${dynamicText}.`;
default:
return '';
}
}

@task
@waitFor
*save(event: Event) {
Expand Down
1 change: 1 addition & 0 deletions ui/tests/helpers/sync/sync-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const PAGE = {
toolbar: (btnText) => `[data-test-toolbar="${btnText}"]`,
form: {
enableInput: (attr) => `[data-test-enable-field="${attr}"] [data-test-icon="edit"]`,
fieldGroupSubtext: (group) => `[data-test-destination-subText="${group}"]`,
fillInByAttr: async (attr, value) => {
// for handling more complex form input elements by attr name
switch (attr) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,23 +56,48 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE
assert.true(transition, 'transitions to vault.cluster.sync.secrets.destinations.create on cancel');
});

test('create: it renders fieldGroups subtext', async function (assert) {
assert.expect(2);
const { type } = SYNC_DESTINATIONS[0];
this.model = this.store.createRecord(`sync/destinations/${type}`, { type });

await this.renderFormComponent();

assert
.dom(PAGE.form.fieldGroupSubtext('Credentials'))
.hasText('Connection credentials are sensitive information used to authenticate with the destination.');
assert
.dom(PAGE.form.fieldGroupSubtext('Advanced configuration'))
.hasText('Configuration options for the destination.');
});

test('edit: it renders and navigates back to details on cancel', async function (assert) {
assert.expect(4);
assert.expect(3);
this.model = this.generateModel();

await this.renderFormComponent();
assert.dom(PAGE.breadcrumbs).hasText('Secrets Sync Destinations Destination Edit Destination');
assert.dom('h2').hasText('Credentials', 'renders credentials section on edit');
assert
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this into a subtext specific test

.dom('p.hds-foreground-faint')
.hasText(
'Connection credentials are sensitive information and the value cannot be read. Enable the input to update.'
);
await click(PAGE.cancelButton);
const transition = this.transitionStub.calledWith('vault.cluster.sync.secrets.destinations.destination');
assert.true(transition, 'transitions to vault.cluster.sync.secrets.destinations.destination on cancel');
});

test('edit: it renders fieldGroup subtext', async function (assert) {
assert.expect(2);
const { type } = SYNC_DESTINATIONS[0];
this.model = this.store.createRecord(`sync/destinations/${type}`, { type });
Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the test above, this needs to be updated to the following to accurately render the edit view

Suggested change
const { type } = SYNC_DESTINATIONS[0];
this.model = this.store.createRecord(`sync/destinations/${type}`, { type });
const { type } = SYNC_DESTINATIONS[0];
this.model = this.generateModel()

Copy link
Contributor

Choose a reason for hiding this comment

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

Also since these are "it renders" related tests, the assertions below could also just move into the test above. I don't think separate subtext tests are needed since the existing "edit: it renders" and "create: it renders" are so small already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch re: 1st comment.

Re 2nd comment: I broke these up because I wanted to isolate the isNew functionality from the transition and breadcrumbs. However, let me clear up the tests a bit to make that more evident.


await this.renderFormComponent();

assert
.dom(PAGE.form.fieldGroupSubtext('Credentials'))
.hasText('Connection credentials are sensitive information used to authenticate with the destination.');
assert
.dom(PAGE.form.fieldGroupSubtext('Advanced configuration'))
.hasText('Configuration options for the destination.');
});
Monkeychip marked this conversation as resolved.
Show resolved Hide resolved

test('edit: it PATCH updates custom_tags', async function (assert) {
assert.expect(1);
this.model = this.generateModel();
Expand Down Expand Up @@ -236,7 +261,7 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE
const filteredObfuscatedFields = this.model.formFields.filter((field) =>
obfuscatedFields.includes(field.name)
);
assert.expect(filteredObfuscatedFields.length);
assert.expect(filteredObfuscatedFields.length * 2);
await this.renderFormComponent();
// iterate over the form fields and filter for those that are obfuscated
// fill those in and assert that they are masked
Expand All @@ -246,6 +271,9 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE
assert
.dom(PAGE.maskedInput(field.name))
.hasClass('masked-font', `it renders ${field.name} for ${destination} with masked font`);
assert
.dom(PAGE.form.enableInput(field.name))
.doesNotExist(`it does not render enable input for ${field.name}`);
});
});

Expand Down
Loading