Prevent actorless Plugin Updated activity log entry #8488
Conversation
|
Lets add a test + more comments for this so that we can check that it works as expected. |
sync/class.jetpack-sync-sender.php
Outdated
| foreach ( Jetpack_Sync_Modules::get_modules() as $module ) { | ||
| $module->init_before_send(); | ||
| } | ||
| } | ||
|
|
||
| public function maybe_set_user_from_token() { | ||
| $jetpack = Jetpack::init(); | ||
| if ( $jetpack->verify_xml_rpc_signature() ) { |
There was a problem hiding this comment.
I think we should probably save the output of this call or only do it when we are under doing XMLRPC.
Just to avoid possible false positives.
There was a problem hiding this comment.
RE "only do it when we are under doing XMLRPC" - It doesn't look like there is any kind of is_xml_rpc() method. $jetpack->verify_xml_rpc_signature() appears to check whether we are in xml rpc. RE "we should probably save the output of this call" - the method also appears to memoize/cache the results, see https://github.com/Automattic/jetpack/blob/master/class.jetpack.php#L5249, so the body of the method should execute at most once per request. For these reasons, it looks like this is already only happening when we are under XMLRPC, and the output is already effectively saved.
There was a problem hiding this comment.
From looking at the function if things work out the value gets cached. If they don't then we can go though a lot of checks before returning false.
You can check against a constant XMLRPC_REQUEST to see if we are doing an xml-rpc request.
sync/class.jetpack-sync-sender.php
Outdated
| @@ -54,12 +54,16 @@ private function init() { | |||
|
|
|||
| public function maybe_set_user_from_token() { | |||
| $jetpack = Jetpack::init(); | |||
| if ( $jetpack->verify_xml_rpc_signature() ) { | |||
| if ( defined( 'XMLRPC_REQUEST' ) && | |||
There was a problem hiding this comment.
I think we could use the Jetpack_Constants::is_true( 'XMLRPC_REQUEST' ) instead so that we can write tests for that easier?
|
When it comes to testing. I think it would be good to do a test And then we could check that the request is coming in is has a valid user or not. |
|
@zinigor could you give us approval here |
zinigor
added
[Status] Ready to Merge
zinigor
removed
the
[Status] Ready to Merge
* Changelog 5.8: create base for changelog. * Update 5.8 release post link * fix 5.8 release date * Updates to plugin description * Changelog: add #8499 * Changelog: add #8506 * Changelog: add #8509 * Changelog: add #8516 * Changelog: add #8517 * Changelog: add #8523 * Changelog: add #8547 * Changelog: add #8496 * Changelog: add #8584 * Changelog: add #8595 * Changelog: add #8445 * Changelog: add #8431 * Changelog: add #8284 * Changelog: add #8270 * Changelog: add #8124 * Changelog: add #8581 * Changelog: add #8463 * Changelog: add #8568 (#8646) * Updates to testing list and changelog * Changelog: add #8443 * Changelog: add #8459 * Changelog: add #8469 * Changelog: add #8464 * Changelog: add #8478 and #8479 * Changelog: add #8483 * Changelog: add #8488 * Changelog: add #8513 * Changelog: add #8555 * Changelog: add #8565 * Changelog: add #8601 * Changelog: add #8612 * Changelog: add first pass at Search items. * Changelog: add more info to help test Search. * Changelog: add #8144 * Changelog: add #8313 * Changelog: add #8419 * Changelog: add #8465 * Changelog: add #8515 * Changelog: add #8587 * Changelog: add #8591 * Changelog: add #8659 * Changelog: add #8661 * Changelog: add #8671 * Changelog: add 5.7.1 to archived changelog too. * Reverted changes to readme, removed entry about backups.
This PR prevents the actorless Plugin Updated activity log entry.
Changes proposed in this Pull Request:
Previously, certain wpcom api requests to Jetpack sites resulted in the syncing of callables without a user set. This code ensures that the user is determined from the token and sent along in the Jetpack sync action.
Testing instructions:
Update a plugin via Calypso and ensure that you don't have an actorless Plugin Update activity in your activity log.
Proposed changelog entry for your changes: