Page MenuHomePhabricator

Audit extensions for use of $wgAuth
Closed, ResolvedPublic

Description

We need to know if we need to wrap AuthManager up in an AuthPlugin shim and if so what that shim needs to be able to accomplish.

Related Objects

Event Timeline

bd808 raised the priority of this task from to Medium.
bd808 updated the task description. (Show Details)
bd808 added subscribers: Anomie, csteipp, Legoktm and 2 others.

AuthPlugin-implementing extensions:

  • LdapAuthentication
  • CentralAuth
  • Auth_remoteuser (not WMF deployed)
  • MediaWikiAuth (not WMF deployed, evil)
  • GoogleLogin (not WMF deployed)

WMF-deployed extensions:

  • Echo calls ->allowPropChange( 'emailaddress' )
    • Seems likely to be easy, although it's not something we touched on at all in the RFC.
  • MoodBar calls ->allowPropChange( 'emailaddress' )
    • Seems likely to be easy, although it's not something we touched on at all in the RFC.
  • CheckUser calls ->getUserInstance(), and on the AuthPluginUser ->getId()
    • Basically assumes CentralAuth, it's trying to decide whether to display "Not unified" in its UI. AuthManager will probably need a "userExists( $name )" method, but to support this it would have to return which primary AuthenticationProviders say "yes" and CheckUser would have to decide which should trigger this message.
  • OpenStackManager assumes $wgAuth is provided by LdapAuthentication
    • Maybe not as painful as it seems, since it doesn't call any AuthPlugin methods. We may just be able to replace its $wgAuth access with something that fetches the (primary) AuthenticationProvider for the session and checks if it's Ldap's, and have Ldap provide the same interface on its AuthenticationProvider.
  • UserDailyContribs calls ->userExists()
    • Another CentralAuth assumption, but behind a config toggle. Not sure why it's calling $wgAuth rather than User::isLoggedIn() on the User object it already has. @Reedy added it, and @Krinkle seemed to know the reason when adding the config toggle.
  • Renameuser calls ->getUserInstance(), ->updateExternalDB(), and AuthPluginUser ->resetAuthToken()
    • "resetAuthToken" is implied in bullet 4 here.
    • The other bit is basically "hey, user data changed, if you care", which Ldap uses to update the Ldap info with the new email, nickname, realname, and language. Not really sure whether that event belongs in AuthManager or would be better as a hook, really.

Non-WMF-deployed extensions:

  • OpenID calls ->allowPasswordChange(), ->initUser(), and ->updateUser().
    • "allowPasswordChange" is for showing a password reset link in a few places. Not clear what exactly.
    • The other two are because it's implementing its own version of the "Attempted login with no local user" flow.
  • Configure notes it exists.
    • Probably just delete it.
  • ConfirmAccount calls ->userExists(), ->addUser()
    • Probably wants a rewrite as a SecondaryAuthenticationProvider.
  • AjaxLogin calls ->allowPasswordChange()
    • No idea why it's implementing its own API endpoint for login instead of using core's action=login.
    • The wgAuth call is for deciding to show/allow resetting passwords via its custom UI.
  • SemanticSignup calls ->validDomain(), ->canCreateAccounts(), ->userExists(), ->authenticate(), ->allowPasswordChange(), ->modifyUITemplate(), ->addUser(), ->initUser()
    • NFI. Probably completely reimplementing Special:UserLogin, and should gather its info in a SecondaryAuthenticationProvider.
  • TwitterLogin calls ->allowPasswordChange()

An evaluation of AuthPlugin methods:

  • AuthPlugin::userExists( $username )
    • AuthManager will probably need this. Do we also want to support querying which AuthenticationProviders say "yes"?
  • AuthPlugin::authenticate( $username, $password )
    • Not as-is, but in a version that takes a set of AuthenticationRequest instances.
  • AuthPlugin::modifyUITemplate( &$eew, &$wtf )
    • No, kill it. UI specification is (indirectly) via AuthenticationRequest types.
  • AuthPlugin::setDomain( $domain )
  • AuthPlugin::getDomain()
  • AuthPlugin::validDomain( $domain )
  • AuthPlugin::domainList()
    • These likely belong in LdapAuthenticationRequest somehow, or DomainUserPasswordAuthenticationRequest if we think something besides Ldap might have a use for it.
  • AuthPlugin::updateUser( &$user )
    • The intent here seems to be "This user just logged in"; Ldap and CentralAuth use it to set user data (email, etc) from their global databases, and CentralAuth also replaces $user entirely on migration. This seems more likely as a hook to me.
  • AuthPlugin::initUser( &$user )
    • Same, but for local account creation rather than login. This also seems more likely as a hook to me.
  • AuthPlugin::updateExternalDB( $user )
  • AuthPlugin::updateExternalDBGroups( $user, $addgroups, $delgroups )
    • Counterpart to AuthPlugin::updateUser() for user data being updated. The idea is that the plugin should write its global database. This also seems more likely as a hook to me.
  • AuthPlugin::autoCreate()
    • Not sure whether AuthManager actually needs this, versus the AuthenticationProvider just returning "PASS, no local user, local username should be $X" versus "PASS, no local user, no suggestion for a local username".
  • AuthPlugin::allowPropChange( $prop )
  • AuthPlugin::allowPasswordChange()
    • Ldap might deny password changes, and Auth_remoteuser tries to deny realname, email, and password.
    • I suppose we probably need something along these lines to support backends that simply cannot change certain properties. I'd just have allowPropChange( 'password' ) rather than a separate method though.
  • AuthPlugin::allowSetLocalPassword()
  • AuthPlugin::strict()
    • Makes no sense in AuthManager, "local" is just another AuthenticationProvider.
  • AuthPlugin::strictUserAuth( $username )
    • "FAIL" rather than "ABSTAIN"
  • AuthPlugin::setPassword( $user, $password )
    • Should probably be replaced with something that takes an AuthenticationRequest and tells the AuthenticationProvider to set data rather than validate it.
  • AuthPlugin::canCreateAccounts()
    • We have 'link' and 'create' type AuthenticationProviders, probably we just need to add 'no'.
  • AuthPlugin::addUser( $user, $password, $email, $realname )
    • In AuthManager, this would take a $user and an AuthenticationRequest instance instead, and probably pick up $email and $realname from whatever replaces AuthPlugin::updateExternalDB.
  • AuthPlugin::getCanonicalName( $username )
    • What happens if multiple things have different ideas on what makes a good "canonical" name?
  • AuthPlugin::getUserInstance( &$user )
    • The concept of AuthPluginUser is probably ok (except kill ->getId()), but the naming and CentralAuth's implementation are awful.

Created T110291 and T110282 based on the information here. Is this task resolved?

What about extensions which mess with user data (e.g. EditAccount)? Can we generate a list of them?

Opened T110412 as a more general audit task to avoid spamming this one.

What was the search method? Grepping in the mediawiki/extensions directory? Do we ignore non-Wikimedia-hosted extensions? (Seems reasonable to me.)

In T91303#1577347, @Tgr wrote:

What was the search method? Grepping in the mediawiki/extensions directory? Do we ignore non-Wikimedia-hosted extensions? (Seems reasonable to me.)

Yes, just gerrit hosted extensions were audited. I'm sure there are some (many?) AuthPlugins in the known universe that this doesn't cover, but we can't grep what we can't see. :)

mediawiki.org categories could be used to find some non-wikimedia-hosted extensions, although that of course depends on correct self-reporting (the AuthPluginSetup category for example is not exactly flooded with extensions). For T110414 and T110431 checking the categories would be generally straightforward; and one can google, and search on GitHub and whatnot. (Just by searching for "authplugin" on mediawiki.org I could find a long list of things: PwAuthPlugin, IlchAuth, AuthIMAP, DNNAuthentication, CrowdAuthentication, Siteminder Authentication, AuthPOP3, Shibboleth Authentication, NTLMActiveDirectory, Wget Authentication, AuthSymfony, PHP-Fusion Auth, SAMLAuth, Facebook, AuthDrupal, QISSingleSignOn.) I was just wondering if it is worth the effort; I guess it's not.

Anomie claimed this task.

Done, tasks for the extensions in Gerrit are filed.

In T91303#1578718, @Tgr wrote:

mediawiki.org categories could be used to find some non-wikimedia-hosted extensions

Was https://www.mediawiki.org/wiki/Category:User_identity_extensions inspected?

No. AuthManager did get an AuthPlugin shim though, so not sure what that would have added.

In T91303#1578718, @Tgr wrote:

mediawiki.org categories could be used to find some non-wikimedia-hosted extensions

Was https://www.mediawiki.org/wiki/Category:User_identity_extensions inspected?

Extensions hosted in Gerrit (i.e. mediawiki/extensions/*) were audited. Extensions that might have pages in that category were not audited if they were not hosted in Gerrit.