-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add site health check to detect blocked REST API and short-circuit optimization when unavailable #1762
Add site health check to detect blocked REST API and short-circuit optimization when unavailable #1762
Changes from 21 commits
88952a5
4bff9fe
56d769a
ba911e1
12a229c
a0f460d
c29cb7c
8d8ae3e
422a5bf
0bc74f7
36042fd
ad21df8
0bba468
e1b9004
dbede65
2b2ca3e
e7c1001
7e4e057
a2baa65
e1ec10c
48c83a5
8895d35
2fbd530
5949bc9
1aa82b4
98fff4d
c095436
ebcd804
57f7566
226584f
1855ecb
0ad417a
7ad36cd
d957745
4c1b2dc
7f76d31
a0cce21
22dde1b
9869150
55e3b55
85b13f4
51a56ba
2a132ca
e22bb84
7eda72c
cc16600
523daaa
a1335e0
e5d3f72
c6b4417
3212dae
19ff8e4
546217f
3d5e8a8
c3b682f
c085ff0
50a1898
c7d31a7
85da162
0772ebe
dbff1f7
0693086
8926f10
6fee919
ddc6164
b4736a4
02953c7
14e9471
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 | ||||
---|---|---|---|---|---|---|
|
@@ -51,6 +51,18 @@ static function ( string $global_var_name, string $version, Closure $load ): voi | |||||
* action), so this is why it gets initialized at priority 9. | ||||||
*/ | ||||||
add_action( 'init', $bootstrap, 9 ); | ||||||
|
||||||
register_activation_hook( | ||||||
__FILE__, | ||||||
static function () use ( $bootstrap ): void { | ||||||
/* | ||||||
* The activation hook is called before the init action, so the plugin is not loaded yet. This | ||||||
* means that the plugin must be bootstrapped here to run the activation logic. | ||||||
*/ | ||||||
$bootstrap(); | ||||||
od_rest_api_health_check_plugin_activation(); | ||||||
} | ||||||
); | ||||||
} | ||||||
|
||||||
// Register this copy of the plugin. | ||||||
|
@@ -127,5 +139,8 @@ class_alias( OD_URL_Metric_Group_Collection::class, 'OD_URL_Metrics_Group_Collec | |||||
|
||||||
// Add hooks for the above requires. | ||||||
require_once __DIR__ . '/hooks.php'; | ||||||
|
||||||
// Load site health checks. | ||||||
require_once __DIR__ . '/site-health/load.php'; | ||||||
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. As noted below, I think the directory can be removed in favor of just a simpler
Suggested change
|
||||||
} | ||||||
); |
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 think this file can be removed in favor of moving the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<?php | ||
/** | ||
* Site Health checks loader. | ||
* | ||
* @package optimization-detective | ||
* @since n.e.x.t | ||
*/ | ||
|
||
if ( ! defined( 'ABSPATH' ) ) { | ||
exit; // Exit if accessed directly. | ||
} | ||
|
||
// REST API site health check. | ||
require_once __DIR__ . '/rest-api/helper.php'; | ||
require_once __DIR__ . '/rest-api/hooks.php'; |
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. Since there is only one Site Health test, what about eliminating the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
<?php | ||
/** | ||
* Helper functions for the Optimization Detective REST API health check. | ||
* | ||
* @package optimization-detective | ||
* @since n.e.x.t | ||
*/ | ||
|
||
if ( ! defined( 'ABSPATH' ) ) { | ||
exit; // Exit if accessed directly. | ||
} | ||
|
||
/** | ||
* Tests availability of the Optimization Detective REST API endpoint. | ||
* | ||
* @since n.e.x.t | ||
* | ||
* @return array{label: string, status: string, badge: array{label: string, color: string}, description: string, actions: string, test: string} Result. | ||
*/ | ||
function od_optimization_detective_rest_api_test(): array { | ||
$result = array( | ||
'label' => __( 'The REST API endpoint is functional.', 'optimization-detective' ), | ||
'status' => 'good', | ||
'badge' => array( | ||
'label' => __( 'Optimization Detective', 'optimization-detective' ), | ||
'color' => 'blue', | ||
), | ||
'description' => sprintf( | ||
'<p>%s</p>', | ||
__( 'Your site can send and receive URL metrics via the REST API endpoint.', 'optimization-detective' ) | ||
), | ||
'actions' => '', | ||
'test' => 'optimization_detective_rest_api', | ||
); | ||
|
||
$rest_url = get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ); | ||
$response = wp_remote_post( | ||
$rest_url, | ||
array( | ||
'headers' => array( 'Content-Type' => 'application/json' ), | ||
'sslverify' => false, | ||
) | ||
); | ||
|
||
if ( is_wp_error( $response ) ) { | ||
$result['status'] = 'recommended'; | ||
$result['label'] = __( 'Error accessing the REST API endpoint', 'optimization-detective' ); | ||
$result['description'] = sprintf( | ||
'<p>%s</p>', | ||
esc_html__( 'There was an issue reaching the REST API endpoint. This might be due to server settings or the REST API being disabled.', 'optimization-detective' ) | ||
); | ||
$info = array( | ||
'error_message' => $response->get_error_message(), | ||
'error_code' => $response->get_error_code(), | ||
'available' => false, | ||
); | ||
} else { | ||
$status_code = wp_remote_retrieve_response_code( $response ); | ||
$data = json_decode( wp_remote_retrieve_body( $response ), true ); | ||
$expected_params = array( 'slug', 'current_etag', 'hmac', 'url', 'viewport', 'elements' ); | ||
$info = array( | ||
'status_code' => $status_code, | ||
'available' => false, | ||
); | ||
|
||
if ( | ||
400 === $status_code | ||
&& isset( $data['data']['params'] ) | ||
&& is_array( $data['data']['params'] ) | ||
&& count( $expected_params ) === count( array_intersect( $data['data']['params'], $expected_params ) ) | ||
) { | ||
// The REST API endpoint is available. | ||
$info['available'] = true; | ||
} elseif ( 401 === $status_code ) { | ||
$result['status'] = 'recommended'; | ||
$result['label'] = __( 'Authorization should not be required to access the REST API endpoint.', 'optimization-detective' ); | ||
$result['description'] = sprintf( | ||
'<p>%s</p>', | ||
esc_html__( 'To collect URL metrics, the REST API endpoint should be accessible without requiring authorization.', 'optimization-detective' ) | ||
); | ||
} elseif ( 403 === $status_code ) { | ||
$result['status'] = 'recommended'; | ||
$result['label'] = __( 'The REST API endpoint should not be forbidden.', 'optimization-detective' ); | ||
$result['description'] = sprintf( | ||
'<p>%s</p>', | ||
esc_html__( 'The REST API endpoint is blocked. Please review your server or security settings.', 'optimization-detective' ) | ||
); | ||
} else { | ||
$result['status'] = 'recommended'; | ||
$result['label'] = __( 'Error accessing the REST API endpoint', 'optimization-detective' ); | ||
$result['description'] = sprintf( | ||
'<p>%s</p>', | ||
esc_html__( 'There was an issue reaching the REST API endpoint. This might be due to server settings or the REST API being disabled.', 'optimization-detective' ) | ||
); | ||
} | ||
$info['error_message'] = $result['label']; | ||
} | ||
|
||
update_option( 'od_rest_api_info', $info ); | ||
return $result; | ||
} | ||
|
||
/** | ||
* Periodically runs the Optimization Detective REST API health check. | ||
* | ||
* @since n.e.x.t | ||
*/ | ||
function od_schedule_rest_api_health_check(): void { | ||
if ( ! (bool) wp_next_scheduled( 'od_rest_api_health_check_event' ) ) { | ||
wp_schedule_event( time(), 'weekly', 'od_rest_api_health_check_event' ); | ||
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. Do we really need to check this weekly? That seems excessive as it is very unlikely to change. How about checking when the plugin is activated and when users visit the Site Health screen? 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. Oh, right, won't this test automatically get run anyway due to wp_site_health_scheduled_check? cf. #1762 (comment) In that way, we don't need to schedule our own action at all but just re-use Site Health. Our test can persist the result in an option in a way similar to how site health in core stores issue counts in a transient. |
||
} | ||
} | ||
|
||
/** | ||
* Hook for the scheduled REST API health check. | ||
* | ||
* @since n.e.x.t | ||
*/ | ||
function od_run_scheduled_rest_api_health_check(): void { | ||
od_optimization_detective_rest_api_test(); | ||
} | ||
|
||
/** | ||
* Displays an admin notice if the REST API health check fails. | ||
* | ||
* @since n.e.x.t | ||
* | ||
* @param string $plugin_file Plugin file. | ||
*/ | ||
function od_rest_api_health_check_admin_notice( string $plugin_file ): void { | ||
if ( 'optimization-detective/load.php' !== $plugin_file ) { | ||
return; | ||
} | ||
|
||
$od_rest_api_info = get_option( 'od_rest_api_info', array() ); | ||
if ( | ||
isset( $od_rest_api_info['available'] ) && | ||
! (bool) $od_rest_api_info['available'] && | ||
isset( $od_rest_api_info['error_message'] ) | ||
) { | ||
wp_admin_notice( | ||
esc_html( $od_rest_api_info['error_message'] ), | ||
array( | ||
'type' => 'warning', | ||
'additional_classes' => array( 'inline', 'notice-alt' ), | ||
) | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Plugin activation hook for the REST API health check. | ||
* | ||
* @since n.e.x.t | ||
*/ | ||
function od_rest_api_health_check_plugin_activation(): void { | ||
// Add the option if it doesn't exist. | ||
if ( ! (bool) get_option( 'od_rest_api_info' ) ) { | ||
add_option( 'od_rest_api_info', array() ); | ||
} | ||
od_schedule_rest_api_health_check(); | ||
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 run check directly, caching results? 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 would like to request you to review this section again, as there have been many changes. Thank you. |
||
// Run the check immediately after Optimization Detective is activated. | ||
add_action( | ||
'activated_plugin', | ||
static function ( string $plugin ): void { | ||
if ( 'optimization-detective/load.php' === $plugin ) { | ||
od_optimization_detective_rest_api_test(); | ||
} | ||
} | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
<?php | ||
/** | ||
* Hook callbacks used for the Optimization Detective REST API health check. | ||
* | ||
* @package optimization-detective | ||
* @since n.e.x.t | ||
*/ | ||
|
||
if ( ! defined( 'ABSPATH' ) ) { | ||
exit; // Exit if accessed directly. | ||
} | ||
|
||
/** | ||
* Adds the Optimization Detective REST API check to site health tests. | ||
* | ||
* @since n.e.x.t | ||
* | ||
* @param array{direct: array<string, array{label: string, test: string}>} $tests Site Health Tests. | ||
* @return array{direct: array<string, array{label: string, test: string}>} Amended tests. | ||
*/ | ||
function od_optimization_detective_add_rest_api_test( array $tests ): array { | ||
$tests['direct']['optimization_detective_rest_api'] = array( | ||
'label' => __( 'Optimization Detective REST API Endpoint Availability', 'optimization-detective' ), | ||
'test' => 'od_optimization_detective_rest_api_test', | ||
); | ||
|
||
return $tests; | ||
} | ||
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 think this could be moved to a |
||
add_filter( 'site_status_tests', 'od_optimization_detective_add_rest_api_test' ); | ||
|
||
// Hook for the scheduled REST API health check. | ||
add_action( 'od_rest_api_health_check_event', 'od_run_scheduled_rest_api_health_check' ); | ||
add_action( 'after_plugin_row_meta', 'od_rest_api_health_check_admin_notice', 30 ); | ||
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 about moving these to a section in the plugin's existing |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
<?php | ||
/** | ||
* Tests for Optimization Detective REST API site health check. | ||
* | ||
* @package optimization-detective | ||
*/ | ||
|
||
class Test_OD_REST_API_Site_Health_Check extends WP_UnitTestCase { | ||
|
||
/** | ||
* Holds mocked response headers for different test scenarios. | ||
* | ||
* @var array<string, array<string, mixed>> | ||
*/ | ||
protected $mocked_responses = array(); | ||
|
||
/** | ||
* Setup each test. | ||
*/ | ||
public function setUp(): void { | ||
parent::setUp(); | ||
|
||
// Clear any filters or mocks. | ||
remove_all_filters( 'pre_http_request' ); | ||
|
||
// Add the filter to mock HTTP requests. | ||
add_filter( 'pre_http_request', array( $this, 'mock_http_requests' ), 10, 3 ); | ||
} | ||
|
||
/** | ||
* Test that the site health check is `good` when the REST API is available. | ||
*/ | ||
public function test_rest_api_available(): void { | ||
$this->mocked_responses = array( | ||
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response( | ||
400, | ||
'Bad Request', | ||
array( | ||
'data' => array( | ||
'params' => array( 'slug', 'current_etag', 'hmac', 'url', 'viewport', 'elements' ), | ||
), | ||
) | ||
), | ||
); | ||
|
||
$result = od_optimization_detective_rest_api_test(); | ||
$od_rest_api_info = get_option( 'od_rest_api_info', array() ); | ||
|
||
$this->assertSame( 'good', $result['status'] ); | ||
$this->assertSame( 400, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' ); | ||
$this->assertTrue( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : false ); | ||
} | ||
|
||
/** | ||
* Test behavior when REST API returns an unauthorized error. | ||
*/ | ||
public function test_rest_api_unauthorized(): void { | ||
$this->mocked_responses = array( | ||
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response( | ||
401, | ||
'Unauthorized' | ||
), | ||
); | ||
|
||
$result = od_optimization_detective_rest_api_test(); | ||
$od_rest_api_info = get_option( 'od_rest_api_info', array() ); | ||
|
||
$this->assertSame( 'recommended', $result['status'] ); | ||
$this->assertSame( 401, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' ); | ||
$this->assertFalse( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : true ); | ||
} | ||
|
||
/** | ||
* Test behavior when REST API returns an forbidden error. | ||
*/ | ||
public function test_rest_api_forbidden(): void { | ||
$this->mocked_responses = array( | ||
get_rest_url( null, OD_REST_API_NAMESPACE . OD_URL_METRICS_ROUTE ) => $this->build_mock_response( | ||
403, | ||
'Forbidden' | ||
), | ||
); | ||
|
||
$result = od_optimization_detective_rest_api_test(); | ||
$od_rest_api_info = get_option( 'od_rest_api_info', array() ); | ||
|
||
$this->assertSame( 'recommended', $result['status'] ); | ||
$this->assertSame( 403, isset( $od_rest_api_info['status_code'] ) ? $od_rest_api_info['status_code'] : '' ); | ||
$this->assertFalse( isset( $od_rest_api_info['available'] ) ? $od_rest_api_info['available'] : true ); | ||
} | ||
|
||
/** | ||
* Mock HTTP requests for assets to simulate different responses. | ||
* | ||
* @param bool $response A preemptive return value of an HTTP request. Default false. | ||
* @param array<string, mixed> $args Request arguments. | ||
* @param string $url The request URL. | ||
* @return array<string, mixed> Mocked response. | ||
*/ | ||
public function mock_http_requests( bool $response, array $args, string $url ): array { | ||
if ( isset( $this->mocked_responses[ $url ] ) ) { | ||
return $this->mocked_responses[ $url ]; | ||
} | ||
|
||
// If no specific mock set, default to a generic success response. | ||
return array( | ||
'response' => array( | ||
'code' => 200, | ||
'message' => 'OK', | ||
), | ||
); | ||
} | ||
|
||
/** | ||
* Build a mock response. | ||
* | ||
* @param int $status_code HTTP status code. | ||
* @param string $message HTTP status message. | ||
* @param array<string, mixed> $body Response body. | ||
* @return array<string, mixed> Mocked response. | ||
*/ | ||
protected function build_mock_response( int $status_code, string $message, array $body = array() ): array { | ||
return array( | ||
'response' => array( | ||
'code' => $status_code, | ||
'message' => $message, | ||
), | ||
'body' => wp_json_encode( $body ), | ||
); | ||
} | ||
} |
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.
See note below about how the activation hook does not run when network-activating a plugin. I think this should switch to use
admin_init
. For example, add this tohooks.php
: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 are the two reasons why I chose the activation hook approach:
Immediate Notice Display:
With the activation hook approach, when the user activates the plugin, they will see the notice about the REST API during the same page load as the plugin activation.
admin_init
hook instead, the user will not see the notice immediately after activating the plugin because the health check is only scheduled during the plugin activation page load. It will execute asynchronously on the next load, and the notice will only appear on the third page load.admin_init
hook.Efficient Option Handling:
As mentioned here, we should add an empty option during plugin activation. If we use the
admin_init
hook, the code to check whether the option exists and create it if not will run on every admin page load, which is inefficient.Alternative Approach:
We can also adopt a hybrid approach where the event is scheduled in the
admin_init
hook, while the logic for adding the option and immediate notice display is handled in the activation hook. This approach would also address the multisite issue where the event may not get scheduled.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.
Will this be inefficient? WordPress will remembers options which don't exist in
notoptions
so that no DB query occurs with the nextget_option()
call (when an external object cache is being used). Nevertheless, if upon activation the plugin has anadmin_init
action which will immediately call the Site Health logic to see if the REST API is available, then this will set the option so that no such request happens with the next admin page load. Also, if this request is gated behind admin requests in the first place, then this further reduces the performance impact since frontend users would experience nothing.So I propose something like this:
and:
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.
As discussed in #1762 (comment), we don't want to schedule any cron events at this time. To ensure our check runs for the first time, we can modify the provided function by directly calling
od_optimization_detective_rest_api_test
instead ofod_run_scheduled_rest_api_health_check
.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.
Also for the admin notices which needs to be displayed only once after plugin activation can be included in this function.