Case-insensitive GitHub username matching at login#449
Conversation
GitHub usernames are case-insensitive, meaning 'JohnDoe', 'johndoe', and 'JOHNDOE' all refer to the same GitHub account. This change ensures that when a user logs in via GitHub, we match their username case-insensitively against existing users, while preserving the latest case from GitHub for display purposes. Changes: - User.get_by_type_and_id: For GitHub users, uses iexact lookup - User.update_or_create: For GitHub users, does case-insensitive lookup and updates the stored user_id to preserve the current case from GitHub - Added GitHubUserTests with 4 tests to verify the behavior
Instead of hardcoding 'github' checks in the User model, auth systems can now opt into case-insensitive user ID matching by setting CASE_INSENSITIVE_USER_ID = True in their module. Changes: - Added uses_case_insensitive_user_id() helper in auth_systems/__init__.py - Added CASE_INSENSITIVE_USER_ID = True to github.py - Updated User model to use the helper instead of hardcoded checks This makes it easy for other auth systems to opt into case-insensitive matching in the future without modifying the core User model.
There was a problem hiding this comment.
Pull request overview
This PR implements case-insensitive username matching for GitHub authentication while preserving the display case from the most recent login. GitHub usernames are inherently case-insensitive (e.g., 'JohnDoe', 'johndoe', and 'JOHNDOE' all refer to the same user), and this change ensures the authentication system respects that behavior.
Key changes:
- Added a new
CASE_INSENSITIVE_USER_IDflag to the GitHub auth system - Modified
get_by_type_and_idandupdate_or_createmethods to use case-insensitive lookups for applicable auth systems - Comprehensive test suite added to verify case-insensitive matching and case preservation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| helios_auth/auth_systems/init.py | Added uses_case_insensitive_user_id helper function to check if an auth system has case-insensitive user IDs |
| helios_auth/auth_systems/github.py | Declared GitHub auth system as having case-insensitive user IDs via CASE_INSENSITIVE_USER_ID = True |
| helios_auth/models.py | Modified get_by_type_and_id to use user_id__iexact for case-insensitive auth systems; updated update_or_create to handle case-insensitive lookups and preserve the most recent case |
| helios_auth/tests.py | Added comprehensive test suite covering case-insensitive matching, case preservation, and verification that password auth remains case-sensitive |
Comments suppressed due to low confidence (1)
helios_auth/models.py:69
- The code doesn't handle the case where info parameter is None. When updating an existing user, line 69 will attempt to access info['password'], which will raise a TypeError if info is None. Add a check to ensure info is not None before attempting to preserve the password.
if not created_p:
# special case the password: don't replace it if it exists
if 'password' in obj.info:
info['password'] = obj.info['password']
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if case_insensitive: | ||
| try: | ||
| obj = cls.objects.get(user_type=user_type, user_id__iexact=user_id) | ||
| created_p = False | ||
| except cls.DoesNotExist: | ||
| obj = cls.objects.create(user_type=user_type, user_id=user_id, name=name, info=info, token=token) | ||
| created_p = True |
There was a problem hiding this comment.
The try-except-create pattern introduces a race condition. If two requests attempt to create the same user (with different casing) simultaneously, both might pass the DoesNotExist check and attempt to create, causing an IntegrityError. Consider using a database transaction with select_for_update or handling IntegrityError to retry the get operation.
There was a problem hiding this comment.
fair, but seems extremely unlikely so leaving it as is.
| if case_insensitive: | ||
| try: | ||
| obj = cls.objects.get(user_type=user_type, user_id__iexact=user_id) | ||
| created_p = False | ||
| except cls.DoesNotExist: | ||
| obj = cls.objects.create(user_type=user_type, user_id=user_id, name=name, info=info, token=token) | ||
| created_p = True |
There was a problem hiding this comment.
The unique constraint on (user_type, user_id) is case-sensitive in the database, which means 'github' users with user_ids 'JohnDoe' and 'johndoe' could both be created if they bypass the application-level check. This could happen due to race conditions or direct database manipulation. Consider adding a database-level case-insensitive unique constraint or index for GitHub users, or implementing a migration to normalize existing user_ids to lowercase.
There was a problem hiding this comment.
yep, we'll leave it as is.
| class GitHubUserTests(TestCase): | ||
| """ | ||
| Tests specific to GitHub authentication, particularly case-insensitive username matching. | ||
| GitHub usernames are case-insensitive, so 'JohnDoe', 'johndoe', and 'JOHNDOE' should all | ||
| refer to the same user. | ||
| """ | ||
|
|
||
| def test_github_case_insensitive_update_or_create(self): | ||
| """ | ||
| Test that update_or_create matches GitHub users case-insensitively | ||
| """ | ||
| # Create a user with mixed case | ||
| u1 = models.User.update_or_create( | ||
| user_type='github', | ||
| user_id='JohnDoe', | ||
| name='John Doe (JohnDoe)', | ||
| info={'email': 'john@example.com'} | ||
| ) | ||
|
|
||
| # Login again with lowercase - should return the same user | ||
| u2 = models.User.update_or_create( | ||
| user_type='github', | ||
| user_id='johndoe', | ||
| name='John Doe (johndoe)', | ||
| info={'email': 'john@example.com'} | ||
| ) | ||
|
|
||
| # Should be the same database record | ||
| self.assertEqual(u1.id, u2.id) | ||
|
|
||
| # The user_id should be updated to the new case | ||
| u2.refresh_from_db() | ||
| self.assertEqual(u2.user_id, 'johndoe') | ||
|
|
||
| def test_github_case_insensitive_get_by_type_and_id(self): | ||
| """ | ||
| Test that get_by_type_and_id finds GitHub users case-insensitively | ||
| """ | ||
| # Create a user with uppercase | ||
| models.User.update_or_create( | ||
| user_type='github', | ||
| user_id='TESTUSER', | ||
| name='Test User (TESTUSER)', | ||
| info={'email': 'test@example.com'} | ||
| ) | ||
|
|
||
| # Should find the user with lowercase | ||
| u = models.User.get_by_type_and_id('github', 'testuser') | ||
| self.assertEqual(u.user_id, 'TESTUSER') | ||
|
|
||
| # Should find the user with mixed case | ||
| u = models.User.get_by_type_and_id('github', 'TestUser') | ||
| self.assertEqual(u.user_id, 'TESTUSER') | ||
|
|
||
| def test_github_preserves_display_case(self): | ||
| """ | ||
| Test that the username case is preserved from the most recent login | ||
| """ | ||
| # First login with lowercase | ||
| u1 = models.User.update_or_create( | ||
| user_type='github', | ||
| user_id='myuser', | ||
| name='My User (myuser)', | ||
| info={'email': 'my@example.com'} | ||
| ) | ||
| self.assertEqual(u1.user_id, 'myuser') | ||
|
|
||
| # Second login with mixed case - should update stored case | ||
| u2 = models.User.update_or_create( | ||
| user_type='github', | ||
| user_id='MyUser', | ||
| name='My User (MyUser)', | ||
| info={'email': 'my@example.com'} | ||
| ) | ||
| self.assertEqual(u2.user_id, 'MyUser') | ||
|
|
||
| # Verify it's the same user | ||
| self.assertEqual(u1.id, u2.id) | ||
|
|
||
| def test_password_auth_still_case_sensitive(self): | ||
| """ | ||
| Test that password auth remains case-sensitive (not affected by GitHub changes) | ||
| """ | ||
| # Create a user with mixed case | ||
| u1 = models.User.update_or_create( | ||
| user_type='password', | ||
| user_id='TestUser@example.com', | ||
| name='Test User', | ||
| info={'password': 'hashed_password'} | ||
| ) | ||
|
|
||
| # Create another user with different case - should be a new user | ||
| u2 = models.User.update_or_create( | ||
| user_type='password', | ||
| user_id='testuser@example.com', | ||
| name='Test User 2', | ||
| info={'password': 'hashed_password2'} | ||
| ) | ||
|
|
||
| # Should be different users | ||
| self.assertNotEqual(u1.id, u2.id) | ||
|
|
||
|
|
There was a problem hiding this comment.
Consider adding a test case that verifies the behavior when two users with different case variations try to be created simultaneously. This would help ensure the race condition handling (if implemented) works correctly. Additionally, test the edge case where info parameter is None to ensure the password preservation logic doesn't crash.
2 participants
allow an auth system to declare itself as having case-insensitive usernames, but we preserve the case because sometimes displaying it as such is useful.
fixes #362