-
Notifications
You must be signed in to change notification settings - Fork 217
Prevent Stripe API calls after detecting the API keys are not valid (401 response from API) #4323
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
Changes from all commits
10fc910
1063336
5a11e7c
4f674ab
7ef8df3
f90e45e
1c7c8f5
47b42d2
24ea6ea
9740ff3
9666dcf
cd43fc9
4d288fc
52048d9
13e6401
ff5d7eb
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 |
---|---|---|
|
@@ -16,6 +16,20 @@ class WC_Stripe_API { | |
const ENDPOINT = 'https://api.stripe.com/v1/'; | ||
const STRIPE_API_VERSION = '2024-06-20'; | ||
|
||
/** | ||
* The test mode invalid API keys option key. | ||
* | ||
* @var string | ||
*/ | ||
const TEST_MODE_INVALID_API_KEYS_OPTION_KEY = 'wc_stripe_test_invalid_api_keys_detected'; | ||
|
||
/** | ||
* The live mode invalid API keys option key. | ||
* | ||
* @var string | ||
*/ | ||
const LIVE_MODE_INVALID_API_KEYS_OPTION_KEY = 'wc_stripe_live_invalid_api_keys_detected'; | ||
|
||
/** | ||
* Secret API Key. | ||
* | ||
|
@@ -231,6 +245,13 @@ public static function request( $request, $api = 'charges', $method = 'POST', $w | |
* @param string $api | ||
*/ | ||
public static function retrieve( $api ) { | ||
// If we have an option flag indicating that the secret key is not valid, we don't attempt the API call and we return an error. | ||
$invalid_api_keys_option_key = WC_Stripe_Mode::is_test() ? self::TEST_MODE_INVALID_API_KEYS_OPTION_KEY : self::LIVE_MODE_INVALID_API_KEYS_OPTION_KEY; | ||
$invalid_api_keys_detected = get_option( $invalid_api_keys_option_key ); | ||
if ( $invalid_api_keys_detected ) { | ||
return null; // The UI expects this empty response in case of invalid API keys. | ||
} | ||
|
||
WC_Stripe_Logger::log( "{$api}" ); | ||
|
||
$response = wp_safe_remote_get( | ||
|
@@ -242,6 +263,18 @@ public static function retrieve( $api ) { | |
] | ||
); | ||
|
||
// If we get a 401 error, we know the secret key is not valid. | ||
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. Maybe we could consider later changing this to check for anything different from 200, to cover more possible issues. 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. My proposed changes in #4327 take that approach when looking at responses. 😁 |
||
if ( is_array( $response ) && isset( $response['response'] ) && is_array( $response['response'] ) && isset( $response['response']['code'] ) && 401 === $response['response']['code'] ) { | ||
// We save a flag in the options to avoid making calls until the secret key gets updated. | ||
update_option( $invalid_api_keys_option_key, true ); | ||
update_option( $invalid_api_keys_option_key . '_at', time() ); | ||
|
||
// We delete the transient for the account data to trigger the not-connected UI in the admin dashboard. | ||
delete_transient( WC_Stripe_Mode::is_test() ? WC_Stripe_Account::TEST_ACCOUNT_OPTION : WC_Stripe_Account::LIVE_ACCOUNT_OPTION ); | ||
|
||
return null; // The UI expects this empty response in case of invalid API keys. | ||
} | ||
|
||
if ( is_wp_error( $response ) || empty( $response['body'] ) ) { | ||
WC_Stripe_Logger::log( 'Error Response: ' . print_r( $response, true ) ); | ||
return new WP_Error( 'stripe_error', __( 'There was a problem connecting to the Stripe API endpoint.', 'woocommerce-gateway-stripe' ) ); | ||
|
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.
Not urgent in this case. But, will this break internationalization of this string in the patch release?
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.
Good call.
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.
That said, I think the trade-off for correctly communicating the status is probably worth it.
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.
Correct, new strings need to be translated in the community translations site: https://translate.wordpress.org/projects/wp-plugins/woocommerce-gateway-stripe/