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]

@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.

[jeffpaul]

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.

Bah, that's not great. The approach in this PR does seem more like the "right" way of doing things, though impacting how others might be extending the work of the plugin is not great. Perhaps we hold this for 0.13.0 and message in the 0.12.0 release that this change is coming and to look into this PR specifically for anyone concerned with the change planned for 0.13.0?

[dd32]

I want to suggest it's a safe change, but without testing it I can't really confirm, it's just that the entire login flow is rather weird and ill-designed for this use-case while it preserves back-compat.. I've run into weirdness a multitude of times both where I've said Why is two-factor working like this?! and What? Why is Core.. wheres the filter.. ugh fine..

I'm not a fan of putting any form of warning in release notes for future changes, because the people who would need to see it never do. I'd just make the change.

[kasparsd]

@dd32 Would it be possible for you to test this on any kind of environment?

The only regression we're trying to avoid is the complete lack of second factor workflow. Honestly, I'm not even sure what kind of setup could trigger that because as shown in the example above -- any integration doing something strange during wp_login will simply disable ALL login because we're silencing the login cookies during authenticate.

WP-org and WPVIP are probably our two largest users so any testing on either of implementations would be helpful.

@sjinks Could you help with getting this tested on a baseline VIP setup?

[sjinks]

It's not that easy, but I'll see what I can do. The issue is that we have an ancient version of Two Factor @ VIP. We do have plans to update, though :-)