Page MenuHomePhabricator

MassMessage::getMessengerUser() takeover broken due to Password API changes
Closed, ResolvedPublic

Description

In MassMessage::getMessengerUser(), we do some evil things to create a system account that cannot be logged into:

$user = User::newFromName( $wgMassMessageAccountUsername );
$user->load();
if ( $user->getId() && $user->mPassword == '' && $user->mNewpassword == '' ) {
// We've already stolen the account
return $user;
}

Problems here are:

a) User::load() no longer loads the password members, so we can't check if they are equal to empty string.
b) null == '', should have been using triple equals.

Also, AbuseFilter has a very similar function (I got the idea from it), so we'll need to patch this there too.


Version: unspecified
Severity: major

Details

Reference
bz68843

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.
bzimport set Reference to bz68843.
bzimport added a subscriber: Unknown Object (MLST).
Legoktm created this task.Jul 30 2014, 5:01 AM

From my reading of User::setInternalPassword(), an InvalidPassword-type password is saved for the user. But I don't see any non-hacky way for me to check whether a user has that password type. Am I missing something?

:/ is there a reason an actual account is being created and registered (as opposed to just adding the username to $wgReservedUsernames)?

(In reply to Tyler Romeo from comment #2)

:/ is there a reason an actual account is being created and registered (as
opposed to just adding the username to $wgReservedUsernames)?

Yes. We do this so local wikis can adjust the accounts' user groups if they wish to (some remove it from the "bot" group it auto-adds itself to), or if they want to block the account for whatever reason.

The password API changes in question were https://gerrit.wikimedia.org/r/#/c/77645/

The breaking test has been temporarily commented out in https://gerrit.wikimedia.org/r/#/c/150770/

Ideas on how to fix...

  • Have User::load() call loadPasswords() - fixes all back-compat issues
  • Make loadPasswords() public
  • Add getPassword()/getTemporaryPassword() accessors and make the member variables private
  • Have MM call checkPassword() with a dummy value and then check ->mPassword directly (ewwww)

(In reply to Kunal Mehta (Legoktm) from comment #5)

Ideas on how to fix...

  • Have User::load() call loadPasswords() - fixes all back-compat issues

Would rather not, for the same reason load() does not call loadOptions()

  • Make loadPasswords() public

This makes sense, and I'm not even sure why I didn't make it public in the first place.

  • Add getPassword()/getTemporaryPassword() accessors and make the member variables private

This also makes sense. And while we're at it, also make $mId, $mName, $mRealName, etc. private since you shouldn't be accessing those directly either.

  • Have MM call checkPassword() with a dummy value and then check ->mPassword directly (ewwww)

ewwww

Agreed. Stick with above.

(In reply to Tyler Romeo from comment #6)

(In reply to Kunal Mehta (Legoktm) from comment #5)
> * Make loadPasswords() public

This makes sense, and I'm not even sure why I didn't make it public in the
first place.

If we add the accessors, there should be no reason to do this I think.

> * Add getPassword()/getTemporaryPassword() accessors and make the member
> variables private

This also makes sense. And while we're at it, also make $mId, $mName,
$mRealName, etc. private since you shouldn't be accessing those directly
either.

Ib79ce01a47f90af681e376ce918eda559b4b94a6. Will do a second patch once extensions accessing directly are fixed (MassMessage, AbuseFilter, OpenID).

Change 151570 had a related patch set uploaded by Legoktm:
Fix MassMessage::getMessengerUser() after Password API changes

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

Change 151570 merged by jenkins-bot:
Fix MassMessage::getMessengerUser() after Password API changes

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

Change 153042 had a related patch set uploaded by Legoktm:
Fix MassMessage::getMessengerUser() after Password API changes

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

Change 153042 merged by jenkins-bot:
Fix MassMessage::getMessengerUser() after Password API changes

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

Deployed by Ori, thanks!

Change 154449 had a related patch set uploaded by Wctaiwan:
Fix MassMessage::getMessengerUser() after Password API changes

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

Change 154449 merged by jenkins-bot:
Fix MassMessage::getMessengerUser() after Password API changes

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