plugins which use the 'authenticate' hook unable to return errors

[willnorris]

The 'authenticate' hook is designed to allow functions to return either an authenticated WP_User object (which will cause the user to be logged in), or a WP_Error object, which will cause the errors to be displayed to the user.

In practice, most plugins that use this hook don't rely on the username and password at all, but instead on other means entirely. So what is happening with these plugins (the OpenID plugin chief among them), is that they are returning a WP_Error object that describes the error, but then the wp_authenticate_username_password function is ignoring that and returning its own WP_Error object which rightfully shows that the username and password fields were left empty. Unfortunately, this error object (containing both an empty username AND password) is explicitly checked for and removed in the wp_signon method. This is normally the right behavior and handles the case of a user who simply clicks "Log In" without entering anything... we don't show them an error, we just redraw the login form. However, in the case described above, an actual error did occur with an authentication plugin, but the user simply sees the normal login form with no error displayed.

(patch forthcoming)

[nacin]

If you hook into authenticate at a priority > 20, would this still be a problem?

[willnorris]

The way the 'authenticate' hook is designed, it's up to each function which implements the hook to look at the passed in user value and decide what to do with it. WordPress core has two functions which implement this hook, wp_authenticate_username_password and wp_authenticate_cookie. Both of these will immediately return if the passed in user is a WP_User object, but they don't do anything special if it's a WP_Error object. In some cases, the error will get passed through untouched, but in other cases it will get overwritten, as described above.

We have to be careful about returning immediately if a WP_Error object is present, however. The authentication system currently allows for failover. Even though one authentication method may have failed, there's nothing to say that a later one might not still succeed and successfully authenticate the user (this is especially true of wp_authenticate_cookie which pretty much exclusively relies on this approach).

The provided patch modifies only the wp_authenticate_username_password function to check, at the point where it would have otherwise overwritten an existing WP_Error, and return the passed in WP_Error if it exists. The rationale here being that if a WP_Error is already present, it must have been set by a plugin trying to handle authentication, and should therefore trump the "no username or password" error. If a plugin actually does care about the absence of the username or password, it will be up to them to add that error as well. This also still allows wp_authenticate_username_password to attempt to authenticate the user using the username and password if present.

[willnorris]

@nacin: that would also be one solution. However, at least for any auth plugins I've written, users typically want the alternate authentication method to take precedence over the username/password approach. I'd have to think a bit more if this would actually introduce any problems in practice though.

(that said, the provided patch is pretty minimal and has very low risk for introducing problems)

[nacin]

Thank you very much for the detailed explanation.

[willnorris]

hmm, okay now I'm torn. I still feel like auth plugins should typically happen before the username/password though I'm having trouble articulating exactly why.

However, I just noticed that wp_authenticate_username_password also has checks for the user or their site being marked as spam. Based on how things work today, the spammer would still be able to login using an auth plugin like OpenID, since that happens before the spammer checks. My gut reaction is that the spammer check should not happen inside the wp_authenticate_username_password method, since that actually has nothing to do with the original intent of that method... authenticating a user by username and password. Instead, I think it should be its own function that hooks into 'authenticate' much later. If you're okay with the patch as written, I'll open a separate bug to track this.

[willnorris]

ping. any new thoughts on this?

[nacin]

I am trying to think about what problems the patch could introduce, and whether we should be concerned. I don't think we should, and I think I like the patch. As long as $user is already a WP_Error object, then clearly a plugin has stepped in, and the login won't be successful.

The other thing I've thought about is allowing add() to run on the existing WP_Error object, thus creating an object with at least three error codes, and then tweaking it so empty_username and empty_password is still filtered out. But the end result is the same, AFAICT.

I agree that wp_authenticate_username_password() is being abused with the spam checks. In MU, it did exist as a separate function hooked into wp_authenticate_user. That was merged in with [12879]. The main reason for why that occurred was to clean up the MU codebase, but I think ryan would support bringing that back out. The wp_authenticate_user hook does seem better than authenticate for this, though, so I'm a bit torn as to the specifics of how we'd handle it.

[nacin]

Sidebar — This code in wp_authenticate() made me think about just letting add() get called:

	$ignore_codes = array('empty_username', 'empty_password');

	if (is_wp_error($user) && !in_array($user->get_error_code(), $ignore_codes) ) {
		do_action('wp_login_failed', $username);
	}

My take on this code is that it should instead be checking to see if get_error_codes() != array( 'empty_username', 'empty_password' ), per the check in wp_signon(). wp_login_failed seems like a fringe hook, though.

[nacin]

#22516 was marked as a duplicate.

[willnorris]

Patch updated.

I'm still bailing out of wp_authenticate_username_password early if the passed in $user is a WP_Error, rather than letting the missing username and password errors be attached as @nacin suggested above. Otherwise, that would then require removing them from the error object later, and the code started getting really ugly. Besides, I can't think of a use case where a plugin would have already returned an auth error, and we would still care about whether a username and password were provided. If the username or password are relevant to the plugin, then its error message should mention that.

I've also made two additional changes:

• as discussed previously, the spam check has been moved to it's own function and added as a late hook (priority 99) on the authenticate filter. This will now block spam accounts regardless of what authentication method they used.
• default authentication filters have been moved out to default-filters.php where they really belong

[willnorris]

patch now also updated to fix @nacin's note above about wp_authenticate properly calling the wp_login_failed action

[willnorris]

updated to use is_user_spammy changes in #24771, so that patch will need to be applied before this one.

[willnorris]

well on second thought, you're not actually going to be able to apply this patch on top of #24771. I'm happy to rework this one once #24771 is checked in.

[nacin]

In 24848:

Remove "special" multisite spam check in the authentication API.

The spamming of a site no longer directly affects a user of said site.

Moves the spam check to the wp_authenticate filter. Networks in need
of enhanced spam-fighting should leverage this same technique.

Allow is_user_spammy() to accept a WP_User object.

props willnorris, brianhogg.
fixes #24771. see #19714.

[nacin]

Patch needs refresh thanks to [24848].

[nacin]

In 24850:

Don't override an existing WP_Error object in wp_authenticate_username_password().

props willnorris.
fixes #19714.