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

State handling during login process for both types #406

Merged
merged 4 commits into from
Mar 12, 2018
Merged

State handling during login process for both types #406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13f2f38
State handling during login process for both types, implicit and redi…
joshcanhelp Mar 2, 2018
3231279
remove stateHandler->issue() from SLO logout footer template to avoid…
joshcanhelp Mar 5, 2018
0fcf435
Review feedback on state handler property
joshcanhelp Mar 5, 2018
18c574c
Better state getting in WP_Auth0_LoginManager
joshcanhelp Mar 8, 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
1 change: 1 addition & 0 deletions WP_Auth0.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ public function a0_register_query_vars( $qvars ) {
$qvars[] = 'a0_action';
$qvars[] = 'auth0';
$qvars[] = 'code';
$qvars[] = 'state';
return $qvars;
}

Expand Down
11 changes: 5 additions & 6 deletions lib/WP_Auth0_Lock10_Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,12 @@ public function modal_button_name() {

public function get_state_obj( $redirect_to = null ) {

if ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ) {
$interim_login = true;
} else {
$interim_login = false;
}
$stateHandler = new WP_Auth0_State_Handler();
$stateObj = array(
'interim' => ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ),
'nonce' => $stateHandler->issue()
);

$stateObj = array( "interim" => $interim_login, "uuid" =>uniqid() );
if ( !empty( $redirect_to ) ) {
$stateObj["redirect_to"] = addslashes( $redirect_to );
}
Expand Down
11 changes: 5 additions & 6 deletions lib/WP_Auth0_Lock_Options.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,12 +98,11 @@ public function modal_button_name() {
}

public function get_state_obj( $redirect_to = null ) {
if ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ) {
$interim_login = true;
} else {
$interim_login = false;
}
$stateObj = array( "interim" => $interim_login, "uuid" =>uniqid() );
$stateHandler = new WP_Auth0_State_Handler();
$stateObj = array(
'interim' => ( isset( $_GET['interim-login'] ) && $_GET['interim-login'] == 1 ),
'nonce' => $stateHandler->issue()
);
if ( !empty( $redirect_to ) ) {
$stateObj["redirect_to"] = addslashes( $redirect_to );
}
Expand Down
90 changes: 57 additions & 33 deletions lib/WP_Auth0_LoginManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class WP_Auth0_LoginManager {
protected $default_role;
protected $ignore_unverified_email;
protected $users_repo;
protected $state;

public function __construct( WP_Auth0_UsersRepo $users_repo, $a0_options = null, $default_role = null, $ignore_unverified_email = false ) {

Expand Down Expand Up @@ -66,19 +67,6 @@ public function auth0_singlelogout_footer( $previous_html ) {
return;
}

$current_user = get_currentauth0user();
$user_profile = $current_user->auth0_obj;

if ( empty( $user_profile ) ) {
return;
}

$lock_options = new WP_Auth0_Lock10_Options();
$cdn = $this->a0_options->get('auth0js-cdn');
$client_id = $this->a0_options->get( 'client_id' );
$domain = $this->a0_options->get( 'domain' );
$logout_url = wp_logout_url( get_permalink() ) . '&SLO=1';

include WPA0_PLUGIN_DIR . 'templates/auth0-singlelogout-handler.php';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed vars above were moved into templates/auth0-singlelogout-handler.php down below

}

Expand Down Expand Up @@ -162,6 +150,13 @@ public function init_auth0() {
return;
}

// Check for valid state nonce
// See https://auth0.com/docs/protocols/oauth2/oauth-state
$state_decoded = $this->get_state();
if ( empty( $state_decoded->nonce ) || ! WP_Auth0_State_Handler::validate( $state_decoded->nonce ) ) {
$this->die_on_login( __( 'Invalid state', 'wp-auth0' ) );
}

try {
if ( $this->query_vars( 'auth0' ) === 'implicit' ) {
$this->implicit_login();
Expand All @@ -170,19 +165,11 @@ public function init_auth0() {
}
} catch (WP_Auth0_LoginFlowValidationException $e) {

$msg = __( 'There was a problem with your log in. ', 'wp-auth0' );
$msg .= ' '. $e->getMessage();
$msg .= '<br/><br/>';
$msg .= '<a href="' . wp_login_url() . '">' . __( '← Login', 'wp-auth0' ) . '</a>';
wp_die( $msg );
$this->die_on_login( $e->getMessage(), $e->getCode() );
Copy link
Contributor

Choose a reason for hiding this comment

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

This 2 die_on_login calls only differ on the boolean you pass at the end.. This uses the default TRUE, meaning is "LOGIN_LINK" (idk what that means). The other passes FALSE, so its "LOGOUT_LINK" (again idk). How is that an exception type on the same "try logic" could tell you that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an internal exception from when the user needs to log in / log out? In that case I think I understand now, and depending on the bool you are adding the link to log in / log out on the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That whole try block needs attention in another PR but, as I understand the intended flow, there are exceptions that are thrown during the login process, WP_Auth0_LoginFlowValidationException, and exceptions thrown once the login flow is complete but before you're actually logged in, WP_Auth0_BeforeLoginException. That bool determines what the messaging is.


} catch (WP_Auth0_BeforeLoginException $e) {

$msg = __( 'You have logged in successfully, but there is a problem accessing this site. ', 'wp-auth0' );
$msg .= ' '. $e->getMessage();
$msg .= '<br/><br/>';
$msg .= '<a href="' . wp_logout_url() . '">' . __( '← Logout', 'wp-auth0' ) . '</a>';
wp_die( $msg );
$this->die_on_login( $e->getMessage(), $e->getCode(), FALSE );

} catch (Exception $e) {

Expand All @@ -206,9 +193,7 @@ public function redirect_login() {
}

$code = $this->query_vars( 'code' );
$state = $this->query_vars( 'state' );

$stateFromGet = json_decode( base64_decode( $state ) );
$state_decoded = $this->get_state();

$domain = $this->a0_options->get( 'domain' );

Expand Down Expand Up @@ -265,12 +250,12 @@ public function redirect_login() {

$userinfo = json_decode( $response['body'] );
if ( $this->login_user( $userinfo, $data->id_token, $data->access_token ) ) {
if ( ! empty( $stateFromGet->interim ) ) {
if ( ! empty( $state_decoded->interim ) ) {
include WPA0_PLUGIN_DIR . 'templates/login-interim.php';
exit();
} else {
if ( ! empty( $stateFromGet->redirect_to ) && wp_login_url() !== $stateFromGet->redirect_to ) {
$redirectURL = $stateFromGet->redirect_to;
if ( ! empty( $state_decoded->redirect_to ) && wp_login_url() !== $state_decoded->redirect_to ) {
$redirectURL = $state_decoded->redirect_to;
} else {
$redirectURL = $this->a0_options->get( 'default_login_redirection' );
}
Expand Down Expand Up @@ -311,7 +296,7 @@ public function redirect_login() {
public function implicit_login() {

$token = $_POST['token'];
$stateFromGet = json_decode( base64_decode( $_POST['state'] ) );
$state_decoded = $this->get_state();

$secret = $this->a0_options->get_client_secret_as_key();

Expand All @@ -327,12 +312,12 @@ public function implicit_login() {
$decodedToken->user_id = $decodedToken->sub;

if ( $this->login_user( $decodedToken, $token, null ) ) {
if ( ! empty( $stateFromGet->interim ) ) {
if ( ! empty( $state_decoded->interim ) ) {
include WPA0_PLUGIN_DIR . 'templates/login-interim.php';
exit();
} else {
if ( ! empty( $stateFromGet->redirect_to ) && wp_login_url() !== $stateFromGet->redirect_to ) {
$redirectURL = $stateFromGet->redirect_to;
if ( ! empty( $state_decoded->redirect_to ) && wp_login_url() !== $state_decoded->redirect_to ) {
$redirectURL = $state_decoded->redirect_to;
} else {
$redirectURL = $this->a0_options->get( 'default_login_redirection' );
}
Expand Down Expand Up @@ -537,6 +522,45 @@ protected function query_vars( $key ) {
return null;
}

/**
* Get and store state returned from Auth0
*
* @return object|null
*/
protected function get_state() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lbalmaceda - FYI, I just added this method and it's usage above (using this instead of always parsing the state parameter). It didn't invalidate your review but thought I would point it out.


if ( empty( $this->state ) ) {
$this->state = json_decode( base64_decode( $this->query_vars( 'state' ) ) );
}
return is_object( $this->state ) ? $this->state : null;
}

/**
* Die during login process with a message
*
* @param string $msg - translated error message to display
* @param string|int $code - error code, if given
* @param bool $login_link - TRUE for login link, FALSE for logout link
*/
protected function die_on_login( $msg = '', $code = 0, $login_link = TRUE ) {

wp_die( sprintf(
'%s: %s [%s: %s]<br><br><a href="%s">%s</a>',
$login_link
? __( 'There was a problem with your log in', 'wp-auth0' )
: __( 'You have logged in successfully, but there is a problem accessing this site', 'wp-auth0' ),
! empty( $msg )
? sanitize_text_field( $msg )
: __( 'Please see the site administrator', 'wp-auth0' ),
__( 'error code', 'wp-auth0' ),
$code ? sanitize_text_field( $code ) : __( 'unknown', 'wp-auth0' ),
$login_link ? wp_login_url() : wp_logout_url(),
$login_link
? __( '← Login', 'wp-auth0' )
: __( '← Logout', 'wp-auth0' )
) );
}

/**
* DEPRECATED 3.5.0
* Deprecated to improve the functionality and move to a new class
Expand Down
78 changes: 78 additions & 0 deletions lib/WP_Auth0_State_Handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

class WP_Auth0_State_Handler {

/**
* @var string
*/
protected $uniqid;

/**
* @var string
*/
const COOKIE_NAME = 'auth0_uniqid';

/**
* @var int
*/
protected $cookieExpiresIn = MINUTE_IN_SECONDS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this value comes from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WP core:

https://github.com/WordPress/WordPress/blob/master/wp-includes/default-constants.php#L129


/**
* WP_Auth0_State_Handler constructor.
*/
public function __construct() {
$this->uniqid = self::generateNonce();
}

/**
* Set the unique ID for state and return
*
* @return string
*/
public function issue() {
$this->store();
return $this->uniqid;
}

/**
* Set the state cookie value
*
* @return bool
*/
protected function store() {
return setcookie( self::COOKIE_NAME, $this->uniqid, time() + $this->cookieExpiresIn );
}

/**
* Check if the stored state matches a specific value
*
* @param $state
*
* @return bool
*/
public static function validate( $state ) {
$valid = isset( $_COOKIE[ self::COOKIE_NAME ] ) ? $_COOKIE[ self::COOKIE_NAME ] === $state : FALSE;
self::reset();
return $valid;
}

/**
* Reset the state cookie value
*
* @return bool
*/
public static function reset() {
return setcookie( self::COOKIE_NAME, '', 0 );
}

/**
* Generate a pseudo-random ID (not cryptographically secure)
*
* @see https://stackoverflow.com/a/1846229/728480
*
* @return string
*/
public static function generateNonce() {
return md5( uniqid( rand(), true ) );
}
}
27 changes: 14 additions & 13 deletions templates/auth0-singlelogout-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,29 @@
<script id="auth0" src="<?php echo $cdn ?>"></script>
<?php
$current_user = get_currentauth0user();
if ( empty( $current_user->auth0_obj ) ) {
return;
}
?>
<script id="auth0" src="<?php echo esc_url( $this->a0_options->get('auth0js-cdn') ) ?>"></script>
<script type="text/javascript">
(function(){

var uuids = '<?php echo $user_profile->user_id; ?>';
document.addEventListener("DOMContentLoaded", function() {
if (typeof(auth0) === 'undefined') {
return;
}

var webAuth = new auth0.WebAuth({
clientID:'<?php echo $client_id; ?>',
domain:'<?php echo $domain; ?>'
clientID:'<?php echo sanitize_text_field( $this->a0_options->get( 'client_id' ) ); ?>',
domain:'<?php echo sanitize_text_field( $this->a0_options->get( 'domain' ) ); ?>'
});

var options = <?php echo json_encode( $lock_options->get_sso_options() ); ?>;
options.responseType = 'token id_token';
webAuth.checkSession(options, function (err, authResult) {
if (err !== null) {
if(err.error ==='login_required') {
window.location = '<?php echo html_entity_decode( $logout_url ); ?>';
webAuth.checkSession( { 'responseType' : 'token', 'redirectUri' : window.location.href }, function ( err ) {
Copy link
Contributor Author

@joshcanhelp joshcanhelp Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This change might seem a little out-of-scope but this template file was pulling Lock options which was, in turn, generating a state nonce and trying to store it in cookies. checkSession only needs a couple of options and pulling all the Lock ones and then overriding one was confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this looks as a good call. 👏

if ( err && err.error ==='login_required' ) {
window.location = '<?php echo esc_url( wp_logout_url( get_permalink() ) . '&SLO=1' ); ?>';
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the SLO=1 query param added at the end of the url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Takes the redirect_to query param (the argument in wp_logout_url()) and redirects after running the logout process:

https://github.com/auth0/wp-auth0/blob/master/lib/WP_Auth0_LoginManager.php#L93

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow. This is an "internal" query param that you later use to know if you have to 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.

Basically, yes. It sends it through the regular logout process, then that param tells the process to look for the redirect param.

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm Ok. Sounds hacky and I'm not going to block you for this if it works. But it should probably be reviewed in the future 🐔

Copy link
Contributor Author

@joshcanhelp joshcanhelp Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Logout process is getting a once-over in another PR for this same release. You'll be able to sleep through the night soon enough 💤

}
}
}
});

);
});
})();
</script>