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

Change logged-in user redirect to login_init hook #584

Merged
merged 3 commits into from
Nov 13, 2018
Merged

Change logged-in user redirect to login_init hook #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
896c03b
Change login redirect to proper action; tests
joshcanhelp Nov 9, 2018
75d4ed1
PR feedback
joshcanhelp Nov 9, 2018
d534ae8
Refactor login_auto for clarity
joshcanhelp Nov 9, 2018
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
12 changes: 0 additions & 12 deletions WP_Auth0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meat of the PR here. This still was not early enough to precede any HTML output, leading to a PHP warning.

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

Expand Down
59 changes: 41 additions & 18 deletions lib/WP_Auth0_LoginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,26 +96,49 @@ 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 (
// 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.
|| null !== $this->query_vars( 'auth0' )
// Do not redirect if already authenticated.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dealing with this condition in the block added above in this method ☝️

|| is_user_logged_in()
) {
return;

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

// 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;
Copy link
Contributor Author

@joshcanhelp joshcanhelp Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is used in a hook call so return value is never used ... except in tests now!

}

$connection = apply_filters( 'auth0_get_auto_login_connection', $this->a0_options->get( 'auto_login_method' ) );
Expand Down
3 changes: 2 additions & 1 deletion tests/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
246 changes: 238 additions & 8 deletions tests/testLoginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

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

this is done once, but its contents are changed multiple times in different tests. I think it should be moved into a method that runs "before each test" so you can ensure the tests are fully independent of each other.

e.g. some tests set the client_id property, some others don't. Independently of if those tests are running methods that use or not that client_id value from the props, if not part of the context then make sure that the props object is always with the values you need for test and nothing else. By doing what I mentioned in the paragraph above you can ensure that those props get reseted on each test run. And each test should prepare their own test context/environment prior to call the "method in test".

Copy link
Contributor Author

@joshcanhelp joshcanhelp Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Totally agree but keep in mind that we always have a "state," which is the database and that's where these are stored. A new options object pulls from the test DB so it needs to be explicitly reset each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also ... WP_Auth0_Options::Instance() returns an existing object with options stored in memory.

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', '' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems easier to throw this instance and create a new one than be changing manually each prop, as you could be forgetting some of them when adding new tests. Note this are tests and performance hit is not relevant here. Instantiate whatever you need the times you need 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. There are a number of setUp/tearDown things that can/should happen always. Plan is to have a super class that extends the test suite to handle everything that should be done in between. I'm logging your suggestion here and the others I have so it gets handled.

self::$opts->set( 'auth0_implicit_workflow', false );
self::$opts->set( 'auto_login', 0 );
}

/**
* Test that the default auth scopes are returned and filtered properly.
Expand Down Expand Up @@ -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'] );
Expand All @@ -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 );
Expand Down Expand Up @@ -133,4 +173,194 @@ 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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from tests/testRenderForm.php 👇

$this->startRedirectHalting();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I didn't fully understand from yesterday's response. Couldn't you call this line at the very end of the "run before each test" method? It's cleaner than having to copy-paste it on every test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaner but not needed for every test. Some of these cannot be combined because they throw exceptions at different places.


$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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set everything up to make sure the ULP redirect happens under normal conditions.

$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 for non-GET methods.
*/
public function testThatUlpRedirectIsSkippedForNonGetMethod() {
$this->startRedirectHalting();

$login_manager = new WP_Auth0_LoginManager( self::$users_repo, self::$opts );

// 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 request method will stop redirect.
$_SERVER['REQUEST_METHOD'] = 'POST';
$this->assertFalse( $login_manager->login_auto() );

$_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.
Copy link
Contributor

Choose a reason for hiding this comment

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

All these 3 checks below should be moved into 3 different tests as the condition for the test changes.

$_GET['wle'] = 1;
$this->assertFalse( $login_manager->login_auto() );
}

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

/**
* 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() );
}
}
Loading