Page MenuHomePhabricator

Update TranslationNotifications to use AuthManager
Closed, ResolvedPublic


See T110433 and T110291.

Related Objects

Event Timeline

Tgr raised the priority of this task from to Needs Triage.
Tgr updated the task description. (Show Details)
Tgr added subscribers: Aklapper, Tgr.

The whole setup with respect to TranslationNotificationJob posting to other wikis is pretty ugly and I'd recommend replacing it with something else if you can:

  1. Ideally would be done with Echo cross-wiki notifications or something like that.
  2. If you want to keep the bot-like user talk page posts, I'd recommend using an OAuth owner-only consumer.
  3. If you don't want to use OAuth either, I'd recommend you make some sort of TranslationNotification-specific SessionProvider (that would have to be installed on every remote wiki) that uses the HTTP Authorization header in a manner similar to OAuth.
  4. If you insist on continuing to do it with requests to API action=login and action=edit, you'll need to set up BotPasswords logins because action=login isn't guaranteed to continue working for the "main" account password in an AuthManager world.

In any case, that should allow you to get rid of the $user->setPassword() call. Ideally things would even work if you use User::newSystemUser() to properly steal your notification account.

For prioritization @Amire80 is a better person to talk to. According to wikiapiary this extension hardly has any third party users, so making it use Echo instead seems best option.

I don't understand point (4): are you going to break normal login with password via API? If not, we have global account on WMF sites, so we are not creating any users and thus I do not see anything urgent here to fix that would break.

I don't understand point (4): are you going to break normal login with password via API?

Sort of. Logging in using action=login with the main account password will fail under AuthManager in various cases, such as two-factor auth being enabled for the account, or the account needing a password change, or a captcha being required (although that one isn't new). You can hope nothing like these starts applying to your extension's account, but we can't promise it won't.

Bots should use OAuth or BotPasswords (details in this mailing list post), while apps interacting with a human should use action=clientlogin instead which has the ability to instruct the app how to ask for the captcha or second factor or new password (details in this mailing list post). Extensions have a few additional options (detials on now, but the relevant bits are about the same as I wrote in T110766#2287230).

Arrbee triaged this task as High priority.Jun 1 2016, 7:22 AM

Change 292120 had a related patch set uploaded (by Nikerabbit):
Remove publishHere

Change 292121 had a related patch set uploaded (by Nikerabbit):
Update csrf token handling to use modern API

Change 292122 had a related patch set uploaded (by Nikerabbit):
Update login token fetching

I did some cleanups on the code, removing parts that would be problematic with auth manager, review would be welcome.

I also created a bot password. I guess that the name change can be SWAT-deployed and the corresponding password updated at the same time.

By the way, T48310: Deliver notification using Echo where available already exists for using Echo notifications, which I consider to be better long term solution.

Change 292898 had a related patch set uploaded (by Nikerabbit):
Use BotPassword for TNBot

Change 292120 merged by jenkins-bot:
Remove publishHere

Change 292121 merged by jenkins-bot:
Update csrf token handling to use modern API

Change 292122 merged by jenkins-bot:
Update login token fetching

Change 292898 merged by jenkins-bot:
Use bot password for TNBot

Is there anything left to do?

Nikerabbit added a subscriber: Nemo_bis.

@Nemo_bis found a bug about incorrect username when posting to local wiki, which should be fixed by my patch to use same method for local and other wikis. This is to be confirmed after meta is on MW-1.28-release (WMF-deploy-2016-06-07_(1.28.0-wmf.5))

I confirmed that the mentioned bug was fixed on meta.

Let's call this "resolved" then, since T48310 exists for the longer-term solution.

Change 296509 had a related patch set uploaded (by Gergő Tisza):
Remove publishHere

Change 296509 merged by jenkins-bot:
Remove publishHere