Make WordPress Core

Opened 5 years ago

Last modified 3 weeks ago

#48885 assigned enhancement

REST API: Support reading public settings, implement context handling

Reported by: scruffian's profile scruffian Owned by: spacedmonkey's profile spacedmonkey
Milestone: Future Release Priority: normal
Severity: normal Version: 3.7
Component: REST API Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: rest-api 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 5 years ago.
Patch for REST API
patch.2.diff (11.8 KB) - added by scruffian 5 years ago.
Fix the update method
patch.3.diff (11.8 KB) - added by scruffian 5 years ago.
Remove the hacks for testing
patch.4.diff (3.4 KB) - added by scruffian 5 years ago.
Patch for REST API
48885.diff (8.8 KB) - added by pento 5 years ago.
48885.2.patch (8.7 KB) - added by scruffian 5 years ago.
new patch with context

Download all attachments as: .zip

Change History (47)

@scruffian
5 years ago

Patch for REST API

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


5 years ago

@scruffian
5 years ago

Fix the update method

@scruffian
5 years ago

Remove the hacks for testing

#2 @scruffian
5 years ago

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

@scruffian
5 years ago

Patch for REST API

#3 in reply to: ↑ description @scruffian
5 years ago

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

@pento
5 years ago

#4 @pento
5 years 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
5 years 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
5 years 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
5 years 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 5 years ago by pento (previous) (diff)

#8 @TimothyBlynJacobs
5 years 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
5 years 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
5 years 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
5 years 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
5 years ago

new patch with context

#12 @scruffian
5 years 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.


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#17 @TimothyBlynJacobs
5 years 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 years ago by TimothyBlynJacobs (previous) (diff)

#18 @TimothyBlynJacobs
5 years 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 years 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 years ago

  • Milestone changed from 5.4 to Future Release

Thanks for the confirmation @jorgefilipecosta!

#21 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Future Release to 5.6
  • Owner set to spacedmonkey
  • Status changed from new to assigned

@spacedmonkey expressed an interest in working on this for 5.6. I think our tentative plan is to add support for a permission_callback and prepare_callback to register_setting.

#22 @TimothyBlynJacobs
4 years ago

Something to keep in mind with permission callbacks is that reading permission would be different than writing permission. And do we want to support a custom write permission callback? And if we do, do we need to update options.php to use that capability instead of manage_options.

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


4 years ago

This ticket was mentioned in PR #534 on WordPress/wordpress-develop by spacedmonkey.


4 years ago
#24

Working on #48885. Decided to go a different direction than the ticket name. Use the existing endpoint but add some new callbacks.

To use this PR, here is the example I use to test.

add_action('init', function(){
	register_setting(
		'general',
		'wibble',
		array(
			'show_in_rest' => array(
				'name' => 'wibble',
				'edit_permission_callback' => '__return_true',
				'read_permission_callback' => '__return_true',
			),
			'type'         => 'string',
			'description'  => __( 'Site title.' ),
		)
	);
});

This make a setting called wibble, that will now be public to read and edit. You never do this, but it gives you an idea of how the code works.

Trac ticket: https://core.trac.wordpress.org/ticket/48885

spacedmonkey commented on PR #534:


4 years ago
#25

@TimothyBJacobs This PR is a work in progress, but gives you an idea of what i am thinking. If you are happy with the way I did this, I will add tests.

TimothyBJacobs commented on PR #534:


4 years ago
#26

This path looks good to me!

We should make sure that if the user passed an invalid callback the option isn't disclosed.

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


4 years ago

#28 @spacedmonkey
4 years ago

Something to keep in mind with permission callbacks is that reading permission would be different than writing permission. And do we want to support a custom write permission callback? And if we do, do we need to update options.php to use that capability instead of manage_options.

As discussed in office hours, I do not believe we should make permission callbacks apply to other parts of core like options.php. Read and edit callbacks, should be limited to the REST API. The name of the ticket and original purpose are clear. To make values from wp_options available to read and update via the REST API. To add a whole permission model to register_settings, is massively out of scope for this ticket and introduces edge cases issues we simple do not need to tackle to complete this ticket's goal.

I would love @johnjamesjacoby thoughts on the matter, as he is the maintainer of options and meta.

#29 @spacedmonkey
4 years ago

I also think that changing the scope of this ticket, this late into the 5.6 cycle, would mean we would miss the 5.6 beta 1 cut off in just under two weeks. I think we should scope the change to the smallest we can, an MVP if you will and iterate in other releases.

Thoughts

#30 @TimothyBlynJacobs
4 years ago

  • Summary changed from REST API: Add a route to read and update separate site settings to REST API: Support reading public settings, implement context handling

Based off @johnjamesjacoby's feedback, I wanted to restate what the direct needs are from the Gutenberg team out of this ticket that have gotten lost thru the various Slack and GitHub conversations.

Gutenberg makes use of a number of settings in Full Site Editing such as the website's title, posts per page, etc... This data is typically things that are or would be exposed via the REST API index at wp-json. Additionally, some of these values have a different "rendered" form and "raw" form like the site title and description.

Right now as I understand it Gutenberg either reads these values from the index, or bootstraps them in the JS settings. When interacting with them in an editable fashion, Gutenberg then uses the settings endpoint.

Architecturally, things would be simpler if Gutenberg could use the same API endpoint for both cases. To accomplish this, there are three changes we must make.

  1. Allow for marking settings as public in some fashion, so that they are available in the settings endpoint to unauthorized users.
  2. Add support for context to allow for settings with a raw and rendered form ( or similar patterns ).
  3. Introduce a prepare_callback to allow for settings to customize their output.

We also need to make sure that the title and description settings that currently are exposed can change to outputting a raw and rendered object while maintaining backward compatibility. This could be done by only using that format when the context value is explicitly defined, or we'll have to bite the bullet and version the endpoint.

This doesn't mean it has to be done using a public flag. Allowing for callbacks to implement more specific handling makes sense to me, but it isn't mandatory. As stated in my earlier comment, my worry about implementing an edit_permission_callback is that we need to make sure it isn't bypassable via options.php. This could be done by checking for the callback specifically before the individual update_option calls, or we could introduce a manage_option meta capability perhaps.

I think if we don't take the permission callback into account at all, it will be misleading to developers who think they can use it to safely secure an option.


Based on the additional work required before Beta 1 in two weeks, I think we should move this to 5.7. Full Site Editing isn't shipping in 5.6, so we'll have more time to give this the refinement it needs.

#31 @hellofromTonya
4 years ago

  • Milestone changed from 5.6 to Future Release

Per Timothy's comment above, punting this ticket to Future Release to allow more soak and refinement time.

Based on the additional work required before Beta 1 in two weeks, I think we should move this to 5.7. Full Site Editing isn't shipping in 5.6, so we'll have more time to give this the refinement it needs.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#32 @TimothyBlynJacobs
4 years ago

#38731 was marked as a duplicate.

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


4 years ago

#35 @spacedmonkey
3 years ago

  • Focuses rest-api added
  • Version set to 3.7

@TimothyBlynJacobs I have updated #534 with unit tests. Can you review again?

I think there is a good chance we could get this in WordPress 5.9.

One question is, should we make some of the none sensitive settings public. Things you can easily workout.

Examples could be public

  • name
  • description
  • url
  • home
  • gmt_offset
  • timezone_string
  • language
  • posts_per_page

Example that could behind edit_posts capabilty

  • default_category
  • default_post_format

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


3 years ago

adamziel commented on PR #534:


2 years ago
#37

The tests are all failing unfortunately :(

This ticket was mentioned in PR #2985 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#38

Working on #48885. Decided to go a different direction than the ticket name. Use the existing endpoint but add some new callbacks.

To use this PR, here is the example I use to test.

add_action('init', function(){
	register_setting(
		'general',
		'wibble',
		array(
			'show_in_rest' => array(
				'name' => 'wibble',
				'edit_permission_callback' => '__return_true',
				'read_permission_callback' => '__return_true',
			),
			'type'         => 'string',
			'description'  => __( 'Site title.' ),
		)
	);
});

This make a setting called wibble, that will now be public to read and edit. You never do this, but it gives you an idea of how the code works.

Trac ticket: https://core.trac.wordpress.org/ticket/48885

#39 @youknowriad
5 weeks ago

Hello folks!

I came here to say that a need for resolution for this ticket is becoming more urgent day by day. Especially as we want to decouple the frontend from the backend using REST API.

The immediate need for us is a solution for this PR https://github.com/WordPress/gutenberg/pull/66459
We want to move defaultTemplates and defaultTemplateParts settings to an endpoint.

Note that these two are a bit specific compared to the settings mentioned above:

  • They are not editable
  • They are filterable in php
  • They are not wp_options settings.
  • They are also not editor specific.
  • They are also not sensitive data.

So what we need is a clear path forward:

  • Should we continue adding this kind of settings to the / endpoint?
  • Should we work on allowing "readonly" settings in the settings endpoint, even if they are no wp_options settings?
  • Should this be a completely new endpoint: why? and what makes a setting belong to one endpoint but not the other.

I think we need to stop delaying a solution here and come to an agreement on the path forward. Let's discuss this in the next days as we want to move forward and avoid bike shedding too much here.

#40 @spacedmonkey
3 weeks ago

I completely agree @youknowriad.

However, after creating a PR two years ago, I am still awaiting feedback. Sadly, I think this change needs to handle in core first and then get backported, as this means changing how the register_settings API works.

Pinging @TimothyBlynJacobs @antonvlasenko @kadamwhite in the hopes we can get this one into the next release.

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


3 weeks ago

Note: See TracTickets for help on using tickets.