Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrfelton’s picture

Status: Active » Needs review
FileSize
9.38 KB

Here is an initial stab at this. It's basically working, though could do with some cleanup and refactoring.

erikwebb’s picture

Status: Needs review » Needs work

This is a pretty far-reaching patch for what this issue is describing. It's good work, but do you think we could break this up?

mrfelton’s picture

far-reaching? how so? Break it up? Like how? It implements one piece of new functionality (force password change on first login), by adding a new ctools Item plugin.

Actually, I did slip one other fix into the mix which was the addition of the enabled_policies() function - that actually needs to be a separate bug report, as right now you are running all of the plugin code all of the time, even for policies that are disabled via the ctools export ui.

mrfelton’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

Moved the bug fix over to a new issue at #1923794: Do not run password policy rules for disabled policies. Now this patch only contains the new force plugin.

erikwebb’s picture

Status: Needs review » Needs work

Thanks for splitting the patch for me. The other functionality looks great to me and I already committed it.

A couple of notes/questions on this functionality -

  • Please remove the commented out code. Was that for debugging?
  • Should we be adding columns to the main user table? I've always seen this as typically bad practice. We should either use our own table or add it to the user's data field.
  • It looks like you've added force login-specific code to the .module as well as in a contained plugin. Is there any way we can isolate the plugin completely?
mrfelton’s picture

I'm working on a reworked version of this, where the functionality is just a condition plugin instead. Patch to follow shortly

mrfelton’s picture

Status: Needs work » Needs review
FileSize
3.18 KB

Reworked to act as a condition plugin. Now that there are multiple conditions, it relies on AND logic from #1923990: Run multiple conditions in AND mode.

mrfelton’s picture

Just to add some context, the way I'm using this is to define a new policy, that applies to certain roles (using the role condition), and that aplies to first time login (using the new first condition provided by this patch). I don't set any of the constraint options, and then turn on the expire option by setting the time to -1. That means that the password expires as soon as the user first logs into the site, and they are then forced to change it.

The problem I have now, is that the new user also receives an email telling them that their password just expired - which obviously I dont want. So, I'm thinking that the expire item could do with an option to say wether or not notifications are required.

mrfelton’s picture

Updated patch, last patch forgot to check the password history properly.

mrfelton’s picture

check slightly wrong still. This one works :)

erikwebb’s picture

Title: force password change on next login 7.x-2.x » Force password change on first login 7.x-2.x
FileSize
1.78 KB

I'm going to more generally track install time because of past issues - #1929266: Track install time of Password Policy module

I'm not able to see this code actually running. Is this a POC or were you seeing expected behavior somehow?

A few notes -

  • I think "first" may be too generic, so I'm going to rename things to the "first_login" namespace.
  • Core uses $_SERVER['REQUEST_TIME'] to set the "install_time" variable, so I've done the same.
  • Add a password_policy_get_password_history() helper function to avoid the extra user_load().
mrfelton’s picture

This is not POC, this is tested and working code. If you are not seeing it running, its most likely because of #1923990: Run multiple conditions in AND mode..

erikwebb’s picture

Status: Needs review » Needs work
FileSize
3.58 KB

I don't see anything in your patch that is actually causing a redirect or forcing the password change. Is there something missing?

I've attached my latest changes with the extra first_login.inc file.

mrfelton’s picture

Status: Needs work » Needs review

Correct. There is nothing here that causes a redirect. This simply implements a new condition plugin. The existing Expire plugin already has the code to expire a password and cause a redirect etc. So to use this, you would use this in conjunction with the Expire rule, and possibly other conditions.

In my case, I'm using the Role condition to target users of a specific role only, this condition to limit it further to only that that are on their first login (ie, those that have not changed their password at least once), and then using the Expiry plugin with an expiry time of -1 second (ie, immediately) to expire the passwod for the users that meet those conditions.

Status: Needs review » Needs work

The last submitted patch, password_policy-first_login-1920614-13.patch, failed testing.

mrfelton’s picture

Status: Needs work » Needs review
monstrfolk’s picture

erikwebb’s picture

Issue summary: View changes
Status: Needs review » Needs work
coltrane’s picture

Reading through this issue I'm a bit confused if there's an agreed upon approach between a condition for first login (#10) or a new policy item (#13) so I tested both of them to better understand.

#10 doesn't work for me, upon login a user is required to set their password but (I think because of expire_limit -1) they continuously get redirected to their edit page to change password -- seems the condition is always applying. Here were my setup steps:

  1. have logged out regular user, no passwords in password history for them
  2. new policy with first login condition
  3. default constraints
  4. Set expire limit to -1
  5. user logs in and redirected to change password -- good
  6. user changes password
  7. any attempt to visit another page is redirected back to edit form to change password

#13 doesn't work because any attempt to create/edit a policy errors with

Class 'PasswordPolicyFirstLogin' not found in /mysite/sites/all/modules/password_policy-7/includes/PasswordPolicyItem.inc on line 62
Drupa1ish’s picture

Status: Needs work » Needs review
FileSize
4.58 KB

#10 not working because the plugin first.inc is not activated, even the condition is correct. To prove this point, just for laughs, I temporary deleted the file password_policy\plugins\condition\role.inc, and magically the other plugin first.inc from #10 works perfectly. There is a conflict between those 2 plugins, but I had not time to debug in depth.

I’d rather prefered to workaround the issue, by embedding the condition from first.inc into role.inc.

Comments regarding the patch

The policy Force change password on first login works with password expiration- expire limit 1 sec ( instead of -1 like in #14)
Even they are disabled by default, in order to work you should enable both rules from admin/config/people/password_policy (Force change password on first login and Example policy or your own modified rule)

Cleanup unrelated the code from #10 force_password_change_uninstall.

Drupa1ish’s picture

Status: Needs review » Needs work
hanoii’s picture

I am not trying to add noise to this issue, but I needed to re-roll patch at #10 but without the .install bits as those were already added to #1929266: Track install time of Password Policy module as per #11. So it's basically the same as #10without that and with the variable name changed.

Drupa1ish’s picture

#22 works only if you set password expiration limit to -1 sec, in the same password policy, as per #14. @hanoii, is the same for you?