Delete Invalid Access Tokens from Settings #952
Conversation
This ensures it can be accessed when a non-admin request refreshes or deletes an access token.
WordPress Playground🚀 Your PR has been built and is ready for testing in WordPress Playground! |
n7studios
requested review from
a team,
corydhmiller and
noelherrick
and removed request for
a team
| * @param WP_Error $result Error result. | ||
| * @param string $client_id OAuth Client ID used for the Access and Refresh Tokens. | ||
| */ | ||
| public function maybe_delete_credentials( $result, $client_id ) { |
There was a problem hiding this comment.
What happens if the server returns a 5xx error? Would we just delete the credentials? What about a 429 rate limit?
There was a problem hiding this comment.
maybe_delete_credentials is only called when the Kit WordPress Libraries fires specific hooks:
convertkit_api_refresh_token_error: An attempt to refresh an expired access token failed: https://github.com/Kit/convertkit-wordpress-libraries/blob/add-hooks-on-request-error/src/class-convertkit-api-v4.php#L414-L440convertkit_api_access_token_invalid: An invalid access token was used (note, this differs from an expired access token being used): https://github.com/Kit/convertkit-wordpress-libraries/blob/add-hooks-on-request-error/src/class-convertkit-api-v4.php#L1463-L1519
Code in this PR that checks for these hooks:
https://github.com/Kit/convertkit-wordpress/blob/delete-invalid-access-token-from-settings/includes/class-convertkit-settings.php#L52-L55
Other errors, such as a 429 or 5xx, won't result in tokens being deleted from the Plugin's settings. Libraries return a WP_Error, which the Plugin then handles accordingly.
There was a problem hiding this comment.
But if we get a non-401 from the refresh itself, wouldn't that cause the tokens to be deleted?
There was a problem hiding this comment.
Noted. Removed convertkit_api_refresh_token_error hook, ensuring access token only deleted if the API class uses an invalid access token, not an expired one.
WordPress Playground🚀 Your PR has been built and is ready for testing in WordPress Playground! |
Summary
When an access token is either invalid, or expired and fails to refresh, the Kit Plugin currently retains the access token, refresh token and expiry date in the Plugin's settings.
This results in many API requests which 401 - the below from a single WordPress administration screen (34 requests):
As a result, the API has millions of 401 requests over a 24 hour period:
Whilst caching strategies are already implemented in the Plugin, these do not apply if:
This PR resolves by using an updated version of the WordPress Libraries, listening for the hook when an access token is invalid or refreshing an expired token fails, deleting said tokens from the Plugin's settings. This prevents repetitive requests that will fail - the below from the same WordPress administration screen (1 request):
PR's already exist to:
It's unclear why some sites still end up in a situation where their access tokens expire and do not get refreshed. These individual cases will need to be examined after this update.
Testing
testAccessTokenDeletedWhenRefreshFails: Test that the access token, refresh token and expiry are deleted from the Plugin's settings when refreshing an expired access token failstestAccessTokenDeletedWhenInvalid: Test that the access token, refresh token and expiry are deleted from the Plugin's settings when an invalid access token is usedChecklist