This repository has been archived by the owner on Sep 24, 2023. It is now read-only.
Fix IntegrityError when user has multiple email addresses #29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Since you can set up multiple email addresses for users, it is possible that sentry_ldap_auth updates the wrong email address when changing the UserEmail field. Say, you have two UserEmail objects for an user, 'personal' and 'system-wide'. When 'personal' gets updated to "system-wide", this generates an IntegrityError because (user, email) is unique in the database, preventing logon. I doubt this entire structure is even necessary because when the 'email' attribute is set correctly on ``AUTH_LDAP_USER_ATTR_MAP`` this should all happen automatically.
aleksihakli
suggested changes
Apr 19, 2018
| userEmail.email = ldap_user.attrs.get('mail')[0] | ||
| email = ldap_user.attrs.get('mail')[0] | ||
| elif not hasattr(settings, 'AUTH_LDAP_DEFAULT_EMAIL_DOMAIN'): | ||
| email = '' |
There was a problem hiding this comment.
This changes the currently "empty" getsentry-ldap-auth email from ' ' to '', possibly leaving dangling database rows with email=' '.
| email = username + '@' + settings.AUTH_LDAP_DEFAULT_EMAIL_DOMAIN | ||
|
|
||
| # django-auth-ldap may have accidentally created an empty email address | ||
| UserEmail.objects.filter(user=user, email='').delete() |
There was a problem hiding this comment.
This should be something like
from django.db.models import Q
# ...
UserEmail.objects.filter(user=user, Q(email='') | Q(email=' ')).delete()
|
@barronhagerman is it possible to get this baked into a new release, e.g. 2.6? We have ongoing issues with users inputting custom emails and failing authentication over LDAP afterwards. |
barronhagerman
approved these changes
Apr 20, 2018
There was a problem hiding this comment.
Sorry I'm just getting around to this. I don't have the problem you've described, and I don't have a good environment to test it. However, I do think this should fix it without breaking something else. I'll try to get a release cut next week with this and the updates @aleksihakli brought up.
barronhagerman
pushed a commit
that referenced
this pull request
Apr 20, 2018
Since you can set up multiple email addresses for users, it is possible that sentry_ldap_auth updates the wrong email address when changing the UserEmail field. Say, you have two UserEmail objects for an user, 'personal' and 'system-wide'. When 'personal' gets updated to "system-wide", this generates an IntegrityError because (user, email) is unique in the database, preventing logon. I doubt this entire structure is even necessary because when the 'email' attribute is set correctly on ``AUTH_LDAP_USER_ATTR_MAP`` this should all happen automatically.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since you can set up multiple email addresses for users, it is possible that sentry_ldap_auth updates the wrong email address when changing the UserEmail field.
Say, you have two UserEmail objects for an user, 'personal' and 'system-wide'. When 'personal' gets updated to "system-wide", this generates an IntegrityError because (user, email) is unique in the database, preventing logon.
Instead of changing the attribute, I add it to the list of the user's email addresses. There's no reliable way to remove an old email address. I also do not add any empty email addresses.
I doubt this entire structure is even necessary because when the 'email' attribute is set correctly on
AUTH_LDAP_USER_ATTR_MAPthis should all happen automatically, since Sentry has added a signal on User creation to automatically create the UserEmail object.I do delete any lingering UserEmails that are empty (this is the case if the map is not set up correctly, since django-auth-ldap will then create a User object without an email address).