Page MenuHomePhabricator

Problem with auto-creation of LDAP user in the wiki in case of the first login
Closed, ResolvedPublic

Description

I had a problem with logging in with LDAP user in my wiki.
That's the first log in of that user, so the same user should be auto-created in the wiki.
But it my case login page was loading few minutes after submit, and then re-load after timeout.

After some debugging I found out that it happens because of eternal recursion, caused by one of onSaveSettings hook handlers.
Let me share some details on that.

In case with auto-created user that PluggableAuth code is called.
It causes call of \User::saveSettings() here, when user real name and email are updated.
\User::saveSettings() executes all hook handlers which are attached to onSaveSettings hook.

In my case one such hook handlers uses \User::newSystemUser( 'MediaWiki default' ) for internal purposes, which seems to be okay.
And here eternal recursion, mentioned above, happens.

MediaWiki checks if user MediaWiki default is a system one. Check is done here
Method \User::isSystemUser() calls \MediaWiki\Auth\AuthManager::userCanAuthenticate( $username ) here which goes through all primary authentication providers and checks if specified user can authenticate with that provider. If user cannot authenticate with any of them - user is considered as system one.
As soon as PluggableAuth primary authentication provider just checks if user basically exists (here) - user is considered as not system user. That seems to be incorrect.
So MediaWiki tries to "steal" that user to use as system one, and at some point calls \User::saveSettings() here.
And that, assuming that we are already in \User::saveSettings() call - causes eternal recursion.

My proposal is to add to PluggableAuth primary authentication provider additional check (when testing if user can authenticate) that specified username is "usable". I mean this check if username is reserved for system user etc.
That's how it's done in \MediaWiki\Auth\LocalPasswordPrimaryAuthenticationProvider, which is used for local login - see

My LocalSettings.php:

wfLoadExtension( 'PluggableAuth' );
wfLoadExtension( 'LDAPProvider' );
wfLoadExtension( 'LDAPAuthentication2' );
wfLoadExtension( 'LDAPAuthorization' );
wfLoadExtension( 'LDAPUserInfo' );
wfLoadExtension( 'LDAPGroups' );
wfLoadExtension( 'LDAPSyncAll' );


$LDAPProviderDomainConfigs = "$IP/extensions/LDAPProvider/docs/ldapprovider.json";

$wgPluggableAuth_Config['Log In (LDAP 1)'] = [
	'plugin' => 'LDAPAuthentication2',
	'data' => [
		'domain' => 'LDAP1'
	]
];

$wgPluggableAuth_Config['Log In (LDAP 2)'] = [
	'plugin' => 'LDAPAuthentication2',
	'data' => [
		'domain' => 'LDAP2'
	]
];

$wgPluggableAuth_EnableLocalLogin = true;

$LDAPAuthentication2AllowLocalLogin = true;

Event Timeline

Change 905987 had a related patch set uploaded (by Aklapper; author: AKulbii):

[mediawiki/extensions/PluggableAuth@master] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/905987

Thank you very much for the bug report and the fix.

I wonder if this fix should actually go in core in AbstractPrimaryAuthenticationProvider::testUserCanAuthenticate(). @Tgr, do you have any thoughts on this?

In the meantime, I will review and hopefully merge this patch. This may turn out to be temporary for a couple MediaWiki releases until and if a fix makes it into core.

There are a number of things that could be improved there:

  • Authentication providers should not save the user during login / signup, it will be saved in the end anyway. At best it will degrade performance. This should be documented better, or maybe we could do something similar to SessionBackend::delaySave() for user objects.
  • PluggableAuth should expose testUserCanAuthenticate() to its plugins. I guess the LDAP plugin should check if the user has any password data stored. (Or maybe that it exists at all, but in LDAP, not locally.)
  • We could revisit T212720: System users should be in a dedicated user group to make detecting system users less involved.

The patch isn't a reliable fix, ideally system users' names should be reserved but in practice that isn't always true. (Currently reserving the name is not even mentioned in the User::newSystemUser() documentation. That should probably be improved too.)

I believe the provided patch for PluggableAuth is a reasonable solution. It does not solve any core issues, that is true, but it improves the situation without introducing much risk. We can not fix that within the LDAP extension, as the reported issue is a side effected caused by a hookhandler. The LDAP extension has no way to know about this.

Change 905987 merged by jenkins-bot:

[mediawiki/extensions/PluggableAuth@master] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/905987

I went ahead and merged the fix to resolve the current issue. Issues in core can be addressed separately.

Change 926699 had a related patch set uploaded (by Cicalese; author: AKulbii):

[mediawiki/extensions/PluggableAuth@REL1_39] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/926699

Change 926700 had a related patch set uploaded (by Cicalese; author: AKulbii):

[mediawiki/extensions/PluggableAuth@REL1_40] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/926700

Change 926699 merged by jenkins-bot:

[mediawiki/extensions/PluggableAuth@REL1_39] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/926699

Change 926700 merged by jenkins-bot:

[mediawiki/extensions/PluggableAuth@REL1_40] Fix problem with user auto-creation in case of the first login

https://gerrit.wikimedia.org/r/926700