Normalize auth email handling to prevent false login failures#45
Conversation
Agent-Logs-Url: https://github.com/mukund58/taskflow-dotnet/sessions/d1c2b1fd-5147-400b-a591-cc0a543fcf36 Co-authored-by: mukund58 <162794838+mukund58@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent authentication and registration failures caused by email casing and surrounding whitespace differences by normalizing emails during auth operations.
Changes:
- Added centralized email normalization (
Trim().ToLowerInvariant()) inAuthService. - Applied normalization to registration uniqueness checks, persisted user emails, and login lookups.
- Added
AuthServiceunit tests covering normalized persistence, login with case/whitespace variants, and duplicate detection across case variants.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Backend/Services/Implementations/AuthService.cs | Normalizes emails in Register and Login, and adds a NormalizeEmail helper. |
| Backend.Tests/Services/AuthServiceTests.cs | Adds tests validating normalized persistence, case/whitespace-tolerant login, and duplicate rejection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Assert.Equal("test.user@example.com", result.User!.Email); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
The added tests cover normalization when registration happens through AuthService, but they don’t cover backward compatibility for pre-existing users whose emails were stored with mixed casing/whitespace (or were updated elsewhere and not normalized). Adding a test that seeds a User directly into the context with a non-normalized Email and then verifies Login succeeds would prevent regressions where existing accounts get locked out after this change.
| [Fact] | |
| [Fact] | |
| public async Task Login_AllowsNormalizedEmailForExistingUserWithNonNormalizedStoredEmail() | |
| { | |
| await using var context = CreateContext(); | |
| var service = CreateService(context); | |
| var registrationResult = await service.Register(new RegisterDto | |
| { | |
| Name = "Test User", | |
| Email = "test.user@example.com", | |
| Password = "Password123!" | |
| }); | |
| Assert.True(registrationResult.Success); | |
| var existingUser = await context.Users.SingleAsync(); | |
| existingUser.Email = " TEST.User@Example.COM "; | |
| await context.SaveChangesAsync(); | |
| var result = await service.Login(new LoginDto | |
| { | |
| Email = "test.user@example.com", | |
| Password = "Password123!" | |
| }); | |
| Assert.True(result.Success); | |
| Assert.False(string.IsNullOrWhiteSpace(result.Token)); | |
| Assert.NotNull(result.User); | |
| Assert.Equal(" TEST.User@Example.COM ", result.User!.Email); | |
| } | |
| [Fact] |
| var existingUser = await _context.Users | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(x => x.Email == dto.Email && !x.IsDeleted); | ||
| .FirstOrDefaultAsync(x => x.Email == normalizedEmail && !x.IsDeleted); | ||
|
|
There was a problem hiding this comment.
Register still enforces email uniqueness only at the application level (query then insert). Without a database-level unique constraint/index on the canonical email value, concurrent registrations can still create duplicate accounts for the same email (race condition), and the new normalization makes this check more central. Consider adding a DB unique index/constraint on a normalized/canonical email (e.g., CITEXT column or a unique index on lower(trim(email))) to make this robust.
| var existingUser = await _context.Users | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(x => x.Email == dto.Email && !x.IsDeleted); | ||
| .FirstOrDefaultAsync(x => x.Email == normalizedEmail && !x.IsDeleted); |
There was a problem hiding this comment.
Register now normalizes the input email and then does a strict equality lookup (x.Email == normalizedEmail). This will miss any existing users whose Email was previously stored with different casing/whitespace (including users created before this change or updated via other flows like ProfileService), which can allow duplicate registrations for the same logical email and can fragment accounts. Consider either (a) migrating/normalizing existing stored emails (and enforcing it everywhere emails are written) or (b) performing the lookup using a normalized comparison on the stored value as well (e.g., trimming/lowercasing in the query) until data is fully normalized, ideally backed by a DB unique constraint on the normalized value.
| .FirstOrDefaultAsync(x => x.Email == normalizedEmail && !x.IsDeleted); | |
| .FirstOrDefaultAsync(x => x.Email != null && x.Email.Trim().ToLower() == normalizedEmail && !x.IsDeleted); |
| var user = await _context.Users | ||
| .AsNoTracking() | ||
| .FirstOrDefaultAsync(x => x.Email == dto.Email && !x.IsDeleted); | ||
| .FirstOrDefaultAsync(x => x.Email == normalizedEmail && !x.IsDeleted); |
There was a problem hiding this comment.
Login uses normalizedEmail but still compares with strict equality against the stored Email (x.Email == normalizedEmail). If existing rows contain mixed-case or whitespace (or if other parts of the app persist non-normalized emails), those users will no longer be able to log in even if they enter the same email they previously used. To avoid locking out existing accounts, either normalize/migrate stored emails, or normalize the stored value in the query (e.g., trim/lowercase) as a compatibility bridge.
| .FirstOrDefaultAsync(x => x.Email == normalizedEmail && !x.IsDeleted); | |
| .FirstOrDefaultAsync(x => !x.IsDeleted && x.Email != null && x.Email.Trim().ToLower() == normalizedEmail); |
| private static string NormalizeEmail(string email) | ||
| { | ||
| return email.Trim().ToLowerInvariant(); | ||
| } |
There was a problem hiding this comment.
AuthService introduces its own NormalizeEmail helper, but other services (e.g., ProjectService) already have similar logic and ProfileService currently trims but does not lowercase on persistence. This inconsistency means user emails can end up stored in multiple formats, reintroducing auth/duplicate-check issues. Consider centralizing normalization in a shared utility and using it consistently anywhere User.Email is written (including profile updates), plus aligning uniqueness checks accordingly.
Users could fail login with valid credentials when the entered email had different casing or surrounding whitespace than the stored value. The auth flow treated these as different strings during lookup.
Problem summary
Backend auth behavior changes
Trim().ToLowerInvariant()) inAuthService.Registeruniqueness checkRegisterpersistenceLoginuser lookupRegression coverage
AuthServicetests for: