Fix Implicit login handling #426

joshcanhelp · 2018-04-03T20:52:40Z

Fixed error handling for implicit login
Moved implicit handling JS to own file; refactored
Added nonce handling for auto-login
Added state handling for auto-login
Better nonce generator if < PHP 7 (openssl_random_pseudo_bytes())

cocojoe

LGTM, presume still going through your extensive manual testing?

joshcanhelp · 2018-04-04T14:13:57Z

@cocojoe - as usual 😄 working on a Docker setup now to make life a little easier.

lbalmaceda · 2018-04-04T18:24:01Z

WP_Auth0.php

 	}

 	public function render_form( $html ) {
-		if ( isset( $_GET['action'] ) && $_GET['action'] == 'lostpassword' ) {
+		if ( ( isset( $_GET['action'] ) && $_GET['action'] == 'lostpassword' ) || ! empty( $_GET[ 'auth0' ] ) ) {


what does this change represent?

If the auth0 URL param is set, don't load the form. It's only sent when coming back from an implicit grant. Auth code grant goes straight to index.php?auth0=1

If I load the form, I get a new state set so I don't want to do that ... also, no need to load the form at all since we're just passing through.

Alright, I remember the auth0 param now. Are you planning to refactor this "auth0 hack" anytime soon?

The param can show up on the login page and on the index.php entry point.

Not loading the form in this case does solve the problem of re-setting a state cookie but we don't need to load the login form at all if this is a response from Auth0. So, I would say that the URL parameter check will stay but state checking will evolve to handle this better (by abstracting that WP_Auth0_Nonce_Handler class to handle both scenarios)

lbalmaceda · 2018-04-04T18:25:26Z

assets/js/implicit-login.js

@@ -0,0 +1,35 @@
+document.addEventListener( 'DOMContentLoaded', function() {
+    if ( ! window.location.hash || window.location.hash.indexOf('id_token') < 0 ) {
+        return;


If the token is missing you return. Does that mean that you are ignoring a possible auth error?

Good question and I don't know if I have a good answer. The core login page processing does not use anchors at all and I don't want this to run unless I have something to do here. I'm not sure of another situation where we would have a hash and want to do something with it. I don't see anything else in the implicit grant doc.

Is there an error message that I might get back? error or error_description?

Yeah. You might receive error and error_description in the hash/query params. There are a few knowns errors, I'll share privately a reference.

lbalmaceda · 2018-04-04T18:26:20Z

assets/js/implicit-login.js

+        return;
+    }
+
+    var hash = window.location.hash;


I don't know how much resources you are saving, but this line could be the first one and hash used on the if

lbalmaceda · 2018-04-04T18:27:01Z

assets/js/implicit-login.js

+    }
+
+    var hash = window.location.hash;
+    if (hash[0] === '#') {


will this work fine if hash=""?

But it can't be empty because we've escaped already if that's the case

lbalmaceda · 2018-04-04T18:45:31Z

lib/WP_Auth0_LoginManager.php

+      if ( ! empty( $state_decoded->interim ) ) {
+        include WPA0_PLUGIN_DIR . 'templates/login-interim.php';
+      } else {
+        $redirect_to = ! empty( $state_decoded->redirect_to ) ? $state_decoded->redirect_to : null;


Isn't this the same, or you need an explicit null later?

$redirect_to = $state_decoded->redirect_to

$state_decoded->redirect_to will throw an error if it does not exist, so I'm checking whether it's there or not with empty()

lbalmaceda · 2018-04-04T18:48:37Z

lib/WP_Auth0_LoginManager.php

@@ -551,7 +561,7 @@ protected function get_state( $decoded = FALSE ) {

    if ( empty( $this->state ) ) {
      // Get and store base64 encoded state
-      $state_val = $this->query_vars( 'state' );
+      $state_val = $_REQUEST[ 'state' ];


Why, did the previous stop working?

$this->query_vars() is not explicit enough, makes for confusion around how these vars are handled. I want state from either POST or GET (aka $_REQUEST), I don't want anything else.

lbalmaceda · 2018-04-04T18:49:17Z

lib/WP_Auth0_Nonce_Handler.php

-  public function generateNonce() {
-    return function_exists( 'random_bytes' ) ? bin2hex( random_bytes( 32 ) ) : md5( uniqid( rand(), true ) );
+  public function generateNonce( $bytes = 32 ) {
+    $nonce_bytes = function_exists( 'random_bytes' ) ? random_bytes( $bytes ) : openssl_random_pseudo_bytes( $bytes );


will openssl_random_pseudo_bytes always exist?

This plugin is tagged PHP 5.3, which is when this function was introduced:

https://secure.php.net/manual/en/function.openssl-random-pseudo-bytes.php

lbalmaceda · 2018-04-04T19:28:48Z

WP_Auth0.php

@@ -123,6 +123,10 @@ public function init() {

 		$this->check_signup_status();

+		if ( $this->a0_options->get( 'auto_login' ) ) {
+			WP_Auth0_Nonce_Handler::getInstance()->setCookie();


Is this well indented?

Like, correct whitespace? Yes. This project mixes tabs and spaces, as well as how many spaces for each. I'll do an automated whitespace cleanup before I start the next release.

lbalmaceda · 2018-04-05T14:39:38Z

assets/js/implicit-login.js

+        form.appendChild( createHiddenField( 'token', data.id_token ) );
+        form.appendChild( createHiddenField( 'state', data.state ) );
+    } else {
+        redirectWithoutQuery();


Is this a valid path? When will it happen? Do you have an example? If you don't have one but still want to catch it, would be nice to log that query somewhere to debug and understand it I guess. If the authentication was initiated for "implicit flow" and the token is not there, there must be an error.

It's valid as in it exists but without a hash we don't have much to do. The param triggers this JS to load and for the form to be excluded (param tells us that we need to process something from Auth0). If we don't have the information we need, then we need to cease processing, hence the redirect.

If we have the Auth0 param but no error or no state, that could be an intermittent problem, in which case showing the login form to try again seems reasonable. I'm not sure I want to catch and log every possible outcome here, I think it's best to get someone back to a place where they can try again.

lbalmaceda · 2018-04-05T14:47:19Z

lib/WP_Auth0_LoginManager.php

@@ -150,9 +150,10 @@ public function init_auth0() {

    // Catch any incoming errors and stop the login process
    // See https://auth0.com/docs/libraries/error-messages
-    if ( $this->query_vars( 'error_description' ) ) {
+    if ( $this->query_vars( 'error' ) || $this->query_vars( 'error_description' ) ) {


I think the error_description might be missing sometimes (AFAIK is optional). In any case, because error is always present that condition should suffice.

Move the JS that POSTs auth data back to the WP site to it's own file. Better enqueuing for login JS and CSS.

lbalmaceda

Aaaand done!

joshcanhelp added the CH: Fixed label Apr 3, 2018

joshcanhelp added this to the v3-Next milestone Apr 3, 2018

joshcanhelp self-assigned this Apr 3, 2018

joshcanhelp added the Ready for Review label Apr 3, 2018

cocojoe approved these changes Apr 4, 2018

View reviewed changes

lbalmaceda suggested changes Apr 4, 2018

View reviewed changes

joshcanhelp force-pushed the fix-implicit-login branch from b3443fe to a21333c Compare April 4, 2018 19:13

lbalmaceda reviewed Apr 4, 2018

View reviewed changes

joshcanhelp force-pushed the fix-implicit-login branch from 92253c9 to 7561602 Compare April 5, 2018 02:45

lbalmaceda reviewed Apr 5, 2018

View reviewed changes

joshcanhelp added 3 commits April 5, 2018 11:01

Move implicit JS to external file

f30a944

Move the JS that POSTs auth data back to the WP site to it's own file. Better enqueuing for login JS and CSS.

Fix Implicit state, nonce, and error handling

0b3bb44

Add error handling

971021a

joshcanhelp force-pushed the fix-implicit-login branch from 7561602 to 971021a Compare April 5, 2018 18:01

lbalmaceda approved these changes Apr 5, 2018

View reviewed changes

joshcanhelp merged commit d7ad3f3 into dev Apr 5, 2018

joshcanhelp deleted the fix-implicit-login branch April 5, 2018 19:06

joshcanhelp removed the Ready for Review label Apr 5, 2018

joshcanhelp mentioned this pull request Apr 16, 2018

Fix Passwordless handling; update Lock instantiation #434

Merged

joshcanhelp mentioned this pull request May 3, 2018

Linked Users won't be able to login using implicit flow and pipeline 2 #272

Closed

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

Fix Implicit login handling #426

Fix Implicit login handling #426

joshcanhelp commented Apr 3, 2018

cocojoe left a comment

joshcanhelp commented Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

lbalmaceda Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 4, 2018

joshcanhelp Apr 4, 2018

lbalmaceda Apr 5, 2018

joshcanhelp Apr 5, 2018

lbalmaceda Apr 5, 2018

lbalmaceda left a comment

Fix Implicit login handling #426

Fix Implicit login handling #426

Conversation

joshcanhelp commented Apr 3, 2018

cocojoe left a comment

Choose a reason for hiding this comment

joshcanhelp commented Apr 4, 2018

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

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

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

lbalmaceda left a comment

Choose a reason for hiding this comment