-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Mask obfuscated Secret sync create/edit fields #27348
Conversation
CI Results: |
@@ -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}} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Build Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, and elegant solution! 🚢
@@ -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 ""}} |
There was a problem hiding this comment.
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
data-test-textarea={{or @name ""}} | |
data-test-textarea={{@name}} |
There was a problem hiding this comment.
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?
@@ -131,6 +131,28 @@ module('Integration | Component | sync | Secrets::Page::Destinations::CreateAndE | |||
await click(PAGE.saveButton); | |||
}); | |||
|
|||
test('edit: payload only contains masked inputs when they have changed', async function (assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
This PR masks while typing the obfuscated fields on the destinations create/edit form for Secrets sync. Screenshots for the changes are included on the file changes view.
Notes:
Not backporting to 1.17.x. This is a feature and too many changes to safely backport.
GCP json credentials field is the only param using the enable-input wrap and not the masked-input because it's not a simple one-to-one replacement and enable-input is doing a great job at handling it's use case.
Enterprise test pass
Tested that patch still works as expected. There used to be an issue where we'd send ** values if a obfuscated field had been changed previously and then not changed again. I will link to the internal JIRA ticket within slack.