State handling during login process for both types #406

joshcanhelp · 2018-03-02T22:39:36Z

The WordPress plugin doesn't not currently handle state checking at all. A value is set in Lock and sent back but not stored and checked. This PR:

Adds a WP_Auth0_State_Handler class used to generate, store, and validate a state nonce using cookies
Adjusts Lock to use the new nonce value and stores it as a cookie
Checks for this value during login for both flows, redirect and implicit
Adds a WP_Auth0_LoginManager->die_on_login() method to handling error message display for login errors
Adjusts the auth0-singlelogout-handler.php to not pull Lock options and skip anything related to state

…rect; added WP_Auth0_LoginManager->die_on_login() to handle login errors better

… setting a cookie unecessarily

joshcanhelp · 2018-03-05T20:06:34Z

lib/WP_Auth0_LoginManager.php

-    $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';


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

joshcanhelp · 2018-03-05T20:23:10Z

templates/auth0-singlelogout-handler.php

-      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 ) {


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.

To me this looks as a good call. 👏

lbalmaceda · 2018-03-05T20:41:02Z

lib/WP_Auth0_State_Handler.php

+  /**
+   * @var string
+   */
+  protected $uniqid = '';


If this is being set in the __construct() method (I guess the class instance constructor) there shouldn't be a need to set it to '' here.

lbalmaceda · 2018-03-05T20:42:20Z

templates/auth0-singlelogout-handler.php

-          window.location = '<?php echo html_entity_decode( $logout_url ); ?>';
+    webAuth.checkSession( { 'responseType' : 'token', 'redirectUri' : window.location.href }, function ( err ) {
+            if ( err && err.error ==='login_required' ) {
+                window.location = '<?php echo esc_url( wp_logout_url( get_permalink() ) . '&SLO=1' ); ?>';


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

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

I don't follow. This is an "internal" query param that you later use to know if you have to redirect?

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

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 🐔

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 💤

lbalmaceda · 2018-03-07T21:31:52Z

lib/WP_Auth0_LoginManager.php

@@ -162,6 +149,12 @@ public function init_auth0() {
      return;
    }

+    // Check for valid state UUID
+    $state_decoded = json_decode( base64_decode( $this->query_vars( 'state' ) ), TRUE );


what if state is missing from query_vars. How will the code behave?

Then empty( $state_decoded[ 'nonce' ] ) will be true and the login process will fail

So you are saying this calls

json_decode( base64_decode( $this->query_vars( 'state' ) ), TRUE );

don't fail when the query_vars.state is missing/null/undefined, right?

Exactly. $this->query_vars( 'state' ) returns null if not found, base64_decode() returns false in that case, and json_decode() returns false. PHP's empty() returns true even if that key does not exist (no errors).

Perfect, thanks.

lbalmaceda · 2018-03-07T21:36:07Z

lib/WP_Auth0_LoginManager.php

-      $msg .= '<br/><br/>';
-      $msg .= '<a href="' . wp_login_url() . '">' . __( '← Login', 'wp-auth0' ) . '</a>';
-      wp_die( $msg );
+      $this->die_on_login( $e->getMessage(), $e->getCode() );


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?

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.

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.

lbalmaceda · 2018-03-07T21:37:59Z

lib/WP_Auth0_State_Handler.php

+  /**
+   * @var int
+   */
+  protected $cookieExpiresIn = MINUTE_IN_SECONDS;


Where does this value comes from?

WP core:

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

123456789012345678901234567890123456789012345678901234567890123456789012 Added a get_state method to WP_Auth0_LoginManager to get and parse the state parameter as an object. Added 'state' to allowed query_vars.

joshcanhelp · 2018-03-08T23:47:05Z

lib/WP_Auth0_LoginManager.php

+   *
+   * @return object|null
+   */
+  protected function get_state() {


@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.

cocojoe

LGTM in general, as always test thoroughly.

joshcanhelp added the WIP label Mar 2, 2018

joshcanhelp added this to the v3-Next milestone Mar 2, 2018

joshcanhelp force-pushed the fixed-handle-state-checking branch from 9a0d68f to ad26af1 Compare March 3, 2018 04:19

State handling during login process for both types, implicit and redi…

13f2f38

…rect; added WP_Auth0_LoginManager->die_on_login() to handle login errors better

joshcanhelp force-pushed the fixed-handle-state-checking branch from ad26af1 to 13f2f38 Compare March 3, 2018 04:29

joshcanhelp added a commit that referenced this pull request Mar 3, 2018

Removing state checking, fixed in #406

000c621

remove stateHandler->issue() from SLO logout footer template to avoid…

3231279

… setting a cookie unecessarily

joshcanhelp commented Mar 5, 2018

View reviewed changes

joshcanhelp added CH: Added and removed WIP labels Mar 5, 2018

lbalmaceda reviewed Mar 5, 2018

View reviewed changes

Review feedback on state handler property

0fcf435

joshcanhelp changed the title ~~[WIP] State handling during login process for both types~~ State handling during login process for both types Mar 6, 2018

lbalmaceda reviewed Mar 7, 2018

View reviewed changes

lbalmaceda self-requested a review March 7, 2018 22:01

lbalmaceda approved these changes Mar 7, 2018

View reviewed changes

joshcanhelp requested a review from cocojoe March 7, 2018 22:03

Better state getting in WP_Auth0_LoginManager

18c574c

123456789012345678901234567890123456789012345678901234567890123456789012 Added a get_state method to WP_Auth0_LoginManager to get and parse the state parameter as an object. Added 'state' to allowed query_vars.

joshcanhelp commented Mar 8, 2018

View reviewed changes

cocojoe approved these changes Mar 12, 2018

View reviewed changes

joshcanhelp merged commit 02c6bba into dev Mar 12, 2018

joshcanhelp deleted the fixed-handle-state-checking branch March 27, 2018 22:32

joshcanhelp mentioned this pull request Jun 5, 2018

Release 3.6.0 #475

Merged

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

State handling during login process for both types #406

State handling during login process for both types #406

joshcanhelp commented Mar 2, 2018 •

edited

Loading

joshcanhelp Mar 5, 2018

joshcanhelp Mar 5, 2018 •

edited

Loading

lbalmaceda Mar 7, 2018

lbalmaceda Mar 5, 2018

lbalmaceda Mar 5, 2018

joshcanhelp Mar 5, 2018

lbalmaceda Mar 7, 2018

joshcanhelp Mar 7, 2018

lbalmaceda Mar 7, 2018

joshcanhelp Mar 7, 2018 •

edited

Loading

lbalmaceda Mar 7, 2018

joshcanhelp Mar 7, 2018

lbalmaceda Mar 7, 2018 •

edited

Loading

joshcanhelp Mar 7, 2018

lbalmaceda Mar 7, 2018

lbalmaceda Mar 7, 2018

lbalmaceda Mar 7, 2018

joshcanhelp Mar 7, 2018

lbalmaceda Mar 7, 2018

joshcanhelp Mar 7, 2018

joshcanhelp Mar 8, 2018

cocojoe left a comment

State handling during login process for both types #406

State handling during login process for both types #406

Conversation

joshcanhelp commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

joshcanhelp Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe left a comment

Choose a reason for hiding this comment

joshcanhelp commented Mar 2, 2018 •

edited

Loading

joshcanhelp Mar 5, 2018 •

edited

Loading

joshcanhelp Mar 7, 2018 •

edited

Loading

lbalmaceda Mar 7, 2018 •

edited

Loading