Page MenuHomePhabricator

Refactoring to make external authentication and identity systems easier
Open, LowPublic

Description

Author: jernst+wikimedia.org

Description:
Nothing is broken, per se, but I'll file this bug anyway because there are a
number of generally quite very minor things that could be done in order to make
it much easier for people to plug in external authentication and identity
systems. This is a collection of the issues we've seen when working to let
MediaWiki recognize LID ( http://lid.netmesh.org/ ) Light-Weight Digital Identities.

While the AuthPlugin is a good start, to integrate with some identity schemes it
may be necessary or advantageous to subclass, or entirely replace class User
with some other class that can capture more, or different information about a
user while presenting the same interface to the rest of MediaWiki. It may also
be that structurally or behaviorally different kinds of User need to be
supported. Currently, class User is kind of a catch-all, with different
execution paths depending on whether the User is anonymous, registered and/or
only known by an IP address.

So here are the issues:

  1. much code using class User continues to distinguish anonymous from known

users by looking at the user id (comparing against 0). Appropriate methods that
abstract away from that exist already (isAnon(), isLoggedIn()). They should be
used everywhere instead of comparing against the user id.

  1. We need an extension point, similar to the AuthPlugin extension point that

can be configured from LocalSettings, that allows a developer to use a different
class than User to instantiate $wgUser. Given the code is currently structured,
it does not appear to be possible to do this from within LocalSettings.php (one
of the reasons: an alternate implementation of User is most likely going to need
setup information that is not available yet at the time LocalSettings is
processed). The best place appears to be right where $wgUser is assigned for the
first time in includes/Setup.php. Maybe one could create a new global setting
called $wgUserClass, which could be require_once'd, if given, so developers
could easily put whatever $wgUser instantiation code there that they need. That
would be invoked from Setup.php.

  1. While not strictly required, I would strongly recommend to take a hard look

at User.php and refactor it into separate classes, such as AbstractUser,
AnonymousUser, and RegisteredUser. It may also make sense to separate the class
instantiated for the owner of the current session ($wgUser) from the class
instantiated for other users who are not currently the owner of the session
(e.g. in the various SpecialXXX pages) Following various execution paths through
User.php, it appears that this would substantially increase code readability
while likely reducing memory consumption. This would also produce a clear avenue
for implementors to provide other kinds of User classes. Unfortunately, this is
fairly difficult to do for an outsider like myself because it is exceedingly
time-consuming trying to figure out which aspects of User.php are really needed
for which use case. It does seem to carry a lot of baggage.

  1. It would be nice if methods that are really statics didn't use the $this

pointer and instead did something like User::method(). That way, alternate
implementations of the User class can at least delegate to some of the code in
User.php instead of having to copy it.

For some of what I'm saying here I can create (LID-independent or LID-dependent,
as desired) patches assuming they are of interest. If so, what should I do with
them?


Version: 1.5.x
Severity: normal

Details

Reference
bz3709

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 8:52 PM
bzimport set Reference to bz3709.
bzimport added a subscriber: Unknown Object (MLST).

jernst+wikimedia.org wrote:

  1. There should be a "equals" method on class User (or AbstractUser) so

comparisons can be performed between objects and do not need to resort to
comparing the id attribute directly.

jernst+wikimedia.org wrote:

Patch for issues #1 and #2 mentioned in this bug

Attached:

Couple of quick notes:

  1. We really should use isAnon()/isLoggedIn() methods instead of just checking getId() > 0. Patch will need some tweaking, but it's got the right idea.
  2. Subclassing User for external auth systems is now possible. Simetrical added this ability in r53497.
  3. I kinda like the idea of making User more modular (abstract User, AnonUser and RegisteredUser extend). Might be worth poking.

*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*

sumanah wrote:

Johannes Ernst, thanks for your patch. The codebase has changed substantially since you submitted your patch; if you have time and interest, please come into MediaWiki-General on freenode IRC to discuss how to continue working on this issue. Thanks!

T89459 may be a duplicate of this report.

The AuthManager project is related to this feature request but attacks the problem from a slightly different direction. Rather than providing a mechanism for subclassing and extending MediaWiki's User class, AuthManager will enable a PrimaryAuthenticationProvider implementation to track and manage the mapping from external user entities to User instances via the "link" account creation type. (https://www.mediawiki.org/wiki/Requests_for_comment/AuthManager#PrimaryAuthenticationProvider). I think this should alleviate the need for items 2-5 of this feature request by encapsulating these differences inside the PrimaryAuthenticationProvider implementation.