Deleting Users in Manage Signups Leaving Database Entries

[aaclayton]

Using BuddyPress 2.0, when I delete unactivated user accounts using the new Manage Signups admin component, it is successfully deleting the user's entry in the wp_signups table, however it is NOT removing data from other tables including wp_users, wp_usermeta, and wp_bp_xprofile_data.

This has some really negative side effects including database bloat and preventing future users (particularly the same person) from re-registering the username correctly.

To make things worse, the previous working solution for managing pending activations, the BuddyPress Pending Activations plugin no longer can be used as a fallback as it does not remove entries from the wp_signups table.

[imath]

hi aaclayton

Thanks for your feedback. Here are the tests i've made. This case is only happening on a non multisite config.

1. On a "clean" configuration : that is to say that the BuddyPress registration process has not been altered by another plugin or direct manipulation of the database by a user. Deleting a signup will also delete the user.
2. On a "not clean" configuration : that is to say something happened in the database and more precisely, to reproduce your issue, i had to manually empty the user meta "activation_key". That is to say the meta key exists with no value while the user status is set to 2. Deleting a signup will not delete the user.

That said :
I've tested the BuddyPress Pending Activations plugin to make sure the fact a user with a user status set to 2 having an empty activation key was not related to the way this plugin works. And this plugin seems to do things right.
I've also made other tests :

3. After test 2. if i go on the user's profile in front end, i'm able to delete the user.
4. i've activated BuddyPress Pending Activations and deleted a signup from its interface, it also deleted the user, but not the signup in BuddyPress signup management screen which is normal. Once done Deleting the signup from there worked.

The reason a signup is not deleted when using the BuddyPress signups management screen :
When deleting a signup, we check if the username exists to get an eventual user_id, if we get this id we then check for the activation_key. This "extra security" check is the reason why the user is not deleted. As the activation_key is empty it cannot be a key generated by BuddyPress at first place.

I'm attaching 5575.diff to this ticket which removes the activation_key check. It would be great if :

• core team could give me an opinion about it
• aaclayton could : check the signups that gave him troubles to see if they have a corresponding empty "activation_key" user meta. And check if applying the patch solves his issue.

[boonebgorges]

What is the purpose of the "extra security" check? What's the scenario we're protecting against? (Obviously, cases where the activation_key doesn't match. But when does this happen? Is it when a plugin is interfering in the registration process? If this is the case, what is the danger of deleting the user account, given that user_status=2 *and* the signup ID has been passed to the delete method? Just trying to get a grasp on the point of having the check in the first place.)

[imath]

Replying to boonebgorges:

Just trying to get a grasp on the point of having the check in the first place.

Retrospectively the username_exists check is obviously enough, the activation check was for me a way to be definitively sure the signup had a 100% correspondance with the user_id as activation_key in the signups table == wp_hash( $user_id ).

Even if this should be fixed as we will be re-generating the activation_key while upgrading, i agree that my "excès de zèle" is not necessary.

[boonebgorges]

Thanks, imath. I can't see any danger in removing the check. But let's wait to hear back from aaclayton to see whether it actually addresses his issue before we go changing something that may not be broken.

[aaclayton]

So, I'm no longer 100% certain about what is causing the issue I originally reported. I created this ticket based on database behavior I was experiencing in a local test environment. I ended up rolling out my theme changes for BuddyPress 2.0 to my live site (expecting to deal with the same issues which prompted my ticket) only to find different (but also not-intended) behavior.

I am at work all day today and can't dig too deeply into the code to track down what might be happening, so for the short term allow me to provide a lot of information about my setup in hopes that it will spark some ideas on your end.

My Setup
I'm running WordPress (3.9) single-site with BuddyPress (2.0) and bbPress (2.5.3). No other relevant plugins are in-play.

On My Local Environment
On my local test site, upgrading to BuddyPress 2.0 created a wp_signups table which had not previously existed. New user registrations would create the following:

1) An entry in wp_users with user_status = 2 and user_activation_key = <key>
2) Entries in wp_usermeta for default WordPress meta, activation_key = <key>, and registration_ip = <ip> (a custom usermeta I save for new accounts).
3) An entry in wp_signups which was fully populated

Deleting an un-activated user through the Manage Signups panel would remove item (3) but leave all records in (1) and (2) intact. Deleting an un-activated user with the BuddyPress Pending Activations plugin would remove all records in items (1) and (2), but leave (3) intact.

I tested and replicated this process using both my own custom theme and the default WordPress 2013 theme.

On My Live Site
On my live site environment, upgrading to BuddyPress 2.0 did not create a wp_signups table. New user registrations are creating:

1) An entry in wp_users with user_status = 2 and user_activation_key = <key>
2) Entries in wp_usermeta for default WordPress meta, activation_key = <key>, and registration_ip = <ip> (a custom usermeta I save for new accounts).

No un-activated users are displayed in the new BP2.0 Manage Signups interface, I presume this is because the wp_signups table was not created and no users are assigned the "pending" role.

I am still able to use the BP Pending Activations plugin on my live site to delete both (1) and (2).

I'm not certain why a wp_signups table was not created when BuddyPress was upgraded to 2.0. I assume this was the expected behavior? Is there any information I can provide that could help determine whether this is some flaw in BP itself or a peculiarity of my specific setup?

Please let me know what would be the most helpful from my end.

[imath]

Replying to aaclayton:

Please let me know what would be the most helpful from my end.

aaclayton
Could you check the signups that gave you troubles to see if there is a corresponding empty "activation_key" in usermeta table. I suspect that in your usermeta table you have something like this for the problematic signups :

id	user_id	meta_key	meta_value
154	45	activation_key

[aaclayton]

I'll be able to check this once I get home (to my local test environment). I don't recall seeing any empty activation meta keys, though. I'll get back to you with a confirmation of this later tonight.

Would an empty activation_key in usermeta prevent the manage signups interface from removing wp_users and wp_usermeta entries when a wp_signups entry was deleted?

Similarly, why was a wp_signups table not created on my live site? New registrations on my live site are only using wp_users and wp_usermeta and therefore are not able to be managed by the BuddyPress administration component.

[imath]

Replying to aaclayton:

I'll be able to check this once I get home (to my local test environment). I don't recall seeing any empty activation meta keys, though. I'll get back to you with a confirmation of this later tonight.

thanks

Would an empty activation_key in usermeta prevent the manage signups interface from removing wp_users and wp_usermeta entries when a wp_signups entry was deleted?

Yes, because during the upgrade process the corresponding activation_key will end up empty and it's the only way i've managed in reproducing your issue. So i'd like to be sure that's what happened on your local dev.

Similarly, why was a wp_signups table not created on my live site? New registrations on my live site are only using wp_users and wp_usermeta and therefore are not able to be managed by the BuddyPress administration component.

That's a different bug/ticket i think, it's pretty annoying as the activate process ( bp_core_activate_signup() ) needs this table.

[aaclayton]

OK, I looked into this further on my local environment once I got home. On my local site new user registrations ARE generating an activation_key entry in wp_usermeta, it is not blank. The activation_key column in wp_users, IS blank, if this matters. The wp_signups entry is fully populated except for the domain, path, and title fields.

I am able to resend the activation email using the "Manage Signups" screen with no issues.

I am also able to activate an account successfully using this screen, which changes their user status and removes the activation_key from their usermeta. However, this DOES NOT remove their wp_signups entry, which persists even after successful activation.

I am NOT able to successfully delete a pending activation. The "Manage Signups" screen displays a success message, and the wp_signups entry is deleted, but all data in wp_usermeta and wp_users is left intact (consistent with my original post).

[imath]

Replying to aaclayton:

I am also able to activate an account successfully using this screen, which changes their user status and removes the activation_key from their usermeta. However, this DOES NOT remove their wp_signups entry, which persists even after successful activation.

The fact the entry persists is the right behavior. Though, you should observe that the field active is not more 0 but 1 for the activated users.

I am NOT able to successfully delete a pending activation. The "Manage Signups" screen displays a success message, and the wp_signups entry is deleted, but all data in wp_usermeta and wp_users is left intact (consistent with my original post).

Strange, this should mean that the activation_key is not set the way BuddyPress does..

Can you test the ​5575.diff patch to see if it solves the delete issue ?

Thanks again for your help.

[aaclayton]

Both my live site and it's local test environment are single-site installations (or at least, they are intended to be). I will test the diff when I get home tonight and let you know if it helped!

Regarding the issue on my live site in which the wp_signups table was not created or used, causing bp_core_activate_signups() to have trouble. I traced down this issue.

My live installation was using a database user who does not have table creation permissions (for security best practices). I've since re-installed BuddyPress 2.0 using a database user with administrative privileges, and the wp_signups table is alive and well now.

It might be a good idea to put a failsafe in the BP installation script to make sure the wp_signups table was successfully created, and warn the user if it was not, since it will prevent new user registrations from occurring with no obvious evidence as to why.

Now to do some tests on my live site installation and see if pending user deletion works...
More info to come.

[imath]

Replying to aaclayton:

It might be a good idea to put a failsafe in the BP installation script to make sure the wp_signups table was successfully created, and warn the user if it was not, since it will prevent new user registrations from occurring with no obvious evidence as to why.

I agree, i'm creating a ticket for it.

Thanks, waiting for the result of your tests.

[aaclayton]

OK, here's some testing on my live site.

New user registrations create:
1) a wp_users entry with user_status = 2 and activation_key left blank
2) wp_usermeta entries for default meta entries with the activation_key meta populated
3) a wp_signups entry

I am able to manually activate the user. This changes user_status to zero, deletes the activation_key usermeta row, and changes active = 1 in the wp_signups table. Looks to be working as intented so far.

If I attempt to delete a pending activation, ONLY the wp_signups entry is removed. The entries in wp_users and wp_usermeta are left untouched. So the problem reported in my original ticket persists. I'll try again with the .diff applied. More info to come.

[aaclayton]

So, I got caught up with work stuff, but I was eventually able to test 5575.diff. Unfortunately it did not resolve the issue.

I took some time to look into the codebase, and I think I have determined the problem.

The delete() method in BP_Signup is running correctly regardless of whether the hash check is made. This calls bp_core_delete_account( $user_id ) which is where the problem lies. Please refer to bp-members-functions.php line 1051.

	// Bail if account deletion is disabled
	if ( bp_disable_account_deletion() )
		return false;

This is the problem. I do not want my users deleting their own accounts, so I have used the BuddyPress setting to prevent this from happening. What ends up happening is that we bail out of bp_core_delete_account without wp_delete_user() ever being called.

After this, we return to the BP_Signup:delete() method, and no errors have been generated, so on lines 640-650 the entry in wp_signups is removed. This perfectly explains the behavior I have been noticing.

The key issue here is that the optional setting bp_disable_account_deletion is getting involved in the administrative process, not only the front-end deletion capability. The wording of the BuddyPress option is "Allow registered members to delete their own accounts". This is a great option to have, but it should not interfere with site administrators deleting non-activated accounts, so this check needs to be overridden in bp_core_delete_account to allow site admins to delete users even if the BuddyPress setting disallows it.

Modifying line 1058 of bp-members-functions.php to the following has temporarily resolved my issue.

if ( bp_disable_account_deletion() && !is_super_admin( bp_loggedin_user_id() ) )

I hope this has done an adequate job of explaining the problem, and hopefully a more well-thought out solution can be implemented in 2.0.1! Please let me know if you have any follow-up questions.

[imath]

Replying to aaclayton:

	// Bail if account deletion is disabled
	if ( bp_disable_account_deletion() )
		return false;

Nice catch ! Thanks a lot for you research about this.

I think we have 2 options to solve this,

• 5575.option1.diff
• 5575.option2.diff

i'm in favor of option 2 as deleting a user will only happen on non multisite configs, what do you think ?

[boonebgorges]

aaclayton - Thanks very much for the additional debugging. Your analysis sounds right.

imath - Both option1 and option2 seem like hacks to me. The underlying issue is that bp_disable_account_deletion() is a setting that is supposed to limit only front-end, non-admin users, but we're checking it in a fairly low-level function that is used in a wide variety of instances. 5575.02.patch is, IMO, the correct fix: move the bp_disable_account_deletion() check into the screen function that applies to front-end deletion, removing it from bp_core_delete_account(). That way, delete_users users have unfettered ability to delete accounts (which is how it should be).

[imath]

Yes i was a bit afraid to play in this area in case of multisite..., and wanted to only fix the "deleting a signup issue". But you're right.

Just for info: tested it and i got
Notice: Trying to get property of non-object in /Users/imath/Sites/wp-mu/wp-includes/ms-blogs.php on line 886

Looks like there will be a new function in 3.4 ;)

@since 3.4.0 > function _update_blog_date_on_post_delete

[boonebgorges]

Erm - What is causing this function to be called? Are we triggering transition_post_status somewhere?

[imath]

Maybe as this function hooks 'delete_post' and when a user is wpmu_delete_user() all posts of a blog owned by this user are trashed. I've tried again on another multisite and the notice did not show. It's strongly possible that as i made some tests about #3460 on the other config, the fact i've removed the post type explains the notice.

[imath]

Sorry i was testing the capacity 'delete_users' with the setting bp_disable_account_deletion() in multisite (BuddyPress not network/network activated) when deleting a real user or a signup being a super admin or a regular admin.

When deleting a signup the _update_blog_date_on_post_delete thing doesn't happen.

You patch (5575.02.patch) is the solution to the problem.

[boonebgorges]

Ah, thanks imath. This sounds like an unrelated (WP) bug that is triggered when a plugin that created post types is deactivated.

[boonebgorges]

In 8324:

Check bp_disable_account_deletion() setting in screen function rather than bp_core_delete_account() business function

The purpose of the disable-account-deletion setting is to prevent normal (ie
non-admin) BP users from deleting their own accounts through the front-end
interface. Locating the settings check in the business function
bp_core_delete_account() therefore caused problems when deleting accounts in
areas that were not meant to be covered by the setting, such as when accounts
are deleted in the Dashboard by an admin, or when the BP_Signup::delete()
method is called. This caused a particular problem in BP 2.0, because the
deletion of signups was no longer correctly cleaning up other data from the
database (xprofile, usermeta, etc).

Fixes #5575

[boonebgorges]

In 8325:

Check bp_disable_account_deletion() setting in screen function rather than bp_core_delete_account() business function

The purpose of the disable-account-deletion setting is to prevent normal (ie
non-admin) BP users from deleting their own accounts through the front-end
interface. Locating the settings check in the business function
bp_core_delete_account() therefore caused problems when deleting accounts in
areas that were not meant to be covered by the setting, such as when accounts
are deleted in the Dashboard by an admin, or when the BP_Signup::delete()
method is called. This caused a particular problem in BP 2.0, because the
deletion of signups was no longer correctly cleaning up other data from the
database (xprofile, usermeta, etc).

Fixes #5575

[boonebgorges]

In 8330:

Remove disable_account_deletion cases from bp_core_delete_account() tests

As of r8324, this settings check takes place in an action controller, not here
in the business level function. See #5575

[boonebgorges]

In 8331:

Remove disable_account_deletion cases from bp_core_delete_account() tests

As of r8324, this settings check takes place in an action controller, not here
in the business level function. See #5575