Page MenuHomePhabricator

TemporaryPasswordPrimaryAuthenticationProvider does not work with non-DB-based passwords
Open, Needs TriagePublic

Description

TemporaryPasswordPrimaryAuthenticationProvider assumes passwords are stored in the users table; it does not work together with e.g. LdapAuthentiaction.

Event Timeline

Some options:

  • Every password-based primary authentication provider has to reimplement temporary password handling (possibly using an abstract base class that we provide). Cumbersome, will not work sanely when a site uses multiple password-based authentication methods (e.g. both LDAP and local password).
  • Inject some sort of password getter/setter service into TemporaryPasswordPrimaryAuthenticationProvider, and LdapAuth and similar extensions can redefine that service. This is easy enough for password backends which behave like a database, but problematic for backends which try to offer strong security guarantees (such as T120484) and therefore only offer a restricted set of operations. Plus even less likely to work sanely with multiple providers.

Using temporary passwords on a site which uses multiple password-based auth methods seems hard. On login, we just check all methods until one works; on account creation, the configuration of AuthManager will decide which method to use. When creating a temporary password, though, we need to know which is the authoritative provider for that user - if they use LDAP and we set a temporary password in the database, we sort of defeated the security guarantees that LDAP would give us. Or am I overthinking it?

And of course even if we store the temporary password locally, when the user succeeds in a password reset, the new password needs to be set wherever non-temporary passwords are stored for that user. That means ResetPasswordSecondaryAuthenticationProvider also needs to be password-backend-aware... maybe we should have an internal equivalent of the service that's planned for T120484? With some way to check if a given user is handled by the service, and then on mixed-backend wikis we can have multiple password services which work as a fallback chain.

it does not work together with e.g. LdapAuthentiaction.

This seems unlikely, TemporaryPasswordPrimaryAuthenticationProvider does not care at all what other password-using authentication providers might be configured. Can you provide details as to how exactly it doesn't "work together"?

When I spin up a mediawiki-vagrant with the ldapauth role, hack it to remove LocalPasswordPrimaryAuthenticationProvider, and use Special:PasswordReset, it works fine. When I create a new account with an emailed temporary password, login works fine but LdapAuthentication blows up in LdapPrimaryAuthenticationProvider::providerChangeAuthenticationData() from ResetPasswordSecondaryAuthenticationProvider, apparently it fails actually setting the password. Probably because the user was never created in the LDAP to begin with. That's a bug in LdapAuthentication, it should handle that situation.

Every password-based primary authentication provider has to reimplement temporary password handling (possibly using an abstract base class that we provide).

They don't have to. But making it easy to subclass for it probably not the worst idea. At a basic level, all we'd really need is to do is to abstract the $dbr->selectRow() and $dbw->update() calls, change providerAllowsAuthenticationDataChange() to use the abstracted selectRow(), maybe change the implementation of testUserExists() to use the abstracted selectRow(), and figure out if the onTransactionIdle() calls need adjusting somehow.

Inject some sort of password getter/setter service into TemporaryPasswordPrimaryAuthenticationProvider

Meh. That's basically the same thing as the subclass idea except more complicated since you have to define a new service and inject it. If we already had a such service it could make sense, but we don't.

Using temporary passwords on a site which uses multiple password-based auth methods seems hard. On login, we just check all methods until one works; on account creation, the configuration of AuthManager will decide which method to use. When creating a temporary password, though, we need to know which is the authoritative provider for that user - if they use LDAP and we set a temporary password in the database, we sort of defeated the security guarantees that LDAP would give us. Or am I overthinking it?

You're overthinking it.

And of course even if we store the temporary password locally, when the user succeeds in a password reset, the new password needs to be set wherever non-temporary passwords are stored for that user. That means ResetPasswordSecondaryAuthenticationProvider also needs to be password-backend-aware

No it doesn't. It means ResetPasswordSecondaryAuthenticationProvider needs to use AuthManager::changeAuthenticationData() to do the password changing and AuthManager will handle passing it on to all the relevant primaries. Which is exactly what it already does, so there's no problem there.

The task is a somewhat unfortunate mix of two different issues - that IMO temporary passwords should use the same storage mechanism as normal passwords, or at least it should be easy to do so (that will be needed for the WMF password service as well) and that temporary passwords seem to not work with LdapAuthentication (that was reported in https://gerrit.wikimedia.org/r/#/c/313019/1, see next to last comment).