sjkingo / django_auth_ldap3

A library for connecting Django's authentication system to an LDAP directory
BSD 2-Clause "Simplified" License
23 stars 13 forks source link

Username case sensitivity #7

Closed rmassoth closed 9 years ago

rmassoth commented 9 years ago

When using Active Directory, username lookups are case insensitive but django will create a new user if given a different case username. For example, given a username "steve", if Steve logs in with username "Steve" AD will authenticate the existing user but django will create a new user with. I fixed it with a simple username = username.lower() at the top of the authenticate method to force all lower case usernames in django but I don't know if this is compatible with openLDAP as my company uses AD.

sjkingo commented 9 years ago

Nice catch, I hadn't thought to test that with AD.

Could you please send a pull request for the change, and I will test it on OpenLDAP for you.

rmassoth commented 9 years ago

Yeah, I only caught it because a mobile browser automatically capitalized the first letter when logging in and created a new user. I love this library by the way. It does just what I needed it to out of the box and your examples were very helpful to get started.

sjkingo commented 9 years ago

Thanks for the kind words.. it's great to see it being useful to someone else!

With regards to this change, I have given it some thought and while I agree that there is a definite issue with the case sensitivity of Django's usernames (which is a long standing issue that the devs refuse to do anything about) - I don't think this is the way to go about it.

It brings in an edge case that I don't like: if you pull down (from LDAP) a username such as Bob, but store it in Django as bob by calling .lower() (thus preventing duplicate users being created), you have a new problem where the displayable username is not what the user may be used to.

This may not be a problem for anyone else, but I don't really like it. It's not immediately obvious that bob == Bob in Django's auth table (given Django's case-sensitivity, that is).

Could we do something like store the correct case-sensitive username in Django (such as Bob), but modify the check in django_auth_ldap3 to handle case-insensitivity? This way duplicates won't get created, and a user can authenticate with regards to case-sensitivity.

But you are right - OpenLDAP is the same as AD in that the uid attribute is case-insensitive.

rmassoth commented 9 years ago

Yeah, I was thinking about this after submitting the pull request. This breaks it for current users of this library because if they already have usernames not in all lower case it's going to create new users instead of using the existing. It needs to do a precheck for username existance in django first, authenticate through ldap, then create or log in the user. Usernames still have to be unique but they can be stored in the case specified at creation.

sjkingo commented 9 years ago

Sounds like a solid plan - I agree this is a pretty significant blocker and needs to be resolved.

Do you want to modify the PR to implement this? I can't seeing it take more than a couple of extra lines in the authenticate() method.

rmassoth commented 9 years ago

This is trickier than I thought. In order to support case sensitive names in django you will have to check every permutation of the entered username to see if it exists first in django. This could be n^2 checks against the database. If the username doesn't exist, then authenticate through ldap and create the user. I think the accepted way of handling this is to just force to lower case as I've done but it's up to you. The django devs have certainly created a tricky situation with this one considering how many people might want to use 3rd party apps for authentication.

sjkingo commented 9 years ago

Couldn't you just go: User.objects.filter(username__icontains='Bob')

sjkingo commented 9 years ago

Sorry, I meant to say username__iexact='Bob'. This would mean the following are true:

Do you think that will work?

sjkingo commented 9 years ago

I haven't tested this, but I was thinking this may work:

diff --git a/django_auth_ldap3/backends.py b/django_auth_ldap3/backends.py
index c165c32..fbbb5a8 100644
--- a/django_auth_ldap3/backends.py
+++ b/django_auth_ldap3/backends.py
@@ -77,25 +77,27 @@ class LDAPBackend(object):
         # Get or create the User object in Django's auth, populating it with
         # fields from the LDAPUser. Note we set the password to a random hash
         # as authentication should never occur directly off this user.
-        user, created = User.objects.get_or_create(username=username, defaults={
-                'password': hashlib.sha1().hexdigest(),
-                'first_name': ldap_user.givenName,
-                'last_name': ldap_user.sn,
-                'email': ldap_user.mail,
-                'is_superuser': False,
-                'is_staff': admin,
-                'is_active': True
-        })
+        django_user = User.objects.filter(username__iexact=username)
+        if not django_user:
+            # Create new user
+            django_user = User(username=username,
+                    password=hashlib.sha1().hexdigest(),
+                    first_name=ldap_user.givenName,
+                    last_name=ldap_user.sn,
+                    email=ldap_user.mail,
+                    is_superuser=False,
+                    is_staff=admin,
+                    is_active=True
+            )
+        else:
+            # If the user wasn't created, update its fields from the directory.
+            django_user.first_name = ldap_user.givenName
+            django_user.last_name = ldap_user.sn
+            django_user.email = ldap_user.mail
+            django_user.is_staff = admin
+            django_user.save()

-        # If the user wasn't created, update its fields from the directory.
-        if not created:
-            user.first_name = ldap_user.givenName
-            user.last_name = ldap_user.sn
-            user.email = ldap_user.mail
-            user.is_staff = admin
-            user.save()
-
-        return user
+        return django_user

     def get_user(self, user_id):
         """
rmassoth commented 9 years ago

Good call. I'm new to django so I forgot about the awesome db api available. It could suffer from an issue with sqlite here but I think it will work for most cases.

sjkingo commented 9 years ago

Hmm that could cause issues with UTF-8 usernames, but perhaps we should just link to that section in the README with a caveat that if you use sqlite bad things may happen.

Personally I have always exclusively used postgres so I've never come across that issue.

I will test against AD and OpenLDAP later and see if that patch works.

rmassoth commented 9 years ago

I am testing your changes right now too. I'm using sqlite for testing but my live site will use postgres. Doing iexact could still be a performace issue if you have a lot of users but in practice it's probably fine for most.

sjkingo commented 9 years ago

Could you try and test rev 40a7515 (the master branch) now, as I believe this should work correctly now. I've tested with both AD and OpenLDAP and it seems to behave as expected:

I haven't tested sqlite though.

rmassoth commented 9 years ago

Works great. I used sqlite so as long as characters are ascii it works as intended. Thanks for your help.

sjkingo commented 9 years ago

No worries at all, I'll push a new version right now, and add you to AUTHORS. Thanks for your help!

Out of curiosity, what happens on sqlite when a UTF-8 username is found in LDAP? Does it break sanely? It might be worth putting a note in the README about that if so.

sjkingo commented 9 years ago

I've just pushed version 0.9.3 to pypi that contains this bug fix.