Cache REST API endpoints #121

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
8 participants
@iandunn
Member

iandunn commented Jun 8, 2016

This is still a work in progress, but a good chunk of it is there.

I'd love to get some feedback from @donnchawp and @kraftbj. Do you think this is heading in the right direction? Are there any major parts missing? Any other thoughts?

One thing I haven't looked at yet is support for mod_rewrite, so this is just for legacy and PHP modes.

@iandunn iandunn added the enhancement label Jun 8, 2016

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Jun 8, 2016

Member

The main things this is doing right now are:

  1. wpsc_rest_eof_tags() - This adds regex patterns for JSON objects and collections to the end-of-file tags, so that cache files will be generated for JSON "pages".

  2. wpsc_rest_toggle_slash_check() - This works around the $wp_cache_slash_check logic in wp_cache_serve_cache_file(), so that the cache files will be served for both {url} and {url}/.

  3. wpsc_rest_remove_comments() - This removes the HTML comments that normally get appended to the cache buffer -- e.g., Dynamic page generated in 0.578 seconds. -- because those break JSON parsing.

Member

iandunn commented Jun 8, 2016

The main things this is doing right now are:

  1. wpsc_rest_eof_tags() - This adds regex patterns for JSON objects and collections to the end-of-file tags, so that cache files will be generated for JSON "pages".

  2. wpsc_rest_toggle_slash_check() - This works around the $wp_cache_slash_check logic in wp_cache_serve_cache_file(), so that the cache files will be served for both {url} and {url}/.

  3. wpsc_rest_remove_comments() - This removes the HTML comments that normally get appended to the cache buffer -- e.g., Dynamic page generated in 0.578 seconds. -- because those break JSON parsing.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Jun 8, 2016

Member

There are two main problems left to solve:

  1. Adding support for mod_rewrite, so we don't have to hit PHP
  2. Ensuring the correct headers are sent, including for JSONP callbacks, and being future proof against any modifications to the current security headers the API sends.

I did some searching to see what other plugins have done in this space, but I only came up with 2 small ones, and they're both specifically targeting the REST API. None of the major general-purpose plugins have done this yet.

Those two plugins both use transients, so they're not really an effective solution for the type of user who would use a general-purpose caching plugin. Using transients is definitely better than nothing, but unless you have an object cache setup, you're still hitting MySQL. Even if you do have an object cache, you're still hitting PHP and loading all of WP.

The approach here should be much faster than that, even if we don't come up with a way to support mod_rewrite.

Member

iandunn commented Jun 8, 2016

There are two main problems left to solve:

  1. Adding support for mod_rewrite, so we don't have to hit PHP
  2. Ensuring the correct headers are sent, including for JSONP callbacks, and being future proof against any modifications to the current security headers the API sends.

I did some searching to see what other plugins have done in this space, but I only came up with 2 small ones, and they're both specifically targeting the REST API. None of the major general-purpose plugins have done this yet.

Those two plugins both use transients, so they're not really an effective solution for the type of user who would use a general-purpose caching plugin. Using transients is definitely better than nothing, but unless you have an object cache setup, you're still hitting MySQL. Even if you do have an object cache, you're still hitting PHP and loading all of WP.

The approach here should be much faster than that, even if we don't come up with a way to support mod_rewrite.

@kraftbj kraftbj added this to the 1.4.9 milestone Aug 2, 2016

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Aug 2, 2016

Member

I rebased the branch into a handful of commits, to make reviewing easier, and to give it a cleaner merge history. I haven't found any problems in my tests, so I think it's ready for review.

@kraftbj Do you mind taking a look and giving me any feedback you have? I can see three options for moving forward:

  1. Merge the entire pull request and leave the REST plugin on by default.
  2. Merge the entire pull request, but change the REST plugin to be off by default. After it's gotten some real-world testing, we can switch it to be on by default.
  3. Only merge the new filters, but not the REST plugin. Then, I can run the plugin on WordCamp.org for awhile just to get some real-world testing, and then we can go back to options 1 or 2.
Member

iandunn commented Aug 2, 2016

I rebased the branch into a handful of commits, to make reviewing easier, and to give it a cleaner merge history. I haven't found any problems in my tests, so I think it's ready for review.

@kraftbj Do you mind taking a look and giving me any feedback you have? I can see three options for moving forward:

  1. Merge the entire pull request and leave the REST plugin on by default.
  2. Merge the entire pull request, but change the REST plugin to be off by default. After it's gotten some real-world testing, we can switch it to be on by default.
  3. Only merge the new filters, but not the REST plugin. Then, I can run the plugin on WordCamp.org for awhile just to get some real-world testing, and then we can go back to options 1 or 2.
@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Aug 22, 2016

Member

@kraftbj, any thoughts on the above?

Member

iandunn commented Aug 22, 2016

@kraftbj, any thoughts on the above?

@kraftbj

This comment has been minimized.

Show comment
Hide comment
@kraftbj

kraftbj Aug 23, 2016

Contributor

Apologies for the delay. I'll take a look tomorrow.

Contributor

kraftbj commented Aug 23, 2016

Apologies for the delay. I'll take a look tomorrow.

+ * Need to update the cache when underlying data changes, rather than just waiting for manual expiration?
+ * would be bad if deleted post continue to show up in cache, etc
+ * how to tell that updating a post or category maps to an API endpoint?
+ * worst case, could just flush all endpoint cache files when any post/comment/taxterm/etc is updated

This comment has been minimized.

@kraftbj

kraftbj Aug 23, 2016

Contributor

I'd like to see if there's a way to determine what we need to clear in a more specific manner. Nuking everything is more conservative but afraid about scaling on really large sites.

@kraftbj

kraftbj Aug 23, 2016

Contributor

I'd like to see if there's a way to determine what we need to clear in a more specific manner. Nuking everything is more conservative but afraid about scaling on really large sites.

This comment has been minimized.

@iandunn

iandunn Aug 23, 2016

Member

Yeah, that's a good point to bring up. I don't have any ideas for how to map content to API endpoints right now (other than just a hardcoded list for known endpoints like /posts).

One thing that could help avoid scaling issues, would be to keep track of when we flush the cache, and don't do it more often then once every 5 minutes, or something like that. We'll need to do more thinking/testing/etc there, though

@iandunn

iandunn Aug 23, 2016

Member

Yeah, that's a good point to bring up. I don't have any ideas for how to map content to API endpoints right now (other than just a hardcoded list for known endpoints like /posts).

One thing that could help avoid scaling issues, would be to keep track of when we flush the cache, and don't do it more often then once every 5 minutes, or something like that. We'll need to do more thinking/testing/etc there, though

This comment has been minimized.

@rmccue

rmccue Aug 27, 2016

You can check rest_base on the post types, but otherwise there's no generic mapping.

@rmccue

rmccue Aug 27, 2016

You can check rest_base on the post types, but otherwise there's no generic mapping.

+
+ /*
+ * Disable the rest of the plugin if that was explicitly configured.
+ * Otherwise, leave it enabled by default and set a canonical value for `$cache_rest_api`.

This comment has been minimized.

@kraftbj

kraftbj Aug 23, 2016

Contributor

I'm fine with making this default. Decisions not options and if someone is using the API, it makes sense to me to cache it from the onset.

@kraftbj

kraftbj Aug 23, 2016

Contributor

I'm fine with making this default. Decisions not options and if someone is using the API, it makes sense to me to cache it from the onset.

wp-cache-phase1.php
@@ -187,7 +189,7 @@ function wp_cache_serve_cache_file() {
$wp_cache_home_path = '/';
// make sure ending slashes are ok
- if ( $wp_cache_request_uri == $wp_cache_home_path || ( $wp_cache_slash_check && substr( $wp_cache_request_uri, -1 ) == '/' ) || ( $wp_cache_slash_check == 0 && substr( $wp_cache_request_uri, -1 ) != '/' ) ) {
+ if ( $wp_cache_request_uri == $wp_cache_home_path || ( $wp_cache_slash_check && substr( $wp_cache_request_uri, -1 ) == '/' ) || ( $wp_cache_slash_check == 0 && substr( $wp_cache_request_uri, -1 ) != '/' ) || do_cacheaction( 'serve_supercache_file', false ) ) {

This comment has been minimized.

@kraftbj

kraftbj Aug 23, 2016

Contributor

Just a styling note, but can we multiline this if?

if (
    $wp_cache_request_uri == $wp_cache_home_path ||
    ( $something && something ) ||
    etc
@kraftbj

kraftbj Aug 23, 2016

Contributor

Just a styling note, but can we multiline this if?

if (
    $wp_cache_request_uri == $wp_cache_home_path ||
    ( $something && something ) ||
    etc

This comment has been minimized.

@iandunn

iandunn Aug 23, 2016

Member

Ah, that's a good idea. I initially rewrote that line into multiple variables so that the condition itself would read like:

if ( $is_home_page || $request_meets_slashed_expectation || $request_meets_unslashed_expectation || $serve_supercache_file ) {

...but then I worried about making too many changes to the core plugin, so I reverted it back. Changing them to be on separate lines is a great middle-ground, though.

@iandunn

iandunn Aug 23, 2016

Member

Ah, that's a good idea. I initially rewrote that line into multiple variables so that the condition itself would read like:

if ( $is_home_page || $request_meets_slashed_expectation || $request_meets_unslashed_expectation || $serve_supercache_file ) {

...but then I worried about making too many changes to the core plugin, so I reverted it back. Changing them to be on separate lines is a great middle-ground, though.

This comment has been minimized.

@iandunn

iandunn Aug 23, 2016

Member

Done in 09346d2

@kraftbj

This comment has been minimized.

Show comment
Hide comment
@kraftbj

kraftbj Aug 23, 2016

Contributor

I really like it and reads wonderfully. I need to go back through to make sure there's a way to confirm you're being served a cached file; my initial tests didn't have a header or content added to the json, etc to confirm that it's working, clearing, etc.

Going to ask Guardians for a second set of eyes too.

Contributor

kraftbj commented Aug 23, 2016

I really like it and reads wonderfully. I need to go back through to make sure there's a way to confirm you're being served a cached file; my initial tests didn't have a header or content added to the json, etc to confirm that it's working, clearing, etc.

Going to ask Guardians for a second set of eyes too.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Aug 23, 2016

Member

it reads wonderfully

Thanks! That warms my heart :D

make sure there's a way to confirm you're being served a cached file

Ah, I should have documented what I did. Added that in ca8a37a.

Member

iandunn commented Aug 23, 2016

it reads wonderfully

Thanks! That warms my heart :D

make sure there's a way to confirm you're being served a cached file

Ah, I should have documented what I did. Added that in ca8a37a.

@iandunn iandunn referenced this pull request in WP-API/WP-API Aug 26, 2016

Closed

Caching #724

+ */
+function wpsc_rest_eof_tags( $eof_pattern ) {
+ $json_object_pattern = '^[{].*[}]$';
+ $json_collection_pattern = '^[\[].*[\]]$';

This comment has been minimized.

@rmccue

rmccue Aug 27, 2016

Note that null, "hello", and 21345 are all completely valid JSON responses as well, but core wouldn't generate that by default.

@rmccue

rmccue Aug 27, 2016

Note that null, "hello", and 21345 are all completely valid JSON responses as well, but core wouldn't generate that by default.

This comment has been minimized.

@iandunn

iandunn Nov 4, 2016

Member

Thanks, I'll add a @todo note about that for future iterations.

@iandunn

iandunn Nov 4, 2016

Member

Thanks, I'll add a @todo note about that for future iterations.

This comment has been minimized.

@breams

breams Dec 1, 2016

What about JSONP responses? They won't match either pattern. Is it as simple as adding extra patterns?

$jsonp_object_pattern     = '^\/\*\*\/[_.a-zA-Z0-9]+\([{].*[}]\)$';
$jsonp_collection_pattern = '^\/\*\*\/[_.a-zA-Z0-9]+\([\[].*[\]]\)$';
...
sprintf( '<\?xml|%s|%s|%s|%s', $json_object_pattern, $json_collection_pattern, $jsonp_object_pattern, $jsonp_collection_pattern ),

Or is there more under the hood that needs changing? You mentioned in an earlier comment "Ensuring the correct headers are sent, including for JSONP callbacks" is still to do. Is that something else that would cause JSONP requests not to cache currently?

@breams

breams Dec 1, 2016

What about JSONP responses? They won't match either pattern. Is it as simple as adding extra patterns?

$jsonp_object_pattern     = '^\/\*\*\/[_.a-zA-Z0-9]+\([{].*[}]\)$';
$jsonp_collection_pattern = '^\/\*\*\/[_.a-zA-Z0-9]+\([\[].*[\]]\)$';
...
sprintf( '<\?xml|%s|%s|%s|%s', $json_object_pattern, $json_collection_pattern, $jsonp_object_pattern, $jsonp_collection_pattern ),

Or is there more under the hood that needs changing? You mentioned in an earlier comment "Ensuring the correct headers are sent, including for JSONP callbacks" is still to do. Is that something else that would cause JSONP requests not to cache currently?

@oskarrough oskarrough referenced this pull request in oskarrough/ember-wordpress Aug 29, 2016

Closed

Document WP caching #24

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Sep 1, 2016

Member

I enabled this on WordCamp.org production so that we can get some real-world testing, and so far things look good. I'll keep an eye on it, though, and push commits for any bugs.

Member

iandunn commented Sep 1, 2016

I enabled this on WordCamp.org production so that we can get some real-world testing, and so far things look good. I'll keep an eye on it, though, and push commits for any bugs.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Sep 2, 2016

Member

I noticed a problem on production, but it turned out to be #97, so I'll send a PR for that.

If WP_ALLOW_MULTISITE doesn't exist, then legacy files are never served during phase 1, but they'll be generated in phase 2.

Member

iandunn commented Sep 2, 2016

I noticed a problem on production, but it turned out to be #97, so I'll send a PR for that.

If WP_ALLOW_MULTISITE doesn't exist, then legacy files are never served during phase 1, but they'll be generated in phase 2.

@axelson

This comment has been minimized.

Show comment
Hide comment
@axelson

axelson Dec 10, 2016

Sorry for the noise but I just want to say that I'm really looking forward to this feature. Especially since the latest version of wordpress includes the rest api by default.

axelson commented Dec 10, 2016

Sorry for the noise but I just want to say that I'm really looking forward to this feature. Especially since the latest version of wordpress includes the rest api by default.

svn2github pushed a commit to svn2github/wordcamp-mu-plugins that referenced this pull request Feb 8, 2017

iandunn
Remove REST API caching v1 in favor of v2.
This has been disabled for awhile, and will be replaced by the dedicated WP Super Cache plugin.

See Automattic/wp-super-cache#121

git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org/public_html/wp-content/mu-plugins@3770 74240141-8908-4e6f-9713-ba540dce6ec7
@donnchawp

This comment has been minimized.

Show comment
Hide comment
@donnchawp

donnchawp Feb 10, 2017

Contributor

I think legacy cache files will the best way of storing REST API calls, especially as now I've merged in code that stores those files in the supercache directories. See #177. They'll store header fields as well as page contents.

I think we'll be able to figure out a way to map garbage collection of the "main" supercache directory with the REST API directories that will be created. After all, we'll have post_ids of any posts.

I also added wpsc_delete_files and wpsc_rebuild_files functions to somewhat replace prune_super_cache in some situations so those could be entry points for doing the deleting of cached REST API calls.

Contributor

donnchawp commented Feb 10, 2017

I think legacy cache files will the best way of storing REST API calls, especially as now I've merged in code that stores those files in the supercache directories. See #177. They'll store header fields as well as page contents.

I think we'll be able to figure out a way to map garbage collection of the "main" supercache directory with the REST API directories that will be created. After all, we'll have post_ids of any posts.

I also added wpsc_delete_files and wpsc_rebuild_files functions to somewhat replace prune_super_cache in some situations so those could be entry points for doing the deleting of cached REST API calls.

@donnchawp

This comment has been minimized.

Show comment
Hide comment
@donnchawp

donnchawp Feb 10, 2017

Contributor

I think something like the gc_cache action could be used to delete the REST API cache files, but I might need to add a gc_cache_id action - that would pass a post_id rather than a path..

Contributor

donnchawp commented Feb 10, 2017

I think something like the gc_cache action could be used to delete the REST API cache files, but I might need to add a gc_cache_id action - that would pass a post_id rather than a path..

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Feb 15, 2017

Member

I rebased the PR against master to merge the latest updates. I also squashed some of the commits and force-pushed the new ones, so that they'll be cleaner when they're merged.

In hindsight, those were both probably bad ideas, since any comments on the original commits were lost, and anyone who has a local checkout will need to git fetch && git reset origin/cache-rest-api --hard, in order to pull down the new commits.

Member

iandunn commented Feb 15, 2017

I rebased the PR against master to merge the latest updates. I also squashed some of the commits and force-pushed the new ones, so that they'll be cleaner when they're merged.

In hindsight, those were both probably bad ideas, since any comments on the original commits were lost, and anyone who has a local checkout will need to git fetch && git reset origin/cache-rest-api --hard, in order to pull down the new commits.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Feb 15, 2017

Member

@donnchawp, unfortunately I don't think I'll have much more time to devote to this anytime soon, so what do you think about merging the current PR as v1, and then doing the changes in your comments in future iterations?

If it'd help, we could keep the plugin off by default, in order to get more testing before it's turned on by default.

Member

iandunn commented Feb 15, 2017

@donnchawp, unfortunately I don't think I'll have much more time to devote to this anytime soon, so what do you think about merging the current PR as v1, and then doing the changes in your comments in future iterations?

If it'd help, we could keep the plugin off by default, in order to get more testing before it's turned on by default.

@donnchawp

This comment has been minimized.

Show comment
Hide comment
@donnchawp

donnchawp Feb 21, 2017

Contributor

I'll test it when I have time. We'll need some sort of garbage collection in there but I don't think it'll be hard to do.

I agree, we should try and get a version 1 out there as soon as possible, with the changes to where legacy cache files are stored we could almost call it a version 2.0 :)

Contributor

donnchawp commented Feb 21, 2017

I'll test it when I have time. We'll need some sort of garbage collection in there but I don't think it'll be hard to do.

I agree, we should try and get a version 1 out there as soon as possible, with the changes to where legacy cache files are stored we could almost call it a version 2.0 :)

@DanReyLop DanReyLop referenced this pull request in Automattic/woocommerce-services Mar 10, 2017

Merged

Fix compatibility with WP Super Cache #925

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Jun 19, 2017

Member

I ran into a problem with this branch today, and it's already documented above, but I'd completely forgotten about it, so it's probably worth repeating.

Right now the HTTP headers aren't saved and served during cached responses, which can cause bugs in plugins. For example, here's an API response that returns a WP_Error

Cache miss:

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 500 Internal Server Error
Server: nginx
Date: Mon, 19 Jun 2017 22:15:22 GMT
Content-Type: application/json; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Cookie
X-Robots-Tag: noindex
Link: <https://wp.dev/wp-json/>; rel="https://api.w.org/"
X-Content-Type-Options: nosniff
Access-Control-Expose-Headers: X-WP-Total, X-WP-TotalPages
Access-Control-Allow-Headers: Authorization, Content-Type
Allow: GET

{"code":"errorunique","message":"this is a test","data":null}

Cache hit

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 200 OK
Server: nginx
Date: Mon, 19 Jun 2017 22:14:50 GMT
Content-Type: application/json
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Accept-Encoding, Cookie
Cache-Control: max-age=3, must-revalidate
WP-Super-Cache: Served supercache file from PHP

{"code":"errorunique","message":"this is a test","data":null}

If there's some code that's checking the status code of the response, it'll correctly get 500 for the cache miss, but 200 for the cache hit. During cache hits, it'll treat the WP_Error as if it were valid data, leading to all kinds of bugs.

I think this is something that'll need to be fixed before this branch can be merged. It sounds like @donnchawp already has plans for that.

Member

iandunn commented Jun 19, 2017

I ran into a problem with this branch today, and it's already documented above, but I'd completely forgotten about it, so it's probably worth repeating.

Right now the HTTP headers aren't saved and served during cached responses, which can cause bugs in plugins. For example, here's an API response that returns a WP_Error

Cache miss:

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 500 Internal Server Error
Server: nginx
Date: Mon, 19 Jun 2017 22:15:22 GMT
Content-Type: application/json; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Cookie
X-Robots-Tag: noindex
Link: <https://wp.dev/wp-json/>; rel="https://api.w.org/"
X-Content-Type-Options: nosniff
Access-Control-Expose-Headers: X-WP-Total, X-WP-TotalPages
Access-Control-Allow-Headers: Authorization, Content-Type
Allow: GET

{"code":"errorunique","message":"this is a test","data":null}

Cache hit

> curl -i https://wp.dev/wp-json/test/v1/foo
HTTP/1.1 200 OK
Server: nginx
Date: Mon, 19 Jun 2017 22:14:50 GMT
Content-Type: application/json
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/5.5.9-1ubuntu4.20
Vary: Accept-Encoding, Cookie
Cache-Control: max-age=3, must-revalidate
WP-Super-Cache: Served supercache file from PHP

{"code":"errorunique","message":"this is a test","data":null}

If there's some code that's checking the status code of the response, it'll correctly get 500 for the cache miss, but 200 for the cache hit. During cache hits, it'll treat the WP_Error as if it were valid data, leading to all kinds of bugs.

I think this is something that'll need to be fixed before this branch can be merged. It sounds like @donnchawp already has plans for that.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Jun 20, 2017

Member

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

For normal requests, Super Cache relies on WP's cookie auth, which can also be used with the REST API, but the API allows several additional methods -- e.g., OAuth, JSON web tokens, etc -- especially for server-to-server requests.

At first glance, it seems like checking for the Authorization header would cover all standards cases. I'm not 100% confident about that, though.

There'll still be situations where plugin authors pass tokens in arbitrary $_GET parameters, though. I'm not sure if we should make custom endpoints opt-in to avoid problems there, or just say they're doing it wrong. Maybe there's a better way that I haven't thought of, too.

To err on the side of caution, it might be good to also avoid caching requests that contain an X-WP-Nonce header, or $_REQUEST['_wpnonce']. That won't catch arbitrary parameters names, though.

cc @donnchawp

Member

iandunn commented Jun 20, 2017

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

For normal requests, Super Cache relies on WP's cookie auth, which can also be used with the REST API, but the API allows several additional methods -- e.g., OAuth, JSON web tokens, etc -- especially for server-to-server requests.

At first glance, it seems like checking for the Authorization header would cover all standards cases. I'm not 100% confident about that, though.

There'll still be situations where plugin authors pass tokens in arbitrary $_GET parameters, though. I'm not sure if we should make custom endpoints opt-in to avoid problems there, or just say they're doing it wrong. Maybe there's a better way that I haven't thought of, too.

To err on the side of caution, it might be good to also avoid caching requests that contain an X-WP-Nonce header, or $_REQUEST['_wpnonce']. That won't catch arbitrary parameters names, though.

cc @donnchawp

@beaulebens

This comment has been minimized.

Show comment
Hide comment
@beaulebens

beaulebens Jun 20, 2017

Member

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

Could we maybe checked for an Authorization header, and also for a current user (which would imply that some authorization happened, and if either are there, then don't cache?

Member

beaulebens commented Jun 20, 2017

Another blocker for merging that I just realized, is that we need to make sure that API requests that require some form of authentication are never cached.

Could we maybe checked for an Authorization header, and also for a current user (which would imply that some authorization happened, and if either are there, then don't cache?

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Jun 20, 2017

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

The Authorization header will cover a bunch of use cases, but OAuth 1 and 2 can both pass request parameters elsewhere (oauth_token and access_token respectively), so it's not a certainty.

rmccue commented Jun 20, 2017

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

The Authorization header will cover a bunch of use cases, but OAuth 1 and 2 can both pass request parameters elsewhere (oauth_token and access_token respectively), so it's not a certainty.

@iandunn

This comment has been minimized.

Show comment
Hide comment
@iandunn

iandunn Jun 20, 2017

Member

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

Are you thinking that it's enough to just check for a current user, rather than checking for both the current user AND the Authorization header? I don't think that's what you meant, but given the context I want to be extra careful that there isn't any miscommunication.

I think checking for Authorization (and other known parameters, like you mentioned) will still be required, because there will be plugins that rely only on the Authorization header, and don't touch the current user.

Member

iandunn commented Jun 20, 2017

If there's a current user by the time the response is generated, then some form of authentication has worked. Authentication plugins for the REST API don't do anything special, they just hook into the determine_current_user filter in core.

Are you thinking that it's enough to just check for a current user, rather than checking for both the current user AND the Authorization header? I don't think that's what you meant, but given the context I want to be extra careful that there isn't any miscommunication.

I think checking for Authorization (and other known parameters, like you mentioned) will still be required, because there will be plugins that rely only on the Authorization header, and don't touch the current user.

@donnchawp

This comment has been minimized.

Show comment
Hide comment
@donnchawp

donnchawp Oct 24, 2017

Contributor

As I said previously, I think we should use legacy cache files to store REST API calls. That may go some way towards taking care of authentication, but serving cache files to authenticated REST API clients might be problematic.

The plugin needs to support an authentication key that's plugged into the code that generates wp_cache_key. That code fires way before WordPress authentication happens however.

Contributor

donnchawp commented Oct 24, 2017

As I said previously, I think we should use legacy cache files to store REST API calls. That may go some way towards taking care of authentication, but serving cache files to authenticated REST API clients might be problematic.

The plugin needs to support an authentication key that's plugged into the code that generates wp_cache_key. That code fires way before WordPress authentication happens however.

@donnchawp

This comment has been minimized.

Show comment
Hide comment
@donnchawp

donnchawp Oct 24, 2017

Contributor

https://github.com/airesvsg/wp-rest-api-cache/blob/master/class-wp-rest-cache.php does something interesting. It uses rest_pre_dispatch to intercept REST API requests, and then "runs" the request if it's not cached, caches it in a transient option before serving it. It doesn't do any granular maintenance of cache files.
It uses $request_uri, $server and $request for the cache key which should take care of authentication..

Contributor

donnchawp commented Oct 24, 2017

https://github.com/airesvsg/wp-rest-api-cache/blob/master/class-wp-rest-cache.php does something interesting. It uses rest_pre_dispatch to intercept REST API requests, and then "runs" the request if it's not cached, caches it in a transient option before serving it. It doesn't do any granular maintenance of cache files.
It uses $request_uri, $server and $request for the cache key which should take care of authentication..

@jom jom modified the milestones: 1.4.9, vFuture Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment