Trigger two-factor flow only when expected #660
Conversation
…ly know the user state" This reverts commit 0e1b244.
| * | ||
| * Run after core username/password and cookie checks. | ||
| */ | ||
| add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate' ), 31, 3 ); |
There was a problem hiding this comment.
Priority 31 is still after wp_authenticate_username_password() so the resolved $user will be set correctly.
| __( 'Error: API login for user disabled.', 'two-factor' ) | ||
| ); | ||
| public static function filter_authenticate( $user, $username, $password ) { | ||
| if ( strlen( $username ) && $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) ) { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Trigger the two-factor flow only for login attempts. | ||
| add_action( 'wp_login', array( __CLASS__, 'wp_login' ), PHP_INT_MAX, 2 ); |
There was a problem hiding this comment.
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.
|
@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. |
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 |
|
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 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. |
|
@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-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? |
|
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 :-) |
What?
Avoid triggering the two-factor logic on every
wp_loginhook even when no login attempt is expected.Why?
Fixes #659, #592.
How?
WP core uses the following filters during the
wp-login.phprequest in the order of priority:We want to trigger the two-factor workflow only for requests that:
So we hook right after
wp_authenticate_cookiepriority 30.Secondly, to allow other plugins from utilizing the
wp_loginaction, we move our callback to priorityPHP_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 duringauthenticate.I used https://wpdirectory.net/search/01JH7SBWRMX2F41V0G9W19WPHE to find the top 100 most popular plugins using the
wp_loginaction and none of them appear to do anything that would disable the updated two-factor flow logic.Testing Instructions
wp-login.phpand 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.