Page MenuHomePhabricator

TranslateSandbox::addUser does not register email
Closed, ResolvedPublic

Description

Follow-up to T111486, it was reported that emails are not getting set for newly created user accounts via the sandbox api. I tried to give a quick shot to investigate this, but got stuck at not having email field in AuthenticationRequest::loadFromSubmission but having it in UserDataAuthenticationRequest::getFieldInfo which gets called somewhere later.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 22 2016, 1:52 PM
Nikerabbit triaged this task as High priority.Aug 22 2016, 1:52 PM
Tgr added a comment.Aug 22 2016, 5:13 PM

Do you mean an email field? Unless something went horribly wrong, password should be in PasswordAuthenticationRequest and nowhere else.

Hmm. It looks like you don't have 'realname' in $wgHiddenPrefs on translatewiki, so you'll need to add a value for 'realname' to your $data array for UserDataAuthenticationRequest to properly load. It can be the empty string if you don't have any other value for it.

Tgr added a comment.Aug 22 2016, 6:41 PM

Maybe that should be fixed in AuthManager? Omitting optional fields from the array seems convenient when programmatically creating requests. Or maybe we could log something to make it less surprising - the current code treats missing array fields as an error condition but doesn't behave the way you would except from error-handling code.

UserDataAuthenticationRequest as a whole is optional, so there's no error if it fails to load. But the default loading for AuthenticationRequests insists on all (non-checkbox) fields being present in the submission even if optional, because otherwise there's no way to differentiate "Request wasn't submitted" and "Request was submitted but all fields were empty/default" if something needs to do that.

Nikerabbit updated the task description. (Show Details)Aug 22 2016, 7:20 PM

Corrected password -> email in description.

Tgr added a comment.Aug 22 2016, 7:31 PM

It's not all fields in this case though. But I guess with potential overlaps between different requests, the sanest way the case when some fields are missing but others are not is to assume that the present ones are for some other request.

Change 306660 had a related patch set uploaded (by Nikerabbit):
Sandbox: Quick workaround for emails not being saved

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

I made a quick patch to make this work again. Are you planning further fixes on core?

Change 306660 merged by jenkins-bot:
Sandbox: Quick workaround for emails not being saved

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

But I guess with potential overlaps between different requests, the sanest way the case when some fields are missing but others are not is to assume that the present ones are for some other request.

Exactly. For example, PasswordDomainAuthenticationRequest has 'username', 'password', 'retype', and 'domain' fields (depending on the action), but if 'domain' is missing it's still a valid PasswordAuthenticationRequest and if all except 'username' are missing it could still be a UsernameAuthenticationRequest.

Are you planning further fixes on core?

I'm not planning on any. I don't know if @Tgr is thinking of trying anything.

Nikerabbit removed a project: Patch-For-Review.

Okay, I propose @Tgr to file a new task(s) if he thinks it is worth it. We can then close this in our sprint.

Nikerabbit closed this task as Resolved.Aug 29 2016, 8:48 AM
Nikerabbit claimed this task.

Confirmed to work.

Tgr added a comment.Aug 30 2016, 2:21 AM

Okay, I propose @Tgr to file a new task(s) if he thinks it is worth it.

I don't think anything else could be done about this.