Using auth_cookie filter instead of wp_login hook to start 2FA flow

[nathanrona]

Currently flow is started when wp_login is triggered, i.e. when the user has already been logged in, and then reversers the last part of the the default login-flow, by removing the the auth-cookie in function wp_login.

public static function wp_login( $user_login, $user ) {
		if ( ! self::is_user_using_two_factor( $user->ID ) ) {
			return;
		}

		// Invalidate the current login session to prevent from being re-used.
		self::destroy_current_session_for_user( $user );

		// Also clear the cookies which are no longer valid.
		wp_clear_auth_cookie();

		self::show_two_factor_login( $user );
		exit;
}

Why don't instead use the hook auth_cookie filter, to prevent the cookie from being set unit 2FA has been completed?

Or use wp_authenticate action hook that is triggered before the WP backend authentication process is done, removing need to destroy the session
I think that use of wp_login hook, in addition to being somewhat backward, as already completed login is reversed, is more likely to conflict with other hooks in sites that seek to do actions after successful login.

Ref:
https://usersinsights.com/wordpress-user-login-hooks/
https://developer.wordpress.org/reference/hooks/auth_cookie/
https://developer.wordpress.org/reference/hooks/wp_authenticate/

[kkmuffme]

Could you create a PR?

[dd32]

I don't think using the authenticate or auth_cookie filters are very appropriate here, and using those has the potential of breaking other use-cases which has to use those filters.

However, the current method of allowing the cookies to be sent to browser (even if the sessions are destroyed, causing the cookies to be invalid) opens up the possibility for vulnerabilities, especially if anyone is overriding any of the pluggable.php files.

Instead though, we can use the send_auth_cookies filter to prevent sending the authentication cookies in the first place. This would also help with #385 as it would prevent any other plugins from setting auth cookies unless two-factor deems it appropriate to do so. This filter is however in a pluggable functions which means that some sites, if using custom authentication, may not work with two-factor. In a way, if a site is overriding those, there's little we can do IMHO.

The Core filter is designed for unit testing, and so doesn't pass any context, which is a shame. See WordPress/wordpress-develop#3554
As it means we need to keep track of the user_id that the filter is being called for ourselves, not an issue as filters with context are run directly before it.

It's worth noting, that by preventing plugins from accidentally circumventing two-factor, it's also going to block cases where a plugin is deliberately bypassing it and setting cookies, for example, <?php include 'wp-load.php'; wp_set_auth_cookie( 1 ); ?> in a standalone script should not log the two-factor-protected user in.. as a result, we probably want to redirect to the 2fa prompt in that case.

I've experimented with two approaches.

1. Using send_auth_cookies only and removing wp_login but that has a potential vulnerability if the pluggable wp_set_auth_cookie() is replaced without the filter.
2. Continuing to use wp_login but also using send_auth_cookies to either:
   a. Not send the cookies, if the two-factor prompt is going to kick in. OR
   b. Redirect the request to wp-login.php?action=show_2fa to finalise the login flow, if being run outside of login.

Attempt 1 is in master...dd32:two-factor:try/send_auth_cookies and I'm going to submit a PR with the second for review.

[iandunn]

I don't think using the authenticate [...filter is] very appropriate here

Why is that? If we only modify send_auth_cookies, will that leave a gap when other auth methods are used (XML-RPC, HTTP Basic Auth or oAuth w/ the REST API, etc)?

[dd32]

I don't think using the authenticate [...filter is] very appropriate here

Why is that? If we only modify send_auth_cookies, will that leave a gap when other auth methods are used (XML-RPC, HTTP Basic Auth or oAuth w/ the REST API, etc)?

I guess in my mind I see those filters as validating authentication, where as two-factor is validated after authentication succeeds or fails. It's about using the filter for the intention of it, rather than hijacking the request. Admittedly, #490 does just that for cases where wp_set_auth_cookie() is called outside of the login flows, but that's done as a UX-thing rather than just failing to send the cookies entirely.

Other Authentication methods, such as those you listed, need to have explicit handlers that respects the type of authentication. For example, you would not want to exit to a 2FA prompt for an XML-RPC client, instead you would disable authentication through the filter.

[iandunn]

Ah, I was thinking of it more as "the user hasn't fully authenticated until both factors are validated", because the 2nd factor is part of the authentication process. Core doesn't have any concept of multiple factors or custom login steps, so anything we do is gonna be somewhat janky. I can see what you mean, though 👍🏻

I guess #300 covers the concern about other methods, and there isn't a gap currently since it's disabled by default 👍🏻

[dd32]

Based on the above feedback, and limited movement on #490 I've proposed #502 which is a much smaller change and should be easier to review and approve.

[iandunn]

I think this was fixed by #502. Is that right, @dd32 ? If not, please reopen it.