Page MenuHomePhabricator

OATHAuth doesn't work in MediaWiki 1.31
Closed, ResolvedPublic

Description

Hi folks,

I've just managed to drag a wiki of mine kicking and screaming up to 1.31 (don't worry, I plan to get fuly up to date). Was hoping to enable the bundled OATHAuth extension for two factor auth, but it doesn't appear to work out-of-the-box with 1.31.

Reproduction environment:

  1. Clone the MediaWiki repo
  2. Checkout REL1_31
  3. Checkout all submodules: git submodule init; git submodule update
  4. Backport the docker runtime: git checkout REL1_35 -- docker-compose.yml
  5. Bring up the docker-compose environment: docker-compose up -d
  6. Install composer dependencies:
    1. docker-compose exec mediawiki composer update
    2. for d in $(find ./extensions -name 'composer.json'); do docker-compose exec mediawiki composer install --no-dev --working-dir "$(dirname $d)"; done
  7. Install MediaWiki: mkdir cache/sqlite; docker-compose exec mediawiki /bin/bash /docker/install.sh
  8. Enable Vector skin: echo "wfLoadSkin( 'Vector' );" >> LocalSettings.php
  9. Enable OATHAuth: echo "wfLoadExtension( 'OATHAuth' );" >> LocalSettings.php
  10. Run the update script: docker-compose exec mediawiki php maintenance/update.php

Reproduction steps:

  1. Log into admin account
  2. Open Preferences
  3. Enable two-factor authentication
    1. Save into 1Password or alternate two factor app
  4. Log out
  5. Log back in
  6. Enter two factor token

Expected result: Be logged into the account

Actual: MediaWiki exception

[c3b33b516e3b57b3c6c4f6f7] /index.php?title=Special:UserLogin&returnto=Special:Preferences TypeError from line 92 of /var/www/html/includes/user/CentralIdLookup.php: Return value of CentralIdLookup::factoryNonLocal() must be an instance of CentralIdLookup, null returned

Backtrace:

#0 /var/www/html/includes/user/User.php(2114): CentralIdLookup::factoryNonLocal()
#1 /var/www/html/extensions/OATHAuth/includes/auth/TOTPSecondaryAuthenticationProvider.php(93): User->pingLimiter(string, integer)
#2 /var/www/html/includes/auth/AuthManager.php(646): TOTPSecondaryAuthenticationProvider->continueSecondaryAuthentication(User, array)
#3 /var/www/html/includes/specialpage/AuthManagerSpecialPage.php(355): MediaWiki\Auth\AuthManager->continueAuthentication(array)
#4 /var/www/html/includes/specialpage/AuthManagerSpecialPage.php(482): AuthManagerSpecialPage->performAuthenticationStep(string, array)
#5 [internal function]: AuthManagerSpecialPage->handleFormSubmit(array, VFormHTMLForm)
#6 /var/www/html/includes/htmlform/HTMLForm.php(660): call_user_func(array, array, VFormHTMLForm)
#7 /var/www/html/includes/specialpage/AuthManagerSpecialPage.php(416): HTMLForm->trySubmit()
#8 /var/www/html/includes/specialpage/LoginSignupSpecialPage.php(317): AuthManagerSpecialPage->trySubmit()
#9 /var/www/html/includes/specialpage/SpecialPage.php(565): LoginSignupSpecialPage->execute(NULL)
#10 /var/www/html/includes/specialpage/SpecialPageFactory.php(568): SpecialPage->run(NULL)
#11 /var/www/html/includes/MediaWiki.php(288): SpecialPageFactory::executePath(Title, RequestContext)
#12 /var/www/html/includes/MediaWiki.php(818): MediaWiki->performRequest()
#13 /var/www/html/includes/MediaWiki.php(524): MediaWiki->main()
#14 /var/www/html/index.php(42): MediaWiki->run()
#15 {main}

Testing across releases::

VersionGit refTest meets expectations?
MediaWiki 1.30.05cfc9acYes
MediaWiki 1.30.1a4c8065(Untested)
MediaWiki 1.30.25951e3e(Untested)
MediaWiki 1.30.344ef9dd(Untested)
MediaWiki 1.30.4171d96b(Untested)
MediaWiki 1.30.5152cbbfYes
MediaWiki 1.30.60441c81(Untested)
MediaWiki 1.30.70bea246(Untested)
MediaWiki 1.30.8c759d64Yes
MediaWiki 1.30.965f4875No
MediaWiki 1.30.100fa7240No

Bisecting between 1.30.8 (c759d64) and 1.30.9 (65f4875) suggests the breakage was introduced in 4ec92246b02f61672946d6ea199b47cb87eb7fe4

(Apologies if I've made any mistakes or missed anything in the bug reporting procedure! First time flagging something with y'all)

Event Timeline

Change 633498 had a related patch set uploaded (by Urbanecm; owner: Urbanecm):
[mediawiki/core@REL1_31] Remove typehint from CentralIdLookup::factoryNonLocal

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

Urbanecm triaged this task as Medium priority.
Urbanecm added a subscriber: Urbanecm.

This is caused by CentralIdLookup::factoryNonLocal() having a non-nullable return typehint, while the function can actually return null. Because nullable return typehints were introduced in 7.0, we need to remove the whole typehint.

@Rjacksonm1 Thanks for your thorough bug report - it helped to locate the error quickly. I've uploaded a fix patch :).

Change 633258 had a related patch set uploaded (by Urbanecm; owner: DannyS712):
[mediawiki/core@REL1_31] CentralIdLookup::factoryNonLocal can return null

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

Change 633498 abandoned by Urbanecm:
[mediawiki/core@REL1_31] Remove typehint from CentralIdLookup::factoryNonLocal

Reason:
dupe of https://gerrit.wikimedia.org/r/c/mediawiki/core/ /633258

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

Change 633258 merged by jenkins-bot:
[mediawiki/core@REL1_31] CentralIdLookup::factoryNonLocal can return null

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

@Rjacksonm1 this should be solved in the next release of mediawiki 1.31

Ooh magic, thanks for the super quick turnaround! Great work :)