wp_authenticate_username_password() should check for a WP_Error object

[kwisatz]

This is a follow-up to #19714 and #22516

These were closed, but IMHO, the bug as such is not resolved.
wp_authenticate_username_password() does only check whether $user is a WP_Error object when either password or username are empty:

75 if ( empty($username) || empty($password) ) {
76	if ( is_wp_error( $user ) )
77            return $user;
78 …

However, another plugin that hooks into authenticate might pass a WP_Error even if $username and $password were provided, but the specific authentication mechanism failed.
The current implementation completely ignores this.

The result is that users who exist locally can log in using a local, possibly older password, bypassing the plugins' additional authentication mechanisms completely.

I'd advocate to check for is_wp_error($user) much earlier in wp_authenticate_username_password(), so that the authentication process will fail as soon as one "authenticate" filter returns a WP_Error object.

[kwisatz]

Appendix:

I am no longer 100% sure about this.

The problem is, what happens when the plugin hooking into the authentication process is unable to perform authentication (perhaps the authentication server is unreachable). In such a case, you might also want to return a WP_Error, but in this case, it's not that the user is invalid, it's just that we cannot tell.

However, even in that case, the Login form should also display the WP_Error returned by plugins hooking into authenticate I guess.

[DrewAPicture]

Hi @kwisatz, welcome to Trac. Did you still want to try to pursue this? Care to try submitting a patch?

[extendwings]

Replying to kwisatz:

the Login form should also display the WP_Error returned by plugins hooking into authenticate

It seems the WP_Error contents from plugins are displayed by ​this code. (Sorry if I'm missing something.)

[iandunn]

Switching from wontfix to maybelater, since that's more accurate.

xref: ​https://make.wordpress.org/core/2019/01/14/follow-up-on-recent-trac-bulk-edit/

[donmhico]

I'm not sure how to proceed with this. From the documentation of wp_authenticate_username_password()

@param WP_User|WP_Error|null $user  WP_User or WP_Error object from a previous callback. Default null.

It seems that it's expecting the $user input as a WP_Error instance in some cases. And considering that the WP_Error check is placed below the empty $username and empty $password check -

if ( empty( $username ) || empty( $password ) ) {
    if ( is_wp_error( $user ) ) {
        return $user;
    }
    .......
}

It maybe possible that wp_authenticate_username_password() is design to give a chance to a WP_Error $user to still be authenticated if the $username and $password are provided. I'm just not sure if there's a good use-case for such scenario or if this is really an intended behaviour.

IMHO, if the $user passed is a WP_Error then it should immediately return the WP_Error. Like @kwisatz 's concern, this maybe a potential security hole.

We need more feedback regarding this.

Reference:
​https://developer.wordpress.org/reference/functions/wp_authenticate_username_password/