Skip to content
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

Connection: Add REST support for jsonAPI endpoints #39432

Open
wants to merge 41 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
3a60d13
Connection: REST access for jsonAPI endpoints.
sergeymitr Sep 18, 2024
9b85565
Make endpoints provide their own REST route.
sergeymitr Sep 18, 2024
36368f0
Fix routes for name and slug requests.
sergeymitr Sep 18, 2024
61b021e
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Sep 23, 2024
8bfbadb
Add blog/user token verification.
sergeymitr Sep 23, 2024
6357e97
Add the version prefix to the endpoints.
sergeymitr Sep 23, 2024
9b5e5ae
Authenticate WP user if user token is provided, fix a few smaller aut…
sergeymitr Sep 24, 2024
20974a0
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Sep 25, 2024
52fe990
Fix a phpdoc typo.
sergeymitr Sep 25, 2024
549e826
Activate REST for the '/plugins' endpoint.
sergeymitr Sep 25, 2024
a3cc273
Improve REST authentication.
sergeymitr Sep 26, 2024
0d757d3
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Oct 11, 2024
4aaa656
Improve custom permission checking.
sergeymitr Oct 11, 2024
40eec5f
Add missing phpdoc.
sergeymitr Oct 11, 2024
b4836df
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Oct 16, 2024
ab97174
Implement response signature.
sergeymitr Oct 17, 2024
9ba813f
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Oct 24, 2024
bb92f7a
Make endpoints provide minimum the Jetpack version.
sergeymitr Oct 25, 2024
3db7335
Revert Post endpoints changes, we'll focus on the Plugins one for now.
sergeymitr Oct 25, 2024
9b3202f
Don't create REST route on WPCOM.
sergeymitr Oct 30, 2024
2e1ae27
Remove excessive REST authentication call.
sergeymitr Oct 30, 2024
8abca0d
Remove excessive admin.php loading.
sergeymitr Oct 31, 2024
e97dd90
Remove excessive fallback to blog token.
sergeymitr Nov 4, 2024
79fdce8
Improve comments for permission check callbacks.
sergeymitr Nov 4, 2024
22e01d7
Copy some code from XML-RPC's 'serve'.
sergeymitr Nov 7, 2024
01a8fbc
Handle locale.
sergeymitr Nov 13, 2024
a0ec6ce
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Nov 14, 2024
541cbe3
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Nov 19, 2024
8013673
Properly wrap REST JSON API errors.
sergeymitr Nov 19, 2024
b2b467a
Properly wrap REST response.
sergeymitr Nov 20, 2024
b9faed5
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Nov 21, 2024
eb68e2c
Fix phan error.
sergeymitr Nov 21, 2024
d2a9835
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Dec 3, 2024
bbb226a
Fix the empty output response.
sergeymitr Dec 3, 2024
debee21
Revert accidental debugging code commit.
sergeymitr Dec 3, 2024
95e1279
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Dec 10, 2024
2b2ff8c
Proper response wrapping and error handling.
sergeymitr Dec 13, 2024
a4806b7
Merge branch 'trunk' into add/json-api-direct-access
sergeymitr Dec 13, 2024
9d6f78a
Revert changes to the 'output()' method.
sergeymitr Dec 13, 2024
9739eaa
Mix some reverting issues.
sergeymitr Dec 13, 2024
4850259
Fix a type mismatch.
sergeymitr Dec 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Add the 'is_signed_with_user_token()' method for REST authentication.
13 changes: 13 additions & 0 deletions projects/packages/connection/src/class-rest-authentication.php
Original file line number Diff line number Diff line change
Expand Up @@ -219,4 +219,17 @@ public static function is_signed_with_blog_token() {

return true === $instance->rest_authentication_status && 'blog' === $instance->rest_authentication_type;
}

/**
* Whether the request was signed with a user token.
*
* @since $$next-version$$
*
* @return bool True if the request was signed with a valid user token, false otherwise.
*/
public static function is_signed_with_user_token() {
$instance = self::init();

return true === $instance->rest_authentication_status && 'user' === $instance->rest_authentication_type;
}
}
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/add-json-api-direct-access
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: other

Add REST support for jsonAPI endpoints.
3 changes: 0 additions & 3 deletions projects/plugins/jetpack/class-jetpack-xmlrpc-methods.php
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,6 @@ public static function json_api( $args = array() ) {
define( 'REST_API_REQUEST', true );
define( 'WPCOM_JSON_API__BASE', 'public-api.wordpress.com/rest/v1' );

// needed?
require_once ABSPATH . 'wp-admin/includes/admin.php';

require_once JETPACK__PLUGIN_DIR . 'class.json-api.php';
$api = WPCOM_JSON_API::init( $method, $url, $post_body );
$api->token_details['user'] = $user_details;
Expand Down
18 changes: 18 additions & 0 deletions projects/plugins/jetpack/class.jetpack.php
Original file line number Diff line number Diff line change
Expand Up @@ -855,8 +855,11 @@ function ( $methods ) {
if ( $is_connection_ready ) {
require_once JETPACK__PLUGIN_DIR . '_inc/lib/class.jetpack-iframe-embed.php';
add_action( 'init', array( 'Jetpack_Iframe_Embed', 'init' ), 9, 0 );

require_once JETPACK__PLUGIN_DIR . '_inc/lib/class.jetpack-keyring-service-helper.php';
add_action( 'init', array( 'Jetpack_Keyring_Service_Helper', 'init' ), 9, 0 );

add_action( 'rest_api_init', array( $this, 'maybe_initialize_rest_jsonapi' ) );
}
}

Expand Down Expand Up @@ -6111,6 +6114,21 @@ public function run_initialize_tracking_action() {
do_action( 'jetpack_initialize_tracking' );
}

/**
* Initialize REST jsonAPI if needed.
*
* @return void
*/
public function maybe_initialize_rest_jsonapi() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that maybe it would make sense to shuffle things a bit so that we make this method consistent with json_api here.

require_once JETPACK__PLUGIN_DIR . 'class.json-api.php';
WPCOM_JSON_API::init( $method, $url, $post_body ); // And we register the routes via the api

// phpcs:ignore WordPress.Security.NonceVerification.Recommended
if ( ! empty( $_GET['jsonapi'] ) && ( ! defined( 'IS_WPCOM' ) || ! IS_WPCOM ) ) {
require_once ABSPATH . 'wp-admin/includes/admin.php'; // JSON API relies on WP functionality not autoloaded in REST.
fgiannar marked this conversation as resolved.
Show resolved Hide resolved

define( 'WPCOM_JSON_API__BASE', 'public-api.wordpress.com/rest/v1' );
require_once JETPACK__PLUGIN_DIR . 'class.json-api-endpoints.php';
}
}

/**
* Run plugin post-activation actions if we need to.
*
Expand Down
158 changes: 158 additions & 0 deletions projects/plugins/jetpack/class.json-api-endpoints.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
*/

use Automattic\Jetpack\Connection\Client;
use Automattic\Jetpack\Connection\Manager;
use Automattic\Jetpack\Connection\Rest_Authentication;
use Automattic\Jetpack\Connection\Tokens;
use Automattic\Jetpack\Status;

require_once __DIR__ . '/json-api-config.php';
Expand Down Expand Up @@ -124,6 +127,20 @@ abstract class WPCOM_JSON_API_Endpoint {
*/
public $path_labels = array();

/**
* The REST endpoint if available.
*
* @var string
*/
public $rest_route;

/**
* Jetpack Version in which REST support was introduced.
*
* @var string
*/
public $rest_min_jp_version;

/**
* Accepted query parameters
*
Expand Down Expand Up @@ -277,6 +294,11 @@ abstract class WPCOM_JSON_API_Endpoint {
*/
public $allow_fallback_to_jetpack_blog_token = false;

/**
* REST namespace.
*/
const REST_NAMESPACE = 'jetpack/rest';

/**
* Constructor.
*
Expand All @@ -300,6 +322,8 @@ public function __construct( $args ) {
'new_version' => WPCOM_JSON_API__CURRENT_VERSION,
'jp_disabled' => false,
'path_labels' => array(),
'rest_route' => null,
'rest_min_jp_version' => null,
'request_format' => array(),
'response_format' => array(),
'query_parameters' => array(),
Expand Down Expand Up @@ -339,6 +363,9 @@ public function __construct( $args ) {
$this->deprecated = $args['deprecated'];
$this->new_version = $args['new_version'];

$this->rest_route = $args['rest_route'];
$this->rest_min_jp_version = $args['rest_min_jp_version'];

// Ensure max version is not less than min version.
if ( version_compare( $this->min_version, $this->max_version, '>' ) ) {
$this->max_version = $this->min_version;
Expand Down Expand Up @@ -387,6 +414,10 @@ public function __construct( $args ) {
$this->example_response = $args['example_response'];

$this->api->add( $this );

if ( ( ! defined( 'IS_WPCOM' ) || ! IS_WPCOM ) && $this->rest_route && ( ! defined( 'XMLRPC_REQUEST' ) || ! XMLRPC_REQUEST ) ) {
$this->create_rest_route_for_endpoint();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should never happen on WPCOM :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it feels like this logic would be better suited to be part of the api, aka WPCOM_JSON_API vs the endpoint definitions. Is there a reason you chose to add it here?

Copy link
Contributor Author

@sergeymitr sergeymitr Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I added the "not WPCOM" condition: 9b3202f

Is there a reason you chose to add it here?

The REST endpoint belongs to the endpoint object, so that seems logical to me to let the endpoint object initialize it.
That way the whole logic is encapsulated within a single class, simplifying the code.

}
}

/**
Expand Down Expand Up @@ -2618,6 +2649,133 @@ public function get_amp_cache_origins( $siteurl ) {
);
}

/**
* Register a REST route for this jsonAPI endpoint.
*
* @return void
* @throws Exception The exception if something goes wrong.
*/
public function create_rest_route_for_endpoint() {
register_rest_route(
static::REST_NAMESPACE,
$this->build_rest_route(),
array(
'methods' => $this->method,
'callback' => array( $this, 'rest_callback' ),
'permission_callback' => array( $this, 'rest_permission_callback' ),
)
);
}

/**
* Handle the rest call.
*
* @param WP_REST_Request $request The request object.
*
* @return mixed|WP_Error
*/
public function rest_callback( WP_REST_Request $request ) {
$blog_id = Jetpack_Options::get_option( 'id' );

$this->api->initialize();
$this->api->endpoint = $this;

$response = call_user_func_array(
array( $this, 'callback' ),
array_values( array( $this->path, $blog_id ) + $request->get_url_params() )
);

$token_data = ( new Manager() )->verify_xml_rpc_signature();

if ( ! $token_data || empty( $token_data['token_key'] ) || ! array_key_exists( 'user_id', $token_data ) ) {
return new WP_Error( 'response_signature_error' );
}

$token = ( new Tokens() )->get_access_token( $token_data['user_id'], $token_data['token_key'] );
if ( is_wp_error( $token ) ) {
return $token;
}
if ( ! $token ) {
return new WP_Error( 'response_signature_error' );
}

$response = wp_json_encode( $response );
$nonce = wp_generate_password( 10, false );
$hmac = hash_hmac( 'sha1', $nonce . $response, $token->secret );

return array(
$response,
(string) $nonce,
(string) $hmac,
);
}

/**
* The REST endpoint should only be available for requests signed with a valid blog or user token.
* Declaring it "final" so individual endpoints couldn't remove this requirement.
*
* @return true|WP_Error
*/
final public function rest_permission_callback() {
$manager = new Manager( 'jetpack' );
if ( ! $manager->is_connected() ) {
return new WP_Error( 'site_not_connected' );
}

$user_id = get_current_user_id();
$allow_blog_token = $this->allow_fallback_to_jetpack_blog_token || $this->allow_jetpack_site_auth;

if ( ( $allow_blog_token && Rest_Authentication::is_signed_with_blog_token() ) || ( $user_id && Rest_Authentication::is_signed_with_user_token() ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have accepts_site_based_authentication which essentially covers the cases of blog tokens used plus the endpoints accepting it. We don't need to check allow_fallback_to_jetpack_blog_token, this is only used on WPCOM to determine if unauthorized requests can fall back to using a blog token.
That said, we already check accepts_site_based_authentication as part of the capabilities check here but it only applies to endpoints extending Jetpack_JSON_API_Endpoint 🤔
Also do we really need to check Rest_Authentication::is_signed_with_user_token()? Wouldn't it be enough to check if $user_id is not empty assuming the request is signed with a user token?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to check allow_fallback_to_jetpack_blog_token

Good point, removed in e97dd90

Also do we really need to check Rest_Authentication::is_signed_with_user_token()? Wouldn't it be enough to check if $user_id is not empty assuming the request is signed with a user token?

We don't really want to open those endpoints for requests coming from logged in users.
So we check if this actually is user token based auth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, removed in e97dd90

I believe we can use $this->accepts_site_based_authentication instead of $this->allow_jetpack_site_auth && Rest_Authentication::is_signed_with_blog_token()

Copy link
Contributor Author

@sergeymitr sergeymitr Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, but I'm not quite sure we should do that:

  1. token_details will have blog_id defined for both user and blog token, so is_jetpack_authorized_for_site() will essentially use get_current_user_id() to determine the type of token (blog/user).
    is_signed_with_blog_token() takes that information directly from token, so I find that more straightforward and reliable.

  2. We initialize JSON API in the callback. That means that token_details is empty during the REST endpoint permission callback, and I'd rather avoid that.

  3. I also don't feel confident about relying on public_token property because of this line:

    $api->token_details['user'] = $user_details;

    $user_details here comes from WPCOM directly, and trusted only because the request is signed. It's not something that comes from the token.
    While it doesn't affect the blog token check in question directly, it's still something I'd rather stay away from.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we should probably update accepts_site_based_authentication then and re-use it everywhere, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a widely used method already, so editing it may have unintended side effects in such a sensitive component as JSON API.
So I wouldn't want to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is, we're already doing some major changes here, so I'd prefer to minimize potential breaking points where we can.
So I'd rather avoid excessive refactoring, and revisit it separately later on.

$custom_permission_result = $this->rest_permission_callback_custom();

// Successful custom permission check.
if ( $custom_permission_result === true ) {
return true;
}

// Custom permission check errored, returning the error.
if ( is_wp_error( $custom_permission_result ) ) {
return $custom_permission_result;
}

// Custom permission check failed, but didn't return a specific error. Proceed to returning the generic error.
}

$message = esc_html__(
'You do not have the correct user permissions to perform this action. Please contact your site admin if you think this is a mistake.',
'jetpack'
);
return new WP_Error( 'rest_api_invalid_permission', $message, array( 'status' => rest_authorization_required_code() ) );
}

/**
* Redefine in individual endpoint classes to further customize the permission check.
*
* @return true|WP_Error
*/
public function rest_permission_callback_custom() {
fgiannar marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

/**
* Build the REST endpoint URL.
*
* @return string
*/
public function build_rest_route() {
$version_prefix = $this->max_version ? 'v' . $this->max_version : '';
return $version_prefix . $this->rest_route;
fgiannar marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Get Jetpack Version where support for the endpoint was introduced.
*
* @return string
*/
public function get_rest_min_jp_version() {
return $this->rest_min_jp_version;
}

/**
* Return endpoint response
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
'description' => 'Get installed Plugins on your blog',
'method' => 'GET',
'path' => '/sites/%s/plugins',
'rest_route' => '/plugins',
'rest_min_jp_version' => '14.0-a.7',
'stat' => 'plugins',
'min_version' => '1',
'max_version' => '1.1',
Expand Down
Loading