New users deleted inappropriately - Deletion query is incorrect [#413916]

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

hunmonk CreditAttribution: hunmonk commented 26 March 2009 at 23:40

Status:

Patch (to be ported)

» Postponed (maintainer needs more info)

not 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:

user registers, and the table is updated, setting them as having the pre-auth status
user clicks the validation link, and LT removes their pre-auth status from the table, or admin manually removes the role, and the code checks to see if the user is marked as pre-auth in LT's table, and if it is, removes it.
cron checks the LT table, not the user roles, do determine who to remove.

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

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 26 March 2009 at 23:48

I 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 commented 27 March 2009 at 00:30

this 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

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 27 March 2009 at 17:44

I 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.

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 commented 27 March 2009 at 18:36

no, i was actually asking between:

Implementing a new table listing users that have not yet been validated and managing automatic deletion of users using that table.
disallowing admins to re-add the pre-auth role once it's been removed. this effectively makes it a one way change after registration

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 commented 28 March 2009 at 21:29

Assigned:	Unassigned	» hunmonk
Status:	Postponed (maintainer needs more info)	» Needs review

File	Size
lt_fix_user_purge.patch	11.35 KB

the 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

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 29 March 2009 at 13:47

I 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

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 29 March 2009 at 15:14

I 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:

This will allow users to choose their initial password when registering (note that this setting is merely a mirror of the Require e-mail verification when a visitor creates an account setting

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.