Add token support to blacklist constraint

A blacklist constraint was implemented in #2134997: Create password blacklist. However, it does not yet have token support.

Patch adds tokens support to blacklist constraint. Currently requires first applying the following patch from #2466301: Option to disallow passwords containing blacklisted passwords:
https://www.drupal.org/files/issues/password_policy-7.x-2.x-blacklist_ma...

Lacks tests.

Add tests.

Still currently requires first applying patch from other issue.

- Remove trailing space.
- Update to apply over latest patch in #2466301: Option to disallow passwords containing blacklisted passwords.

1. The text in the vertical tab does not indicate whether "Also disallow passwords containing blacklisted passwords." has been checked. Until it is, this test does not function.
2. While "Password cannot match certain disallowed passwords." is accurate, it is meaningless to my users. It would be nice to have some way to indicate to the users what things they may not use.

Otherwise, this seems to work fine and takes care of #2472491: Add constraint for custom user fields (e.g. First Name, Last Name).

Thanks for reviewing the patch.

While "Password cannot match certain disallowed passwords." is accurate, it is meaningless to my users. It would be nice to have some way to indicate to the users what things they may not use.

It would be problematic to show the list of disallowed passwords in some cases. For instance, an administrator could blacklist a list of the 1000 most common passwords. If they were listed directly on the page, it would probably dominate the page and also not be very helpful to the user. If listed indirectly (e.g., via a link), another problem is the list could contain profanity, which might be undesirable for some sites.

Another approach would be to only indicate the disallowed substring that the user has entered. For instance, if they enter "password123", the message could say "Password cannot contain 'password'" or similar. A problem with this though is it could indicate to a malicious observer part of the password the user thought they were entering secretly. If that partial password is used in a password on another system, that password may become compromised.

So I propose:
- Indicate only the blacklisted passwords that the user has entered.
- Have this indication be an option which is off by default. (The default is more secure.)

Actually, here I was not considering the vertical tab text. I am more concerned with the display on password change pages. Even if there was a drupal_alter() that my module could react to, that would take care of it. I could return "You may not include your first or last name or your email address" for the "Password Requirements" text.

BTW, 7.x-2.x is awesome!

Even if there was a drupal_alter() that my module could react to, that would take care of it. I could return "You may not include your first or last name or your email address" for the "Password Requirements" text.

The message should be translatable, so could a translation be used for that?

We could add an option to display all blacklisted passwords to the user, but the tokens would not be meaningful to the user unless they were mapped to human-readable descriptions. So maybe we should just make that message editable through the UI.

IIRC, this patch may not work for user registration. Need to test.

I was thinking that before the text goes on a password set / change page (see attached), it could be passed through drupal_alter() with the constraint name and my custom module can react to it. You can translate it after, or require me to translate. It's just a matter of making it mean something to my end-users who are going to have to be changing the passwords.

The text on the admin page is fine for me - no need to change it, except to show that it has been turned on, that is that the "Also disallow passwords containing blacklisted passwords" option has been checked.

I was thinking that before the text goes on a password set / change page (see attached), it could be passed through drupal_alter() with the constraint name and my custom module can react to it. You can translate it after, or require me to translate. It's just a matter of making it mean something to my end-users who are going to have to be changing the passwords.

Makes sense. I wanted to make sure there was not already a good way to do it. I suppose t() is more properly used for language translations than overriding a string. I wonder if an existing function like hook_ctools_plugin_post_alter() could be used. That is probably not intuitive, though.

This seems to work:

function my_module_ctools_plugin_post_alter(&$plugin, &$info) {
  if ($plugin['module'] == 'password_policy'
      && $plugin['name'] == 'blacklist') {
    $plugin['message'] = t('my custom message');
  }   
}

Even if this fully works, it still might be worth adding a drupal_alter() to make this more straightforward.

The text on the admin page is fine for me - no need to change it, except to show that it has been turned on, that is that the "Also disallow passwords containing blacklisted passwords" option has been checked.

We should do that. Opened a separate issue for this as I think it is separable from adding token support to the blacklist constraint:
#2497701: Indicate in settings summary when blacklist "contains" setting enabled

Thank you, AohRveTPV. As you can see from the attached image, it seems to work just fine for me.

Setting to "needs review" because I see nothing amiss in this code. I will even present it to a Drupal group on Monday to begin making it a standard in our company.

Oops, one little issue I just found:

I include "[user:mail]" in the list and it works fine, but the dynamic message (jScript, I assume) incorrectly checks the user who is making the change (as in an admin).

To better explain, I have a testing user account (user/4) for myself. On that account, I use my personal email address, e.g. "nanwich@example.com". But I am using user/1 to make the change, and the email address for that account is my company email, e.g. "nancydru@mycompany.org". The password I am trying to set (for user/4) is "NancyDru@mycompany.org#1". The JS message is giving my custom error message. But the module does allow me to save the password, so the actual check is correct.

Since I, as an admin, will rarely be changing other people's password, this is just a very minor annoyance to me. I am still going to go ahead and promote this version at my company.

I include "[user:mail]" in the list and it works fine, but the dynamic message (jScript, I assume) incorrectly checks the user who is making the change (as in an admin).

I'm pretty sure this is a broader issue that affects some other constraints too, like the username constraint. I think the culprit is this code in the AJAX callback:

  // Using this user is not always going to work.
  global $user;
  $account = $user;
  password_policy_user_load(array($account->uid => $account));

Then later in the same function:

  $errors = $errors + $policy->check($password, $account);

At least the author flagged it as problematic. I will open a new issue for this bug.

Opened new bug report for the issue described in #16 and #17: #2497923: Wrong user used when administrator changing another user's password

Please correct if I got something wrong.

I suspect you are correct. I'll test the patch tomorrow.

All the other stuff aside, the token support seems to work great. Need someone else to RTBC.

From #2497701-22: Indicate in settings summary when blacklist "contains" setting enabled, add summary text that indicates when this is enabled.

Need to test that this works when the token value does not exist. For instance, [user:name] does not exist on the registration form.

1. Blacklisted passwords: [user:mail]
2. Use Tokens: checked
3. Who can register accounts?: Visitors
4. Log out.
5. Browse to "Create new account".
6. Enter e-mail address.
7. Enter e-mail address as password. <-- Causes undefined property PHP errors as AJAX checks performed.
8. Press "Create new account". <-- Causes PHP error and password allowed in violation of constraint.

This problem is not specific to the [user:mail] token. Probably also affects user fields.

Is it an issue for PP or Tokens? And have you checked empty values? Or invalid token names?

I'll try to do some of this today.

Is it an issue for PP or Tokens? And have you checked empty values? Or invalid token names?

Password Policy loads the user object and passes it to the blacklist constraint, but when it does so for user registration, I suspect the e-mail address has not yet been saved into the user object. So the token replacement tries to access the mail attribute of the user object, but it is undefined.

The tokens solution works, however the error message does not provide enough information for users to know WHICH values are prohibited -this is very problematic for fields like first or last name. Would be good to display the Field Title for the tokens that are blacklisted.

While "Password cannot match certain disallowed passwords." is accurate, it is meaningless to my users. It would be nice to have some way to indicate to the users what things they may not use.

I wholeheartedly agree. Further I find that statement inaccurate as it is a string within the password that is not allowed, not necessarily the entire password.

I've added patch to https://www.drupal.org/node/2472491#comment-10446695 explicitly for configuration of user field constraint. This new constraint clearly indicates any user fields prohibited by Password Policy and lists them by field label rather than a blanket 'blacklist' of 'certain disallowed passwords' which confuses users.

I would strongly suggest this in addition to any blackslist+token functionality as they have different purposes. While tokens provides a workaround, it does not inform the user of the field that is not allowed which may be a very obvious field like their own name. This is a natural extension of the module as it already has an explicit constraint for username, which while also configurable via token, makes more sense as its own constraint.