Page MenuHomePhabricator

UserLoadFromSession considered evil
Closed, ResolvedPublic

Description

Running code from CentralAuth, AbuseFilter, TitleBlacklist etc. which collectively call half the codebase does not seem like a good thing to be doing while the main context user is half-initialised and has lots of methods which will fail horribly if you try to call them (e.g. T43198).

Perhaps initialisation of the User object from the session can be moved to a function called from Setup.php, such as RequestContext::getUser(). It's not lazy-loaded anyway, User::newFromSession() has always been called unconditionally. Then CentralAuth (and anything else that uses the UserLoadFromSession hook) can be called without User::load() in its call stack.


Version: unspecified
Severity: normal
See Also: T60731

Details

Reference
bz41201

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 1:01 AM
bzimport set Reference to bz41201.
bzimport added a subscriber: Unknown Object (MLST).

Looking at WMF-deployed extensions, I see that OAuth and CentralAuth use this hook.

OAuth checks the request's Title object to avoid running on Special:OAuth itself (Special:OAuth needs to do special stuff). To be able to call UserLoadFromSession in Setup.php, we'd either have to change this check or create Title in Setup.php too, before the $wgExtensionFunctions hooks.

CentralAuth doesn't seem to directly do anything that would blow up if called from Setup.php. But it might call the AbortAutoAccount and AuthPluginAutoCreate hooks, which might have the same sort of expectations of being called after Setup.php as OAuth.

mmodell raised the priority of this task from Normal to High.Feb 14 2015, 1:42 AM
mmodell added a subscriber: mmodell.

priority: high because this is batshit insane and can't be justified.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 18 2015, 7:24 PM
Tgr updated the task description. (Show Details)EditedAug 27 2015, 6:27 AM
Tgr added a subscriber: Tgr.

AuthManager deprecates UserLoadFromSession. We'll see if the new code is less evil.

Coming up with documentation standards for "this method might be called in a pre-auth context" sounds like a good idea.

Tgr closed this task as Resolved.Apr 17 2016, 8:02 AM
Tgr claimed this task.

Fixed a while ago in SessionManager; See Setup.php L715-780 and L833-843.