-
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 Inaccessible #1762
base: trunk
Are you sure you want to change the base?
Add site health check to detect blocked REST API and short-circuit optimization when Inaccessible #1762
Changes from all 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
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 @@ | |
* 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(); | ||
} | ||
); | ||
Comment on lines
+54
to
+65
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. See note below about how the activation hook does not run when network-activating a plugin. I think this should switch to use add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' ); 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. These are the two reasons why I chose the activation hook approach:
Alternative Approach: 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.
Will this be inefficient? WordPress will remembers options which don't exist in So I propose something like this: /**
* Plugin activation hook for the REST API health check.
*
* @since n.e.x.t
*/
function od_rest_api_health_check_plugin_activation(): void {
// The option already exists, so do nothing.
if ( false !== get_option( 'od_rest_api_info' ) ) {
return;
}
// This will populate the od_rest_api_info option so that the function won't execute for the next page load.
od_run_scheduled_rest_api_health_check();
} and: add_action( 'admin_init`, 'od_rest_api_health_check_plugin_activation' ); 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 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 /**
* Plugin activation hook for the REST API health check.
*
* @since n.e.x.t
*/
function od_rest_api_health_check_plugin_activation(): void {
// If the option already exists, do nothing.
if ( false !== get_option( 'od_rest_api_info' ) ) {
return;
}
// This will populate the od_rest_api_info option so that the function won't execute on the next page load.
od_optimization_detective_rest_api_test();
}
add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' ); 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. Also for the admin notices which needs to be displayed only once after plugin activation can be included in this function. /**
* Plugin activation hook for the REST API health check.
*
* @since n.e.x.t
*/
function od_rest_api_health_check_plugin_activation(): void {
// If the option already exists, do nothing.
$rest_api_info = get_option( 'od_rest_api_info' );
if ( false !== $rest_api_info ) {
return;
}
// This will populate the od_rest_api_info option so that the function won't execute on the next page load.
od_optimization_detective_rest_api_test();
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( $rest_api_info['error_message'] ),
array(
'type' => 'warning',
'additional_classes' => array( 'notice-alt' ),
)
);
}
}
add_action( 'admin_init', 'od_rest_api_health_check_plugin_activation' ); |
||
} | ||
|
||
// Register this copy of the plugin. | ||
|
@@ -127,5 +139,8 @@ | |
|
||
// Add hooks for the above requires. | ||
require_once __DIR__ . '/hooks.php'; | ||
|
||
// Load site health checks. | ||
require_once __DIR__ . '/site-health.php'; | ||
} | ||
); |
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,188 @@ | ||||||||||||||
<?php | ||||||||||||||
/** | ||||||||||||||
* Site Health checks. | ||||||||||||||
* | ||||||||||||||
* @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; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* 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' ); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* 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' ), | ||||||||||||||
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. Related to https://github.com/WordPress/performance/pull/1762/files#r1907962460, I think this should not use
Suggested change
|
||||||||||||||
) | ||||||||||||||
); | ||||||||||||||
Comment on lines
+158
to
+164
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. In wp-env, which doesn't support loopback requests, it's expected for there an error message to display. And I am indeed getting one, but it's not telling me what impact the error has: In other words, this error is happening, but it should explain that Optimization Detective will not work because of the error. In other words, the same strings shown on the Site Health screen should be shown here as well: performance/plugins/optimization-detective/site-health.php Lines 63 to 68 in 98fff4d
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. Currently, I am displaying 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. Sorry, where are you displaying 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. 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. Show the more detailed notice. In fact, perhaps only show the notice in |
||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/** | ||||||||||||||
* 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(); | ||||||||||||||
// 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,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.
Question: Why use the
after_plugin_row_meta
action? If it so happens that the user has many plugins active, which is common, then Optimization Detective may not appear on the initial page and so the error will never be seen. I think theadmin_notices
action is more appropriate.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.
Or rather there could be an
inline
notice with the plugin row which displays always, and then there could also be a notice shown atadmin_notices
for the first time that the logic in https://github.com/WordPress/performance/pull/1762/files#r1907965242 runs. In this way, a user should see the notice after activating the plugin on the plugin list table screen, but then it will go away after reloading to stop nagging them forever. But they'll still be able to see the notice if they scroll down to Optimization Detective as well as when they open Site Health.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.
A potential solution could be to display a one-time notice upon plugin activation, as suggested in this comment #1762(comment). Additionally, a persistent error message can be shown
inline
using theafter_plugin_row_meta
hook.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.
Yeah, a one-time notice upon activation, and then otherwise if this one-time notice is not displayed at
admin_notices
to then instead show it as an inline notice atafter_plugin_row_meta
.