In our case, we are seeing invalid DN as error message when connecting to OpenLDAP with local accounts (this makes sense, but we also have a little bit of magic in front of our LDAP to transfer logins in form of joe.doe@ffzg.hr to cn=joe.doe,dn=ffzg,dn=hr, so I'm not sure that this is only error on which we want to fallback to local auth.

Unfortunately, I can't connect to OpenLDAP directly (since bind needs this cn=,dn=,dn= syntax) and don't have Active directory to test against that (but I suspect that AD users are not using auth_by_bind - again I might be wrong).

Hmm.. I'm still not really sure if a return code of 34 (LDAP_INVALID_DN_SYNTAX (34)) is all we should be looking for here.

For example, what about: LDAP_NO_SUCH_OBJECT (32)

Or, should we be catching LDAP_INVALID_CREDENTIALS (49) for an expired password only?

Dobrica, do you have any comment?

Sure. it makes perfect sense to catch all three of them.

Do either of your fancy doing that followup, then I can proceed further with QA?

Sorry for delay, I wanted to test things before submitting more code.

I can't really get LDAP_NO_SUCH_OBJECT error on bind. My guess is that this error would appear if I was searching for object which wasn't defined in LDAP schema as opposed to bind call. Did you manage to get this error from LDAP server on bind?

I wouldn't like to insert code there just because it seems right, since LDAP code in Koha is already black magic to almost everyone and accumulating cruft doesn't seem like a good idea.

Regarding LDAP_INVALID_CREDENTIALS, it's already handled by else part of if statement and I did test with invalid password and it works as-is.

Upon further investigation, I decided to obsolete my patch because I suspect that if fixes problem only for our particular installation instead of providing general solution to problem. Let me explain...

Problem comes from the fact that in LDAP bind we can't distinguish between users which doesn't exist in LDAP and invalid passwords. Both of those return LDAP_INVALID_CREDENTIALS (in our installation we have a bit of LDAP software in between which creates invalid DN for users which are not in LDAP, but that is not generally applicable).

This means that we have to know (somehow) weather users should use LDAP passwords or not. Currently we don't have that information.

One possibility is to do LDAP search (anonymous or via some account) and if user does exist in LDAP force him to provide proper LDAP password. However, this might not be practical because it creates additional load on LDAP server on one hand, and on another anonymous searches might be disabled (that's probably why we are using auth_by_bind in the first place) or there might not be special user which can search whole LDAP (or library might not have permissions to add such user which is our case).

Another possibility is to track locally in Koha if users should use LDAP or local passwords. This might be implemented simply by invalidating local password (with word LDAP for example :-) which would prevent local fallback from working if user is logged in using LDAP.

I'm biased towards second solution but I would love to hear some feedback about it before producing code.

However, in any case we don't have clear migration path for existing installations because they do have local LDAP passwords in database (like we do). After first LDAP login users will have local password invalidated, but users which never login with correct LDAP password (or who are already disabled) we still be able to login using cached password.

To be quite honest, I did use that side-effect of old code which allowed our patrons to login while LDAP wasn't available, so it might even make sense to have that behavior again (as LDAP configuration option perhaps). But if that is the case, we again don't have ability to expire users just in LDAP and expect them to be disabled in Koha also.

I am not sure I understood completely what Dobrica wrote, but I feel like ideally we shouldn't store passwords for LDAP users, but still have the option to have 'local password only' users - think of external library users that are not in LDAP. And as this worked before, we really need to make sure it does again.

Unfortunately I don't think there's going to be a 'quick fix' here. As Dobrica says, I believe his fix doesn't really catch the general case and as such we could be chasing our tails for a while.  That said, it's probably better than nothing to start with.

Our LDAP implementation relies heavily on 'quirks' all over the place and isn't really a very thorough implementation at all.

The more I look at it, the more questions I ask about it. I'd love to get a some feedback from the community as to how people are configuring the module and expecting it to behave. Then we'de have a better idea of what features are 'broken', what features are in fact bugs, and where people are using bugs as features to get around our bad implementation.

I think we can't leave that it is currently - we need local logins or we will break people's LDAP setups on update :(

Speaking for ourselves, we feel that it's absolutely essential that there by fallback logins. That is, that if the LDAP login fails for any reason, you can still login with a local Koha account. In fact, on a number of our partner sites, the staff accounts aren't even using the LDAP accounts; they are only local.

ByWater's $0.02.

- Larry

I think we can all agree with that sentiment Larry ;).

The problem is how to achieve it in a secure way; any chance you could share a few examples of how your ldap customers are set up, and what response you get from the ldap server given an incorrect username:password combination vs's the response for a user not in the ldap directory?

That would really help move things along on this bug.

I think what we really want to achieve is a failure for bad username:password combination only.. and allow local fallback for users that are plain missing from the ldap directory.

Does everyone agree that's what we need?

(In reply to Dobrica Pavlinusic from comment #8)

Having re-read Dobrica's suggestion here I'm also in favour of the last option he puts forward of invalidating the local password upon first successful LDAP login. (Though I would insert an randomised string as the 'bad' password rather than a set word for all official LDAP users)

I also feel this should be optional, with the option being something like 'Allow local fallback when LDAP server not found'.

Finally, getting around the issue of expiring ldap users, there's no clear single signal for such an action it seems, so I tihnk we're really going to struggle to get 'best of both' here.

Dobrica, do you have the time to develop such a patch as you suggested... I'm kinda tight for time at the minute, but feel this really needs to be fixed for release :(

I think it's possible to just delete it or set it to a special thing that indicates "no password" as you can generate borrowers with no password in the GUI. Then it would also be easy to see, who has logged in over LDAP.

A pref restoring the old (and maybe bad behaviour) if anything else fails would maybe be an option too. Generally having a pref could make sense.

We also don't have staff users in LDAP but only in Koha, as we keep the normal accounts and staff accounts separate or at least encourage to do that.

Could 9165 help with this?

My attached patch 'should' fix this for the case where a library is using auth_by_bind and not anonymous_bind. Reading the code, I believe this was the only case that needed fixing in this way.

To re-introduce the functionality of bug 8148 fully however, we should also take a careful look at bug 9165 as suggested by cait, as with some minor adjustments with would allow for the functionality we're looking for for all three major LDAP configurations.

Please, we need testing on this ASAP.

Here's the test plan I followed:

1. Create a LDAP user (uid=julian, userpassword=foobar)
2. Create a Koha user (userid=julian, password=julian)
3. Create another Koha (local) user (userid=123, password=123)
3. Enable LDAP in $KOHA_CONF with auth_by_bind = 1, and anonymous_bind = 0
4. Update source code to current master

5. Login with julian/foobar => success => OK
6. Login with julian/julian => failure => OK
7. Login with 123/123 => failure => NOT OK
8. Set anonymous_bind = 1 in $KOHA_CONF
9. Login with julian/foobar => success => OK
10. Login with julian/julian => failure => OK
10. Login with 123/123 => success => OK
11. So, here I can confirm the problem for local login when auth_by_bind = 1 and anonymous_bind = 1, let's apply the patch...

12. Apply the patch
13. Reset anonymous_bind to 0
14. Login with julian/foobar => success => OK
15. Login with julian/julian => success => ??
16. Login with 123/123 => success => OK
17. Set anonymous_bind to 1
18. Login with julian/foobar => success => OK
19. Login with julian/julian => failure => OK
20. Login with 123/123 => success => OK

So the patch fixes the problem, but allows LDAP users to login with their LDAP password and their local password.
But this should be fixed by bug 9165, so I will QA this patch.

Very thorough and well explained, cheers Julian.

You're spot on with point 15 being exactly where bug 9165 comes in.  

Unfortunately 9165, even with my follow-up, isn't an instant solution as it relies on one successful ldap login per user, after the config preference is updated, to remove the local password and therefore disallow local fallback. I think this is the best possibly solution however with our current workflows.

I'll clarify the above point in the other bug to make sure people are aware of this.

Created attachment 34002 [details] [review]
Bug 12831: (RM followup) regression test

checkpw_ldap should return 0 if it is not an anonymous bind, and authentication
fails. This is better explained on the bug comments. This is just a regression
test for the revised functionality.

To test:
- Run
  $ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: it fails because C4::Auth_with_ldap doesn't match the expected behaviour
- Apply the bugfix from Martin
- Run
  $ prove t/db_dependent/Auth_with_ldap.t
=> SUCCESS: tests now pass.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 34003 [details] [review]
Bug 12831: Local Only logins with LDAP

Local only logins should continue to function when LDAP is enabled.
This was not the case after bug 8148 [LDAP Auth should FAIL when ldap
contains a NEW password].  For this case, we need to diferentiate
between local accounts and ldap accounts.  This is somewhat challenging
and thus this patch is only part of the story.

The other half can be achieved with bug 9165

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Patches pushed to master.

Thanks Martin!