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)
| Comment | File | Size | Author |
|---|---|---|---|
| #34 | 2008210-reset-stale-accounts-34.patch | 413 bytes | banviktor |
| #30 | 2008210-reset-stale-accounts-30.patch | 1.68 KB | banviktor |
| #23 | 2008210_password_change_inactive_users_23.patch | 10.34 KB | banviktor |
Comments
Comment #1
gregglesGiven 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.
Comment #2
matt v. commented@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:
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.
Comment #3
matt v. commentedI 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?
Comment #4
gregglesMy 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%'.
Comment #5
gregglesAnother faster solution could be:
update users set pass = concat('ZZZ', sha(concat(pass, md5(rand())))) where...your conditions here.
Comment #6
matt v. commentedHere's an updated patch that incorporates the ideas above and addresses the remaining todo items mentioned in comment #2.
Comment #7
gregglesDefinitely getting there - thanks Matt!
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.
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.
Comment #8
matt v. commented@greggles,
Here's an updated patch, with the changes you pointed out.
Comment #9
gregglesLooks solid. Let's leave it at "needs review" for a bit in case anyone else has ideas?
Thanks!
Comment #10
gregglesHere'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).
Comment #11
gregglesOK, 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.
Comment #13
gregglesThe error is just because of this line using short array syntax:
Comment #14
gregglesComment #21
banviktor commentedFixed 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 resetComment #22
banviktor commentedCode style fixes
Comment #23
banviktor commentedLast one
Comment #25
gregglesThanks, banviktor!
Moving this so it can be ported to Drupal 8.
Comment #26
coltraneIs 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.
Comment #27
gregglesSeems like a good idea, coltrane.
@banviktor, want to work on that?
Comment #28
banviktor commentedYes, using 'login' makes sense, I'll upload a patch.
Comment #29
banviktor commentedComment #30
banviktor commentedUploaded 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?
Comment #31
banviktor commentedComment #33
gregglesThanks, @banviktor. Now applied to 7.x and back to be ported to 8.x.
Comment #34
banviktor commentedHere's another incremental patch to the 7.x code which fixes the --limit option of the drush command.