From e122ce80de458ea915eb65d783e47c337de8e3c8 Mon Sep 17 00:00:00 2001
From: Chelsea Shaw <82459713+hashishaw@users.noreply.github.com>
Date: Wed, 13 Dec 2023 15:38:43 -0600
Subject: [PATCH] UI: better calculation for advanced secret in KV v2 (#24513)

* Add util for determining whether secret data is advanced

* Add test coverage for bug

* use non-dumb logic for detecting advanced object

* Add changelog

* Add header

* Move util to core

* Add escaped newline to test coverage

* headers again *eyeroll*
---
 changelog/24513.txt                           |  3 ++
 ui/lib/core/addon/utils/advanced-secret.js    | 20 ++++++++++
 ui/lib/core/app/utils/advanced-secret.js      |  6 +++
 .../addon/components/page/secret/details.js   |  6 +--
 .../kv/addon/components/page/secret/edit.js   |  6 +--
 .../kv/kv-v2-workflow-edge-cases-test.js      | 13 +++++-
 ui/tests/unit/utils/advanced-secret-test.js   | 40 +++++++++++++++++++
 7 files changed, 85 insertions(+), 9 deletions(-)
 create mode 100644 changelog/24513.txt
 create mode 100644 ui/lib/core/addon/utils/advanced-secret.js
 create mode 100644 ui/lib/core/app/utils/advanced-secret.js
 create mode 100644 ui/tests/unit/utils/advanced-secret-test.js

diff --git a/changelog/24513.txt b/changelog/24513.txt
new file mode 100644
index 0000000000..41b47f6be4
--- /dev/null
+++ b/changelog/24513.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+ui: fix KV v2 details view defaulting to JSON view when secret value includes `{`
+```
diff --git a/ui/lib/core/addon/utils/advanced-secret.js b/ui/lib/core/addon/utils/advanced-secret.js
new file mode 100644
index 0000000000..3046c4b557
--- /dev/null
+++ b/ui/lib/core/addon/utils/advanced-secret.js
@@ -0,0 +1,20 @@
+/**
+ * Copyright (c) HashiCorp, Inc.
+ * SPDX-License-Identifier: MPL-2.0
+ */
+
+/**
+ * Method to check whether the secret value is a nested object (returns true)
+ * All other values return false
+ * @param value string or stringified JSON
+ * @returns boolean
+ */
+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');
+  } catch (e) {
+    return false;
+  }
+}
diff --git a/ui/lib/core/app/utils/advanced-secret.js b/ui/lib/core/app/utils/advanced-secret.js
new file mode 100644
index 0000000000..0975476278
--- /dev/null
+++ b/ui/lib/core/app/utils/advanced-secret.js
@@ -0,0 +1,6 @@
+/**
+ * Copyright (c) HashiCorp, Inc.
+ * SPDX-License-Identifier: BUSL-1.1
+ */
+
+export { default } from 'core/utils/advanced-secret';
diff --git a/ui/lib/kv/addon/components/page/secret/details.js b/ui/lib/kv/addon/components/page/secret/details.js
index 6924519cde..e1d16898de 100644
--- a/ui/lib/kv/addon/components/page/secret/details.js
+++ b/ui/lib/kv/addon/components/page/secret/details.js
@@ -11,6 +11,7 @@ import { inject as service } from '@ember/service';
 import { task } from 'ember-concurrency';
 import { waitFor } from '@ember/test-waiters';
 import { isDeleted } from 'kv/utils/kv-deleted';
+import { isAdvancedSecret } from 'core/utils/advanced-secret';
 
 /**
  * @module KvSecretDetails renders the key/value data of a KV secret.
@@ -42,10 +43,7 @@ export default class KvSecretDetails extends Component {
     super(...arguments);
     this.fetchSyncStatus.perform();
     this.originalSecret = JSON.stringify(this.args.secret.secretData || {});
-    if (this.originalSecret.lastIndexOf('{') > 0) {
-      // Dumb way to check if there's a nested object in the secret
-      this.secretDataIsAdvanced = true;
-    }
+    this.secretDataIsAdvanced = isAdvancedSecret(this.originalSecret);
   }
 
   @action
diff --git a/ui/lib/kv/addon/components/page/secret/edit.js b/ui/lib/kv/addon/components/page/secret/edit.js
index 26e8b39295..3ef4ef49c8 100644
--- a/ui/lib/kv/addon/components/page/secret/edit.js
+++ b/ui/lib/kv/addon/components/page/secret/edit.js
@@ -9,6 +9,7 @@ import { tracked } from '@glimmer/tracking';
 import { task } from 'ember-concurrency';
 import { inject as service } from '@ember/service';
 import errorMessage from 'vault/utils/error-message';
+import { isAdvancedSecret } from 'core/utils/advanced-secret';
 
 /**
  * @module KvSecretEdit is used for creating a new version of a secret
@@ -43,10 +44,7 @@ export default class KvSecretEdit extends Component {
   constructor() {
     super(...arguments);
     this.originalSecret = JSON.stringify(this.args.secret.secretData || {});
-    if (this.originalSecret.lastIndexOf('{') > 0) {
-      // Dumb way to check if there's a nested object in the secret
-      this.secretDataIsAdvanced = true;
-    }
+    this.secretDataIsAdvanced = isAdvancedSecret(this.originalSecret);
   }
 
   get showOldVersionAlert() {
diff --git 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
index 87aad84cc2..c5eb6a2a59 100644
--- 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
@@ -271,7 +271,7 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {
     assert.dom(PAGE.list.item()).exists({ count: 2 }, 'two secrets are listed');
   });
 
-  test('complex values default to JSON display', async function (assert) {
+  test('advanced secret values default to JSON display', async function (assert) {
     await visit(`/vault/secrets/${this.backend}/kv/create`);
     await fillIn(FORM.inputByAttr('path'), 'complex');
 
@@ -284,6 +284,17 @@ module('Acceptance | kv-v2 workflow | edge cases', function (hooks) {
     assert.dom(FORM.toggleJson).isDisabled();
     assert.dom(FORM.toggleJson).isChecked();
   });
+  test('does not register as advanced when value includes {', async function (assert) {
+    await visit(`/vault/secrets/${this.backend}/kv/create`);
+    await fillIn(FORM.inputByAttr('path'), 'not-advanced');
+
+    await fillIn(FORM.keyInput(), 'foo');
+    await fillIn(FORM.maskedValueInput(), '{bar}');
+    await click(FORM.saveBtn);
+    await click(PAGE.detail.createNewVersion);
+    assert.dom(FORM.toggleJson).isNotDisabled();
+    assert.dom(FORM.toggleJson).isNotChecked();
+  });
 });
 
 // NAMESPACE TESTS
diff --git a/ui/tests/unit/utils/advanced-secret-test.js b/ui/tests/unit/utils/advanced-secret-test.js
new file mode 100644
index 0000000000..891a13ea9d
--- /dev/null
+++ b/ui/tests/unit/utils/advanced-secret-test.js
@@ -0,0 +1,40 @@
+/**
+ * Copyright (c) HashiCorp, Inc.
+ * SPDX-License-Identifier: BUSL-1.1
+ */
+
+import { isAdvancedSecret } from 'core/utils/advanced-secret';
+import { module, test } from 'qunit';
+
+module('Unit | Utility | advanced-secret', function () {
+  test('it returns false for non-valid JSON', function (assert) {
+    assert.expect(5);
+    let result;
+    ['some-string', 'character{string', '{value}', '[blah]', 'multi\nline\nstring'].forEach((value) => {
+      result = isAdvancedSecret('some-string');
+      assert.false(result, `returns false for ${value}`);
+    });
+  });
+
+  test('it returns false for single-level objects', function (assert) {
+    assert.expect(3);
+    let result;
+    [{ single: 'one' }, { first: '1', two: 'three' }, ['my', 'array']].forEach((value) => {
+      result = isAdvancedSecret(JSON.stringify(value));
+      assert.false(result, `returns false for object ${JSON.stringify(value)}`);
+    });
+  });
+
+  test('it returns true for any nested object', function (assert) {
+    assert.expect(3);
+    let result;
+    [
+      { single: { one: 'uno' } },
+      { first: ['this', 'counts\ntoo'] },
+      { deeply: { nested: { item: 1 } } },
+    ].forEach((value) => {
+      result = isAdvancedSecret(JSON.stringify(value));
+      assert.true(result, `returns true for object ${JSON.stringify(value)}`);
+    });
+  });
+});