Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger two-factor flow only when expected #660

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
+86 −39
Open

Trigger two-factor flow only when expected #660

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8aef3df
Move to auth trigger
kasparsd Jan 10, 2025
0e1b244
Move to the generic authenticate filter since there we reliably know …
kasparsd Jan 10, 2025
e7f1ad6
Revert "Move to the generic authenticate filter since there we reliab…
kasparsd Jan 10, 2025
7a3d5ed
Move earlier right after the cookie check
kasparsd Jan 10, 2025
0de50df
Fire super late since we’re replacing the rest of login flow
kasparsd Jan 10, 2025
76192e6
Pass the missing args
kasparsd Jan 10, 2025
4eafeed
Add a test case for existing user session
kasparsd Jan 10, 2025
315cf26
Break out for readability
kasparsd Jan 10, 2025
4c2345b
Account for username “0”
kasparsd Jan 10, 2025
2a713ea
Use positive lookup for readability
kasparsd Jan 10, 2025
b9c200d
Describe tests for easier lookup during fails
kasparsd Jan 10, 2025
7cb84f5
Test during API requests
kasparsd Jan 10, 2025
cf0852b
Move the description to the main docblock
kasparsd Jan 10, 2025
2dd1c2f
Test the base flow
kasparsd Jan 10, 2025
89b6829
Add back the password as the hook will provide it
kasparsd Jan 10, 2025
a096066
Skip the cookies when we know we’re doing the two-factor dance
kasparsd Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ) );
Expand All @@ -117,8 +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 );
/**
* 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 );
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Run as late as possible to prevent other plugins from unintentionally bypassing.
add_filter( 'authenticate', array( __CLASS__, 'filter_authenticate_block_cookies' ), PHP_INT_MAX );
Expand Down Expand Up @@ -673,21 +677,30 @@ 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' )
);
public static function filter_authenticate( $user, $username, $password ) {
if ( strlen( $username ) && $user instanceof WP_User && self::is_user_using_two_factor( $user->ID ) ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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' )
);
}

// Trigger the two-factor flow only for login attempts.
add_action( 'wp_login', array( __CLASS__, 'wp_login' ), PHP_INT_MAX, 2 );
Copy link
Collaborator Author

@kasparsd kasparsd Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

no-cookies-on-redirect

}

return $user;
Expand Down
61 changes: 59 additions & 2 deletions tests/class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
);
}

Expand Down
Loading