Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#5831 closed defect (bug) (fixed)

Activation link - change from "?key=" to action variable

Reported by: r-a-y's profile r-a-y Owned by: djpaul's profile djpaul
Milestone: 2.2 Priority: normal
Severity: normal Version: 1.5
Component: Members Keywords: has-patch needs-testing
Cc:

Description

Referenced in a support forum thread:
http://jrfom.com/2014/08/11/buddypress-and-lighttpd/

This might be causing a lot of the activation email issues that we've been having.

Let's switch from using this:
example.com/activate/?key=XXX

to:
example.com/activate/XXX/

or:
example.com/activate/key/xxx/

While keeping the older $_GET method for backward compatiblity.

Attachments (1)

5831.01.patch (3.1 KB) - added by r-a-y 10 years ago.

Download all attachments as: .zip

Change History (17)

#1 @boonebgorges
10 years ago

  • Keywords needs-patch added

#3 @r-a-y
10 years ago

I think ?key hits spam / malware email filters a certain way. Both the WordPress ticket and the bloggingatmidnight post kind of confirm this.

We just need some testers once this ticket has a patch :)

@r-a-y
10 years ago

#4 @r-a-y
10 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

01.patch changes the activation link to example.com/activate/XXX/ and also supports activating with older links (example.com/activate/?key=XXX).

Needs testing for those that have had activation email problems in the past.

#5 @DJPaul
10 years ago

Patch looks OK, we need to figure out how to create users in the unit tests that aren't automatically activated so we can test this.

#6 @DJPaul
10 years ago

Upon consideration, this would be more of an integration test, especially now we have BP_Signups class, so I'm not going to worry about that.

#7 @djpaul
10 years ago

  • Owner set to djpaul
  • Resolution set to fixed
  • Status changed from new to closed

In 9109:

Signups: move the activation link's key from the query param into the URL proper.

Some email hosts strip the key from the activation link, or mangle it, or prevent it being easily copyable inside their web interfaces. Some web servers/configurations, can also be commonly configured to strip query parameters from the majority of requests, so hopefully this change will alleviate these problems.

Compatibility is maintained for the older query param-style activation links.

Fixes #5831, props r-a-y.

#8 @johnjamesjacoby
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This is certainly going to break backwards compatibility for anyone checking the $_GET on their own.

Can we post something on BP Devel? I think we should encapsulate the key getting logic in a function to direct people to; if our new approach doesn't fix the issue (or breaks again years from now) the function gives us a way to change the innards without breaking changes again.

#9 @vimes1984
10 years ago

I've just experienced a host having a spam filter installed that follows the external validation link in the outbound email, activating the users accounts always automatically rendering the activation link useless...
https://buddypress.trac.wordpress.org/ticket/6049
Am happy to help how i can :D

#10 @r-a-y
10 years ago

In 9203:

Signups: Do not bail if no key is found on the activation screen.

r9109 broke how the activation page is rendered if no key is passed in
the URL. For example, if you go to example.com/activate/ instead of
example.com/activate/?key=XXX or example.com/activate/XXX/.

Anti-props r-a-y.

See #5831.

#11 @r-a-y
10 years ago

This is certainly going to break backwards compatibility for anyone checking the $_GET on their own.

If plugins were doing checks for the key on their own, wouldn't the plugin need to remove how BP does its activation process with:

remove_action( 'bp_screens', 'bp_core_screen_activation' );

I think we should encapsulate the key getting logic in a function to direct people to

Sure, I think that's fair.

Last edited 10 years ago by r-a-y (previous) (diff)

#12 @vimes1984
10 years ago

can i help with anything? I've tried the IRC but it seems pretty dead...

#13 @r-a-y
10 years ago

vimes1984 - Your comments should be left in #6049. This is a slightly, different issue.

I've tried the IRC but it seems pretty dead...

We now use Slack instead of IRC. Check out this post for more info on how to get on Slack:
https://bpdevel.wordpress.com/2014/10/28/were-going-to-move-our/

#14 @r-a-y
10 years ago

jjj - Have you read comment:11?

I don't see a way how a dev checks the old $_GET['key'] and adds their own implementation without getting rid of the existing activation screen.

Here's the old code for reference:
https://buddypress.trac.wordpress.org/browser/tags/2.1.1/src/bp-members/bp-members-screens.php#L214

If there are problems with this implementation, can you let us know?

I'm going to leave this ticket open during beta.

#15 @johnjamesjacoby
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 9400:

Review r9109 and r9203, and clean up bp_core_screen_activation() to improve readability and separate a few procedural tasks. Fixes #5831.

#16 @DJPaul
9 years ago

We missed several instances of these; I'm going to address in 2.5 with other email changes.

Note: See TracTickets for help on using tickets.