Skip to content

Commit

Permalink
models: make sure uid is compared case-sensitive
Browse files Browse the repository at this point in the history
The database collation might vary, so use safer comparing in Python.
  • Loading branch information
nijel committed Apr 24, 2024
1 parent 7033ff7 commit 31c3e0c
Showing 1 changed file with 9 additions and 5 deletions.
14 changes: 9 additions & 5 deletions social_django/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,15 @@ class Meta:
abstract = True

@classmethod
def get_social_auth(cls, provider, uid):
try:
return cls.objects.select_related("user").get(provider=provider, uid=uid)
except cls.DoesNotExist:
return None
def get_social_auth(cls, provider: str, uid: str):
for social in cls.objects.select_related("user").filter(
provider=provider, uid=uid
):
# We need to compare to filter out case-insensitive lookups in
# some databases (MySQL/MariaDB)
if social.uid == uid:
return social
return None

@classmethod
def username_max_length(cls):
Expand Down

2 comments on commit 31c3e0c

@snstanton
Copy link

@snstanton snstanton commented on 31c3e0c Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this change is correct. If you have a case-insensitive column storing the data, then requiring a case-sensitive comparison in python will break some lookups. The correct way to handle this is to either make the column collation case-sensitive, or to case-fold the data on the way in when it is saved or looked up.

@nijel
Copy link
Member Author

@nijel nijel commented on 31c3e0c Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better solutions are welcome. Doing the column case-sensitive requires MySQL-specific code (all other Django database backends default to case sensitive collations), which I didn't have time and motivation to develop and test.

Case-folding would break the authentication for providers relying on IDs being case-sensitive (and that's what triggered this fix), so it is definitely not an option.

Please sign in to comment.