WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#21907 closed enhancement (fixed)

Allow custom errors on login when using XML-RPC

Reported by: koke Owned by: westi
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: XML-RPC Keywords: mobile westi-likes has-patch dev-feedback
Focuses: Cc:

Description

Currently, the code for login in XML-RPC does this:

$user = wp_authenticate($username, $password);

if (is_wp_error($user)) {
        $this->error = new IXR_Error(403, __('Bad login/pass combination.'));
        return false;
}

So any custom error returned by wp_authenticate is thrown away and replaced by a generic error

Our use case: we recently added Two Step authentication to WordPress.com. The XML-RPC api needs to use an application specific password, since there's no support for entering the auth code. When a user logs in with the right username/password we need to return a specific error message asking the user to login with an application specific password.

Other custom login systems may require specific error messages too

Attachments (3)

21907.diff (1.1 KB) - added by JustinSainton 6 years ago.
21907.1.diff (1.1 KB) - added by JustinSainton 6 years ago.
21907.2.diff (1.0 KB) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (18)

#1 @ethitter
6 years ago

  • Cc erick@… added

#2 @batmoo
6 years ago

  • Cc batmoo@… added

#3 @sirzooro
6 years ago

  • Cc sirzooro added

#4 @westi
6 years ago

  • Keywords needs-patch westi-likes added
  • Milestone changed from Awaiting Review to 3.5
  • Owner set to westi
  • Status changed from new to accepted
  • Version set to trunk

These sounds like a great quick win for 3.5.

Would someone like to create a patch?

@JustinSainton
6 years ago

#5 @JustinSainton
6 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

I'm not really familiar with XML-RPC, but would love to learn more. If I understand the issue properly, perhaps this simple patch would suffice? Filter the user object to return a WP_Error object and then filter the error message. Let me know if this is even a proper direction.

#6 @JustinSainton
6 years ago

Whoops, intended to add the $user var to the message filter on the first pass, so you could actually access whatever was passed there.

#7 @koke
6 years ago

Actually, the only reason I find not to return the errors directly, is that some errors from wp_authenticate_username_password return html instead of text only. We could, replace invalud_username, invalid_password, and authentication_failed by the current generic error, and convert everything else to an IXR_Error

#8 @JustinSainton
6 years ago

Actually, unless I'm mis-reading something, all of the errors returned by wp_authenticate_username_password contain HTML (at least the <strong> ERROR: </strong>, if not also a link). All of them except for blog_suspended, at least.

Any thoughts on approach? Could preg_replace or strip_tags - but both would end up hacky. We could map the error codes to non-HTML versions. Or we could just keep it as-is.

FWIW, the reason I used the filter in my initial approach rather than Sergey's approach was because my understanding was that you needed to add a custom error message for an application key, rather than return the message directly from the WP_Error object.

#9 follow-ups: @nacin
6 years ago

  • Type changed from feature request to enhancement

This will probably need to be a filter on the error object. If we want to do more in the future, we can.

To be honest, I'm a little weary of returning more than just the generic error by default, lest XML-RPC becomes a quieter way to slam a site for identifying valid usernames.

#10 @JustinSainton
6 years ago

Maybe like 21907.1.diff sans the xmlrpc_userlogin_error filter?

#11 in reply to: ↑ 9 @koke
6 years ago

Replying to nacin:

To be honest, I'm a little weary of returning more than just the generic error by default, lest XML-RPC becomes a quieter way to slam a site for identifying valid usernames.

I'm totally ok with returning the same generic error, unless a plugin asks otherwise

#12 @nacin
6 years ago

In [21910]:

XML-RPC: Have the deprecated login_pass_ok() method wrap login(). Move it below login() so the proper method is found first. see #21907.

#13 @nacin
6 years ago

In [21911]:

Deprecate user_pass_ok() in favor of wp_authenticate(). see #21907.

#14 @nacin
6 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [21912]:

Introduce the xmlrpc_login_error filter, applied to the IXR_Error being returned by the server when login() fails. props JustinSainton, fixes #21907.

#15 in reply to: ↑ 9 @westi
6 years ago

Replying to nacin:

This will probably need to be a filter on the error object. If we want to do more in the future, we can.

To be honest, I'm a little weary of returning more than just the generic error by default, lest XML-RPC becomes a quieter way to slam a site for identifying valid usernames.

Why is this any easier/quieter than scripting the wp-login.php POST?

Note: See TracTickets for help on using tickets.