Trigger two-factor flow only when expected

[kasparsd]

What?

Avoid triggering the two-factor logic on every wp_login hook even when no login attempt is expected.

Why?

Fixes #659, #592.

How?

WP core uses the following filters during the wp-login.php request in the order of priority:

// Priority 20.
add_filter( 'authenticate', 'wp_authenticate_username_password', 20, 3 );
add_filter( 'authenticate', 'wp_authenticate_email_password', 20, 3 );
add_filter( 'authenticate', 'wp_authenticate_application_password', 20, 3 );

// Priority 30.
add_filter( 'authenticate', 'wp_authenticate_cookie', 30, 3 );

// Priority 99.
add_filter( 'authenticate', 'wp_authenticate_spam_check', 99 );

We want to trigger the two-factor workflow only for requests that:

1. supply username and/or password,
2. and don't have existing and valid user cookies.

So we hook right after wp_authenticate_cookie priority 30.

Secondly, to allow other plugins from utilizing the wp_login action, we move our callback to priority PHP_INT_MAX. As explained in the comments inline, there is no risk to other plugins overwriting the two-factor requirement (through a redirect, for example), because we're disabling login cookies during authenticate.

I used https://wpdirectory.net/search/01JH7SBWRMX2F41V0G9W19WPHE to find the top 100 most popular plugins using the wp_login action and none of them appear to do anything that would disable the updated two-factor flow logic.

Testing Instructions

1. Setup a user with one of two-factor methods enabled.
2. Attempt to login in a new private browser session and confirm that the second factor is requested.
3. After a successful login, visit wp-login.php and confirm that you're redirected to the admin dashboard.

Screenshots or screencast

Changelog Entry

Fixed -- ensure that two-factor workflow is triggered only during login attempts.

[kasparsd]

Priority 31 is still after wp_authenticate_username_password() so the resolved $user will be set correctly.

[kasparsd]

This is ready for code review. Please see the comments inline the pull request for additional details.

[kasparsd]

The main difference here is that we're now triggering any of this logic only for requests with the $username supplied which indicates a login attempt instead of a simple authenticated request to wp-login.php.

[kasparsd]

Another major difference here is that we're now triggering the wp_login much later to avoid any other integration plugins to do their job before we completely hijack the remaining login flow.

The only risk here is that some other plugin hijacks the login flow before us and issues a redirect, for example.

I add this sample hook:

add_action(
	'wp_login',
	function ( $user ) {
		if ( $user ) {
			wp_redirect( admin_url( '?redirect=test' ) );
			exit;
		}
	}
);

which returns a redirect before our wp_login, and the login session correctly fails because we're blocking the login cookies through filter_authenticate() attached to the authenticate hook which fires before wp_login.

[kasparsd]

@dd32 Could this impact the WP.org implementation in any way?

[dd32]

@dd32 Could this impact the WP.org implementation in any way?

Definately could, changing when cookies are sent could affect other things we have hooked in to prevent cookies under specific situations (ie. Other login interstitials). If it could affect us, it could affect other plugins that are hooked into the authentication processes.

I won't be able to test this, or properly review it, for awhile though.