This issue occurs when attempting to register a new account using Password Policy 7.x-1.11 and PHP 5.4 when creating accounts anonymously or as an administrator. We see the following notice message:

Notice: Undefined property: stdClass::$name in password_policy_drupal_strength_constraint() (line 93 of /sites/all/modules/password_policy/plugins/constraint/drupal_strength.inc).

The problem is Password Policy is attempting to check if $password is equal to $account->name. This will not work during a new account registration, because the $account object does not have a 'name' value for anonymous users.

This is the code in question in the password_policy_drupal_strength_constraint function, starting at line 91 in /plugins/constraint/drupal_strength.inc:

  // Check if password is the same as the username.
  if ($password !== '' && strtolower($password) === strtolower($account->name)) {
    // Passwords the same as username are always very weak.
    $strength = 5;
  }

I'm not entirely clear why the module is using $account at this point in the process. Even if it's an existing user updating his or her password, this check is to see if $password matches the previously saved username. Shouldn't this code be checking if the user is entering the same password and username at the same time?

A quick patch to at least ensure $password is compared to the current username would use $_POST:

  // Check if password is the same as the username.
  if ($password !== '' && strtolower($password) === strtolower($_POST['name'])) {
    // Passwords the same as username are always very weak.
    $strength = 5;
  }

However the issue is this won't work with the Ajax updates. The right way to do this is get the value of the username field in an Ajax callback, then compare to $password. Unfortunately I don't have time to write a patch for this right now. Also I haven't checked to see if the approach is different in 2.x-dev, so maybe this has already been resolved.

At a minimum I wanted to post in case anyone else is having the same issue. Thanks.

CommentFileSizeAuthor
#20 password_policy-7.x-2.x-resolve_account_name_error-2478199-20.patch1.89 KBsgdev
#15 password_policy-7.x-2.x-check_username_from_post-2478199-15.patch3.9 KBaohrvetpv
#13 password_policy-7.x-2.x-check_username_from_post-2478199-13.patch3.91 KBaohrvetpv
#11 password_policy-7.x-2.x-check_username_from_post-2478199-11.patch4.01 KBaohrvetpv
#10 password_policy-7.x-2.x-check_username_from_post-2478199-10.patch3.98 KBaohrvetpv
#4 password_policy-7.x-2.x-check_username_from_post-2478199-4.patch2.57 KBaohrvetpv

Comments

aohrvetpv’s picture

aohrvetpv commented
Version: 7.x-1.11 » 7.x-2.x-dev

Thanks for reporting this. I think you must've meant to set this to a 7.x-2.x release, because this constraint does not exist in 7.x-1.x. The bug also exists in 7.x-2.x-dev.

aohrvetpv’s picture

aohrvetpv commented

After each keyUp on the password field, a POST request is made to password_policy/check with the value of the password input as a parameter.

My initial thought is instead we should set the values of all inputs, not just the password input, as parameters. Then the constraint could obtain the username using something like $_POST['name'], as you suggest. Do you see any problem with this approach? I do not understand your comment about not working with AJAX updates.

We need a general approach that works for inputs beyond the username, because there are other inputs against which the password might need to be checked.

aohrvetpv’s picture

aohrvetpv commented

username constraint also has this problem.

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-2.x-check_username_from_post-2478199-4.patch2.57 KB

Patch attempts to fix the specific bug with username. Please review/test.

The general problem of allowing the password to be checked against arbitrary inputs as mentioned in #2 can be solved separately.

The username constraint was avoiding this problem by just allowing passwords that contained the username if the user was registering a new account. This seems like a bad implementation because it allowed weak passwords containing the username, despite administrator intent to disallow them.

aohrvetpv’s picture

aohrvetpv commented

I do not understand your comment about not working with AJAX updates.

I think I understand now what you meant by this. $_POST['name'] is available after form submission, but not with requests to password_policy/check. I solved this (I believe) in patch in #4 by modifying password_policy.js to set name as parameter to the password check.

sgdev’s picture

sgdev commented

I'm sorry, not sure what I was thinking... yes you're right this is 7.x-2.x.

Regarding Ajax, I was referring to the fact that $password == username should be checked and presented to the user as part of entering a password.

When I modified the code using $_POST, I received an error after I submitted the form (as I expected), but not when entering the password values. The UI should notify the user of the error prior to submitting since it does so for other rules. Therefore I assumed there would need to be an update to Ajax and/or JavaScript to accomplish this.

Let me test the patch and get back to you. Thanks for your quick response!

sgdev’s picture

sgdev commented

Couple of questions about the patch. You've added this logic:

  if (isset($_POST['name'])) {
    $username = $_POST['name'];
  }
  else {
    $username = $account->name;
  }

First, are there any complications for sites that use modules like Email Registration and Login Toboggan for user registration, where users can enter a username or email address for login purposes? I don't think there would be, but just mentioning in case there are potential issues.

Second, I'm trying to understand in what situation the "else" applies. If $_POST['name'] is not set, either there will be an error (because username is required), or another Password Policy error supersedes this one (if $_POST['name'] is not set, the only possible option would be that password is blank too).

Wouldn't it make more sense to check if $_POST['name'] is set, otherwise skip the constraint entirely? Maybe there's a different case that I'm missing that $account would apply.

aohrvetpv’s picture

aohrvetpv commented

Second, I'm trying to understand in what situation the "else" applies. If $_POST['name'] is not set, either there will be an error (because username is required), or another Password Policy error supersedes this one (if $_POST['name'] is not set, the only possible option would be that password is blank too).

Wouldn't it make more sense to check if $_POST['name'] is set, otherwise skip the constraint entirely? Maybe there's a different case that I'm missing that $account would apply.

The else block is meant for configurations where the username field is not on the same form as the password field. The Password Tab module moves the password fields to a new tab (and a separate form from the user edit form) which does not have the username field. In that case we need to get the username from the user object.

I am concerned with supporting Password Tab in particular because the password tab module was built in to Password Policy 7.x-1.x, but moved to a separate project in 7.x-2.x. Administrators who migrate from 7.x-1.x may install Password Tab to keep the same functionality.

This should probably be documented in the code.

aohrvetpv’s picture

aohrvetpv commented

First, are there any complications for sites that use modules like Email Registration and Login Toboggan for user registration, where users can enter a username or email address for login purposes? I don't think there would be, but just mentioning in case there are potential issues.

I will check. My recollection with Login Toboggan is username and e-mail address are still separate fields on the user edit form and user registration form, and are stored separately as normal, but the module allows the user to log in using either. So my guess is it will work fine.

Thanks for reviewing the patch.

aohrvetpv’s picture

aohrvetpv commented
Status: Active » Needs review
StatusFileSize
new password_policy-7.x-2.x-check_username_from_post-2478199-10.patch3.98 KB

Changes versus #4:
- Document purpose of else block.
- Trim username input value. (Otherwise leading/trailing spaces may cause comparison to fail.)
- Pass POST parameter through rawurldecode(). (This is consistent with password_policy_ajax_check().)
- Ignore Coder security warning about using $_POST.

There is some code duplication between the "username" and "drupal_strength" constraints, but I could not immediately think of a good way to remove it (without introducing a dependency between the constraints or moving plugin-specific code to the base module).

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-2.x-check_username_from_post-2478199-11.patch4.01 KB

Update per #2478367: Username not escaped for special regular expression characters in username constraint.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Needs work

Trimming is unnecessary since validation will fail if the username has leading or trailing spaces.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review
StatusFileSize
new password_policy-7.x-2.x-check_username_from_post-2478199-13.patch3.91 KB

Change to not trim username.

It is actually valid for a Drupal username to begin or end with spaces (such usernames can occur when using an external authentication system), but validation of the default user forms fail when there are leading or trailing spaces. Not worrying about this edge case for now. It is debatably correct to not trim usernames that begin or end spaces, anyway.

aohrvetpv’s picture

aohrvetpv commented
Related issues: +#1815660: Implement simple integer-based constraint for overall "strength", +#2479115: Add test methods for username constraint

Need tests to ensure no regressions.

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-7.x-2.x-check_username_from_post-2478199-15.patch3.9 KB

Minor JavaScript change per JSLint.

  • AohRveTPV committed 9fb9fde on 7.x-2.x 
    Issue #2478199 by AohRveTPV: Use current username for Username and...
aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

sgdev’s picture

sgdev commented
Status: Closed (fixed) » Needs work

Well, doesn't look like this issue is fully fixed... I'm setting it to needs work, but feel free to create as a new issue if you prefer.

In our logs we're seeing errors due to some of the new code in this patch, specifically the $account->name in _password_policy_username_get_username():

Notice: Undefined property: stdClass::$name in _password_policy_username_get_username() (line 57 of /sites/all/modules/password_policy/plugins/constraint/username.inc).

This is occurring on a registration page with user anonymous. Looks like this is getting triggered when a registration form is submitted with validation errors.

sgdev’s picture

sgdev commented
StatusFileSize
new password_policy-7.x-2.x-resolve_account_name_error-2478199-20.patch1.89 KB

I just looked at this more closely and now I see the issue.

The error is triggered by a user entering a password first before a username. In this case, if (isset($_POST['name'])) { on line 47 is going to return false, which means it uses else instead. This causes an attempt to retrieve account name, which is empty.

Here's a patch that seems to work... although not sure of all the use cases where this would apply. Let me know your thoughts.

sgdev’s picture

sgdev commented
Status: Needs work » Needs review

Setting to review.

  • AohRveTPV committed a5ddc1a on 7.x-2.x authored by ron_s 
    Issue #2478199 by ron_s: Eliminate PHP notice when password entered...
aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.