1. Your editor/IDE seems to be replacing spaces with tabs and reformatting line lengths, etc. It would be much easier if you could turn off this reformatting.
2. There is a stray "e.printStackTrace();" in there… I'm assuming you didn't mean to include that.
3. The commented out statement (searchCtls.setReturningAttributes(returnedAtts)) should be removed or re-enabled (or put under a run-time switch, if necessary).
4. We will probably not bother with the pg.properties and labkey.xml changes; we'll document the new settings and anyone who needs this functionality can easily add the properties directly to their labkey.xml.

1. Your patch includes a hard-coded, Rochester-specific constant in the code: "DC=urmc-sh,DC=rochester,DC=edu". Committing to the standard code base will require generalizing this, perhaps as another setting in labkey.xml.
2. The patch would be smaller and the logic more straightforward if you checked for the presence of the special settings before attempting to authenticate. If the settings exist, use the search account to look up the user's principal; if the settings doesn't exist, just use the email that was typed in.

Adam

5. is obviously an oversight by me. Putting it into labkey.xml works fine by me 6. This is also fine by me.

Some of the code for our modification seems like it doesn't need to be run every time someone attempts to authenticate. In particular, the code that does the JNDI lookup of the username, password, and searchBase that are used to search LDAP - we realy only need to execute this code once. Would it make sense to do that in activate()? I'm not familiar with when that method is called or what it's for, but it looks like it might be the right place to do that.

Please advise. Thanks.

activate() would be a reasonable place to do this, as would the provider or manager constructor, since the labkey.xml settings won't change under you (if the file changes, Tomcat will restart the webapp).

However, I wouldn't actually worry about this at all… JNDI is called constantly and I'm sure these values are cached in memory.

Adam

Please review this, and let me know if any further changes are necessary, of if you need any more info. If it looks okay, we'd like to get it into the 12.1 release, if that's possible.

Thanks.

Andy

One question that just dawned on me: is there a reason why the new properties must reside in the labkey.xml file? The other LDAP properties are stored in the database and accessed via the admin UI… it seems odd to have these in two places.

Thanks, Adam

Michael: If you're reading this, can you comment?

Obviously, there would be more work to update the LDAP configuration UI to support these additional properties. The UI would be slightly more complicated because there would be two "modes" of data entry: 1) Current mode, where you enter:

• servers
• domain
• template
• SASL?

• servers
• domain
• ldapSearch username
• ldapSearch password
• ldapSearch searchBase
• SASL?

Being new to LabKey, I don't know how much work it would be to update the UI like this. Looks like it would require additional data members on the LdapController.Config class, additional form elements in the corresponding configure.jsp, and some way to show/hide the form elements appropriate for the chosen mode, and some way to choose the mode. Also, LdapAuthenticationManager.java would need updating, but that would be pretty straightforward - might simplify that class a bit.

Some alternatives for the dealing with the different modes in the UI:

• The form shows all properties for both modes. Documentation (or text on the page) tells users which fields are required for which mode. We infer the mode by seeing if any of ldapSearch fields are filled in. Simple to implement, but probably the most confusing UI for users.
• Change the template from a text field to a drop-down list with three choices: ${uid}, ${email}, ${emailSearch}. The choice in this drop-down determines the mode (first two are current mode, last one is new mode), as well as driving which form elements are shown/hidden. This requires some JavaScript to dynamically show/hide form elements. It also means the value for template is constrained to the values in the drop-down list, not whatever the user wants to enter - not sure if that's okay.

Andy

Do you want to expose the "new mode" to the general user population, or would it be better to keep this as an undocumented (or less-well-documented) feature, and not clutter the current UI with it? I have the impression that this new mode is needed only by us at the U of R. Using environment values set in labkey.xml means the new mode is not exposed through the UI, and makes the impact of the change smaller: no change to Config class, no change to configure.jsp, perhaps no change to documentation. I'm also not clear on whether additional Config data members affects any database schema.

Andy

If we get pressed for time, we could commit the current approach in 12.1 and migrate later, but updating the UI should be straightforward (even with the conditional) and the LDAP settings get stored in a property bag anyway, so that's easy.

I'll integrate your patch and look at migrating the settings. Either way, I'll need your help with testing the final solution, since I don't have an LDAP server configured this way.

Adam

Our priority is to get the patch into 12.1, with or without the UI changes.

Thanks.

Andy

Thanks.

Andy

Adam

One minor question: your patch includes a commented out line "// searchCtls.setReturningAttributes(returnedAtts);" (current line #258)… should this be removed or enabled?

Please sync up and test the commit against your configuration.

Adam

Thanks.

Andy

You can remove that commented-out line (#258).

Thanks.

Is there a target date for general availability of 12.1?

Andy

We created our 12.1 release branch yesterday, which means we're wrapping up. Key clients started deploying 12.1 on their test & staging servers several weeks ago. Based on timing of previous releases, I expect we'll be posting official installers mid April.

Adam

However, what we are seeing is that users who were users before the switch to LDAP authentication can log in with EITHER their old password (as it is stored in the LabKey database) OR their LDAP credentials.

This seems like a bug, because your docs state the following (from https://www.labkey.org/wiki/home/Documentation/page.view?name=configLdap ):

"LDAP Domain: Specifies the email domain that will be authenticated using LDAP. All users signing in with an email addresses from this domain will be authenticated against the LDAP server; all other email addresses will be authenticated against the logins table in the database. Leave blank to not use LDAP authentication (always use the database)."

Would you please let us know if this is a bug or not, and also suggest a way that we can force authentication to use LDAP for these users.

Thanks

Andy Straw University of Rochester

If you want to proactively prevent LDAP users from using their database passwords you can delete the appropriate rows from core.Logins.

Adam

I want to be sure I understand how core.logins (and related) tables work. Let me know if I have this right:

• An entry in core.logins means the user can authenticate against the password stored in the labkey database. If no entry in core.logins, they would have to authenticate via LDAP (if so configured). Removing an entry in core.logins does NOT mean the user's account is gone - just their ability to authenticate using the password stored in the labkey database. So, for us to force users to authenticate against LDAP, we would remove there records in core.logins.
• You connect a user who successfully authenticates via LDAP, to who they are according to labkey, via the name field of core.principals. Then, core.principals ties their name (email address) to their userid, which is used to tie them to core.usersdata, core.members, perhaps other tables.

Thanks.

Andy

Site admins CAN change users' email addresses / user names. See the "Change Email" button on the details page for each user.

Adam