Opened 5 years ago
Last modified 3 weeks ago
#48885 assigned enhancement
REST API: Support reading public settings, implement context handling
Reported by: | scruffian | Owned by: | 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)
Change History (47)
This ticket was mentioned in Slack in #core-editor by scruffian. View the logs.
5 years ago
#2
@
5 years ago
I added this Gutenberg PR to change over to this API: https://github.com/WordPress/gutenberg/pull/18970
#3
in reply to:
↑ description
@
5 years ago
I updated the patch to expose all "public" settings on the same route, for simplicity.
#4
@
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:
↓ 7
@
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.
- Introduce a
view
andedit
context. Makeedit
the default context. When the resource is requested in aview
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 aview
andedit
version of the response in the data store. - 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.
- 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
@
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
@
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.
- Introduce a
view
andedit
context. Makeedit
the default context. When the resource is requested in aview
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 aview
andedit
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.
- 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. } }
- 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.
#8
@
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
@
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:
↓ 11
@
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
@
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.
#12
@
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
@
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
#18
@
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
@
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
@
5 years ago
- Milestone changed from 5.4 to Future Release
Thanks for the confirmation @jorgefilipecosta!
#21
@
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
@
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
@
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
@
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
@
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.
- Allow for marking settings as
public
in some fashion, so that they are available in the settings endpoint to unauthorized users. - Add support for
context
to allow for settings with araw
andrendered
form ( or similar patterns ). - 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
@
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.
This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.
4 years ago
#35
@
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
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
@
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
@
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.
Patch for REST API