Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Username case sensitivity #7

Closed
rmassoth opened this issue Jul 1, 2015 · 16 comments
Closed

Username case sensitivity #7

rmassoth opened this issue Jul 1, 2015 · 16 comments
Labels
Milestone

Comments

@rmassoth
Copy link

rmassoth commented Jul 1, 2015

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
Copy link
Owner

sjkingo commented Jul 1, 2015

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.

@sjkingo sjkingo added the bug label Jul 1, 2015
@rmassoth
Copy link
Author

rmassoth commented Jul 1, 2015

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
Copy link
Owner

sjkingo commented Jul 3, 2015

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.

@sjkingo sjkingo added this to the v1.0 milestone Jul 4, 2015
@rmassoth
Copy link
Author

rmassoth commented Jul 6, 2015

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
Copy link
Owner

sjkingo commented Jul 6, 2015

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
Copy link
Author

rmassoth commented Jul 6, 2015

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
Copy link
Owner

sjkingo commented Jul 6, 2015

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

@sjkingo
Copy link
Owner

sjkingo commented Jul 6, 2015

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

  • Bob == Bob
  • Bob == bob
  • BoB == BOB
  • etc.

Do you think that will work?

@sjkingo
Copy link
Owner

sjkingo commented Jul 6, 2015

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
Copy link
Author

rmassoth commented Jul 6, 2015

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
Copy link
Owner

sjkingo commented Jul 6, 2015

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
Copy link
Author

rmassoth commented Jul 6, 2015

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 sjkingo closed this as completed in 40a7515 Jul 6, 2015
@sjkingo
Copy link
Owner

sjkingo commented Jul 6, 2015

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:

  • Logging in for the first time as BOBby when the LDAP username is bobby creates a new Django user bobby
  • Using any form of bobby succeeds in logging in, and not creating a duplicate user account

I haven't tested sqlite though.

@rmassoth
Copy link
Author

rmassoth commented Jul 6, 2015

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

@sjkingo
Copy link
Owner

sjkingo commented Jul 6, 2015

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 added a commit that referenced this issue Jul 6, 2015
@sjkingo
Copy link
Owner

sjkingo commented Jul 6, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants