Fix Connection update over-writing Connection settings. #582

joshcanhelp · 2018-11-07T21:52:38Z

Changes

This fixes a bug that can cause unintended changes to database connection settings when saving a new password policy. Specific changes:

Add object property checking to WP_Auth0_Api_Client::update_connection() to avoid unnecessary payload errors from the server.
Remove duplicate connection cleanup from WP_Auth0_Api_Operations::update_wordpress_connection()
Return old password policy setting if an API call fails during validation
Pass entire connection object to the update API call to avoid unintended settings changes in password policy validation and new connection creation
Add a large number of tests to confirm behavior
Docblocks to help investigation and refactoring in the future

References

WP forum report

Testing

This change adds unit test coverage
I included manual testing steps above, if applicable
This change has been tested on the latest version of the platform/language or why not

Scenario 1: Updates to the password policy in wp-admin

Start with a working WP-Auth0 install with a DB connection active
In the Auth0 dashboard, make sure the connection has a non-default setting (username required turned ON or custom DB migration)
In wp-admin, change the password policy and save

Expected behavior:

Password policy is changed, other settings are not affected.

Scenario 2: Connection creation during the setup wizard

Start with a WP install without WP-Auth0 installed
Activate the plugin and walk through the setup wizard (any option)

Expected behavior:

Install completes without errors in WordPress; New client created associated to new DB connection in Auth0.

Checklist

All existing and new tests complete without errors
All code quality tools/guidelines in the Contribution guide have been run/followed
All active GitHub CI checks have passed

joshcanhelp · 2018-11-07T21:55:23Z

lib/WP_Auth0_Api_Operations.php

@@ -30,10 +30,6 @@ public function update_wordpress_connection( $app_token, $connection_id, $passwo
 $connection->options->customScripts->login = $login_script;
 $connection->options->customScripts->get_user = $get_user_script;

- unset( $connection->name );


Moved to WP_Auth0_Api_Client::update_connection()

joshcanhelp · 2018-11-07T21:55:34Z

lib/WP_Auth0_Api_Operations.php

@@ -107,26 +103,21 @@ public function create_wordpress_connection( $app_token, $migration_enabled, $pa
 $connections = WP_Auth0_Api_Client::search_connection( $domain, $app_token, 'auth0' );
 $db_connection = null;

- $migration_connection_id = $response->id;
+ $created_connection_id = $response->id;


Confusing var name

joshcanhelp · 2018-11-07T21:55:50Z

lib/admin/WP_Auth0_Admin_Advanced.php

@@ -741,10 +741,6 @@ public function migration_ws_validation( $old_options, $input ) {
 $connection->options->enabledDatabaseCustomization = false;
 $connection->options->import_mode = false;

- unset( $connection->name );


Moved to WP_Auth0_Api_Client::update_connection()

joshcanhelp · 2018-11-07T21:56:23Z

lib/admin/WP_Auth0_Admin_Features.php

@@ -427,8 +427,8 @@ public function security_validation( $old_options, $input ) {

 foreach ( $connections as $connection ) {
 if ( in_array( $input['client_id'], $connection->enabled_clients ) ) {
- $patch = array( 'options' => array( 'passwordPolicy' => $input['password_policy'] ) );
- $update_resp = WP_Auth0_Api_Client::update_connection( $domain, $app_token, $connection->id, $patch );
+ $connection['options']['passwordPolicy'] = $input['password_policy'];


Here's the bug ... $patch did not include existing Connection settings

joshcanhelp · 2018-11-07T21:57:13Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php

@@ -128,19 +144,21 @@ public function consent_callback( $name ) {
 $connection_exists = false;
 $connection_pwd_policy = null;

- $connections = WP_Auth0_Api_Client::search_connection( $domain, $app_token );
+ $connections = WP_Auth0_Api_Client::search_connection( $domain, $app_token, 'auth0' );


Added strategy filter here to reduce the number of Connections returned.

If possible, can this be renamed to searchConnections (plural)?

Public static method so no, not currently. WP_Auth0_Api_Client is getting refactored one method at a time so it will go away soon.

joshcanhelp · 2018-11-08T00:53:18Z

lib/WP_Auth0_Api_Client.php

@@ -619,6 +630,14 @@ public static function update_connection( $domain, $app_token, $id, $payload ) {
 $headers['Authorization'] = "Bearer $app_token";
 $headers['content-type'] = 'application/json';

+ unset( $payload->name );


These must not be set or the API endpoint returns a payload error.

joshcanhelp · 2018-11-08T00:53:48Z

lib/WP_Auth0_Api_Client.php

+ unset( $payload->strategy );
+ unset( $payload->id );
+
+ if ( ! empty( $payload->enabled_clients ) ) {


If the array is not consecutive then json_encode converts this to an object rather than an array.

Consecutive as in following an ascending/descending order? 🤔 I don't understand why you're doing this. Can you rephrase please?

If $payload->enabled_clients is anything other than an array with consecutive keys (like, if one of the keys in the middle is removed) then json_encode() turns it into an object which is rejected by the server.

joshcanhelp · 2018-11-08T00:54:29Z

lib/WP_Auth0_Api_Operations.php

-
- $migration_connection_id = $response->id;
-
- foreach ( $connections as $connection ) {


This method is only called in one place and the calling method already does this check.

joshcanhelp · 2018-11-08T00:55:48Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php


- foreach ( $connections as $connection ) {
+ if ( $should_create_and_update_connection && ! empty( $connections ) && is_array( $connections ) ) {


Logic here is still not great but did not want to completely refactor this in a patch release. Main thing here is checking whether we have an array and avoiding the loop if we're not using the values later.

Sounds like the main check (the is $should_create_and_update_connection) is repeated right below this one. What about wrapping those like this... ?

if (!$should_create_and_update_connection){ return; //early exit, no need to nest another if in that case 👍 } if (! empty( $connections ) && is_array( $connections )){ //continue with this logic return; //return here if you don't want to keep going down the code } if ( $connection_exists === false ) { // the remaining stuff }

I don't agree that doing this in a patch release is wrong in anyway. On the contrary, you're improving code readability and that's a plus for any PR.

I understand but looking for the minimum number of changes here. This is just one of many things that should be addressed in this method.

joshcanhelp · 2018-11-08T00:56:15Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php

- $connection_exists = $connection->id;
- $connection_pwd_policy = ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) ? $connection->options->passwordPolicy : null;
+ $connection_exists = $connection->id;
+ if ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) {


Ternary was too long and confusing to read.

In those cases what I recommend is extracting the boolean to a variable.

e.g.

$flag = isset( $connection->options ) && isset( $connection->options->passwordPolicy ); if ($flag){ //do flag thing }

Or even better, have a private method somewhere on the file that given a connection would return the password policy value, if any.

That a side, couldn't you get this connection id and pwd policy when you created it? Or you want to consider the case where the user would have changed it?

Lots of better ways to do this, yes :)

joshcanhelp · 2018-11-08T00:56:30Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php

 } else {
- $enabled_clients = array_diff( $connection->enabled_clients, array( $client_id ) );
- WP_Auth0_Api_Client::update_connection( $domain, $app_token, $connection->id, array( 'enabled_clients' => array_values( $enabled_clients ) ) );
+ $connection->enabled_clients = array_diff( $connection->enabled_clients, array( $client_id ) );


Avoiding the same settings override as reported in the original bug.

codecov-io · 2018-11-08T20:07:27Z

Codecov Report

Merging #582 into master will increase coverage by 6.4%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             master    #582     +/-   ##
==========================================
+ Coverage     21.29%   27.7%   +6.4%     
+ Complexity     1308    1307      -1     
==========================================
  Files            51      51             
  Lines          4263    4198     -65     
==========================================
+ Hits            908    1163    +255     
+ Misses         3355    3035    -320

Impacted Files	Coverage Δ	Complexity Δ
lib/admin/WP_Auth0_Admin_Advanced.php	`19.48% <ø> (+0.16%)`	`64 <0> (ø)`	⬇️
lib/WP_Auth0_Api_Client.php	`31.37% <100%> (+29.56%)`	`85 <0> (+1)`	⬆️
lib/admin/WP_Auth0_Admin_Features.php	`13.1% <100%> (+13.1%)`	`39 <0> (ø)`	⬇️
lib/WP_Auth0_Api_Operations.php	`51.72% <100%> (+51.72%)`	`15 <0> (-3)`	⬇️
...ib/initial-setup/WP_Auth0_InitialSetup_Consent.php	`63.63% <90.9%> (+63.63%)`	`26 <0> (+1)`	⬆️
WP_Auth0.php	`24.18% <0%> (+2.45%)`	`64% <0%> (ø)`	⬇️
lib/WP_Auth0_Options.php	`85.14% <0%> (+12.87%)`	`28% <0%> (ø)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea429c...510335a. Read the comment docs.

joshcanhelp · 2018-11-08T20:08:58Z

lib/admin/WP_Auth0_Admin_Features.php

@@ -423,19 +423,24 @@ public function security_validation( $old_options, $input ) {
 __( 'No database connections found for this application. ', 'wp-auth0' ) .
 $this->get_dashboard_link( 'connections/database', __( 'See all database connections', 'wp-auth0' ) )
 );
+ $input['password_policy'] = $old_options['password_policy'];
+ return $input;


Exit here to skip anything else that might process (foreach will throw an error if it's given anything other than an array)

joshcanhelp · 2018-11-08T20:09:18Z

lib/admin/WP_Auth0_Admin_Features.php

@@ -423,19 +423,24 @@ public function security_validation( $old_options, $input ) {
 __( 'No database connections found for this application. ', 'wp-auth0' ) .
 $this->get_dashboard_link( 'connections/database', __( 'See all database connections', 'wp-auth0' ) )
 );
+ $input['password_policy'] = $old_options['password_policy'];


Nothing changed at Auth0 so we should not change the setting in WordPress either.

joshcanhelp · 2018-11-08T20:09:22Z

lib/admin/WP_Auth0_Admin_Features.php


 if ( false === $update_resp ) {
 $this->add_validation_error(
 __( 'There was a problem updating the password policy. ', 'wp-auth0' ) .
 __( 'Please manually review and update the policy. ', 'wp-auth0' ) .
 $this->get_dashboard_link( 'connections/database', __( 'See all database connections', 'wp-auth0' ) )
 );
+ $input['password_policy'] = $old_options['password_policy'];


Nothing changed at Auth0 so we should not change the setting in WordPress either.

lbalmaceda

if the user is giving you the API2 token make sure they know which scopes they must request

lbalmaceda · 2018-11-08T18:49:34Z

lib/WP_Auth0_Api_Client.php

+ unset( $payload->strategy );
+ unset( $payload->id );
+
+ if ( ! empty( $payload->enabled_clients ) ) {


Consecutive as in following an ascending/descending order? 🤔 I don't understand why you're doing this. Can you rephrase please?

lbalmaceda · 2018-11-08T19:33:06Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php

+ * @param string $domain - Auth0 domain for the Application.
+ * @param string $access_token - Management API access token.
+ * @param string $type - Installation type, "social" (AKA standard) or "enterprise".
+ * @param bool $hasInternetConnection - True if the installing site be reached by Auth0, false if not.


what is this last param used for? Seems it's being set as part of a migration step, but depending on when that's read it might be out of context. Shouldn't this be checked in that other place and time instead?

Just documenting, not refactoring

lbalmaceda · 2018-11-08T19:38:08Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php


- foreach ( $connections as $connection ) {
+ if ( $should_create_and_update_connection && ! empty( $connections ) && is_array( $connections ) ) {


Sounds like the main check (the is $should_create_and_update_connection) is repeated right below this one. What about wrapping those like this... ?

if (!$should_create_and_update_connection){ return; //early exit, no need to nest another if in that case 👍 } if (! empty( $connections ) && is_array( $connections )){ //continue with this logic return; //return here if you don't want to keep going down the code } if ( $connection_exists === false ) { // the remaining stuff }

I don't agree that doing this in a patch release is wrong in anyway. On the contrary, you're improving code readability and that's a plus for any PR.

lbalmaceda · 2018-11-08T19:44:52Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php


- if ( in_array( $client_id, $connection->enabled_clients ) ) {
- if ( $connection->strategy === 'auth0' && $should_create_and_update_connection ) {
+ if ( in_array( $client_id, $connection->enabled_clients ) ) {


Would be nice if you could provide in-line comments (in the code) to help anyone follow the logic.

lbalmaceda · 2018-11-08T19:48:44Z

lib/initial-setup/WP_Auth0_InitialSetup_Consent.php

- $connection_exists = $connection->id;
- $connection_pwd_policy = ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) ? $connection->options->passwordPolicy : null;
+ $connection_exists = $connection->id;
+ if ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) {


In those cases what I recommend is extracting the boolean to a variable.

e.g.

$flag = isset( $connection->options ) && isset( $connection->options->passwordPolicy ); if ($flag){ //do flag thing }

Or even better, have a private method somewhere on the file that given a connection would return the password policy value, if any.

That a side, couldn't you get this connection id and pwd policy when you created it? Or you want to consider the case where the user would have changed it?

lbalmaceda · 2018-11-08T20:36:23Z

tests/testApiOperations.php

+ );
+
+ $this->assertContains(
+ 'form:{username:email, access_token:"TEST_MIGRATION_TOKEN',


is this intentionally not having a } at the end of the string in test?

Yes, next character is a newline

lbalmaceda · 2018-11-08T20:39:12Z

tests/testInitialSetupConsent.php

+
+ $caught_redirect = [];
+ try {
+ $setup_consent->callback_with_token( $test_domain, $test_token, 'invalid_state' );


what's the previous context? how can you ensure that invalid_state is an invalid state? normally, you'd manually set that state before this call.

There are only two values that callback_with_token accept here ... not actually state but that's the name used in the method.

lbalmaceda · 2018-11-08T20:40:32Z

tests/testInitialSetupConsent.php

+
+ $caught_http = [];
+ try {
+ $setup_consent->callback_with_token( 'test-wp.auth0.com', $test_token, 'social' );


how is this test logic different from the "test invalid state redirects"? Sounds like whatever caused the above test to fail should do the same here. What's changing? and if nothing is, why would an invalid_state scenario still make the create client request?

"social" is one of two valid "state" values here so this proceeds to the API call

lbalmaceda · 2018-11-08T20:44:17Z

tests/testInitialSetupConsent.php

+ 'success_update_connection',
+ // Connection created successfully.
+ 'success_create_connection',
+ // Client grant failed.


why would this happen? Seems like this should belong in a different test case than "create new connection succeeds"

Maybe the token doesn't have the right scopes or HTTP error or any number of things.

In that case, that could be moved to a different test other than the "happy path" of creating the connection when missing

lbalmaceda · 2018-11-08T20:45:39Z

tests/traits/httpHelpers.php

@@ -62,7 +62,7 @@ public function stopHttpHalting() {
 * Use this at the top of tests that should test behavior for different HTTP responses.
 */
 public function startHttpMocking() {
- add_filter( 'pre_http_request', [ $this, 'httpMock' ], 1 );
+ add_filter( 'pre_http_request', [ $this, 'httpMock' ], 1, 3 );


what changed here?

Technically nothing but that last arg is the number of params that the hooked function receives.

lbalmaceda

LGTM 🎉

joshcanhelp commented Nov 7, 2018

View reviewed changes

joshcanhelp force-pushed the fix-password-policy-validation branch from e482fec to aff92b2 Compare November 8, 2018 00:52

joshcanhelp commented Nov 8, 2018

View reviewed changes

joshcanhelp changed the title ~~Fix Connection update over-writing Connection settings.~~ [WIP] Fix Connection update over-writing Connection settings. Nov 8, 2018

joshcanhelp force-pushed the fix-password-policy-validation branch from aff92b2 to cdb75ce Compare November 8, 2018 18:02

auth0 deleted a comment from codecov-io Nov 8, 2018

Fix Connection update over-writing Connection settings.

f60951e

joshcanhelp force-pushed the fix-password-policy-validation branch from cdb75ce to f60951e Compare November 8, 2018 18:21

auth0 deleted a comment from codecov-io Nov 8, 2018

Tests for password policy validation; update to test suite for halting

510335a

joshcanhelp commented Nov 8, 2018

View reviewed changes

joshcanhelp added the CH: Fixed label Nov 8, 2018

joshcanhelp changed the title ~~[WIP] Fix Connection update over-writing Connection settings.~~ Fix Connection update over-writing Connection settings. Nov 8, 2018

joshcanhelp added this to the 3.8.1 milestone Nov 8, 2018

lbalmaceda reviewed Nov 8, 2018

View reviewed changes

auth0 deleted a comment from lbalmaceda Nov 8, 2018

PR feedback

d9d9e5d

auth0 deleted a comment from lbalmaceda Nov 8, 2018

lbalmaceda approved these changes Nov 8, 2018

View reviewed changes

joshcanhelp merged commit c9d09ac into master Nov 9, 2018

joshcanhelp deleted the fix-password-policy-validation branch November 9, 2018 16:57

github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022


		$migration_connection_id = $response->id;

		foreach ( $connections as $connection ) {


		foreach ( $connections as $connection ) {
		if ( $should_create_and_update_connection && ! empty( $connections ) && is_array( $connections ) ) {

Fix Connection update over-writing Connection settings. #582

Fix Connection update over-writing Connection settings. #582

Conversation

joshcanhelp commented Nov 7, 2018 • edited Loading

Changes

References

Testing

Scenario 1: Updates to the password policy in wp-admin

Scenario 2: Connection creation during the setup wizard

Checklist

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

codecov-io commented Nov 8, 2018 • edited Loading

Codecov Report

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

lbalmaceda left a comment

Choose a reason for hiding this comment

joshcanhelp commented Nov 7, 2018 •

edited

Loading

codecov-io commented Nov 8, 2018 •

edited

Loading