wp_update_user doesn't work properly with current user instance

[oztaser]

After #28020 merged in 5.8, there are two errors that I noticed in wp_update_user.

1. Storing plain password:

If user object to be updated is getting by get_userdata (with current user id) or wp_get_current_user, wp_update_user doesn't hash the new password.

Also doesn't send password change email.

2. Doesn't send Email change email.

Here is my mu-plugin to produce the errors:

<?php
namespace NefisYemekTarifleri\Test_User_Password_Hash;

add_action( 'init', __NAMESPACE__ . '\\init' );
function init() {
        global $wpdb;

//      $user             = get_userdata( 1 );
        $user             = wp_get_current_user();
        $user->user_pass  = '123456';

        wp_update_user( $user );
}

I think it's not the best way to change password using wp_update_user, but if wp_update_user supports it, password should be hashed.

P.S: I couldn't find any related ticket about this problem, sorry if there is.

[SergeyBiryukov]

Hi there, welcome to WordPress Trac! Thanks for the report.

Moving to 6.0 for investigation.

[ravipatel]

wp_update_user() has a condition to convert a password in hash.

​https://github.com/WordPress/WordPress/blob/5.9-branch/wp-includes/user.php

if ( ! empty( $userdata['user_pass'] ) && $userdata['user_pass'] !== $user_obj->user_pass ) {
		// If password is changing, hash it now.
		$plaintext_pass        = $userdata['user_pass'];
		$userdata['user_pass'] = wp_hash_password( $userdata['user_pass'] );

		/**
		 * Filters whether to send the password change email.
		 *
		 * @since 4.3.0
		 *
		 * @see wp_insert_user() For `$user` and `$userdata` fields.
		 *
		 * @param bool  $send     Whether to send the email.
		 * @param array $user     The original user array.
		 * @param array $userdata The updated user array.
		 */
		$send_password_change_email = apply_filters( 'send_password_change_email', true, $user, $userdata );
	}

[peterwilsoncc]

Thanks for providing an mu-plugin as a proof of concept, @oztaser, it was a great help.

The bug seems to be caused by the combination of two things:

• objects are passed by reference
• since 5.8 WP has used the same user object for the current user whenever possible.

The result is that in this block of code, the old user data object is no such thing: it's simply a reference to the modified object that has been passed to the function.

<?php

// In `wp_update_user()`

// First, get all of the original fields.
$user_obj = get_userdata( $user_id );
if ( ! $user_obj ) {
        return new WP_Error( 'invalid_user_id', __( 'Invalid user ID.' ) );
}

​reference

---

In 54984.diff​ I've worked around this by creating a new WP_User object for the purpose of obtaining the old data.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

A technique to force an uncached copy of the user data is what is needed to fix this though.

[dd32]

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Thinking out-loud here: If we're only expecting a WP_User object in these places/globals, and since we've got a WP_User::__set() that takes care of $user->user_pass = 12345;, WP_User could create a copy of the user-data prior to altering fields values and a WP_User::has_changed( 'user_pass' ) could be used to detect if the object has changed / what fields have changed.

That doesn't take care of a situation where $user->data->user_pass is changed directly though.

So perhaps 54984.diff​ is indeed the ideal way forward.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Correct me if I'm wrong, but in those cases, this could've previously been an issue too, right?

[peterwilsoncc]

Replying to dd32:

So perhaps 54984.diff​ is indeed the ideal way forward.

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

Correct me if I'm wrong, but in those cases, this could've previously been an issue too, right?

Reviewing the code, yes. new WP_User() is called directly fairly frequently so one more call won't make a difference.

As RC1 is later this week, I'll move this to the 6.1 milestone to be actioned during the next cycle.

[tykoted]

However, I am not sure this is an ideal solution as get_userdata() and related functions are pluggable so there is no way of knowing if the data returned by new WP_User( $user_id ) is what is returned by get_userdata().

I would say this can be the ideal solution as well. The fix will not touch the get_userdata() which is used in other areas, so there’s no risk of breaking existing functionality.

[cu121]

Hello,

I have thoroughly gone through this issue and inspected the functions related to wp_update_user(), and the reason why the password is not hashed when modifying the property of the same instance of the WP_User object is that when the WP_User user_pass is modified, the function get_user_by() generally fetches the user saved data and it checks if the WP_User object is the current instance or not. If it is the same instance then it returns the same WP_User instance $current_user, which is why the fetched database data of the user is not returned.

I assume the developer who has worked with this logic has done this purposely and from my standpoint, it is absolutely valid. I assume we have to use wp_hash_password() when we want to modify the password of the current WP_User instance when accessing the user_pass property of the current user object.

This is where the main logic lies by returning the same instance of the WP_User ​https://prnt.sc/2Go8mmctOKfw. Feel free to post any feedbacks

Thank You

[peterwilsoncc]

Thanks @cu121, I think I'll revert the initial commit ([50790] for #28020) for the time being.

The performance gain of the earlier commit is negligible and while testing this issue I discovered a proper fix would introduce database calls invalidating the earlier performance gain.

[peterwilsoncc]

In 54397:

Users: Revert use of shared objects for current user.

Reverts [50790].

Props oztaser, ravipatel, dd32, costdev, SergeyBiryukov, tykoted, cu121, xknown.
Fixes #54984.

[SergeyBiryukov]

In 54544:

Users: Revert use of shared objects for current user.

Reverts [50790].

Props oztaser, ravipatel, dd32, costdev, SergeyBiryukov, tykoted, cu121, xknown.
Merges [54397] to the 6.0 branch.
Fixes #54984.