Jetpack: WP REST API: Alternative authentication issues in 8.9

[kraftbj]

I was notified in https://github.com/indieweb/wordpress-micropub/issues/235 of an apparent issue with REST API authentication after upgrading to 8.9 on an unrelated endpoint.

I'm awaiting a quick reproduction step list, but given the debugging done in the original issue, the reversion to 8.8.2 resolving it, and the timing, it is reasonable to assume this is something we did.

I'll work it in the third-party issue for now, opening this so it stays on the radar as we evaluate a 8.9.1.

[kraftbj]

Just a quick note since we're getting close to Wednesday. I've confirmed something in 8.9 is responsible. It seems like we're calling the current user early which is preventing the determine_current_user to run for these requests, but I haven't nailed it down yet.

Need to reset my local env, which will run now. I'll pick this back up tomorrow and bisect it to at least narrow down what code changes I should focus on.

[jeherve]

Possibly related: #16839 (cc @kbrown9 and @sergeymitr who have been working on this).

[sergeymitr]

Thanks for the ping!

That seems to be the same problem @DanReyLop has run into while using WCPay and Client Example plugins without Jetpack.
If that's the case, we merged the fix yesterday: #17158

@kraftbj can you please make sure that the problem doesn't come up on master?

[kraftbj]

@sergeymitr It seems to be fine in master, but not when I add

add_filter( 'jetpack_constant_default_value', __NAMESPACE__ . '\Utils::jetpack_api_constant_filter', 10, 2 );

directly to the rest authentication class. So, I think we fixed it, but digging in some more. Like to get whatever it is to fix it into the point release. I'll bisect it from 8.9 to master to find what may have fixed it.

[kbrown9]

@sergeymitr It seems to be fine in master, but not when I add

          'jetpack_constant_default_value',
          __NAMESPACE__ . '\Utils::jetpack_api_constant_filter',
          10,
          2
      );

directly to the rest authentication class. So, I think we fixed it, but digging in some more. Like to get whatever it is to fix it into the point release. I'll bisect it from 8.9 to master to find what may have fixed it.

I'm seeing the same thing: master fixes the problem, but the changes in #17158 alone do not.

[kraftbj]

@kbrown9 I'm getting pulled away for the next few hours. Any chance you could bisect and see what we might have fixed that we could get in for 8.9.1? I did a bisect, but it pointed to something that landed in 8.9 as the resolution... so I want to run it again.

If not, no biggie. I'll hit it this afternoon.

[kbrown9]

@kbrown9 I'm getting pulled away for the next few hours. Any chance you could bisect and see what we might have fixed that we could get in for 8.9.1? I did a bisect, but it pointed to something that landed in 8.9 as the resolution... so I want to run it again.

If not, no biggie. I'll hit it this afternoon.

I'll take a look!

[kbrown9]

It looks like commit 474a89f caused the problem in 8.9, and commit e975b5e fixes it in master. I don't understand how the problem occurs yet.

[kraftbj]

Untested, but maybe this line:
if ( $terms_of_service->has_agreed() && $connection->is_user_connected() ) {

is_user_connected calls get_current_user_id calls wp_get_current_user which calls _wp_get_current_user and so we're setting the current user.

I think with && $this->ensure_feature( 'tracking' ); this is firing on priority 2, so we're firing and setting on plugins_loaded before their code fires later on plugins_loaded?

Does that make sense? Could be a combo of our fix and suggesting to them that they may want to add their filters on plugins_loaded as early as possible?

(Still on the virtual conference stuff but from a quick glance?)

[kbrown9]

Untested, but maybe this line:
if ( $terms_of_service->has_agreed() && $connection->is_user_connected() ) {

is_user_connected calls get_current_user_id calls wp_get_current_user which calls _wp_get_current_user and so we're setting the current user.

I think with && $this->ensure_feature( 'tracking' ); this is firing on priority 2, so we're firing and setting on plugins_loaded before their code fires later on plugins_loaded?

Does that make sense? Could be a combo of our fix and suggesting to them that they may want to add their filters on plugins_loaded as early as possible?

Yes, I think your explanation makes sense.

We still call $connection->is_user_connected() very early in plugins_loaded, and it look me a few minutes to figure out why the problem doesn't occur in commit e975b5e. We changed the logic from $terms_of_service->has_agreed() && $connection->is_user_connected() to $terms_of_service->has_agreed() || $this->connection->is_user_connected(). Since $terms_of_service->has_agreed() returns true after site registration, $this->connection->is_user_connected() isn't called and the problem doesn't occur.

Regarding asking them to add their filters as early as possible: we need to look at our Jetpack::add_configure_hook method. If they change their filter's priority, I think Jetpack::add_configure_hook is just going to change our hook's priority to the earliest available.

[kraftbj]

Cherry-picking https://github.com/Automattic/jetpack/commit/e975b5e5201137db2f12a21c45a7d8de4850bcd7 and testing on the 8.9 branch with it resolves it. Closing this. 🎉