Fix Login Process Error Handling #409

joshcanhelp · 2018-03-09T23:29:13Z

WP_Auth0_LoginManager was not processing errors well, was poorly
documented, and might have been improperly exposing error messages.
Incoming URL param errors from Auth0 and configuration issues are caught
earlier in the login process. Error message are not exposed to the
user; instead they are logged for an admin. Thrown errors are
standarized and listed in docblocks.

Fixes #305

joshcanhelp · 2018-03-09T23:29:29Z

lib/WP_Auth0_LoginManager.php

@@ -144,12 +144,19 @@ public function login_auto() {
 }

 public function init_auth0() {
- global $wp_query;


joshcanhelp · 2018-03-09T23:30:04Z

lib/WP_Auth0_LoginManager.php

 public function redirect_login() {
- global $wp_query;


joshcanhelp · 2018-03-09T23:30:39Z

lib/WP_Auth0_LoginManager.php

 public function redirect_login() {
- global $wp_query;
-
- if ( $this->query_vars( 'auth0' ) === null ) {


These 3 if blocks were moved to init_auth0() to be caught earlier and for both redirect and implicit flows.

joshcanhelp · 2018-03-09T23:32:46Z

lib/WP_Auth0_LoginManager.php

 ) );

- if ( $response instanceof WP_Error ) {


Diff gets a little mangled from here down because of switching around how the errors are handled. This was removed in favor of checking a specific error code we're looking for, followed by a general check if we have the info we need (access_token)

joshcanhelp · 2018-03-09T23:33:47Z

lib/WP_Auth0_LoginManager.php

- WP_Auth0_Api_Client::get_client_token(),
- $decoded_token->sub
- );
+ // Unsuccessful for another reason


Log the error if we've got something but don't show to end user

joshcanhelp · 2018-03-09T23:34:17Z

lib/WP_Auth0_LoginManager.php


- throw new WP_Auth0_LoginFlowValidationException( );
- }
+ if ( empty( $data->access_token ) ) {


Only need the access token here

WP_Auth0_LoginManager was not processing errors well, was poorly documented, and might have been improperly exposing error messages. Incoming URL param errors from Auth0 and configuration issues are caught earlier in the login process. Error message are not exposed to the user; instead they are logged for an admin. Thrown errors are standarized and listed in docblocks. Fixes #305

joshcanhelp · 2018-03-12T22:05:20Z

lib/WP_Auth0_LoginManager.php

 $this->die_on_login( $e->getMessage(), $e->getCode(), FALSE );
-
- } catch (Exception $e) {


Blank general Exception catching lead to issues like #305

joshcanhelp · 2018-03-12T22:05:56Z

lib/WP_Auth0_LoginManager.php


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

 $client_id = $this->a0_options->get( 'client_id' );
 $client_secret = $this->a0_options->get( 'client_secret' );

- if ( empty( $client_id ) ) {


Handled earlier in init_auth0() by WP_Auth0::ready()

joshcanhelp · 2018-03-12T22:06:29Z

lib/WP_Auth0_LoginManager.php


- $decoded_token = JWT::decode(


joshcanhelp · 2018-03-12T22:06:33Z

lib/WP_Auth0_LoginManager.php


- $data->id_token = null;
- $response = WP_Auth0_Api_Client::get_user(


cocojoe

Kind of hard to read, but looks okay. Lot of handling cleaned up which is nice.

lbalmaceda

left some comments 👍

lbalmaceda · 2018-03-14T16:12:32Z

WP_Auth0.php

@@ -123,6 +123,19 @@ public function init() {
 WP_Auth0_Email_Verification::init();
 }

+ /**
+ * Is the Auth0 plugin ready to proc


if "proc" is the short for something I didn't get it. To process, maybe?

Maybe I just fell asleep in the middle of that docblock or something ...

lbalmaceda · 2018-03-14T16:12:59Z

WP_Auth0.php

+ */
+ public static function ready() {
+ $options = WP_Auth0_Options::Instance();
+ if ( ! $options->get( 'domain' ) || ! $options->get( 'client_id' ) || ! $options->get( 'client_id' ) ) {


duplicated client_id condition

lbalmaceda · 2018-03-14T16:14:12Z

WP_Auth0.php

@@ -240,9 +253,11 @@ public static function get_plugin_dir_url() {
 }

 public function a0_register_query_vars( $qvars ) {


what's the purpose of this function?

It registers query variables (URL params) with the main WP_Query instance that basically runs the whole site.

lbalmaceda · 2018-03-14T16:14:51Z

lib/WP_Auth0_LoginManager.php


- if ( $this->query_vars( 'auth0' ) === null ) {
+ // Not an Auth0 login process or settings are not configured to allow logins
+ if ( ! $this->query_vars( 'auth0' ) || ! WP_Auth0::ready() ) {


auth0 was the query param you use to track context, right?

Yes, can be 1 for a "regular" login or implicit

Hmm.. That's confusing, but as long as it's "internal" I don't care. 👍

I agree but it's legacy and dangerous to unwire at this point. I'd like to just deprecate implicit altogether ...

lbalmaceda · 2018-03-14T16:18:07Z

lib/WP_Auth0_LoginManager.php

+ // See https://auth0.com/docs/libraries/error-messages
+ if ( $this->query_vars( 'error_description' ) ) {
+ $error_msg = sanitize_text_field( $this->query_vars( 'error_description' ) );
+ $this->die_on_login( $error_msg ? $error_msg : sanitize_text_field( $this->query_vars( 'error' ) ) );


the if already checks if error_description is present on the query. Is there any case where the sanitize_test_field(error_description) will fail or give you something null/undefined? if not, it doesn't make sense to have this line's if that calls sanitize_text_field(error)

We'll only ever get a scalar back from query_vars() and sanitize_text_field() is just there to sanitize. We're expecting error descriptions from the Auth0 server so if something comes through that's wonky then it's probably someone screwing around. If sanitize_text_field() returns an empty value then it was empty to begin with or invalid symbols:

https://developer.wordpress.org/reference/functions/sanitize_text_field/

lbalmaceda · 2018-03-14T16:35:36Z

lib/WP_Auth0_LoginManager.php


-  $msg = __( 'Error: the Client Secret configured on the Auth0 plugin is wrong. Make sure to copy the right one from the Auth0 dashboard.', 'wp-auth0' );
+ if ( $userinfo_resp instanceof WP_Error || 200 !== $userinfo_resp_code || empty( $userinfo_resp_body ) ) {


To be honest I think the "error handler" should catch the last 2 conditions already and create/raise a WP_ERROR. You should only be checking for the error existence, if not you are "extending" the error handler logic to every call.

Also a good call. If $userinfo_resp is a WP_Error then this will also be true (resolves the code to 0).

lbalmaceda · 2018-03-14T16:41:52Z

lib/WP_Auth0_LoginManager.php

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


what about

$redirectURL= $state_decoded->redirect_to if ( empty( $redirectURL ) || wp_login_url() !== $redirectURL ) { $redirectURL = $this->a0_options->get( 'default_login_redirection' ); } wp_safe_redirect( $redirectURL );

What if $state_decoded->redirect_to is not set?

I don't know, I guess from your answer that it will throw an unhandled exception. If that's the case then fine 👍. I only suggested this as a way to avoid calling $state_decoded->redirect_to 3 times

If $state_decoded->redirect_to is not set and you try to access it (the exception being empty()) then you get a notice. $redirectURL ends up false-y and things progress fine.

"avoid calling $state_decoded->redirect_to 3 times" - just for the sake of style? Accessing a property like that shouldn't have any kind of performance impact.

lbalmaceda · 2018-03-14T16:42:51Z

lib/WP_Auth0_LoginManager.php

- WP_Auth0_ErrorManager::insert_auth0_error( __METHOD__ . ' => $this->login_user()', $error );
+ if ( $this->login_user( $userinfo, $data->id_token, $data->access_token ) ) {
+ $state_decoded = $this->get_state();
+ if ( ! empty( $state_decoded->interim ) ) {


what's interim? and what is the structure of the object state_decoded?

TBH, I'm not sure about interim at the moment, has never come up so I left it in-tact.

State is an object with interim (bool), nonce (state validation), and state (always set to "nonce" ... which can probably go). Sometimes redirect_to is set to a URL but it can be blank.

@joshcanhelp are you saying that state is the string "nonce" or that state==nonce ? In any case, remember this are individual values and should be different, secure random strings.

Currently (in JSON for clarity):

{ "interim" : true || false, "nonce" : "2309u29j23o28j2je8", "state" : "nonce" }

lbalmaceda · 2018-03-14T16:44:20Z

lib/WP_Auth0_LoginManager.php

+ * @param object $userinfo - the Auth0 profile of the user
+ * @param bool $is_new - `true` if the user was created in the WordPress database, `false` if not
+ * @param string $id_token - user's ID token returned from Auth0
+ * @param string $access_token - user's access token returned from Auth0; not provided when using implicit_login()


AFAIK the AcT is always returned. The one that might be missing is the IdT.

That's true ... but the $id_token param was already there

So why isn't the access token provided "when using implicit_login()"?

We're given an ID token via POST which we use to ID the user. Access token isn't used for anything in that flow (at least in WP). Implicit in WP is a bit of a strange animal ...

lbalmaceda · 2018-03-14T16:45:19Z

lib/WP_Auth0_LoginManager.php

- * that the cookie will be kept. Default false.
- * }
- */
+ // See wp_signon() for documentation on this filter


Ah that's lazy 😛 I don't see it in this diff. Was this a copy of that method documentation? What's the reason behind removing this one?

Not really lazy, more like an unnecessary duplicate of WP core documentation. Seems silly to have that here and in the core location and try to keep the two updated.

That's fair. 👍

joshcanhelp · 2018-03-14T22:19:18Z

@lbalmaceda - Fixes up and ready for re-review!

lbalmaceda · 2018-03-15T18:14:04Z

lib/WP_Auth0_LoginManager.php

+ if ( $this->login_user( $userinfo, $data->id_token, $data->access_token ) ) {
+ $state_decoded = $this->get_state();
+ if ( ! empty( $state_decoded->interim ) ) {
+ include WPA0_PLUGIN_DIR . 'templates/login-interim.php';


there was an exit() before right below this line. Are you sure it's no longer required?

exit() was moved to catch this and wp_safe_redirect() (which requires an exit/die). Here

lbalmaceda · 2018-03-15T19:18:25Z

lib/WP_Auth0_LoginManager.php


 $userinfo_resp_code = (int) wp_remote_retrieve_response_code( $userinfo_resp );
 $userinfo_resp_body = wp_remote_retrieve_body( $userinfo_resp );

- if ( $userinfo_resp instanceof WP_Error || 200 !== $userinfo_resp_code || empty( $userinfo_resp_body ) ) {
+ // Management API call failed
+ if ( 200 !== $userinfo_resp_code || empty( $userinfo_resp_body ) ) {


What I meant is remove this 2 conditions and only keep the $userinfo_resp instanceof WP_Error one. Why did you choose to remove the other?

$userinfo_resp is only a WP_Error if something went wrong during the POST call. If it succeeds but you get a 401 or 404, the response code is what needs to be checked.

lbalmaceda

:)

joshcanhelp added the WIP label Mar 9, 2018

joshcanhelp commented Mar 9, 2018

View reviewed changes

lib/WP_Auth0_LoginManager.php

public function redirect_login() {

global $wp_query;

Copy link

Contributor Author

joshcanhelp Mar 9, 2018

Choose a reason for hiding this comment

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

Not used

joshcanhelp commented Mar 9, 2018

View reviewed changes

joshcanhelp force-pushed the fixed-login-error-handling branch from 325594a to 46e20ec Compare March 12, 2018 22:02

joshcanhelp changed the base branch from fixed-handle-state-checking to dev March 12, 2018 22:04

joshcanhelp commented Mar 12, 2018

View reviewed changes

lib/WP_Auth0_LoginManager.php Outdated

$decoded_token = JWT::decode(

Copy link

Contributor Author

joshcanhelp Mar 12, 2018

Choose a reason for hiding this comment

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

Moved down

joshcanhelp commented Mar 12, 2018

View reviewed changes

lib/WP_Auth0_LoginManager.php Outdated

$data->id_token = null;

$response = WP_Auth0_Api_Client::get_user(

Copy link

Contributor Author

joshcanhelp Mar 12, 2018

Choose a reason for hiding this comment

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

Moved down

joshcanhelp changed the title ~~[WIP] Fix login process error reporting~~ Fix Login Process Error Handling Mar 13, 2018

joshcanhelp added CH: Fixed and removed WIP labels Mar 13, 2018

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

cocojoe approved these changes Mar 14, 2018

View reviewed changes

lbalmaceda suggested changes Mar 14, 2018

View reviewed changes

Fixes from Lucho's PR review

ea3c8d0

lbalmaceda reviewed Mar 15, 2018

View reviewed changes

More clear error code and message

6ac78cd

lbalmaceda reviewed Mar 15, 2018

View reviewed changes

lbalmaceda approved these changes Mar 15, 2018

View reviewed changes

joshcanhelp merged commit 477eacd into dev Mar 15, 2018

joshcanhelp deleted the fixed-login-error-handling branch March 15, 2018 21:10

This was referenced May 3, 2018

Bad request does not raise error #432

Closed

redirect callback errors #280

Closed

This was referenced May 3, 2018

Only decode the payload before user profile fetch in login manager #283

Closed

"Invalid authorization code": Access token is requested twice in a row, breaking the login flow #305

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

		$this->die_on_login( $e->getMessage(), $e->getCode(), FALSE );

		} catch (Exception $e) {


		$data->id_token = null;
		$response = WP_Auth0_Api_Client::get_user(

		@@ -240,9 +253,11 @@ public static function get_plugin_dir_url() {
		}

		public function a0_register_query_vars( $qvars ) {


		$msg = __( 'Error: the Client Secret configured on the Auth0 plugin is wrong. Make sure to copy the right one from the Auth0 dashboard.', 'wp-auth0' );
		if ( $userinfo_resp instanceof WP_Error \|\| 200 !== $userinfo_resp_code \|\| empty( $userinfo_resp_body ) ) {

Fix Login Process Error Handling #409

Fix Login Process Error Handling #409

Conversation

joshcanhelp commented Mar 9, 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

Choose a reason for hiding this comment

cocojoe left a comment

Choose a reason for hiding this comment

lbalmaceda left a 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

joshcanhelp Mar 15, 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 commented Mar 14, 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

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp commented Mar 9, 2018 •

edited

Loading

joshcanhelp Mar 15, 2018 •

edited

Loading