Skip to content

Commit ca61af3

Browse files
n7studiosn7studios
authored
Merge pull request #952 from Kit/delete-invalid-access-token-from-settings
Delete Invalid Access Tokens from Settings
2 parents 659fec1 + 4a183a3 commit ca61af3

File tree

8 files changed

+112
-39
lines changed

8 files changed

+112
-39
lines changed

‎admin/section/class-convertkit-admin-section-general.php‎

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,17 @@ private function check_credentials() {
133133

134134
// Bail if no access and refresh token exist.
135135
if ( ! $this->settings->has_access_and_refresh_token() ) {
136-
return;
136+
// Redirect to General screen, which will now show the ConvertKit_Admin_Section_OAuth screen, because
137+
// the Plugin has no access token.
138+
wp_safe_redirect(
139+
add_query_arg(
140+
array(
141+
'page' => $this->settings_key,
142+
),
143+
'options-general.php'
144+
)
145+
);
146+
exit();
137147
}
138148

139149
// Initialize the API.
@@ -152,37 +162,11 @@ private function check_credentials() {
152162

153163
// If the request succeeded, no need to perform further actions.
154164
if ( ! is_wp_error( $this->account ) ) {
155-
// Remove any existing persistent notice.
156-
WP_ConvertKit()->get_class( 'admin_notices' )->delete( 'authorization_failed' );
157-
158165
return;
159166
}
160167

161-
// Depending on the error code, maybe persist a notice in the WordPress Administration until the user
168+
// Depending on the error code, display an error notice in the settings screen until the user
162169
// fixes the problem.
163-
switch ( $this->account->get_error_data( $this->account->get_error_code() ) ) {
164-
case 401:
165-
// Access token either expired or was revoked in ConvertKit.
166-
// Remove from settings.
167-
$this->settings->delete_credentials();
168-
169-
// Display a site wide notice.
170-
WP_ConvertKit()->get_class( 'admin_notices' )->add( 'authorization_failed' );
171-
172-
// Redirect to General screen, which will now show the ConvertKit_Admin_Section_OAuth screen, because
173-
// the Plugin has no access token.
174-
wp_safe_redirect(
175-
add_query_arg(
176-
array(
177-
'page' => $this->settings_key,
178-
),
179-
'options-general.php'
180-
)
181-
);
182-
exit();
183-
}
184-
185-
// Output a non-401 error now.
186170
$this->output_error( $this->account->get_error_message() );
187171

188172
}

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"type": "project",
55
"license": "GPLv3",
66
"require": {
7-
"convertkit/convertkit-wordpress-libraries": "2.1.0"
7+
"convertkit/convertkit-wordpress-libraries": "dev-add-hooks-on-request-error"
88
},
99
"require-dev": {
1010
"php-webdriver/webdriver": "^1.0",

admin/class-convertkit-admin-notices.php renamed to includes/class-convertkit-admin-notices.php

File renamed without changes.

‎includes/class-convertkit-settings.php‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@ public function __construct() {
4949
add_action( 'convertkit_api_get_access_token', array( $this, 'update_credentials' ), 10, 2 );
5050
add_action( 'convertkit_api_refresh_token', array( $this, 'update_credentials' ), 10, 2 );
5151

52+
// Delete credentials if the API class uses a invalid access token.
53+
// This prevents the Plugin making repetitive API requests that will 401.
54+
add_action( 'convertkit_api_access_token_invalid', array( $this, 'maybe_delete_credentials' ), 10, 2 );
55+
5256
}
5357

5458
/**
@@ -629,6 +633,9 @@ public function update_credentials( $result, $client_id ) {
629633
return;
630634
}
631635

636+
// Remove any existing persistent notice.
637+
WP_ConvertKit()->get_class( 'admin_notices' )->delete( 'authorization_failed' );
638+
632639
$this->save(
633640
array(
634641
'access_token' => $result['access_token'],
@@ -646,7 +653,36 @@ public function update_credentials( $result, $client_id ) {
646653
}
647654

648655
/**
649-
* Deletes any existing access token, refresh token and its expiry from the Plugin settings.
656+
* Deletes the stored access token, refresh token and its expiry from the Plugin settings,
657+
* and clears any existing scheduled WordPress Cron event to refresh the token on expiry,
658+
* when either:
659+
* - The access token is invalid
660+
* - The access token expired, and refreshing failed
661+
*
662+
* @since 3.1.0
663+
*
664+
* @param WP_Error $result Error result.
665+
* @param string $client_id OAuth Client ID used for the Access and Refresh Tokens.
666+
*/
667+
public function maybe_delete_credentials( $result, $client_id ) {
668+
669+
// Don't delete these credentials if they're not for this Client ID.
670+
// They're for another Kit Plugin that uses OAuth.
671+
if ( $client_id !== CONVERTKIT_OAUTH_CLIENT_ID ) {
672+
return;
673+
}
674+
675+
// Persist an error notice in the WordPress Administration until the user fixes the problem.
676+
WP_ConvertKit()->get_class( 'admin_notices' )->add( 'authorization_failed' );
677+
678+
// Delete the credentials from the Plugin settings.
679+
$this->delete_credentials();
680+
681+
}
682+
683+
/**
684+
* Deletes any existing access token, refresh token and its expiry from the Plugin settings,
685+
* and clears any existing scheduled WordPress Cron event to refresh the token on expiry.
650686
*
651687
* @since 2.5.0
652688
*/
@@ -660,6 +696,9 @@ public function delete_credentials() {
660696
)
661697
);
662698

699+
// Clear any existing scheduled WordPress Cron event.
700+
wp_clear_scheduled_hook( 'convertkit_refresh_token' );
701+
663702
}
664703

665704
/**

‎includes/class-wp-convertkit.php‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,14 +79,12 @@ private function initialize_admin() {
7979
return;
8080
}
8181

82-
$this->classes['admin_bulk_edit'] = new ConvertKit_Admin_Bulk_Edit();
83-
$this->classes['admin_cache_plugins'] = new ConvertKit_Admin_Cache_Plugins();
84-
$this->classes['admin_category'] = new ConvertKit_Admin_Category();
85-
$this->classes['admin_landing_page'] = new ConvertKit_Admin_Landing_Page();
86-
$this->classes['admin_notices'] = new ConvertKit_Admin_Notices();
87-
$this->classes['admin_post'] = new ConvertKit_Admin_Post();
88-
$this->classes['admin_quick_edit'] = new ConvertKit_Admin_Quick_Edit();
89-
82+
$this->classes['admin_bulk_edit'] = new ConvertKit_Admin_Bulk_Edit();
83+
$this->classes['admin_cache_plugins'] = new ConvertKit_Admin_Cache_Plugins();
84+
$this->classes['admin_category'] = new ConvertKit_Admin_Category();
85+
$this->classes['admin_landing_page'] = new ConvertKit_Admin_Landing_Page();
86+
$this->classes['admin_post'] = new ConvertKit_Admin_Post();
87+
$this->classes['admin_quick_edit'] = new ConvertKit_Admin_Quick_Edit();
9088
$this->classes['admin_restrict_content'] = new ConvertKit_Admin_Restrict_Content();
9189
$this->classes['admin_settings'] = new ConvertKit_Admin_Settings();
9290
$this->classes['admin_setup_wizard_landing_page'] = new ConvertKit_Admin_Setup_Wizard_Landing_Page();
@@ -178,6 +176,7 @@ private function initialize_frontend() {
178176
*/
179177
private function initialize_global() {
180178

179+
$this->classes['admin_notices'] = new ConvertKit_Admin_Notices();
181180
$this->classes['admin_refresh_resources'] = new ConvertKit_Admin_Refresh_Resources();
182181
$this->classes['ajax'] = new ConvertKit_AJAX();
183182
$this->classes['blocks_convertkit_broadcasts'] = new ConvertKit_Block_Broadcasts();

‎tests/Integration/APITest.php‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,12 +82,60 @@ public function testAccessTokenRefreshedAndSavedWhenExpired()
8282
// Run request, which will trigger the above filters as if the token expired and refreshes automatically.
8383
$result = $this->api->get_account();
8484

85+
// Confirm no WP_Error is returned.
86+
$this->assertNotInstanceOf( 'WP_Error', $result );
87+
$this->assertIsArray( $result );
88+
8589
// Confirm "new" tokens now exist in the Plugin's settings, which confirms the `convertkit_api_refresh_token` hook was called when
8690
// the tokens were refreshed.
8791
$this->assertEquals( $settings->get_access_token(), $_ENV['CONVERTKIT_OAUTH_ACCESS_TOKEN'] );
8892
$this->assertEquals( $settings->get_refresh_token(), $_ENV['CONVERTKIT_OAUTH_REFRESH_TOKEN'] );
8993
}
9094

95+
/**
96+
* Test that the Access Token, Refresh Token and Token Expiry are deleted from the Plugin's settings
97+
* when the Access Token used is invalid.
98+
*
99+
* @since 3.1.0
100+
*/
101+
public function testAccessTokenDeletedWhenInvalid()
102+
{
103+
// Save an invalid access token and refresh token in the Plugin's settings.
104+
$settings = new \ConvertKit_Settings();
105+
$settings->save(
106+
array(
107+
'access_token' => 'invalidAccessToken',
108+
'refresh_token' => $_ENV['CONVERTKIT_OAUTH_REFRESH_TOKEN'],
109+
'token_expires' => time() + 10000,
110+
)
111+
);
112+
113+
// Confirm the tokens saved.
114+
$this->assertEquals( $settings->get_access_token(), 'invalidAccessToken' );
115+
$this->assertEquals( $settings->get_refresh_token(), $_ENV['CONVERTKIT_OAUTH_REFRESH_TOKEN'] );
116+
117+
// Initialize the API using the invalid refresh token.
118+
$api = new \ConvertKit_API_V4(
119+
$_ENV['CONVERTKIT_OAUTH_CLIENT_ID'],
120+
$_ENV['KIT_OAUTH_REDIRECT_URI'],
121+
$settings->get_access_token(),
122+
$settings->get_refresh_token()
123+
);
124+
125+
// Run request.
126+
$result = $api->get_account();
127+
128+
// Confirm a WP_Error is returned.
129+
$this->assertInstanceOf( 'WP_Error', $result );
130+
$this->assertEquals( $result->get_error_code(), 'convertkit_api_error' );
131+
$this->assertEquals( $result->get_error_message(), 'The access token is invalid' );
132+
133+
// Confirm tokens removed from the Plugin's settings, which confirms the `convertkit_api_access_token_invalid` hook was called when the tokens were deleted.
134+
$this->assertEmpty( $settings->get_access_token() );
135+
$this->assertEmpty( $settings->get_refresh_token() );
136+
$this->assertEmpty( $settings->get_token_expiry() );
137+
}
138+
91139
/**
92140
* Test that a WordPress Cron event is created when an access token is obtained.
93141
*

‎tests/Support/Helper/KitPlugin.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -531,6 +531,9 @@ public function resetKitPlugin($I)
531531
$I->dontHaveOptionInDatabase('convertkit_tags');
532532
$I->dontHaveOptionInDatabase('convertkit_tags_last_queried');
533533

534+
// Persistent notices.
535+
$I->dontHaveOptionInDatabase('convertkit-admin-notices');
536+
534537
// Review Request.
535538
$I->dontHaveOptionInDatabase('convertkit-review-request');
536539
$I->dontHaveOptionInDatabase('convertkit-review-dismissed');

‎wp-convertkit.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
require_once CONVERTKIT_PLUGIN_PATH . '/includes/cron-functions.php';
5353
require_once CONVERTKIT_PLUGIN_PATH . '/includes/functions.php';
5454
require_once CONVERTKIT_PLUGIN_PATH . '/includes/class-wp-convertkit.php';
55+
require_once CONVERTKIT_PLUGIN_PATH . '/includes/class-convertkit-admin-notices.php';
5556
require_once CONVERTKIT_PLUGIN_PATH . '/includes/class-convertkit-ajax.php';
5657
require_once CONVERTKIT_PLUGIN_PATH . '/includes/class-convertkit-broadcasts-exporter.php';
5758
require_once CONVERTKIT_PLUGIN_PATH . '/includes/class-convertkit-broadcasts-importer.php';
@@ -108,7 +109,6 @@
108109
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-cache-plugins.php';
109110
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-category.php';
110111
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-landing-page.php';
111-
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-notices.php';
112112
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-post.php';
113113
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-refresh-resources.php';
114114
require_once CONVERTKIT_PLUGIN_PATH . '/admin/class-convertkit-admin-restrict-content.php';

0 commit comments

Comments
 (0)