processwire / processwire-issues

ProcessWire issue reports.
45 stars 2 forks source link

3.0.83 prevents saving user #432

Closed adrianbj closed 5 years ago

adrianbj commented 7 years ago

Short description of the issue

Try to save a user without changing anything. Nothing happens and there is a console error

Expected behavior

Should save

Actual behavior

Won't save

Optional: Screenshots/Links that demonstrate the issue

screen shot 2017-11-10 at 11 33 04 am

Optional: Suggestion for a possible fix

The problem seems to be because the first of the password fields is getting automatically populated which is triggering the check for a matching password confirmation value. Why is the password field being populated?

Toutouwai commented 7 years ago

Why is the password field being populated?

Chrome ignores autocomplete="off" and sees the password field as something that is okay to autofill. Probably a similar solution to what was recently added for the name field should do the trick.

The console warning is unrelated and occurs any time Chrome detects a password field on a non-HTTPS page.

adrianbj commented 7 years ago

The console warning is unrelated and occurs any time Chrome detects a password field on a non-HTTPS page.

Sorry I captured the wrong console error. This is the issue: screen shot 2017-11-10 at 12 25 53 pm

Related to these:

https://processwire.com/talk/topic/14574-image-cropping-not-working-in-backend/ https://stackoverflow.com/questions/22148080/an-invalid-form-control-with-name-is-not-focusable

Toutouwai commented 7 years ago

Ah, right. If we can stop the browser autofilling the first password field then the console error should resolve too.

ryancramerdesign commented 7 years ago

Can't say I've ever used autofill, so don't have a good environment to test it at the moment, but wondering if you can try the latest commit to see if that fixes the issue? Thanks.

matjazpotocnik commented 7 years ago

Autocomplete fix solved the issue here.

adrianbj commented 7 years ago

Thanks @ryancramerdesign although I think it might be even safer to use: pw-new-password to make sure it's unique. This would also be inline with your fix for the page name issue where you used: pw-page-name - https://github.com/processwire/processwire/commit/34d15dadae9e482a11939110277565aa38f8f1a4

ryancramerdesign commented 6 years ago

From what I understand the new-password value actually has meaning to both Chrome and Firefox, whereas pw-new-password would not. https://www.chromium.org/developers/design-documents/form-styles-that-chromium-understands

adrianbj commented 6 years ago

Thanks for the explanation Ryan - I didn't notice they were suggested values. I thought it was about being unique - sorry for misunderstanding that.

jacmaes commented 6 years ago

I'm having the same issue again on Firefox 62 and PW 3.0.111: the password confirmation field is marked as required, and thus the page cannot be saved until I refill both password fields.

jacmaes commented 5 years ago

The issue is still present on Firefox 64 and PW 3.0.123. Boy, this is very annoying…

adrianbj commented 5 years ago

@jacmaes - I can't seem to replicate this anymore. I have Firefox 65. I wonder if they recently fixed something or if there is something else going on.

jacmaes commented 5 years ago

I'm on Firefox 66 beta 9. It's still happening to me on 3.0.125. I have a few custom fields, such as first name + Soma's HiddenAdminPages module to hide select pages from certain users. Whenever I change the user's first name, or the list of hidden pages, I cannot save the page without confirming the password again:

user

matjazpotocnik commented 5 years ago

I can confirm what @jacmaes experienced. The problem is that FF does not support autocomplete="new-password" and prefill "New password" field with the current password. If you empty the "New password" field then you can save the user. What if we compare "New password" and "Confirm" fields only when the second one is filled in?

if(isset($input->$confirmKey) && strlen($input->$confirmKey) && $input->$confirmKey != $pass) {
    $this->error($this->_("Passwords do not match"));
}

Just to add some salt: if you set Minimum password length to 0, you can't change the password as it's "To common".

netcarver commented 5 years ago

@jacmaes @matjazpotocnik Thanks for the report. I'm re-opening this issue.

Toutouwai commented 5 years ago

This Firefox issue with the unwanted filling of form fields goes back a long way: http://forums.mozillazine.org/viewtopic.php?f=25&t=289077

If the FF development team haven't addressed this in 14 years you wouldn't want to hold your breath that they'll address it now. Mozilla's developer docs casually declare that there is standard for disabling autocomplete but it's not implemented in Firefox: https://developer.mozilla.org/en-US/docs/Web/Security/Securing_your_site/Turning_off_form_autocompletion

If you are defining a user management page where a user can specify a new password for another person, and therefore you want to prevent autofilling of password fields, you can use autocomplete="new-password"; however, support for this value has not been implemented on Firefox.

Some hacky workarounds that people have used in response to the problem...

Fake form inputs: https://stackoverflow.com/a/31923341/1036672 Randomised input names: https://stackoverflow.com/a/218453/1036672 Removing a readonly attribute on input focus: https://stackoverflow.com/a/37153976/1036672 Emptying input value after FF has filled it: https://stackoverflow.com/a/31658459/1036672

matjazpotocnik commented 5 years ago

Removing a readonly attribute on input focus works here, tested in Chrome and FF:

"<input placeholder='$newPassLabel' " . $this->getAttributesString() . " readonly onfocus='this.removeAttribute(\"readonly\");' /> " .

or better:

$this->attr('readonly', 'readonly');
$this->attr('onfocus', "this.removeAttribute('readonly');");

around line 148

ryancramerdesign commented 5 years ago

@matjazpotocnik Thanks, I will add that. Since the readonly attribute is removed by JS, do you think it would be better if it was also added by JS? Or would that prevent it from solving the issue? I was just thinking that InputfieldPassword is sometimes used in places outside the admin (like the front-end) and adding a readonly attribute might block the field from being usable without JS.

matjazpotocnik commented 5 years ago

Hm, I'm not sure if we should fix that in the first place, as this is a browser issue. But as Robin said, we don't know if FF devs will ever fix that, I agree we should do something to help users.

I prefer a solution without javascript, if possible and applicable in this context, see my comment https://github.com/processwire/processwire-issues/issues/432#issuecomment-466462168.

But you are right - adding something on the server side and then removing on the client side, not knowing if the client is capable of doing that, is not the best approach. From my testing, adding readonly attribute to the inputfield in InputfieldPassword.js is not enough, it's too late. What did help was adding this line at the bottom in _main.php, before ProcessWireAdminTheme.init();

var input=document.getElementById("Inputfield_pass");
if(input) {
    input.setAttribute("readonly", "readonly");
}

I'm not familiar enough with admin themes so don't know where would be an appropriate place to put that code.

But, from my point of view, it's not a problem if FF (or any other browser) autofill the password, that's users' choice. The problem is that confirmation password field is empty and thus different from auto-filled current password and that prevents the page to be saved, erroring "Passwords do not match". That's why I suggested checking for password match only if there is something entered in the confirmation password.

Edit: adding a readonly attribute and removing it on focus event is not working good in IE, you need to click twice on the password field. But adding a readonly attribute in _main.php (for AdminThemeDefault I added readonly attribute in default.php) and then removing it in InputfieldPassword.js like this somewhere at the beginning:

$input.removeAttr('readonly');

works as expected in all browsers.

Edit: using fake password input also works. I just replaced $out=''; in InputfieldPassword.module line 210 with:

$out = "<input type='password' name='fake_pass' id='fake_pass' style='display:none' />";

Edit: And while there, would it be possible to have an option to completely disable complexify addon and minimum password length?

ryancramerdesign commented 5 years ago

@matjazpotocnik I think that having it ignore the password submission when there's no confirmation present (per your earlier suggestion) seems like the cleanest way to go, so I have implemented that. Though I do think your fake-pass suggestion is a good one too, but reluctant to add an extra input to accommodate FF. Plus, since we keep fixing this and they keep changing FF to find another way to work its password in there, I'm guessing at some point FF is going to start ignoring 'display:none' inputs and we'll be back where we started. So just ignoring the input unless both pass and confirm are present seems like it might be more future proof.

And while there, would it be possible to have an option to completely disable complexify addon and minimum password length?

There are options for configuring complexify and/or changing the minlength. For minlength, you'll want to set the "minlength" attribute on InputfieldPassword, and to limit complexify's involvement you'll want to set the complexifyFactor property of the InputfieldPassword to 0. Though I'd recommend something like 0.1, as we're dealing with passwords here so not good to limit this feature too much.

matjazpotocnik commented 5 years ago

@ryancramerdesign thanks for the fix that I prefer too.

Regarding configuring complexifyFactor: you can't enter "0" or "0.0" because after saving the page, the value is set to the default at "0.7". You can set minimum password length to 0, but then new password is always "To common", regardless of the length and/or complexity. You also can't blank out the password requirements field, because after saving letter and digit are selected as this is the default.

I know this is more feature request then an issue, just wanted to inform you about this.

netcarver commented 5 years ago

@matjazpotocnik Any objection to me closing this issue now? Perhaps you could open a new issue in the requests repo to track things going forward.

matjazpotocnik commented 5 years ago

Steve, you can close the issue. It looks like I'm the only one who doesn't like this complexity factor so don't want to bother Ryan with that.

netcarver commented 5 years ago

@matjazpotocnik Ok, closing then, thank you.