Change 931598 had a related patch set uploaded (by Slyngshede; author: Slyngshede):

[mediawiki/extensions/OpenStackManager@master] AttachLdapUser remove email handling.

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

Change 931598 abandoned by Slyngshede:

[mediawiki/extensions/OpenStackManager@master] AttachLdapUser remove email handling.

Reason:

See comment from Majavah.

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

LdapPrimaryAuthenticationProvider::onUserLoggedIn() is called as a handler for the 'UserLoggedIn' hook. This in turn calls LdapAuthenticationPlugin::updateUser() which updates the attached MediaWiki user with data read from the LDAP directory. At the end of LdapAuthenticationPlugin::updateUser() a call is made to $user->saveSettings(). This call eventually fires the LdapPrimaryAuthenticationProvider::onUserSaveSettings() handler for the 'UserSaveSettings' hook. That hook handler calls $ldap->updateExternalDB( $user ) which ultimately writes the value of several user preferences, including email address, back to the LDAP directory if set.

This all looks like it is intended to perform a bi-directional sync of any set email address between the LDAP directory and an attached MediaWiki account. First any email set in LDAP is copied into the MediaWiki user table via the 'UserLoggedIn' hook. Later any email set in the MediaWiki user table is copied into the LDAP directory via the 'UserSaveSettings' hook.

Change 931889 had a related patch set uploaded (by Slyngshede; author: Slyngshede):

[mediawiki/extensions/LdapAuthentication@master] updateExternalDB: Check for empty email.

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

I'm having a bit of a hard time parsing the PHP code... mostly because PHP3 was the last version I used.
One thing I can't seem to figure out is how the attributes are read from LDAP, my current theory is that the email is being attempted read from "email" rather than "mail", which would be the correct field. I'm not use that's entire correct though, as it's clearly targeting the mail field on updates.

In T339917#8952350, @SLyngshede-WMF wrote:

I'm having a bit of a hard time parsing the PHP code... mostly because PHP3 was the last version I used.
One thing I can't seem to figure out is how the attributes are read from LDAP, my current theory is that the email is being attempted read from "email" rather than "mail", which would be the correct field. I'm not use that's entire correct though, as it's clearly targeting the mail field on updates.

LDAPAuthentication hasn't been properly maintained in years so the code is rather messy and out of date wrt modern mediawiki standards. I can (try to) have a look later if I can figure out what's happening.

https://noc.wikimedia.org/conf/highlight.php?file=wikitech.php

$wgLDAPPreferences = [ 'labs' => [ "email" => "mail" ] ];

@taavi Thank you :-)

It is kinda fun poke around in though, but I think I need to have a debug/test enviroment.

@Reedy Okay, so it wasn't something easy ... :-(

Change 931889 abandoned by Slyngshede:

[mediawiki/extensions/LdapAuthentication@master] updateExternalDB: Check for empty email.

Reason:

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

Here is the LDAPAuthentication debug log for an account creation where I could reproduce this: P49562

I think the main interesting thing from it is that the extension can load the email from LDAP just fine:

2023-07-13 20:33:27.069822 [cf64506e-893e-4a66-ad3f-cefb12fef0ff] cloudweb1004 labswiki 1.41.0-wmf.17 ldap INFO: 2.2.0 Retrieved email (hi+ldap-create-test-20230713-01@taavi.wtf) using attribute (mail)

That debug log message comes from LdapAuthenticationPlugin::getPreferences() and indicates that LdapAuthenticationPlugin->email now has the correct email value set.

The next interesting line is this one:

2023-07-13 20:33:28.147031 [cf64506e-893e-4a66-ad3f-cefb12fef0ff] cloudweb1004 labswiki 1.41.0-wmf.17 ldap INFO: 2.2.0 Setting email.

That one comes from LdapAuthenticationPlugin::updateUser() and indicates that it's writing LdapAuthenticationPlugin->email to the MW user and to the database. That's the only place where LdapAuthentication does that.

So seems like something is overriding the value of LdapAuthenticationPlugin->email between the call to getPreferences() and updateUser().

That something is LdapAuthenticationPlugin::updateExternalDB() most likely, which seems to be called just a few rows below. The only thing that calls it is the UserSaveSettings hook, which runs just a few log lines below.

So I think the order is something like this:

1. Someone with an existing LDAP account (with an email set) but no Wikitech account logs into Wikitech.
2. LdapAuthenticationPlugin::authenticate() runs, which calls LdapAuthenticationPlugin::getPreferences() to load the email attribute to LdapAuthenticationPlugin->email but does not save it into the MW user object (as that doesn't exist yet).
3. At this point, MediaWiki still thinks the users email is an empty string.
4. Some other extension runs some other hook which saves user preferences. That causes LdapAuthenticationPlugin::updateExternalDB() to run, which writes that empty email to the MW database, to LdapAuthenticationPlugin->email and to LDAP.
5. Later, LdapPrimaryAuthenticationProvider::onLocalUserCreated() runs and calls the relevant methods to write the email loaded in (2) to the MW database. However, (4) overrode that so it re-writes an empty string to the MW database.
6. Poof. We've lost all copies of that user email.

In T339917#9014178, @taavi wrote:

4. Some other extension runs some other hook which saves user preferences. That causes LdapAuthenticationPlugin::updateExternalDB() to run, which writes that empty email to the MW database, to LdapAuthenticationPlugin->email and to LDAP.

The preference save is in the UserLoggedIn hook handler from LdapPrimaryAuthenticationProvider:

In T339917#8949420, @bd808 wrote:

LdapPrimaryAuthenticationProvider::onUserLoggedIn() is called as a handler for the 'UserLoggedIn' hook. This in turn calls LdapAuthenticationPlugin::updateUser() which updates the attached MediaWiki user with data read from the LDAP directory. At the end of LdapAuthenticationPlugin::updateUser() a call is made to $user->saveSettings().

And I've missed something: since these are "auto-creations", the LdapPrimaryAuthenticationProvider::onLocalUserCreated() hook actually does nothing. But those are handled by LdapPrimaryAuthenticationProvider::autoCreatedAccount which is called by AuthManager just before the hook.

In T339917#9014202, @bd808 wrote:

The preference save is in the UserLoggedIn hook handler from LdapPrimaryAuthenticationProvider:

I don't believe it is. The log entry for that (Entering updateUser) is quite a bit below the first Entering updateExternalDB entry.

The preferences for the test account are all coming from a WikimediaMessages hack:

wikiadmin2023@10.64.48.161(labswiki)> select user_id from user where user_name = 'Taavi user create test 20230713 1';
+---------+
| user_id |
+---------+
|   36915 |
+---------+
1 row in set (0.001 sec)

wikiadmin2023@10.64.48.161(labswiki)> select up_property from user_properties where up_user = 36915;
+-----------------------------+
| up_property                 |
+-----------------------------+
| rcenhancedfilters-seen-tour |
| wlenhancedfilters-seen-tour |
+-----------------------------+
2 rows in set (0.001 sec)

That hook handler does not call saveOptions, that comes from somewhere else.

I don't think patching all the option setters in all extensions is a good idea. Instead I think we should just add some property somewhere that's set during account autocreation that short-circuits updateUser until initUser is called.

We do have sort of a work-around, which is currently for review. We let the IDM call the createUser api on mediawiki, so that the database object for the user already exists when the authentication via LDAP is done.

The LDAPAuthentication plugin will still pointlessly overwrite the value in LDAP, but at least the email will be correct.

Change 938221 had a related patch set uploaded (by Majavah; author: Majavah):

[mediawiki/extensions/LdapAuthentication@master] Try to work around account autocreation deleting email

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

Change 938221 merged by jenkins-bot:

[mediawiki/extensions/LdapAuthentication@master] Try to work around account autocreation deleting email

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