feat: masking support for SingleLineEditor

[krummbar]

Description

Fixes #2217

This change adds masking capabilities to a SingleLineEditor

Issue As of now, an environment variable becomes read-only once it is flagged as secret. This is due to the fact, that a div overlay is rendered instead of an input field. In order to change a secret value, one has to unsecret it, edit it, and flag as secret again. This could cause secrets leaks during screen sharing sessions whenever a environment secret needs to be changed.

Solution Now it uses the SingleLineEditor even if the variable is marked as secret, but each rendered character is altered to a *. The actual value is preserved. The masking functionality is only activated and used if <SingleLineEditor isSecret={true} /> is set, otherwise the existing behaviour is preserved.

New environment settings behaviour

https://github.com/usebruno/bruno/assets/1665841/1a3bd6d1-ba8a-4038-8e61-905a04b35379

UI Change

Changed alignment of environment variable toggles. Toggle for en/disabling variables was centered, slightly shift left. The toggle for secrets was left aligned. Now both are centered in the column.

Contribution Checklist:

• [x] The pull request only addresses one issue or adds one feature.
• [x] The pull request does not introduce any breaking changes
• [x] I have added screenshots or gifs to help explain the change if applicable.
• [x] I have read the contribution guidelines.
• [x] Create an issue and link to the pull request.

[lizziemac]

Nice!! Instead of passing in the masking from the isSecret checkbox, I think it makes more sense to have the ability to mask/unmask be within the field itself (e.g. an eye open/closed icon in the input field where the masking occurs). This is mainly because by toggling the secret field, there's a risk that the secret is saved as a value instead, which by effect could accidentally be saved in a remote repository.

[krummbar]

Nice!! Instead of passing in the masking from the isSecret checkbox, I think it makes more sense to have the ability to mask/unmask be within the field itself (e.g. an eye open/closed icon in the input field where the masking occurs). This is mainly because by toggling the secret field, there's a risk that the secret is saved as a value instead, which by effect could accidentally be saved in a remote repository.

Very true, thanks for your feedback! I'll try to add this to the PR

[krummbar]

Now it looks like this.

https://github.com/usebruno/bruno/assets/1665841/81486706-aaea-4bcb-b337-066d39824dbe

After extending the visuals, I'm wondering if the maskable capabilities should be added as an individual component. Say MaskableSingleLineEditor derived from SingleLineEditor, which has the visuals integrated as well. I can't estimate if this an easy straight-forward implementation as I'm just starting out with the code base and learning about react.

Maybe someone who has experience can give feedback if it would be feasible to separate the logic here or if this can be merged as it is right now.

[lizziemac]

Looks great!

Maybe someone who has experience can give feedback if it would be feasible to separate the logic here or if this can be merged as it is right now.

I've provided a .patch of what you could do to make it solely a part of SingleLineEditor. Generally for features like this in my own projects, I prefer reducing code duplication as much as possible, which is why the patch file keeps the changes in SingleLineEditor, as opposed to a new module (e.g. MaskableSingleLineEditor ) that uses a lot of the same base utilities.

Some improvements could be made to this change, like removing logs and renaming some functions.

To apply to your own branch for testing, do git apply suggested_change.patch suggested_change.patch

Contents (if you don't want to download a file):

diff --git a/packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js b/packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js
index 9677fc36..6c1dc2a5 100644
--- a/packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js
+++ b/packages/bruno-app/src/components/Environments/EnvironmentSettings/EnvironmentList/EnvironmentDetails/EnvironmentVariables/index.js
@@ -7,7 +7,6 @@ import StyledWrapper from './StyledWrapper';
 import { uuid } from 'utils/common';
 import { useFormik } from 'formik';
 import * as Yup from 'yup';
-import { IconEye, IconEyeOff } from '@tabler/icons';
 import { variableNameRegex } from 'utils/common/regex';
 import { saveEnvironment } from 'providers/ReduxStore/slices/collections/actions';
 import cloneDeep from 'lodash/cloneDeep';
@@ -58,15 +57,6 @@ const EnvironmentVariables = ({ environment, collection, setIsModified, original
     setIsModified(formik.dirty);
   }, [formik.dirty]);
 
-  const toggleVisibleSecret = (variable) => {
-    visibleSecrets[variable.uid] = !visibleSecrets[variable.uid];
-    setVisibleSecrets({ ...visibleSecrets });
-  };
-
-  const isMaskedSecret = (variable) => {
-    return variable.secret && visibleSecrets[variable.uid] !== true;
-  };
-
   const ErrorMessage = ({ name }) => {
     const meta = formik.getFieldMeta(name);
     if (!meta.error) {
@@ -148,19 +138,10 @@ const EnvironmentVariables = ({ environment, collection, setIsModified, original
                       collection={collection}
                       name={`${index}.value`}
                       value={variable.value}
-                      maskInput={isMaskedSecret(variable)}
+                      isSecret={variable.secret}
                       onChange={(newValue) => formik.setFieldValue(`${index}.value`, newValue, true)}
                     />
                   </div>
-                  {variable.secret ? (
-                    <button type="button" className="btn btn-sm !pr-0" onClick={() => toggleVisibleSecret(variable)}>
-                      {isMaskedSecret(variable) ? (
-                        <IconEyeOff size={18} strokeWidth={2} />
-                      ) : (
-                        <IconEye size={18} strokeWidth={2} />
-                      )}
-                    </button>
-                  ) : null}
                 </td>
                 <td>
                   <input
diff --git a/packages/bruno-app/src/components/SingleLineEditor/index.js b/packages/bruno-app/src/components/SingleLineEditor/index.js
index f295d712..04cc0e37 100644
--- a/packages/bruno-app/src/components/SingleLineEditor/index.js
+++ b/packages/bruno-app/src/components/SingleLineEditor/index.js
@@ -3,6 +3,7 @@ import isEqual from 'lodash/isEqual';
 import { getAllVariables } from 'utils/collections';
 import { defineCodeMirrorBrunoVariablesMode, MaskedEditor } from 'utils/common/codemirror';
 import StyledWrapper from './StyledWrapper';
+import { IconEye, IconEyeOff } from '@tabler/icons';
 
 let CodeMirror;
 const SERVER_RENDERED = typeof navigator === 'undefined' || global['PREVENT_CODEMIRROR_RENDER'] === true;
@@ -20,7 +21,11 @@ class SingleLineEditor extends Component {
     this.cachedValue = props.value || '';
     this.editorRef = React.createRef();
     this.variables = {};
-  }
+
+    this.state = {
+      maskInput: props.isSecret || false, // Always mask the input by default (if it's a secret)
+    };
+  }
   componentDidMount() {
     // Initialize CodeMirror as a single line editor
     /** @type {import("codemirror").Editor} */
@@ -91,11 +96,13 @@ class SingleLineEditor extends Component {
     this.editor.setValue(String(this.props.value) || '');
     this.editor.on('change', this._onEdit);
     this.addOverlay();
-    if (this.props.maskInput) this._enableMaskedEditor(this.props.maskInput);
+    this._enableMaskedEditor(this.props.isSecret);
+    this.setState({ maskInput: this.props.isSecret });
   }
 
   /** Enable or disable masking the rendered content of the editor */
   _enableMaskedEditor = (enabled) => {
+    console.log('Enabling masked editor: ' + enabled)
     if (enabled == true) {
       if (!this.maskedEditor) this.maskedEditor = new MaskedEditor(this.editor, '*');
       this.maskedEditor.enable();
@@ -131,11 +138,13 @@ class SingleLineEditor extends Component {
       this.cachedValue = String(this.props.value);
       this.editor.setValue(String(this.props.value) || '');
     }
-    if (!isEqual(this.props.maskInput, prevProps.maskInput)) {
-      this._enableMaskedEditor(this.props.maskInput);
-    }
-    if (this.props.maskInput) {
+    if (!isEqual(this.props.isSecret, prevProps.isSecret)) {
+      console.log('Secret flag changed to ' + this.props.isSecret);
+      // If the secret flag has changed, update the editor to reflect the change
+      this._enableMaskedEditor(this.props.isSecret);
       this.maskedEditor?.update();
+      // also set the maskInput flag to the new value
+      this.setState({ maskInput: this.props.isSecret });
     }
     this.ignoreChangeEvent = false;
   }
@@ -152,8 +161,39 @@ class SingleLineEditor extends Component {
     this.editor.setOption('mode', 'brunovariables');
   };
 
+  toggleVisibleSecret = () => {
+    const isVisible = !this.state.maskInput;
+    this.setState({ maskInput: isVisible });
+    this._enableMaskedEditor(isVisible);
+    console.log('Secret flag changed to ' + isVisible);
+  }
+
+  /**
+   * @brief Eye icon to show/hide the secret value
+   * @returns ReactComponent The eye icon
+   */
+  secretEye = (isSecret) => {
+    return (
+      isSecret === true ? (
+        <button type="button" className="btn btn-sm !pr-0" onClick={() => this.toggleVisibleSecret()}>
+          {this.state.maskInput === true ? (
+            <IconEyeOff size={18} strokeWidth={2} />
+          ) : (
+            <IconEye size={18} strokeWidth={2} />
+          )}
+        </button>
+      ) : null
+    );
+  }
+
+
   render() {
-    return <StyledWrapper ref={this.editorRef} className="single-line-editor"></StyledWrapper>;
+    return (
+      <div style={{ display: 'flex', justifyContent: 'space-between', alignItems: 'center' }}>
+        <StyledWrapper ref={this.editorRef} className="single-line-editor" />
+        {this.secretEye(this.props.isSecret)}
+      </div>
+    );
   }
 }
 export default SingleLineEditor;

Edits: bug fix related to changing between secret and non-secret, and clarification on a comment

[krummbar]

Looks great!

Maybe someone who has experience can give feedback if it would be feasible to separate the logic here or if this can be merged as it is right now.

I've provided a .patch of what you could do to make it solely a part of SingleLineEditor. Generally for features like this in my own projects, I prefer reducing code duplication as much as possible, which is why the patch file keeps the changes in SingleLineEditor, as opposed to a new module (e.g. MaskableSingleLineEditor ) that uses a lot of the same base utilities.

Some improvements could be made to this change, like removing logs and renaming some functions.

To apply to your own branch for testing, do git apply suggested_change.patch suggested_change.patch

Contents (if you don't want to download a file): Edits: bug fix related to changing between secret and non-secret, and clarification on a comment

Cool thanks for your valuable input! I've applied your patch, looks a lot cleaner now :) I just removed two console.log statements as they were logging redundant information.

If you have a no-reply mail I'll add you as co-author, which is required according to the documentation

[lizziemac]

Cool thanks for your valuable input! I've applied your patch, looks a lot cleaner now :) I just removed two console.log statements as they were logging redundant information.

I'm happy to help! :D And thanks for getting those logs

If you have a no-reply mail I'll add you as co-author, which is required according to the documentation

Absolutely! My email is [email protected]

Also, if you rebase off of main, your tests should pass now :-)

[krummbar]

Absolutely! My email is [email protected]

Done ☺️

Also, if you rebase off of main, your tests should pass now :-)

Didn't realize this has already been fixed, thanks!

[pietrygamat]

+1 I can see it also fixed this annoying bobbing: before and after:

What I find missing is any indication on the eye icon when it is focused (when using keyboard navigation) - it can be selected, and activated with space, but its not highlighted in any way.

[krummbar]

What I find missing is any indication on the eye icon when it is focused (when using keyboard navigation) - it can be selected, and activated with space, but its not highlighted in any way.

Good catch, didn't think about keyboard navigation. It should work now.

Screencast from 2024-05-08 06-48-59.webm

---

While I'm at it... I think the columns enabled and secret look a little bit odd.

• enabled is centered, but slightly shifted left
• secret is aligned left

IMHO it looks better if both columns are simply centered. If that is fine I can add this change to the PR.

[krummbar]

The content for both columns is now centered. I've updated the PR description to match the changes that have been applied in the meantime.

[lizziemac]

@lohxt1 this might be an easy win for merging

[krummbar]

@lohxt1 I'll make sure to merge the most recent changes from the main branch today. I've just tried it and it seems like the layout of request address bar is breaking now. (see attached screenshot). I cannot reproduce it on the main branch so I assume the merge somehow causing it. I'll try to figure it out and fix it

[krummbar]

Rebased the PR on the latest main changes and fixed the UI glitch. Hope this is fine now, let me know if it still needs some changes.

[nataliecarey]

This PR would really help for the PR I'm in the process of raising to resolve https://github.com/usebruno/bruno/issues/2747

The underlying work @krummbar has done here is easy to re-use in other related situations.

[pasalino]

Can we have some information on when this PR will be merged? It seems ready to merge.

This is a core feature for environment management especially if you have collections with secrets in your repo.

[helloanoop]

Merged! Thanks @krummbar @lizziemac for working on this! Thanks for @pietrygamat @lizziemac @nataliecarey for the reviews and helping polish this feature.