WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 days ago

#31897 assigned enhancement

Update Customizer nonces via Heartbeat API

Reported by: westonruter Owned by:
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.

Attachments (6)

customizer-nonce-heartbeat.diff (1.1 KB) - added by joeyblake 16 months ago.
31897.diff (3.4 KB) - added by adamsilverstein 13 months ago.
31897.2.diff (3.1 KB) - added by adamsilverstein 13 months ago.
31897.3.diff (3.7 KB) - added by adamsilverstein 13 months ago.
31897.4.diff (3.6 KB) - added by adamsilverstein 13 months ago.
31897.5.diff (3.8 KB) - added by lukecavanagh 4 days ago.
Patch refresh

Download all attachments as: .zip

Change History (38)

#1 @jdgrimes
2 years ago

Related: #29312

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


2 years ago

#3 @iseulde
2 years ago

Related: #24447.

#4 @westonruter
23 months 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.


23 months ago

#6 @ocean90
21 months ago

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

No movement in 4.3.

#7 @westonruter
16 months ago

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

#8 @joeyblake
16 months ago

  • Keywords has-patch added; needs-patch removed

Enqueues heartbeat and refreshes nonces on the standard heartbeat timing.

#9 @mikeschroder
16 months 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
16 months 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
14 months ago

  • Owner set to mattgeri

#13 @westonruter
13 months 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.


13 months ago

#15 @ocean90
13 months ago

  • Owner set to voldemortensen

#16 follow-up: @joeyblake
13 months 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?

Edit:
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 13 months ago by joeyblake (previous) (diff)

#17 @voldemortensen
13 months 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
13 months 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
13 months 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
13 months 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
13 months 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
13 months 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
13 months 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 follow-up: @westonruter
13 months 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.

#25 in reply to: ↑ 24 ; follow-up: @adamsilverstein
13 months ago

Replying to westonruter:

I just realized that all nonces now get updated when the preview refreshes as of #35617,

Yea, I realized that working on the patch, I thought this was specifically to address the customizer being left open for a long period (with no refresh) and the nonce expiring.

I still think this is useful. A nonce could expire in as little as 12 hours - and leaving the customizer open overnight could easily expire the nonce; this patch would issue a new nonce lasting 24 hours in this case, and keep extending it every 12 hours as long as the heartbeat was running.

Doesn't the user get a warning to log in again before their session expires in the customizer?

#26 @adamsilverstein
13 months ago

31897.4.diff

  • Check 'customize' === settings.screenId and skip use of isCustomizer

#27 in reply to: ↑ 25 @westonruter
13 months ago

Replying to adamsilverstein:

Doesn't the user get a warning to log in again before their session expires in the customizer?

Once their session expires, the user gets prompted to re-login. But the nonces generally have a shorter lifespan than the user sessions, so this would ensure the nonces remain valid for the entire session, unless… if they sleep their computer with the Customizer open for a day, and then wake their computer: the nonces will have expired, including the nonce for Heartbeat itself. So in this case, it seems the only solution would be for the user to reload the Customizer, or I suppose a prompt to re-login would have the same effect.

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


5 days ago

#29 @westonruter
5 days ago

  • Keywords needs-refresh added
  • Owner voldemortensen deleted

Patch needs refresh. It doesn't apply cleanly anymore.

#30 follow-up: @lukecavanagh
5 days ago

@westonruter 

Would be happy to work on a patch refresh.

#31 in reply to: ↑ 30 @adamsilverstein
5 days ago

  • Keywords has-patch added; needs-patch removed

Replying to lukecavanagh:

Would be happy to work on a patch refresh.

Thanks, @lukecavanagh! The patch should still work once we fix the failure applying it. @westonruter any ideas for an approach to a unit test, where it would go with the current tests?

#32 @westonruter
4 days ago

For QUnit tests, I think it would involve doing jQuery( document ).trigger( 'heartbeat-tick.wp-refresh-nonces', [ { 'wp-customize-nonces':{…} } ] ) and making sure that this results in nonce-refresh being triggered on wp.customize.

Likewise, when doing jQuery( document ).trigger( 'heartbeat-send.wp-refresh-nonces', [ { 'wp-customize-nonces':{…} } ] ) that the current wp.customize.settings.nonce gets populated into the heartbeat data being passed in.

The QUnit tests would go into tests/qunit/wp-admin/js/customize-controls.js, I think.

For PHPUnit, similarly, we'd want to apply filters on heartbeat_received and heartbeat_settings to make sure the supplied arrays get the expected values amended onto them via wp_heartbeat_refresh_customizer_nonces() and wp_heartbeat_settings_customizer_filter() respectively. This would go into tests/phpunit/tests/customize/manager.php

@lukecavanagh
4 days ago

Patch refresh

Note: See TracTickets for help on using tickets.