BP 2.0 upgrade routine improperly deletes existing user roles if activation_key usermeta is present

[boonebgorges]

In WP non-Multisite prior to BP 2.0, the user registration workflow worked like this:

1. At registration, a user is created with user_status=2 and a usermeta with the key 'activation_key'
2. Activation email is sent
3. When the activation URL is loaded, (a) user_status is switched to 0, and (b) the activation_key usermeta is deleted

The switch to the wp_signups schema for signups in BP 2.0 includes a migration tool that moves old-style unactivated signups to the new system. It identifies signups as those WP users that have an activation_key value in usermeta. ​https://buddypress.trac.wordpress.org/browser/tags/2.0/bp-core/bp-core-update.php#L353 Then, as part of the migration, it deletes capabilities and user_level usermeta for the user, to keep them out of regular user lists. (See line 391-393.)

It turns out (see http://buddypress.org/support/topic/lost-admin-access-after-2-o-update/) that there are situations where a user can be activated but still have the activation_key value in the DB. The result: when the migration routine runs, these users are identified incorrectly as unactivated signups, and their roles are improperly revoked.

There are probably various ways in which the activation_key could be retained for activated users. One concrete one I've identified is the use of this plugin ​http://wordpress.org/plugins/bp-disable-activation/, which activates the user by switching the user_status to 0 but does *not* delete activation_key.

I'll follow up with suggested fixes.

[boonebgorges]

5553.patch adds a sanity check against user_status before migrating any individual user. This should catch many/most of the problematic cases. Also includes unit tests that demonstrate the problem.

[r-a-y]

5553.patch is good to go in my eyes.

I was looking at an alternative, similar fix that did not pan out! ;)

What we could also do is remove the 'activation_key' meta entry as well before bailing.

[DJPaul]

Patch looks OK.

[imath]

I think there's another trouble. checking something.

[imath]

I think we should also bail if the activation_key is empty.

[imath]

r-a-y 's comment made me think a bit more to it

What we could also do is remove the 'activation_key' meta entry as well before bailing.

I think he's right, maybe we should :
1- check for user_status and if !== 2, delete activation key and bail
2- if activation_key has disappeared and user status is === 2 then regenerate the activation key

So i've changed my mind about 5553.02.patch and suggest 5553.03.diff

[DJPaul]

The idea about deleting a meta key if we don't need to use, worries me. What if something else is expecting it to be there in some situation where we've decided to delete it?

What also is the point of re-generating the activation key if we're never going to tell the user what it is (re-send the activation email, etc).

[imath]

Replying to DJPaul:

What also is the point of re-generating the activation key if we're never going to tell the user what it is (re-send the activation email, etc).

I've spent the day playing with activation_key and user_status. I think there are 5 possible cases

regular cases

1. user status is 0 and no activation key user meta
2. user status is 2 and not empty activation key user meta

Non regular ones

3. user status is 0 and activation key user meta but empty value
4. user status is 0 and activation key user meta not empty value
5. user_status is 2 and activation key user meta but empty value

5553.patch will handle 3 and 4, but not the case number 5 and this will create a signup with no value for the activation key and will never be activable.. And the user won't display in user's list as he has a user_status set to 2.

[boonebgorges]

Hi all - thanks for your attention to this ticket.

Let's back up for a second here and reassess the problem we're trying to solve. We are migrating old-style unactivated signups to the new schema, and we want to make sure that we do this in as reliable a way as possible. "Reliable" is a tricky concept here. Obviously, we want to make sure that we successfully migrate as many items as is reasonably possible. However, we should err on the side of caution, because false positives - migrations that should not have been performed - have serious consequences.

In contrast, false *negatives* - cases where we fail to migrate an unactivated signup - are not nearly as serious. Practically speaking, the worst that can happen here is that a signup will not show up in the new Pending Users interface. Not ideal, but obviously not a dealbreaker - activation should still work fine.

And it's worth remembering that these false negatives are always the result of some other plugin doing some sort of weird stuff (such as the bp-disable-activation plugin I describe above). So there's no way we can predict the negative side effects of false negatives.

So, while I understand the motivation behind imath's additions, I think they're only asking for trouble. As DJPaul notes, deleting metadata is dangerous stuff, and in this case it would serve no purpose from BP's point of view - BP 2.0+ doesn't care about activation_key in usermeta anymore - while it could cause bugs in other plugins that expect that data to be there (which are, very likely, the plugins that caused the false positive in the first place). Likewise, since BP only looks in wp_signups for activation_key values, there's no point in copying anything over to wp_usermeta. Our job is not necessarily to clean up after other plugins, it's just to make sure that we don't screw up our own data because of what other plugins do.

On that note, 5553.02.patch looks like the prudent route to me.

[imath]

Replying to boonebgorges:

On that note, 5553.02.patch looks like the prudent route to me.

I'm sorry to insist, i agree with all Paul and you said, i just have a problem with the case where a user has a user_status set to 2 and a user meta activation_key but with an empty value.

5553.02.patch will not put this user in the signups table as the activation_key is empty. So After upgrade, this user will be able to log in even if his status is set to 2.

But that might be another problem see bp_core_signup_disable_inactive() at line 1755

[boonebgorges]

5553.02.patch will not put this user in the signups table as the activation_key is empty. So After upgrade, this user will be able to log in even if his status is set to 2.

I guess this is a legitimate concern, in particular since the new login logic comes from BP and not from some other plugin. I remain skeptical that there are real-life scenarios where user_status = 2 and '' === get_user_meta( $user_id, 'activation_key', true ), but I guess it's possible.

If it's a huge concern, how about switching around the way the query is run? It seems like activation_key is not a reliable test of who is an unactivated user. On the other hand, user_status=2 is. See 5553.04.patch. (I have not tested the patch - just putting it up to see if people think it's a more reliable idea.) This does not take into account the 'activation_key' at all, so will not help with your case (5) above. But, as I suggest above, I don't think this is much of a meaningful risk.

[imath]

Replying to boonebgorges:

But, as I suggest above, I don't think this is much of a meaningful risk.

I think i like much more 5553.04.patch. Just tested it and the case (5) is going into the signups table, so he won't be able to log in.

Activating him from the signup management screen will result in an error unless #5559 patch is applied. If this last patch is applied we have "That username is already activated" and the signup is not converted. Reason is the activation_key in the signups table is empty.

If case 5 try to log in he has the not active account message and the link to receive his activation key. If he clicks on it, he will receive '' :(

I really think we should regenerate the activation_key if it's empty to use it in the signups table without creating the activation_key user meta.

I've tested 5553.05.diff and this way the user will receive it's activation key, the admin will be able to activate him..

[boonebgorges]

imath - 5553.05.diff looks fine to me.

What do you think of changing the 'fields' clause in the get_users() function to 'all'? (Or even an array of the fields we need - I think its user_login, user_pass, user_registered, user_email.) all_with_meta loads a very large amount of additional data into memory (and requires a potentially huge SQL query); from what I can see, this data is never used.

[imath]

Replying to boonebgorges:

What do you think of changing the 'fields' clause..

I think i agree with you :)
all with meta was here to include the activation_key user_meta.
If we're sure no plugin is playing with the way a BuddyPress activation key is generated, we can just use the array of fields i've put in 5553.06.diff, and rebuild the activation key as it won't be in the query in this case.

[boonebgorges]

imath - Thanks. But your 5553.06.diff won't work, because the new activation keys will be different from the old ones. That means that existing links in activation emails will no longer work.

This is getting too complicated, so I broke it out and wrote some unit tests. See 5553.07.diff. I've kept the 'fields' param of get_users() as you set it up, and performed a separate query to get existing activation keys, which I then merge back into the $signups array. This should still have much less overhead than all_with_meta, which grabs *all* usermeta and puts it in the cache.

Unit tests are now passing. Let me know what you think.

[imath]

Replying to boonebgorges:

the new activation keys will be different from the old ones

Well, only if the constant BP_SIGNUPS_SKIP_USER_CREATION is defined, meaning the upgrade to 2.0 should allways happen with old system, but i agree there's a risk if a plugin alter the way BuddyPress used to define activation keys.

See 5553.07.diff. Let me know what you think.

I'll test it tonight from home with my specific testing database.

[boonebgorges]

Well, only if the constant BP_SIGNUPS_SKIP_USER_CREATION is defined,

Ah, OK. I was looking at the current code and forgetting we used to do this: ​https://buddypress.trac.wordpress.org/browser/tags/1.9.2/bp-members/bp-members-functions.php#L1320

My method is safer, but it does introduce more overhead during the upgrade process. Have a test and see what you think.

[imath]

Replying to boonebgorges:

My method is safer, but it does introduce more overhead during the upgrade process. Have a test and see what you think.

Yes i agree, let's take as less as possible risks.

I've tested it and had a problem. I think it's due to the fact the $signup var is first use in the loop to attach activation_keys to signups, then use in the loop to migrate the signups
As a result, i had the same user migrated several times

unsetting $signup seems to fix this.

In 5553.08.diff i've also edited the tests, though i'm not sure i did well. The migrate process will only happen on non multisite, and keys are defined "old-way" with wp_hash( $user_id ). So edited the functions using this way of setting the activation_keys.

[boonebgorges]

unsetting $signup seems to fix this.

Yup, looks good.

The migrate process will only happen on non multisite, and keys are defined "old-way" with wp_hash( $user_id ).

Oh, thanks. Doesn't really matter - the test just checks that they're the same before and after. The key could be 'foo' :)

How do we feel about the state of this? It looks to me like the following items are addressed:

1. By focusing on user_status=2, we should ensure that we don't accidentally delete any proper permissions (the original purpose of the ticket)
2. This new way of identifying users also allows us to avoid 'all_with_meta' and the unnecessary querying of users with user_status=0, which should result in significant performance benefits.

So I think we're good to go here. imath, does that seem right to you?

[imath]

Replying to boonebgorges:

So I think we're good to go here. imath, does that seem right to you?

Absolutely, i also think we're good to go ;)

[boonebgorges]

In 8314:

Improve the migration tool that converts old non-multisite signups to post-2.0 system

This refactoring involves a number of improvements:

• Old signups are identified as those users with user_status=2, instead of those users with an 'activation_key' usermeta value. Use of the latter technique caused problems with upgrades when plugins had interfered in the normal BuddyPress registration flow in such a way as to retain the activation key on activated accounts. user_status=2 is a more reliable technique.
• Querying by user_status=2 means that the get_users() query does not have to include all usermeta, which makes for a more performant query on large sites.
• The migration routine has been broken out into a separate function bp_members_migrate_signups(), and unit tests have been written for it.
• Activation keys are now regenerated for migrated users who did not have an existing key in usermeta for some reason.

Fixes #5553

Props imath

[boonebgorges]

In 8315:

Improve the migration tool that converts old non-multisite signups to post-2.0 system

This refactoring involves a number of improvements:

• Old signups are identified as those users with user_status=2, instead of those users with an 'activation_key' usermeta value. Use of the latter technique caused problems with upgrades when plugins had interfered in the normal BuddyPress registration flow in such a way as to retain the activation key on activated accounts. user_status=2 is a more reliable technique.
• Querying by user_status=2 means that the get_users() query does not have to include all usermeta, which makes for a more performant query on large sites.
• The migration routine has been broken out into a separate function bp_members_migrate_signups(), and unit tests have been written for it.
• Activation keys are now regenerated for migrated users who did not have an existing key in usermeta for some reason.

Fixes #5553

Props imath