Page MenuHomePhabricator

RollbackPageTest always fails with default $wgRateLimits
Closed, ResolvedPublic

Description

RollbackPageTest::testAuthorize() creates a UserIdentity with a name and ID, but doesn't register it in the database. With $wgRateLimits and $wgAutopromote set as in DefaultSettings.php, RollbackPage::authorizeRollback() calls User::pingLimiter(), leading to a call to User::load(), which resets the user name and ID. Then the cache fetch at the end of UserGroupManager::getUserEffectiveGroups() fails, because $userKey does not match the key used by setCache().

With notices enabled, it fails with "Undefined index: implicit". Without notices disabled, it fails with "TypeError: Return value of MediaWiki\User\UserGroupManager::getUserImplicitGroups() must be of the type array, null returned".

Stack trace showing User::getRegistration() being called from this test case:

#0 /srv/mw/core/includes/user/UserGroupManager.php(534): User->getRegistration()
#1 /srv/mw/core/includes/user/UserGroupManager.php(497): MediaWiki\User\UserGroupManager->checkCondition(array, User)
#2 /srv/mw/core/includes/user/UserGroupManager.php(458): MediaWiki\User\UserGroupManager->recCheckCondition(array, User)
#3 /srv/mw/core/includes/user/UserGroupManager.php(383): MediaWiki\User\UserGroupManager->recCheckCondition(array, User)
#4 /srv/mw/core/includes/user/UserGroupManager.php(271): MediaWiki\User\UserGroupManager->getUserAutopromoteGroups(User)
#5 /srv/mw/core/includes/user/UserGroupManager.php(309): MediaWiki\User\UserGroupManager->getUserImplicitGroups(User, integer, boolean)
#6 /srv/mw/core/includes/Permissions/PermissionManager.php(1385): MediaWiki\User\UserGroupManager->getUserEffectiveGroups(User)
#7 /srv/mw/core/includes/Permissions/PermissionManager.php(1334): MediaWiki\Permissions\PermissionManager->getUserPermissions(User)
#8 /srv/mw/core/includes/Permissions/PermissionManager.php(907): MediaWiki\Permissions\PermissionManager->userHasRight(User, string)
#9 /srv/mw/core/includes/Permissions/PermissionManager.php(460): MediaWiki\Permissions\PermissionManager->checkQuickPermissions(string, User, array, string, boolean, Title)
#10 /srv/mw/core/includes/Permissions/PermissionManager.php(264): MediaWiki\Permissions\PermissionManager->getPermissionErrorsInternal(string, User, Title, string, boolean)
#11 /srv/mw/core/includes/Permissions/UserAuthority.php(265): MediaWiki\Permissions\PermissionManager->userCan(string, User, Title, string)
#12 /srv/mw/core/includes/Permissions/UserAuthority.php(223): MediaWiki\Permissions\UserAuthority->internalCan(string, string, Title, NULL)
#13 /srv/mw/core/includes/user/User.php(4302): MediaWiki\Permissions\UserAuthority->authorizeWrite(string, Title, NULL)
#14 /srv/mw/core/includes/page/WikiPage.php(2085): User->authorizeWrite(string, Title)
#15 /srv/mw/core/includes/page/WikiPage.php(1948): WikiPage->doUserEditContent(WikitextContent, User, CommentStoreComment, integer, boolean, array, integer)
#16 /srv/mw/core/tests/phpunit/MediaWikiIntegrationTestCase.php(1396): WikiPage->doEditContent(WikitextContent, string, integer, boolean, User)
#17 /srv/mw/core/tests/phpunit/MediaWikiIntegrationTestCase.php(431): MediaWikiIntegrationTestCase->addCoreDBData()
#18 /srv/mw/core/vendor/phpunit/phpunit/src/Framework/TestSuite.php(627): MediaWikiIntegrationTestCase->run(PHPUnit\Framework\TestResult)
#19 /srv/mw/core/vendor/phpunit/phpunit/src/Framework/TestSuite.php(627): PHPUnit\Framework\TestSuite->run(PHPUnit\Framework\TestResult)
#20 /srv/mw/core/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(656): PHPUnit\Framework\TestSuite->run(PHPUnit\Framework\TestResult)
#21 /srv/mw/core/vendor/phpunit/phpunit/src/TextUI/Command.php(236): PHPUnit\TextUI\TestRunner->doRun(PHPUnit\Framework\TestSuite, array, array, boolean)
#22 /srv/mw/core/tests/phpunit/phpunit.php(141): PHPUnit\TextUI\Command->run(array, boolean)
#23 /srv/mw/core/tests/phpunit/phpunit.php(204): PHPUnitMaintClass->execute()
#24 {main}

Suggestions:

  • UserGroupManager::setCache() should take a cache key as a parameter
  • RollbackPageTest is an integration test with @group database -- it could just use a real registered user
  • The rate limit hack in DevelopmentSettings.php (T225796) could hide production-visible errors, since it causes a substantial amount of complex code to be missed during tests. Ideally rate limits should be infinite, not disabled.

Event Timeline

The dev setting, I believe, was put there it aid browser tests, not PHPUnit tests. Afaik, PHPUnit tests either don't trip the limit or have sufficient resets already (That is, they've run for years without being disabled.)

I suppose one way could be to set the relevant config in the setup of this PHPUnit test as we do with other configuration already, to avoid variance from local settings. (Afaik the only reason we load local settings is for DB settings and maybe a small set of other environment type things that need have non trivial values filled; but that's an issue for another time).

Or perhaps add it to the shared TestSetup for all PHPUnit tests among other config globals we set there.

The dev setting, I believe, was put there it aid browser tests, not PHPUnit tests. Afaik, PHPUnit tests either don't trip the limit or have sufficient resets already (That is, they've run for years without being disabled.)

Right, but Tim's suggestion with semi-infinite limits should keep the tests passing and make it exercise more code. browser tests are integration, so I think I'll try after fixing RollbackPageTest itself.

Change 699419 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] UserGroupManager: consestent cache key withing method calls

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

Change 699449 had a related patch set uploaded (by Ppchelko; author: Ppchelko):

[mediawiki/core@master] Don't switch off rate limits in dev settings.

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

Pchelolo claimed this task.

Change 699419 merged by jenkins-bot:

[mediawiki/core@master] UserGroupManager: Use a consistent cache key within method calls

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

Change 699449 merged by jenkins-bot:

[mediawiki/core@master] DevelopmentSettings: Don't disable rate limits but use very high ones

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