Add show show_in_rest param to register_sidebar by spacedmonkey · Pull Request #1578 · WordPress/wordpress-develop

spacedmonkey · 2021-08-12T19:52:22Z

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

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

TimothyBJacobs · 2021-08-12T20:24:52Z

Won't we need a change in the widgets controller to check if a widget is part of a public sidebar?

spacedmonkey · 2021-08-12T22:29:12Z

@TimothyBJacobs Updated the PR with your feedback.

One question. Should we / can we change raw instance values to only display in edit context.

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php

Lines 783 to 787 in eb6b517

    
           'raw'     => array( 
        
           	'description' => __( 'Unencoded instance settings, if supported.' ), 
        
           	'type'        => 'object', 
        
           	'context'     => array( 'view', 'edit', 'embed' ), 
        
           ),

I am wonder about sensitive data leaking?

TimothyBJacobs · 2021-08-15T21:03:04Z

Thanks. Looks like the test failure may be legit.

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

I think sensitive data leakage is definitely something we should be concerned about. While a theme author may opt-in to having their sidebar available, they might not know that a widget's setting contains an API key for instance.

However, I think there is definitely value in having the instance values available, not just the rendered widget, to allow for rendering the widget yourself.

I think for now we could omit the instance entirely, but it may make sense in the future to allow for show_instance_in_rest to be overloaded to a configuration array similarly to register_meta to allow for a widget author to selectively expose certain fields.

spacedmonkey · 2021-08-23T10:30:23Z

Seems like the issue here was running retrieve_widgets twice, once in permission check and once in *_items function. I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better. Thoughts @TimothyBJacobs

TimothyBJacobs · 2021-08-24T21:28:34Z

Pinging @adamziel as he's been looking into retrieve_widgets issues.

spacedmonkey · 2021-08-27T19:00:52Z

We would need to change the whole instance value to be only be exposed in an edit context, since the encoded form is just encoded, not hidden.

So should only instance be changed to 'context' => array( 'edit' ), or instance, instance.encoded, instance.hash and instance.raw be changed?

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php

Lines 767 to 789 in 52fe9ff

    
           'instance'      => array( 
        
           	'description' => __( 'Instance settings of the widget, if supported.' ), 
        
           	'type'        => 'object', 
        
           	'context'     => array( 'view', 'edit', 'embed' ), 
        
           	'default'     => null, 
        
           	'properties'  => array( 
        
           		'encoded' => array( 
        
           			'description' => __( 'Base64 encoded representation of the instance settings.' ), 
        
           			'type'        => 'string', 
        
           			'context'     => array( 'view', 'edit', 'embed' ), 
        
           		), 
        
           		'hash'    => array( 
        
           			'description' => __( 'Cryptographic hash of the instance settings.' ), 
        
           			'type'        => 'string', 
        
           			'context'     => array( 'view', 'edit', 'embed' ), 
        
           		), 
        
           		'raw'     => array( 
        
           			'description' => __( 'Unencoded instance settings, if supported.' ), 
        
           			'type'        => 'object', 
        
           			'context'     => array( 'view', 'edit', 'embed' ), 
        
           		), 
        
           	), 
        
           ),

adamziel · 2021-08-30T11:43:39Z

I added a workaround for this, with some state is seems to work. I would love some feedback on managing state like this better.

@spacedmonkey I think the mental model of retrieve_widgets is broken right now. Even the name is confusing: That function doesn't just retrieve stuff, it actually updates the database. It's like "switching themes breaks the stored data, so we need to fix the data before each usage". I would like to make it more of "we never break the data in the first place" but that's a larger project. For now I just proposed a more accurate name and it has commit label so you might want to update this PR once that change is merged.

Since your issue seems to come from running retrieve_widgets twice and we've seen similar problems earlier, I wonder if this fix might help you.

Other than that, I don't think that we have a good way around multiple function calls other than what you proposed. Well, maybe aside of a hook that would run when a related REST route is matched – that way we could have only a single call that would cover both the permissions check and the actual handler.

spacedmonkey · 2021-09-02T12:27:18Z

@adamziel Do you think that #1525 blocks this PR? Would it effect anything in your PR? Did you review this PR?

adamziel · 2021-09-02T14:04:37Z

l Do you think that #1525 blocks this PR?

@spacedmonkey I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

		if ( $this->widgets_retrieved ) {
			return;
		}

But I'm not 100% certain about that since I don't have a good grasp of what the problem was.

Would it effect anything in your PR?

I don't this PR would affect #1525 much – a rebase, if necessary, should be pretty easy. Feel free to move forward with this one once you have approvals.

Did you review this PR?

I read the code and it looks good, but I did not test much. I'm happy to help with testing once you confirm the code is where you want it to be, sync_registered_widgets or not.

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with WordPress/gutenberg#34230, it may or may not make sense here as well

spacedmonkey · 2021-09-02T15:14:26Z

Also, one more question – should this PR be moved to the Gutenberg repo so that 5.7+plugin may benefit from these changes? That's what we did with WordPress/gutenberg#34230, it may or may not make sense here as well

Yeah, it can be backported.

spacedmonkey · 2021-09-02T15:15:49Z

I wouldn't say it blocks, no. Maybe it would allow you to get rid of this check:

So we should wait until your work is merged before this goes in?

adamziel · 2021-09-03T13:45:44Z

If it takes a few days then it would be convenient – potentially one check less in your PR. If it takes more, it's probably not worth it – we could re-evaluate that check at some later time.

Maybe just rebasing this PR on top of #1525 would do the trick? I think the consensus is there and we were just adjusting code standards. Also, if #1525 does not solve the "if retrieve_widgets already called" situation, feel free to just proceed without looking back.

adamziel · 2021-09-03T13:52:45Z

Yeah, it can be backported.

Why not Gutenberg-first? These "double" changes have been challenging lately, see the discussion in WordPress/gutenberg#33810 and WordPress/gutenberg#34536.

The sidebars endpoint is getting removed from the next Gutenberg version, so no problem there. It's all about having a consistent widgets endpoint now.

spacedmonkey · 2021-09-04T15:08:53Z

Why not Gutenberg-first? These "double" changes have been challenging lately

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

adamziel · 2021-09-06T10:17:13Z

This change doesn't make sense to backport. It has little use in the gutenberg plugin and it an opt in feature for devs. It is most useful for headless applications of WordPress.

I see, thank you for explaining! I am only concerned about merging changes later on. If we keep updating the same endpoints both in core and in the Gutenberg repo, we may end up with conflicting changes. On the other hand, I think I discussed that with @gziolo in WordPress/gutenberg#33810 and the conclusion was that in practice this rarely becomes a problem so maybe we're good here?

spacedmonkey · 2021-09-14T00:57:59Z

this rarely becomes a problem so maybe we're good here?

If there was any value in adding this to the gutenberg plugin, I would have done it both places. I will do that future PRs.

spacedmonkey · 2021-09-27T09:20:23Z

Unit tests are passing!

adamziel · 2021-09-30T14:57:46Z

 		$this->assertErrorResponse( 'rest_cannot_manage_widgets', $response, 401 );
 	}

+	public function test_get_items_no_permission_public() {


I'm confused because the test name says "no permission public", but then it sets up a sidebar with two widgets and the endpoint returns both. I'd add a few test cases where the API returns a limited list of widgets or a 403 error.

adamziel · 2021-09-30T14:58:48Z

+	/**
+	 * @ticket 41683
+	 */
+	public function test_get_items_no_permission_public() {


A few additional test cases would go a long way, see my other comment

adamziel · 2021-10-26T12:28:40Z

+	 * @param array $sidebar Sidebar.
+	 * @return bool Whether the side can be read.
+	 */
+	public function check_read_permission( $sidebar ) {


How about this below?

Suggested change

public function check_read_permission( $sidebar ) {

public function can_show_sidebar_in_rest( $sidebar ) {

check_read_permission is called quite similarly to get_item_permissions_check and do_permissions_check, I got a bit confused after returning to this PR.

This naming follows existing core methods. See

wordpress-develop/src/wp-includes/rest-api/endpoints/class-wp-rest-blocks-controller.php

Lines 30 to 37 in 2382765

public function check_read_permission( $post ) {

// By default the read_post capability is mapped to edit_posts.

if ( ! current_user_can( 'read_post', $post->ID ) ) {

return false;

}

return parent::check_read_permission( $post );

}

adamziel · 2021-10-26T12:30:31Z

 				continue;
 			}

+			if ( is_wp_error( $permissions_check ) && ! $this->check_read_permission( $sidebar ) ) {


show_in_rest is only checked for users without edit_theme_options capability. Is this intended? Intuitively, I would expect that setting an option named show_in_rest to false will remove my sidebar from any and all REST API responses.

I guess this is namely wrong. show_in_rest only means show publically in rest. It will always be in the rest api.

If we remove this, then it the widget editor will not work.

adamziel · 2021-10-26T12:37:42Z

 				continue;
 			}

+			if ( is_wp_error( $permissions_check ) && ! $this->check_read_sidebar_permission( $sidebar_id ) ) {


I have the same question as above – is it intended that show_in_rest is not effective for users with the right permissions?

adamziel · 2021-10-26T12:38:04Z


-		$prepared = array();
+		$prepared          = array();
+		$permissions_check = $this->permissions_check( $request );


Suggested change

$permissions_check = $this->permissions_check( $request );

$user_has_permissions = $this->permissions_check( $request );

adamziel · 2021-10-26T12:49:59Z

PHPUnit failures seem to be just flaky CI. I think this PR is pretty close 👍 I left some last notes and proposed a few tests.

adamziel

LGTM 👍

TimothyBJacobs · 2021-11-05T02:16:21Z

Merged in https://core.trac.wordpress.org/changeset/52016.

Add show_in_rest param to register_sidebar

d32ec88

TimothyBJacobs reviewed Aug 12, 2021

View reviewed changes

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php Outdated

TimothyBJacobs reviewed Aug 12, 2021

View reviewed changes

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php

spacedmonkey added 2 commits August 12, 2021 23:21

Also, add check for show_in_rest in the widget controller.

cfdc043

Cleaner logic.

33fd18b

Fix tests.

5da16b9

Merge branch 'master' into try/public-sidebars

482e18e

spacedmonkey mentioned this pull request Sep 2, 2021

Refresh sidebars widgets global after calling wp_set_sidebars_widgets #1525

Open

spacedmonkey added 2 commits September 14, 2021 01:49

Merge branch 'master' into try/public-sidebars

386fed0

Fix renaming.

2012e07

spacedmonkey added 4 commits September 14, 2021 02:02

Context is edit

e4291b9

Fix tests

0f6552e

Fix tests

b88a764

Fix fixture data

2511298

spacedmonkey requested a review from TimothyBJacobs September 27, 2021 09:16

adamziel reviewed Sep 30, 2021

View reviewed changes

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-widgets-controller.php Outdated

adamziel reviewed Sep 30, 2021

View reviewed changes

Comment thread src/wp-includes/rest-api/endpoints/class-wp-rest-sidebars-controller.php Outdated

adamziel reviewed Sep 30, 2021

View reviewed changes

Address feedback

77635dc

adamziel reviewed Oct 26, 2021

View reviewed changes

Comment thread tests/phpunit/tests/rest-api/rest-sidebars-controller.php

adamziel reviewed Oct 26, 2021

View reviewed changes

Comment thread tests/phpunit/tests/rest-api/rest-widgets-controller.php

spacedmonkey added 2 commits November 1, 2021 09:50

Merge branch 'master' into try/public-sidebars

f426951

Add some tests

87eef4a

spacedmonkey requested a review from adamziel November 2, 2021 09:55

adamziel approved these changes Nov 2, 2021

View reviewed changes

TimothyBJacobs added 2 commits November 4, 2021 21:24

Minor cleanup.

da9db3c

Some more doc block changes.

d67d56e

TimothyBJacobs closed this Nov 5, 2021

	public function check_read_permission( $sidebar ) {
	public function can_show_sidebar_in_rest( $sidebar ) {

	public function check_read_permission( $post ) {
	// By default the read_post capability is mapped to edit_posts.
	if ( ! current_user_can( 'read_post', $post->ID ) ) {
	return false;
	}

	return parent::check_read_permission( $post );
	}

	$permissions_check = $this->permissions_check( $request );
	$user_has_permissions = $this->permissions_check( $request );

Conversation

spacedmonkey commented Aug 12, 2021

Uh oh!

Uh oh!

Uh oh!

TimothyBJacobs commented Aug 12, 2021

Uh oh!

spacedmonkey commented Aug 12, 2021

Uh oh!

TimothyBJacobs commented Aug 15, 2021

Uh oh!

spacedmonkey commented Aug 23, 2021

Uh oh!

TimothyBJacobs commented Aug 24, 2021

Uh oh!

spacedmonkey commented Aug 27, 2021

Uh oh!

adamziel commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacedmonkey commented Sep 2, 2021

Uh oh!

adamziel commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacedmonkey commented Sep 2, 2021

Uh oh!

spacedmonkey commented Sep 2, 2021

Uh oh!

adamziel commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamziel commented Sep 3, 2021

Uh oh!

spacedmonkey commented Sep 4, 2021

Uh oh!

adamziel commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacedmonkey commented Sep 14, 2021

Uh oh!

spacedmonkey commented Sep 27, 2021

Uh oh!

Uh oh!

Uh oh!

adamziel Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

adamziel Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamziel Oct 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacedmonkey Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

adamziel Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

spacedmonkey Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamziel Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

adamziel Oct 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamziel commented Oct 26, 2021

Uh oh!

adamziel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyBJacobs commented Nov 5, 2021

adamziel commented Aug 30, 2021 •

edited

Loading

adamziel commented Sep 2, 2021 •

edited

Loading

adamziel commented Sep 3, 2021 •

edited

Loading

adamziel commented Sep 6, 2021 •

edited

Loading

adamziel Sep 30, 2021 •

edited

Loading

adamziel Oct 26, 2021 •

edited

Loading

adamziel left a comment •

edited

Loading