register_authentication_servers() runs too late?

[tyrann0us]

Authentication#register_authentication_servers() is a callback for the plugins_loaded action hook invoked with a priority of 8. It adds a callback for the determine_current_user filter that is executed in _wp_get_current_user(): https://github.com/cedaro/satispress/blob/3ff7ffe56f32b9b744dff0fba23b2fc0beff4d76/src/Provider/Authentication.php#L46

Now if a plugin also uses the plugins_loaded hook (which it should) but with a lower priority (meaning it's callback is executed earlier) that can cause problems if it calls wp_get_current_user() (which in turn calls _wp_get_current_user()). The reason is that the determine_current_user filter is only run once per request, i.e. if the global variable $current_user is still empty.

This will cause UnauthorizedServer#authenticate to not be executed at all, which in turn causes the auth_status property to be null, which then causes SatisPress' authentication to fail with an Uncaught SatisPress\Exception\HttpException: Forbidden resource requested; User: 0; URI: /satispress/packages.json in /path/to/plugins/satispress/src/Exception/HttpException.php on line 71.

Changing the plugins_loaded action priority from 8 to 0 would fix this. Is that something you might want to consider? Thanks!

[bradyvercher]

Is there a reason you need to call wp_get_current_user() that early? Doing so likely causes issues with more plugins than just SatisPress. For instance, the OAuth 1 Server for the REST API doesn't wire up its filter until init, which used to be earliest that functions like wp_get_current_user() and get_current_user_id() could be called.

They can be called earlier than init now, which caused me issues in another authentication plugin when plugins started using get_user_locale() on plugins_loaded:10 after WP 4.7 was released.

[tyrann0us]

Is there a reason you need to call wp_get_current_user() that early?

Don't ask me, ask the folks over at @moderntribe. 😄 Their "plugin bootstrapper" runs the main initialization routine at a priority of 1:

add_action( 'plugins_loaded', array( $this, 'plugins_loaded' ), 1 );

One of their premium addons is then initiated in the plugins_loaded method (hooked to the tribe_common_loaded action) and calls wp_get_current_user() (simply put). I will write a ticket why they need such a low priority but I'm afraid they will change that given the fact @jesseeproductions has only recently reduced the priority from 5 to 1. Note: I must have the free basic plugin installed and active to ensure that the premium addon's auto updater works.

Since SatisPress' purpose is to provide a Composer repository for non-public/premium plugins whose developers still haven't understood that composer is the de facto standard today, it must be assumed that the code quality of the plugins managed by SatisPress is also poor (CodeCanyon, ThemeForest). It can therefore not be ruled out that other plugins will also call the function too early, so that the problem starts all over again.

Since wp_get_current_user() is pluggable, maybe the best approach would be to replace it with a function that always runs determine_current_user and/or write a Trac ticket?

[tyrann0us]

Related Trac ticket: https://core.trac.wordpress.org/ticket/46586.

[bradyvercher]

Hmm, that is unfortunate. A couple of approaches I see going forward would be to:

• Attach the determine_current_user filter immediately. This would prevent custom authentication servers from being registered.
• Change the priority to plugins_loaded:0. Custom authentication servers would need to be registered earlier and it'd still be possible to wind up in the same situation if someone decides to call wp_get_current_user() before that.
• It might be possible to wire up determine_current_user immediately and iterate through the servers in that callback instead. That would likely require a bit of refactoring, but it would at least allow custom authentication servers to continue to be registered before the callback runs.

I don't think running determine_current_user multiple times per request is feasible since it could have unintended consequences, especially for existing authentication methods that only expect to ever be run once.

[bradyvercher]

@tyrann0us I believe this should be good to go after #97.

[tyrann0us]

I can confirm it works now for the specific plugin. Thanks alot!

[bradyvercher]

@tyrann0us Awesome! Thanks for testing that out and letting me know. I'll try to push a new release with that fix in the next day or two.