diff --git a/class-two-factor-core.php b/class-two-factor-core.php index 61179cd9..2f140a97 100644 --- a/class-two-factor-core.php +++ b/class-two-factor-core.php @@ -94,7 +94,6 @@ class Two_Factor_Core { */ public static function add_hooks( $compat ) { add_action( 'init', array( __CLASS__, 'get_providers' ) ); // @phpstan-ignore return.void - add_action( 'wp_login', array( __CLASS__, 'wp_login' ), 10, 2 ); add_filter( 'wp_login_errors', array( __CLASS__, 'maybe_show_reset_password_notice' ) ); add_action( 'after_password_reset', array( __CLASS__, 'clear_password_reset_notice' ) ); add_action( 'login_form_validate_2fa', array( __CLASS__, 'login_form_validate_2fa' ) ); @@ -117,11 +116,13 @@ public static function add_hooks( $compat ) { add_action( 'set_auth_cookie', array( __CLASS__, 'collect_auth_cookie_tokens' ) ); add_action( 'set_logged_in_cookie', array( __CLASS__, 'collect_auth_cookie_tokens' ) ); - // Run only after the core wp_authenticate_username_password() check. - add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate' ), 50 ); - - // Run as late as possible to prevent other plugins from unintentionally bypassing. - add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate_block_cookies' ), PHP_INT_MAX ); + /** + * Enable the two-factor workflow only for login requests with username + * and without an existing user cookie. + * + * Run after core username/password and cookie checks. + */ + add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate' ), 31, 3 ); add_filter( 'attach_session_information', array( __CLASS__, 'filter_session_information' ), 10, 2 ); @@ -673,44 +674,33 @@ public static function destroy_current_session_for_user( $user ) { } /** - * Prevent login through XML-RPC and REST API for users with at least one - * two-factor method enabled. + * Trigget the two-factor workflow only for valid login attempts + * with username present. Prevent authentication during API requests + * unless explicitly enabled for the user (disabled by default). * - * @param WP_User|WP_Error $user Valid WP_User only if the previous filters + * @param WP_User|WP_Error $user Valid WP_User only if the previous filters * have verified and confirmed the * authentication credentials. + * @param string $username The username. + * @param string $password The password. * * @return WP_User|WP_Error */ - public static function filter_authenticate( $user ) { - if ( $user instanceof WP_User && self::is_api_request() && self::is_user_using_two_factor( $user->ID ) && ! self::is_user_api_login_enabled( $user->ID ) ) { - return new WP_Error( - 'invalid_application_credentials', - __( 'Error: API login for user disabled.', 'two-factor' ) - ); - } - - return $user; - } + public static function filter_authenticate( $user, $username, $password ) { + if ( strlen( $username ) && $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) ) { + // Disable authentication requests for API requests for users with two-factor enabled. + if ( self::is_api_request() && ! self::is_user_api_login_enabled( $user->ID ) ) { + return new WP_Error( + 'invalid_application_credentials', + __( 'Error: API login for user disabled.', 'two-factor' ) + ); + } - /** - * Prevent login cookies being set on login for Two Factor users. - * - * This makes it so that Core never sends the auth cookies. `login_form_validate_2fa()` will send them manually once the 2nd factor has been verified. - * - * @param WP_User|WP_Error $user Valid WP_User only if the previous filters - * have verified and confirmed the - * authentication credentials. - * - * @return WP_User|WP_Error - */ - public static function filter_authenticate_block_cookies( $user ) { - /* - * NOTE: The `login_init` action is checked for here to ensure we're within the regular login flow, - * rather than through an unsupported 3rd-party login process which this plugin doesn't support. - */ - if ( $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) && did_action( 'login_init' ) ) { + // Disable core auth cookies because we must send them manually once the 2nd factor has been verified. add_filter( 'send_auth_cookies', '__return_false', PHP_INT_MAX ); + + // Trigger the two-factor flow only for login attempts. + add_action( 'wp_login', array( __CLASS__, 'wp_login' ), PHP_INT_MAX, 2 ); } return $user; @@ -1390,7 +1380,7 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider = /* * NOTE: This filter removal is not normally required, this is included for protection against * a plugin/two factor provider which runs the `authenticate` filter during it's validation. - * Such a plugin would cause self::filter_authenticate_block_cookies() to run and add this filter. + * Such a plugin would cause self::filter_authenticate() to run and add this filter. */ remove_filter( 'send_auth_cookies', '__return_false', PHP_INT_MAX ); diff --git a/tests/class-two-factor-core.php b/tests/class-two-factor-core.php index 992ddc60..5c02c689 100644 --- a/tests/class-two-factor-core.php +++ b/tests/class-two-factor-core.php @@ -392,17 +392,74 @@ public function test_filter_authenticate() { $user_default = new WP_User( self::factory()->user->create() ); $user_2fa_enabled = $this->get_dummy_user(); // User with a dummy two-factor method enabled. + $this->assertFalse( Two_Factor_Core::is_api_request(), 'Is not an API request by default' ); + + $this->assertInstanceOf( + 'WP_User', + Two_Factor_Core::filter_authenticate( $user_default, '', '' ), + 'Existing non-2FA user session should not trigger 2FA' + ); + + $this->assertInstanceOf( + 'WP_User', + Two_Factor_Core::filter_authenticate( $user_default, 'username', '' ), + 'Existing non-2FA user login attempts should not trigger 2FA' + ); + + $this->assertInstanceOf( + 'WP_User', + Two_Factor_Core::filter_authenticate( $user_2fa_enabled, '', '' ), + 'Existing 2FA user sessions should not trigger 2FA' + ); + + $this->assertFalse( + has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), + 'Requests with existing user sessions should not trigger the two-factor flow' + ); + + $this->assertInstanceOf( + 'WP_User', + Two_Factor_Core::filter_authenticate( $user_2fa_enabled, 'user-name', 'password' ), + 'Existing 2FA user session with username present should forward the user' + ); + + $this->assertNotFalse( + has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), + 'Existing 2FA user session with username present should trigger two-factor flow' + ); + } + + /** + * Verify authentication filters. + * + * @covers Two_Factor_Core::filter_authenticate + * @covers Two_Factor_Core::is_api_request + */ + public function test_filter_authenticate_api() { + $user_default = new WP_User( self::factory()->user->create() ); + $user_2fa_enabled = $this->get_dummy_user(); // User with a dummy two-factor method enabled. + // TODO: Get Two_Factor_Core away from static methods to allow mocking this. define( 'XMLRPC_REQUEST', true ); + $this->assertTrue( Two_Factor_Core::is_api_request(), 'Can detect an API request' ); + $this->assertInstanceOf( 'WP_User', - Two_Factor_Core::filter_authenticate( $user_default ) + Two_Factor_Core::filter_authenticate( $user_default, 'username', 'password' ), + 'Non-2FA user should be able to authenticate during API requests' ); $this->assertInstanceOf( 'WP_Error', - Two_Factor_Core::filter_authenticate( $user_2fa_enabled ) + Two_Factor_Core::filter_authenticate( $user_2fa_enabled, 'username', 'password' ), + '2FA user should not be able to authenticate during API requests' + ); + + $this->assertInstanceOf( + 'WP_User', + Two_Factor_Core::filter_authenticate( $user_2fa_enabled, '', null ), + 'Existing user session without a username should not trigger 2FA' ); }