3 related log entries

[{reqId}] {exception_url}   PHP Deprecated: Returning a string from a hook handler is deprecated since MediaWiki 1.35 ' .
							'(done by LdapAuthenticationHooks::onBlockIpComplete for BlockIpComplete)

from /srv/mediawiki/php-1.39.0-wmf.6/includes/debug/MWDebug.php(377)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
#1 /srv/mediawiki/php-1.39.0-wmf.6/includes/debug/MWDebug.php(377): trigger_error(string, integer)
#2 /srv/mediawiki/php-1.39.0-wmf.6/includes/debug/MWDebug.php(353): MWDebug::sendRawDeprecated(string, boolean, string)
#3 /srv/mediawiki/php-1.39.0-wmf.6/includes/GlobalFunctions.php(1038): MWDebug::deprecatedMsg(string, string, string, boolean)
#4 /srv/mediawiki/php-1.39.0-wmf.6/includes/HookContainer/HookContainer.php(149): wfDeprecatedMsg(string, string, boolean, boolean)
#5 /srv/mediawiki/php-1.39.0-wmf.6/includes/HookContainer/HookRunner.php(1033): MediaWiki\HookContainer\HookContainer->run(string, array)
#6 /srv/mediawiki/php-1.39.0-wmf.6/includes/block/BlockUser.php(603): MediaWiki\HookContainer\HookRunner->onBlockIpComplete(MediaWiki\Block\DatabaseBlock, User, NULL)
#7 /srv/mediawiki/php-1.39.0-wmf.6/includes/block/BlockUser.php(531): MediaWiki\Block\BlockUser->placeBlockInternal(boolean)
#8 /srv/mediawiki/php-1.39.0-wmf.6/includes/block/BlockUser.php(465): MediaWiki\Block\BlockUser->placeBlockUnsafe(boolean)
#9 /srv/mediawiki/php-1.39.0-wmf.6/includes/specials/SpecialBlock.php(912): MediaWiki\Block\BlockUser->placeBlock(boolean)
#10 /srv/mediawiki/php-1.39.0-wmf.6/includes/specials/SpecialBlock.php(1026): SpecialBlock::processFormInternal(array, User, MediaWiki\Block\UserBlockCommandFactory, MediaWiki\Block\BlockUtils)
#11 /srv/mediawiki/php-1.39.0-wmf.6/includes/htmlform/HTMLForm.php(726): SpecialBlock->onSubmit(array, OOUIHTMLForm)
#12 /srv/mediawiki/php-1.39.0-wmf.6/includes/htmlform/HTMLForm.php(616): HTMLForm->trySubmit()
#13 /srv/mediawiki/php-1.39.0-wmf.6/includes/htmlform/HTMLForm.php(632): HTMLForm->tryAuthorizedSubmit()
#14 /srv/mediawiki/php-1.39.0-wmf.6/includes/specialpage/FormSpecialPage.php(209): HTMLForm->show()
#15 /srv/mediawiki/php-1.39.0-wmf.6/includes/specialpage/SpecialPage.php(671): FormSpecialPage->execute(string)
#16 /srv/mediawiki/php-1.39.0-wmf.6/includes/specialpage/SpecialPageFactory.php(1382): SpecialPage->run(string)
#17 /srv/mediawiki/php-1.39.0-wmf.6/includes/MediaWiki.php(315): MediaWiki\SpecialPage\SpecialPageFactory->executePath(string, RequestContext)
#18 /srv/mediawiki/php-1.39.0-wmf.6/includes/MediaWiki.php(911): MediaWiki->performRequest()
#19 /srv/mediawiki/php-1.39.0-wmf.6/includes/MediaWiki.php(565): MediaWiki->main()
#20 /srv/mediawiki/php-1.39.0-wmf.6/index.php(50): MediaWiki->run()
#21 /srv/mediawiki/php-1.39.0-wmf.6/index.php(46): wfIndexMain()
#22 /srv/mediawiki/w/index.php(3): require(string)
#23 {main}

Gerrit block of miraheze funny! failed with status 404

Phab user.ldapquery error '{"result":null,"error_code":"ERR-INVALID-PARAMETER","error_info":"Unknown or missing ldap names: Miraheze funny!"}'

The issue seems to be that the user is in the labswiki user table, but not in the LDAP directory. I have searched with wildcards and cannot find a user with cn=Miraheze funny!. That seems to be because the account was created by the now blocked 'Miraheze is the worst thing ever invented' user. I thought we had a better bug for problems with that process, but T178417: New e-mail-created wikitechwiki user "Per Magnus" can't set their password is all i can find. TL;DR MediaWiki-extensions-LdapAuthentication is broken for the Special:CreateAccount workflow where a logged in user creates an account for another user.

In T305786#7843027, @Zabe wrote:

[{reqId}] {exception_url}   PHP Deprecated: Returning a string from a hook handler is deprecated since MediaWiki 1.35 ' .
							'(done by LdapAuthenticationHooks::onBlockIpComplete for BlockIpComplete)

Blocking the next train as per policy on hard deprecation.

Also given a new fatal error for something that seems quite important to various workflow, probably also important by itself to fix soon, but that's outside the train policy and upto the relevant teams.

In T305786#7855575, @Krinkle wrote:

Also given a new fatal error for something that seems quite important to various workflow, probably also important by itself to fix soon, but that's outside the train policy and upto the relevant teams.

The fatal is caused by bad data which in turn was caused by vandal doing a vandal thing. I don't think there is anything actionable here honestly to address that. Wikitech is pretty much the only MediaWiki-extensions-LdapAuthentication consumer. The Enterprise MediaWiki folks who are the larger target of LDAP authentication have moved on to MediaWiki-extensions-LDAPAuthentication2 and MediaWiki-extensions-Pluggable-Auth. Wikitech is left behind mostly because of MediaWiki-extensions-OpenStackManager and our lack of resourcing/collective will to do the work needed to get to T161859: Make Wikitech an SUL wiki.

I think the main actionable here is that the hook handlers should not be returning strings. Instead, the errors should be logged and the hooks shouldn't return anything; returning strings was hard-deprecated in 2020 (unrelated, the git blame took me a while because r605054 was on the way and I spent some time laughing out loud).

Change 781061 had a related patch set uploaded (by BryanDavis; author: Bryan Davis):

[mediawiki/extensions/LdapAuthentication@master] Hooks: return false rather than strings on failure

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

Change 781061 merged by jenkins-bot:

[mediawiki/extensions/LdapAuthentication@master] Hooks: return false rather than strings on failure

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

Is there anything else that should be done here?

Change 783917 had a related patch set uploaded (by Jforrester; author: Bryan Davis):

[mediawiki/extensions/LdapAuthentication@wmf/1.39.0-wmf.8] Hooks: return false rather than strings on failure

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

Change 783917 merged by jenkins-bot:

[mediawiki/extensions/LdapAuthentication@wmf/1.39.0-wmf.8] Hooks: return false rather than strings on failure

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

Mentioned in SAL (#wikimedia-operations) [2022-04-19T21:41:15Z] <jhuneidi@deploy1002> Synchronized php-1.39.0-wmf.8/extensions/LdapAuthentication/includes/LdapAuthenticationHooks.php: Backport: [[gerrit:783917|Hooks: return false rather than strings on failure (T305786)]] (duration: 01m 30s)

The behavior identical to the old one (minus the deprecation warning) would be for setLdapLockStatus to just throw an exception instead of returning a string. Pro: makes it obvious to the user that the block has failed (which might be important if LdapAuthentication disabling the blocked account has security implications, like someone retaining access they shouldn't have to internal company data). Con: it prevents other hooks from running. But then, returning false also prevents other hooks from running so if you do that, I think you are better off with the exception.

EDIT: oops, see Gergo's comment above.

No longer a train blocker, but from talking to @Tgr, the hook should not fail silently and should return an error. Ex of previous behavior: https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/99a84628f422b28739cf2ff1d42ef43f0f38fe5b/includes/Hooks.php#210

That is a different file so I'm not exactly sure how it needs to be handled/what changes need to happen.