HTML password fields #926

martinpovolny · 2014-10-24T14:22:06Z

Do not pass values to HTML form input type password fields.

Fryguy · 2014-10-24T16:03:42Z

vmdb/app/views/layouts/_auth_credentials.html.erb

@@ -24,7 +24,7 @@
  <td class="key"><%= pwd_label %></td>
  <td>
    <%= password_field_tag("#{pfx}_password",
-                          @edit[:new]["#{pfx}_password".to_sym],
+                          '',


This makes it appear like no password was ever entered which can be confusing. On other applications I used to put a literal 8 stars ********, then on submission if it was an update it meant keep the existing password, and if it's a new entry, reject 8 stars as a possibility for a password.

This is such a common thing with password fields that I'm surprised a solution doesn't already exist. It almost feels like the HTML tag itself should have a "populated" parameter or something, so you can leave the value blank, but it will display 8 stars or maybe a faded background string like "password saved" or something.

Perhaps you can use the placeholder attribute with 8 stars? or 8 dots: placeholder="●●●●●●●●"

@Fryguy: Maybe I could play with the placeholder: make it "password saved" when, there already is a password and "password empty" when there's empty password or none was entered. What do you think?

In the future I'd address this with a new control such as a field with or w/o stars and button "Set password" that would open a modal overlay with the password entry fields and a button or something similar. But I'd do that as part of the Angular project.

Here I just aim to have that stuff working (not necessary pretty) and have id merged ASAP.

martinpovolny · 2014-10-28T16:24:03Z

@h-kataria : can you review this, please?

h-kataria · 2014-10-28T19:25:34Z

@martinpovolny verified fix, it makes me think as if there is no value entered in password fields even tho there is a value but not being displayed. @Fryguy you can merge if you are good with these changes.

martinpovolny · 2014-10-28T20:22:31Z

@h-kataria : yes, it's far from perfect, we shall work on this, but I'd merge this and carry on from there

Fryguy · 2014-10-28T21:19:54Z

I'm ok with merging if the rest is done in a follow up PR. I just don't want us to forget because the user experience is not good...feels weird. Is it hard to add something like placeholder="●●●●●●●●" into the code in this PR, or do you really need a separate PR for that.

Or make an application_helper method like password_placeholder that returns "●●●●●●●●".html_safe, and the option can be :placeholder => password_placeholder,

martinpovolny · 2014-10-29T12:15:10Z

@Fryguy : added the placeholders as you wanted.
@h-kataria : please, check this quickly one again before we merge

Thx!

h-kataria · 2014-10-29T13:15:07Z

@martinpovolny when there is no value saved in password field it shows blurred out ●●●●●●●● in the text field, i guess you will be fixing that in a separate PR but other than that it looks good.

martinpovolny · 2014-10-29T13:20:29Z

@h-kataria : I understood that that was what @Fryguy suggested -- displaying the image. But really we should discuss that as a separate problem and address in a new PR.

Fryguy · 2014-10-29T18:16:45Z

Yeah, placeholder will show as grey, but that's better than showing nothing at all. I guess you could use CSS styling to make it black if necessary, but that can be a follow up PR too.

@martinpovolny did you test all form submission cases?

User does not have a saved password and
- User enters new password (Should save the password)
- User does not enter a password (Should give a validation error)
User has a saved password and
- User enters new password (Should save the password)
- User changes something else in the form, but does not touch the password (Should not change the saved password, and should not fail validation. Technically the form submission is a blank string, so I'm curious if the controller will ignore, or try to set the blank password giving a validation error.)

If all those cases work, then I think this is good to merge.

martinpovolny · 2014-10-29T18:34:35Z

@Fryguy : the forms are not submitted normally. The viewstate in the session is updated upon DOM event. The "submit" logic does not have to do anything about the field being empty because normal form submission is not the source of the password values.

Therefor the patch is so simple and the logic remained unchanged.

For that reason I did not test all the cases you enumerate in all forms. I made sure the password do change in the view state when changed in the browser and I tested some saving loading etc. I went through all the forms but did not do a checklist as you enumerated it.

Fryguy · 2014-10-29T19:02:42Z

Oh interesting. I think I'm just concerned really about the last use case mentioned. Because the value is blank, I was concerned it would set the password to blank.

Based on what you described I could see another issue with the following workflow:

User has a saved password
User starts to type a new password
User changes their mind and deletes the password (when the box is empty the dots appear again)
User optionally changes something else in the form
User submits the form.

If the view state is changing as they type, will this cause a blank to be set to the password field? Will that cause a validation error, and be weird in the UI?

martinpovolny · 2014-10-29T19:19:27Z

The password would be blank in that case and based on the logic of the particular task either a validation error would occur (and a flash) or successful saving of the blank password would occur.

Fryguy · 2014-10-31T18:46:16Z

So, is that a problem preventing merge, or you're ok with that? Is that how the functionality works now, and thus nothing changed?

martinpovolny · 2014-10-31T18:55:17Z

@Fryguy : from my point of view the current status is not ideal, but works for me. That applies also for the status w/o the last patch. So if you agree I would merge. We can improve the user experience in a follow up patch.

Fryguy · 2014-10-31T19:00:06Z

I reread the code and am confused. Is the placeholder dots there when a password has not been entered yet? That's not what I was expecting. I was expecting the placeholder to only be there is we actually had a password saved. It is an indicator to the user that they entered a password before.

h-kataria · 2014-10-31T19:01:10Z

@Fryguy yes, it shows dots in the field even when password has not been entered.

Fryguy · 2014-10-31T19:02:14Z

Oh, that's just as bad in the opposite way. Sorry, maybe I wasn't clear but I expected the placeholder to only have dots when we have a saved password. It is a visual indicator to the user that some value is present.

h-kataria · 2014-10-31T19:05:19Z

@Fryguy it looked odd to me which is why i had mentioned that in my comment above but my understanding was that @martinpovolny is going to fix that in a separate PR.

Fryguy · 2014-10-31T19:28:38Z

@h-kataria I misunderstood your comment. If it's in a follow up PR, I guess that's ok, I just don't want it to be forgotten. It's still not a good UX.

martinpovolny · 2014-10-31T20:24:17Z

The fact that there's no password is also an information about the password. So it might actually not be a bad thing that the dots are there even for an empty password if we want to prevent "sharing" password information even among people who have admin access.

But as I said, I think that this needs more discussion we shall get back to what functionality in each of the cases so I'd leave that for future work and another PR.

Fryguy · 2014-10-31T23:30:57Z

You can't have an empty password as a legit value IIRC. It would only
truly be blank on initial entry which is why it's weird... It shows dots on
initial entry.
On Oct 31, 2014 4:24 PM, "Martin Povolny" notifications@github.com wrote:

The fact that there's no password is also an information about the
password. So it might actually not be a bad thing that the dots are there
even for an empty password if we want to prevent "sharing" password even
among people who have admin access.

But as I said, I think that this needs more discussion we shall get back
to what functionality in each of the cases so I'd leave that for future
work and another PR.

—
Reply to this email directly or view it on GitHub
#926 (comment).

martinpovolny · 2014-11-12T18:25:40Z

@h-kataria : based on @Fryguy input and our short discussion. What about doing it like this?

Fryguy · 2014-11-15T00:30:28Z

vmdb/app/views/ops/_settings_authentication_tab.html.erb

-                                    "data-miq_observe"=>{:interval=>'.5', :url=>url}.to_json) %>
+                <%= password_field_tag("authentication_amazon_secret", '',
+                                    :maxlength         => 50,
+                                    :placeholder       => "●●●●●●●●",


Should this be changed as well?

Fryguy · 2014-11-15T00:31:06Z

@martinpovolny I like this. I found 2 more locations that aren't using the new method, but after that I think this is good to go if @h-kataria is good with it.

miq-bot · 2014-11-16T08:27:27Z

Checked commits martinpovolny@fd3beeb .. martinpovolny@67e4c94 with rubocop 0.21.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

martinpovolny · 2014-11-16T12:04:07Z

@Fryguy : fixed
@h-kataria : please, review

Fryguy · 2014-11-16T14:20:39Z

Looks good to me. Thanks for fixing this one @martinpovolny .

EDIT: wow, autocorrect fail.

h-kataria · 2014-11-17T16:14:20Z

@martinpovolny i found an issue while testing changes in this PR, it was not introduced by changes in this PR but i feel it should be fixed, either in this PR or we can open a separate issue for that.
When i go user edit screen, it shows blank in the password & confirm password fields(no bullets). When i try to get a password for an existing record it gives me back nil even tho i have a password saved for that user in database. I think it is because Scott made some changes a while ago to user model that does not allow us to read the password of user record in db. I am thinking that maybe we can show bullets on the screen if an existing user record is being edited as password is a required field so it has to be there and show the fields with blank if a new user record is being edited. @martinpovolny @Fryguy @dclarizio Thoughts?
Other than the issue mentioned above other changes look good, verified them in UI.

martinpovolny · 2014-11-17T18:42:52Z

@h-kataria : I can fix it tomorrow.

On the question of doing this in a separate PR: I don't have a strong opinion and slightly incline to doing a separate PR for that. @dclarizio : Can you decide that, please?

p.s. Thx for the review!

dclarizio · 2014-11-17T20:43:02Z

Sure, separate PR is best.

Fryguy · 2014-11-17T21:31:04Z

Yes, separate PR is fine. I thought there was a method we could call on the model like password_encrypted or something, which would be an encrypted string if there's a password, or blank if not. We could use that to show the bullets.

HTML password fields

Fix topology view styling issues (cherry picked from commit 021a839) https://bugzilla.redhat.com/show_bug.cgi?id=1440400 https://bugzilla.redhat.com/show_bug.cgi?id=1440399

Fryguy reviewed Oct 24, 2014
View reviewed changes

Fryguy added the enhancement label Oct 24, 2014

martinpovolny force-pushed the login_password branch from ceed337 to 5420876 Compare October 29, 2014 12:13

Martin Povolny added 7 commits November 12, 2014 12:46

Cleanup credentials for RHN partial.

fd3beeb

Cleanup email password partial.

394d07c

Do not pass SMTP password to HTML form.

2038347

Do not pass DB password to HTML form.

5dbdfe3

Do not pass Amazon secrets and LDAP password to HTML form.

3f8cf61

Do not pass infra provider passwords to HTML form.

10a5d72

Add conditional placeholders to HTML password fields.

6105cd6

martinpovolny force-pushed the login_password branch from 5420876 to 6105cd6 Compare November 12, 2014 18:24

Fryguy reviewed Nov 15, 2014
View reviewed changes

Martin Povolny added 2 commits November 16, 2014 09:20

Add conditional placeholders to HTML password fields.

e030433

Revert hiding of amazon key.

67e4c94

Fryguy added a commit that referenced this pull request Nov 17, 2014

Merge pull request #926 from martinpovolny/login_password

fdd47ce

HTML password fields

Fryguy merged commit fdd47ce into ManageIQ:master Nov 17, 2014

Fryguy added this to the Sprint 16 Ending Dec 1, 2014 milestone Nov 17, 2014

martinpovolny mentioned this pull request Nov 18, 2014

Placeholder dots for passwords for user editing #1075

Closed

martinpovolny deleted the login_password branch November 28, 2017 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML password fields #926

HTML password fields #926

martinpovolny commented Oct 24, 2014

Fryguy Oct 24, 2014

Fryguy Oct 24, 2014

martinpovolny Oct 26, 2014

martinpovolny commented Oct 28, 2014

h-kataria commented Oct 28, 2014

martinpovolny commented Oct 28, 2014

Fryguy commented Oct 28, 2014

martinpovolny commented Oct 29, 2014

h-kataria commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Oct 31, 2014

Fryguy commented Oct 31, 2014

h-kataria commented Oct 31, 2014

Fryguy commented Oct 31, 2014

h-kataria commented Oct 31, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Oct 31, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Nov 12, 2014

Fryguy Nov 15, 2014

Fryguy commented Nov 15, 2014

miq-bot commented Nov 16, 2014

martinpovolny commented Nov 16, 2014

Fryguy commented Nov 16, 2014

h-kataria commented Nov 17, 2014

martinpovolny commented Nov 17, 2014

dclarizio commented Nov 17, 2014

Fryguy commented Nov 17, 2014

HTML password fields #926

HTML password fields #926

Conversation

martinpovolny commented Oct 24, 2014

Fryguy Oct 24, 2014

Choose a reason for hiding this comment

Fryguy Oct 24, 2014

Choose a reason for hiding this comment

martinpovolny Oct 26, 2014

Choose a reason for hiding this comment

martinpovolny commented Oct 28, 2014

h-kataria commented Oct 28, 2014

martinpovolny commented Oct 28, 2014

Fryguy commented Oct 28, 2014

martinpovolny commented Oct 29, 2014

h-kataria commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 29, 2014

martinpovolny commented Oct 29, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Oct 31, 2014

Fryguy commented Oct 31, 2014

h-kataria commented Oct 31, 2014

Fryguy commented Oct 31, 2014

h-kataria commented Oct 31, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Oct 31, 2014

Fryguy commented Oct 31, 2014

martinpovolny commented Nov 12, 2014

Fryguy Nov 15, 2014

Choose a reason for hiding this comment

Fryguy commented Nov 15, 2014

miq-bot commented Nov 16, 2014

martinpovolny commented Nov 16, 2014

Fryguy commented Nov 16, 2014

h-kataria commented Nov 17, 2014

martinpovolny commented Nov 17, 2014

dclarizio commented Nov 17, 2014

Fryguy commented Nov 17, 2014