From 896c03b81e93b619881e4b10d6dfef934c55171a Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Fri, 9 Nov 2018 10:16:48 -0800 Subject: [PATCH 1/3] Change login redirect to proper action; tests --- WP_Auth0.php | 12 --- lib/WP_Auth0_LoginManager.php | 23 +++- tests/bootstrap.php | 3 +- tests/testLoginManager.php | 185 ++++++++++++++++++++++++++++++-- tests/testRenderForm.php | 78 ++------------ tests/traits/optionsHelpers.php | 33 ++++++ 6 files changed, 242 insertions(+), 92 deletions(-) create mode 100644 tests/traits/optionsHelpers.php diff --git a/WP_Auth0.php b/WP_Auth0.php index e314210e..87293aa7 100644 --- a/WP_Auth0.php +++ b/WP_Auth0.php @@ -372,18 +372,6 @@ public function render_auth0_login_css() { return; } - // If the user has a WP session, determine where they should end up and redirect. - if ( is_user_logged_in() ) { - $login_redirect = empty( $_REQUEST['redirect_to'] ) ? - $this->a0_options->get( 'default_login_redirection' ) : - filter_var( $_REQUEST['redirect_to'], FILTER_SANITIZE_URL ); - - // Add a cache buster to avoid an infinite redirect loop on pages that check for auth. - $login_redirect = add_query_arg( time(), '', $login_redirect ); - wp_safe_redirect( $login_redirect ); - exit; - } - wp_enqueue_style( 'auth0', WPA0_PLUGIN_CSS_URL . 'login.css', false, WPA0_VERSION ); } diff --git a/lib/WP_Auth0_LoginManager.php b/lib/WP_Auth0_LoginManager.php index a4323273..40b531ca 100755 --- a/lib/WP_Auth0_LoginManager.php +++ b/lib/WP_Auth0_LoginManager.php @@ -96,9 +96,25 @@ public function init() { } /** - * Redirect to a specific connection designated in Settings > Advanced + * Redirect logged-in users from wp-login.php. + * Redirect to Universal Login Page under certain conditions and if the option is turned on. + * + * @return bool */ public function login_auto() { + + // If the user has a WP session, determine where they should end up and redirect. + if ( is_user_logged_in() ) { + $login_redirect = empty( $_REQUEST['redirect_to'] ) ? + $this->a0_options->get( 'default_login_redirection' ) : + filter_var( $_REQUEST['redirect_to'], FILTER_SANITIZE_URL ); + + // Add a cache buster to avoid an infinite redirect loop on pages that check for auth. + $login_redirect = add_query_arg( time(), '', $login_redirect ); + wp_safe_redirect( $login_redirect ); + exit; + } + if ( // Nothing to do. ( ! $this->a0_options->get( 'auto_login', false ) ) @@ -111,11 +127,10 @@ public function login_auto() { // Do not redirect log out action. || ( isset( $_GET['action'] ) && 'logout' === $_GET['action'] ) // Do not redirect Auth0 login processing. + // TODO: This should be removed as this page is no longer used for callbacks. || null !== $this->query_vars( 'auth0' ) - // Do not redirect if already authenticated. - || is_user_logged_in() ) { - return; + return false; } $connection = apply_filters( 'auth0_get_auto_login_connection', $this->a0_options->get( 'auto_login_method' ) ); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index d116e52f..fdb88a4c 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -39,12 +39,13 @@ function _manually_load_plugin() { require dirname( __FILE__ ) . '/../vendor/autoload.php'; +// TODO: Move these to an autoloader. require dirname( __FILE__ ) . '/classes/Test_WP_Auth0_Api_Abstract.php'; - require dirname( __FILE__ ) . '/traits/ajaxHelpers.php'; require dirname( __FILE__ ) . '/traits/domDocumentHelpers.php'; require dirname( __FILE__ ) . '/traits/hookHelpers.php'; require dirname( __FILE__ ) . '/traits/httpHelpers.php'; +require dirname( __FILE__ ) . '/traits/optionsHelpers.php'; require dirname( __FILE__ ) . '/traits/redirectHelpers.php'; require dirname( __FILE__ ) . '/traits/setUpTestDb.php'; require dirname( __FILE__ ) . '/traits/usersHelper.php'; diff --git a/tests/testLoginManager.php b/tests/testLoginManager.php index 03583ab5..e067f39e 100644 --- a/tests/testLoginManager.php +++ b/tests/testLoginManager.php @@ -14,7 +14,50 @@ */ class TestLoginManager extends TestCase { - use setUpTestDb; + use OptionsHelpers; + + use RedirectHelpers; + + use SetUpTestDb; + + use UsersHelper; + + /** + * WP_Auth0_ErrorLog instance. + * + * @var WP_Auth0_ErrorLog + */ + protected static $error_log; + + /** + * WP_Auth0_UsersRepo instance. + * + * @var WP_Auth0_UsersRepo + */ + protected static $users_repo; + + /** + * Setup for entire test class. + */ + public static function setUpBeforeClass() { + parent::setUpBeforeClass(); + self::$opts = WP_Auth0_Options::Instance(); + self::$users_repo = new WP_Auth0_UsersRepo( self::$opts ); + self::$error_log = new WP_Auth0_ErrorLog(); + } + + /** + * Run after each test. + */ + public function tearDown() { + parent::tearDown(); + self::auth0Ready( false ); + $this->stopRedirectHalting(); + self::$error_log->clear(); + self::$opts->set( 'custom_domain', '' ); + self::$opts->set( 'auth0_implicit_workflow', false ); + self::$opts->set( 'auto_login', 0 ); + } /** * Test that the default auth scopes are returned and filtered properly. @@ -54,13 +97,12 @@ public function testAuthorizeParams() { $auth_params = WP_Auth0_LoginManager::get_authorize_params( $test_connection ); $this->assertEquals( $test_connection, $auth_params['connection'] ); - $options = WP_Auth0_Options::Instance(); - $options->set( 'client_id', $test_client_id ); + self::$opts->set( 'client_id', $test_client_id ); $auth_params = WP_Auth0_LoginManager::get_authorize_params(); $this->assertEquals( $test_client_id, $auth_params['client_id'] ); - $options->set( 'auth0_implicit_workflow', 1 ); + self::$opts->set( 'auth0_implicit_workflow', 1 ); $auth_params = WP_Auth0_LoginManager::get_authorize_params(); $this->assertEquals( add_query_arg( 'auth0', 'implicit', site_url( 'index.php' ) ), $auth_params['redirect_uri'] ); $this->assertEquals( 'id_token', $auth_params['response_type'] ); @@ -85,17 +127,15 @@ function( $params, $connection, $redirect_to ) { * Test that the authorize URL is built properly. */ public function testBuildAuthorizeUrl() { - $options = WP_Auth0_Options::Instance(); // Basic authorize URL. - $options->set( 'domain', 'test.auth0.com' ); - $options->set( 'custom_domain', '' ); + self::$opts->set( 'domain', 'test.auth0.com' ); $auth_url = WP_Auth0_LoginManager::build_authorize_url(); $this->assertEquals( 'https://test.auth0.com/authorize', $auth_url ); // Custom domain authorize URL. - $options->set( 'custom_domain', 'test-custom.auth0.com' ); + self::$opts->set( 'custom_domain', 'test-custom.auth0.com' ); $auth_url = WP_Auth0_LoginManager::build_authorize_url(); $this->assertEquals( 'https://test-custom.auth0.com/authorize', $auth_url ); @@ -133,4 +173,133 @@ function ( $auth_url, $params ) { ); $this->assertEquals( 'https://test-custom.auth0.com/authorize?test=this', $auth_url ); } + + /** + * Test that logged-in users are redirected from the wp-login.php page. + */ + public function testThatLoggedInUserIsRedirectedFromWpLogin() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + + // Configure Auth0. + self::auth0Ready(); + $this->assertTrue( WP_Auth0::ready() ); + + // Set the current user to admin. + $this->setGlobalUser(); + + // Use the default login redirection. + $caught_redirect = [ + 'location' => null, + 'status' => null, + ]; + try { + $login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + + // Redirect will have a dynamic cache breaker so cannot assertEquals here. + $this->assertNotFalse( strpos( $caught_redirect['location'], 'http://example.org?' ) ); + $this->assertEquals( 302, $caught_redirect['status'] ); + + // Set a one-time login redirect URL. + $_REQUEST['redirect_to'] = 'http://example.org/custom'; + + // Use the default login redirection. + $caught_redirect = [ + 'location' => null, + 'status' => null, + ]; + try { + $login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + + // Redirect will have a dynamic cache breaker so cannot assertEquals here. + $this->assertNotFalse( strpos( $caught_redirect['location'], 'http://example.org/custom?' ) ); + $this->assertEquals( 302, $caught_redirect['status'] ); + } + + /** + * Test that the ULP redirect happens. + */ + public function testUlpRedirect() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + $this->assertFalse( $login_manager->login_auto() ); + + // Activate settings that result in a ULP redirect. + self::$opts->set( 'auto_login', 1 ); + self::auth0Ready( true ); + self::$opts->set( 'domain', 'test-wp.auth0.com' ); + $this->assertTrue( WP_Auth0::ready() ); + + $caught_redirect = [ + 'location' => null, + 'status' => null, + ]; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + + $this->assertNotFalse( strpos( $caught_redirect['location'], 'https://test-wp.auth0.com/authorize?' ) ); + $this->assertEquals( 302, $caught_redirect['status'] ); + } + + /** + * Test that the ULP redirect does not happen under certain conditions. + */ + public function testThatUlpRedirectDoesNotOccur() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + $this->assertFalse( $login_manager->login_auto() ); + + // First, check that a redirect is happening. + self::$opts->set( 'auto_login', 1 ); + self::auth0Ready( true ); + $this->assertTrue( WP_Auth0::ready() ); + + $caught_redirect = [ + 'location' => null, + 'status' => null, + ]; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + $this->assertEquals( 302, $caught_redirect['status'] ); + + // Test that request method will stop redirect. + $req_method_backup = $_SERVER['REQUEST_METHOD']; + $_SERVER['REQUEST_METHOD'] = 'POST'; + $this->assertFalse( $login_manager->login_auto() ); + $_SERVER['REQUEST_METHOD'] = $req_method_backup; + + // Test that WP login override will skip the redirect. + $_GET['wle'] = 1; + $this->assertFalse( $login_manager->login_auto() ); + unset( $_GET['wle'] ); + + // Test that logout will skip the redirect. + $_GET['action'] = 'logout'; + $this->assertFalse( $login_manager->login_auto() ); + unset( $_GET['action'] ); + + // Test that the auth0 URL param will skip the redirect. + $_REQUEST['auth0'] = 1; + $this->assertFalse( $login_manager->login_auto() ); + unset( $_REQUEST['auth0'] ); + } } diff --git a/tests/testRenderForm.php b/tests/testRenderForm.php index ceec420b..1d0d6df0 100644 --- a/tests/testRenderForm.php +++ b/tests/testRenderForm.php @@ -15,10 +15,12 @@ */ class TestRenderForm extends TestCase { - use SetUpTestDb; + use OptionsHelpers; use RedirectHelpers; + use SetUpTestDb; + use UsersHelper; /** @@ -28,13 +30,6 @@ class TestRenderForm extends TestCase { */ public static $wp_auth0; - /** - * WP_Auth0_Options instance. - * - * @var WP_Auth0_Options - */ - public static $opts; - /** * Initial HTML value for render_form filter value. * @@ -51,6 +46,14 @@ public static function setUpBeforeClass() { self::$wp_auth0 = new WP_Auth0( self::$opts ); } + /** + * Run after each test. + */ + public function tearDown() { + parent::tearDown(); + self::auth0Ready( false ); + } + /** * Test that a specific region and domain return the correct number of IP addresses. */ @@ -87,63 +90,4 @@ public function testThatFormRendersWhenAuth0IsReady() { $this->assertContains( 'auth0-login-form', self::$wp_auth0->render_form( self::$html ) ); } - - /** - * Test that a specific region and domain return the correct number of IP addresses. - */ - public function testThatLoggedInUserIsRedirected() { - $this->startRedirectHalting(); - - // Configure Auth0. - self::auth0Ready(); - $this->assertTrue( WP_Auth0::ready() ); - - $this->assertContains( 'auth0-login-form', self::$wp_auth0->render_form( self::$html ) ); - - // Set the current user to admin. - $this->setGlobalUser(); - - // Use the default login redirection. - $caught_exception = false; - try { - self::$wp_auth0->render_auth0_login_css(); - } catch ( Exception $e ) { - $err_msg = unserialize( $e->getMessage() ); - $caught_exception = 0 === strpos( $err_msg['location'], 'http://example.org' ) && 302 === $err_msg['status']; - } - $this->assertTrue( $caught_exception ); - - // Set a login redirect URL. - $_REQUEST['redirect_to'] = 'http://example.org/custom'; - - $caught_exception = false; - try { - self::$wp_auth0->render_auth0_login_css(); - } catch ( Exception $e ) { - $err_msg = unserialize( $e->getMessage() ); - $caught_exception = 0 === strpos( $err_msg['location'], $_REQUEST['redirect_to'] ) && 302 === $err_msg['status']; - } - $this->assertTrue( $caught_exception ); - } - - /** - * Set the Auth0 plugin settings. - * - * @param boolean $on - True to turn Auth0 on, false to turn off. - */ - public static function auth0Ready( $on = true ) { - $value = $on ? uniqid() : null; - self::$opts->set( 'domain', $value ); - self::$opts->set( 'client_id', $value ); - self::$opts->set( 'client_secret', $value ); - } - - /** - * Run after each test. - */ - public function tearDown() { - parent::tearDown(); - self::auth0Ready( false ); - $this->stopRedirectHalting(); - } } diff --git a/tests/traits/optionsHelpers.php b/tests/traits/optionsHelpers.php new file mode 100644 index 00000000..915f910b --- /dev/null +++ b/tests/traits/optionsHelpers.php @@ -0,0 +1,33 @@ +set( 'domain', $value ); + self::$opts->set( 'client_id', $value ); + self::$opts->set( 'client_secret', $value ); + } +} From 75d4ed1acdc31815183c5efc659be4b126e4cd94 Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Fri, 9 Nov 2018 11:50:02 -0800 Subject: [PATCH 2/3] PR feedback --- tests/testLoginManager.php | 89 ++++++++++++++++++++++++++++++++------ 1 file changed, 75 insertions(+), 14 deletions(-) diff --git a/tests/testLoginManager.php b/tests/testLoginManager.php index e067f39e..0fdac548 100644 --- a/tests/testLoginManager.php +++ b/tests/testLoginManager.php @@ -255,23 +255,17 @@ public function testUlpRedirect() { } /** - * Test that the ULP redirect does not happen under certain conditions. + * Test that the ULP redirect does not happen for non-GET methods. */ - public function testThatUlpRedirectDoesNotOccur() { + public function testThatUlpRedirectIsSkippedForNonGetMethod() { $this->startRedirectHalting(); $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); - $this->assertFalse( $login_manager->login_auto() ); // First, check that a redirect is happening. self::$opts->set( 'auto_login', 1 ); self::auth0Ready( true ); - $this->assertTrue( WP_Auth0::ready() ); - - $caught_redirect = [ - 'location' => null, - 'status' => null, - ]; + $caught_redirect = []; try { // Need to hide error messages here because a cookie is set. // phpcs:ignore @@ -282,24 +276,91 @@ public function testThatUlpRedirectDoesNotOccur() { $this->assertEquals( 302, $caught_redirect['status'] ); // Test that request method will stop redirect. - $req_method_backup = $_SERVER['REQUEST_METHOD']; $_SERVER['REQUEST_METHOD'] = 'POST'; $this->assertFalse( $login_manager->login_auto() ); - $_SERVER['REQUEST_METHOD'] = $req_method_backup; + + $_SERVER['REQUEST_METHOD'] = 'PATCH'; + $this->assertFalse( $login_manager->login_auto() ); + } + + /** + * Test that the ULP redirect does not happen if we're loading the WP login form. + */ + public function testThatUlpRedirectIsSkippedForWleOverride() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + $this->assertFalse( $login_manager->login_auto() ); + + // First, check that a redirect is happening. + self::$opts->set( 'auto_login', 1 ); + self::auth0Ready( true ); + $caught_redirect = []; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + $this->assertEquals( 302, $caught_redirect['status'] ); // Test that WP login override will skip the redirect. $_GET['wle'] = 1; $this->assertFalse( $login_manager->login_auto() ); - unset( $_GET['wle'] ); + } + + /** + * Test that the ULP redirect does not happen is this is a logout action. + */ + public function testThatUlpRedirectIsSkippedForLogout() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + $this->assertFalse( $login_manager->login_auto() ); + + // First, check that a redirect is happening. + self::$opts->set( 'auto_login', 1 ); + self::auth0Ready( true ); + $caught_redirect = []; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + $this->assertEquals( 302, $caught_redirect['status'] ); // Test that logout will skip the redirect. $_GET['action'] = 'logout'; $this->assertFalse( $login_manager->login_auto() ); - unset( $_GET['action'] ); + } + + /** + * Test that the ULP redirect does not happen if wp-login.php is used as a callback. + */ + public function testThatUlpRedirectIsSkippedForCallback() { + $this->startRedirectHalting(); + + $login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts ); + $this->assertFalse( $login_manager->login_auto() ); + + // First, check that a redirect is happening. + self::$opts->set( 'auto_login', 1 ); + self::auth0Ready( true ); + $caught_redirect = []; + try { + // Need to hide error messages here because a cookie is set. + // phpcs:ignore + @$login_manager->login_auto(); + } catch ( Exception $e ) { + $caught_redirect = unserialize( $e->getMessage() ); + } + $this->assertEquals( 302, $caught_redirect['status'] ); // Test that the auth0 URL param will skip the redirect. $_REQUEST['auth0'] = 1; $this->assertFalse( $login_manager->login_auto() ); - unset( $_REQUEST['auth0'] ); } } From d534ae850fb543382f13bdf499ea749f7bc3746b Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Fri, 9 Nov 2018 12:10:02 -0800 Subject: [PATCH 3/3] Refactor login_auto for clarity --- lib/WP_Auth0_LoginManager.php | 38 +++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/lib/WP_Auth0_LoginManager.php b/lib/WP_Auth0_LoginManager.php index 40b531ca..1352a650 100755 --- a/lib/WP_Auth0_LoginManager.php +++ b/lib/WP_Auth0_LoginManager.php @@ -103,6 +103,27 @@ public function init() { */ public function login_auto() { + // Do not redirect anywhere if this is a logout action. + if ( isset( $_GET['action'] ) && 'logout' === $_GET['action'] ) { + return false; + } + + // Do not redirect login page override. + if ( isset( $_GET['wle'] ) ) { + return false; + } + + // Do not redirect non-GET requests. + if ( strtolower( $_SERVER['REQUEST_METHOD'] ) !== 'get' ) { + return false; + } + + // Do not redirect Auth0 login processing. + // TODO: This should be removed as this page is no longer used for callbacks. + if ( null !== $this->query_vars( 'auth0' ) ) { + return false; + } + // If the user has a WP session, determine where they should end up and redirect. if ( is_user_logged_in() ) { $login_redirect = empty( $_REQUEST['redirect_to'] ) ? @@ -115,21 +136,8 @@ public function login_auto() { exit; } - if ( - // Nothing to do. - ( ! $this->a0_options->get( 'auto_login', false ) ) - // Auth0 is not ready to process logins. - || ! WP_Auth0::ready() - // Do not redirect POST requests. - || strtolower( $_SERVER['REQUEST_METHOD'] ) !== 'get' - // Do not redirect login page override. - || isset( $_GET['wle'] ) - // Do not redirect log out action. - || ( isset( $_GET['action'] ) && 'logout' === $_GET['action'] ) - // Do not redirect Auth0 login processing. - // TODO: This should be removed as this page is no longer used for callbacks. - || null !== $this->query_vars( 'auth0' ) - ) { + // Do not use the ULP if the setting is off or if the plugin is not configured. + if ( ! $this->a0_options->get( 'auto_login', false ) || ! WP_Auth0::ready() ) { return false; }