-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix: Add PasswordField to mask input value of the field with `format:… #1059
Conversation
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.
Looks good @igarashitm, apart from the couple comments, there's something that I'm not quite sure why happens.
At the moment of writing, this is how the form looks like
But with this PR, for some reason, those fields get prefilled with autocompletion coming from the browser
And more important, those values gets serialized in the YAML 😕
const generatePassword = () => { | ||
const length = 12; | ||
const charset = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@%()_-=+'; | ||
let retVal = ''; | ||
for (let i = 0, n = charset.length; i < length; ++i) { | ||
retVal += charset.charAt(Math.floor(Math.random() * n)); | ||
} | ||
return retVal; | ||
}; |
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.
Could be possible for this function to be moved to the utils folder?
const autocomplete = ( | ||
<Menu ref={autocompleteRef} onSelect={onSelect}> | ||
<MenuContent> | ||
<MenuList> | ||
<MenuItem | ||
data-testid={'password-use-suggested-menuitem'} | ||
itemId={0} | ||
actions={ | ||
<MenuItemAction | ||
icon={<RedoIcon aria-hidden />} | ||
onClick={(_e) => { | ||
setGeneratedPassword(generatePassword()); | ||
}} | ||
actionId="redo" | ||
aria-label="Generate a new suggested password" | ||
/> | ||
} | ||
> | ||
Use suggested password: <b>{`${generatedPassword}`}</b> | ||
</MenuItem> | ||
</MenuList> | ||
</MenuContent> | ||
</Menu> | ||
); |
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.
Considering that we need to provide existing passwords and that we're not creating new user accounts through Kaoto (at least not at the moment) I wonder if we would be better off without the password generator bit.
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.
TBH I have no idea from what background we wanted to introduce password generator, @lhein could you add some context?
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.
The idea was that this component provides the visibility switch and the masking functionality ootb. If you can come up with something similar I am not opposed to hear your solution :)
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.
OK so no need to use PF password generator at all, visibility toggle is just a plain button and the masking is HTML standard. I'll remove the password generator.
w/r/t the browser auto fill, I know chrome is eager like crazy to fill the username/password thing everywhere, I think it's done if the input name attribute is same on the combination of one |
Maybe this will help: https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion Adding |
… password` Fixes: KaotoIO#285 Fixes: KaotoIO#503
Co-authored-by: Ricardo M. <lordrip@gmail.com>
Co-authored-by: Ricardo M. <lordrip@gmail.com>
Co-authored-by: Ricardo M. <lordrip@gmail.com>
… password`
Fixes: #285
Fixes: #503