MediaWiki r53497 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:r53496‎ | r53497 (on ViewVC)‎ | r53498 >
Date:22:02, 19 July 2009
Author:simetrical
Status:ok (Comments)
Tags:todo 
Comment:
Add experimental new auth framework, ExternalAuth

This should not affect any existing behavior. (Except that it reorders
some error conditions in attemptAutoCreate(), but probably no one cares
about that.) It adds a new database table, but it will be unused unless
you enable external authentication.

An outline of the rationale for this system, and the design planning, is
at <http://www.mediawiki.org/wiki/ExternalAuth&gt;. Essentially,
AuthPlugin puts too much of a burden on plugin authors, requiring them
to write a lot of policy logic instead of just handling the actual
interface to the external user database. This system uses a standard
framework to decide policy questions, and auth plugins only need to
provide some low-level, clearly-specified data.

There are lots of features still missing, marked in the code, but basic
functionality is present. The commit includes initial support for one
type of external authentication, the forum software vBulletin (which I
happen to know well, and want to integrate with my MediaWiki).

I'm encouraging the inclusion of ExternalAuth plugins in core because in
this framework, the amount of code required to add an additional backend
is quite small -- well under 100 lines in this case. I'd hope to see a
lot more of these, and it seems unreasonable to make an armada of tiny
extensions instead of letting them live happily in their own directory
out of everyone's way.
Modified paths:

Diff [purge]

Loading diff…

Follow-up revisions

Rev.Commit summaryAuthorDate
r69912Follow up r53497. $wgExternalAuthConfig -> $wgExternalAuthConf...platonides21:11, 25 July 2010

Comments

#Comment by Brion VIBBER (talk | contribs)   00:19, 22 August 2009

eu_wiki_id is rather confusing; term 'wiki id' refers to the prefix+dbname for cross-wiki referencing in a farm setup. Haven't looked at the rest.

#Comment by Simetrical (talk | contribs)   01:47, 24 August 2009

How about eu_wiki_user and eu_external_user?

#Comment by Simetrical (talk | contribs)   20:31, 24 August 2009

. . . well, I'll wait for further feedback before committing changes, I guess.

#Comment by Werdna (talk | contribs)   18:31, 27 August 2009

Checked for anything that affects our deployment on Wikimedia and found nothing, so marking as OK with TODOs

#Comment by Tim Starling (talk | contribs)   05:03, 3 December 2009

Please rename:

  • prefMessage() to getPrefMessage(), needs verb
  • eu_wiki_id to eu_local_id, unclear
  • link() to linkToLocal() or some such longer name, link means other things in MediaWiki so it needs the longer name to avoid ambiguity.

Please change the schema and the configuration to allow for multiple external user providers, like we do for images with $wgForeignFileRepos. In my opinion, the fact that AuthPlugin couldn't do multiple user databases was its biggest architectural error. It's an error which is easy to avoid, so I'd hate to see it repeated in a rewrite. To support the feature would only need 100 lines or so of glue code, similar to RepoGroup.php.

This might be good enough as it is for Wikimedia deployment, but it's not good enough for a release. As soon as an interface is released, we're stuck with it forever. If you don't feel like working on it, it can be moved out to a branch for now.

#Comment by Simetrical (talk | contribs)   17:57, 3 December 2009

I'll try to find the time to fix this in the next week or two.

#Comment by Simetrical (talk | contribs)   19:51, 8 December 2009

I've done all the renames you've asked for. I haven't allowed multiple auth providers yet because I don't have any clear concept of who would use it, so I'm not sure how to design it. When you try to log in, should the software stop at the first external auth provider that returns true, or try to link the user to multiple accounts? If the user changes preferences, should it bubble up to everywhere the user is linked to? If the user isn't allowed to change local preferences when changing them externally fails ('global' in $wgAllowPrefChange), what if one provider fails but another doesn't? Should users be allowed to be linked to multiple auth sources at all, or is this unnecessary?

I don't think architectural changes would be needed in any event. The core part of the mechanism is very open-ended. If someone wants multiple auth mechanisms, I think the best way to do it would be to just write ExternalUser_Multiple. If it then turns out there's some other way of doing multiple auth sources that someone else wants, you could do some new class without everyone having to work around an awkward configuration format.

#Comment by Tim Starling (talk | contribs)   01:25, 9 December 2009

Thanks for the changes. Try to grab me on IRC when you're next online so we can talk about the architecture.

Status & tagging log