Page MenuHomePhabricator

action=createaccount should do checks in the first request, not second
Closed, ResolvedPublic

Description

Should check for blocks, existing usernames, and other issues in the first request, and then ask for a captcha. Right now, if the user tries to register a name already registered, we ask them to solve a captcha and *then* tell them it is taken. And then they have to try again, and we ask them for a captcha solving before proceeding. This is terrible UX, and I can't think of a reason why this has to be the case.


Version: 1.23.0
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=34447

Details

Reference
bz61704

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:57 AM
bzimport set Reference to bz61704.
bzimport added a subscriber: Unknown Object (MLST).

If you want to refactor LoginForm::addNewaccountInternal() into a validation function and an actual creation function, feel free and I'll review it. Keep in mind that we'll most likely still want to run 'AddNewAccountApiForm' before 'AbortNewAccount'.

But let's not duplicate all the checks in LoginForm::addNewaccountInternal() into ApiCreateAccount.

Change 116872 had a related patch set uploaded by MaxSem:
action=createaccount should do checks in the first request, not second

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

Looking at this closer, I think the description of this bug is misleading. Which means comment 1 should be ignored, at least as far as ConfirmEdit goes.

The problem isn't the order of the checks, it's that the captcha information is added at the "NeedToken" step (implying it needs to be solved there) and not in a later step. That should probably be the other way around.

So what should really be happening is that ConfirmEdit shouldn't use the 'AddNewAccountApiResult' hook at all. Instead, the case in ApiCreateAccount handling a not-OK status should call some other hook function to populate an $extraData array (which ConfirmEdit would use to inject its captcha data), then call $this->getErrorFromStatus() and $this->dieUsage($msg, $code, 0, $extraData). Or we could add an $extraData parameter to ApiBase::dieStatus.

And note you're probably still going to run into problems of this sort if there are multiple AbortNewAccount hook functions, since ConfirmEdit's might be run before the other extensions'. There's nothing the API can do about that, it would take redefining AbortNewAccount to still fail the creation when $abortStatus is non-OK even if the hook function returned true (and then probably deprecate returning non-true for that hook).

Change 116872 abandoned by MaxSem:
action=createaccount should do checks in the first request, not second

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

After poking at it some more, what would fully help is to perform all checks but token, then call AbortNewAccount, then check token and only then call some new hook for ConfirmEdit ("AbortNewAccountForRealz"?). Simply adding extra data to dieStatus() would result in clients having to handle two cases with principally diffrent data structure: token is needed (normal return with {createaccount:{result:"NeedsToken", captcha:...}}) and all other problems ({error:{...}}).

Change 118374 had a related patch set uploaded by MaxSem:
Allow extensions modify API action=createaccount errors

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

Change 118381 had a related patch set uploaded by MaxSem:
Return captcha information via createaccount API only if no other errors

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

Change 118374 abandoned by MaxSem:
Allow extensions modify API action=createaccount errors

Reason:
Ehm, not needed.

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

Change 118381 merged by Brion VIBBER:
Return captcha information via createaccount API only if no other errors

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

Ok, 8a336ddd9 from that last change set tweaks things so the initial 'NeedsToken' response doesn't include the captcha information. This means the user gets prompted for a captcha only after we've checked other fatal errors. Seems to work in testing. :D Will update docs.

Be aware that this hasn't gone out live yet... Do we need an LD or just wait for it and make sure the apps handle the new mode?

@brion: Is there anything left to do here, or is this resolved?

Anomie set Security to None.