johnbillion / user-switching

WordPress plugin that provides instant switching between user accounts.
https://wordpress.org/plugins/user-switching/
GNU General Public License v2.0
187 stars 49 forks source link

Fatals when using `USER_SWITCHING_SECURE_COOKIE` which is not defined until `plugins_loaded` #115

Closed BrianHenryIE closed 4 months ago

BrianHenryIE commented 4 months ago

I have a login plugin where a user has reported an issue when user-switching is also active:

https://github.com/BrianHenryIE/bh-wp-autologin-urls/issues/24

My login code runs on determine_current_user, which runs before plugins_loaded. Your plugin does not define its constants until plugins_loaded but it does hook onto wp_login which then calls code which expects the constants to be defined, resulting in the crash.

My code in determine_current_user is firing wp_login as described in the first comment of the wp_set_current_user() codex.

I think you should just define your constants when the plugin is loaded. Or maybe early on determine_current_user.

Of course, I might be wrong here. I'm not clear on the order of hooks that are run after a plugin is loaded and plugins_loaded is fired!

johnbillion commented 4 months ago

Thanks for the report.

I think the root of the problem is that your plugin is calling the wp_login action earlier than it gets called by WordPress core. It's only called in one place in core, which is inside the wp_signon() function which is itself only called in one place, when signing in via wp-login.php. Anything that's hooked into wp_login can therefore safely assume that the bootstrap process is complete.

The reason User Switching defines its cookie constants on plugins_loaded is because the COOKIEHASH constant is defined by WordPress after mu-plugins are loaded, and User Switching can be used as a mu-plugin (background here), so that can't be changed.

Is there a reason your plugin explicitly calls the wp_login action? When the auth cookie gets set by User Switching it doesn't call wp_login. I assume it's because, unlike User Switching which switches from one user to another, your plugin actually logs a user in. User Switching does allow a user to switch off and back on again without it explicitly calling the wp_login action though.

Honestly not sure what the best fix here is, but I can't change the way that User Switching defines its cookie constants.

johnbillion commented 4 months ago

I know it's a pain, but you might consider trying to route all your "log in" related functionality through wp-login.php. So when a user visits a URL with ?autologin and they're not logged in, redirect them to wp-login.php, perform the login there at an appropriate point (eg. init) and then redirect them back to where they came from. This is essentially what User Switching does. That way you don't need to try to set the user on the determine_current_user action which is too early.

BrianHenryIE commented 4 months ago

Thanks. That gives me a lot to work with and I'll solve it on my end.

I think the easiest fix might be to wrap that wp_login action like:

add_action( 'init', function () use ( $wp_user ) {
    do_action( 'wp_login', $wp_user->user_login, $wp_user );
} );

Is there a reason your plugin explicitly calls the wp_login action?

I just put it there because the Codex had it there. I probably started writing this plugin in 2018 so I was relatively inexperienced then and just trusted the instructions.

you might consider trying to route all your "log in" related functionality through wp-login.php.

Good idea. The plugin sort of has this as an option in settings, as a fix for some caching issues.

you don't need to try to set the user on the determine_current_user action which is too early.

It certainly sounds like the right hook to use. I think I copied its use from the WP-API/Basic-Auth plugin.

Thanks again.

Edit: not suggesting you actually do this, but another fix would be to not add your wp_login action until plugins_loaded.

johnbillion commented 4 months ago

not suggesting you actually do this, but another fix would be to not add your wp_login action until plugins_loaded.

Interesting suggestion, thanks. I wonder if this might help obscure the underlying problem though. I'll have a think about it!

Good luck with the plugin, it actually sounds very useful and I'll keep an eye on it.