Media: Remove unnecessary XMLRPC/XHR checks from handlers

[tyxla]

As suggested by @brbrr via Slack and documented via https://github.com/Automattic/jetpack/pull/13253/files#r315117316 we are hooking the remote file upload handlers with arbitrary checks that could be removed. This PR removes them.

Question: should we keep those for some reason? They were reintroduced in #13253 because they were removed in #12699, but it likely makes sense to clean them up now?

Changes proposed in this Pull Request:

• Media: Remove unnecessary XMLRPC/XHR checks from handlers

Is this a new feature or does it add/remove features to an existing part of Jetpack?

• A continuation of Connection: Move remote file upload handlers back to Jetpack class #13253

Testing instructions:

• Checkout this branch on a connected Jetpack site.
• Try uploading a file via Calypso.

Proposed changelog entry for your changes:

• Media: Remove unnecessary XMLRPC/XHR checks from handlers

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against 3b7bb5c

[mdawaffe]

I'm flagging this as "Request changes", but really it's I'm requesting changes to #13253 :) That's where these two methods' calls were split into two different places.

[mdawaffe]

• ->add_remote_request_handlers() doesn't need to be wrapped in the conditional: the AJAX handler will only be triggered if these conditions are met anaway, but
• ->require_jetpack_authentication() must be wrapped in the conditional: we only want to require Jetpack auth for requests that trigger these AJAX handlers, but by the time those AJAX handlers are triggered, we already need to have required Jetpack auth. So we require Jetpack auth earlier (here).

The relationship between the two was clearer before (if it looks like the these AJAX requests, require Jetpack auth and set up the AJAX handlers):

jetpack/class.jetpack.php

Lines 599 to 607 in 90c52c5

	} elseif (
	is_admin() &&
	isset( $_POST['action'] ) && (
	'jetpack_upload_file' == $_POST['action'] ||
	'jetpack_update_file' == $_POST['action']
	)
	) {
	$this->require_jetpack_authentication();
	$this->add_remote_request_handlers();

(or

jetpack/packages/connection/src/Manager.php

Lines 61 to 73 in ca302c4

	if (
	! $is_jetpack_xmlrpc_request
	&& is_admin()
	&& isset( $_POST['action'] ) // phpcs:ignore WordPress.Security.NonceVerification
	&& (
	'jetpack_upload_file' === $_POST['action'] // phpcs:ignore WordPress.Security.NonceVerification
	|| 'jetpack_update_file' === $_POST['action'] // phpcs:ignore WordPress.Security.NonceVerification
	)
	) {
	$this->require_jetpack_authentication();
	$this->add_remote_request_handlers();
	return;
	}

in Manager)

The relationship between the two is now essentially impossible to see: In the Manager, we require Jetpack auth if it looks like the correct AJAX request, and here in class.jetpack.php, we add the AJAX handler.

[tyxla]

Thanks for the feedback @mdawaffe!

What I'm not sure about here is: is that relationship so important so we have to keep these lines together? Let me try to clarify my point with more details.

The first line handles Jetpack auth. The other one handles registering handlers for remote file uploads. Although the former is required for the latter, I don't see why they should live together. After all, having a valid connection is required for sync, but we don't strive to put connection bits everywhere in sync, and code is decoupled as I'd expect. We can think of a thousand other cases where pieces of functionality rely on each other, but they don't necessarily live together in terms of code, and that's perfectly fine, and recommended for better separation of concerns.

To summarize this, I don't see why we have to keep those 2 lines together. We can always add a couple of @see comments to illustrate a relationship between them, and IMHO that would make it clear enough that these work together. I've done that in 3b7bb5c.

So, I realize why the checks in #13256 were unnecessary, but I think this PR, in context of what I just explained above, solves the problems, because:

• ->add_remote_request_handlers() will no longer be wrapped in the conditional, as it's unnecessary.
• ->require_jetpack_authentication() will still be wrapped in the conditional, as it's necessary
• Load order will still be the same - $this->require_jetpack_authentication() will be called before $this->add_remote_request_handlers()
• The relationship between these 2 will be clear, thanks to the inline docs we added in 3b7bb5c.

There is an alternative solution in case you don't agree going ahead with this:

• We could add a jetpack_should_require_authentication filter that's applied in Automattic\Jetpack\Connection\Manager::init(), returning false by default.
• We could use that filter to decide whether to require Jetpack authentication in Automattic\Jetpack\Connection\Manager::init()
• We could hook to the filter in Jetpack::init() to force it to require Jetpack auth if the condition is met (not XMLRPC, and a jetpack remote file upload XHR request).

Let me know what you think.

[mdawaffe]

We only have to require Jetpack Auth because of these "AJAX" handlers. (They're not really AJAX - we just (ab)use the Core Admin AJAX endpoint because that's available on all WordPress sites.)

I think doing anything in the Manager for these AJAX handlers means we're coupling the Manager too tightly to the file upload feature.

Suppose Woo starts using the Connection package. Suppose that plugin needs to do something similar with file uploads but needs their own endpoint for whatever reason. Should Woo make a PR to change the Manager to also require Jetpack Auth for 'woo_file_upload' === $_POST['action']? That seems miserable to maintain :)

We could add a jetpack_should_require_authentication filter

This decouples the Manager nicely, though I worry it's overly complicated.

Can we just call the Manager's ->require_jetpack_authentication() directly from the Jetpack class?

Something like:

• Remove the conditional in Jetpack::__construct() and always call $this->add_remote_request_handlers() (just like this PR is currently doing).
• In Jetpack's add_remote_request_handlers(), do:

public function add_remote_request_handlers() {
  if ( ! is_admin() || ! Constants::get_constant( 'DOING_AJAX' ) ) {
    return;
  }

  $actions = [ 'jetpack_upload_file', 'jetpack_update_file' ];
  if ( ! isset( $_POST['action'] ) || ! in_array( $_POST['action'], $actions, true ) ) {
    return;
  }

  // Require Jetpack Auth for these "AJAX" endpoints.
  $this->connection_manager->require_jetpack_authentication();

  foreach ( $actions as $action ) {
    add_action( "wp_ajax_nopriv_{$action}", [ $this, 'remote_request_handlers' ] );
  }
}

(The above is totally untested :) Not sure it even parses.)

[tyxla]

I see what you mean! It's a completely different approach to what I've done here, so I'll start a new PR for it.

Thank you!

[tyxla]

@mdawaffe see #13283

[tyxla]

Thanks for the feedback @mdawaffe!

Replied to it and re-requested review.

[tyxla]

Closing in favor of #13283