WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#9682 closed enhancement (fixed)

Make wp-login.php form pluggable

Reported by: misterbisson Owned by:
Milestone: 2.8 Priority: high
Severity: major Version: 2.7.1
Component: Users Keywords: has-patch tested commit
Focuses: Cc:

Description

Allowing plugins to override functions like retrieve_password() and reset_password() would help me integrate WP (well, BuddyPress, really) into my environment (academic institution with existing identity framework).

Attachments (6)

move-login-funcs-to-pluggable.diff (17.9 KB) - added by misterbisson 9 years ago.
Makes the switch( $action ) part of wp-login.php hookable, moves functions defined in wp-login.php to wp-includes/pluggable.php so they can be overridden
9682.diff (2.4 KB) - added by Denis-de-Bernardy 9 years ago.
9682.2.diff (3.0 KB) - added by Denis-de-Bernardy 9 years ago.
action validation, with filter on top
9682.3.diff (3.1 KB) - added by Denis-de-Bernardy 9 years ago.
make login, rather than an empty string, the default action
9682.4.diff (3.1 KB) - added by Denis-de-Bernardy 9 years ago.
fix the has_filter check
9682.5.diff (3.4 KB) - added by Denis-de-Bernardy 9 years ago.
against trunk @ 11219

Download all attachments as: .zip

Change History (29)

#1 @ryan
9 years ago

There's another patch somewhere that makes mouch of wp-login pluggable and checks for a login.php type file in the current theme? Would that approach be better so we're not putting stuff in pluggable that is used only by wp-login.php?

#3 @Denis-de-Bernardy
9 years ago

while they don't need to be pluginable, they're probably missing a hook or two.

picture someone creating a membership plugin or a forum plugin that needs to override the retrieve_password() because it won't filter the message, for instance.

patch coming up in a few seconds.

#4 @Denis-de-Bernardy
9 years ago

  • Keywords has-patch added

#5 @misterbisson
9 years ago

My application requires they either be replaceable, or there be more sophisticated hooks.

I need to replace reset_password(), for instance, so that it sends the message to user_email as well as an alternate email address I'm managing via a plugin. (user_email is being set to a person's on campus address, but if they can't log in to WP, they probably also can't get a reset email there.) Additionally, I'm also sending a reset code to the user's cell phone.

I also need to replace retrieve_password() and make the switch( $action ) pluggable because:

1: the reset code sent to the user's phone is different (shorter) than the one sent by email and so needs to be verified differently 2: sending a password back by email is unacceptable in my environment, so I then need to re-verify the username and allow the user to immediately set a new password.

@misterbisson
9 years ago

Makes the switch( $action ) part of wp-login.php hookable, moves functions defined in wp-login.php to wp-includes/pluggable.php so they can be overridden

#6 @misterbisson
9 years ago

My latest diff still moves the functions to pluggable.php, but the real point is the following addition just above the main switch( $action ):

global $wp_filter;
if( isset( $wp_filter[ 'wp_login_'. $action ] )):
	do_action( 'wp_login_'. $action );
else:
switch( $action ) {...

This allows plugins to hook into that and, perhaps, override the existing functionality. It has to check if there's an action attached to prevent leaving the user with a blank screen because somebody added an invalid action to the get vars.

That solution could eliminate my need to move the functions, as I could replace them with the action switch.

#7 @Denis-de-Bernardy
9 years ago

I've updated my patch based on your first feedback. looking into your patch and the rest of the feedback right away.

reset password is extended with:

do_action('password_reset', $user, $new_pass);

... so you can send the new password to wherever you want it to go.

both of the emails can be now cancelled by returning an empty message. in other words, you can disable the WP functionality in the sense that, WP will still do its stuff but it won't show to the user.

this hook seems valid to generate a cell phone key on the fly:

do_action('retrieve_password_key', $user_login, $key);

just ignore the $key and generate yours.

the last change is at the very end of the file: I've changed the default case in the switch statement, so as to make the entire thing pluggable.

it needs a little testing on that last point. please give it a roll quickly, so we know if anything else is needed. I'd love to see this one get into 2.8 as well, as a matter of fact, because the last point makes the entire screen overriddable.

#8 @Denis-de-Bernardy
9 years ago

  • Keywords needs-testing added

the check on wp_filter isn't always valid. if a callback is added and then removed from that hook.

@Denis-de-Bernardy
9 years ago

action validation, with filter on top

#9 @Denis-de-Bernardy
9 years ago

  • Keywords 2nd-opinion added

please give 9682.2.diff a whirl. it should even allow to replace the login form if you're not happy with it.

@Denis-de-Bernardy
9 years ago

make login, rather than an empty string, the default action

#10 @misterbisson
9 years ago

Putting the do_action() at the end of the switch doesn't allow plugins to override the built-in action functions. This is important because the existing functions really don't do what they need to for my application, and the actions don't help.

problems:

The retrieve_password_key action only gets fired if the user_activation_key in the users table is empty.

The $key stored in the users table never expires (so far as I can tell).

The $key stored in the users table will be re-used for future password resets (as far as I can tell).

The password_reset action happens after the $key has been validated (preventing plugins from doing their own validation/expiration).

#11 @misterbisson
9 years ago

The solution in your last diffs is what I'd first considered, but that either limits the action to the built-in ones or requires another hook to set allowable actions.

Understanding your point about actions being set and then removed, the following should work:

global $wp_filter;
if( is_array( $wp_filter[ 'wp_login_'. $action ] ) && count( $wp_filter[ 'wp_login_'. $action ] )):

#12 @misterbisson
9 years ago

My mistake. I didn't read your last patch well enough. Yes, it allows any number of actions.

#13 @misterbisson
9 years ago

false !== has_filter('login_form_' . $action) )

Needs to be

false == has_filter('login_form_' . $action) )

#14 @Denis-de-Bernardy
9 years ago

Replying to misterbisson:

The retrieve_password_key action only gets fired if the user_activation_key in the users table is empty.

The $key stored in the users table never expires (so far as I can tell).

The $key stored in the users table will be re-used for future password resets (as far as I can tell).

actually... it's unset in wp_set_password(). so that it only gets fired once -- until used up. this is probably desirable, too, so as to avoid getting repeated email if the form is the target of a robot.

that it doesn't expire is a potential issue, but it's going to trigger such a debate in trac that it probably belongs in a separate ticket.

The password_reset action happens after the $key has been validated (preventing plugins from doing their own validation/expiration).

shouldn't this be the job of a separate action? that way you get to keep the best of both worlds: if the user is sent an email by WP and an SMS, the first would send him to ?action=rp&key=foo, and the second would send him to ?action=smsrp&smskey=foo.

you'd generate smskey on the fly on do_action(retrieve_password) or do_action(retrieve_password_key), whichever makes the most sense, and proceed as needed from there.

you're correct on the has_filter. fixing this right away.

@Denis-de-Bernardy
9 years ago

fix the has_filter check

#15 @Denis-de-Bernardy
9 years ago

it needs to be ===, though, because has_filter can return 0 if the first filter has that priority.

#16 @Denis-de-Bernardy
9 years ago

yeah, it lets you do any action, and even override the default login screen and other actions with whatever you want as a bonus. :-)

#17 @misterbisson
9 years ago

Replying to Denis-de-Bernardy:

The password_reset action happens after the $key has been validated (preventing plugins from doing their own validation/expiration).

shouldn't this be the job of a separate action? that way you get to keep the best of both worlds: if the user is sent an email by WP and an SMS, the first would send him to ?action=rp&key=foo, and the second would send him to ?action=smsrp&smskey=foo.

If WP expired the reset codes, then maybe, but because expiring is a required feature for me....

I've actually developed a framework to handle this and similar actions: http://maisonbisson.com/blog/post/13862/wordpress-action-ticketing-api/. Since we seem to be working in such a similar space, I'd appreciate your feedback.

Your latest patch allows me to override the built in actions, though, so I'm good.

#18 @Denis-de-Bernardy
9 years ago

  • Keywords tested commit added; needs-testing 2nd-opinion removed

#19 @ryan
9 years ago

Patch not applying cleanly.

#20 @Denis-de-Bernardy
9 years ago

  • Keywords needs-patch added; has-patch tested commit removed

will get this fixed later today

@Denis-de-Bernardy
9 years ago

against trunk @ 11219

#21 @Denis-de-Bernardy
9 years ago

  • Keywords has-patch tested commit added; needs-patch removed

#22 @Denis-de-Bernardy
9 years ago

  • Priority changed from normal to high
  • Severity changed from normal to major
  • Summary changed from Move wp-login.php function definitions to pluggable.php to Make wp-login.php form pluggable

#23 @ryan
9 years ago

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

(In [11291]) Make login more pluggable. Props Denis-de-Bernardy. fixes #9682

Note: See TracTickets for help on using tickets.