Make WordPress Core

Opened 5 years ago

Last modified 5 years ago

#47676 new defect (bug)

Add support for If-Unmodified-Since header to make a conditional request when updating posts via REST API (for offline and collaborative editing)

Reported by: westonruter's profile westonruter Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Editor Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

In order to guard against accidentally overwriting someone else's changes when updating a post via the REST API, there should be support added for Conditional Requests. If a post update request includes an If-Modified-Since header and the supplied date is before the post's post_modified_gmt then the request should fail with a 412 Precondition Failed response, with no change to the post being made.

It seems generally useful to include this If-Unmodified-Since header when making requests to update posts in the editor as additional protection on top of post locking.

Additionally, this header will be important when adding support for offline editing, in particular when a service worker (see PWA feature plugin) is sending queued REST API requests which were made while the user was offline (via background sync). Since it could be several hours or days between when a post was edited and the user goes back online, there needs to be protection against overwriting the post on the server if it has been modified in the mean time.

When the service worker uses background sync to re-send the request and it fails with a 412 Precondition Failed response, this would then be a signal for the service worker to re-submit the changes as an autosave revision which could then be shown to the user via some conflict resolution UI the next time they access the editor.

As such, this is also important for the collaborative editing in a future phase.

See also comments at https://github.com/WordPress/gutenberg/issues/6322#issuecomment-410578818

Attachments (1)

47676.0.diff (4.9 KB) - added by westonruter 5 years ago.
https://github.com/westonruter/wordpress-develop/pull/3

Download all attachments as: .zip

Change History (6)

#1 @westonruter
5 years ago

  • Keywords has-patch has-unit-tests added

#2 follow-up: @TimothyBlynJacobs
5 years ago

Why If-Unmodified-Since instead of using ETag's? An ETag could be built by serializing the item response. That would handle updates to the entity that don't trigger post_modified like metadata, terms, and any other custom fields.

Or is the fact that post_modified would only track changes to the post resource a benefit?

One downside to an ETag is it'd be more intensive to calculate. I suppose if we wanted to get similar benefits, we could force a post_modified update whenever a post entity is updated through the posts controller.


If the If-Unmodified-Since is used I think we should definitely send Last-Modified as a header, and ideally support If-Modified-Since as well. Last-Modified can also be sent as a targetHint so it is accessible in the collection route.

#3 in reply to: ↑ 2 @westonruter
5 years ago

Replying to TimothyBlynJacobs:

Why If-Unmodified-Since instead of using ETag's? An ETag could be built by serializing the item response.

Good question. I didn't think about that. One issue with using ETag generated from serializing the response is that it would require always querying data for all fields, even if they are not requested by the user. For example, if someone had obtained the entity originally via /wp-json/wp/v2/posts/2553?_fields=id,title,link and then they modify the title and PUT back the modified post, then the ETag would not have been generated to include the content. Or perhaps this doesn't matter because you only requested the title in the first place?

That would handle updates to the entity that don't trigger post_modified like metadata, terms, and any other custom fields.
I suppose if we wanted to get similar benefits, we could force a post_modified update whenever a post entity is updated through the posts controller.

That is a very good point. For custom fields, the only thing to protect against that would be to store some additional last_modified in postmeta and bump it whenever postmeta is changed or term associations are changed, such as via the posts controller. But that's not ideal.

Or is the fact that post_modified would only track changes to the post resource a benefit?

The post_modified would probably be an 80% solution.

If the If-Unmodified-Since is used I think we should definitely send Last-Modified as a header, and ideally support If-Modified-Since as well. Last-Modified can also be sent as a targetHint so it is accessible in the collection route.

Another great point. Yes, sending the Last-Modified header as derived from post_modified_gmt is something I missed. A client should not be trying to convert the post_modified_gmt into a value to for the If-Unmodified-Since header.

One downside to an ETag is it'd be more intensive to calculate.

I see it being more intensive in two ways:

  1. The get_item method would need to compute the ETag from serializing the response data.
  2. The update_item method would need to call get_item if the request has an If-Match header, and then compare the get_item method's response ETag with the PUT request's If-Match header to see if there is a match.

In other words:

  • src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php

    a b class WP_REST_Posts_Controller extends WP_REST_Controller { 
    477477                        $response->link_header( 'alternate', get_permalink( $post->ID ), array( 'type' => 'text/html' ) );
    478478                }
    479479
     480                $response->set_headers(
     481                        array(
     482                                'ETag' => sprintf( '"%s"', md5( serialize( $response->get_data() ) ) ),
     483                        )
     484                );
     485
    480486                return $response;
    481487        }
    482488
    class WP_REST_Posts_Controller extends WP_REST_Controller { 
    685691                        return $valid_check;
    686692                }
    687693
     694                // Handle conditional request.
     695                if ( $request->get_header( 'If-Match' ) ) {
     696                        $get_request = clone $request;
     697                        $get_request->set_method( 'GET' );
     698                        $get_request->set_body( null );
     699                        $etag = md5( serialize( $this->get_item( $get_request )->get_data() ) );
     700                        if ( $etag !== trim( $request->get_header( 'If-Match' ), '"' ) ) {
     701                                return new WP_Error( 'rest_precondition_failed', __( 'Post has been changed on server. Please resolve conflicts and try again' ), array( 'status' => 412 ) );
     702                        }
     703                }
     704
    688705                $post = $this->prepare_item_for_database( $request );
    689706
    690707                if ( is_wp_error( $post ) ) {
Last edited 5 years ago by westonruter (previous) (diff)

#4 @swissspidy
5 years ago

  • Milestone changed from Awaiting Review to Future Release

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

Note: See TracTickets for help on using tickets.