WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 5 months ago

#48885 new enhancement

REST API: Add a route to read and update separate site settings

Reported by: scruffian Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: Cc:

Description

It would be good to make it possible to read and update individual site settings at

/wp/v2/settings/title

for example.

This is needed as part of https://github.com/WordPress/gutenberg/pull/18760

Attachments (6)

patch.diff (11.8 KB) - added by scruffian 7 months ago.
Patch for REST API
patch.2.diff (11.8 KB) - added by scruffian 7 months ago.
Fix the update method
patch.3.diff (11.8 KB) - added by scruffian 7 months ago.
Remove the hacks for testing
patch.4.diff (3.4 KB) - added by scruffian 7 months ago.
Patch for REST API
48885.diff (8.8 KB) - added by pento 7 months ago.
48885.2.patch (8.7 KB) - added by scruffian 7 months ago.
new patch with context

Download all attachments as: .zip

Change History (26)

@scruffian
7 months ago

Patch for REST API

This ticket was mentioned in Slack in #core-editor by scruffian. View the logs.


7 months ago

@scruffian
7 months ago

Fix the update method

@scruffian
7 months ago

Remove the hacks for testing

#2 @scruffian
7 months ago

I added this Gutenberg PR to change over to this API: https://github.com/WordPress/gutenberg/pull/18970

@scruffian
7 months ago

Patch for REST API

#3 in reply to: ↑ description @scruffian
7 months ago

I updated the patch to expose all "public" settings on the same route, for simplicity.

@pento
7 months ago

#4 @pento
7 months ago

  • Keywords has-patch has-unit-tests 2nd-opinion added
  • Milestone changed from Awaiting Review to 5.4

@scruffian: Nice work exploring this!

I'm pondering whether we really need to register a new route here: instead, the existing route can just return particular settings based on whether the current user has read access to all settings, or just those marked public.

48885.diff is an experiment along those lines, what do you think of this direction?

@TimothyBlynJacobs, do you have thoughts here? You mentioned concerns over on GB18760 with exposing read-only access to the entire settings API to non-admins, which I agree is a non-starter. Restricting which settings show to non-admins seems like it would cover this problem, however.

#5 follow-up: @TimothyBlynJacobs
7 months ago

Yeah this is tricky. Even partial access could be a security concern as I described in #48812. We don't provide a filter to modify the response in the settings controller itself. So if you want to do any modification, you'd have to do it in rest_request_after_callbacks. Something like this for instance.

<?php
add_filter( 'rest_request_after_callbacks', static function ( $response, $route, WP_REST_Request $request ) {
        if ( 0 !== strpos( $request->get_route(), '/wp/v2/settings' ) ) {
                return $response;
        }

        if ( is_wp_error( $response ) || $response->is_error() ) {
                return $response;
        }

        $response->data['do_some'] = 'privileged thing';

        return $response;
}, 10, 3 );

Returning a partial response when the user has no permissions would mean this previously valid mechanism of extending or modifying the response could now potentially reveal sensitive information.

I'm not sure how to evaluate how prevalent this pattern is, but I don't think it is particularly "weird" so to speak. I'm not seeing anything concerning on the plugins directory, no idea about custom code.

Making separate routes also doesn't necessarily solve this depending on how a developer is checking the route. strpos vs ===.


I think we also still need to figure out how to expose the "rendered" value of the site title to Gutenberg. That is where the bloginfo filter has been applied. I'm not sure if there's a great way of doing that on the existing settings route without breaking backwards compatibility. We couldn't just change the shape of the response to be a raw/rendered object like the title field. That would break anyone reading that field. So we would have to use something like context. Right now there is no context on the settings controller. A couple of possibilities.

  1. Introduce a view and edit context. Make edit the default context. When the resource is requested in a view context, change the value to the rendered version. This would work for public access, but doesn't solve the editing requirement. Gutenberg would need to maintain both a view and edit version of the response in the data store.
  2. Effectively "version" the endpoint based on a request parameter of some sort. If that request parameter is present, returns an object for the site title that includes the raw and rendered version. When returning the public version of the response, only return the rendered property.
  3. Introduce the single option endpoints and use the desired response format there. This would lead to inconsistencies between the collection and single item responses.

I'm not wild about any of these options. They all would introduce complexity into the security sensitive settings controller. To me, it doesn't seem like any more of a desirable solution than handling it in the client. It feels strange to expose settings to the public in a manner like this.

#6 @scruffian
7 months ago

It feels strange to expose settings to the public in a manner like this.

I dont think they would be exposed to the public, only to those who had edit_posts.

#7 in reply to: ↑ 5 @pento
7 months ago

Replying to TimothyBlynJacobs:

So if you want to do any modification, you'd have to do it in rest_request_after_callbacks.

Adding new data to this response seems like an unsupported edge case: if you want to add data, add a setting. An appropriate dev-note should be published, of course, to let people know it's happening.

  1. Introduce a view and edit context. Make edit the default context. When the resource is requested in a view context, change the value to the rendered version. This would work for public access, but doesn't solve the editing requirement. Gutenberg would need to maintain both a view and edit version of the response in the data store.

For Gutenberg to show an accurately rendered preview, and then to be able to edit the data, it probably needs to know how it looks in both the view and edit contexts. So, we need to send that data somehow.

  1. Effectively "version" the endpoint based on a request parameter of some sort. If that request parameter is present, returns an object for the site title that includes the raw and rendered version. When returning the public version of the response, only return the rendered property.

I'm fine with versioning. We haven't done that in Core yet, so there will probably need to be a bit of work to come up with a pattern, but it's not a super complex thing to do. Eg, a simple model would look something like this:

<?php
class WP_REST_Settings_v2_1_Controller extends WP_REST_Settings_Controller {
        public function __construct() {
                parent::__construct();
                $this->namespace = 'wp/v2.1';
        }

        public function register_routes() {
                // Register the same routes, with new namespace and read permisions.

        }

        protected function get_registered_options() {
                // Updated logic for getting registered options based on current user permissions.
        }
}
  1. Introduce the single option endpoints and use the desired response format there. This would lead to inconsistencies between the collection and single item responses.

This option feels like overkill, and I agree that the inconsistencies would be undesirable.

I'm not wild about any of these options. They all would introduce complexity into the security sensitive settings controller. To me, it doesn't seem like any more of a desirable solution than handling it in the client. It feels strange to expose settings to the public in a manner like this.

The problem is that we have a bunch of obviously public options (eg, blogname, blogdescription, siteurl, timezone_string) mixed in with a few obviously private options (eg, admin_email).

I don't think it's necessary to create an entirely new way of registering public options, just adding the public flag to the register_setting() call is much the same as when the show_in_rest flag was added.

Replying to scruffian:

I dont think they would be exposed to the public, only to those who had edit_posts.

I'd be inclined to expose these options publicly, rather than restricting to those with edit_posts. Otherwise, the public flag doesn't really make sense as it's named. Also, edit_posts is highly specific to the circumstances under which this REST API change is coming into being: it's reasonable to assume that an option marked as "public" should be readable by anyone.

Last edited 7 months ago by pento (previous) (diff)

#8 @TimothyBlynJacobs
7 months ago

Adding new data to this response seems like an unsupported edge case: if you want to add data, add a setting.

It would also be for modifying an option's representation. But I agree, and would classify it as #doingitwrong, but it makes me nervous that that it would lead to a potential information disclosure issue. Particularly since we haven't really said it is a misappropriation of that filter.

I'm fine with versioning. We haven't done that in Core yet, so there will probably need to be a bit of work to come up with a pattern, but it's not a super complex thing to do.

Yeah not technically difficult, but a lot of implications it would be interesting to explore.

The problem is that we have a bunch of obviously public options (eg, blogname, blogdescription, siteurl, timezone_string) mixed in with a few obviously private options (eg, admin_email).

These have been available or made available via the REST API index /wp-json/, WP_REST_Server::get_index().

The question seems to be whether this needs to be addressed in the client, or in the server. The complexity exists either way, I'm still not understanding why it is better to solve it in the server. These feel a lot like entirely separate contexts ( the logged in administrator, versus everyone else ) to me. But I'm just repeating myself at this point 😃. I'll let @kadamwhite and other REST API team members chime in. I may very well be making a mountain out of a mole hill.

#9 @pento
7 months ago

Historically speaking, the reason for these settings being exposed in / (as opposed to allowing non-admins read access to /wp/v2/settings) is that there wasn't an immediate need for unifying the way these settings were read, so the discussion we're having on this ticket now was punted. 🙂

I agree with @TimothyBlynJacobs's assessment that the primary question is whether the context switching should happen on the client side (read values from /, write values to /wp/v2/settings), or on the server side (alter /wp/v2/settings so all users can read public settings). I find the idea of reading and writing values to two different locations weird, though. As @scruffian mentioned in #48812, this is part of the first step towards full site editing in Gutenberg, which means now is a good time to make the API for that a bit more consistent than it currently is.

The question of how to get the rendered vs raw values is interesting. I don't think returning an object with rendered and raw properties is necessary, since non-admins probably shouldn't have access to the raw value in the first place, regardless of whether the settings is considered public or not. There are plenty of reasons why setting value might be filtered for non-admins before it's shown to them, let's not circumvent that.

In which case, we need to add view and edit context support to the endpoint. Non-admins can only GET the endpoint with view context. Admins will need to GET with view context for previewing the block, and with edit context for editing the block, and then can POST to update the setting value.

#10 follow-up: @TimothyBlynJacobs
7 months ago

Admins will need to GET with view context for previewing the block, and with edit context for editing the block, and then can POST to update the setting value.

At that point, is there a benefit to consolidating everything into a single endpoint? As I understand it, the purpose of centralizing it into one endpoint is to make it easy for core-data. In this model, the data store would need to maintain two copies of the resource that are fetched differently.

Switching how we format a value based on the context would be a new pattern I believe. I can't think of an example of us doing that in core, and couldn't find any doing a brief search. Currently you can request the "maximum" level of context available to you, and retrieve access to all data. I'm not opposed to it, but I wanted to document that.

#11 in reply to: ↑ 10 @pento
7 months ago

Replying to TimothyBlynJacobs:

At that point, is there a benefit to consolidating everything into a single endpoint?

Yup, that's the benefit: one endpoint, rather than two endpoints. This isn't specific to Gutenberg (which has WordPress experts working on it), this is about ensuring the data is available in the most logical location for any developer who wants to use this endpoint.

Switching how we format a value based on the context would be a new pattern I believe. I can't think of an example of us doing that in core, and couldn't find any doing a brief search. Currently you can request the "maximum" level of context available to you, and retrieve access to all data. I'm not opposed to it, but I wanted to document that.

It's not quite the same, but this is an example of a value changing based on the context. Given a post with a post password set on it, and the password is not sent in the GET request for that post, content.rendered on the returned post object will be empty in the view context, and populated correctly in the edit context.

@scruffian
7 months ago

new patch with context

#12 @scruffian
7 months ago

I added a new patch which also takes into account the context parameter.

This patch assumes that the name of settings in the rest api match the values of $show in get_bloginfo (https://developer.wordpress.org/reference/functions/get_bloginfo/) which they do in the cases we have made public so far, but won't for all options. Should we add a new key for this mapping?

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


7 months ago

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


6 months ago

This ticket was mentioned in Slack in #core-editor by scruffian. View the logs.


6 months ago

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


6 months ago

#17 @TimothyBlynJacobs
5 months ago

We discussed this in #core-restapi and decided to take another go at this as a Gutenberg PR. We're going to go with a public registration option tentatively combined with the WP_REST_Request context inspection to determine the response format. Adding support for public options alone does not solve GB's issue.

Slack reference: https://wordpress.slack.com/archives/C02RQC26G/p1579198597407600
PR: https://github.com/WordPress/gutenberg/pull/19967

Last edited 5 months ago by TimothyBlynJacobs (previous) (diff)

#18 @TimothyBlynJacobs
5 months ago

Chatting with @scruffian I think we can move this out of the 5.4 milestone since we should be able to make the changes Gutenberg needs in the plugin itself, and it is for Full Site Editing which I don't think is landing in WP 5.4? Pinging @jorgefilipecosta as the 5.4 GB tech lead for confirmation.

#19 @jorgefilipecosta
5 months ago

Hi @TimothyBlynJacobs, thank you for bringing this to my attention.
Your assessment seems correct. Full Site Editing is not part of WordPress 5.4, and it is technically possible to implement the required endpoint in the plugin.

#20 @TimothyBlynJacobs
5 months ago

  • Milestone changed from 5.4 to Future Release

Thanks for the confirmation @jorgefilipecosta!

Note: See TracTickets for help on using tickets.