Page MenuHomePhabricator

Update Translate to use AuthManager
Closed, ResolvedPublic

Description

See parent task and T110431#1604967.

Event Timeline

Tgr created this task.Sep 4 2015, 2:56 AM
Tgr raised the priority of this task from to Normal.
Tgr updated the task description. (Show Details)
Tgr added subscribers: Aklapper, Tgr.
Liuxinyu970226 set Security to None.Sep 4 2015, 9:25 AM
Liuxinyu970226 added a subscriber: Liuxinyu970226.
Nikerabbit changed the task status from Open to Stalled.Sep 9 2015, 7:19 AM
Nikerabbit added a subscriber: Nikerabbit.

Insufficient explanation of what is the expected outcome here.

Translate: provides its own auth manager; that should probably be killed

It does not, as per my understand of auth manager, please correct me if I am wrong. What Translate needs is 1) way to create new local user account in MediaWiki with given username, password and email. 2) restrict all write actions by the user except those allowed by Translate extension.

The point 2 is important because we want to be able to safely wipe out the accounts completely in case they do not pass certain tasks we ask them to do. In other words it is a one kind of spam bot control as well (but not only that.)

Tgr added a comment.Sep 9 2015, 9:54 PM

Insufficient explanation of what is the expected outcome here.

For now the task is just for capacity planning. Soon-ish, the User methods for password management will be deprecated, and user creation should happen via AuthManager.

It does not, as per my understand of auth manager, please correct me if I am wrong.

You are right; it does user management, not authentication.

What Translate needs is 1) way to create new local user account in MediaWiki with given username, password and email. 2) restrict all write actions by the user except those allowed by Translate extension.

Those in themselves should not be problematic; user deletion might be. Although using AuthManager for user creation will probably not make it more problematic than it already is.

Anomie added a subscriber: Anomie.May 11 2016, 9:53 PM

What Translate needs is 1) way to create new local user account in MediaWiki with given username, password and email.

This seems like a very odd thing to need. What problem does Translate need to solve that makes it need this rather than using the normal account auto-creation mechanisms?

  1. restrict all write actions by the user except those allowed by Translate extension.

Is there anything that requires this beyond the "safely wipe out the accounts" bit below?

The point 2 is important because we want to be able to safely wipe out the accounts completely in case they do not pass certain tasks we ask them to do. In other words it is a one kind of spam bot control as well (but not only that.)

This is also a very strange thing to need. Why do the accounts have to be deleted at all, instead of just being blocked if they prove problematic as is done with every other problematic account creation on wikis?


As for current problems besides the weirdness above, I see:

  • Use of the AddNewAccount hook, which has been replaced by LocalUserCreated since 1.26wmf24 and will no longer be reliably called under AuthManager.
  • Use of $user->addNewUserLogEntry(), which is deprecated since AuthManager handles logging when it creates accounts.
  • Use of $user->setPassword(), which may not actually do anything if the wiki doesn't have any password-based providers installed (e.g. a wiki that only does "login with your Facebook/Google/etc account").

What Translate needs is 1) way to create new local user account in MediaWiki with given username, password and email.

This seems like a very odd thing to need. What problem does Translate need to solve that makes it need this rather than using the normal account auto-creation mechanisms?

Have you looked at the front page of translatewiki.net? Is there nowadays an easy to use API which allows account creation and skipping captcha?

  1. restrict all write actions by the user except those allowed by Translate extension.

Is there anything that requires this beyond the "safely wipe out the accounts" bit below?

And not have spam all over the place.

The point 2 is important because we want to be able to safely wipe out the accounts completely in case they do not pass certain tasks we ask them to do. In other words it is a one kind of spam bot control as well (but not only that.)

This is also a very strange thing to need. Why do the accounts have to be deleted at all, instead of just being blocked if they prove problematic as is done with every other problematic account creation on wikis?

If they were blocked, the users could not use them again if they want to re-apply. We would also need to find alternative way to count new users without spam bots etc.

As for current problems besides the weirdness above, I see:

  • Use of the AddNewAccount hook, which has been replaced by LocalUserCreated since 1.26wmf24 and will no longer be reliably called under AuthManager.
  • Use of $user->addNewUserLogEntry(), which is deprecated since AuthManager handles logging when it creates accounts.
  • Use of $user->setPassword(), which may not actually do anything if the wiki doesn't have any password-based providers installed (e.g. a wiki that only does "login with your Facebook/Google/etc account").

Right now this feature is only used at translatewiki.net (as far as I know) and we do not support Facebook/Google/etc. accounts at this point of time. Supporting them would be nice but not essential right now.

Have you looked at the front page of translatewiki.net?

Not recently. I think I see what you're talking about, though: if you visit while logged out, you have a "create account" form on the main page.

Is there nowadays an easy to use API which allows account creation and skipping captcha?

There isn't a public API that allows account creation while skipping the captcha, because then spambots would use it to skip captchas.

Internally, you should be able to use AuthManager::autoCreateUser() to create the user as an auto-creation, then AuthManager::changeAuthenticationData() to set the password (assuming a PasswordAuthenticationRequest-using authentication provider is configured; you could copy the code from User::setPassword() if you want) and User::setEmail() to set the email.

Or you could try to create your own instance of the AuthManager class with a custom $wgAuthManagerConfig that wouldn't include ConfirmEdit's CaptchaPreAuthenticationProvider (or whatever else you didn't want to include), although it would probably be really easy to break other codes' assumptions in there somehow (e.g. hook functions that use AuthManager::singleton()).

But IMO it's better to have users create accounts through the normal mechanism, Special:CreateAccount or API action=createaccount.

If they were blocked, the users could not use them again if they want to re-apply.

If a user is a legitimate user instead of a spambot, ideally they wouldn't have been blocked in the first place. But normal blocks would leave them with access to their own talk page where they can do something to request an unblock (like enwiki's Template:Unblock and the processes behind it).

And if they are a spambot, they're not likely to usefully re-apply.

We would also need to find alternative way to count new users without spam bots etc.

You might ask WMF's Analytics team how they manage it.

As for current problems besides the weirdness above, I see:

  • Use of the AddNewAccount hook, which has been replaced by LocalUserCreated since 1.26wmf24 and will no longer be reliably called under AuthManager.
  • Use of $user->addNewUserLogEntry(), which is deprecated since AuthManager handles logging when it creates accounts.
  • Use of $user->setPassword(), which may not actually do anything if the wiki doesn't have any password-based providers installed (e.g. a wiki that only does "login with your Facebook/Google/etc account").

Right now this feature is only used at translatewiki.net (as far as I know) and we do not support Facebook/Google/etc. accounts at this point of time. Supporting them would be nice but not essential right now.

The AddNewAccount hook is still deprecated. To have your new accounts work right with other extensions, you'll want to change to creating the users through AuthManager in some way.

$user->addNewUserLogEntry() is going to be a no-op under AuthManager, so you'd want to change something there so you still get your log messages logged.

$user->setPassword() will eventually start logging deprecation warnings, but if you're fine with requiring that a password-based authentication provider is configured you could inline it.

Anyway, since this only applies to the parts of Translate that aren't used at WMF, I'm going to move this from blocking T110282 to blocking T110291 instead. And added T41480 for good measure.

Nikerabbit changed the task status from Stalled to Open.May 12 2016, 4:14 PM

But IMO it's better to have users create accounts through the normal mechanism, Special:CreateAccount or API action=createaccount.

I would be happy with a solution that supports these requirements:

  1. can be embedded in the front page
  2. allows skipping captcha
  3. allows adding user to the restricted group immediately

Skipping captcha is kind of moot because I think it is easy to not enable a captcha. The existing API can likely be called from the front page. But I don't know how I would force users to be in a particular group.

If a user is a legitimate user instead of a spambot, ideally they wouldn't have been blocked in the first place. But normal blocks would leave them with access to their own talk page where they can do something to request an unblock (like enwiki's Template:Unblock and the processes behind it).

The fact is that even though we have tried to make it super simple to become a translator, some people are not able to complete the process successfully [1][2]. It is not ideal how it is now, but I think blocking them would be worse. Even if we tried to guide that process I am not sure they would be able to get they account unblocked.

[1] https://docs.google.com/spreadsheets/d/1S0GhrnD9XVM6S_DlpMzkoK8kF6sn-Nd2FynrE6piEKM/edit#gid=1529333999
[2] http://laxstrom.name/blag/2014/03/03/numbers-on-translatewiki-net-sign-up-process/

We would also need to find alternative way to count new users without spam bots etc.

You might ask WMF's Analytics team how they manage it.

I can but whatever tooling they are using is most likely not available for translatewiki.net. I am happy to be proved wrong. Anyway, this is low on the list of requirements.

  • Use of $user->addNewUserLogEntry(), which is deprecated since AuthManager handles logging when it creates accounts.

Which means I would no longer be able to delay the log entry until the account is actually approved? On the other hand deleting that log entry is easy to do as well.

you'll want to change

While we continue to discuss how to align these two systems, I have some practical questions:

  1. when do things need to be fixed before stuff starts to break?
  2. what kind of help can I get for the fixing?

Obviously this is essential stuff for translatewiki.net and I cannot let it break, but at the same time I cannot agree to more work without understanding how much work it is and without appropriate planning with Language team.

Tgr added a comment.May 12 2016, 5:47 PM

Couldn't Translate just use user groups for this? Put new users in some 'temporary' group, strip most rights from that group, and leave user creation to core?

But IMO it's better to have users create accounts through the normal mechanism, Special:CreateAccount or API action=createaccount.

I would be happy with a solution that supports these requirements:

  1. can be embedded in the front page

In the general case, this would be a bit hard. You could do it with the API and a fair bit of custom JavaScript to build a UI from the description given by action=query&meta=authmanagerinfo, but it could get tricky to make that UI look good. Or someone could try to figure out how to make {{Special:CreateAccount}} transclude the account creation form, keeping in mind that ideally doing so should trigger OutputPage::disallowUserJs() ;)

If you limit it to only being compatible with PasswordAuthenticationRequest (i.e. fields for username, password, retype) and UserDataAuthenticationRequest (i.e. fields for email address and real name (if enabled)), you could likely make a mostly static form and point the form action at Special:CreateAccount.

  1. allows skipping captcha

I think what you said below is best

Skipping captcha is kind of moot because I think it is easy to not enable a captcha.

  1. allows adding user to the restricted group immediately

The simplest way would be to just use the LocalUserCreated hook, it gets called right after the $user->addToDatabase() during account creation (and auto-creation).

Or if you want to integrate a little more closely with the authentication flow, make a SecondaryAuthenticationProvider like this and configure it to run first.

use MediaWiki\Auth\AbstractSecondaryAuthenticationProvider;
use MediaWiki\Auth\AuthenticationResponse;

class AddRestrictedGroupSecondaryAuthenticationProvider extends AbstractSecondaryAuthenticationProvider {
    public function getAuthenticationRequests( $action, array $options ) {
        return [];
    }

    public function beginSecondaryAuthentication( $user, array $reqs ) {
        return AuthenticationResponse::newAbstain();
    }

    public function beginSecondaryAccountCreation( $user, $creator, array $reqs ) {
        $user->addGroup( 'restricted' );
        return AuthenticationResponse::newPass();
    }
}

If you wanted to (insecurely[1]) differentiate between the front page form and Special:CreateAccount in here, you could make an AuthenticationRequest subclass with a 'hidden' field, put a corresponding <input type="hidden"> in the front-page form, and check for it much like is done for the "ignore title blacklist" checkbox in this patch. Or do the same with an actual checkbox to allow admins to directly create an unrestricted user.

[1]: Anyone could add/remove/alter the <input type="hidden"> if that's all there is; to make it more secure you'd have to store something in memcache and make the <input type="hidden"> hold the key, something like what ConfirmEdit does.

The fact is that even though we have tried to make it super simple to become a translator, some people are not able to complete the process successfully [1][2]. It is not ideal how it is now, but I think blocking them would be worse. Even if we tried to guide that process I am not sure they would be able to get they account unblocked.

The process now is "sign up, do so badly on the sandbox tasks that the account gets deleted, sign up again with the same username"? How many are eventually successful with this, that wouldn't be able to figure out "sign up, do so badly on the sandbox tasks that the account gets blocked, request unblock"?

  • Use of $user->addNewUserLogEntry(), which is deprecated since AuthManager handles logging when it creates accounts.

Which means I would no longer be able to delay the log entry until the account is actually approved? On the other hand deleting that log entry is easy to do as well.

Correct, it would log "User is created" when the account gets created rather than when it gets approved. You could disable that with $wgNewUserLog = false, of course.

And, of course, you can log whatever you want when the account gets approved, just use ManualLogEntry directly instead of calling User::addNewUserLogEntry().

While we continue to discuss how to align these two systems, I have some practical questions:

  1. when do things need to be fixed before stuff starts to break?

The current plan looks something like this:

  1. Merge the core patches to add AuthManager: backend, web UI, API, release notes. At this point AuthManager exists in the code, but is not yet enabled (everything still goes through the existing code) so nothing should break.
  2. Update WMF-deployed extensions for AuthManager, i.e. close T110282.
  3. Set $wgDisableAuthManager = false on the WMF cluster. Probably a gradual roll-out (testwiki, group0, etc), with the possibility to re-disable if things blow up too bad at any point.
  4. Once we're confident everything works well, remove the disable flag (and the old codepaths it used).

We're about ready to perform step 1, and we're working on step 2 in parallel. At any point after step 1 you could set $wgDisableAuthManager = false on translatewiki, and I wouldn't be opposed to blocking step 4 on translatewiki being ready as well.

  1. what kind of help can I get for the fixing?

I, and I presume @Tgr too, will be happy to look at stuff and offer advice, as I'm doing here. And depending on the extension we might even write the code.

Glancing at https://translatewiki.net/wiki/Special:Version, besides this stuff in Translate it seems like UserMerge's deleting the user might be troublesome (how well does it work now with something like OAuth or CentralAuth?). It might be easier to rename users to names like "Deleted user #12345" and revoke access (e.g. Renameuser and DisableAccount) instead of deleting them.

I hope to have time soon to look through all non-WMF-deployed extensions and make sure there are subtasks of T110291 with useful information on what needs updating, as I did yesterday for the WMF-deployed extensions.

One other thing that could be improved: The FuzzyBot class could use User::newSystemUser() to avoid the possibility that someone somehow managed to create the account before Translate got a chance to create it. Nothing much will break if you don't, though.

Glancing at https://translatewiki.net/wiki/Special:Version, besides this stuff in Translate it seems like UserMerge's deleting the user might be troublesome (how well does it work now with something like OAuth or CentralAuth?). It might be easier to rename users to names like "Deleted user #12345" and revoke access (e.g. Renameuser and DisableAccount) instead of deleting them.

Looking at the code, UserMerge has hooks that should enable it to work together well enough with other extensions. So that's one less thing to worry about, as long as the right hooks are registered it should be workable.

Change 292945 had a related patch set uploaded (by Nikerabbit):
Use User::newSystemUser if available

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

Change 292945 merged by jenkins-bot:
Use User::newSystemUser if available

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

I did not get to work on this during Wikimania. I am catching up now after a vacation and I am planning to start on this next week.

I haven't gotten to this yet. I am still hoping to start this this week.

Nikerabbit added a comment.EditedJul 14 2016, 1:28 PM

Setting $wgDisableAuthManager = false; I get this when trying to log in:

2016-07-14 13:19:23 translatewiki.net dev_translatewiki_net-bw_: [5cb363bda4ba911adc62ae5d] /w/i.php?title=Special:UserLogin&returnto=Special:MainPage   ErrorException from line 401 of /www/dev.translatewiki.net/docroot/w/includes/session/Session.php: PHP Error: Argument 2 passed to hash_pbkdf2() must be an instance of string, bool given
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array, array)
#1 /www/dev.translatewiki.net/docroot/w/includes/session/Session.php(401): hash_pbkdf2(string, boolean, string, integer, integer, boolean)
#2 /www/dev.translatewiki.net/docroot/w/includes/session/Session.php(449): MediaWiki\Session\Session->getSecretKeys()
#3 /www/dev.translatewiki.net/docroot/w/includes/auth/AuthManager.php(2196): MediaWiki\Session\Session->setSecret(string, array)
#4 /www/dev.translatewiki.net/docroot/w/includes/auth/ThrottlePreAuthenticationProvider.php(144): MediaWiki\Auth\AuthManager->setAuthenticationSessionData(string, array)
#5 /www/dev.translatewiki.net/docroot/w/includes/auth/AuthManager.php(312): MediaWiki\Auth\ThrottlePreAuthenticationProvider->testForAuthentication(array)
#6 /www/dev.translatewiki.net/docroot/w/includes/specialpage/AuthManagerSpecialPage.php(354): MediaWiki\Auth\AuthManager->beginAuthentication(array, string)
#7 /www/dev.translatewiki.net/docroot/w/includes/specialpage/AuthManagerSpecialPage.php(483): AuthManagerSpecialPage->performAuthenticationStep(string, array)
#8 /www/dev.translatewiki.net/docroot/w/includes/htmlform/HTMLForm.php(615): AuthManagerSpecialPage->handleFormSubmit(array, VFormHTMLForm)
#9 /www/dev.translatewiki.net/docroot/w/includes/specialpage/AuthManagerSpecialPage.php(417): HTMLForm->trySubmit()
#10 /www/dev.translatewiki.net/docroot/w/includes/specialpage/LoginSignupSpecialPage.php(292): AuthManagerSpecialPage->trySubmit()
#11 /www/dev.translatewiki.net/docroot/w/includes/specialpage/SpecialPage.php(505): LoginSignupSpecialPage->execute(NULL)
#12 /www/dev.translatewiki.net/docroot/w/includes/specialpage/SpecialPageFactory.php(598): SpecialPage->run(NULL)
#13 /www/dev.translatewiki.net/docroot/w/includes/MediaWiki.php(282): SpecialPageFactory::executePath(Title, RequestContext)
#14 /www/dev.translatewiki.net/docroot/w/includes/MediaWiki.php(748): MediaWiki->performRequest()
#15 /www/dev.translatewiki.net/docroot/w/includes/MediaWiki.php(520): MediaWiki->main()
#16 /www/dev.translatewiki.net/docroot/w/index.php(43): MediaWiki->run()
#17 {main}
[Thu Jul 14 15:19:23 2016] [hphp] [9541:7f47d67ff700:30:000001] [] \nCatchable fatal error: Argument 2 passed to hash_pbkdf2() must be an instance of string, bool given in /www/dev.translatewiki.net/docroot/w/includes/session/Session.php on line 401

I don't have $wgSecretKey set. Did someone forgot to add this to release notes and fail gracefully?

I am still wondering wouldn't it be easier to just convert TranslateSandbox::addUser to use AuthManager to create a local user account? We also use InviteSignup so I don't want to unconditionally put all new users into sandbox. Rewriting our form to use action=createaccount looks far from trivial and I don't see what the benefits of that would be.

I don't have $wgSecretKey set. Did someone forgot to add this to release notes and fail gracefully?

DefaultSettings.php
/**
 * This should always be customised in LocalSettings.php
 */
$wgSecretKey = false;

The installer sets it in LocalSettings.php by default, and has since 2008. Before then it was $wgProxyKey (finally removed in 1.24), which was set by the installer since it was added in 2004.

I am still wondering wouldn't it be easier to just convert TranslateSandbox::addUser to use AuthManager to create a local user account?

If you want to go that route, this code could serve as an example. It doesn't work for the purpose it was written for, but should work well enough to replace your TranslateSandbox::addUser code in combination with the general limitations described in T111486#2290234.

FYI translatewiki.net is older than 2008, somewhere around 2005 :). Would be good to mention this now in the release notes when it is really needed and handle it gracefully.

I'll check the code and see how it goes.

It looks like it was back in 1.24 (now in HISTORY).

Other things that use it include TemplateParser, MWCryptRand, MWCryptHKDF (which requires it), and SpecialRunJobs, and also the OAuth, CheckUser, and FlaggedRevs extensions.

It looks like it was back in 1.24 (now in HISTORY).

Where it's one of 30 items listed in the section "Configuration changes", not as a breaking change which will cause fatals.

@Anomie Is this the only task blocking https://gerrit.wikimedia.org/r/#/c/280945/ or are the others? Do you have any ETA in mind for that?

In any case, I will continue working on this this week.

@Anomie Is this the only task blocking https://gerrit.wikimedia.org/r/#/c/280945/ or are the others?

The real blocker there is on translatewiki being able to turn on AuthManager. I don't know of any other blockers for that, but you'd probably know about it better than me.

Do you have any ETA in mind for that?

As soon as translatewiki is ready ;) It would be nice to kill the dead code from master.

In any case, I will continue working on this this week.

Thanks!

https://gerrit.wikimedia.org/r/#/c/300283/ should take care of this. Let me know if I missed some part.

This is being tested on translatewiki.net since Monday afternoon. No issues spotted yet.

2016-08-02 10:58:27 translatewiki.net translatewiki_net-bw_: [4aeff36be271776ce2584cff] /w/api.php   MWException from line 58 of /srv/mediawiki/tags/2016-08-01_11:20:00/extensions/Translate/utils/TranslateSandbox.php: Account creation failed: Visitors to this wiki using your IP address have created 1 account in the last day, which is the maximum allowed in this time period.
As a result, visitors using this IP address cannot create any more accounts at the moment.
#0 /srv/mediawiki/tags/2016-08-01_11:20:00/extensions/Translate/api/ApiTranslateSandbox.php(68): TranslateSandbox::addUser(string, string, string)
#1 /srv/mediawiki/tags/2016-08-01_11:20:00/extensions/Translate/api/ApiTranslateSandbox.php(24): ApiTranslateSandbox->doCreate()
#2 /srv/mediawiki/tags/2016-08-01_11:20:00/includes/api/ApiMain.php(1435): ApiTranslateSandbox->execute()
#3 /srv/mediawiki/tags/2016-08-01_11:20:00/includes/api/ApiMain.php(508): ApiMain->executeAction()
#4 /srv/mediawiki/tags/2016-08-01_11:20:00/includes/api/ApiMain.php(479): ApiMain->executeActionWithErrorHandling()
#5 /srv/mediawiki/tags/2016-08-01_11:20:00/api.php(83): ApiMain->execute()
#6 {main}

I think this is coming from: $wgAccountCreationThrottle = 1;.

Change 302428 had a related patch set uploaded (by Nikerabbit):
Remove $wgAccountCreationThrottle

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

Tgr added a comment.Aug 2 2016, 4:25 PM

The account creation throttle has become more strict in AuthManager; while the pre-authmanager version did throttle for some failed account creation attempts, it ignored most of them, and the current version counts significantly more (e.g. captcha errors). That's a bug.
(A wgAccountCreationThrottle of 1 is probably not reasonable in any case.)

Change 302428 merged by jenkins-bot:
Remove $wgAccountCreationThrottle

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

Tgr added a comment.Aug 3 2016, 12:40 AM

Filed T141953: Account creation throttle should only count successful account creations.

I think the real solution for this use case would be proper dependency injection so that Translate can create its own AuthManager instance (with full control of what kind of providers are included) and use that for programmatic account creation. See T141495: AuthManager should use dependency injection.

Arrbee moved this task from QA to Done on the Language-Q1-2016-17 Sprint 2 board.Aug 3 2016, 6:58 AM

Thanks. So far no other issues have surfaced. I will remove the notice at translatewiki.net about contacting me in case of issues next week.

Nikerabbit closed this task as Resolved.Aug 9 2016, 6:35 AM

It has been running for a week now. I consider this done.

At https://translatewiki.net/w/i.php?title=Special:RecentChanges&translations=filter , under today's newusers log (2:18 CEST), there is an entry "L'utenza 131.175.28.130" or whatever is your current IP. IIRC this was already fixed once.

https://translatewiki.net/wiki/Special:Log/newusers looks okay so probably something to do with RecentChanges. Would you mind opening a new bug?