-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Connection update over-writing Connection settings. #582
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -611,6 +611,17 @@ public static function get_connection( $domain, $app_token, $id ) { | |
return json_decode( $response['body'] ); | ||
} | ||
|
||
/** | ||
* Update a Connection via the Management API. | ||
* Note: $payload must be a complete settings object, not just the property to change. | ||
* | ||
* @param string $domain - Auth0 Domain. | ||
* @param string $app_token - Valid Auth0 Management API token. | ||
* @param string $id - DB Connection ID. | ||
* @param stdClass $payload - DB Connection settings, will override existing. | ||
* | ||
* @return bool|object | ||
*/ | ||
public static function update_connection( $domain, $app_token, $id, $payload ) { | ||
$endpoint = "https://$domain/api/v2/connections/$id"; | ||
|
||
|
@@ -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 ); | ||
unset( $payload->strategy ); | ||
unset( $payload->id ); | ||
|
||
if ( ! empty( $payload->enabled_clients ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the array is not consecutive then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consecutive as in following an ascending/descending order? 🤔 I don't understand why you're doing this. Can you rephrase please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
$payload->enabled_clients = array_values( $payload->enabled_clients ); | ||
} | ||
|
||
$response = wp_remote_post( | ||
$endpoint, | ||
array( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to |
||
unset( $connection->strategy ); | ||
unset( $connection->id ); | ||
|
||
WP_Auth0_Api_Client::update_connection( $domain, $app_token, $connection_id, $connection ); | ||
|
||
} | ||
|
@@ -99,34 +95,10 @@ public function create_wordpress_connection( $app_token, $migration_enabled, $pa | |
$response = WP_Auth0_Api_Client::create_connection( $domain, $app_token, $body ); | ||
|
||
if ( $response === false ) { | ||
|
||
return false; | ||
|
||
} | ||
|
||
$connections = WP_Auth0_Api_Client::search_connection( $domain, $app_token, 'auth0' ); | ||
$db_connection = null; | ||
|
||
$migration_connection_id = $response->id; | ||
|
||
foreach ( $connections as $connection ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is only called in one place and the calling method already does this check. |
||
if ( $migration_connection_id != $connection->id && in_array( $client_id, $connection->enabled_clients ) ) { | ||
$db_connection = $connection; | ||
|
||
$enabled_clients = array_diff( $db_connection->enabled_clients, array( $client_id ) ); | ||
|
||
WP_Auth0_Api_Client::update_connection( | ||
$domain, | ||
$app_token, | ||
$db_connection->id, | ||
array( | ||
'enabled_clients' => array_values( $enabled_clients ), | ||
) | ||
); | ||
} | ||
} | ||
|
||
return $migration_connection_id; | ||
return $response->id; | ||
} | ||
|
||
// $input['geo_rule'] = ( isset( $input['geo_rule'] ) ? $input['geo_rule'] : 0 ); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -741,10 +741,6 @@ public function migration_ws_validation( $old_options, $input ) { | |
$connection->options->enabledDatabaseCustomization = false; | ||
$connection->options->import_mode = false; | ||
|
||
unset( $connection->name ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to |
||
unset( $connection->strategy ); | ||
unset( $connection->id ); | ||
|
||
$response = WP_Auth0_Api_Client::update_connection( $input['domain'], $input['auth0_app_token'], $old_options['db_connection_id'], $connection ); | ||
} else { | ||
$response = false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,16 @@ public function __construct( WP_Auth0_Options $a0_options ) { | |
public function render( $step ) { | ||
} | ||
|
||
/** | ||
* Used by both Setup Wizard installation flows. | ||
* Called in WP_Auth0_InitialSetup_ConnectionProfile::callback() when an API token is used during install. | ||
* Called in self::callback() when returning from consent URL install. | ||
* | ||
* @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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just documenting, not refactoring |
||
*/ | ||
public function callback_with_token( $domain, $access_token, $type, $hasInternetConnection = true ) { | ||
|
||
$this->a0_options->set( 'auth0_app_token', $access_token ); | ||
|
@@ -92,6 +102,12 @@ public function exchange_code() { | |
return $obj->access_token; | ||
} | ||
|
||
/** | ||
* Used by both Setup Wizard installation flows. | ||
* Called by self::callback_with_token() to create a Client, Connection, and Client Grant. | ||
* | ||
* @param string $name - Client name to use. | ||
*/ | ||
public function consent_callback( $name ) { | ||
|
||
$domain = $this->a0_options->get( 'domain' ); | ||
|
@@ -128,19 +144,22 @@ 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' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added strategy filter here to reduce the number of Connections returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If possible, can this be renamed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Public static method so no, not currently. |
||
|
||
foreach ( $connections as $connection ) { | ||
if ( $should_create_and_update_connection && ! empty( $connections ) && is_array( $connections ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like the main check (the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
foreach ( $connections as $connection ) { | ||
|
||
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 ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice if you could provide in-line comments (in the code) to help anyone follow the logic. |
||
|
||
if ( $db_connection_name === $connection->name ) { | ||
$connection_exists = $connection->id; | ||
$connection_pwd_policy = ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) ? $connection->options->passwordPolicy : null; | ||
$connection_exists = $connection->id; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be renamed? I get why you'd call it like that, but down in the code you store "connection exists" as the "connection id". I can't think now of a better name 😛 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessary for this PR |
||
if ( isset( $connection->options ) && isset( $connection->options->passwordPolicy ) ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ternary was too long and confusing to read. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lots of better ways to do this, yes :) |
||
$connection_pwd_policy = $connection->options->passwordPolicy; | ||
} | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of //Inside a loop:
if (condition){
//do if
continue;
}
//do else Though I have to say I looked at the "final code" (not just the diff) and looks OK. |
||
$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 ) ) ); | ||
$u_connection = clone $connection; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a comment above this line: "//disable connection for this application/client id" |
||
$u_connection->enabled_clients = array_diff( $u_connection->enabled_clients, array( $client_id ) ); | ||
WP_Auth0_Api_Client::update_connection( $domain, $app_token, $u_connection->id, $u_connection ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what if any of this requests fail? How are you going to capture those errors and what do you plan to do next? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK to continue it they fail. That method logs it internally. |
||
} | ||
} | ||
} | ||
|
@@ -191,6 +210,6 @@ public function consent_callback( $name ) { | |
} | ||
|
||
wp_redirect( admin_url( 'admin.php?page=wpa0-setup&step=2&profile=' . $this->state ) ); | ||
exit(); | ||
exit; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was this change intended? |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
<?php | ||
/** | ||
* Contains Class TestApiOperations. | ||
* | ||
* @package WP-Auth0 | ||
* | ||
* @since 3.8.1 | ||
*/ | ||
|
||
use PHPUnit\Framework\TestCase; | ||
|
||
/** | ||
* Class TestApiOperations. | ||
*/ | ||
class TestApiOperations extends TestCase { | ||
|
||
use HttpHelpers; | ||
|
||
use RedirectHelpers; | ||
|
||
use SetUpTestDb; | ||
|
||
/** | ||
* Instance of WP_Auth0_Options. | ||
* | ||
* @var WP_Auth0_Options | ||
*/ | ||
public static $opts; | ||
|
||
/** | ||
* WP_Auth0_ErrorLog instance. | ||
* | ||
* @var WP_Auth0_ErrorLog | ||
*/ | ||
protected static $error_log; | ||
|
||
/** | ||
* Setup for entire test class. | ||
*/ | ||
public static function setUpBeforeClass() { | ||
parent::setUpBeforeClass(); | ||
self::$opts = WP_Auth0_Options::Instance(); | ||
self::$error_log = new WP_Auth0_ErrorLog(); | ||
} | ||
|
||
/** | ||
* Test that a basic create connection command requests properly. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. normally the test name is enought, no need to add docblocks on tests. Though now that you added them no need to delete them, just for future reference 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code sniffer beeps at me without it |
||
*/ | ||
public function testThatCreateConnectionRequestsCorrectly() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Odd naming. Consider the suffix you've used above "WithoutErrors". Applies to more tests on this file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm checking the request that's being sent, not an outcome. |
||
$this->startHttpHalting(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this sounds like a candidate for a "pre each test" method, even if not used in all the tests cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't halt and mock at the same time though ... some improvements are possible here, for sure. |
||
|
||
$api_ops = new WP_Auth0_Api_Operations( self::$opts ); | ||
$test_token = implode( '.', [ uniqid(), uniqid(), uniqid() ] ); | ||
|
||
self::$opts->set( 'domain', 'test-wp.auth0.com' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you changing the |
||
self::$opts->set( 'client_id', 'TEST_CLIENT_ID' ); | ||
|
||
$caught_http = []; | ||
try { | ||
$api_ops->create_wordpress_connection( $test_token, false, 'good' ); | ||
} catch ( Exception $e ) { | ||
$caught_http = unserialize( $e->getMessage() ); | ||
} | ||
|
||
$this->assertEquals( 'https://test-wp.auth0.com/api/v2/connections', $caught_http['url'] ); | ||
$this->assertEquals( 'Bearer ' . $test_token, $caught_http['headers']['Authorization'] ); | ||
$this->assertEquals( 'DB-Test-Blog', $caught_http['body']['name'] ); | ||
$this->assertEquals( 'auth0', $caught_http['body']['strategy'] ); | ||
$this->assertEquals( 'good', $caught_http['body']['options']['passwordPolicy'] ); | ||
$this->assertContains( 'TEST_CLIENT_ID', $caught_http['body']['enabled_clients'] ); | ||
} | ||
|
||
/** | ||
* Test that a migration create connection command requests properly. | ||
*/ | ||
public function testThatCreateConnectionWithMigrationRequestsCorrectly() { | ||
$this->startHttpHalting(); | ||
|
||
$api_ops = new WP_Auth0_Api_Operations( self::$opts ); | ||
$test_token = implode( '.', [ uniqid(), uniqid(), uniqid() ] ); | ||
|
||
self::$opts->set( 'domain', 'test-wp2.auth0.com' ); | ||
self::$opts->set( 'client_id', 'TEST_CLIENT_ID_2' ); | ||
|
||
$caught_http = []; | ||
try { | ||
$api_ops->create_wordpress_connection( $test_token, true, 'fair', 'TEST_MIGRATION_TOKEN' ); | ||
} catch ( Exception $e ) { | ||
$caught_http = unserialize( $e->getMessage() ); | ||
} | ||
|
||
$this->assertEquals( 'https://test-wp2.auth0.com/api/v2/connections', $caught_http['url'] ); | ||
$this->assertEquals( 'Bearer ' . $test_token, $caught_http['headers']['Authorization'] ); | ||
$this->assertEquals( 'DB-Test-Blog', $caught_http['body']['name'] ); | ||
$this->assertEquals( 'auth0', $caught_http['body']['strategy'] ); | ||
$this->assertContains( 'TEST_CLIENT_ID_2', $caught_http['body']['enabled_clients'] ); | ||
|
||
$this->assertEquals( true, $caught_http['body']['options']['requires_username'] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider, if possible, using |
||
$this->assertEquals( true, $caught_http['body']['options']['import_mode'] ); | ||
$this->assertEquals( true, $caught_http['body']['options']['enabledDatabaseCustomization'] ); | ||
$this->assertEquals( | ||
[ | ||
'min' => 1, | ||
'max' => 100, | ||
], | ||
$caught_http['body']['options']['validation']['username'] | ||
); | ||
|
||
$this->assertContains( | ||
'request.post("http://example.org/index.php?a0_action=migration-ws-login"', | ||
$caught_http['body']['options']['customScripts']['login'] | ||
); | ||
|
||
$this->assertContains( | ||
'form:{username:email, password:password, access_token:"TEST_MIGRATION_TOKEN"}', | ||
$caught_http['body']['options']['customScripts']['login'] | ||
); | ||
|
||
$this->assertContains( | ||
'request.post("http://example.org/index.php?a0_action=migration-ws-get-user"', | ||
$caught_http['body']['options']['customScripts']['get_user'] | ||
); | ||
|
||
$this->assertContains( | ||
'form:{username:email, access_token:"TEST_MIGRATION_TOKEN', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this intentionally not having a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, next character is a newline |
||
$caught_http['body']['options']['customScripts']['get_user'] | ||
); | ||
} | ||
|
||
/** | ||
* Test that successful and unsuccessful requests return properly. | ||
*/ | ||
public function testThatCreateConnectionReturnsCorrectly() { | ||
$this->startHttpMocking(); | ||
|
||
$api_ops = new WP_Auth0_Api_Operations( self::$opts ); | ||
$test_token = implode( '.', [ uniqid(), uniqid(), uniqid() ] ); | ||
|
||
self::$opts->set( 'domain', 'test-wp.auth0.com' ); | ||
self::$opts->set( 'client_id', 'TEST_CLIENT_ID' ); | ||
|
||
$this->http_request_type = 'success_create_connection'; | ||
|
||
$result = $api_ops->create_wordpress_connection( $test_token, false ); | ||
|
||
$this->assertEquals( 'TEST_CREATED_CONN_ID', $result ); | ||
$this->assertEquals( 'DB-Test-Blog', self::$opts->get( 'db_connection_name' ) ); | ||
|
||
$this->http_request_type = 'wp_error'; | ||
|
||
$result = $api_ops->create_wordpress_connection( $test_token, false ); | ||
|
||
$this->assertFalse( $result ); | ||
} | ||
|
||
/** | ||
* Runs after each test method. | ||
*/ | ||
public function tearDown() { | ||
parent::tearDown(); | ||
|
||
self::$opts->set( 'domain', self::$opts->get_default( 'domain' ) ); | ||
self::$opts->set( 'client_id', self::$opts->get_default( 'client_id' ) ); | ||
self::$opts->set( 'migration_ips', self::$opts->get_default( 'migration_ips' ) ); | ||
self::$opts->set( 'migration_ips_filter', self::$opts->get_default( 'migration_ips_filter' ) ); | ||
self::$opts->set( 'db_connection_name', null ); | ||
|
||
$this->stopHttpHalting(); | ||
$this->stopHttpMocking(); | ||
|
||
self::$error_log->clear(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These must not be set or the API endpoint returns a payload error.