Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#31897 assigned enhancement

Update Customizer nonces via Heartbeat API — at Version 24

Reported by: westonruter Owned by: voldemortensen
Milestone: Future Release Priority: low
Severity: normal Version: 3.4
Component: Customize Keywords: needs-unit-tests needs-refresh has-patch
Focuses: javascript Cc:

Description (last modified by westonruter)

Currently the Customizer's nonces get updated when the preview gets refreshed (only the save and preview nonces, not the update-widget nonce, however). If the user leaves the window open in the background for a long time, they will get stale nonces. We should be using the Heartbeat API and the wp_ajax_customize_refresh_nonces filter introduced in #31294 to keep the nonces up date. (This is no longer true as of #35617.)

See also #31436 where Heartbeat integration will also be required to handle Customizer concurrency issues.

Change History (28)

#1 @jdgrimes
5 years ago

Related: #29312

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

5 years ago

#3 @iseulde
5 years ago

Related: #24447.

#4 @westonruter
5 years ago

  • Milestone changed from Future Release to 4.3
  • Owner set to westonruter
  • Status changed from new to assigned

I'll tackle this as part of #31436.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.

5 years ago

#6 @ocean90
5 years ago

  • Milestone changed from 4.3 to Future Release
  • Type changed from defect (bug) to enhancement

No movement in 4.3.

#7 @westonruter
5 years ago

  • Milestone changed from Future Release to 4.5
  • Owner westonruter deleted

#8 @joeyblake
5 years ago

  • Keywords has-patch added; needs-patch removed

Enqueues heartbeat and refreshes nonces on the standard heartbeat timing.

#9 @mikeschroder
5 years ago

  • Keywords needs-patch added; has-patch removed

Hi @joeyblake! Thanks for the patch.

Took a look with @azaozz -- this works, but it looks like it's only using Heartbeat as a timer, rather than using the Heartbeat API to transfer the nonce data. I'd suggest using that transport instead, since it will avoid extra requests.

As a second note, it looks like this is running every 60 seconds. Since a user shouldn't need nonces this often, you could change this so that it doesn't check as often.

#10 @joeyblake
5 years ago

Yep, I see. It also doesn't lend itself to using the heartbeat for other things like #31436 . I'll make those changes.

#12 @westonruter
4 years ago

  • Owner set to mattgeri

#13 @westonruter
4 years ago

  • Owner mattgeri deleted

Will be punted unless solid patch comes in the next several hours.

This ticket was mentioned in Slack in #core by jorbin. View the logs.

4 years ago

#15 @ocean90
4 years ago

  • Owner set to voldemortensen

#16 follow-up: @joeyblake
4 years ago

Sorry for the radio silence. Here is the issue that I've had with this ticket. I'm definitely missing something, and maybe someone else can see where.

My inclination when adding the filter to 'heartbeat_received' to refresh the nonces would be to have the logic and filter inside class-wp-customize-manager.php. However, when adding the filter there, it never gets executed.

If I were to move the filter out of there, to admin-filters.php it will execute, but admin-filters.php has no knowledge of WP_Customize_Manager, or the instantiated global at all. So I calling a method inside of there to retrieve the updated nonces is not possible.

I'm currently running adding data to the heartbeat-send event from the api.Previewer object in customize-controls.js

Is there anything obvious that I'm missing here?

Also, if timeframe is an issue here, hopefully this info helps someone else, I most likely won't be able to touch it today.

Last edited 4 years ago by joeyblake (previous) (diff)

#17 @voldemortensen
4 years ago

  • Milestone changed from 4.5 to Future Release

Punting because I wasn't able to get this done last night and beta is today.

#18 in reply to: ↑ 16 ; follow-up: @westonruter
4 years ago

Replying to joeyblake:

My inclination when adding the filter to 'heartbeat_received' to refresh the nonces would be to have the logic and filter inside class-wp-customize-manager.php. However, when adding the filter there, it never gets executed.

Good point. This is due to the heartbeat requests being made without the wp_customize=on POST param, and so the WP_Customize_Manager doesn't get bootstrapped. For the logic for how the class gets bootstrapped, see: https://core.trac.wordpress.org/browser/tags/4.4.2/src/wp-includes/theme.php?marks=1985-2007#L1985

Perhaps the Customizer should also get bootstrapped if the heartbeat_received includes wp_customize=on?

Actually, I just remembered this is something that @valendesigns and I have done in the Customize Concurrency feature plugin.

Here's the heartbeat_settings and heartbeat_received filters: https://github.com/xwp/wp-customize-concurrency/blob/dcaf1d421cc2d546ea0cdb9b97b60d8de9327a13/php/class-customize-concurrency.php#L221-L273

Notice the injection of the screenId which would not get set by default when loading heartbeat from the Customizer. And then notice how this screenId=customize is used as a signal to bootstrap the Customizer if not already instantiated in the heartbeat_received filter.

#19 in reply to: ↑ 18 @adamsilverstein
4 years ago

In 31897.diff

  • Move customizer nonce refresh to hearbeat api
  • Bootstrap customizer when heartbeat_received data contains wp-refresh-customizer-nonce data
  • Add and apply new nonces if wp_verify_nonce returns 2.

@westonruter I worked on this before reading your comment, open to suggestions for making the bootstrapping cleaner.

#20 follow-up: @westonruter
4 years ago

  • Keywords needs-unit-tests added

@adamsilverstein nice. The patch is looking good.

  • In wp_refresh_customizer_nonces() you can prevent instantiating WP_Customize_manager if the $wp_customize global already exists.
  • Add a current_user_can( 'customize' ) cap to the condition along with array_key_exists(). Otherwise, an unprivileged user could potentially obtain nonces.
  • Needs @param and @return phpdoc tags.
  • It would be useful for other plugins that make use of Heartbeat in the Customizer to have the $screen_id populated to be customize.
  • Maybe rename wp_refresh_customizer_nonces() to wp_heartbeat_refresh_customizer_nonces().
  • Maybe rename the heartbeat data array key from wp-refresh-customizer-nonce to wp-customize-nonces.

#21 in reply to: ↑ 20 @adamsilverstein
4 years ago

@westonruter Thanks for the additional feedback; I will work to address these issues.

In 31897.2.diff I took an alternate approach: I added support for sending wp_customize=on and moved the heartbeat php code back into class-wp-customize-manager.php. What do you think of this approach? It prevents having to manually bootstrap the customizer in the heartbeat callback.

#22 @adamsilverstein
4 years ago

@westonruter think I addressed all your points...

In 31897.3.diff:

  • Add current_user_can( 'customize' ) cap check
  • Add docblocks
  • Add a heartbeat_settings filter to set the $screen_id to customize
  • Some renaming as suggested

#23 @westonruter
4 years ago

@adamsilverstein I think you can remove all references to isCustomizer and instead just do:

if ( 'customize' === settings.screenId ) {
    ajaxData.wp_customize = 'on';

This also raises another question: should not the heartbeat sent to the server bring along with it the full context of the current Customizer state? In other words, shouldn't the customized JSON blob of dirty settings and the current theme be sent along in these heartbeat requests? This would ensure that when the Ajax request is received, all of the Customizer state will apply. Something will have to be done here to ensure that the preview nonce can be included alongside the heartbeat nonce.

#24 @westonruter
4 years ago

  • Description modified (diff)
  • Priority changed from normal to low

I just realized that all nonces now get updated when the preview refreshes as of #35617, making this ticket less important. This ticket now specifically fix the issue where the Customizer is left open for a long time without the preview being refreshed to keep the nonces up to date. Nevertheless, if the user does leave the browser session open for such a long time, it is also likely that their session will expire and they will need to re-login anyway: by default, non-remember user authentication sessions expire after 48 hours (auth_cookie_expiration) and nonces expire after 24 hours (nonce_life).

Nevertheless, the integration of Heartbeat into the Customizer will be useful for plugins generally, so I'd love to see that happen, and keeping nonces up-to-date should be the first application of Heartbeat in the Customizer. That can either be made the scope of this ticket, or another ticket can be made specific for that feature.

Note: See TracTickets for help on using tickets.