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

Backport UI: KV version 2 advanced secret updates to 1.15.x (#25235) #25238

Merged
merged 10 commits into from
Feb 14, 2024
3 changes: 3 additions & 0 deletions changelog/25235.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Do not disable JSON display toggle for KV version 2 secrets
```
3 changes: 3 additions & 0 deletions changelog/25269.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
ui: Fix copy button not working on masked input when value is not a string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#25269 was reliant on changes made in #25235, so I just lumped them in together here so I wouldn't forget to open the backport for this after the JSON update backport was merged

```
15 changes: 14 additions & 1 deletion ui/lib/core/addon/components/json-editor.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@
<Icon @name="reload" />
</button>
{{/if}}
{{#if (and @allowObscure @readOnly)}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From precursor work

{{! For safety we only use obscured values in readonly mode }}
<div>
<Toggle
@name="revealValues"
@checked={{this.revealValues}}
@size="small"
Copy link
Contributor

Choose a reason for hiding this comment

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

The original doesn't have @size="small" - is this discrepancy intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in 1.15 Toggle does not default to small size so we have to pass it

@onChange={{fn (mut this.revealValues)}}
>
<span class="has-text-grey">Reveal values</span>
</Toggle>
</div>
{{/if}}
<div class="toolbar-separator"></div>
<CopyButton
class="button is-transparent"
Expand All @@ -42,7 +55,7 @@
{{/if}}
<div
{{code-mirror
content=(or @value @example)
content=(if this.showObfuscatedData this.obfuscatedData (or @value @example))
extraKeys=@extraKeys
gutters=@gutters
lineNumbers=(if @readOnly false true)
Expand Down
11 changes: 11 additions & 0 deletions ui/lib/core/addon/components/json-editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@

import Component from '@glimmer/component';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';
import { stringify } from 'core/helpers/stringify';
import { obfuscateData } from 'core/utils/advanced-secret';

/**
* @module JsonEditor
Expand All @@ -30,10 +33,18 @@ import { action } from '@ember/object';
*/

export default class JsonEditorComponent extends Component {
@tracked revealValues = false;
get getShowToolbar() {
return this.args.showToolbar === false ? false : true;
}

get showObfuscatedData() {
return this.args.readOnly && this.args.allowObscure && !this.revealValues;
}
get obfuscatedData() {
return stringify([obfuscateData(JSON.parse(this.args.value))], {});
}

@action
onSetup(editor) {
// store reference to codemirror editor so that it can be passed to valueUpdated when restoring example
Expand Down
12 changes: 10 additions & 2 deletions ui/lib/core/addon/components/kv-object-editor.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
@value={{row.name}}
placeholder={{this.placeholders.key}}
{{on "change" (fn this.updateRow row index)}}
{{on "input" (fn this.validateKey index)}}
class="input"
/>
</div>
Expand Down Expand Up @@ -71,7 +70,7 @@
{{/if}}
</div>
</div>
{{#if (includes index this.whitespaceWarningRows)}}
{{#if (this.showWhitespaceWarning row.name)}}
<div class="has-bottom-margin-s">
<AlertInline
@type="warning"
Expand All @@ -80,6 +79,15 @@
/>
</div>
{{/if}}
{{#if (this.showNonStringWarning row.value)}}
<div class="has-bottom-margin-s">
<AlertInline
@type="warning"
@message="This value will be saved as a string. If you need to save a non-string value, please use the JSON editor."
data-test-kv-object-warning={{index}}
/>
</div>
{{/if}}
{{/each}}
{{#if this.hasDuplicateKeys}}
<Hds::Alert data-test-duplicate-keys-warning @type="inline" @color="warning" as |A|>
Expand Down
30 changes: 15 additions & 15 deletions ui/lib/core/addon/components/kv-object-editor.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { isNone } from '@ember/utils';
import { assert } from '@ember/debug';
import { action } from '@ember/object';
import { guidFor } from '@ember/object/internals';
import { A } from '@ember/array';
import KVObject from 'vault/lib/kv-object';

/**
Expand All @@ -26,7 +25,7 @@ import KVObject from 'vault/lib/kv-object';
* ```
* @param {string} value - the value is captured from the model.
* @param {function} onChange - function that captures the value on change
* @param {boolean} [isMasked = false] - when true the <MaskedInput> renders instead of the default <textarea> to input the value portion of the key/value object
* @param {boolean} [isMasked = false] - when true the <MaskedInput> renders instead of the default <textarea> to input the value portion of the key/value object
* @param {function} [onKeyUp] - function passed in that handles the dom keyup event. Used for validation on the kv custom metadata.
* @param {string} [label] - label displayed over key value inputs
* @param {string} [labelClass] - override default label class in FormFieldLabel component
Expand All @@ -35,11 +34,12 @@ import KVObject from 'vault/lib/kv-object';
* @param {string} [subText] - placed under label.
* @param {string} [keyPlaceholder] - placeholder for key input
* @param {string} [valuePlaceholder] - placeholder for value input
* @param {boolean} [allowWhiteSpace = false] - when true, allows whitespace in the key input
* @param {boolean} [warnNonStringValues = false] - when true, shows a warning if the value is a non-string
*/

export default class KvObjectEditor extends Component {
@tracked kvData;
whitespaceWarningRows = A();

get placeholders() {
return {
Expand Down Expand Up @@ -75,7 +75,6 @@ export default class KvObjectEditor extends Component {
const oldObj = this.kvData.objectAt(index);
assert('object guids match', guidFor(oldObj) === guidFor(object));
this.kvData.removeAt(index);
this.whitespaceWarningRows.removeObject(index);
this.args.onChange(this.kvData.toJSON());
}
@action
Expand All @@ -84,16 +83,17 @@ export default class KvObjectEditor extends Component {
this.args.onKeyUp(event.target.value);
}
}
@action
validateKey(rowIndex, event) {
const { value } = event.target;
const keyHasWhitespace = new RegExp('\\s', 'g').test(value);
const rows = [...this.whitespaceWarningRows];
const rowHasWarning = rows.includes(rowIndex);
if (!keyHasWhitespace && rowHasWarning) {
this.whitespaceWarningRows.removeObject(rowIndex);
} else if (keyHasWhitespace && !rowHasWarning) {
this.whitespaceWarningRows.addObject(rowIndex);
showWhitespaceWarning = (name) => {
if (this.args.allowWhiteSpace) return false;
return new RegExp('\\s', 'g').test(name);
};
showNonStringWarning = (value) => {
if (!this.args.warnNonStringValues) return false;
try {
JSON.parse(value);
return true;
} catch (e) {
return false;
}
}
};
}
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 @@ -36,7 +36,7 @@
{{/if}}
{{#if @allowCopy}}
<CopyButton
@clipboardText={{@value}}
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I don't think this needs to be updated for CopyButton just Hds::CopyButton which was implemented in 1.16

It doesn't hurt to format 🤔 - mostly an FYI!

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 checked before I made this backport that it was still failing for objects. Otherwise I'd love to not need this backport 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@clipboardText={{this.copyValue}}
@success={{action (set-flash-message "Data copied!")}}
@error={{action (set-flash-message "Error copying data" "danger")}}
class="copy-button button {{if @displayOnly 'is-compact'}}"
Expand Down
8 changes: 8 additions & 0 deletions ui/lib/core/addon/components/masked-input.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,12 @@ export default class MaskedInputComponent extends Component {
@action toggleStringifyDownload(event) {
this.stringifyDownload = event.target.checked;
}

get copyValue() {
// Value must be a string to be copied
const { value } = this.args;
if (!value || typeof value === 'string') return value;
if (typeof value === 'object') return JSON.stringify(value);
return value.toString();
}
}
26 changes: 23 additions & 3 deletions ui/lib/core/addon/utils/advanced-secret.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,30 @@
*/
export function isAdvancedSecret(value) {
try {
const json = JSON.parse(value);
if (Array.isArray(json)) return false;
return Object.values(json).some((value) => typeof value !== 'string');
const obj = typeof value === 'string' ? JSON.parse(value) : value;
if (Array.isArray(obj)) return false;
return Object.values(obj).any((value) => typeof value !== 'string');
} catch (e) {
return false;
}
}

/**
* Method to obfuscate all values in a map, including nested values and arrays
* @param obj object
* @returns object
*/
export function obfuscateData(obj) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From precursor work

if (typeof obj !== 'object' || Array.isArray(obj)) return obj;
const newObj = {};
for (const key of Object.keys(obj)) {
if (Array.isArray(obj[key])) {
newObj[key] = obj[key].map(() => '********');
} else if (typeof obj[key] === 'object') {
newObj[key] = obfuscateData(obj[key]);
} else {
newObj[key] = '********';
}
}
return newObj;
}
2 changes: 2 additions & 0 deletions ui/lib/kv/addon/components/kv-data-fields.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<JsonEditor
@title="{{if (eq @type 'create') 'Secret' 'Version'}} data"
@value={{this.stringifiedSecretData}}
@allowObscure={{true}}
@valueUpdated={{this.handleJson}}
@readOnly={{eq @type "details"}}
/>
Expand Down Expand Up @@ -43,5 +44,6 @@
@value={{@secret.secretData}}
@onChange={{fn (mut @secret.secretData)}}
@isMasked={{true}}
@warnNonStringValues={{true}}
/>
{{/if}}
5 changes: 2 additions & 3 deletions ui/lib/kv/addon/components/page/secret/details.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,8 @@
@name="json"
@status="success"
@size="small"
@checked={{or this.showJsonView this.secretDataIsAdvanced}}
@checked={{this.showJsonView}}
@onChange={{fn (mut this.showJsonView)}}
@disabled={{this.secretDataIsAdvanced}}
>
<span class="has-text-grey">JSON</span>
</Toggle>
Expand Down Expand Up @@ -101,7 +100,7 @@
</EmptyState>
{{else}}
<KvDataFields
@showJson={{or this.showJsonView this.secretDataIsAdvanced}}
@showJson={{this.showJsonView}}
@secret={{@secret}}
@modelValidations={{this.modelValidations}}
@type="details"
Expand Down
6 changes: 4 additions & 2 deletions ui/lib/kv/addon/components/page/secret/details.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,14 @@ export default class KvSecretDetails extends Component {

@tracked showJsonView = false;
@tracked wrappedData = null;
secretDataIsAdvanced;

constructor() {
super(...arguments);
this.originalSecret = JSON.stringify(this.args.secret.secretData || {});
this.secretDataIsAdvanced = isAdvancedSecret(this.originalSecret);
if (isAdvancedSecret(this.originalSecret)) {
// Default to JSON view if advanced
this.showJsonView = true;
}
}

@action
Expand Down
6 changes: 2 additions & 4 deletions ui/lib/kv/addon/components/page/secret/edit.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@
@name="json"
@status="success"
@size="small"
@checked={{or this.showJsonView this.secretDataIsAdvanced}}
@checked={{this.showJsonView}}
@onChange={{fn (mut this.showJsonView)}}
@disabled={{this.secretDataIsAdvanced}}
>
<span class="has-text-grey">JSON</span>
</Toggle>
Expand Down Expand Up @@ -45,7 +44,7 @@
<MessageError @model={{@secret}} @errorMessage={{this.errorMessage}} />

<KvDataFields
@showJson={{or this.showJsonView this.secretDataIsAdvanced}}
@showJson={{this.showJsonView}}
@secret={{@secret}}
@modelValidations={{this.modelValidations}}
@type="edit"
Expand All @@ -58,7 +57,6 @@
@size="small"
@onChange={{fn (mut this.showDiff)}}
@checked={{this.showDiff}}
@disabled={{not this.diffDelta}}
>
<span class="ttl-picker-label is-large">Show diff</span><br />
<div class="description has-text-grey" data-test-diff-description>{{if
Expand Down
6 changes: 4 additions & 2 deletions ui/lib/kv/addon/components/page/secret/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ export default class KvSecretEdit extends Component {
@tracked modelValidations;
@tracked invalidFormAlert;
originalSecret;
secretDataIsAdvanced;

constructor() {
super(...arguments);
this.originalSecret = JSON.stringify(this.args.secret.secretData || {});
this.secretDataIsAdvanced = isAdvancedSecret(this.originalSecret);
if (isAdvancedSecret(this.originalSecret)) {
// Default to JSON view if advanced
this.showJsonView = true;
}
}

get showOldVersionAlert() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {
await click(FORM.saveBtn);
// Future: test that json is automatic on details too
await click(PAGE.detail.createNewVersion);
assert.dom(FORM.toggleJson).isDisabled();
assert.dom(FORM.toggleJson).isNotDisabled();
assert.dom(FORM.toggleJson).isChecked();
});

Expand Down Expand Up @@ -313,6 +313,7 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {
// View the first version and make sure the secret data is correct
await click(PAGE.detail.versionDropdown);
await click(`${PAGE.detail.version(1)} a`);
await click(FORM.toggleJsonValues);
assert.strictEqual(codemirror().getValue(), obscuredDataV1, 'Version one data is displayed');

// Navigate back the second version and make sure the secret data is correct
Expand Down
1 change: 1 addition & 0 deletions ui/tests/helpers/kv/kv-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export const FORM = {
inputByAttr: (attr) => `[data-test-input="${attr}"]`,
fieldByAttr: (attr) => `[data=test=field="${attr}"]`, // formfield
toggleJson: '[data-test-toggle-input="json"]',
toggleJsonValues: '[data-test-toggle-input="revealValues"]',
toggleMasked: '[data-test-button="toggle-masked"]',
toggleMetadata: '[data-test-metadata-toggle]',
jsonEditor: '[data-test-component="code-mirror-modifier"]',
Expand Down
21 changes: 21 additions & 0 deletions ui/tests/integration/components/json-editor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,25 @@ module('Integration | Component | json-editor', function (hooks) {
assert.dom('.CodeMirror-code').hasText(`1${this.example}`, 'Example is restored');
assert.strictEqual(this.value, null, 'Value is cleared on restore example');
});

test('obscure option works correctly', async function (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.

From precursor work

this.set('readOnly', true);
await render(hbs`<JsonEditor
@value={{this.json_blob}}
@allowObscure={{true}}
@readOnly={{this.readOnly}}
@valueUpdated={{this.valueUpdated}}
@onFocusOut={{this.onFocusOut}}
/>`);
assert.dom('.CodeMirror-code').hasText(`{ "test": "********"}`, 'shows data with obscured values');
assert.dom('[data-test-toggle-input="revealValues"]').isNotChecked('reveal values toggle is unchecked');
await click('[data-test-toggle-input="revealValues"]');
assert.dom('.CodeMirror-code').hasText(JSON_BLOB, 'shows data with real values');
assert.dom('[data-test-toggle-input="revealValues"]').isChecked('reveal values toggle is checked');
// turn obscure back on to ensure readonly overrides reveal setting
await click('[data-test-toggle-input="revealValues"]');
this.set('readOnly', false);
assert.dom('[data-test-toggle-input="revealValues"]').doesNotExist('reveal values toggle is hidden');
assert.dom('.CodeMirror-code').hasText(JSON_BLOB, 'shows data with real values on edit mode');
});
});
13 changes: 13 additions & 0 deletions ui/tests/integration/components/kv-object-editor-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,17 @@ module('Integration | Component | kv-object-editor', function (hooks) {
await click('[data-test-kv-delete-row="0"]');
assert.dom('[data-test-kv-whitespace-warning="0"]').doesNotExist();
});

test('it should display object warning for values when warnNonStringValues true', async function (assert) {
const objValue = `{
"a": "b"
}`;
await render(hbs`<KvObjectEditor @onChange={{this.spy}} @warnNonStringValues={{true}} />`);
await fillIn('[data-test-kv-value="0"]', objValue);
assert.dom('[data-test-kv-object-warning="0"]').exists();
await fillIn('[data-test-kv-value="0"]', 'test ');
assert.dom('[data-test-kv-object-warning="0"]').doesNotExist();
await fillIn('[data-test-kv-value="0"]', 7);
assert.dom('[data-test-kv-object-warning="0"]').exists();
});
});
Loading
Loading