UI: KV version 2 advanced secret updates (#25235)

* Show JSON editor by default if secret data is complex, but do not disable JSON toggle * Add text object warning to kv object editor * a11y fix: do not disable show diff toggle * test and language cleanup * update language * Update tests for expected behavior * language! * Add changelog
2025-10-29 01:32:33 +00:00 · 2024-02-06 11:50:22 -06:00
parent a775c17cf9
commit 7c14aa6e00
18 changed files with 72 additions and 56 deletions
--- a/changelog/25235.txt
+++ b/changelog/25235.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ui: Do not disable JSON display toggle for KV version 2 secrets
+```
--- a/ui/lib/core/addon/components/json-editor.hbs
+++ b/ui/lib/core/addon/components/json-editor.hbs
@@ -25,7 +25,7 @@
              data-test-restore-example
            />
          {{/if}}
-          {{#if (and @obscure @readOnly)}}
+          {{#if (and @allowObscure @readOnly)}}
            {{! For safety we only use obscured values in readonly mode }}
            <div>
              <Toggle @name="revealValues" @checked={{this.revealValues}} @onChange={{fn (mut this.revealValues)}}>
--- a/ui/lib/core/addon/components/json-editor.js
+++ b/ui/lib/core/addon/components/json-editor.js
@@ -43,7 +43,7 @@ export default class JsonEditorComponent extends Component {
  }

  get showObfuscatedData() {
-    return this.args.readOnly && this.args.obscure && !this.revealValues;
+    return this.args.readOnly && this.args.allowObscure && !this.revealValues;
  }
  get obfuscatedData() {
    return stringify([obfuscateData(JSON.parse(this.args.value))], {});
--- a/ui/lib/core/addon/components/kv-object-editor.hbs
+++ b/ui/lib/core/addon/components/kv-object-editor.hbs
@@ -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>
@@ -71,7 +70,7 @@
          </div>
        {{/unless}}
      </div>
-      {{#if (includes index this.whitespaceWarningRows)}}
+      {{#if (this.showWhitespaceWarning row.name)}}
        <div class="has-bottom-margin-s">
          <AlertInline
            @type="warning"
@@ -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|>
--- a/ui/lib/core/addon/components/kv-object-editor.js
+++ b/ui/lib/core/addon/components/kv-object-editor.js
@@ -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';

 /**
@@ -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 {boolean} [isSingleRow = false] - when true the kv object editor will only show one row and hide the Add button
 * @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
@@ -36,11 +35,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 {
@@ -79,7 +79,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
@@ -88,19 +87,17 @@ export default class KvObjectEditor extends Component {
      this.args.onKeyUp(event.target.value);
    }
  }
-  @action
-  validateKey(rowIndex, event) {
-    if (this.args.allowWhiteSpace) {
-      return;
+  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;
    }
-    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);
-    }
-  }
+  };
 }
--- a/ui/lib/kv/addon/components/kv-data-fields.hbs
+++ b/ui/lib/kv/addon/components/kv-data-fields.hbs
@@ -16,7 +16,7 @@
  <JsonEditor
    @title="{{if (eq @type 'create') 'Secret' 'Version'}} data"
    @value={{this.stringifiedSecretData}}
-    @obscure={{@obscureJson}}
+    @allowObscure={{true}}
    @valueUpdated={{this.handleJson}}
    @readOnly={{eq @type "details"}}
  />
@@ -46,5 +46,6 @@
    @value={{@secret.secretData}}
    @onChange={{fn (mut @secret.secretData)}}
    @isMasked={{true}}
+    @warnNonStringValues={{true}}
  />
 {{/if}}
--- a/ui/lib/kv/addon/components/kv-data-fields.js
+++ b/ui/lib/kv/addon/components/kv-data-fields.js
@@ -24,7 +24,6 @@ import { stringify } from 'core/helpers/stringify';
 * @param {object} [modelValidations] - object of errors.  If attr.name is in object and has error message display in AlertInline.
 * @param {callback} [pathValidations] - callback function fired for the path input on key up
 * @param {boolean} [type=null] - can be edit, create, or details. Used to change text for some form labels
- * @param {boolean} [obscureJson=false] - used to obfuscate json values in JsonEditor
 */

 export default class KvDataFields extends Component {
--- a/ui/lib/kv/addon/components/page/secret/details.hbs
+++ b/ui/lib/kv/addon/components/page/secret/details.hbs
@@ -54,12 +54,7 @@

  <:toolbarFilters>
    {{#unless this.emptyState}}
-      <Toggle
-        @name="json"
-        @checked={{or this.showJsonView this.secretDataIsAdvanced}}
-        @onChange={{fn (mut this.showJsonView)}}
-        @disabled={{this.secretDataIsAdvanced}}
-      >
+      <Toggle @name="json" @checked={{this.showJsonView}} @onChange={{fn (mut this.showJsonView)}}>
        <span class="has-text-grey">JSON</span>
      </Toggle>
    {{/unless}}
@@ -143,8 +138,7 @@
  </EmptyState>
 {{else}}
  <KvDataFields
-    @showJson={{or this.showJsonView this.secretDataIsAdvanced}}
-    @obscureJson={{this.secretDataIsAdvanced}}
+    @showJson={{this.showJsonView}}
    @secret={{@secret}}
    @modelValidations={{this.modelValidations}}
    @type="details"
--- a/ui/lib/kv/addon/components/page/secret/details.js
+++ b/ui/lib/kv/addon/components/page/secret/details.js
@@ -37,13 +37,15 @@ export default class KvSecretDetails extends Component {
  @tracked showJsonView = false;
  @tracked wrappedData = null;
  @tracked syncStatus = null; // array of association sync status info by destination
-  secretDataIsAdvanced;

  constructor() {
    super(...arguments);
    this.fetchSyncStatus.perform();
    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
--- a/ui/lib/kv/addon/components/page/secret/edit.hbs
+++ b/ui/lib/kv/addon/components/page/secret/edit.hbs
@@ -5,12 +5,7 @@

 <KvPageHeader @breadcrumbs={{@breadcrumbs}} @pageTitle="Create New Version">
  <:toolbarFilters>
-    <Toggle
-      @name="json"
-      @checked={{or this.showJsonView this.secretDataIsAdvanced}}
-      @onChange={{fn (mut this.showJsonView)}}
-      @disabled={{this.secretDataIsAdvanced}}
-    >
+    <Toggle @name="json" @checked={{this.showJsonView}} @onChange={{fn (mut this.showJsonView)}}>
      <span class="has-text-grey">JSON</span>
    </Toggle>
  </:toolbarFilters>
@@ -43,19 +38,14 @@
    <MessageError @model={{@secret}} @errorMessage={{this.errorMessage}} />

    <KvDataFields
-      @showJson={{or this.showJsonView this.secretDataIsAdvanced}}
+      @showJson={{this.showJsonView}}
      @secret={{@secret}}
      @modelValidations={{this.modelValidations}}
      @type="edit"
    />

    <div class="has-top-margin-m">
-      <Toggle
-        @name="Show diff"
-        @onChange={{fn (mut this.showDiff)}}
-        @checked={{this.showDiff}}
-        @disabled={{not this.diffDelta}}
-      >
+      <Toggle @name="Show diff" @onChange={{fn (mut this.showDiff)}} @checked={{this.showDiff}}>
        <span class="ttl-picker-label is-large">Show diff</span><br />
        <div class="description has-text-grey" data-test-diff-description>{{if
            this.diffDelta
--- a/ui/lib/kv/addon/components/page/secret/edit.js
+++ b/ui/lib/kv/addon/components/page/secret/edit.js
@@ -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() {
--- a/ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-edge-cases-test.js
+++ b/ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-edge-cases-test.js
@@ -286,7 +286,7 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {
    await click(FORM.saveBtn);

    // Details view
-    assert.dom(FORM.toggleJson).isDisabled();
+    assert.dom(FORM.toggleJson).isNotDisabled();
    assert.dom(FORM.toggleJson).isChecked();
    assert.strictEqual(
      codemirror().getValue(),
@@ -298,7 +298,7 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {

    // New version view
    await click(PAGE.detail.createNewVersion);
-    assert.dom(FORM.toggleJson).isDisabled();
+    assert.dom(FORM.toggleJson).isNotDisabled();
    assert.dom(FORM.toggleJson).isChecked();
    assert.false(codemirror().getValue().includes('*'), 'Values are not obscured on edit view');
  });
--- a/ui/tests/helpers/kv/kv-selectors.js
+++ b/ui/tests/helpers/kv/kv-selectors.js
@@ -96,6 +96,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"]',
--- a/ui/tests/integration/components/json-editor-test.js
+++ b/ui/tests/integration/components/json-editor-test.js
@@ -114,7 +114,7 @@ module('Integration | Component | json-editor', function (hooks) {
    this.set('readOnly', true);
    await render(hbs`<JsonEditor
      @value={{this.json_blob}}
-      @obscure={{true}}
+      @allowObscure={{true}}
      @readOnly={{this.readOnly}}
      @valueUpdated={{this.valueUpdated}}
      @onFocusOut={{this.onFocusOut}}
--- a/ui/tests/integration/components/kv-object-editor-test.js
+++ b/ui/tests/integration/components/kv-object-editor-test.js
@@ -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();
+  });
 });
--- a/ui/tests/integration/components/kv/kv-data-fields-test.js
+++ b/ui/tests/integration/components/kv/kv-data-fields-test.js
@@ -96,7 +96,7 @@ module('Integration | Component | kv-v2 | KvDataFields', function (hooks) {
  });

  test('it shows readonly json editor when viewing secret details of complex secret', async function (assert) {
-    assert.expect(3);
+    assert.expect(4);
    this.secret.secretData = {
      foo: {
        bar: 'baz',
@@ -109,6 +109,10 @@ module('Integration | Component | kv-v2 | KvDataFields', function (hooks) {
    });
    assert.dom(PAGE.infoRowValue('foo')).doesNotExist('does not render rows of secret data');
    assert.dom('[data-test-component="code-mirror-modifier"]').hasClass('readonly-codemirror');
+    assert
+      .dom('[data-test-component="code-mirror-modifier"]')
+      .includesText(`{ "foo": { "bar": "********" }}`);
+    await click(FORM.toggleJsonValues);
    assert.dom('[data-test-component="code-mirror-modifier"]').includesText(`{ "foo": { "bar": "baz" }}`);
  });
 });
--- a/ui/tests/integration/components/kv/page/kv-page-secret-details-test.js
+++ b/ui/tests/integration/components/kv/page/kv-page-secret-details-test.js
@@ -135,6 +135,7 @@ module('Integration | Component | kv-v2 | Page::Secret::Details', function (hook
    await click(FORM.toggleMasked);
    assert.dom(PAGE.infoRowValue('foo')).hasText('bar', 'renders secret value');
    await click(FORM.toggleJson);
+    await click(FORM.toggleJsonValues);
    assert.propEqual(parseJsonEditor(find), this.secretData, 'json editor renders secret data');
    assert
      .dom(PAGE.detail.versionTimestamp)
@@ -142,7 +143,7 @@ module('Integration | Component | kv-v2 | Page::Secret::Details', function (hook
  });

  test('it renders json view when secret is complex', async function (assert) {
-    assert.expect(3);
+    assert.expect(4);
    await render(
      hbs`
       <Page::Secret::Details
@@ -154,7 +155,8 @@ module('Integration | Component | kv-v2 | Page::Secret::Details', function (hook
      { owner: this.engine }
    );
    assert.dom(PAGE.infoRowValue('foo')).doesNotExist('does not render rows of secret data');
-    assert.dom(FORM.toggleJson).isDisabled();
+    assert.dom(FORM.toggleJson).isChecked();
+    assert.dom(FORM.toggleJson).isNotDisabled();
    assert.dom('[data-test-component="code-mirror-modifier"]').exists('shows json editor');
  });

--- a/ui/tests/integration/components/kv/page/kv-page-secret-edit-test.js
+++ b/ui/tests/integration/components/kv/page/kv-page-secret-edit-test.js
@@ -114,7 +114,7 @@ module('Integration | Component | kv-v2 | Page::Secret::Edit', function (hooks)
      { owner: this.engine }
    );

-    assert.dom(PAGE.edit.toggleDiff).isDisabled('Diff toggle is disabled');
+    assert.dom(PAGE.edit.toggleDiff).isNotDisabled('Diff toggle is not disabled');
    assert.dom(PAGE.edit.toggleDiffDescription).hasText('No changes to show. Update secret to view diff');
    assert.dom(PAGE.diff.visualDiff).doesNotExist('Does not show visual diff');