656232 - [auth] Unicode comparison problems from LDAP

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Peter Bengtsson [:peterbe]]

If LDAP sends first- or last names in non-ascii byte code it fails to compare against the Unicode strings we have in the database. 

Also there was a bug related to `<userinstance>.last_name` not being set.

Comment 1

[Peter Bengtsson [:peterbe]]

Attachment: coverage of backends.py

Disclaimer: 100% coverage does NOT mean it's perfect in any way.

Comment 2

[Peter Bengtsson [:peterbe]]

Attachment: Fixes Unicode problem hopefully

Note: This patch includes moving auth/backends.py from "apps/l10n_site" to "lib" that's why the big fat "cacert.pem" is taking up so much space of the patch.

Tests are run like this:

    ./manage.py test -s lib.auth.tests

Note that the patch includes a new file called `requirements/dev.txt` which you need to `pip -r` to be able to run the tests. 

This patch assumes that the LDAP sometimes sends the first- and last name incorrectly as LATIN-1 (aka. ISO-8859-1)

Comment 3

[Peter Bengtsson [:peterbe]]

Attachment: Fixes Unicode problem hopefully diff with -M70%

This time with -M70% (thanks stas)

Comment 4

[Peter Bengtsson [:peterbe]]

Attachment: Mocking the ldap non-ascii byte strings in UTF-8 instead

UTF-8 in tests.py instead

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 531574 [details] [diff] [review]
Fixes Unicode problem hopefully diff with -M70%

Review of attachment 531574 [details] [diff] [review]:
-----------------------------------------------------------------

r-, we're not doing the latin1 stuff, but just plain unicode. Tested with python shell and discussed over IRC.

The basic failure for anything non-ascii is that the default encoding is ascii, and that throws. Independent on whether the data would be latin-1 or utf-8.

For setting, this probably works thanks to data-mangling hooks in django fields, I guess. Just the comparison fails.

::: apps/l10n_site/auth/backends.py
@@ +90,5 @@
> +            # 'Pet\xc3\xa3r' which is the correct UTF-8 version.
> +            try:
> +                first_name = force_unicode(first_name)
> +            except DjangoUnicodeDecodeError:
> +                first_name = force_unicode(first_name, encoding='latin1')

We don't need to do a latin1 dance here, our data should be utf-8 encoded.

If it's not, no idea what to fall back to, tbh. Log something and leave it empty?

@@ +94,5 @@
> +                first_name = force_unicode(first_name, encoding='latin1')
> +            try:
> +                last_name = force_unicode(last_name)
> +            except DjangoUnicodeDecodeError:
> +                last_name = force_unicode(last_name, encoding='latin1')

... same here.

::: lib/auth/tests.py
@@ +54,5 @@
> +          ('mail=[e-mail],...',
> +           {'cn': ['Peter Bengtsson'],
> +            'givenName': ['Pet\xe3r'], # latin-1 byte string
> +            'mail': ['[e-mail]'],
> +            'sn': ['Bengtss\xa2n'],

Make these utf-8 strings instead, please?

@@ +131,5 @@
> +        ok_(user)
> +        _first_name = self.fake_user[0][1]['givenName'][0]
> +        eq_(user.first_name, unicode(_first_name, 'latin1'))
> +        _last_name = self.fake_user[0][1]['sn'][0]
> +        eq_(user.last_name, unicode(_last_name, 'latin-1'))

utf-8 here, too.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 531583 [details] [diff] [review]
Mocking the ldap non-ascii byte strings in UTF-8 instead

You're adding the ldap tests to build.sh later, I assume?

Comment 7

[Peter Bengtsson [:peterbe]]

Landed.
https://github.com/mozilla/elmo/commit/cf27ad585afdca80023c7b5b4e682331f12ec346

It gets included in the way we run tests now. Sadly I forgot to update the script/build.sh file to the new way as I have it set up on Jenkins.