Fixed login flow for new tenants, refactored verification email resend #364

joshcanhelp · 2018-01-16T15:53:46Z

New tenants were unable to create accounts in WordPress because the login was using the deprecated ID token process (legacy accounts had a setting activated that would allow this). This PR switches the login process to use access_tokens, which works for legacy and new.

This also refactors the resend email process, which was exposing management tokens.

Caveat here: the WP-created client needs to be authorized on the management API before the resend email process will work.

…0_Api_Client::get_token()and adding scopes for email and identities

…ted to new tenants

…nk token for backwards-compat

…ror from AJAX

lbalmaceda · 2018-01-16T17:15:03Z

WP_Auth0.php

@@ -11,7 +11,7 @@
 define( 'WPA0_PLUGIN_URL', trailingslashit( plugin_dir_url( __FILE__ ) ) );
 define( 'WPA0_LANG', 'wp-auth0' ); // deprecated; do not use for translations
 define( 'AUTH0_DB_VERSION', 15 );
-define( 'WPA0_VERSION', '3.4.0' );
+define( 'WPA0_VERSION', '3.4.1' );


If this is the version of this library, then don't bump the version here. Do it in a separate "release pr" instead.

lbalmaceda · 2018-01-16T17:15:15Z

WP_Auth0.php

@@ -2,7 +2,7 @@
 /**
 * Plugin Name: PLUGIN_NAME
 * Description: PLUGIN_DESCRIPTION
- * Version: 3.4.0
+ * Version: 3.4.1


lbalmaceda · 2018-01-16T17:20:08Z

assets/js/die-with-verify-email.js

@@ -0,0 +1,30 @@
+/* globals jQuery, console, WPAuth0EmailVerification */


Why is it called "die-with-verify-email" ?

@lbalmaceda - The process dies using wp_die() and only happens if the incoming user needs to verify the email. Baiscally: process stops, nothing proceeds.

lbalmaceda · 2018-01-16T17:20:38Z

assets/js/die-with-verify-email.js

+ $.post( WPAuth0EmailVerification.ajaxUrl, postData )
+ .done( function( data ) {
+
+ if ( '1' === data ) {


No magic numbers

@lbalmaceda - Unfortunately WP has a default magic number for AJAX calls ("-1" if you don't pass the nonce check) so a bit of a bad pattern WP folks follow. Will adjust to something more clear.

lbalmaceda · 2018-01-16T17:21:40Z

lib/WP_Auth0_Api_Client.php

+ *
+ * @return string
+ */
+ public static function get_endpoint( $path = '', $domain = '' ) {


why does it need to be public?? The signature is too error prone for the end user.

@lbalmaceda - Everything is public static in that file, for better or worse. This is a case of consistency vs accuracy :)

But if this file is going to end up being the "API Client" then I would prefer not to leave this method open for users to use, as it's only used internally, and also because once we publish this we can't remove it without breaking the public api.

@lbalmaceda - I see what you're getting at here. Was a little confused with who you were referring to as "users" but I see the 💡

lbalmaceda · 2018-01-16T18:03:46Z

lib/WP_Auth0_LoginManager.php

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

 } catch (WP_Auth0_BeforeLoginException $e) {

- $msg = __( 'You have logged in successfully, but there is a problem accessing this site', 'wp-auth0' );
+ $msg = __( 'You have logged in successfully, but there is a problem accessing this site. ', 'wp-auth0' );


@lbalmaceda - yes?

Should probably change these back as this will break existing i18n for this message as it's changed.

It's not that the main message changed but fixed grammatically, I see no need to rollback this change.

Well fixing it breaks it for existing use, no huge feeling either way.

We don't have a PO file in here that we keep up to date so if someone is generating that and translating for themselves, they'll be used to doing that on each release (or used to things breaking/not translating).

lbalmaceda · 2018-01-16T18:04:45Z

lib/WP_Auth0_LoginManager.php

- $response = WP_Auth0_Api_Client::get_user( $domain, $data->id_token, $decodedToken->sub );
- }
+
+ $data->id_token = null;


why you remove the id_token but not the access_token?

@lbalmaceda - Not sure I'm totally clear on your question but this was basically 'if you have an id token, use it, otherwise use the access token' and we want to get rid of the first condition. I'm forcing it to use the access_token and left the $data->id_token = null; because I don't know what the downstream effects are of keeping that id_token around. Maybe there is another check like this down the line? Seemed safer to leave this than remove it.

lbalmaceda · 2018-01-16T18:12:54Z

lib/WP_Auth0_LoginManager.php

+ * @param $id_token
+ */
+ private function dieWithVerifyEmail( $userinfo, $id_token = '' ) {
+ trigger_error( __( 'Method dieWithVerifyEmail is deprecated.', 'wp-auth0' ), E_USER_DEPRECATED);


I don't fully understand the trigger_error method. Is it just logging an error on the console or exploding over the air? If the latter then this is no deprecation. This is breaking the api 🤕 . Old methods must remain the same, working without changing their signature at all. Normally we achieve this by re-routing the call to the new method that will eventually replace it. On a side note, we prefer to handle deprecations on a separate PR so the changelog is clearer. Would you mind rolling back this change?

@lbalmaceda - trigger_error is just that, triggers an error of a certain type. Depending on the error type and the environment, it will be handled different ways. In the case of a deprecated error like this one, it's either displayed or not, won't stop the process from continuing.

Happy to do whatever is prudent here but the idea here is that the function still exists and can be called as based but does the right thing with a soft error. Signature is essentially the same, just defaulting to a blank $id_token and the replacement function is called with the $userinfo it needs. The chances that this function is being used in the wild is very small but, if so, this should keep folks running.

Let me know if you still want me to back it out and how to handle going forward!

lbalmaceda · 2018-01-16T18:24:57Z

lib/exceptions/WP_Auth0_EmailNotVerifiedException.php

+ * WP_Auth0_EmailNotVerifiedException constructor.
+ *
+ * @param stdClass $userinfo - userinfo object returned from Auth0
+ * @param string $id_token - DEPRECATED 3.4.1, should not be output in any template


Move to a separate PR please

@lbalmaceda - Will wait for direction on the other one so I can do them both. Should they each be in their own PR?

Not necessary as long as the PR only deprecates stuff 👌

lbalmaceda · 2018-01-16T18:26:06Z

lib/WP_Auth0_UsersRepo.php

+ *
+ * @return null|WP_User
+ */
+ public function find_auth0_user ( $id ) {


Just asking. Why is this method needed?

@lbalmaceda - Finding the WP user with Auth0 info

…aching a0_options in WP_Auth0_Api_Client; missing docs

cocojoe · 2018-01-16T21:19:22Z

@joshcanhelp Have you tested on new and old (make sure still works) tenants?

joshcanhelp · 2018-01-16T21:48:48Z

@cocojoe - Yes, walked through start to finish on old and new tenants

cocojoe

Have discussed internally, fine as tested.

joshcanhelp added 17 commits January 15, 2018 08:48

changing the login flow to use the access token received from WP_Auth…

521c34c

…0_Api_Client::get_token()and adding scopes for email and identities

fixed incorrect var name

cf31845

whitespace

f609e17

moving in changes from fixed-user-login-uses-access-token branch rela…

6464925

…ted to new tenants

whitespace and comments

9155a46

removing admin user meta creation

d2ab07f

tested and working for new tenants

98d6e55

Deprecated error

5879149

updating the auth0_verify_email_page apply_filter call to pass in bla…

1de0e1a

…nk token for backwards-compat

whitespace formatting

5a4dde8

Merge branch 'dev' into fixed-token-login-flow

92b8569

whitespace and fixed docs link for token generation

5cdffe7

version number

725df16

fixing warnings on user profile

67ead53

changing error message on email verification send

2842cd3

Fixing WP_Auth0_Email_Verification to keep static methods, fix 400 er…

9be8067

…ror from AJAX

minor formatting, whitespace, tweaks before pull request

ed33a49

joshcanhelp requested a review from cocojoe January 16, 2018 15:53

joshcanhelp added CH: Fixed and removed CH: Removed labels Jan 16, 2018

joshcanhelp added this to the v3-Next milestone Jan 16, 2018

lbalmaceda suggested changes Jan 16, 2018

View reviewed changes

joshcanhelp added 3 commits January 16, 2018 11:20

backing out version number change; no magic numbers in AJAX return; c…

5ffdbaf

…aching a0_options in WP_Auth0_Api_Client; missing docs

moving new methods to private; moving nonce to constant

6675a7d

removing deprecated doc tag in WP_Auth0_EmailNotVerifiedException

ca056b7

cocojoe removed the CH: Security label Jan 16, 2018

cocojoe approved these changes Jan 16, 2018

View reviewed changes

joshcanhelp merged commit 798a31a into dev Jan 16, 2018

cocojoe deleted the fixed-token-login-flow branch January 18, 2018 09:49

joshcanhelp mentioned this pull request Jan 24, 2018

Check how id_token decoding works in redirect callback #279

Closed

cocojoe mentioned this pull request Jan 25, 2018

Release 3.5.0 #373

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

Fixed login flow for new tenants, refactored verification email resend #364

Fixed login flow for new tenants, refactored verification email resend #364

joshcanhelp commented Jan 16, 2018

lbalmaceda Jan 16, 2018

lbalmaceda Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

cocojoe Jan 16, 2018

lbalmaceda Jan 16, 2018

cocojoe Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

lbalmaceda Jan 16, 2018

lbalmaceda Jan 16, 2018

joshcanhelp Jan 16, 2018

cocojoe commented Jan 16, 2018

joshcanhelp commented Jan 16, 2018

cocojoe left a comment

		@@ -0,0 +1,30 @@
		/* globals jQuery, console, WPAuth0EmailVerification */

Fixed login flow for new tenants, refactored verification email resend #364

Fixed login flow for new tenants, refactored verification email resend #364

Conversation

joshcanhelp commented Jan 16, 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

Choose a reason for hiding this comment

cocojoe commented Jan 16, 2018

joshcanhelp commented Jan 16, 2018

cocojoe left a comment

Choose a reason for hiding this comment