Better Management API token handling #632

joshcanhelp · 2019-03-14T23:10:31Z

Changes

Remove Management API access token validation
Store new API tokens and scope in transients

References

Closes #631

Testing

This change adds unit test coverage
This change has been tested on the latest version of WP

Checklist

I read Auth0's general contribution guidelines?
I read Auth0's code of conduct guidelines
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

…cope

codecov-io · 2019-03-15T03:14:13Z

Codecov Report

Merging #632 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #632      +/-   ##
============================================
+ Coverage     37.83%   37.96%   +0.13%     
+ Complexity     1243     1240       -3     
============================================
  Files            51       51              
  Lines          3840     3832       -8     
============================================
+ Hits           1453     1455       +2     
+ Misses         2387     2377      -10

Impacted Files	Coverage Δ	Complexity Δ
lib/profile/WP_Auth0_Profile_Delete_Data.php	`100% <ø> (ø)`	`8 <0> (ø)`	⬇️
lib/profile/WP_Auth0_Profile_Delete_Mfa.php	`100% <ø> (ø)`	`12 <0> (ø)`	⬇️
lib/api/WP_Auth0_Api_Client_Credentials.php	`100% <100%> (ø)`	`11 <4> (+3)`	⬆️
lib/api/WP_Auth0_Api_Abstract.php	`100% <100%> (+27.65%)`	`27 <0> (-6)`	⬇️
lib/WP_Auth0_Options.php	`73.26% <0%> (-10.9%)`	`28% <0%> (ø)`
lib/WP_Auth0_Api_Client.php	`31.7% <0%> (-1.36%)`	`81% <0%> (ø)`

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 9bddec6...5a2a230. Read the comment docs.

lbalmaceda · 2019-03-15T17:14:52Z

lib/api/WP_Auth0_Api_Abstract.php

-		if ( ! $this->api_token ) {
-			$this->api_token = $this->options->get( 'auth0_app_token' ); // REMOVE
-		}
+		$api_token = WP_Auth0_Api_Client_Credentials::get_stored_token();


What's the reason to have this method static and the call() one part of the instance instead?

call() is part of an abstract so it needs to be defined. This can accept parameters required for the API call. In this case, no parameters are needed but other child classes use that.

The static methods don't require any parameters and act on global state (database).

lbalmaceda · 2019-03-15T17:18:11Z

lib/api/WP_Auth0_Api_Abstract.php

 		}

 		// No token to use, error recorded in previous steps.
-		if ( ! $this->api_token_decoded ) {
-			wp_cache_delete( self::CACHE_KEY, WPA0_CACHE_GROUP );
+		if ( ! $api_token ) {


shouldn't this case be covered by a exception thrown in call() if the request fails somehow?

No exceptions thrown in call() or else they would have to be caught. It just returns bool whether it worked or not.

lbalmaceda · 2019-03-15T17:19:02Z

lib/api/WP_Auth0_Api_Abstract.php

 			return false;
 		}

+		if ( WP_Auth0_Api_Client_Credentials::check_stored_scope( $scope ) ) {


What is this line doing?

Checks that the scope we have stored (from the CC grant call) matches the scope we need.

lbalmaceda · 2019-03-15T17:19:44Z

lib/api/WP_Auth0_Api_Client_Credentials.php

+	public static function check_stored_scope( $scope ) {
+		$stored_scope = get_transient( self::SCOPE_TRANSIENT_KEY );
+		$scopes       = explode( ' ', $stored_scope );
+		return ! empty( $scopes ) && in_array( $scope, $scopes );


this ! empty( $scopes ) is redundant. You're already looking for a match with in_array.
Also I don't see you exploding the $scope parameter. What if the value has multiple scopes separated with spaces? Adding a tests scenario for this will help you fix it

this ! empty( $scopes ) is redundant

You're right, explode() always returns an array even if the "string" is null/false. I'll change that.

Also I don't see you exploding the $scope parameter. What if the value has multiple scopes separated with spaces? Adding a tests scenario for this will help you fix it

There is a test for multiple scopes. The $scope parameter only excepts one (we only ever need one for the API call [at least the ones we have currently]). AFAIK, the endpoints never require more than one scope to take action so we'd never check for multiple.

lbalmaceda · 2019-03-15T17:31:22Z

tests/testApiChangeEmail.php

+		set_transient( 'auth0_api_token_scope', 'update:users' );
+
+		$this->http_request_type = 'success_empty_body';
+		$api                     = new WP_Auth0_Api_Change_Email( self::$opts, self::$api_client_creds );


"Test that the bearer setting succeeds". Where is this setting being called here? Maybe you are not missing the call but the name of the test method should be different and not disclose "black box" internals.

This is basically a generic test against set_bearer instead of reflecting/exposing. This class requires the bearer token to be set so I'm making sure that it is and checking that it was stored.

Where is this setting being called here?

I'm not clear on your question.

Not a question but rather an obversation. That the name of the test suggest you're going to call set_bearer but in the test you don't. It's true that you might be calling it internally when a new instance of WP_Auth0_Api_Change_Email is created, but the test should not know what methods are called internally. The test should only know that a new instance of WP_Auth0_Api_Change_Email is being created. That's why I mentioned to only test the "black box" of the method and not its internals.

lbalmaceda · 2019-03-15T17:46:39Z

tests/testProfileChangePassword.php

@@ -20,7 +20,7 @@ class TestProfileChangePassword extends WP_Auth0_Test_Case {

 	use UsersHelper;

-	use httpHelpers;
+	use HttpHelpers;


You're changing the case of the class name but the file name is still the same

File name is fine, will get picked up by the autoloader. Testing individual files failed when the case of the trait didn't match the case of the use statement.

lbalmaceda · 2019-03-15T19:08:45Z

tests/testApiChangeEmail.php

+		set_transient( 'auth0_api_token_scope', 'update:users' );
+
+		$this->http_request_type = 'success_empty_body';
+		$api                     = new WP_Auth0_Api_Change_Email( self::$opts, self::$api_client_creds );


Not a question but rather an obversation. That the name of the test suggest you're going to call set_bearer but in the test you don't. It's true that you might be calling it internally when a new instance of WP_Auth0_Api_Change_Email is created, but the test should not know what methods are called internally. The test should only know that a new instance of WP_Auth0_Api_Change_Email is being created. That's why I mentioned to only test the "black box" of the method and not its internals.

lbalmaceda · 2019-03-15T19:12:20Z

lib/api/WP_Auth0_Api_Abstract.php

-			$this->api_token_decoded = $this->api_client_creds->get_token_decoded();
+		// No stored API token so need to get a new one.
+		if ( ! $api_token && $this->api_client_creds instanceof WP_Auth0_Api_Client_Credentials ) {
+			$api_token = $this->api_client_creds->call();


is this call aware of the required $scope?

No, it doesn't need to be. You get the scopes back that the Application is authorized for.

lbalmaceda · 2019-03-15T19:13:49Z

lib/api/WP_Auth0_Api_Abstract.php

-		wp_cache_set( self::CACHE_KEY, $this->api_token, WPA0_CACHE_GROUP );
-		return true;
+		// Delete the stored token so we can try again.
+		WP_Auth0_Api_Client_Credentials::delete_store();


why would you need to delete the token? What if the user wants to use the stored token for a different API endpoint call?

Then it tries again. We don't want the token stored without required scopes or else we can't try again.

lbalmaceda · 2019-03-15T19:16:11Z

lib/api/WP_Auth0_Api_Client_Credentials.php

+	/**
+	 * Check the stored API token scope for a specific scope.
+	 *
+	 * @param string $scope - Scope to check for.


maybe clarify here that the scope param is for a single scope vs a space separated string of scopes. This was confusing to me on the initial review because when we build /authorize URLs we have a "scope" parameter that accepts as many scopes as wanted, separated by a space.

This is only for the client credentials grant (hence the class name) but I will make that more explicit.

joshcanhelp added 2 commits March 14, 2019 16:08

Failing tests for revised Client Credentials behavior

56f9c23

Failing tests for revised set_bearer functionality

c1b56f6

joshcanhelp force-pushed the fix-m-api-token-handling branch from 23975e1 to c1b56f6 Compare March 15, 2019 00:20

joshcanhelp added 2 commits March 14, 2019 17:28

Store api token and scopes in transients; remove api token validation

6153378

More tests for added functionality; add error code for insufficient s…

c3ba87a

…cope

auth0 deleted a comment from codecov-io Mar 15, 2019

joshcanhelp changed the title ~~Failing tests for revised Client Credentials behavior~~ Better Management API token handling Mar 15, 2019

Cleanup and quality scan

50b0228

joshcanhelp added the CH: Fixed label Mar 15, 2019

joshcanhelp added this to the 3.10.0 milestone Mar 15, 2019

joshcanhelp marked this pull request as ready for review March 15, 2019 17:13

lbalmaceda reviewed Mar 15, 2019

View reviewed changes

PR feedback on management token handling

5a2a230

lbalmaceda approved these changes Mar 15, 2019

View reviewed changes

joshcanhelp merged commit a78b427 into master Mar 15, 2019

joshcanhelp deleted the fix-m-api-token-handling branch March 15, 2019 19:55

This was referenced Mar 25, 2019

Do not store the API token on install #639

Merged

OpenSSL error in WP_Auth0_Api_Client_Credentials::call #631

Closed

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

Better Management API token handling #632

Better Management API token handling #632

joshcanhelp commented Mar 14, 2019 •

edited

Loading

codecov-io commented Mar 15, 2019 •

edited

Loading

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019 •

edited

Loading

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019

lbalmaceda Mar 15, 2019

joshcanhelp Mar 15, 2019 •

edited

Loading

Better Management API token handling #632

Better Management API token handling #632

Conversation

joshcanhelp commented Mar 14, 2019 • edited Loading

Changes

References

Testing

Checklist

codecov-io commented Mar 15, 2019 • 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

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 Mar 15, 2019 • 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

joshcanhelp Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

joshcanhelp commented Mar 14, 2019 •

edited

Loading

codecov-io commented Mar 15, 2019 •

edited

Loading

joshcanhelp Mar 15, 2019 •

edited

Loading

joshcanhelp Mar 15, 2019 •

edited

Loading