Connection: Add REST support for jsonAPI endpoints #39432

sergeymitr · 2024-09-18T09:42:31Z

Proposed changes:

Add REST support for jsonAPI endpoints

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

pf5801-18d-p2

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

See D162401-code

github-actions · 2024-09-18T09:43:21Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the add/json-api-direct-access branch.
- For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack add/json-api-direct-access

bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/json-api-direct-access

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-09-18T09:43:30Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
🔴 Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

WordPress.com Simple releases happen semi-continuously (PCYsg-Jjm-p2).
WoA releases happen weekly.
Releases to self-hosted sites happen monthly. The next release is scheduled for January 7, 2025 (scheduled code freeze on undefined).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

projects/plugins/jetpack/class.json-api-endpoints.php

…h issues.

fgiannar · 2024-11-01T11:11:47Z

projects/plugins/jetpack/class.json-api-endpoints.php

+		$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() ) ) {


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?

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.

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()

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

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.

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.

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

jetpack/projects/plugins/jetpack/class-jetpack-xmlrpc-methods.php

Line 181 in 8abca0d

$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.

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

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.

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.

sergeymitr · 2024-11-07T22:16:16Z

Hi @fgiannar
I copied a little bit of code from the serve() method: 22e01d7
There's some more, but I'd rather approach that as we get to the endpoint it's actually needed for.
Please take a look when you get a minute.

fgiannar · 2024-11-14T10:25:43Z

I copied a little bit of code from the serve() method: 22e01d7
There's some more, but I'd rather approach that as we get to the endpoint it's actually needed for.
Please take a look when you get a minute.

I believe we also need to copy the output logic from serve, otherwise we don't properly return WP_Error responses.
You can easily test this by modifying the endpoint's capabilities to dummy and confirm that we get a 200 response instead of the permissions error

fgiannar · 2024-12-02T09:27:27Z

projects/plugins/jetpack/class.json-api-endpoints.php

+			array_values( array( $this->path, $blog_id ) + $request->get_url_params() )
+		);
+
+		if ( ! $callback_response && ! is_array( $callback_response ) ) {


Do we really need to use the output method? Mostly asking because it seems to overcomplicate the approach..
I was testing this out and it seems to work fine if we simply return early in case of WP_Error: if ( is_wp_error( $callback_response ) ) { and leave the rest logic same as before.

Hi @fgiannar 👋
We use the output() method here to wrap the response if http_envelope=1 is provided.
There was a bug though which didn't make it all work properly, fixed in bbb226a (don't mind the accidental $response = null;, already reverted).

Here's what I get when I make $callback_response = null; with http_envelope:

HTTP/2 200 {"code":400,"headers":[{"name":"Content-Type","value":"application\/json"}],"body":{"error":"server_error","message":"The Jetpack site is inaccessible or returned an error: transport error - HTTP status code was not 200 (500) [-32300]"}}

And without http_envelope:

HTTP/2 400 {"error":"server_error","message":"The Jetpack site is inaccessible or returned an error: transport error - HTTP status code was not 200 (500) [-32300]"}

fgiannar · 2024-12-16T09:15:26Z

projects/plugins/jetpack/class.json-api-endpoints.php

+		}
+
+		if ( $request->get_param( 'http_envelope' ) ) {
+			$response = wp_json_encode( WPCOM_JSON_API::wrap_http_envelope( $status_code, $response, 'application/json' ) );


I believe we need to json encode the response independently of the http_envelope param. Otherwise, we get a PHP Warning locally: PHP Warning: Array to string conversion in class.json-api-endpoints.php on line 2740 and a Fatal on WPCOM: Fatal error: Uncaught Error: Object of class stdClass could not be converted to string

Connection: REST access for jsonAPI endpoints.

3a60d13

sergeymitr added the [Status] In Progress label Sep 18, 2024

sergeymitr self-assigned this Sep 18, 2024

github-actions bot added [Feature] WPCOM API [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Sep 18, 2024

sergeymitr added 2 commits September 18, 2024 14:58

Make endpoints provide their own REST route.

9b85565

Fix routes for name and slug requests.

36368f0

fgiannar reviewed Sep 18, 2024

View reviewed changes

projects/plugins/jetpack/class.json-api-endpoints.php Outdated Show resolved Hide resolved

sergeymitr mentioned this pull request Sep 23, 2024

Jetpack API: add 'jsonAPI' REST endpoint #38555

Closed

3 tasks

sergeymitr added 2 commits September 23, 2024 13:00

Merge branch 'trunk' into add/json-api-direct-access

61b021e

Add blog/user token verification.

8bfbadb

github-actions bot added the [Package] Connection label Sep 23, 2024

sergeymitr added 6 commits September 23, 2024 17:07

Add the version prefix to the endpoints.

6357e97

Authenticate WP user if user token is provided, fix a few smaller aut…

9b5e5ae

…h issues.

Merge branch 'trunk' into add/json-api-direct-access

20974a0

Fix a phpdoc typo.

52fe990

Activate REST for the '/plugins' endpoint.

549e826

Improve REST authentication.

a3cc273

sergeymitr force-pushed the add/json-api-direct-access branch from 1b9fbb2 to a3cc273 Compare September 26, 2024 20:53

sergeymitr added 7 commits October 11, 2024 15:02

Merge branch 'trunk' into add/json-api-direct-access

0d757d3

Improve custom permission checking.

4aaa656

Add missing phpdoc.

40eec5f

Merge branch 'trunk' into add/json-api-direct-access

b4836df

Implement response signature.

ab97174

Merge branch 'trunk' into add/json-api-direct-access

9ba813f

Make endpoints provide minimum the Jetpack version.

bb92f7a

sergeymitr force-pushed the add/json-api-direct-access branch from 9633b2c to bb92f7a Compare October 25, 2024 14:52

Revert Post endpoints changes, we'll focus on the Plugins one for now.

3db7335

fgiannar removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Oct 30, 2024

sergeymitr added 2 commits October 30, 2024 11:26

Don't create REST route on WPCOM.

9b3202f

Remove excessive REST authentication call.

2e1ae27

sergeymitr added [Status] In Progress and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Oct 31, 2024

Remove excessive admin.php loading.

8abca0d

fgiannar reviewed Nov 1, 2024

View reviewed changes

sergeymitr added 3 commits November 4, 2024 13:02

Remove excessive fallback to blog token.

e97dd90

Improve comments for permission check callbacks.

79fdce8

Copy some code from XML-RPC's 'serve'.

22e01d7

Handle locale.

01a8fbc

sergeymitr added 6 commits November 14, 2024 16:17

Merge branch 'trunk' into add/json-api-direct-access

a0ec6ce

Merge branch 'trunk' into add/json-api-direct-access

541cbe3

Properly wrap REST JSON API errors.

8013673

Properly wrap REST response.

b2b467a

Merge branch 'trunk' into add/json-api-direct-access

b9faed5

Fix phan error.

eb68e2c

fgiannar reviewed Dec 2, 2024

View reviewed changes

sergeymitr added 9 commits December 3, 2024 10:32

Merge branch 'trunk' into add/json-api-direct-access

d2a9835

Fix the empty output response.

bbb226a

Revert accidental debugging code commit.

debee21

Merge branch 'trunk' into add/json-api-direct-access

95e1279

Proper response wrapping and error handling.

2b2ff8c

Merge branch 'trunk' into add/json-api-direct-access

a4806b7

Revert changes to the 'output()' method.

9d6f78a

Mix some reverting issues.

9739eaa

Fix a type mismatch.

4850259

fgiannar reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection: Add REST support for jsonAPI endpoints #39432

Connection: Add REST support for jsonAPI endpoints #39432

sergeymitr commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

fgiannar Nov 1, 2024

sergeymitr Nov 4, 2024

fgiannar Nov 5, 2024

sergeymitr Nov 7, 2024 •

edited

Loading

fgiannar Nov 8, 2024

sergeymitr Nov 8, 2024

sergeymitr Nov 8, 2024

sergeymitr commented Nov 7, 2024

fgiannar commented Nov 14, 2024 •

edited

Loading

fgiannar Dec 2, 2024

sergeymitr Dec 3, 2024 •

edited

Loading

fgiannar Dec 16, 2024

Connection: Add REST support for jsonAPI endpoints #39432

Are you sure you want to change the base?

Connection: Add REST support for jsonAPI endpoints #39432

Conversation

sergeymitr commented Sep 18, 2024 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeymitr Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeymitr commented Nov 7, 2024

fgiannar commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

sergeymitr Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeymitr commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

sergeymitr Nov 7, 2024 •

edited

Loading

fgiannar commented Nov 14, 2024 •

edited

Loading

sergeymitr Dec 3, 2024 •

edited

Loading