LDAP Authenticator: Identify user profile page by DN to prevent profile duplication

I recently ran into a problem with the way the LDAP Authenticator identifies the profile page of the user logging in. The use case is the following:

This happened when switching the login to SSO. Before enabling SSO the users logged in using the AD Attribute userPrincipalName, which contains their E-Mail address - or at least something looking similar to it, like firstname.lastname@example.com.
That was a client side requirement: for all web applications which do not support SSO (or do not have it enabled atm), use that attribute instead of the usual sAMAccountName as login.

This attribute is stored in the user profile as the uid attribute of the XWiki.LDAPProfileClass, and is used to identify the user profile belonging to the user when they log in.

When switching to SSO however the apache mod_auth_kerb sends a header with a value of the sAMAccountName@EXMAPLE.COM as authentication data. After extracting the sAMAccountname from the header, the user is logged in. But when the LDAP Authenticator tries to identify the corresponding user profile in XWiki, of course it cannot find the already existing profile, because that one has stored the userPrincipalName instead.

However I see that the LDAPProfileClass object of the user profile also stores the DN, which should not change over the lifetime of the user account, and does certainly not change when switching to SSO. To me it looks like a better way to identify the user profile in XWiki (in my use case).

The current code does not do this, because the user profile is extracted very early in the ldapAuthenticateInContext method, i.e. at:

https://github.com/xwiki-contrib/ldap/blob/ldap-9.4.5/ldap-authenticator/src/main/java/org/xwiki/contrib/ldap/XWikiLDAPAuthServiceImpl.java#L567

This is before any data is fetched from the LDAP, and hence only the login data given by the user is available, and not e.g. the DN.

The XWiki user profile is only used in “Step 9”, however:

https://github.com/xwiki-contrib/ldap/blob/ldap-9.4.5/ldap-authenticator/src/main/java/org/xwiki/contrib/ldap/XWikiLDAPAuthServiceImpl.java#L679

and in that step data from the LDAP is already available.

So I wonder if I could postpone fetching the user profile in XWiki to that point, and then add a configuration variable to alternatively use the DN from the LDAP to identify the user profile.

Is this a reasonable addition? My client would very much prefer to use the “official” LDAP Authenticator and not a patched version, because of the obvious maintenance overhead. If I can implement this as a feature in the main line of development (of course with a configuration value that defaults to the current behavior) that would be a solution that I prefer, too.

However before I add new features to the LDAP Authenticator, I want to ask the other developers, especially @tmortagne as the maintainer, if this is acceptable. As far as I can see this will add only little complexity. On needs to read a new configuration variable and add an if clause to determine the attribute in the HQL query here:

https://github.com/xwiki-contrib/ldap/blob/a92722b9249d39ac48c577ef88c31685b055bfa5/ldap-authenticator/src/main/java/org/xwiki/contrib/ldap/LDAPProfileXClass.java#L243

I can also imagine that this feature can be useful in other cases where the attribute to be used for login changes. But of course your mileage may vary.

(The alternative way to handle my use case is to do a data migration and change the uid value in the XWiki.LDAPProfileClass from the userPrincipalName to sAMAccountName. That is possible without code changes to the LDAP Authenticator, but then in cases where the SSO cannot be applied (like non-Windows clients, remote access outside the AD-domain) people will use the form based login and be back with the same problem with duplicate profiles, just the other way around.)

Is there anyone finding an immediate issue with that feature, or should I go ahead and create a JIRA issue?

I’m fine with the concept but I don’t fully agree with the proposed implementation which mixes two different things IMO.

I would keep the existing code the way it is but I would add a new check after getting the DN that if we don’t know the user profile yet (the one searched based on the uid) try to find an existing one but based on the DN this time. In most cases where the login is stable (and even in the use case you described I’m sure that many users mostly use the same login) the current logic is faster so I prefer to keep it like this but it also make perfect sense to also check for the DN later (when you enable this extra search) to be sure to not cause any duplicates.

This sounds even easier, no extra configuration involved. Thanks for the suggestion, I guess I will try this approach then.