Password Policy should optionally ignore users managed by other modules

I wonder how we make this exception. The LDAP module allows some configurations to manage the password through Drupal, so those email notifications would be legit. Do you think we should add an exception for the LDAP module or maybe implement a hook that allows other modules to exclude users from the policies?

I think we should go the route of implementing a hook to exclude users. Other modules like SSO might need this functionality as well.

Maybe hook_password_policy_exclude_user() and simply bypass if any module returns TRUE.

With Password Policy disabled? Disabled modules won't run through cron, so I'm not sure how you'd still be seeing emails.

Cron should only look up enabled modules when it is run, regardless of whether there is a queue or not. Can you double-check and confirm this was happening?

+1 for this. We were just thinking about how to work around this by using roles too! Too funny.

Why not use user_get_authmaps() to discover which users to exclude?

I'm not sure a new hook_password_policy_exclude_user() would work well for cron runs.

@Sborsody - The problem with user_get_authmaps() I mentioned above. There are cases where an external auth module may use Drupal to manage its passwords. I'd hate to put all of those conditions into this module.

Why do you think the hook approach wouldn't work well for cron runs? Efficiency?

Sorry for the somewhat drive-by post earlier. Servers were being shut down and we were trying to power through the ticket backlog. So I missed what you wrote about the LDAP module allowing password management through Drupal in some configurations. We are not using that configuration so I wasn't thinking about it. My concern about cron was that the LDAP module only knows the user at the time of authentication, yet I should have answered my own question because there's the authmap table which the LDAP module could probably use to keep track of users.

My initial thought was that this module could provide an admin interface to select whether to ignore or include users of modules found in authmap. But I suppose the LDAP module could do the same thing for its own users (afaik it only tracks users via authmap) and just implement the hook.

For my immediate needs and use case though, I need to hack password policy to ignore users found in authmap.

I'll work to implement this feature in 7.x-2.x.

Hi,
Thank you for your work on this. Not sure if it was intended, the authmap plugin uses authname column rather than module column from the authmap table so it displays user names as "authentication systems". As a sidenote, when I upgraded to 2.x-dev, I got fatal error message about not finding PasswordPolicy class on line 160 of password_policy.module. I had to add a require_once line in password_policy_init(), which was kind of weird since you have the file listed in .info.

New patch against dev. Changes db_select query to use module column and updates variable name to reflect change. I have this implemented on an internal site. I will let you know if ldap_authentication users are successfully ignored.

I get this error while setting up a new site with password policy and ldap authentication:
Warning: array_intersect_key(): Argument #1 is not an array in password_policy_authmap_condition() (line 54 of /export/www/*****/html/sites/all/modules/contrib/password_policy/plugins/condition/authmap.inc).

This is caused because user_get_authmaps() returns 0 if there are no users in the authmap table.

Dropping this note here to remind myself to provide a patch later.

An updated patch to take #18 into consideration. Also some tests for this new authmap policy condition.

This patch makes the following nonfunctional changes:
- Change prime value from "authnames" to "authmap" for consistency with naming in rest of condition code.
- Fix two comments that were copied from role.inc and still referred to roles.
- Pluralize "Authentication systems" for consistency with "Roles".
- Correct erroneous in-line comments in condition().

Is there a dummy or example authentication module that can be used to easily test this? There's OpenID, but I'm not immediately sure how to get it to generate authmap table entries.

There's no check for the case when there is an anonymous user. Getting this error:

Notice: Undefined property: stdClass::$name in password_policy_authmap_condition() (line 53 of C:\Users\Stacey\Documents\Code\hpc\hpcicweb-drupal\sites\all\modules\contrib\password_policy\plugins\condition\authmap.inc).

Here is how I had fixed this issue:

+/**
+ * Condition callback for role condition.
+ */
+function password_policy_authmap_condition($account, $condition) {
+  if (isset($account->name)  && user_get_authmaps($account->name) != 0) ) {
+    $authmodules = array_filter($condition->config['authmodules']);
+
+    // Always enfore this policy if no authmodules were excluded.
+    if (count($authmodules) == 0) {
+      return TRUE;
+    }
+
+    // Otherwise, test if policy authmap intersect with any selected roles.
+    return !count(array_intersect_key(user_get_authmaps($account->name), $authmodules));
+  }
+  return TRUE;
+}

Sborsody, thanks for the fix. I added an automated test and made the anonymous user check more explicit, so the purpose of the condition is a bit clearer to someone reading the code. If you have a chance, it would be appreciated if you could confirm that this patch also fixes the problem.

I can confirm the patch from #24 eliminates the notice from #23.

I think this plugin makes user accounts match all password policies when they shouldn't.

In the following cases, this plugin will return TRUE:
Case 1. No authentication system is excluded from the policy
Case 2. User is anonymous
Case 3. User login isn't authenticated using an excluded authentication system

This will make the match method in PasswordPolicy.inc return TRUE so the password will be checked against the expiry policy even if the account didn't match the Roles condition.

I've uploaded patch password_policy-authmap-plugin-no-match-by-default.patch with a fix

Thanks for the report and patch. I think PasswordPolicy::match() might actually be wrong:

 /**
   * Returns whether all active conditions match.
   */
  public function match($account) {
    $does_match = FALSE;
    foreach ($this->get_items('condition') as $condition) {
      // Any condition with non-default configuration is "active"
      if ($condition->info['config'] != $condition->config) {
        $does_match = $does_match || $condition->match($account);
      }
    }
    return $does_match;
  }

That seems to be performing a boolean OR operation when it should be performing a boolean AND. That is, it should only return TRUE when all conditions match, as the function comment says.

Would it fix the problem to change match() to perform a boolean AND?

Yes that would work too. Not sure which way is best...

Opened new issue for #29 for the sake of referencing an issue when later committing: #2458069: Conditions should be ANDed