Logging in with LDAP user fails when user has multiple emails in right conditions

[aleksihakli]

Relates to the currently open PR #29 and older PR #17.

Logging in with a user accounts fails under the right conditions if the user that logs in has multiple email addresses, because a wrong address can be fetched and an update can be attempted, but the update attempts to save the wrong UserEmail with conflicting email when the email value exists in another UserEmail object.

Steps to reproduce:

1. there is an User with at least two UserEmail objects with e.g. the following addresses:

user@example.com
user@example.org

2. the User is trying to log in with user@example.org address
3. the LDAP backend attempts to fetch the UserEmail objects
4. the LDAP fetches two addresses and uses a randomly selected one for storing the user email; in the buggy case it uses e.g. user@example.com.
5. the LDAP backend attempts to update the randomly selected UserEmail with existing user@example.com value with the new user@example.org, but that UserEmail already exists with the same (pk, email='user@example.org') combination, which leads to the database throwing an IntegrityError and the user getting a failed login

An example traceback follows. Please note that the line number errors are not from the current master branch but from 2.5 release tag.

web_1        | 172.19.0.1 - - [19/Apr/2018:13:14:58 +0000] "GET / HTTP/1.0" 302 513 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36"
web_1        | 172.19.0.1 - - [19/Apr/2018:13:14:58 +0000] "GET /auth/login/ HTTP/1.0" 302 614 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36"
web_1        | 172.19.0.1 - - [19/Apr/2018:13:14:58 +0000] "GET /auth/login/example/ HTTP/1.0" 200 10742 "-" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36"
web_1        | Traceback (most recent call last):
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 112, in get_response
web_1        |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/views/generic/base.py", line 69, in view
web_1        |     return self.dispatch(request, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/views/decorators/csrf.py", line 57, in wrapped_view
web_1        |     return view_func(*args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/web/frontend/base.py", line 212, in dispatch
web_1        |     return self.handle(request, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/views/decorators/cache.py", line 52, in _wrapped_view_func
web_1        |     response = view_func(request, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/transaction.py", line 371, in inner
web_1        |     return func(*args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/web/frontend/auth_organization_login.py", line 69, in handle
web_1        |     response = self.handle_basic_auth(request, organization)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/web/frontend/auth_login.py", line 118, in handle_basic_auth
web_1        |     elif login_form.is_valid():
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/forms/forms.py", line 129, in is_valid
web_1        |     return self.is_bound and not bool(self.errors)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/forms/forms.py", line 121, in errors
web_1        |     self.full_clean()
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/forms/forms.py", line 274, in full_clean
web_1        |     self._clean_form()
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/forms/forms.py", line 300, in _clean_form
web_1        |     self.cleaned_data = self.clean()
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/web/forms/accounts.py", line 155, in clean
web_1        |     self.user_cache = authenticate(username=username, password=password)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/contrib/auth/__init__.py", line 49, in authenticate
web_1        |     user = backend.authenticate(**credentials)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django_auth_ldap/backend.py", line 173, in authenticate
web_1        |     user = ldap_user.authenticate(password)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django_auth_ldap/backend.py", line 350, in authenticate
web_1        |     self._get_or_create_user()
web_1        |   File "/usr/local/lib/python2.7/site-packages/django_auth_ldap/backend.py", line 577, in _get_or_create_user
web_1        |     self._user, created = self.backend.get_or_create_user(username, self)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry_ldap_auth/backend.py", line 42, in get_or_create_user
web_1        |     userEmail.save()
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/base.py", line 545, in save
web_1        |     force_update=force_update, update_fields=update_fields)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/base.py", line 573, in save_base
web_1        |     updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/base.py", line 635, in _save_table
web_1        |     forced_update)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/base.py", line 679, in _do_update
web_1        |     return filtered._update(values) > 0
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/query.py", line 510, in _update
web_1        |     return query.get_compiler(self.db).execute_sql(None)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 980, in execute_sql
web_1        |     cursor = super(SQLUpdateCompiler, self).execute_sql(result_type)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/models/sql/compiler.py", line 786, in execute_sql
web_1        |     cursor.execute(sql, params)
web_1        |   File "/usr/local/lib/python2.7/site-packages/raven/contrib/django/client.py", line 112, in execute
web_1        |     return real_execute(self, sql, params)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/backends/util.py", line 53, in execute
web_1        |     return self.cursor.execute(sql, params)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/utils.py", line 99, in __exit__
web_1        |     six.reraise(dj_exc_type, dj_exc_value, traceback)
web_1        |   File "/usr/local/lib/python2.7/site-packages/django/db/backends/util.py", line 53, in execute
web_1        |     return self.cursor.execute(sql, params)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/decorators.py", line 80, in inner
web_1        |     raise_the_exception(self.db, e)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/decorators.py", line 78, in inner
web_1        |     return func(self, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/decorators.py", line 22, in inner
web_1        |     return func(self, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/decorators.py", line 101, in inner
web_1        |     six.reraise(exc_info[0], exc_info[0](msg), exc_info[2])
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/decorators.py", line 94, in inner
web_1        |     return func(self, sql, *args, **kwargs)
web_1        |   File "/usr/local/lib/python2.7/site-packages/sentry/db/postgres/base.py", line 39, in execute
web_1        |     return self.cursor.execute(sql, params)
web_1        | IntegrityError: IntegrityError('duplicate key value violates unique constraint "sentry_useremail_user_id_469ffbb142507df2_uniq"\nDETAIL:  Key (user_id, email)=(13, user@example.org) already exists.\n',)
web_1        | SQL: UPDATE "sentry_useremail" SET "user_id" = %s, "email" = %s, "validation_hash" = %s, "date_hash_added" = %s, "is_verified" = %s WHERE "sentry_useremail"."id" = %s
web_1        | 13:14:59 [ERROR] django.request: Internal Server Error: /auth/login/example/ (status_code=500 request=<WSGIRequest: POST u'/auth/login/example/'>)

The erronous line is here:

https://github.com/Banno/getsentry-ldap-auth/blob/master/sentry_ldap_auth/backend.py#L39

There is an suggested fix at:

https://github.com/ralphje/getsentry-ldap-auth/blob/9b5d356e46ada90843552dc99ee8363071944a8f/sentry_ldap_auth/backend.py

The only problem with the fix is that it changes the empty email identifier from ' ' to '' but does not remove old empty UserEmails that have been created with or set to ' ', creating potential problems in the populated databases. Old getsentry-ldap-auth email addresses won't work with this fix.

An alternative fix would be:

# use Q for fetching empty UserEmails with '' or ' ' as email value 
from django.db.models import Q

class SentryLdapBackend(LDAPBackend):
    def get_or_create_user(self, username, ldap_user):
        # ...
        try:    
            from sentry.models import (UserEmail)
        except ImportError:
            pass
        else:
            if 'mail' in ldap_user.attrs:
                email = ldap_user.attrs.get('mail')[0]        
            elif hasattr(settings, 'AUTH_LDAP_DEFAULT_EMAIL_DOMAIN'):
                email = username + '@' + settings.AUTH_LDAP_DEFAULT_EMAIL_DOMAIN
            else:
                email = ''

            if email:
                # note this change, which also removes unnecessary empty UserEmails,
                # but only in the case where we have a better alternative
                UserEmail.objects.filter(user=user, Q(email='') | Q(email=' ')).delete()
                UserEmail.objects.get_or_create(user=user, email=email)
        # ...

This alternative would allow a few things:

• User not having empty emails in the Sentry database
• User not getting his or her UserEmail objects overridden by getsentry-auth-ldap
• Backwards compatibility for empty ' ' ("empty" email for getsentry-ldap-auth) and '' (empty email for Sentry) addresses for the user.

The current implementation just randomly overwrites the first UserEmail address is fetches if the user does not have a matching UserEmail but has something in the database.

The new implementation on the other hand just cleans up old empty email addresses from the database and inserts a new clean email if there is one available.

[aleksihakli]

This should be fixed now that #29 is merged into master so I'm closing this :)