Can't edit user profile because password policy validates even when password unchanged

Just a really minor tweak so that the patch will work with the -p1 format of patch, which is the Drupal standard (and makes it compatible with composer)

I think the status should be "Needs review". "Patch (to be ported)" means "The patch has been successfully committed to a branch of the project, and still needs to be committed to another, but the current patch doesn't apply to the target branch and needs to be modified in order to do so."

Not sure if this issue should actually be a duplicate of #2786315: Allow users to bypass validation when editing another user, and correct permission is set.. I think that approach essentially adds a permission around password policy enforcement.

The patch in #3 breaks the validation flow, since _password_policy_constraints_validate contains some validation that do not directly relate to the constraints but does some validation on the field_password_expiration field...

  $expiration = $form_state->getValue('field_password_expiration');
  if (!is_null($expiration) && $expiration['value'] === FALSE) {
    $form_state->setValue('field_password_expiration', ['value' => 0]);
  }

I've submitted a new patch, the condition is right below another scenario where password validation should also be skipped. In the condition I'm just checking that is the edit user form, and that the password input is empty, same as core does so you can change any other value without having to deal with password validation.

Tested #8. Patch applies cleanly and fixes the problem of adminstrator accounts unable to change roles for other accounts.

But, there are some points mentioned in this issue regarding $force_failure, which I think relates to this issue . Attached the issue as related.

Not changing status, I think it needs more eyes.
Thanks!

Edited title for slightly more accuracy.

Without something like this patch, it's quite impossible to edit user accounts (my own or anyone else's) without also changing the password every time.

When I tested the patch in #8, though, it made it so when I edited my or another's account, the constraints didn't seem to apply; on the edit form, instead of the constraints under the password fields, I just saw:

There are no constraints for the selected user roles

And I could change the password to something that didn't meet the required restraints.

So setting this back to needs work.

Subscribe

Moved #8 condition to bottom of the validate function, and moved the existing logic to elseif.
Kindly review the patch.

Modified the patch.
Included the patch fromPassword policy module issue with saving constraint to resolve the test error in the current patch #13

The patch in 15 no longer applies because the second patch it included has been committed. The patch from #13 works just fine, and now appears to pass automated testing.

Looks patch #13 passes fine for a couple of items, but not PHP 7 & MySQL 5.5, D8.7 49 pass, 17 fail. Drupal 8.7.x needs to be tested again and review of the failings to see what's up with that one.

Marking critical as a note to get this in the next release.

Just tested #13 against my 8.7.7 install - seems to work great!

I have tested this patch on 8.7.11 environment and seems to install properly.

Can this be set to "Reviewed & tested by the community" now? (#21, #22)

I'd love to get this in a new release. very soon.

Minor grammatical issue with comment in #13, will provide new patch shortly.

I'm unable to reproduce the problem in 8.x-3.x-dev. Could someone please provide steps to reproduce it?

Here's what I did:
1. Created user 'foo' with password 'foo'.
2. Enabled policy for role 'authenticated user' with password length constraint at least 6 characters.
3. Logged in as user 'foo'.
4. Changed email address for 'foo' by entering the new email address and the current password. The changes were saved and there was no password policy validation failure, despite the password not meeting the constraint and "Password" & "Confirm password" being empty.

So I reproduced it very quick just watch. I apologiese for my bad english pronouncing. schould talk more often in that language and not just translate ui and read text but in my opinion you will be able to follow me. The Video renders right now. Klick the link later on just to see how you can reproduce the behaviour tahts devenetively wrong by now.

https://www.youtube.com/watch?v=rjfRzFAS1co

There you go

In short When an administrator tries to add a role to a usert passwort policy forces the administrator to change the password for the user the role is added, too because otherwise the user edit form could not be saved.

This behavieor forces the user on the other hand to use a password reset link every time a new role was given to teh user right now.

For reproducing steps click the link above and watch the video

Thx.

Thanks, Joachim, for the video.

The video shows that password policy validation fails when a role is changed for a user and the user is saved without entering a password.

I spent some time tracing through _password_policy_constraints_validate() to understand why that is happening. I believe the behavior is by design. If you add a role for a user, the current password of the user may not satisfy the password policies that apply to the new role.

The code causing the failed validation is this:

  if ($roles != $original_roles && $form_state->getValue('pass') == '' && !empty($applicable_policies)) {
    // New role has been added and applicable policies are available.
    $force_failure = TRUE;
  }

To ensure the user's password satisfies the password policies applying to the new role, a password must be supplied.

I think the current behavior is problematic in the case that an administrator is adding a role to a user. The administrator probably would not know the user's password nor would they want to change it. Perhaps the ideal behavior would be to force the user to change their password on next login?

In 7.x-1.x and 7.x-2.x, I believe roles can be changed and the password is not required to satisfy the new policies for those roles. This is arguably wrong (insecure) behavior.

So, I think the current behavior is by design but problematic, and #13 does not solve the problem. #13 would allow a user's password to not satisfy password policies that apply to the user.

As an administrator I need to be able to change someone's roles without entering a password or having to change the user's password. In fact I should be able to change anything except the password without entering a password. Users should be able to save changes to custom fields without providing their password. Those are the use cases that triggered this issue.

While the concern raised in #29 is valid for some use cases, the current behavior doesn't really meet that condition. If I want to enforce a rule addition with a role change I should force the user to change the password themselves. Since passwords are hashed for storage there is no way to validate whether or not their existing password passes the rules of that role. They may very well pass and I should allow them to keep their password, but you cannot check. It might be a good feature request to have this module allow administrators to force a password change when adding roles that include additional rules, but that's a different change than the one discussed here.

As both the user and as the administrator I should be able to update fields outside the security protections of core without password validation impacting the change, and that's what this was designed to address.

I've updated a revised patch, which is just a single character change from #13 to address the grammar concern raised in #25 (at least that's the only one I spotted).

Agree mostly if not completely with #30.

I will try to reproduce the problem with changing custom fields causing failed validation. Agreed that an administrator or the user themselves should be able to do that without doing anything with the password.

It might be a good feature request to have this module allow administrators to force a password change when adding roles that include additional rules, but that's a different change than the one discussed here.

Yes, I think that is the more secure behavior. A smart way to do it may be to silently validate the user's password versus password policies the next time they log in. If the password does not satisfy the password policy, only then are they forced to change it.

A stopgap would be to at least warn the administrator when they change a user's roles that the user's new password may not satisfy the new password policies. If the administrator is concerned about it, they could force the user to change their password. I can try to add a warning message to the patch if that seems desirable.

I've updated a revised patch, which is just a single character change from #13 to address the grammar concern raised in #25 (at least that's the only one I spotted).

Yep, that's the one, thanks.

I agree adding features to support administrators handling role changes would be improvements to the module, but it seems reasonable to handle those as a separate issue from this bug.

As noted in #11:

Without something like this patch, it's quite impossible to edit user accounts (my own or anyone else's) without also changing the password every time.

That makes the module essentially unusable unpatched for sites with user editable fields on the User entity. Lots of folks don't like using patched module in production.

Working to commit #3064853: Refactor _password_policy_constraints_validate(). , then we can commit this on top of it.

Would like to commit this on top of #3064853: Refactor _password_policy_constraints_validate(). . Any review/testing to speed along committing the changes in that issue would be appreciated.

Need to update #30 to apply now that commit made for #3064853: Refactor _password_policy_constraints_validate(). .

I've re-rolled the patch for the latest on 8.x-3.x. As for the grammar error, I've put it back to 'a user', instead of 'an user' per this article

Patch in #37 worked as expected

I think this should be set to RTBC

I have tested the patch in #37 and it appears to fix the issue being faced here.

Confirming #37 is working. Would be great to get this committed soon.

Any Updates here?

I'll commit/push #37 shortly. Sorry for the delay.

As discussed in #28-#33, a downside of this fix is that an administrator could change a user's role and the user's password will not meet the password constraints for the new role. I'll open a new issue to address this problem, as suggested in #33.

Just since this is critical, I wanted to bump the issue; I'm guessing it should be marked 'Fixed' now?

@AohRveTPV have you added the follow-up issue? I've applied patch #37, but if I want (as an admin), to edit a user's profile, I get the error even though I haven't touched the password field.

I also want to mention regarding this issue...autocomplete in a user's browser can cause the field to be auto-populated by the browser, potentially meaning this issue is not fully fixed by patch #37.

I'm working on 8.x-3.0-beta1. And I notice that the cancel account validation has the same issue as saving the user changes. I'd imagine it's the same logic, just on the cancel account action. Should I create a new issue for this or should this be considered a continuation of this ticket as it's the same issue happening with another validation. It may get more traction since there are already eyes on this issue for the same problem.

Meant to mark as fixed. Thanks, geerlingguy.

I'm working on 8.x-3.0-beta1. And I notice that the cancel account validation has the same issue as saving the user changes. I'd imagine it's the same logic, just on the cancel account action. Should I create a new issue for this or should this be considered a continuation of this ticket as it's the same issue happening with another validation. It may get more traction since there are already eyes on this issue for the same problem.

I'd prefer a new issue since there is already a commit for this issue. Fewer eyes as you say, but it can be hard to follow a long issue thread with multiple commits. We'll get a fix for the cancel issue committed.

I've applied patch #37, but if I want (as an admin), to edit a user's profile, I get the error even though I haven't touched the password field.

It should be possible to edit a user's profile as admin with #37 applied. Which field are you trying to edit? Perhaps open a new issue and reference this one?

I also want to mention regarding this issue...autocomplete in a user's browser can cause the field to be auto-populated by the browser, potentially meaning this issue is not fully fixed by patch #37.

Perhaps open a new issue for this problem too so we can focus on it?

patch #37 does not work in combination with a module like form_mode_manager. If you have different form modes
$form['#form_id'] == 'user_form'
will not work.
Maybe something like in_array('user_form', $form['#theme']) would work better?

3.0beta1 was not working as per above patch but latest 3.0 version seems to work as described thus no need for the patch

Thanks! I've got a distribution that locks me to the 3.0-beta1 so I can't update the module yet. The patch in #37 works great!