Change auto-login action #449

joshcanhelp · 2018-04-24T19:35:28Z

Change auto-login logic to be more clear and explicit
Change implicit to handle nonce correctly
Add authorize URL param builder
Fix callback error handling for implicit and redirect

joshcanhelp · 2018-04-24T19:36:51Z

lib/WP_Auth0_LoginManager.php

@@ -156,8 +147,8 @@ public function init_auth0() {
 // Catch any incoming errors and stop the login process.
 // See https://auth0.com/docs/libraries/error-messages for more info.
 if ( $this->query_vars( 'error' ) || $this->query_vars( 'error_description' ) ) {
- $error_msg = sanitize_text_field( $this->query_vars( 'error_description' ) );
- $error_code = sanitize_text_field( $this->query_vars( 'error' ) );
+ $error_msg = sanitize_text_field( rawurldecode( $_REQUEST[ 'error_description' ] ) );


$this->query_vars() checks for WP query vars first, which was leaving out the error parameter for some reason. Switched to a more explicit check and now pulls error more consistently.

Why does this call rawurldecode first when the line below doesn't?

My thought was that the error code is usually a machine name without any characters that need decoding but that's a little bit of wishful thinking ... and no harm decoding that as well.

Honestly, the errors returned by the server should be used directly. No need to parse nor decode them at all (besides JSON if you need to translate them to an Object structure). Some error descriptions might include URLs to help articles. I wouldn't do any urldecoding here.

Error descriptions typically include at least spaces, if not other characters. Without urldecoding it, you'll get "This%20is%20an%20error" and such. URLs in a URL definitely need to be encoded.

I've never seen an error code have decode-able characters, though, which was why I left it out.

Ah yes, you're right! I thought you were obtaining this from a JSON body, not from the query. Now that I read, the param says "query_vars".

joshcanhelp · 2018-04-24T19:37:27Z

lib/WP_Auth0_LoginManager.php

- __( 'Invalid nonce', 'wp-auth0' )
- );
- }
+ $nonce = isset( $decoded_token->nonce ) ? $decoded_token->nonce : null;


Nonce now works for any implicit login 🎉

joshcanhelp · 2018-04-24T19:37:57Z

lib/WP_Auth0_LoginManager.php

+ *
+ * @return string
+ */
+ public static function get_userinfo_scope( $context = '' ) {


Moved default scopes to here so they can be filtered

Also $context is only for that filter. I pass in a context when I request those scopes so the filter function can decide to do something different based on where it's being used.

how does this method work? I saw you call it passing sso and lock. The method should have a single known behavior. What's the input/output?

On a basic install with no customizations, 'openid email name nickname picture' will always be returned. If someone wants to modify the scopes that are being requested, they can use the auth0_auth_token_scope filter to add/remove. The $context is there to provide ... context for where those scopes are used. This is a pattern used in i18n in WordPress where a word might be different depending on context.

Adjusted this to use an array for filtering ... better data structure for what's being done here.

joshcanhelp · 2018-04-24T19:38:42Z

lib/WP_Auth0_Nonce_Handler.php

@@ -12,7 +12,7 @@
 /**
 *
 */
- const COOKIE_EXPIRES = MINUTE_IN_SECONDS;
+ const COOKIE_EXPIRES = HOUR_IN_SECONDS;


Longer state cookie, was failing if you left the login page open for more than a minute.

lbalmaceda · 2018-04-25T20:19:14Z

lib/WP_Auth0_LoginManager.php

@@ -156,8 +147,8 @@ public function init_auth0() {
 // Catch any incoming errors and stop the login process.
 // See https://auth0.com/docs/libraries/error-messages for more info.
 if ( $this->query_vars( 'error' ) || $this->query_vars( 'error_description' ) ) {
- $error_msg = sanitize_text_field( $this->query_vars( 'error_description' ) );
- $error_code = sanitize_text_field( $this->query_vars( 'error' ) );
+ $error_msg = sanitize_text_field( rawurldecode( $_REQUEST[ 'error_description' ] ) );


Why does this call rawurldecode first when the line below doesn't?

lbalmaceda · 2018-04-25T20:20:34Z

lib/WP_Auth0_LoginManager.php

- );
- }
+ $nonce = isset( $decoded_token->nonce ) ? $decoded_token->nonce : null;
+ if ( ! WP_Auth0_Nonce_Handler::getInstance()->validate( $nonce ) ) {


Will this work fine (fail) if the nonce was not found (null) in the id token and the flow was implicit?

Yes. Regardless of whether we have a value or not, that validate() method checks against what's stored. Since null is not equal to whatever string we stored, this will return false.

lbalmaceda · 2018-04-25T20:22:51Z

lib/WP_Auth0_LoginManager.php

+ *
+ * @return string
+ */
+ public static function get_userinfo_scope( $context = '' ) {


how does this method work? I saw you call it passing sso and lock. The method should have a single known behavior. What's the input/output?

lbalmaceda · 2018-04-25T20:24:56Z

lib/WP_Auth0_LoginManager.php

+ }
+
+ /**
+ * Get a base authorize URL for handling HLP redirects.


HLP -> Universal Login Page

lbalmaceda · 2018-04-25T20:25:40Z

lib/WP_Auth0_LoginManager.php

+ /**
+ * Get a base authorize URL for handling HLP redirects.
+ *
+ * @param null|string $connection - specific connection to use, pass null to use all enabled.


I think it reads better this way:

specific connection to use, pass null to use all the enabled connections.

lbalmaceda · 2018-04-25T20:29:36Z

lib/WP_Auth0_LoginManager.php

+ $params[ 'response_type' ] = $is_implicit ? 'id_token': 'code';
+ $params[ 'redirect_uri' ] = $is_implicit
+ ? $lock_options->get_implicit_callback_url()
+ : $options->get_wp_auth0_url( null, $is_implicit );


why use $is_implicit if you already know the value?

You know, when I create a variable, I don't want it to just do a job and go away ... I want that variable to experience it's short time in memory.

lbalmaceda · 2018-04-25T20:31:16Z

lib/WP_Auth0_LoginManager.php

+ }
+
+ // Get the telemetry header.
+ $telemetry = WP_Auth0_Api_Client::get_info_headers();


this object generates headers with lots of information, right? why use only Auth0-Client? what about the rest?

If you look at what WP_Auth0_Api_Client::get_info_headers() returns, you'll see. That's meant to built telemetry headers for API calls but the value of the array it returns can be used here. That was part of my comment about it being a good candidate for refactoring but beyond the scope of this release.

lbalmaceda · 2018-04-25T20:41:44Z

lib/WP_Auth0_LoginManager.php

+ $params[ 'auth0Client' ] = $telemetry[ 'Auth0-Client' ];
+
+ // Where should the user be redirected after logging in?
+ if ( empty( $redirect_to ) && ! empty( $_GET['redirect_to'] ) ) {


Line 608 talks about a redirect_uri. What would be the difference with this redirect_to parameter, and shouldn't they be merged or at least be closer? (lines distance in this file)

The redirect_uri is the one that /authorize is looking for whereas redirect_to is where the plugin sends them after they authenticate (sent with state below).

lbalmaceda · 2018-04-25T20:44:21Z

lib/WP_Auth0_LoginManager.php

+ $params[ 'state' ] = base64_encode( json_encode( array(
+ 'interim' => false,
+ 'nonce' => $nonce,
+ 'redirect_to' => filter_var( $redirect_to, FILTER_SANITIZE_URL ),


why do you need to manipulate the redirect_to value? can't you use directly the one defined just a few lines above this?

Since this could come from a URL param or a settings field, I want to make sure that it's an actual URL.

then why you don't do the same in the lines above?

Can you clarify what your question is here? Do what to which lines?

Line 628 or 630 define what $redirect_to is going to be. Then in this line (637) you set as the redirect_to property (inside the state object) filter_var( $redirect_to, FILTER_SANITIZE_URL ) as value. What's the difference and why do you need to call filter_var first this time?

Line 628 and 630 pull a raw value from the URL or from the database, could be anything. So if a redirect URL is not passed and there's one in the URL, get that. Otherwise if it's not passed but nothing in the URL, get the default. Then take that value, get rid of non-URL-y characters, and pack it up into state to be sent to Auth0 and returned. If that's null/empty in the callback then it will go to the homepage.

Hopefully that answers your question? If not, ask another way and I'll try again 😀

lbalmaceda · 2018-04-25T20:44:54Z

lib/WP_Auth0_Options.php

 $site_url = site_url( 'index.php', $protocol );
- return add_query_arg( 'auth0', '1', $site_url );
+ return add_query_arg( 'auth0', ( $implicit ? 'implicit' : '1' ), $site_url );


how are you handling this with a query param?

Determines the login flow to use:

https://github.com/auth0/wp-auth0/blob/dev/lib/WP_Auth0_LoginManager.php#L171

- Change auto-login logic to be more clear and explicit - Change implicit to handle nonce correctly - Add authorize URL param builder

lbalmaceda

LGTM 🎉

joshcanhelp added CH: Changed labels Apr 24, 2018

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

joshcanhelp commented Apr 24, 2018

View reviewed changes

lbalmaceda reviewed Apr 25, 2018

View reviewed changes

joshcanhelp added 2 commits April 25, 2018 15:44

Auto-login refactor

271448d

- Change auto-login logic to be more clear and explicit - Change implicit to handle nonce correctly - Add authorize URL param builder

PR review feedback

f3ff234

joshcanhelp force-pushed the change-auto-login-refactor branch from 5a5aac8 to f3ff234 Compare April 25, 2018 22:44

lbalmaceda approved these changes May 2, 2018

View reviewed changes

joshcanhelp merged commit 10c2794 into dev May 2, 2018

joshcanhelp removed the Ready for Review label May 2, 2018

joshcanhelp deleted the change-auto-login-refactor branch May 2, 2018 22:03

joshcanhelp mentioned this pull request May 3, 2018

Encode auth0Client in login redirect URL #448

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

Change auto-login action #449

Change auto-login action #449

joshcanhelp commented Apr 24, 2018

joshcanhelp Apr 24, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 26, 2018

joshcanhelp Apr 26, 2018

lbalmaceda Apr 26, 2018

joshcanhelp Apr 24, 2018

joshcanhelp Apr 24, 2018

joshcanhelp Apr 24, 2018 •

edited

Loading

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

joshcanhelp Apr 24, 2018

lbalmaceda Apr 25, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 25, 2018

lbalmaceda Apr 25, 2018

lbalmaceda Apr 25, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda Apr 26, 2018 •

edited

Loading

joshcanhelp Apr 26, 2018

lbalmaceda Apr 26, 2018

joshcanhelp Apr 27, 2018

lbalmaceda Apr 25, 2018

joshcanhelp Apr 25, 2018

lbalmaceda left a comment

Change auto-login action #449

Change auto-login action #449

Conversation

joshcanhelp commented Apr 24, 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

joshcanhelp Apr 24, 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

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 Apr 26, 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

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp Apr 24, 2018 •

edited

Loading

lbalmaceda Apr 26, 2018 •

edited

Loading