New class for state handling; set cookie for implicit nonce #458

joshcanhelp · 2018-05-04T13:38:53Z

Refactored state and nonce handling to extend a WP_Auth0_Random_Storage class.
Set a nonce cookie for implicit logins during Lock init
Set a nonce cookie for implicit auto-logins before redirect
Renamed methods to conform to WP code standards

joshcanhelp · 2018-05-04T13:39:43Z

lib/WP_Auth0_Nonce_Handler.php

@@ -1,134 +1,24 @@
 <?php

-final class WP_Auth0_Nonce_Handler {


Move to parent class

joshcanhelp · 2018-05-04T13:40:51Z

lib/WP_Auth0_Random_Storage.php

@@ -0,0 +1,121 @@
+<?php
+
+class WP_Auth0_Random_Storage {


This was previously the WP_Auth0_Nonce_Handler. I added a more generic cookie name and removed the verify method so extending classes can implement it themselves.

joshcanhelp · 2018-05-07T21:48:09Z

lib/WP_Auth0_LoginManager.php

-		if ( $this->query_vars( 'error' ) || $this->query_vars( 'error_description' ) ) {
-			$error_msg  = sanitize_text_field( rawurldecode( $_REQUEST[ 'error_description' ] ) );
-			$error_code = sanitize_text_field( rawurldecode( $_REQUEST[ 'error' ] ) );
+		if ( ! empty( $_REQUEST['error'] ) || ! empty( $_REQUEST['error_description'] ) ) {


query_vars was causing problems in the state retrieval so I changed that here as well.

joshcanhelp · 2018-05-07T21:48:31Z

lib/WP_Auth0_LoginManager.php

@@ -228,8 +217,7 @@ public function redirect_login() {

 			// Look for clues as to what went wrong.
 			$e_message = ! empty( $data->error_description ) ? $data->error_description : __( 'Unknown error', 'wp-auth0' );
-			$e_code    = ! empty( $data->error ) ? $data->error : $exchange_resp_code;
-			throw new WP_Auth0_LoginFlowValidationException( $e_message, $e_code );
+			throw new WP_Auth0_LoginFlowValidationException( $e_message, $exchange_resp_code );


Didn't like the string error code ... found during debugging

joshcanhelp · 2018-05-07T21:53:23Z

lib/WP_Auth0_LoginManager.php

@@ -608,30 +595,30 @@ public static function get_userinfo_scope( $context = '' ) {
 	 * @return array
 	 */
 	public static function get_authorize_params( $connection = null, $redirect_to = null ) {


Mainly whitespace changes in this method

lbalmaceda

I'd like to discuss the inheritance of the new storage classes with you.

lbalmaceda · 2018-05-09T17:25:31Z

lib/WP_Auth0_LoginManager.php

 		$auth_params = self::get_authorize_params( $connection );

 		$auth_url = 'https://' . $this->a0_options->get( 'domain' ) . '/authorize';
 		$auth_url = add_query_arg( array_map( 'rawurlencode', $auth_params ), $auth_url );

-		setcookie( WPA0_STATE_COOKIE_NAME, $auth_params['state'], time() + WP_Auth0_Nonce_Handler::COOKIE_EXPIRES, '/' );
+		WP_Auth0_State_Handler::get_instance()->setStateCookie( $auth_params['state'] );


I guess this line doesn't check for isset (state) because state is ALWAYS sent, right?

Correct. That array comes from the get_authorize_params() method in this same class and is always generated.

lbalmaceda · 2018-05-09T17:46:57Z

lib/WP_Auth0_LoginManager.php

@@ -326,7 +314,7 @@ public function implicit_login() {

 		// Validate the nonce if one was included in the request if using auto-login.
 		$nonce = isset( $decoded_token->nonce ) ? $decoded_token->nonce : null;
-		if ( ! WP_Auth0_Nonce_Handler::getInstance()->validate( $nonce ) ) {
+		if ( ! WP_Auth0_Nonce_Handler::get_instance()->validate( $nonce ) ) {


are you following the same method naming convention everywhere? Asking since I see a misxture for class names, i.e. WP_Auth0_LoginFlowValidationException

For new methods, I'm following the WP standard, which is snake-case. There was no naming convention, really, up until now.

lbalmaceda · 2018-05-09T17:59:13Z

lib/WP_Auth0_Nonce_Handler.php

+/**
+ * Contains WP_Auth0_Nonce_Handler.


I'm seeing 2 docs for this class definition. Should they be merged?

It's a file and a class comment. Duplicative but part of the code quality scan.

lbalmaceda · 2018-05-09T18:11:13Z

lib/WP_Auth0_Nonce_Handler.php

+	 * @var WP_Auth0_Nonce_Handler|null
+	 */
+	protected static $_instance = null;


No need to make it protected.

I actually think that I do in this case. The classes that extend WP_Auth0_Random_Storage use late static binding to call the child class from the parent (static when you see that). A private property here would affect how that functions. If I switch these to private, I get:

Fatal error: Access level to WP_Auth0_Nonce_Handler::$_instance must be protected (as in class WP_Auth0_Random_Storage) or weaker in /Users/josh-cunningham/Sites/wp-auth0-v1/wp-content/plugins/auth0/lib/WP_Auth0_Nonce_Handler.php on line 12

... and they cant be public so there we are.

you already have a getInstance method (the singleton getter) on the super class. Use that from the child ones.

Yes, I do have that and do use that but the child classes need to have an _instance property for that to work.

lbalmaceda · 2018-05-09T18:13:18Z

lib/WP_Auth0_State_Handler.php

+	 *
+	 * @var WP_Auth0_State_Handler|null
+	 */
+	protected static $_instance = null;


No need to make it protected.

See above ☝️

lbalmaceda · 2018-05-09T19:19:28Z

lib/WP_Auth0_State_Handler.php

+final class WP_Auth0_State_Handler extends WP_Auth0_Random_Storage {
+
+	/**
+	 * Cookie name used to store nonce


nonce or state?

nonce is correct in this case. This nonce is added to a state object

lbalmaceda · 2018-05-09T19:19:31Z

lib/WP_Auth0_State_Handler.php

+	const UNIQID_COOKIE_NAME = 'auth0_state_uniqid';
+
+	/**
+	 * Cookie name used to store nonce


nonce or state?

lbalmaceda · 2018-05-09T19:20:47Z

lib/WP_Auth0_Nonce_Handler.php

+	 * @var string
+	 */
+	const UNIQID_COOKIE_NAME = 'auth0_nonce_uniqid';


shouldn't this be called NONE_COOKIE_NAME or even COOKIE_NAME? Since it's already inside the nonce handler class

Synonymous as used. The uniqid is used as a nonce in the Nonce class.

lbalmaceda · 2018-05-09T19:21:47Z

lib/WP_Auth0_State_Handler.php

+	 *
+	 * @var string
+	 */
+	const STATE_COOKIE_NAME = 'auth0_state';


wait, what's the difference between this one and the uniqid one?

State is state, nonce is the salt in state.

lbalmaceda · 2018-05-09T19:22:08Z

lib/WP_Auth0_State_Handler.php

+	 *
+	 * @return bool
+	 */
+	public function validate() {


see my long comment over class inheritance and protected methods.

lbalmaceda · 2018-05-10T19:32:37Z

lib/WP_Auth0_LoginManager.php

-		if ( $this->query_vars( 'error' ) || $this->query_vars( 'error_description' ) ) {
-			$error_msg  = sanitize_text_field( rawurldecode( $_REQUEST[ 'error_description' ] ) );
-			$error_code = sanitize_text_field( rawurldecode( $_REQUEST[ 'error' ] ) );
+		if ( ! empty( $_REQUEST['error'] ) || ! empty( $_REQUEST['error_description'] ) ) {


would an error_description by itself be valid? I guess only when error is present. So 2nd clause could be removed.

lbalmaceda · 2018-05-10T19:34:43Z

lib/WP_Auth0_LoginManager.php

 			$this->die_on_login( $error_msg, $error_code );
 		}

 		// Check for valid state nonce, set in WP_Auth0_Lock10_Options::get_state_obj().
 		// See https://auth0.com/docs/protocols/oauth2/oauth-state for more info.
-		if ( ! $this->validate_state() ) {
+		$state_returned = isset( $_REQUEST['state'] ) ? rawurldecode( $_REQUEST['state'] ) : null;
+		if ( ! $state_returned || ! WP_Auth0_State_Handler::get_instance()->validate( $state_returned ) ) {


! $state_returned is already covered inside the verify method

lbalmaceda · 2018-05-10T19:36:38Z

lib/WP_Auth0_Nonce_Handler.php

+		} else {
+			// No cookie, need to create one.
+			$this->_uniqid = $this->generate_nonce();


don't you need to set the generated value to $_COOKIE?

lbalmaceda · 2018-05-10T19:38:25Z

lib/WP_Auth0_Nonce_Handler.php

+	 * @return bool
+	 */
+	public function set_cookie() {


shouldn't this be called internally after generating a new nonce? (private). It doesn't receive any param so I don't see the point of it being public at all

lbalmaceda

LGTM

joshcanhelp added CH: Changed labels May 4, 2018

joshcanhelp added this to the 3.6.0 milestone May 4, 2018

joshcanhelp commented May 4, 2018

View reviewed changes

joshcanhelp added work in progress and removed Ready for Review labels May 4, 2018

joshcanhelp force-pushed the change-state-and-nonce-handling branch 4 times, most recently from 0f63a3f to 9cee822 Compare May 7, 2018 21:34

joshcanhelp added Ready for Review and removed work in progress labels May 7, 2018

joshcanhelp commented May 7, 2018

View reviewed changes

joshcanhelp force-pushed the change-state-and-nonce-handling branch 2 times, most recently from f52c688 to 2bf199a Compare May 9, 2018 17:12

lbalmaceda reviewed May 9, 2018

View reviewed changes

joshcanhelp force-pushed the change-state-and-nonce-handling branch 3 times, most recently from 0112dd4 to 02dc5b5 Compare May 10, 2018 18:49

lbalmaceda reviewed May 10, 2018

View reviewed changes

joshcanhelp force-pushed the change-state-and-nonce-handling branch 3 times, most recently from 9912d9d to 81f15f4 Compare May 10, 2018 20:34

lbalmaceda approved these changes May 10, 2018

View reviewed changes

New class for state handling; set cookie for implicit nonce

d13201d

joshcanhelp force-pushed the change-state-and-nonce-handling branch from 81f15f4 to d13201d Compare May 10, 2018 22:00

joshcanhelp removed the Ready for Review label May 10, 2018

joshcanhelp merged commit 014529c into dev May 10, 2018

joshcanhelp deleted the change-state-and-nonce-handling branch May 10, 2018 22:01

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

New class for state handling; set cookie for implicit nonce #458

New class for state handling; set cookie for implicit nonce #458

joshcanhelp commented May 4, 2018 •

edited

Loading

joshcanhelp May 4, 2018

joshcanhelp May 4, 2018

joshcanhelp May 7, 2018

joshcanhelp May 7, 2018

joshcanhelp May 7, 2018

lbalmaceda left a comment

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 10, 2018

joshcanhelp May 10, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

joshcanhelp May 9, 2018

lbalmaceda May 9, 2018

lbalmaceda May 10, 2018

lbalmaceda May 10, 2018

lbalmaceda May 10, 2018

lbalmaceda May 10, 2018

lbalmaceda left a comment

New class for state handling; set cookie for implicit nonce #458

New class for state handling; set cookie for implicit nonce #458

Conversation

joshcanhelp commented May 4, 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

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

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 May 4, 2018 •

edited

Loading