feature/92/implement-login-endpoint#247
Conversation
Markian-Grybok
requested review from
Skiper29,
VladysMoroz,
denys-pavskyi,
oleksandrhofner and
weazy12
WalkthroughAdds a MediatR-based login flow: request/handler, validators, Web API endpoint, and localization for auth errors. Removes DTO data annotations in favor of FluentValidation. Updates project resources and introduces unit tests for the handler and validators. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as AuthController
participant M as Mediator
participant H as LoginHandler
participant UM as UserManager
participant JWT as IJwtTokenService
participant L as ILoggerService
C->>API: POST /auth/login { UserLoginDTO }
API->>M: Send(LoginCommand)
M->>H: Handle(LoginCommand)
H->>UM: FindByEmailAsync(login)
alt User not found
H->>L: Log error
H-->>M: Result.Fail(IncorrectEmailOrPassword)
else User found
H->>UM: CheckPasswordAsync(user, password)
alt Wrong password
H->>L: Log error
H-->>M: Result.Fail(IncorrectEmailOrPassword)
else Valid password
H->>JWT: GenerateTokenAsync(user.Id)
H-->>M: Result.Ok(LoginResultDTO{ token })
end
end
M-->>API: Result
alt Success
API-->>C: 200 OK { token }
else Failure
API-->>C: 400 BadRequest { errors }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs (1)
34-37: Standardize error response shape across endpointsChangePassword returns
BadRequest(result)while Register/Login returnBadRequest(result.Errors.Select(...)). Consider unifying the error payload shape for consistency and cleaner clients.Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs (1)
12-18: Stop-on-first failure and validate emptiness before formatReorder to avoid dual errors on empty input and stop cascading once NotEmpty fails. Consider built-in EmailAddress for maintainability.
- RuleFor(x => x.Login) - .Matches(@"^(?!.*\.\.)[a-zA-Z0-9_%+-]+(?:\.[a-zA-Z0-9_%+-]+)*@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$").WithMessage(Errors_Validation.EmailAddressFormat.FormatWith("Login")) - .NotEmpty().WithMessage(Errors_Validation.CannotBeEmpty.FormatWith("Login")); + RuleFor(x => x.Login) + .Cascade(CascadeMode.Stop) + .NotEmpty().WithMessage(Errors_Validation.CannotBeEmpty.FormatWith("Login")) + .EmailAddress().WithMessage(Errors_Validation.EmailAddressFormat.FormatWith("Login"));Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginCommand.cs (1)
7-8: Public API naming: use PascalCase for record propertyAvoid camelCase public members. Recommend renaming
userLoginDTOtoUserLoginDto(orCredentials) for consistency.-public record LoginCommand(UserLoginDTO userLoginDTO) +public record LoginCommand(UserLoginDTO UserLoginDto) : IRequest<Result<LoginResultDTO>>;Follow-up: update usages in handler, validators, tests (e.g.,
request.UserLoginDto,RuleFor(c => c.UserLoginDto), and test expressions).Streetcode/Streetcode.BLL/Validators/Auth/LoginCommandValidator.cs (1)
8-11: Guard against null DTO before delegating to nested validatorIf
userLoginDTOis null, the nested validator won’t run and no error will be produced. Add a NotNull guard.- RuleFor(dto => dto.userLoginDTO).SetValidator(loginValidator); + RuleFor(c => c.userLoginDTO) + .NotNull() + .SetValidator(loginValidator);If you apply the property rename in LoginCommand, update to
c => c.UserLoginDto.Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginCommandValidatorTests.cs (1)
104-119: Add a test for null DTO to cover the NotNull guardCovers the case where the command is constructed with a null payload.
[Fact] public void Validate_ComplexValidEmail_ShouldPass() { // Arrange var command = new LoginCommand(new UserLoginDTO { Login = "user.name+test@subdomain.example.co.uk", Password = "ComplexPass123!" }); // Act var result = _validator.TestValidate(command); // Assert result.ShouldNotHaveAnyValidationErrors(); } + + [Fact] + public void Validate_NullDto_ShouldFail() + { + // Arrange + var command = new LoginCommand(null); + + // Act + var result = _validator.TestValidate(command); + + // Assert + result.ShouldHaveValidationErrorFor(x => x.userLoginDTO); + }Streetcode/Streetcode.BLL/Streetcode.BLL.csproj (1)
130-133: PublicResXFileCodeGenerator choiceUsing PublicResXFileCodeGenerator is appropriate if consumers outside BLL need these strings. If not, consider internal to reduce surface area.
Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginHandler.cs (1)
29-45: Consider SignInManager for lockout/2FA/email‑confirm flowsUsing UserManager for password checks bypasses Identity features (lockout on failure, 2FA, email confirmation). Prefer
SignInManager.CheckPasswordSignInAsync(user, password, lockoutOnFailure: true)and handle its result.Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginValidatorTests.cs (2)
137-152: Add whitespace cases and trimming behaviorCurrent validator doesn’t trim inputs; whitespace-only values may pass
Matches. Add tests (and optionally Trim in the validator).Apply this diff to extend tests:
+ [Fact] + public void Validate_LoginWithWhitespaceOnly_ShouldHaveError() + { + var dto = new UserLoginDTO { Login = " ", Password = "validPassword123" }; + var result = _validator.TestValidate(dto); + result.ShouldHaveValidationErrorFor(x => x.Login); + } + + [Fact] + public void Validate_PasswordWithWhitespaceOnly_ShouldHaveError() + { + var dto = new UserLoginDTO { Login = "test@example.com", Password = " " }; + var result = _validator.TestValidate(dto); + result.ShouldHaveValidationErrorFor(x => x.Password); + }Optionally update LoginValidator to normalize input:
- RuleFor(x => x.Login) + RuleFor(x => x.Login) + .Transform(x => x?.Trim()) .Matches(@"^(?!.*\.\.)[a-zA-Z0-9_%+-]+(?:\.[a-zA-Z0-9_%+-]+)*@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$").WithMessage(Errors_Validation.EmailAddressFormat.FormatWith("Login")) .NotEmpty().WithMessage(Errors_Validation.CannotBeEmpty.FormatWith("Login")); - RuleFor(x => x.Password) + RuleFor(x => x.Password) + .Transform(x => x?.Trim()) .NotEmpty().WithMessage(Errors_Validation.CannotBeEmpty.FormatWith("Password"));
8-170: Reduce duplication with [Theory]/InlineDataMultiple tests differ only by inputs. Consider parameterizing to speed up maintenance.
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (1)
118-125: Align exception-path assertions with secure handler behaviorIf the handler stops returning raw exception messages (recommended), assert a generic, localized message and still verify logging captured the exception.
Apply this diff:
- result.IsFailed.Should().BeTrue(); - result.Errors.Should().HaveCount(1); - result.Errors.First().Message.Should().Be(exception.Message); - _mockLogger.Verify(x => x.LogError(command, exception.Message), Times.Once); + result.IsFailed.Should().BeTrue(); + result.Errors.Should().HaveCount(1); + var genericMsg = Errors_Auth.IncorrectEmailOrPassword.FormatWith("Login", "******"); + result.Errors.First().Message.Should().Be(genericMsg); + _mockLogger.Verify(x => x.LogError(command, It.Is<string>(m => m.Contains(exception.Message))), Times.Once);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginCommand.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginHandler.cs(1 hunks)Streetcode/Streetcode.BLL/Resources/Errors.Auth.Designer.cs(1 hunks)Streetcode/Streetcode.BLL/Resources/Errors.Auth.resx(1 hunks)Streetcode/Streetcode.BLL/Streetcode.BLL.csproj(3 hunks)Streetcode/Streetcode.BLL/Validators/Auth/LoginCommandValidator.cs(1 hunks)Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs(1 hunks)Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs(2 hunks)Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginCommandValidatorTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginValidatorTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs
🧰 Additional context used
🧬 Code graph analysis (8)
Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginCommand.cs (2)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (2)
UserLoginDTO(140-147)LoginResultDTO(127-138)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginCommandValidatorTests.cs (3)
Streetcode/Streetcode.BLL/Validators/Auth/LoginCommandValidator.cs (2)
LoginCommandValidator(6-12)LoginCommandValidator(8-11)Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs (2)
LoginValidator(8-19)LoginValidator(10-18)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)
Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs (3)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (1)
UserLoginDTO(140-147)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)Streetcode/Streetcode.BLL/Util/Extensions/ResourceExtensions.cs (1)
FormatWith(5-8)
Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs (2)
Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginHandler.cs (1)
Task(29-51)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (5)
Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginHandler.cs (3)
LoginHandler(13-52)LoginHandler(19-27)Task(29-51)Streetcode/Streetcode.BLL/Util/Extensions/ResourceExtensions.cs (1)
FormatWith(5-8)Streetcode/Streetcode.BLL/Services/Logging/LoggerService.cs (1)
LogError(35-40)Streetcode/Streetcode.BLL/DTO/Users/UserDTO.cs (1)
UserDTO(6-26)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)
Streetcode/Streetcode.BLL/Validators/Auth/LoginCommandValidator.cs (1)
Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs (2)
LoginValidator(8-19)LoginValidator(10-18)
Streetcode/Streetcode.BLL/MediatR/Auth/Login/LoginHandler.cs (3)
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (2)
LoginResultDTO(127-138)User(149-159)Streetcode/Streetcode.BLL/Util/Extensions/ResourceExtensions.cs (1)
FormatWith(5-8)Streetcode/Streetcode.BLL/Services/Logging/LoggerService.cs (1)
LogError(35-40)
Streetcode/Streetcode.XUnitTest/BLL/Validators/Auth/LoginValidatorTests.cs (2)
Streetcode/Streetcode.BLL/Validators/Auth/LoginValidator.cs (2)
LoginValidator(8-19)LoginValidator(10-18)Streetcode/Streetcode.BLL/DTO/Users/UserLoginDTO.cs (1)
UserLoginDTO(5-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and analyze
🔇 Additional comments (5)
Streetcode/Streetcode.BLL/Resources/Errors.Auth.Designer.cs (1)
39-47: Resource wrapper looks fine; confirm public exposure is intendedThis auto-generated wrapper and base name alignment look correct. If these resources aren’t meant to be consumed outside BLL, consider switching the resx generator to internal to avoid leaking surface area.
Streetcode/Streetcode.BLL/Streetcode.BLL.csproj (2)
73-77: Resource wiring LGTMErrors.Auth resource/designer linkage is correctly configured.
170-172: Res.resx generator choice
ResXFileCodeGenerator(internal) is fine for internal-only resources. No action required.Streetcode/Streetcode.WebApi/Controllers/Auth/AuthController.cs (1)
14-17: Confirmed: BaseApiController routes by action (api/[controller]/[action]) — no ambiguous [HttpPost] matchesBaseApiController has [Route("api/[controller]/[action]")] (Streetcode/Streetcode.WebApi/Controllers/BaseApiController.cs:8-9), so [HttpPost] actions without explicit templates are disambiguated by action name; only duplicate action names would collide.
Streetcode/Streetcode.XUnitTest/BLL/MediatR/Auth/Login/LoginHandleTests.cs (1)
45-48: Mock return type consistency if service signature changesIf you decide to keep IJwtTokenService returning Result, update the mock accordingly.
Apply this diff (only if the service returns Result):
- _mockJwtTokenService.Setup(i => i.GenerateTokenAsync(user.Id)).ReturnsAsync(loginUserRes); + _mockJwtTokenService.Setup(i => i.GenerateTokenAsync(user.Id)).ReturnsAsync(Result.Ok(loginUserRes));
4 participants
dev
JIRA
Code reviewers
Second Level Review
Summary of issue
ToDo
Summary of change
ToDo
Testing approach
ToDo
CHECK LIST
Summary by CodeRabbit