The query used to delete users in logintoboggan_cron() is incorrect, and will delete ALL users who have not yet clicked the activation link.
The existing query uses $user->access (which is 0 for a new user) in its query instead of user->created. Since user->access is 0, and the query decides to delete all users whose role is the pre-validated role where user->access < the time calculated, all users unlucky enough to get caught by cron will be deleted.
The attached patch changes to use $user->created for the query instead of $user->access.
In honor of my user who was deleted 2 seconds after her account was created :-)
Comment | File | Size | Author |
---|---|---|---|
#6 | lt_fix_user_purge.patch | 11.35 KB | hunmonk |
logintoboggan_user_deletion_error.patch | 931 bytes | rfay |
Comments
Comment #1
hunmonk CreditAttribution: hunmonk commentednot sure this is the fix that we want. honestly this was poorly thought out from the get go. perhaps we should store the user's pre-auth status in our own database table, as follows:
this is more stable because we only mark the user as in the pre-auth role when they register. this avoids the rather unpleasant situation where a user admin (by accident or on purpose as a temporary reduction of permissions) puts the user back into the pre-auth role manually.
or, we could just make it that once the user is removed from the pre-auth role initially, the code prevents admins from being able to accidentally add that role back to the user.
thoughts?
Comment #2
rfayI agree that using an arbitrary role as a marker for users to be deleted is a fragile approach, and that the one you outline is a step forward.
I probably recommend either committing this simple patch or temporarily removing the delete functionality, since this is a critical bug, arbitrarily deleting innocent new users. (ALL new users who don't click the link before the next cron.)
Comment #3
hunmonk CreditAttribution: hunmonk commentedthis is only the case if the 'Immediate login' feature is disabled, otherwise access time is updated on registration and this issue is not a problem. i'm guessing that most people have immediate login enabled, thus it took awhile for somebody to catch this.
this bug has been around since the initial release of 6.x. i will not rush out an incomplete fix, i want the right fix. so do you favor the database table approach, or the disabling of the pre-auth role approach?
my opinion is that the second approach will be the cleanest for 6.x, and this could be revamped in 7.x if need be, along with accompanying new database tables.
Comment #4
rfayI think you're asking if I favor
1. Implementing a new table listing users that have not yet been validated and managing automatic deletion of users using that table.
or
2. Disabling the automatic deletion of users for this version of the module.
You're the maintainer, but I think if I were you I would do #2, since it has essentially no stability issues. A quick update to the doc explaining how to delete accumulated unvalidated users using the admin/user/user interface would probably be adequate.
Comment #5
hunmonk CreditAttribution: hunmonk commentedno, i was actually asking between:
turning off the feature is a bad idea. it's been in the wild too long, and it's actually a nice feature as long as we get it working correctly.
Comment #6
hunmonk CreditAttribution: hunmonk commentedthe attached patch should fix this feature pretty cleanly for 6.x, and provide plenty of protection and documentation for users that are interested in using it. please test!
Comment #7
rfayI have confirmed that this bug is fixed by this patch. I set up the user deletion scenario and demonstrated the bug, ran the patch, and did the same scenario again, and it worked. I will test a couple of other aspects and respond again.
Thanks for your prompt response to this. It sure is nice to see a module so well-maintained.
Comment #8
rfayI examined the patch and experimented with the features. It looks good to me.
My one comment in reading the various description text changes would be that the description text for "Set Password" could be improved:
Wouldn't it be better to say something unambiguous, like "(use this setting instead of the Require e-mail verification ...)" or better yet, hide the core drupal checkbox? That's probably another issue, but I bring it up because that stuff was being changed.
Comment #9
hunmonk CreditAttribution: hunmonk commentedhttp://drupal.org/node/409338
patch applied to 6.x-1.x-dev and HEAD. thanks for taking the time to review!