Logout rather than 2FA when Duo session expires (#34)

* Move logout outside of start second factor call
duosecurity · Nov 9, 2023 · a744207 · a744207
1 parent 3a1b9bd
commit a744207
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 17 deletions.
diff --git a/class-duouniversal-wordpressplugin.php b/class-duouniversal-wordpressplugin.php
@@ -124,9 +124,6 @@ function duo_start_second_factor( $user ) {
 		$redirect_url                   = $this->get_page_url();
 		$this->duo_client->redirect_url = $redirect_url;
 
-		// logging out clears cookies and transients so it should be done _before_ updating
-		// the auth status
-		\wp_logout();
 		$this->update_user_auth_status( $user->user_login, 'in-progress', $redirect_url, $oidc_state );
 
 		$prompt_uri = $this->duo_client->createAuthUrl( $user->user_login, $oidc_state );
@@ -232,6 +229,9 @@ function duo_authenticate_user( $user = '', $username = '', $password = '' ) {
 
 			$this->update_user_auth_status( $user->user_login, 'in-progress' );
 			try {
+				// logging out clears cookies and transients so it should be done _before_ updating
+				// the auth status
+				\wp_logout();
 				$this->duo_start_second_factor( $user );
 			} catch ( Duo\DuoUniversal\DuoException $e ) {
 				$this->duo_debug_log( $e->getMessage() );
@@ -259,7 +259,6 @@ function duo_authenticate_user( $user = '', $username = '', $password = '' ) {
 	 */
 	function duo_verify_auth() {
 		if ( ! $this->duo_utils->duo_auth_enabled() ) {
-			// XXX do we still need this skipping logic?
 			if ( \is_multisite() ) {
 				$site_info = \get_current_site();
 				$this->duo_debug_log( 'Duo not enabled on ' . $site_info->site_name );
@@ -273,8 +272,9 @@ function duo_verify_auth() {
 			$user = \wp_get_current_user();
 			$this->duo_debug_log( "Verifying auth state for user: $user->user_login" );
 			if ( $this->duo_utils->duo_role_require_mfa( $user ) && ! $this->duo_verify_auth_status( $user->user_login ) ) {
-				$this->duo_debug_log( "User not authenticated with Duo. Starting second factor for: $user->user_login" );
-				$this->duo_start_second_factor( $user );
+				\wp_logout();
+				wp_redirect( wp_login_url() );
+				$this->exit();
 			}
 			$this->duo_debug_log( "User $user->user_login allowed" );
 		}

diff --git a/tests/duoUniversalAuthenticationTest.php b/tests/duoUniversalAuthenticationTest.php
@@ -96,7 +96,6 @@ function testStartSecondFactorRedirectURL(): void
         $authentication->method('get_page_url')->willReturn('fake url');
         WP_Mock::passthruFunction('wp_redirect');
         WP_Mock::passthruFunction('set_transient');
-        WP_Mock::userFunction('wp_logout')->once();
 
         $authentication->duo_start_second_factor($user);
 
@@ -121,9 +120,6 @@ function testDoubleLoginTransients(): void
             $transients[$name] = $value;
             return;
         }]);
-        WP_Mock::userFunction('wp_logout', [ 'return' => function() use (&$transients){
-            $transients = [];
-        }])->once();
 
         $authentication->duo_start_second_factor($user);
 
@@ -145,7 +141,6 @@ function testPromptRedirect(): void
             ->onlyMethods(['get_page_url', 'exit'])
             ->getMock();
         WP_Mock::passthruFunction('set_transient');
-        WP_Mock::userFunction('wp_logout')->once();
 
         $authentication->duo_start_second_factor($user);
         $this->assertConditionsMet();
@@ -169,7 +164,6 @@ function testStartSecondFactorTransients(): void
         $authentication->method('get_page_url')->willReturn("test url");
         WP_Mock::passthruFunction('wp_redirect');
         WP_Mock::userFunction('set_transient', ['return' => $callback]);
-        WP_Mock::userFunction('wp_logout')->once();
         $this->duo_client->method('generateState')->willReturn("test state");
 
         $authentication->duo_start_second_factor($user);
@@ -191,7 +185,6 @@ function testStartSecondFactorLogout(): void
             ->setConstructorArgs(array($this->duo_utils, $this->duo_client))
             ->onlyMethods(['get_page_url', 'exit'])
             ->getMock();
-        WP_Mock::userFunction('wp_logout')->once();
         WP_Mock::passthruFunction('set_transient');
         WP_Mock::passthruFunction('wp_redirect');
 
@@ -213,7 +206,6 @@ function testStartSecondFactorExit(): void
         $authentication->expects($this->once())->method('exit');
         WP_Mock::passthruFunction('set_transient');
         WP_Mock::passthruFunction('wp_redirect');
-        WP_Mock::userFunction('wp_logout')->once();
 
         $authentication->duo_start_second_factor($user);
     }
@@ -572,6 +564,7 @@ function testAuthUserPrimaryUpdatesAuthStatus(): void
             $map[$key] = $value;
         };
         WP_Mock::userFunction('set_transient', ['return' => $callback]);
+        WP_Mock::userFunction('wp_logout')->once();
         $authentication = $this->getMockBuilder(DuoUniversal_WordpressPlugin::class)
             ->setConstructorArgs(array($this->duo_utils, $this->duo_client))
             ->onlyMethods(
@@ -626,6 +619,7 @@ function testAuthUserSecondaryExceptionFailmodeOpen(): void
         $this->duo_utils->method('new_WP_user')->willReturn($user);
         WP_Mock::userFunction('wp_authenticate_username_password', [ 'return' => $user ]);
         WP_Mock::passthruFunction('remove_action');
+        WP_Mock::userFunction('wp_logout')->once();
         $this->duo_utils->method('duo_get_option')->willReturn('open');
 
         $result = $authentication->duo_authenticate_user(null, "test user");
@@ -672,6 +666,7 @@ function testAuthUserSecondaryExceptionFailmodeClose(): void
         WP_Mock::passthruFunction('__');
         WP_Mock::passthruFunction('remove_action');
         WP_Mock::userFunction('wp_authenticate_username_password', [ 'return' => $user ]);
+        WP_Mock::userFunction('wp_logout')->once();
         $this->duo_utils->method('duo_get_option')->willReturn('closed');
 
         $result = $authentication->duo_authenticate_user(null, "test user");
@@ -824,19 +819,22 @@ function testVerifyAuthNeeds2FA(): void
             ->setConstructorArgs(array($this->duo_utils, $this->duo_client))
             ->onlyMethods(
                 [
-                'duo_debug_log', 'duo_verify_auth_status', 'duo_start_second_factor',
+                'duo_debug_log', 'duo_verify_auth_status', 'duo_start_second_factor', 'exit'
                 ]
             )
             ->getMock();
         $this->duo_utils->method('duo_auth_enabled')->willReturn(true);
         WP_Mock::userFunction('is_user_logged_in', [ 'return' => true ]);
         WP_Mock::userFunction('wp_get_current_user', [ 'return' => $user ]);
+        WP_Mock::userFunction('wp_logout')->once();
+        WP_Mock::userFunction('wp_redirect')->once();
+        WP_Mock::userFunction('wp_login_url')->once();
         $this->duo_utils->method('duo_role_require_mfa')->willReturn(true);
         $authentication->method('duo_verify_auth_status')->willReturn(false);
-        $authentication->expects($this->once())->method('duo_start_second_factor');
+        $authentication->expects($this->once())->method('exit');
 
         $result = $authentication->duo_verify_auth();
 
-        $this->assertEquals($result, null);
+        $this->assertConditionsMet();
     }
 }