Skip to:
Content

BuddyPress.org

Opened 2 years ago

Last modified 3 months ago

#7938 new defect (bug)

Using the same activation key results in wrong "Invalid activation key" error

Reported by: r-a-y Owned by:
Milestone: Awaiting Contributions Priority: normal
Severity: normal Version:
Component: Members Keywords: needs-unit-tests
Cc:

Description

Reported here:
https://buddypress.org/support/topic/invalid-activation-key-10/

Original report follows:

So I’ve been plagued by this from users who either try and use their activation email again, or some that double click on the link. They all end up on the classic ‘Invalid activation key’ page.

Looking at the buddypress code I think there is a bug (feature?) that means that once an activation code is used once, it will never be able to be used again. This is despite there being code further on in this process to check this scenario and return an error of ‘The user is already active’. This code NEVER RUNS, because of the way the key is looked up in the database.

once an account is activated (active=1), the key can never be looked up again. Hence the error my users were getting.


Forum thread also has code that attempts to address the issue.

Sounds like a valid report, but needs verification and testing.

Attachments (1)

7938.diff (468 bytes) - added by Jean-David 3 months ago.

Download all attachments as: .zip

Change History (7)

#1 @DJPaul
23 months ago

  • Milestone changed from Awaiting Review to Under Consideration

Assuming these codes are somewhat randomly generated in our end (MD5, I'd guess), it's amusing to me that we find reports with the key conflicting. I'm not sure where we store it, but if a user is activated, I can't see any reason why we need to keep that key around (we could delete it?).

#2 @DJPaul
23 months ago

See also #7914

#3 @DJPaul
20 months ago

  • Milestone changed from Under Consideration to Awaiting Contributions

#4 @dsar
3 months ago

The original report is not about conflicting keys, it's about users trying to activate keys after they had already been activated. At the moment, that will result in an "Invalid activation key" message for the user.

The code in bp-members-function.php, function bp_core_activate_signup()

                if ( $signup->active ) {
                        if ( empty( $signup->domain ) ) {
                                return new WP_Error( 'already_active', __( 'The user is already active.', 'buddypress' ), $signup );
                        } else {
                                return new WP_Error( 'already_active', __( 'The site is already active.', 'buddypress' ), $signup );
                        }
                }

will never run, as BP_Signup::get will never fetch non-activated keys from signups table, as it has this code in it:

                $sql['where']   = array();
                $sql['where'][] = "active = 0";

I'm not sure if just replacing

$sql['where'][] = "active = 0";

with

$sql['where'][] = "1 = 1";

would affect anything else, but it would fix this issue.

Having looked back on the changes in these two files, I'm not seeing changes to this part of the code in at least the last 5-6 years, which makes me wonder if anything else might be depending on it?

The issue with "Invalid activation key" error is easily reproduceable by just double clicking the Activate button for any new users.

@Jean-David
3 months ago

#5 @Jean-David
3 months ago

Not sure if my commit doesn't make any side-effect, I'd like some feedback on this, tanks.
Basically, in the request, there is always

$sql['where'][] = "active = 0";

Removing the first index is safe as it's declared earlier in the code as the first key :

$sql['where']   = array();
$sql['where'][] = "active = 0";
Last edited 3 months ago by Jean-David (previous) (diff)

#6 @imath
3 months ago

  • Keywords needs-unit-tests added; needs-testing removed

Hi @Jean-David

Thanks a lot for your contribution, I guess it should fix the issue. It would be awesome if you could make sure it doesn't introduce side effects improving all our unit tests starting by test_get_with into this file:
trunk/tests/phpunit/testcases/members/class-bp-signup.php

The idea is to make sure the active field is 0 for those existing tests and to add a new test like test_get_with_activation_key_already_active to check we get 1 for the active field value when the signup has been activated.

Note: See TracTickets for help on using tickets.