Page MenuHomePhabricator

ApiQueryUsers using PARAM_TYPE = 'user' results in unexpected errors
Closed, ResolvedPublic

Description

ApiQueryUsers defines the username as type user, which means that passing an invalid username results in an API error. Given that this API module is typically the one that gets used to process users whose names come from user input (such as signup form username validation), that is unfortunate. Maybe the API would be better off with text type and an invalid attribute used for that username in the result data.

Event Timeline

Tgr created this task.Aug 13 2016, 5:12 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 13 2016, 5:12 AM
Anomie added a subscriber: Anomie.Aug 15 2016, 1:50 PM

In the specific use case stated here, you're only submitting one name and could as easily interpret the "usbaduser_ususers" error in the same way you'd otherwise interpret an "invalid" tag in the results.

That's not to say this proposed change doesn't have merit on its own, though.

Anomie moved this task from Unsorted to Needs Code on the MediaWiki-API board.Aug 15 2016, 1:50 PM
Tgr added a comment.Aug 15 2016, 6:43 PM

Yes, it is not hard to fix, hard to realize it needs to be fixed and not make a mistake, though. The signup username check has been broken for years without anyone noticing, and signup is one of the more scrutinized workflows.

It was not broken for years, it was broken since January this year when you broke it with 381a6ce691840437c6a449fe52bfd6dacb80af2d :) I verified that reverting this change fixes this issue. It was technically an unannounced backwards-compatibility break.

(To be clear, the issue is that on Special:CreateAccount, entering "A|B" gives an AJAX error, but entering "A<B" only gives an error after submitting the form. I noticed this when reviewing https://gerrit.wikimedia.org/r/#/c/284902/, turned out to be a pre-existing issue.)

@Anomie Should I just update the JS code (resources/src/mediawiki.special/mediawiki.special.userlogin.signup.js) to handle the new error format, or are you planning to restore the previous one in ApiQueryUsers?

Change 306034 had a related patch set uploaded (by Anomie):
API: Don't require 'users' parameter to contain all valid usernames

https://gerrit.wikimedia.org/r/306034

Anomie claimed this task.Aug 22 2016, 6:26 PM
Anomie moved this task from Needs Code to Needs Review on the MediaWiki-API board.

Change 306034 merged by jenkins-bot:
API: Don't require 'users' parameter to contain all valid usernames

https://gerrit.wikimedia.org/r/306034

Anomie closed this task as Resolved.Aug 22 2016, 7:02 PM