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

Mask obfuscated Secret sync create/edit fields #27348

Merged
merged 10 commits into from
Jun 18, 2024
Merged
3 changes: 3 additions & 0 deletions changelog/27348.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ui: Mask obfuscated fields when creating/editing a Secrets sync destination.
```
4 changes: 4 additions & 0 deletions ui/app/models/sync/destinations/aws-sm.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CREATE:
image

EDIT:
image

Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,17 @@ export default class SyncDestinationsAwsSecretsManagerModel extends SyncDestinat
label: 'Access key ID',
subText:
'Access key ID to authenticate against the secrets manager. If empty, Vault will use the AWS_ACCESS_KEY_ID environment variable if configured.',
sensitive: true,
noCopy: true,
})
accessKeyId; // obfuscated, never returned by API

@attr('string', {
label: 'Secret access key',
subText:
'Secret access key to authenticate against the secrets manager. If empty, Vault will use the AWS_SECRET_ACCESS_KEY environment variable if configured.',
sensitive: true,
noCopy: true,
})
secretAccessKey; // obfuscated, never returned by API

Expand Down
2 changes: 2 additions & 0 deletions ui/app/models/sync/destinations/azure-kv.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CREATE:
image

EDIT:
image

Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ export default class SyncDestinationsAzureKeyVaultModel extends SyncDestinationM
@attr('string', {
subText:
'Client secret of an Azure app registration. If empty, Vault will use the AZURE_CLIENT_SECRET environment variable if configured.',
sensitive: true,
noCopy: true,
})
clientSecret; // obfuscated, never returned by API

Expand Down
1 change: 1 addition & 0 deletions ui/app/models/sync/destinations/gcp-sm.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export default class SyncDestinationsGoogleCloudSecretManagerModel extends SyncD
docLink: '/vault/docs/secrets/gcp#authentication',
})
credentials; // obfuscated, never returned by API
// ARG TODO DOUBLE CHECK

@attr('object', {
subText:
Expand Down
2 changes: 2 additions & 0 deletions ui/app/models/sync/destinations/gh.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CREATE:
image

EDIT:
image

Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ export default class SyncDestinationsGithubModel extends SyncDestinationModel {
@attr('string', {
subText:
'Personal access token to authenticate to the GitHub repository. If empty, Vault will use the GITHUB_ACCESS_TOKEN environment variable if configured.',
sensitive: true,
noCopy: true,
})
accessToken; // obfuscated, never returned by API

Expand Down
2 changes: 2 additions & 0 deletions ui/app/models/sync/destinations/vercel-project.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CREATE:
image

EDIT:
image

Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const formFieldGroups = [
export default class SyncDestinationsVercelProjectModel extends SyncDestinationModel {
@attr('string', {
subText: 'Vercel API access token with the permissions to manage environment variables.',
sensitive: true,
noCopy: true,
})
accessToken; // obfuscated, never returned by API

Expand Down
2 changes: 1 addition & 1 deletion ui/lib/core/addon/components/form-field.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@
<MaskedInput
@name={{@attr.name}}
@value={{or (get @model this.valuePath) @attr.options.defaultValue}}
@allowCopy="true"
@allowCopy={{if @attr.options.noCopy false true}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should default to true. not ideal (rather it default to false) but working with an old pattern and trying to keep backporting/scope simple.

@onChange={{this.setAndBroadcast}}
@onKeyUp={{@onKeyUp}}
/>
Expand Down
2 changes: 1 addition & 1 deletion ui/lib/core/addon/components/masked-input.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
aria-label={{or @name "masked input"}}
{{on "change" this.onChange}}
{{on "keyup" (fn this.handleKeyUp @name)}}
data-test-textarea
data-test-textarea={{or @name ""}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need or if the default is an empty string

Suggested change
data-test-textarea={{or @name ""}}
data-test-textarea={{@name}}

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 thought that as well, but without the conditional it broke any test selector that didn't pass in a name. I suspect because it comes in as undefined?

/>
{{/if}}
{{#if @allowCopy}}
Expand Down
2 changes: 1 addition & 1 deletion ui/tests/acceptance/sync/secrets/destination-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ module('Acceptance | sync | destination', function (hooks) {

await visit('vault/sync/secrets/destinations/vercel-project/destination-vercel/edit');
await click(ts.enableField('accessToken'));
await fillIn(ts.inputByAttr('accessToken'), 'foobar');
await fillIn(ts.maskedInput('accessToken'), 'foobar');
await click(ts.saveButton);
await click(ts.toolbar('Edit destination'));
await click(ts.saveButton);
Expand Down
1 change: 1 addition & 0 deletions ui/tests/helpers/general-selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,5 @@ export const GENERAL = {
navLink: (label: string) => `[data-test-sidebar-nav-link="${label}"]`,
cancelButton: '[data-test-cancel]',
saveButton: '[data-test-save]',
maskedInput: (name: string) => `[data-test-textarea="${name}"]`,
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { setupMirage } from 'ember-cli-mirage/test-support';
import hbs from 'htmlbars-inline-precompile';
import sinon from 'sinon';
import { Response } from 'miragejs';
import { click, render, typeIn } from '@ember/test-helpers';
import { click, fillIn, render, typeIn } from '@ember/test-helpers';
import { PAGE } from 'vault/tests/helpers/sync/sync-selectors';
import { syncDestinations } from 'vault/helpers/sync-destinations';
import { decamelize, underscore } from '@ember/string';
Expand Down Expand Up @@ -192,6 +192,7 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE
// CREATE FORM ASSERTIONS FOR EACH DESTINATION TYPE
for (const destination of SYNC_DESTINATIONS) {
const { name, type } = destination;
const obfuscatedFields = ['clientSecret', 'secretAccessKey', 'accessKeyId'];

module(`create destination: ${type}`, function (hooks) {
hooks.beforeEach(function () {
Expand All @@ -209,6 +210,23 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE
}
});

test('it masks obfuscated fields meep', async function (assert) {
const filteredObfuscatedFields = this.model.formFields.filter((field) =>
obfuscatedFields.includes(field.name)
);
assert.expect(filteredObfuscatedFields.length);
await this.renderFormComponent();
// iterate over the form fields and filter for those that are obfuscated
// fill those in and assert that they are masked
filteredObfuscatedFields.forEach(async (field) => {
await fillIn(PAGE.maskedInput(field.name), 'blah');

assert
.dom(PAGE.maskedInput(field.name))
.hasClass('masked-font', `it renders ${field.name} for ${destination} with masked font`);
});
});

test('it saves destination and transitions to details', async function (assert) {
assert.expect(5);
const name = 'my-name';
Expand Down
Loading