If passwords are compromised (whether on the drupal site or elsewhere) it would be nice to know that a stale account in the Drupal site has a invalid password.

* If the Drupal site is compromised, the user's real password is not lost
* If another site is compromised, the stale account credentials cannot be used to login to the Drupal site

The pain inflicted on users by this feature is minimal: they will have to use the lost password feature. If the account is sufficiently old (e.g. 1+ years) then it is likely that the user forgot their password anyway.

So, how about a drush command that:
* Finds accounts that have not been accessed since X days ago - X is an input but would default to 2 years
* Gets a 30 character password from user_password
* Assigns it to the user and saves the account
* Optionally sends a configurable mail to the user letting them know about the policy (so they won't be surprised that their password was changed)

Comments

greggles’s picture

Given that passwords can take certain formats where the first 3 characters have special meaning, it may also be nice to do accept an argument for the contents of the first 3 characters of the password and then do the update on any accounts that match that.

Also, given that this could take a long time, it might make sense to do this for accounts in hunks.

matt v.’s picture

Status: Active » Needs work
StatusFileSize
new2.03 KB

@greggles,

The attached patch is my initial pass at something resembling what you outlined. I implemented the queuing inside a hook_cron implementation, before I noticed that you'd suggested a Drush command. That shouldn't be too difficult to change though.

Here are remaining issues I'm aware of:

@todo Move the queuing to a Drush command, instead of hook_cron.
@todo Add a form to set the paranoia_access_threshold variable.
@todo Track updated accounts, to avoid re-updating the same accounts.

I'm looking for suggestions on the last item. I'm wondering if you can elaborate on what you mentioned about "...passwords can take certain formats where the first 3 characters have special meaning." I haven't been able to find documentation about that detail. I'm wondering if I might be able to exploit that fact, to provide light-weight tracking of which accounts have had their passwords reset by Paranoia.

Alternatively, I'm wondering if this might not be a better fit in the Password Policy module, since it already has a table for tracking past hashes. Another option would be to have this functionality as an entirely separate module that has Password Policy as a dependency.

matt v.’s picture

I forgot about the email notification. My initial thought for that would be to add Rules integration. Would that suffice or would you prefer the email get handled by Paranoia directly?

greggles’s picture

My thought is that it would be in this module (or at least easy to achieve, like if there were an option to print a list of emails that got changed so the list could be imported to mailchimp) just to keep everything together. That said, I'd be fine if the initial version of this code didn't send emails and it became a followup issue.

It seems like a definite improvement to not re-hash the same users. That would be a performance killer on a large site. You could either have another table to track who has had this happen to them (and what their timestamp was at that time, I guess, so you know if/when to hit them again) OR set the hash to something like ZZZ and then a hash which Drupal's login process should never allow to succeed. That way you know not to re-hash anyone whose password is like 'ZZZ%'.

greggles’s picture

Another faster solution could be:

update users set pass = concat('ZZZ', sha(concat(pass, md5(rand())))) where...your conditions here.

matt v.’s picture

Status: Needs work » Needs review
StatusFileSize
new7.09 KB

Here's an updated patch that incorporates the ideas above and addresses the remaining todo items mentioned in comment #2.

greggles’s picture

Status: Needs review » Needs work

Definitely getting there - thanks Matt!

+function hook_permission() {
+  return array(
+    'administer paranoia' => array(
+      'title' => t('Administer paranoia'),
+      'description' => t('Perform administration tasks for the Paranoia module.'),
+    ),
+  );
+}

This permission seems like it should have 'restrict access' => TRUE. Not because it presents a real risk, more because it's possible for someone to be a nuisance.

+function paranoia_mail($key, &$message, $params) {
+  $account = user_load($params['uid']);
+
+  $options = array(
+    'langcode' => $message['language']->language,
+  );

Maybe move those inside the case statement since at least the user_load is expensive and not necessary in 99% of the calls to hook_mail.

matt v.’s picture

Status: Needs work » Needs review
StatusFileSize
new7.13 KB

@greggles,

Here's an updated patch, with the changes you pointed out.

greggles’s picture

Looks solid. Let's leave it at "needs review" for a bit in case anyone else has ideas?

Thanks!

greggles’s picture

Issue summary: View changes
StatusFileSize
new6.99 KB

Here's a re-roll and slight cleanup (added comment to explain the ZZZ and inlined some temp variables that were used once in the mail function).

greggles’s picture

StatusFileSize
new9.28 KB
new7.22 KB

OK, here's a patch and an interdiff of what I changed since #10.
* Added some usage docs to the README.txt
* Cleaned up a few random things.
* Renamed the function hook_permission which wasn't firing :)
* Reduced or removed some comments that seemed unnecessary or unnecessarily long.

Status: Needs review » Needs work

The last submitted patch, 11: 2008210_password_change_inactive_users_11.patch, failed testing.

greggles’s picture

The error is just because of this line using short array syntax:

+  watchdog('paranoia', 'Queued @count users to have their password reset', ['@count' => $count]);	
greggles’s picture

Status: Needs work » Needs review

The last submitted patch, 2: access_threshold-2008210-2.patch, failed testing.

The last submitted patch, 2: access_threshold-2008210-2.patch, failed testing.

The last submitted patch, 6: access_threshold-2008210-6.patch, failed testing.

The last submitted patch, 6: access_threshold-2008210-6.patch, failed testing.

The last submitted patch, 8: access_threshold-2008210-8.patch, failed testing.

The last submitted patch, 8: access_threshold-2008210-8.patch, failed testing.

banviktor’s picture

StatusFileSize
new9.35 KB
new3.81 KB

Fixed the short array syntax, fixed the drush function name so that it fires and inconsistencies in naming. Added a simple output to the drush command (same as the watchdog message):
Queued @count users to have their password reset

banviktor’s picture

Code style fixes

banviktor’s picture

  • greggles committed 65ceb9f on 7.x-1.x authored by Matt V.
    Issue #2008210 by banviktor, Matt V., greggles: Create a drush command...
greggles’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thanks, banviktor!

Moving this so it can be ported to Drupal 8.

coltrane’s picture

Is there a reason the user 'access' property was used instead of 'login'? user 'access' is updated as part of session write while 'login' is updated on login events which seems more accurate for the purpose.

greggles’s picture

Seems like a good idea, coltrane.

@banviktor, want to work on that?

banviktor’s picture

Yes, using 'login' makes sense, I'll upload a patch.

banviktor’s picture

Assigned: Unassigned » banviktor
Status: Patch (to be ported) » Needs work
banviktor’s picture

StatusFileSize
new1.68 KB

Uploaded incremental patch. This one requires login, access and created to be old enough to consider an account stale. Also added a drush option to limit the number of items to queue.
I know this is not exactly what the instructions were, but I think we are trying to queue only those who have definitely not been active so that it's not annoying active users. Anything beyond that resembles more of a password expiration functionality which is not paranoia's concern (yet).
Thoughts?

banviktor’s picture

Assigned: banviktor » Unassigned
Status: Needs work » Needs review

  • greggles committed 17893bb on 7.x-1.x authored by banviktor
    Issue #2008210 by banviktor: Create a drush command to change passwords...
greggles’s picture

Status: Needs review » Patch (to be ported)

Thanks, @banviktor. Now applied to 7.x and back to be ported to 8.x.

banviktor’s picture

StatusFileSize
new413 bytes

Here's another incremental patch to the 7.x code which fixes the --limit option of the drush command.

  • banviktor committed 7a8a63e on 7.x-1.x
    Issue #2008210 by banviktor: Create a drush command to change passwords...