Feature/issue 2129 #2245
Feature/issue 2129 #2245
Conversation
YaroslavRosnovskyi
requested review from
Michael-Kolpakov,
YuliiaAndreieva,
ilko-dev,
lisaaksionova and
vlad-zhadan
WalkthroughThis pull request implements significant changes across authentication, analytics, user management, and testing modules. New components for Google sign-in have been added, including a DTO, interface, and service that validate tokens asynchronously. Legacy analytics commands and Instagram endpoints were removed or refactored to adopt async patterns and reduce dependencies. User handlers have been updated to use email rather than username, with corresponding adjustments in controllers and tests. Additionally, project files, configuration settings, helpers, and test suites have been reorganized and refactored for improved consistency and reliability. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant AC as AuthController
participant GS as GoogleService (via IGoogleService)
participant UM as UserManager
U->>AC: Sends GoogleLogin(GoogleLoginRequest)
AC->>GS: ValidateGoogleToken(idToken)
GS-->>AC: Returns token payload
AC->>UM: Retrieves user data
UM-->>AC: Returns user info
AC-->>U: Responds with auth token & user details
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🔭 Outside diff range comments (11)
Streetcode/Streetcode.BLL/Streetcode.BLL.csproj (1)
385-387:⚠️ Potential issueFix the empty Generator tag for FailedToCreateSharedResource.en-US.resx.
The Generator tag is empty, while other similar resource files use
ResXFileCodeGenerator. This could cause build issues.Apply this diff to fix the Generator tag:
<EmbeddedResource Update="Resources\SharedResource.FailedToCreateSharedResource.en-US.resx"> - <Generator></Generator> + <Generator>ResXFileCodeGenerator</Generator> </EmbeddedResource>Streetcode/Streetcode.BLL/MediatR/Users/UpdateForgotPassword/UpdateForgotPasswordHandler.cs (3)
38-39: 🛠️ Refactor suggestionAdd validation for token format before decoding.
The URL decoding of username and token should be protected against malformed input that could cause exceptions.
- var decodedUserName = Uri.UnescapeDataString(request.UpdateForgotPasswordDto.Username); - var decodedToken = Uri.UnescapeDataString(request.UpdateForgotPasswordDto.Token); + var decodedUserName = Uri.TryCreate( + request.UpdateForgotPasswordDto.Username, + UriKind.RelativeOrAbsolute, + out _) + ? Uri.UnescapeDataString(request.UpdateForgotPasswordDto.Username) + : request.UpdateForgotPasswordDto.Username; + + var decodedToken = Uri.TryCreate( + request.UpdateForgotPasswordDto.Token, + UriKind.RelativeOrAbsolute, + out _) + ? Uri.UnescapeDataString(request.UpdateForgotPasswordDto.Token) + : request.UpdateForgotPasswordDto.Token;
50-53: 🛠️ Refactor suggestionConsider adding token expiration validation.
The password reset flow should validate that the token hasn't expired before attempting to reset the password. This is a security best practice to limit the window of vulnerability.
+ // Verify token hasn't expired + var isValidToken = await _userManager.VerifyUserTokenAsync( + user, + _userManager.Options.Tokens.PasswordResetTokenProvider, + "ResetPassword", + decodedToken); + + if (!isValidToken) + { + string errorMessage = _localizer["PasswordResetTokenExpired"]; + _logger.LogError(request, errorMessage); + return Result.Fail(errorMessage); + } + var result = await _userManager.ResetPasswordAsync(
27-69: 💡 Verification agent🧩 Analysis chain
Consider implementing rate limiting for password reset attempts.
To prevent brute force attacks, consider implementing rate limiting on password reset attempts. This could be done through a middleware or by tracking attempts in a distributed cache.
Let me check if there's any existing rate limiting implementation:
🏁 Script executed:
#!/bin/bash # Search for rate limiting implementations rg -i "ratelimit" -A 5 rg -i "throttle" -A 5Length of output: 7315
Rate Limiting Not Applied to Password Reset Endpoint
While our codebase does implement rate limiting for email-related endpoints (as seen in the EmailController and middleware configuration), there is no rate limiting logic applied to the password reset functionality in
UpdateForgotPasswordHandler.cs. This omission could leave the forgot password workflow vulnerable to brute force attempts.
- Observation: The search results confirm that rate limiting is enabled for email endpoints, but there are no attributes or middleware configurations limiting password reset attempts.
- Suggestion: Consider applying similar rate limiting (e.g., using the
[EnableRateLimiting]attribute or middleware configuration in Program.cs) to this endpoint to protect against potential abuse.Streetcode/Streetcode.BLL/MediatR/Users/ForgotPassword/ForgotPasswordHandler.cs (1)
44-84: 💡 Verification agent🧩 Analysis chain
Consider additional security measures for the password reset flow.
While the current implementation is solid, consider these security enhancements:
- Add token expiration time limit
- Implement rate limiting for password reset requests
- Add more specific success/failure messages without exposing sensitive information
Example implementation for token expiration:
var token = await _userManager.GeneratePasswordResetTokenAsync(user); +// Configure token lifetime in startup +_userManager.Options.Tokens.PasswordResetTokenProvider = TokenOptions.DefaultProvider; +_userManager.Options.Tokens.PasswordResetTokenLifespan = TimeSpan.FromHours(24);Let's check if rate limiting is already implemented elsewhere:
🏁 Script executed:
#!/bin/bash # Search for rate limiting implementations rg -i "rate.*limit|throttle" --type csLength of output: 1859
Security Enhancements for Forgot Password Flow
- Token Expiration: There’s no explicit configuration for the password reset token’s lifespan in the ForgotPassword flow. Consider setting a token expiration (e.g., 24 hours) via the UserManager’s options in your startup configuration to reduce the window for token misuse.
- Rate Limiting: The repository shows that rate limiting is implemented—specifically in the WebAPI’s email endpoint (with an "EmailRateLimit" policy). Ensure that the Forgot Password endpoint (which consumes this MediatR command) is similarly protected to prevent abuse.
- Message Specificity: The current success/failure messages use localized strings and avoid exposing sensitive details, but you might revisit them to ensure they balance user clarity with security best practices.
Overall, while rate limiting is in place, explicitly configuring token expiration would be a valuable security enhancement.
Streetcode/Streetcode.WebApi/appsettings.IntegrationTests.json (1)
2-4: 🛠️ Refactor suggestionSecure database connection string.
The connection string contains sensitive credentials. Consider using environment variables or secure secrets management for the database credentials as well.
"ConnectionStrings": { - "DefaultConnection": "Server=localhost,1455;Database=Streetcode_IntegrationTests_Db;User Id=sa;Password=sdlLKMCD234!@#lkcmds;MultipleActiveResultSets=True;TrustServerCertificate=true" + "DefaultConnection": "Server=${DB_SERVER};Database=${DB_NAME};User Id=${DB_USER};Password=${DB_PASSWORD};MultipleActiveResultSets=True;TrustServerCertificate=true" }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Authentication/UserExtracter.cs (1)
28-30:⚠️ Potential issueFix incorrect role assignment.
The role assignment is using a hardcoded User role instead of the provided roleName parameter.
-IdentityRole role = RoleExtracter.Extract(nameof(UserRole.User)); +IdentityRole role = RoleExtracter.Extract(roleName); RoleExtracter.AddUserRole(testUser.Id, role.Id);Streetcode/Streetcode.BLL/MediatR/Users/GetByEmail/GetByEmailHandler.cs (1)
48-48:⚠️ Potential issueUpdate error message to reference email instead of username.
The error message still references username despite the transition to email-based queries.
- string errorMessage = _localizer["UserWithSuchUsernameNotExists"]; + string errorMessage = _localizer["UserWithSuchEmailNotExists"];Streetcode/Streetcode.WebApi/Controllers/Authentication/AuthController.cs (2)
40-53:⚠️ Potential issueVerify authentication state in Logout method
The removal of user ID validation from claims reduces security checks. Consider maintaining authentication validation to prevent unauthorized logout attempts.
[Authorize] [HttpPost] [ProducesResponseType(StatusCodes.Status200OK)] public async Task<IActionResult> Logout() { + var userId = User.FindFirstValue(ClaimTypes.NameIdentifier); + if (string.IsNullOrEmpty(userId)) + { + return Unauthorized("User not authenticated"); + } var result = await Mediator.Send(new LogoutCommand()); if (result.IsFailed) { return BadRequest(result.Errors.First().Message); } return Ok("Logout successful. Refresh token invalidated."); }
55-67: 🛠️ Refactor suggestionEnhance Google login security with reCAPTCHA v3
Since we're updating to reCAPTCHA v3, consider adding score validation to prevent automated attacks.
[HttpPost] [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(LoginResponseDTO))] public async Task<IActionResult> GoogleLogin(GoogleLoginRequest googleLoginRequest) { + if (googleLoginRequest?.IdToken == null) + { + return BadRequest("Invalid request"); + } + // Verify reCAPTCHA v3 score + if (!await _recaptchaService.ValidateToken(googleLoginRequest.RecaptchaToken, 0.5)) + { + return BadRequest("Invalid reCAPTCHA token"); + } var result = await Mediator.Send(new LoginGoogleQuery(googleLoginRequest.IdToken)); if (result.IsSuccess) { return Ok(result.Value); } return Unauthorized(new { message = result.Errors.FirstOrDefault()?.Message }); }Streetcode/Streetcode.BLL/MediatR/Authentication/LoginGoogle/LoginGoogleHandler.cs (1)
113-117: 🛠️ Refactor suggestionEnhance error handling with specific exception types.
The catch block catches all exceptions, which could mask specific issues. Consider handling different types of exceptions separately.
- catch (Exception ex) + catch (InvalidJwtException ex) + { + _logger.LogError(request, $"Invalid JWT token: {ex.Message}"); + return Result.Fail($"Invalid Google Token format: {ex.Message}"); + } + catch (GoogleJsonWebSignature.InvalidTokenException ex) + { + _logger.LogError(request, $"Token validation failed: {ex.Message}"); + return Result.Fail("Invalid or expired Google token"); + } + catch (Exception ex) { _logger.LogError(request, ex.Message); - return Result.Fail($"Invalid Google Token: {ex.Message}"); + return Result.Fail("An unexpected error occurred during authentication"); }
🧹 Nitpick comments (75)
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Delete/DeleteStatisticRecordHandler.cs (1)
29-55: Consider enhancing error handling and logging.While the implementation is generally solid, consider these improvements for better maintainability and debugging:
- Add transaction scope for atomic operations
- Include more context in error logs (e.g., QR ID value)
- Consider using a custom Result type for domain-specific errors
Here's a suggested enhancement:
public async Task<Result<Unit>> Handle(DeleteStatisticRecordCommand request, CancellationToken cancellationToken) { + using var transaction = await _repositoryWrapper.BeginTransactionAsync(); try { var statRecord = await _repositoryWrapper.StatisticRecordRepository .GetFirstOrDefaultAsync(predicate: (sr) => sr.QrId == request.qrId); if (statRecord is null) { string errorMsg = _stringLocalizerCannotFind["CannotFindRecordWithQrId"].Value; - _logger.LogError(request, errorMsg); + _logger.LogError(request, $"{errorMsg} QrId: {request.qrId}"); return Result.Fail(new Error(errorMsg)); } _repositoryWrapper.StatisticRecordRepository.Delete(statRecord); var resultIsSuccess = await _repositoryWrapper.SaveChangesAsync() > 0; if (resultIsSuccess) { + await transaction.CommitAsync(); return Result.Ok(Unit.Value); } else { string errorMsg = _stringLocalizerFailedToDelete["FailedToDeleteTheRecord"].Value; - _logger.LogError(request, errorMsg); + _logger.LogError(request, $"{errorMsg} QrId: {request.qrId}"); + await transaction.RollbackAsync(); return Result.Fail(new Error(errorMsg)); } + } + catch (Exception ex) + { + await transaction.RollbackAsync(); + _logger.LogError(request, $"Unexpected error deleting record for QrId: {request.qrId}. Error: {ex.Message}"); + throw; + } }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Analytics/StatisticRecordExtracter.cs (2)
8-22: Add documentation and improve parameter handling.Consider these enhancements for better maintainability and reliability:
- Add XML documentation to explain the method's purpose and parameters
- Add input validation
- Make coordinate values configurable or more realistic
Here's a suggested implementation:
+ /// <summary> + /// Extracts a test StatisticRecord with the specified ID and StreetcodeId + /// </summary> + /// <param name="id">The ID to assign to the StatisticRecord</param> + /// <param name="streetcodeId">The StreetcodeId to assign to both StatisticRecord and StreetcodeCoordinate</param> + /// <returns>A configured StatisticRecord for testing</returns> + /// <exception cref="ArgumentException">Thrown when id or streetcodeId is less than 1</exception> public static StatisticRecord Extract(int id, int streetcodeId) { + if (id < 1) + throw new ArgumentException("ID must be greater than 0", nameof(id)); + if (streetcodeId < 1) + throw new ArgumentException("StreetcodeId must be greater than 0", nameof(streetcodeId)); + StatisticRecord testStatisticRecord = TestDataProvider.GetTestData<StatisticRecord>(); StreetcodeCoordinate testStreetcodeCoordinate = new StreetcodeCoordinate { StreetcodeId = streetcodeId, - Latitude = 50, - Longtitude = 50, + Latitude = 50.450001, // Example: Kyiv latitude + Longtitude = 30.523333, // Example: Kyiv longitude };
24-27: Add documentation for the Remove method.The implementation is clean and focused. Consider adding XML documentation for consistency.
Here's a suggested addition:
+ /// <summary> + /// Removes a StatisticRecord from the test data store + /// </summary> + /// <param name="entity">The StatisticRecord entity to remove</param> public static void Remove(StatisticRecord entity)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Users/Expertises/ExpertiseControllerTests.cs (1)
14-21: Consider enhancing test data management.The test setup looks good with unique ID generation, but consider adding:
- Test data cleanup in a Dispose method
- A test data factory or constants class for reusable test cases
Example implementation:
private readonly Expertise _testExpertise; +private const string DEFAULT_EXPERTISE_NAME = "Test Expertise"; + +public void Dispose() +{ + // Cleanup test data after each test + // await Client.Delete(_testExpertise.Id); +}Streetcode/Streetcode.XIntegrationTest/ControllerTests/Analytics/StatisticRecordControllerTests.cs (4)
21-32: Consider extracting test data setup to a separate method.The constructor contains complex setup logic that could be moved to a dedicated method for better readability and maintainability.
public StatisticRecordControllerTests(CustomWebApplicationFactory<Program> factory, TokenStorage tokenStorage) : base(factory, "/api/StatisticRecord", tokenStorage) { - var uniqueStreetcodeId = UniqueNumberGenerator.GenerateInt(); - var uniqueStatisticRecordId = UniqueNumberGenerator.GenerateInt(); - StreetcodeContentExtracter - .Extract( - uniqueStreetcodeId, - uniqueStreetcodeId, - Guid.NewGuid().ToString()); - _statisticRecord = StatisticRecordExtracter.Extract(uniqueStatisticRecordId, uniqueStreetcodeId); + _statisticRecord = SetupTestData(); +} + +private StatisticRecord SetupTestData() +{ + var uniqueStreetcodeId = UniqueNumberGenerator.GenerateInt(); + var uniqueStatisticRecordId = UniqueNumberGenerator.GenerateInt(); + + StreetcodeContentExtracter.Extract( + uniqueStreetcodeId, + uniqueStreetcodeId, + Guid.NewGuid().ToString()); + + return StatisticRecordExtracter.Extract(uniqueStatisticRecordId, uniqueStreetcodeId); }
65-65: Consider adding error handling for JSON deserialization.The
CaseIsensitiveJsonDeserializer.Deserializecall could throw an exception if the response content is malformed.-var returnedValue = CaseIsensitiveJsonDeserializer.Deserialize<List<StatisticRecordDTO>>(response.Content); +List<StatisticRecordDTO> returnedValue; +try +{ + returnedValue = CaseIsensitiveJsonDeserializer.Deserialize<List<StatisticRecordDTO>>(response.Content); +} +catch (Exception ex) +{ + Assert.Fail($"Failed to deserialize response: {ex.Message}"); + return; +}
110-110: Consider using more robust boolean parsing.The current boolean parsing using
TryParsedoesn't handle the case where parsing fails.-bool.TryParse(response.Content, out bool result); +Assert.True(bool.TryParse(response.Content, out bool result), "Response content could not be parsed as boolean");
1-193: Consider adding test cases for edge cases and authorization scenarios.The test suite covers the basic CRUD operations well, but could benefit from additional test cases:
- Concurrent access scenarios
- Different user roles and permissions
- Rate limiting
- Invalid token scenarios
- Malformed request payloads
Would you like me to generate additional test cases for these scenarios?
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/ExistByQrId/ExistStatisticRecordByQrIdHandler.cs (1)
1-1: Remove unused AutoMapper import.The
AutoMapperimport is no longer needed since theIMapperdependency was removed.-using AutoMapper;Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/ExistStatisticRecordByQrIdTests.cs (1)
52-76: Consider adding edge cases.While the current tests cover basic positive and negative scenarios, consider adding tests for:
- Invalid QR IDs (e.g., 0 or extremely large values)
- Repository throwing exceptions
- Cancelled operations using CancellationToken
Example test case for exception handling:
[Fact] public async Task Handle_RepositoryThrows_ShouldReturnFailure() { // Arrange var qrId = 1; _repositoryWrapperMock.Setup(repo => repo.StatisticRecordRepository.GetFirstOrDefaultAsync( It.IsAny<Expression<Func<StatisticRecord, bool>>>(), It.IsAny<Func<IQueryable<StatisticRecord>, IIncludableQueryable<StatisticRecord, object>>>())) .ThrowsAsync(new Exception("Database error")); var request = new ExistStatisticRecordByQrIdCommand(qrId); // Act var result = await _handler.Handle(request, CancellationToken.None); // Assert Assert.False(result.IsSuccess); }Streetcode/Streetcode.XUnitTest/Validators/Users/UpdateUserValidatorTests.cs (2)
17-32: Consider enhancing mock setup for better test isolation.The mock setup is good, but could be improved in a few ways:
- Consider using constants for test values to make maintenance easier
- The base validator's setup could be more specific about what validation context it expects
Here's a suggested improvement:
+ private const string ValidUserName = "testusername"; + private const string ValidEmail = "testemail"; public UpdateUserValidatorTests() { mockNamesLocalizer = new MockFieldNamesLocalizer(); mockValidationLocalizer = new MockFailedToValidateLocalizer(); mockUserSharedResourceLocalizer = new MockUserSharedResourceLocalizer(); _mockRepositoryWrapper = new Mock<IRepositoryWrapper>(); mockBaseValidator = new Mock<BaseUserValidator>(mockValidationLocalizer, mockNamesLocalizer, _mockRepositoryWrapper.Object); - mockBaseValidator.Setup(x => x.Validate(It.IsAny<ValidationContext<UpdateUserDTO>>())) + mockBaseValidator.Setup(x => x.Validate(It.Is<ValidationContext<UpdateUserDTO>>( + ctx => ctx.InstanceToValidate.UserName == ValidUserName && + ctx.InstanceToValidate.Email == ValidEmail))) .Returns(new ValidationResult()); }
50-80: Enhance test data quality and validation.The test data could be improved in several ways:
- Email should follow a valid format (e.g., "test@example.com")
- PhoneNumber is marked as null but has a null! operator
- Consider using a test data builder pattern for more flexible test data creation
Here's a suggested improvement:
private UpdateUserDTO GetValidUser() { return new UpdateUserDTO { Name = "TestName", Surname = "TestSurname", UserName = "testusername", AboutYourself = null, AvatarId = null, Expertises = new List<ExpertiseDTO>() { new () { Id = 1, Title = "testTitle1" }, new () { Id = 2, Title = "testTitle2" }, new () { Id = 3, Title = "testTitle3" }, }, - PhoneNumber = null!, + PhoneNumber = null, - Email = "testemail", + Email = "test@example.com", }; }Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/GetAllByStreetcodeId/GetAllStatisticRecordsByStreetcodeIdHandler.cs (1)
38-43: Consider using null-coalescing operator for cleaner error handling.The current implementation can be simplified while maintaining the same functionality.
-if (!statisticRecords.Any()) -{ - string errorMsg = _stringLocalizerCannotFind["CannotFindRecordWithStreetcodeId", request.streetcodeId]; - _logger.LogError(request, errorMsg); - return Result.Fail(new Error(errorMsg)); -} +return !statisticRecords.Any() + ? Result.Fail(new Error(_stringLocalizerCannotFind["CannotFindRecordWithStreetcodeId", request.streetcodeId])) + .Tap(error => _logger.LogError(request, error.Message)) + : Result.Ok(_mapper.Map<IEnumerable<StatisticRecordDTO>>(statisticRecords));Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetAllStatisticRecordsByStreetcodeIdTests.cs (2)
40-72: Consider adding edge case tests.While the current tests cover basic scenarios well, consider adding tests for:
- Edge cases with very large counts
- Cases with duplicate QrIds
- Boundary conditions for streetcodeId
99-114: Consider using test data builder pattern.The test data creation could be made more maintainable using a builder pattern.
+private class StatisticRecordBuilder +{ + private readonly StatisticRecord _record = new(); + + public StatisticRecordBuilder WithId(int id) + { + _record.Id = id; + return this; + } + + public StatisticRecordBuilder WithQrId(int qrId) + { + _record.QrId = qrId; + return this; + } + + public StatisticRecord Build() => _record; +}Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetAllStatisticRecordsTests.cs (1)
41-71: Consider using Theory for parameterized testing.Convert the test to use [Theory] with [InlineData] to test multiple scenarios with different record counts and ordering.
-[Fact] -public async Task Handle_RecordsExist_ShouldReturnOrderedRecords() +[Theory] +[InlineData(new[] { 10, 20, 15 })] +[InlineData(new[] { 5, 5, 5 })] +[InlineData(new[] { 1 })] +public async Task Handle_RecordsExist_ShouldReturnOrderedRecords(int[] counts)Streetcode/Streetcode.XUnitTest/MediatRTests/Email/SendEmailHandlerTests.cs (4)
41-59: Enhance test coverage with additional assertions.While the test verifies basic success, consider adding assertions to validate:
- The reCAPTCHA token was actually validated
- The email content was correctly passed to the email service
// assert Assert.True(result.IsSuccess); +_mockHttpClientFactory.Verify(f => f.CreateClient(It.IsAny<string>()), Times.Once); +_mockEmailService.Verify(s => s.SendEmailAsync(It.Is<MessageData>(m => + m.Content == emailDto.Content && + m.From == emailDto.From)), Times.Once);
61-80: Verify email service is not called on reCAPTCHA validation failure.Add verification to ensure the email service is not called when reCAPTCHA validation fails.
// assert Assert.False(result.IsSuccess); Assert.Equal(expectedErrorMessage, result.Errors[0].Message); +_mockEmailService.Verify(s => s.SendEmailAsync(It.IsAny<MessageData>()), Times.Never);
82-102: Verify email service is not called on invalid token.Add verification to ensure the email service is not called when the reCAPTCHA token is invalid.
// assert Assert.False(result.IsSuccess); Assert.Equal(expectedErrorMessage, result.Errors[0].Message); +_mockEmailService.Verify(s => s.SendEmailAsync(It.IsAny<MessageData>()), Times.Never);
104-125: Verify operation order in email sending failure test.Add verification to ensure reCAPTCHA validation occurs before attempting to send the email.
// assert Assert.False(result.IsSuccess); Assert.Equal(expectedErrorMessage, result.Errors[0].Message); +var sequence = new MockSequence(); +_mockHttpClientFactory.InSequence(sequence).Verify(f => f.CreateClient(It.IsAny<string>())); +_mockEmailService.InSequence(sequence).Verify(s => s.SendEmailAsync(It.IsAny<MessageData>()));Streetcode/Streetcode.XUnitTest/Mocks/MockUserSharedResourceLocalizer.cs (1)
9-26: Consider organizing error messages into logical groups.The current implementation uses a single group (key: 0) for all error messages. Consider organizing them into logical categories for better maintainability:
return new Dictionary<int, List<string>> { { - 0, new List<string> + 1, new List<string> // Authentication errors { - "UserWithSuchUsernameNotExists", - "UserWithSuchUsernameExists", - "UserWithSuchEmailNotFound", - "UserWithSuchEmailExists", - "UserNotFound", - "UserNameOrTokenIsEmpty", - "UserManagerError", "IncorrectPassword", + "RecaptchaValidationFailed", + "RecaptchaScoreTooLow", + "GoogleTokenValidationFailed", + "GoogleAuthenticationFailed", } }, + { + 2, new List<string> // User existence errors + { + "UserWithSuchUsernameNotExists", + "UserWithSuchUsernameExists", + "UserWithSuchEmailNotExists", + "UserWithSuchEmailExists", + "UserNotFound", + } + }, + { + 3, new List<string> // Validation errors + { + "UserNameOrTokenIsEmpty", + "UserManagerError", + "EmailNotMatch", + } + }, };Streetcode/Streetcode.WebApi/Extensions/ServiceCollectionExtensions.cs (1)
70-71: Consider consolidating reCAPTCHA v3 with Google service registration.Since we're updating to reCAPTCHA v3 which uses behavior analysis, consider if the GoogleService could be utilized by the CaptchaService for token validation. This would promote code reuse and maintain a single source of truth for Google-related validations.
Would you like me to propose a design that integrates these services efficiently?
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Users/UserControllerTests.cs (6)
47-53: Consider using a builder pattern for test data setup.The test user extraction could be made more maintainable using a builder pattern, which would make the test setup more readable and flexible.
Consider refactoring to use a builder pattern:
public class TestUserBuilder { private string _userId = Guid.NewGuid().ToString(); private string _userName = Guid.NewGuid().ToString(); private string _password = GenerateTestPassword(); private string[] _roles = new[] { nameof(UserRole.User), nameof(UserRole.Admin) }; public TestUserBuilder WithUserId(string userId) { _userId = userId; return this; } public (User, string) Build() => UserExtracter.Extract(_userId, _userName, _password, _roles); }
55-65: Add edge cases for email validation.The email retrieval test only covers the happy path. Consider adding tests for edge cases like empty email, malformed email, and non-existent email.
Example test to add:
[Theory] [InlineData("")] [InlineData("invalid.email")] [InlineData("nonexistent@example.com")] public async Task GetByEmail_InvalidEmail_ReturnsBadRequest(string email) { // Act var response = await Client.GetByEmail(email, TokenStorage.UserAccessToken); // Assert Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); }
141-178: Enhance update tests with more validation scenarios.The update tests could be more comprehensive. Consider adding tests for:
- Invalid expertise data
- Empty or whitespace username
- Maximum length validation
Example test to add:
[Theory] [ExtractUpdateTestUser] [InlineData("")] [InlineData(" ")] [InlineData("a")] // Too short [InlineData("thisusernameiswaytoolongandshouldfail")] public async Task UpdateUser_InvalidUserName_ReturnsBadRequest(string userName) { // Arrange var userToUpdate = ExtractUpdateTestUserAttribute.UserForTest; userToUpdate.UserName = userName; // Act var response = await Client.Update(userToUpdate, TokenStorage.UserAccessToken); // Assert Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); }
180-208: Add tests for unauthorized deletion and cascade effects.The deletion tests should verify:
- Unauthorized users cannot delete accounts
- Related data is properly cleaned up after deletion
Example test to add:
[Fact] public async Task DeleteUser_UnauthorizedToken_ReturnsUnauthorized() { // Act var response = await Client.Delete(_testUser.Email!, "invalid_token"); // Assert Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); } [Fact] public async Task DeleteUser_VerifyCascadeDelete_Success() { // Arrange await TokenStorage.GenerateNewTokens(_testUser); // Act await Client.Delete(_testUser.Email!, TokenStorage.UserAccessToken); // Assert // Verify related data is deleted var expertises = await _expertiseRepository.GetByUserId(_testUser.Id); Assert.Empty(expertises); }
244-282: Add password complexity and token expiration tests.The password reset tests should verify:
- Password complexity requirements
- Token expiration handling
Example tests to add:
[Theory] [InlineData("short")] // Too short [InlineData("NoSpecialChar1")] // No special characters [InlineData("no-upper-1!")] // No uppercase [InlineData("NO-LOWER-1!")] // No lowercase public async Task UpdateForgotPassword_InvalidPasswordComplexity_ReturnsBadRequest(string password) { // Arrange var forgotPassword = new UpdateForgotPasswordDTO { Username = Uri.EscapeDataString(_testUser.UserName), Password = password, ConfirmPassword = password }; forgotPassword.Token = await _userManager.GeneratePasswordResetTokenAsync(_testUser); // Act var response = await Client.UpdateForgotPassword(forgotPassword); // Assert Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); }
284-288: Enhance password generation security.The current password generation method could be improved to ensure generated passwords meet all complexity requirements consistently.
Consider using a more robust password generator:
-private static string GenerateTestPassword() -{ - string guid = Guid.NewGuid().ToString(); - return $"TestPass123_{guid.Substring(0, 10)}"; -} +private static string GenerateTestPassword(int length = 16) +{ + const string upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; + const string lower = "abcdefghijklmnopqrstuvwxyz"; + const string number = "0123456789"; + const string special = "!@#$%^&*()_+-=[]{}|;:,.<>?"; + + var random = new Random(); + var result = new StringBuilder(); + + // Ensure at least one of each required char type + result.Append(upper[random.Next(upper.Length)]); + result.Append(lower[random.Next(lower.Length)]); + result.Append(number[random.Next(number.Length)]); + result.Append(special[random.Next(special.Length)]); + + // Fill remaining length with random chars + var chars = upper + lower + number + special; + while (result.Length < length) + { + result.Append(chars[random.Next(chars.Length)]); + } + + return new string(result.ToString().ToCharArray().OrderBy(x => random.Next()).ToArray()); +}Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/DeleteStatisticRecordTests.cs (2)
15-35: Add XML documentation and consider using constants for test data.The test class setup is well-structured. Consider these improvements for better maintainability:
- Add XML documentation to describe the test class purpose
- Consider extracting test data like QR IDs into constants
+/// <summary> +/// Unit tests for <see cref="DeleteStatisticRecordHandler"/> covering successful deletion, +/// record not found, and failed deletion scenarios. +/// </summary> public class DeleteStatisticRecordTests { + private const int ValidQrId = 1; + private const int InvalidQrId = -1; + private readonly Mock<IRepositoryWrapper> _repositoryWrapperMock;
37-64: Enhance test coverage by verifying logger calls.The test effectively verifies the happy path. Consider these enhancements:
- Verify that success is logged via
_loggerMock- Use more descriptive test data by adding meaningful properties to the
StatisticRecord[Fact] public async Task Handle_RecordIsDeletedSuccessfully_ShouldReturnSuccess() { // Arrange var qrId = 1; var statisticRecords = new List<StatisticRecord> { - new () { QrId = qrId }, + new () { + QrId = qrId, + CreatedAt = DateTime.UtcNow, + StreetcodeId = 42 + }, }; // ... existing test code ... // Assert Assert.True(result.IsSuccess); Assert.Equal(Unit.Value, result.Value); _repositoryWrapperMock.Verify(repo => repo.StatisticRecordRepository.Delete(It.IsAny<StatisticRecord>()), Times.Once); _repositoryWrapperMock.Verify(repo => repo.SaveChangesAsync(), Times.Once); + _loggerMock.Verify( + x => x.Log( + LogLevel.Information, + It.Is<string>(s => s.Contains($"Successfully deleted statistic record with QrId: {qrId}"))), + Times.Once); }Streetcode/Streetcode.XUnitTest/MediatRTests/Users/UpdateUserTest.cs (4)
45-45: Consider extracting test constants.Extract magic strings like email addresses into constants for better maintainability and reusability.
+ private const string TEST_EMAIL = "test@example.com"; - var fakeEmail = "test@example.com"; + var fakeEmail = TEST_EMAIL;
66-98: Enhance test coverage for successful update.The test verifies basic properties but could be more comprehensive. Consider:
- Verifying all updated fields (surname, avatarId, aboutYourself, phoneNumber).
- Adding descriptive messages to assertions for better test failure diagnostics.
Assert.True(result.IsSuccess); - Assert.Equal("newusername", result.Value.UserName); - Assert.Equal("New Name", result.Value.Name); + Assert.Equal("newusername", result.Value.UserName, "Username should be updated"); + Assert.Equal("New Name", result.Value.Name, "Name should be updated"); + Assert.Equal("New Surname", result.Value.Surname, "Surname should be updated"); + Assert.Equal(2, result.Value.AvatarId, "Avatar ID should be updated"); + Assert.Equal("About Myself", result.Value.AboutYourself, "About section should be updated"); + Assert.Equal("1234567890", result.Value.PhoneNumber, "Phone number should be updated"); Assert.Contains(result.Value.Expertises, e => e.Title == "New Expertise");
100-130: Consider extracting common test setup.The test contains duplicate setup code from the success test. Consider extracting common setup to a private method or using test fixtures.
+ private void SetupCommonMocks(User existingUser) + { + _userManagerMock.Setup(um => um.Users) + .Returns(new List<User> { existingUser }.AsQueryable().BuildMockDbSet().Object); + + _repositoryWrapperMock.Setup(rw => rw.ExpertiseRepository.GetAllAsync( + It.IsAny<Expression<Func<Expertise, bool>>>(), + It.IsAny<Func<IQueryable<Expertise>, IIncludableQueryable<Expertise, object>>>())) + .ReturnsAsync(new List<Expertise> { new () { Id = 2, Title = "New Expertise" } }); + + _mapperMock.Setup(m => m.Map<UserDTO>(It.IsAny<User>())) + .Returns(GetUserDto); + }Then use it in tests:
- _userManagerMock.Setup(um => um.Users) - .Returns(new List<User> { existingUser }.AsQueryable().BuildMockDbSet().Object); - - _repositoryWrapperMock.Setup(rw => rw.ExpertiseRepository.GetAllAsync( - It.IsAny<Expression<Func<Expertise, bool>>>(), - It.IsAny<Func<IQueryable<Expertise>, IIncludableQueryable<Expertise, object>>>())) - .ReturnsAsync(new List<Expertise> { new () { Id = 2, Title = "New Expertise" } }); - - _mapperMock.Setup(m => m.Map<UserDTO>(It.IsAny<User>())) - .Returns(GetUserDto); + SetupCommonMocks(existingUser);
132-156: Enhance user not found test assertions.The test could be more specific about the error condition. Consider:
- Verifying the specific error message.
- Simplifying the test setup since the user won't be found anyway.
// Assert - Assert.False(result.IsSuccess); + Assert.True(result.IsFailed); + Assert.Contains("User not found", result.Errors[0].Message);Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/AuthorizationFixture/TokenStorage.cs (2)
45-49: Add 'Async' suffix to async method name.Following C# naming conventions, async methods should have the 'Async' suffix.
-public async Task GenerateNewTokens(User user) +public async Task GenerateNewTokensAsync(User user) { UserAccessToken = (await _tokenService.GenerateAccessTokenAsync(user)).RawData; UserRefreshToken = _tokenService.SetNewRefreshTokenForUser(user); }
81-90: Add error handling for missing configuration.The
GetDbContextmethod should handle cases where the connection string is missing or invalid.private StreetcodeDbContext GetDbContext() { var sqlConnectionString = new ConfigurationBuilder() .AddJsonFile("appsettings.IntegrationTests.json") .Build() .GetConnectionString("DefaultConnection"); + + if (string.IsNullOrEmpty(sqlConnectionString)) + { + throw new InvalidOperationException("Connection string 'DefaultConnection' not found in test settings."); + } + var optionBuilder = new DbContextOptionsBuilder<StreetcodeDbContext>(); optionBuilder.UseSqlServer(sqlConnectionString); return new StreetcodeDbContext(optionBuilder.Options); }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Analytics/AnalyticsClient.cs (2)
7-12: Add XML documentation and input validation.Consider enhancing the class with:
- XML documentation to describe the class purpose and constructor parameters
- Input validation for the HttpClient parameter
+/// <summary> +/// Client for interacting with analytics endpoints in integration tests. +/// </summary> public class AnalyticsClient : BaseClient { + /// <summary> + /// Initializes a new instance of the AnalyticsClient class. + /// </summary> + /// <param name="client">The HTTP client to use for requests.</param> + /// <param name="secondPartUrl">Optional URL segment to append to the base URL.</param> + /// <exception cref="ArgumentNullException">Thrown when client is null.</exception> public AnalyticsClient(HttpClient client, string secondPartUrl = "") : base(client, secondPartUrl) { + ArgumentNullException.ThrowIfNull(client); }
1-48: Consider implementing retry logic for transient failures.Since this is an integration test client making HTTP requests, it would be beneficial to implement retry logic for handling transient failures.
Consider using a resilience pattern like Polly:
// Example implementation using Polly private readonly IAsyncPolicy<RestResponse> _retryPolicy = Policy<RestResponse> .Handle<HttpRequestException>() .Or<TimeoutException>() .WaitAndRetryAsync(3, retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))); // Then wrap your SendCommand/SendQuery calls: public async Task<RestResponse> GetAll(string authToken = "") { return await _retryPolicy.ExecuteAsync(async () => await SendQuery($"/GetAll", authToken)); }Streetcode/Streetcode.XIntegrationTest/Streetcode.XIntegrationTest.csproj (1)
61-70: Consider implementing tests for the new reCAPTCHA v3 integration.The new test folder structure is well-organized, but I notice there's no dedicated folder for reCAPTCHA-related tests. Given that this PR implements reCAPTCHA v3, consider adding test coverage for this feature.
Would you like me to help create a test structure for the reCAPTCHA v3 implementation?
Streetcode/Streetcode.XUnitTest/Validators/Email/EmailTests.cs (1)
10-125: Consider adding tests for reCAPTCHA v3 specific scenarios.Since we're transitioning to reCAPTCHA v3, consider adding tests for:
- Score-based validation
- Automatic token generation
- Token expiration handling
Would you like me to help generate test cases for these scenarios?
Streetcode/Streetcode.BLL/DTO/Authentication/GoogleLogin/GoogleLoginRequest.cs (1)
3-6: LGTM! Consider adding XML documentation.The class follows DTO pattern best practices with a good default value for IdToken. Consider adding XML documentation to describe the purpose and usage of this DTO.
public class GoogleLoginRequest { + /// <summary> + /// Gets or sets the Google ID token used for authentication. + /// </summary> public string IdToken { get; set; } = string.Empty; }Streetcode/Streetcode.BLL/Interfaces/Authentication/IGoogleService.cs (1)
7-7: Improve parameter name clarity.The parameter name 'id' could be more descriptive to indicate it's a token.
-Task<GoogleJsonWebSignature.Payload> ValidateGoogleToken(string id); +Task<GoogleJsonWebSignature.Payload> ValidateGoogleToken(string idToken);Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Users/Expertises/ExpertiseClient.cs (2)
6-11: Add XML documentation for the class and constructor.Consider adding XML documentation to improve code maintainability.
+/// <summary> +/// Client for interacting with expertise-related endpoints in integration tests. +/// </summary> public class ExpertiseClient : BaseClient { + /// <summary> + /// Initializes a new instance of the <see cref="ExpertiseClient"/> class. + /// </summary> + /// <param name="client">The HTTP client to use for requests.</param> + /// <param name="secondPartUrl">Optional URL segment to append to the base URL.</param> public ExpertiseClient(HttpClient client, string secondPartUrl = "") : base(client, secondPartUrl) { }
13-16: Add input validation and XML documentation for GetAll method.Consider adding validation for the authToken parameter and XML documentation.
+ /// <summary> + /// Retrieves all expertise entries. + /// </summary> + /// <param name="authToken">Optional authentication token.</param> + /// <returns>A response containing the expertise entries.</returns> public async Task<RestResponse> GetAll(string authToken = "") { + if (authToken != null && authToken.Length > 0 && !authToken.StartsWith("Bearer ", StringComparison.OrdinalIgnoreCase)) + { + authToken = $"Bearer {authToken}"; + } return await SendQuery("/GetAll", authToken); }Streetcode/Streetcode.XIntegrationTest/ControllerTests/BaseController/BaseAuthorizationControllerTests.cs (1)
14-14: Consider keeping TokenStorage immutable.Making TokenStorage mutable could lead to test state issues if modified during test execution. Consider keeping it read-only and initializing through constructor injection.
-protected TokenStorage TokenStorage { get; set; } +protected TokenStorage TokenStorage { get; }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Expertises/ExpertiseExtracter.cs (1)
7-14: Simplify Extract method and improve variable naming.The Extract method can be simplified, and variable names can be more descriptive.
public static Expertise Extract(int expertiseId) { - var testExpertise = TestDataProvider.GetTestData<Expertise>(); - - testExpertise.Id = expertiseId; - var extracter = BaseExtracter.Extract<Expertise>(testExpertise, e => e.Id == expertiseId); - return extracter; + var expertise = TestDataProvider.GetTestData<Expertise>(); + expertise.Id = expertiseId; + return BaseExtracter.Extract<Expertise>(expertise, e => e.Id == expertiseId); }Streetcode/Streetcode.BLL/MediatR/Authentication/Logout/LogoutHandler.cs (2)
26-28: Consider using ID-based lookup for better performanceLooking up users by username is less efficient than using the primary key (ID). Consider retrieving the user ID from claims:
-var userUserName = HttpContextHelper.GetCurrentUserName(_httpContextAccessor); -var user = await _repositoryWrapper.UserRepository.GetFirstOrDefaultAsync(u => u.UserName == userUserName); +var userId = HttpContextHelper.GetCurrentUserId(_httpContextAccessor); +var user = await _repositoryWrapper.UserRepository.GetByIdAsync(userId);
37-39: Add transaction handling for database operationsThe update operation should be wrapped in a transaction to ensure data consistency:
+await using var transaction = await _repositoryWrapper.BeginTransactionAsync(); _repositoryWrapper.UserRepository.Update(user); -var resultIsSuccess = await _repositoryWrapper.SaveChangesAsync() > 0; +try +{ + var resultIsSuccess = await _repositoryWrapper.SaveChangesAsync() > 0; + if (resultIsSuccess) + { + await transaction.CommitAsync(); + return Result.Ok(); + } + await transaction.RollbackAsync(); + return Result.Fail("Failed to logout"); +} +catch (Exception ex) +{ + await transaction.RollbackAsync(); + return Result.Fail(new Error($"Failed to logout: {ex.Message}")); +}Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Authentication/AuthenticationClient.cs (1)
32-35: Consider adding response validation for the Logout method.The method implementation is clean, but consider adding response validation to handle potential edge cases.
public async Task<RestResponse> Logout(string authToken = "") { - return await SendCommand("/Logout", Method.Post, authToken: authToken); + var response = await SendCommand("/Logout", Method.Post, authToken: authToken); + if (!response.IsSuccessful) + { + // Handle unsuccessful logout + } + return response; }Streetcode/Streetcode.XUnitTest/Factories/MessageDataFactory/MessageDataConcreteFactoryTests.cs (2)
29-43: Enhance test coverage for CreateFeedbackMessageDataWhile the test verifies message creation, it should also validate the message properties to ensure correct data mapping.
[Fact] public void CreateFeedbackMessageData_ShouldReturnValidMessage() { // Arrange string from = "user@example.com"; string source = "test_source"; string content = "test_content"; // Act var message = _factory.CreateFeedbackMessageData(from, source, content); // Assert Assert.NotNull(message); Assert.IsType<FeedbackMessageData>(message); + Assert.Equal(from, message.From); + Assert.Equal(source, message.Source); + Assert.Equal(content, message.Content); + Assert.Equal(_emailConfig.To, message.To); }
45-60: Enhance test coverage for CreateForgotPasswordMessageDataSimilar to the feedback message test, this test should validate the message properties.
[Fact] public void CreateForgotPasswordMessageData_ShouldReturnValidMessage() { // Arrange string[] to = { "user@example.com" }; string token = "reset-token"; string username = "TestUser"; string currentDomain = "https://example.com"; // Act var message = _factory.CreateForgotPasswordMessageData(to, token, username, currentDomain); // Assert Assert.NotNull(message); Assert.IsType<ForgotPasswordMessageData>(message); + Assert.Equal(to, message.To); + Assert.Equal(_emailConfig.From, message.From); + Assert.Contains(token, message.Content); + Assert.Contains(username, message.Content); + Assert.Contains(currentDomain, message.Content); }Streetcode/Streetcode.BLL/Services/Instagram/InstagramService.cs (1)
15-15: Make post limit configurableThe post limit is hardcoded and used in multiple places. Consider making it configurable through InstagramEnvirovmentVariables.
-private static int postLimit = 10; +private readonly int _postLimit; public InstagramService(IOptions<InstagramEnvirovmentVariables> instagramEnvirovment, IHttpClientFactory httpClientFactory) { _httpClientFactory = httpClientFactory; _envirovment = instagramEnvirovment.Value; _userId = _envirovment.InstagramID; _accessToken = _envirovment.InstagramToken; + _postLimit = _envirovment.PostLimit; }Also applies to: 27-27
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/BeforeAndAfterTestAtribute/Users/ExtractUpdateTestUserAttribute.cs (2)
44-44: Use a consistent test phone number format.Consider using a more generic phone number format for testing or extracting it to a constants file.
- PhoneNumber = "+380735004490", + PhoneNumber = "+1234567890", // Generic test phone number
60-64: Enhance password generation security.The current password generation could be more robust by ensuring it meets common password requirements.
private string GenerateTestPassword() { - string guid = Guid.NewGuid().ToString(); - return $"TestPass123_{guid.Substring(0, 10)}"; + const string special = "!@#$%^&*"; + var guid = Guid.NewGuid().ToString(); + return $"TestPass{special[new Random().Next(special.Length)]}{guid.Substring(0, 10)}"; }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Base/BaseClient.cs (1)
34-42: Consider enhancing request validationThe null check for
requestDtois good, but consider adding validation forrequestStringto prevent potential null reference exceptions.protected async Task<RestResponse> SendCommand<T>(string requestString, Method method, T? requestDto = default, string authToken = "") where T : class? { + if (string.IsNullOrEmpty(requestString)) + { + throw new ArgumentException("Request string cannot be null or empty", nameof(requestString)); + } var request = new RestRequest($"{SecondPartUrl}{requestString}", method); if (requestDto is not null) { request.AddJsonBody(requestDto); }Streetcode/Streetcode.WebApi/Controllers/Users/UsersController.cs (1)
18-39: Document API changes for username to email migrationThe transition from username to email-based identification is good, but consider adding XML documentation to clarify the purpose of each endpoint.
+ /// <summary> + /// Retrieves the current user's information using their email from the authentication token + /// </summary> [HttpGet] [Authorize] [ProducesResponseType(StatusCodes.Status200OK)] public async Task<IActionResult> GetByEmail() { return HandleResult(await Mediator.Send(new GetByEmailQuery())); } + /// <summary> + /// Checks if a user exists with the specified username + /// </summary> [HttpGet("{userName}")] [Authorize] [ProducesResponseType(StatusCodes.Status200OK)] public async Task<IActionResult> ExistWithUserName([FromRoute] string userName) { return HandleResult(await Mediator.Send(new ExistWithUserNameQuery(userName))); }Streetcode/Streetcode.XUnitTest/MediatRTests/Users/GetOtherUserByUserNameHandlerTests.cs (3)
33-34: Consider simplifying UserManager mock setup using a helper method.The UserManager mock setup could be extracted into a reusable helper method to improve maintainability and reduce duplication across test classes.
+// In MockHelpers.cs +public static Mock<UserManager<User>> CreateMockUserManager() +{ + return new Mock<UserManager<User>>( + new Mock<IUserStore<User>>().Object, + null, null, null, null, null, null, null, null); +} -var userStoreMock = new Mock<IUserStore<User>>(); -_userManagerMock = new Mock<UserManager<User>>(userStoreMock.Object, null, null, null, null, null, null, null, null); +_userManagerMock = MockHelpers.CreateMockUserManager();
44-69: Enhance test coverage with additional scenarios.The test covers the happy path well, but consider adding test cases for:
- Users with multiple roles
- Users with special characters in username
- Case sensitivity handling
71-90: Enhance failure scenario assertions.The test should also verify:
- The localized error message content
- The error details in the result object
// Assert Assert.False(result.IsSuccess); +Assert.Equal("User does not exist", result.Error); +Assert.NotNull(result.ErrorDetails);Streetcode/Streetcode.XUnitTest/Mocks/MockHelpers.cs (2)
70-70: Fix method name typo.The method name contains a typo: "FirstOfDefault" should be "FirstOrDefault" to match the repository method name.
-public static void SetupMockUserRepositoryGetFirstOfDefaultAsync +public static void SetupMockUserRepositoryGetFirstOrDefaultAsync
70-76: Enhance mock setup flexibility.Consider accepting a User object parameter instead of just email to support more test scenarios.
-public static void SetupMockUserRepositoryGetFirstOfDefaultAsync(Mock<IRepositoryWrapper> mockRepositoryWrapper, string email) +public static void SetupMockUserRepositoryGetFirstOrDefaultAsync(Mock<IRepositoryWrapper> mockRepositoryWrapper, User user) { mockRepositoryWrapper.Setup(x => x.UserRepository.GetFirstOrDefaultAsync( It.IsAny<Expression<Func<User, bool>>>(), It.IsAny<Func<IQueryable<User>, IIncludableQueryable<User, object>>>())) - .ReturnsAsync(new User { Email = email }); + .ReturnsAsync(user); }Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/CustomWebApplicationFactory.cs (2)
31-51: Improve mock setup flexibility and maintainability.Consider extracting the hardcoded invalid token string to a constant and making the error message configurable.
+private const string InvalidGoogleToken = "invalid_google_id_token"; -public void SetupMockGoogleLogin(User user, string? token = null) +public void SetupMockGoogleLogin(User user, string? token = null, string? errorMessage = null) { if (token is null) { GoogleServiceMock - .Setup(gs => gs.ValidateGoogleToken("invalid_google_id_token")) - .ThrowsAsync(new InvalidJwtException("Invalid Google Token")); + .Setup(gs => gs.ValidateGoogleToken(InvalidGoogleToken)) + .ThrowsAsync(new InvalidJwtException(errorMessage ?? "Invalid Google Token")); }
89-107: Make HTTP client mock setup more configurable.Consider parameterizing the response status code and content to support different test scenarios.
-private void SetupMockHttpClient() +private void SetupMockHttpClient( + HttpStatusCode statusCode = HttpStatusCode.OK, + string content = "{\"success\": true}") { var mockResponse = new HttpResponseMessage { - StatusCode = HttpStatusCode.OK, - Content = new StringContent("{\"success\": true}"), + StatusCode = statusCode, + Content = new StringContent(content), };Streetcode/Streetcode.XUnitTest/MediatRTests/Authentication/Logout/LogoutHandlerTests.cs (1)
85-105: Consider adding timeout test case.While the current error cases are well covered, consider adding a test for timeout scenarios using the CancellationToken.
+[Fact] +public async Task Handle_Timeout_ShouldReturnFail() +{ + // Arrange + var request = new LogoutCommand(); + var cts = new CancellationTokenSource(); + cts.Cancel(); + + // Act + var result = await _handler.Handle(request, cts.Token); + + // Assert + Assert.True(result.IsFailed); + Assert.Contains("Operation cancelled", result.Errors[0].Message); +}Streetcode/Streetcode.XUnitTest/MediatRTests/Users/DeleteUserHandlerTests.cs (1)
93-110: Consider adding role-based test cases.The email mismatch test is good, but consider adding tests for role-based authorization scenarios.
+[Fact] +public async Task Handle_UserNotAuthorized_ReturnsFailure() +{ + // Arrange + var cancellationToken = CancellationToken.None; + var request = new DeleteUserCommand("user@example.com"); + var user = new User { Email = "user@example.com" }; + + _userManagerMock.Setup(um => um.FindByEmailAsync("user@example.com")) + .ReturnsAsync(user); + _userManagerMock.Setup(um => um.IsInRoleAsync(user, "Admin")) + .ReturnsAsync(false); + + // Act + var result = await _handler.Handle(request, cancellationToken); + + // Assert + Assert.False(result.IsSuccess); + Assert.Contains("Unauthorized", result.Errors[0].Message); +}Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/UpdateCountStatisticRecordTests.cs (1)
117-125: Consider parameterized test data.The test data helper method could be enhanced to support parameterized test cases.
-private StatisticRecord GetStatisticRecord(int qrId) +private StatisticRecord GetStatisticRecord(int qrId, int initialCount = 5) { return new StatisticRecord { Id = 1, QrId = qrId, - Count = 5, + Count = initialCount, }; }Streetcode/Streetcode.XUnitTest/MediatRTests/Users/UpdateForgotPasswordHandlerTests.cs (2)
44-45: Consider using a more secure password for testing.The test password "NewSecurePassword123!" is relatively simple. Consider using a stronger password that meets all your application's password requirements.
- Password = "NewSecurePassword123!", + Password = "P@ssw0rd_2024_Str0ng!#$%^&*()",
93-94: Enhance test coverage for weak password scenarios.The test could be more comprehensive by checking multiple weak password scenarios (too short, no special characters, etc.).
- Password = "weak", + Password = "a", // Test extremely short password + // Add more test cases in separate test methods: + // - Password without numbers + // - Password without special characters + // - Password without uppercase lettersStreetcode/Streetcode.XIntegrationTest/ControllerTests/Streetcode/StreetcodeCreateControllerTests.cs (1)
30-32: Improve documentation for skipped tests.The skip message could be more descriptive about why images are required and how to fix the test.
- [Fact(Skip = "There are no images in the streetcode, so the test will fail without them.")] + [Fact(Skip = "Test requires streetcode images to be present in the test database. " + + "To fix: Add test images using TestImageSeeder or mock the image requirement.")]Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetStatisticRecordByQrIdTests.cs (2)
42-71: Consider adding edge cases to test suite.The test coverage could be enhanced by adding test cases for:
- Negative QR IDs
- Maximum possible QR ID values
- Null or malformed repository responses
127-136: Consider extracting test data to constants.The test data like QR IDs and user data could be extracted to constants or a test data factory for better maintainability and reuse across test methods.
+private const int ValidQrId = 1; +private const int InvalidQrId = -1; +private const int MaxQrId = int.MaxValue;Streetcode/Streetcode.XUnitTest/MediatRTests/Users/ForgotPasswordTests.cs (1)
74-96: Consider adding email content validation test.Add a test to verify the content of the email being sent, including:
- Subject line
- Email body format
- Reset link structure
Example test:
[Fact] public async Task Handle_ShouldSendEmailWithCorrectContent() { // Arrange var user = new User { UserName = "TestUser", Email = "test@example.com" }; var request = new ForgotPasswordCommand(new ForgotPasswordDTO { Email = "test@example.com" }); MessageData capturedMessage = null; SetupUserManagerMock(user); _emailServiceMock.Setup(es => es.SendEmailAsync(It.IsAny<MessageData>())) .Callback<MessageData>(msg => capturedMessage = msg) .ReturnsAsync(true); // Act var result = await _handler.Handle(request, CancellationToken.None); // Assert Assert.NotNull(capturedMessage); Assert.Contains("reset", capturedMessage.Subject.ToLower()); Assert.Contains("reset", capturedMessage.Body.ToLower()); }Streetcode/Streetcode.XUnitTest/Validators/Users/BaseUserValidatorTests.cs (1)
76-93: Consider using Theory for validation format tests.The username format validation tests could be combined into a single parameterized test for better maintainability.
-[Fact] -public void Validate_UserNameIsInvalid_ShouldReturnError() +[Theory] +[InlineData("Нікнейм1", "Invalid characters")] +[InlineData("NAME", "All uppercase")] +[InlineData("!name", "Special characters")] +[InlineData("a", "Too short")] +[InlineData("verylongusername123456", "Too long")] +public void Validate_UserNameIsInvalid_ShouldReturnError(string userName, string testCase)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Authentication/AuthControllerTests.cs (2)
204-268: Add tests for reCAPTCHA v3 integration.Since the PR objectives mention updating to reCAPTCHA v3, add test cases to verify:
- Automatic verification without checkbox
- Score-based bot detection
- Token validation
Would you like me to generate test cases for reCAPTCHA v3 integration?
227-244: Enhance Google login error test coverage.Consider adding test cases for:
- Expired tokens
- Malformed tokens
- Network failures during token validation
Example test:
[Fact] public async Task LoginGoogle_ExpiredToken_ReturnsUnauthorized() { // Arrange var expiredToken = "expired_google_token"; var request = new GoogleLoginRequest { IdToken = expiredToken }; _factory.GoogleServiceMock .Setup(s => s.ValidateGoogleToken(expiredToken)) .ThrowsAsync(new SecurityTokenExpiredException()); // Act var response = await Client.GoogleLogin(request); // Assert Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); Assert.Contains("token expired", response.Content.ToLower()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Streetcode/Streetcode.XIntegrationTest/TestData/Expertise.jsonis excluded by!**/Streetcode/Streetcode.XIntegrationTest/TestData/**Streetcode/Streetcode.XIntegrationTest/TestData/StatisticRecord.jsonis excluded by!**/Streetcode/Streetcode.XIntegrationTest/TestData/**build/Utils/AuthConstants.csis excluded by!**/build/**
📒 Files selected for processing (94)
Streetcode/Streetcode.BLL/DTO/Authentication/GoogleLogin/GoogleLoginRequest.cs(1 hunks)Streetcode/Streetcode.BLL/Interfaces/Authentication/IGoogleService.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Create/CreateStatisticRecordCommand.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Create/CreateStatisticRecordHandler.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Delete/DeleteStatisticRecordHandler.cs(2 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/ExistByQrId/ExistStatisticRecordByQrIdHandler.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/GetAll/GetAllStatisticRecordsQuery.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/GetAllByStreetcodeId/GetAllStatisticRecordsByStreetcodeIdHandler.cs(2 hunks)Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/UpdateCount/UpdateCountStatisticRecordHandler.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Authentication/LoginGoogle/LoginGoogleHandler.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Authentication/Logout/LogoutCommand.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Authentication/Logout/LogoutHandler.cs(2 hunks)Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Instagram/GetAll/GetAllPostsHandler.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Instagram/GetAll/GetAllPostsQuery.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Users/ExistWithUserName/ExistWithUserNameHandler.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/ExistWithUserName/ExistWithUserNameQuery.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/ForgotPassword/ForgotPasswordHandler.cs(3 hunks)Streetcode/Streetcode.BLL/MediatR/Users/GetByEmail/GetByEmailHandler.cs(4 hunks)Streetcode/Streetcode.BLL/MediatR/Users/GetByEmail/GetByEmailQuery.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/GetByName/GetByUserNameQuery.cs(0 hunks)Streetcode/Streetcode.BLL/MediatR/Users/GetOtherUserByUserName/GetOtherUserByUserNameHandler.cs(2 hunks)Streetcode/Streetcode.BLL/MediatR/Users/GetOtherUserByUserName/GetOtherUserByUserNameQuery.cs(1 hunks)Streetcode/Streetcode.BLL/MediatR/Users/Update/UpdateUserHandler.cs(3 hunks)Streetcode/Streetcode.BLL/MediatR/Users/UpdateForgotPassword/UpdateForgotPasswordHandler.cs(2 hunks)Streetcode/Streetcode.BLL/Services/Authentication/GoogleService.cs(1 hunks)Streetcode/Streetcode.BLL/Services/Instagram/InstagramService.cs(2 hunks)Streetcode/Streetcode.BLL/Streetcode.BLL.csproj(2 hunks)Streetcode/Streetcode.BLL/Util/Helpers/HttpContextHelper.cs(1 hunks)Streetcode/Streetcode.BLL/Util/Helpers/UserHelper.cs(2 hunks)Streetcode/Streetcode.BLL/Validators/Analytics/StatisticRecordValidator.cs(0 hunks)Streetcode/Streetcode.BLL/Validators/Users/BaseUserValidator.cs(2 hunks)Streetcode/Streetcode.WebApi/Controllers/Analytics/StatisticRecordController.cs(0 hunks)Streetcode/Streetcode.WebApi/Controllers/Authentication/AuthController.cs(3 hunks)Streetcode/Streetcode.WebApi/Controllers/Instagram/InstagramController.cs(0 hunks)Streetcode/Streetcode.WebApi/Controllers/Users/UsersController.cs(2 hunks)Streetcode/Streetcode.WebApi/Extensions/ServiceCollectionExtensions.cs(1 hunks)Streetcode/Streetcode.WebApi/appsettings.IntegrationTests.json(1 hunks)Streetcode/Streetcode.WebApi/appsettings.Local.json(1 hunks)Streetcode/Streetcode.XIntegrationTest/Constants/ControllerTests/AuthConstants.cs(2 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/AdditionalContent/TagControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/AdditionalContent/TeamPositionsControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Analytics/StatisticRecordControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Authentication/AuthControllerTests.cs(4 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/BaseController/BaseAuthorizationControllerTests.cs(2 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Email/EmailControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Jobs/Create/JobCreateControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/News/NewsGetAllControllerTests.cs(0 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Partners/PartnersControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Streetcode/StreetcodeCreateControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Streetcode/StreetcodeUpdateControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Timeline/HistoricalContextControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Users/Expertises/ExpertiseControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Users/UserControllerTests.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/AuthorizationFixture/AuthorizationCollection.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/AuthorizationFixture/TokenStorage.cs(3 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/BeforeAndAfterTestAtribute/Streetcode/ExtractCreateTestStreetcodeAttribute.cs(0 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/BeforeAndAfterTestAtribute/Users/ExtractUpdateTestUserAttribute.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Analytics/AnalyticsClient.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Authentication/AuthenticationClient.cs(2 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Base/BaseClient.cs(3 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Email/EmailClient.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Users/Expertises/ExpertiseClient.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Users/UserClient.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/CustomWebApplicationFactory.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Analytics/StatisticRecordExtracter.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Authentication/UserExtracter.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Expertises/ExpertiseExtracter.cs(1 hunks)Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/SqlDbHelper.cs(2 hunks)Streetcode/Streetcode.XIntegrationTest/Streetcode.XIntegrationTest.csproj(2 hunks)Streetcode/Streetcode.XUnitTest/Factories/MessageDataFactory/MessageDataConcreteFactoryTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/DeleteStatisticRecordTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/ExistStatisticRecordByQrIdTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetAllStatisticRecordsByStreetcodeIdTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetAllStatisticRecordsTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/GetStatisticRecordByQrIdTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/UpdateCountStatisticRecordTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Authentication/Logout/LogoutHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Email/SendEmailHandlerTests.cs(6 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/DeleteUserHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/ExistWithUserNameHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/ForgotPasswordTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/GetByUserNameTest.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/GetOtherUserByUserNameHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/UpdateForgotPasswordHandlerTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Users/UpdateUserTest.cs(1 hunks)Streetcode/Streetcode.XUnitTest/Mocks/MockFailedToValidateLocalizer.cs(3 hunks)Streetcode/Streetcode.XUnitTest/Mocks/MockFieldNamesLocalizer.cs(1 hunks)Streetcode/Streetcode.XUnitTest/Mocks/MockHelpers.cs(3 hunks)Streetcode/Streetcode.XUnitTest/Mocks/MockUserSharedResourceLocalizer.cs(1 hunks)Streetcode/Streetcode.XUnitTest/Streetcode.XUnitTest.csproj(2 hunks)Streetcode/Streetcode.XUnitTest/Validators/Email/EmailTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/Validators/Users/BaseUserValidatorTests.cs(1 hunks)Streetcode/Streetcode.XUnitTest/Validators/Users/UpdateUserValidatorTests.cs(1 hunks)
💤 Files with no reviewable changes (11)
- Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs
- Streetcode/Streetcode.BLL/MediatR/Instagram/GetAll/GetAllPostsQuery.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/BeforeAndAfterTestAtribute/Streetcode/ExtractCreateTestStreetcodeAttribute.cs
- Streetcode/Streetcode.BLL/MediatR/Instagram/GetAll/GetAllPostsHandler.cs
- Streetcode/Streetcode.WebApi/Controllers/Analytics/StatisticRecordController.cs
- Streetcode/Streetcode.WebApi/Controllers/Instagram/InstagramController.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/News/NewsGetAllControllerTests.cs
- Streetcode/Streetcode.BLL/Validators/Analytics/StatisticRecordValidator.cs
- Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Create/CreateStatisticRecordCommand.cs
- Streetcode/Streetcode.BLL/MediatR/Users/GetByName/GetByUserNameQuery.cs
- Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Create/CreateStatisticRecordHandler.cs
✅ Files skipped from review due to trivial changes (11)
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/AuthorizationFixture/AuthorizationCollection.cs
- Streetcode/Streetcode.BLL/MediatR/Users/ExistWithUserName/ExistWithUserNameHandler.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/Streetcode/StreetcodeUpdateControllerTests.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/Partners/PartnersControllerTests.cs
- Streetcode/Streetcode.WebApi/appsettings.Local.json
- Streetcode/Streetcode.BLL/MediatR/Users/GetOtherUserByUserName/GetOtherUserByUserNameQuery.cs
- Streetcode/Streetcode.XUnitTest/Mocks/MockFieldNamesLocalizer.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/AdditionalContent/TeamPositionsControllerTests.cs
- Streetcode/Streetcode.BLL/MediatR/Users/ExistWithUserName/ExistWithUserNameQuery.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/AdditionalContent/TagControllerTests.cs
- Streetcode/Streetcode.XIntegrationTest/ControllerTests/Timeline/HistoricalContextControllerTests.cs
🔇 Additional comments (40)
Streetcode/Streetcode.XUnitTest/Streetcode.XUnitTest.csproj (2)
34-34: Good addition of MockQueryable.Moq package.The addition of MockQueryable.Moq 7.0.3 aligns well with the project's .NET 7.0 target framework and will enhance the testing capabilities for IQueryable scenarios, particularly useful for the new authentication and user management features.
54-57: Well-structured test organization.The new folder structure effectively supports the PR's focus on authentication improvements:
MessageDataFactoryfor test data generationAuthentication/Logoutfor logout functionality testsUsersfor user management testsValidators/Usersfor user validation testsThis organization promotes better test maintainability and clear separation of concerns.
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Jobs/Create/JobCreateControllerTests.cs (2)
7-7: LGTM! Good enhancement to authorization testing.The addition of AuthorizationFixture import aligns well with the broader improvements to authorization testing across the codebase. This change helps maintain consistency in how authorization is handled in tests.
28-98: Excellent test coverage and structure!The test suite demonstrates thorough coverage of job creation scenarios:
- Success case with proper authorization
- Unauthorized access handling
- Role-based access control (admin vs non-admin)
- Input validation
- Response verification
The consistent use of the Arrange-Act-Assert pattern and meaningful test names makes the test intentions clear and maintainable.
Streetcode/Streetcode.BLL/Streetcode.BLL.csproj (2)
11-15: LGTM! Clean removal of Instagram-related items.The removal of Instagram-related items is clean and comprehensive, covering all associated compile, embedded resource, and none items.
51-51: LGTM! New folder structure for Google authentication.The addition of the GoogleLogin folder aligns well with the implementation of Google sign-in components.
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/Delete/DeleteStatisticRecordHandler.cs (2)
43-43: LGTM! Good async/await practice.The switch to
SaveChangesAsync()follows best practices for database operations, improving scalability by preventing thread blocking during I/O operations.
1-58: Verify the scope of changes against PR objectives.The changes in this file (async refactoring and mapper removal) don't appear to be directly related to the PR's stated objective of updating reCAPTCHA from v2 to v3. Could you clarify if this is part of a broader refactoring effort?
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Extracter/Analytics/StatisticRecordExtracter.cs (1)
4-7: Well-structured test utility class!The class follows good practices with a clear namespace organization and appropriate use of the static modifier for utility methods.
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Analytics/StatisticRecordControllerTests.cs (1)
184-192: LGTM! Well-structured cleanup.The Dispose method properly handles cleanup of test resources and follows the dispose pattern correctly.
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/ExistByQrId/ExistStatisticRecordByQrIdHandler.cs (2)
13-16: LGTM! Constructor simplified appropriately.Good job removing the unused
IMapperdependency. The constructor now only includes the necessaryIRepositoryWrapperdependency, following the YAGNI principle.
18-24: LGTM! Clean and efficient implementation.The
Handlemethod is well-implemented with:
- Proper async/await usage
- Clear null check using pattern matching
- Appropriate use of Result type for error handling
Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/ExistStatisticRecordByQrIdTests.cs (2)
13-21: LGTM! Well-structured test setup.Good job setting up the test class with:
- Clean mock initialization
- Proper dependency injection in constructor
- Reusable mock repository for all tests
23-50: LGTM! Comprehensive positive test case.The test effectively verifies the success scenario with:
- Clear AAA pattern
- Appropriate mock setup
- Thorough assertions
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/GetAll/GetAllStatisticRecordsQuery.cs (1)
7-8: LGTM! Clean implementation of MediatR query.The record correctly implements IRequest with a Result wrapper, following best practices for error handling.
Streetcode/Streetcode.XUnitTest/MediatRTests/Email/SendEmailHandlerTests.cs (2)
20-39: Update tests to align with reCAPTCHA v3 implementation.The test class is currently designed for reCAPTCHA v2, but the PR objectives mention updating to v3. The tests should be updated to reflect v3's automatic verification approach, which may require different mock setups and validation logic.
Could you confirm if these tests need to be updated for reCAPTCHA v3? I can help suggest the necessary changes based on the v3 implementation details.
127-183: Helper methods are well-structured and focused!The helper methods follow good practices:
- Single responsibility principle
- Clear naming
- Focused setup methods
Streetcode/Streetcode.BLL/MediatR/Authentication/Logout/LogoutCommand.cs (1)
6-6: LGTM! Clean and focused command implementation.The simplified parameterless command aligns well with CQRS principles, allowing the handler to manage user identification through IHttpContextAccessor.
Let's verify that all consumers have been updated to use the new parameterless constructor:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining LogoutCommand instantiations that might still try to pass parameters rg -l "new LogoutCommand\(" | xargs rg "new LogoutCommand\([^)]+\)"Length of output: 69
Consumer usage of LogoutCommand verified – no parameterized instantiations found.
The search script did not return any instances where LogoutCommand is instantiated with parameters, which indicates that all consumers have been updated to the new parameterless constructor. This confirms that the change aligns with the intended authentication refactoring. Please continue to monitor new changes to ensure consistency.
Streetcode/Streetcode.WebApi/Extensions/ServiceCollectionExtensions.cs (1)
71-71: Add Google.Apis.Auth NuGet package for GoogleJsonWebSignature.The GoogleService registration looks good, but ensure the required Google authentication package is added to the project.
Run this script to check if the package is properly referenced:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if Google.Apis.Auth package is referenced in the project rg -l "Google.Apis.Auth" --type csprojLength of output: 69
I'll generate a new verification script to search all *.csproj files for a reference to "Google.Apis.Auth" since the previous command using the csproj file type was not recognized. Please run the following script:
Once executed, check if any csproj file lists the "Google.Apis.Auth" package. If no references are found, then the package needs to be added to the project dependencies to support GoogleJsonWebSignature as used in the GoogleService integration.
Please confirm the results after running this script so that we can ensure that the project's dependencies are complete.
🏁 Script executed:
#!/bin/bash # Search for the Google.Apis.Auth package reference in all .csproj files find . -type f -name "*.csproj" -exec grep -H "Google.Apis.Auth" {} \;Length of output: 192
Google Service Dependency is Satisfied
The new scoped registration for
IGoogleServiceis well-placed, and our verification shows that theGoogle.Apis.Authpackage is already referenced in the solution (specifically inStreetcode.BLL/Streetcode.BLL.csproj). Please ensure that the project usingGoogleService(i.e.,Streetcode.WebApi) either directly includes the package or accesses it transitively via a reference toStreetcode.BLL.Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/DeleteStatisticRecordTests.cs (1)
91-121: Well-implemented failure scenario test!The test effectively covers the failed deletion scenario with appropriate verifications of error messages and repository calls.
Streetcode/Streetcode.BLL/MediatR/Users/UpdateForgotPassword/UpdateForgotPasswordHandler.cs (1)
17-25: LGTM! Constructor refactoring improves maintainability.The simplified constructor with focused dependencies follows SOLID principles and improves maintainability.
Streetcode/Streetcode.BLL/MediatR/Users/ForgotPassword/ForgotPasswordHandler.cs (2)
26-42: Well-structured constructor with clear dependency injection!The constructor follows best practices with clear dependency injection and proper initialization of all fields.
59-66: Good security practice with URL encoding!Properly encoding both the token and username prevents potential URL injection vulnerabilities and ensures special characters are handled correctly.
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/AuthorizationFixture/TokenStorage.cs (1)
51-68: Well-implemented dispose pattern!The dispose pattern implementation follows best practices with proper null checks, disposed flag, and finalization suppression.
Streetcode/Streetcode.XIntegrationTest/Streetcode.XIntegrationTest.csproj (1)
33-33: Verify RestSharp version upgrade compatibility.The upgrade from preview version 109.0.0-preview.1 to stable version 112.1.0 is significant. Please ensure compatibility with existing code.
❌ Verification inconclusive
🌐 Web query:
What are the breaking changes in RestSharp 112.1.0 compared to version 109?💡 Result:
RestSharp 112.1.0 introduces several breaking changes compared to version 109, primarily focused on thread safety, API simplification, and modernized dependencies. Here are the key differences:
Core architectural changes
- HTTP client modernization: Complete replacement of legacy
HttpWebRequestwithHttpClient(started in v107)[1][7]- Immutable client configuration:
RestClientOptionsbecame read-only after instantiation to ensure thread safety[4][7]- Interface reduction: The
IRestClientinterface was significantly simplified across versions (deprecated in v107 then modified again in v110/112)[2][5]Breaking API changes
Feature Version 109 Version 112.1.0 Timeout configuration MaxTimeoutpropertyRenamed to TimeoutwithTimeSpantype[3]Cookie handling Per-request cookies Added client-level cookie container[2] Serialization Mixed JSON implementations Full transition to System.Text.Json[1][7]Authentication NtlmAuthenticatorclassReplaced with CredentialsinRestClientOptions[1]Request configuration Mutable client properties Locked behind RestClientOptionsconstructor[4][7]Specific breaking changes
Interface signature changes
IRestClientbecame narrower with reduced surface area[2][5]- Custom implementations require adaptation for new method signatures[5]
Parameter handling
AddParameter("application/json", ...)pattern deprecated in favor ofAddBody[1]- Header management automated (manual
Content-Type/Acceptheaders discouraged)[1]Security enhancements
- CRLF sequences in headers now prohibited (v112)[5]
- Cookie parsing became stricter about RFC compliance[4]
Thread safety improvements
DefaultParameterscollection uses locking for thread-safe mutations[2]- All client configuration must be set during initialization via
RestClientOptions[4][7]Authentication changes
NtlmAuthenticatorremoved in favor of handler-level credential configuration[1]- OAuth1 signature handling improved for special characters[5]
Migration requirements
- Client instantiation must use
RestClientOptionsinstead of property setters:// Old (v109) var client = new RestClient(); client.Timeout = 1000; // New (v112+) var options = new RestClientOptions { Timeout = TimeSpan.FromSeconds(1) }; var client = new RestClient(options);- File uploads require using
Streamparameters instead of byte arrays[1]- Custom serializers must implement updated interfaces from
System.Text.Json[1][7]These changes address long-standing issues with connection pooling, async handling, and HTTP/2 support while modernizing RestSharp's architecture[1][3][5]. Developers should audit their use of client configuration, parameter handling, and authentication when upgrading.[1][2][4][7]
Citations:
- 1: https://restsharp.dev/migration/
- 2: https://restsharp.dev/docs/v110/changelog/
- 3: https://restsharp.dev/docs/changelog/
- 4: restsharp/RestSharp#1976
- 5: https://github.com/restsharp/RestSharp/releases
- 6: restsharp/RestSharp#2259
- 7: https://www.nuget.org/packages/restsharp/
- 8: https://developer.genesys.cloud/forum/t/problem-with-restsharp-purecloudplatform-client-v2/28968
- 9: https://www.nuget.org/packages/RestSharp.Serializers.NewtonsoftJson/
- 10: https://restsharp.dev/docs/advanced/authenticators/
Action Required: Confirm Integration Updates for RestSharp Breaking Changes
The update from RestSharp 109 (preview) to 112.1.0 introduces several breaking changes—such as new client instantiation via immutable RestClientOptions, revised timeout and parameter handling, and interface signature updates—that may require corresponding adjustments in our integration test code. Please review integration test methods where RestSharp is used and ensure that any instantiations, configuration, or parameter usage align with the new API conventions.
Streetcode/Streetcode.XUnitTest/Validators/Email/EmailTests.cs (3)
18-20: LGTM! Clean initialization of dependencies.The removal of the
thiskeyword simplifies the code while maintaining clear initialization of mocks and validator.
24-24: LGTM! Improved test method naming.The new test method names follow a clear pattern (
Validate_Condition_ExpectedResult) that makes the test purpose immediately obvious.Also applies to: 37-37, 53-53, 69-69, 85-85, 101-101
85-98: Consider updating token validation for reCAPTCHA v3.Since the PR objectives mention transitioning from reCAPTCHA v2 to v3, this test might need updates to validate the new token format and handling.
Let's verify if there are any other reCAPTCHA-related tests that might need updates:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for reCAPTCHA-related tests rg -l "captcha|recaptcha|token" --type cs -g "*test*.cs"Length of output: 58
Below is a new comprehensive search script to cover potential case sensitivity issues. Please run it to verify if any additional reCAPTCHA‑ or token‑related tests exist:
Next steps for you:
- If additional tests are found: Review them to ensure they accommodate the new reCAPTCHA v3 behavior.
- If no other tests appear: Confirm that the only test affected is in
Streetcode/Streetcode.XUnitTest/Validators/Email/EmailTests.cs, and then decide if the reCAPTCHA v3 update requires modifying the existing test or adding new ones.Please verify the output of the above search so we can determine the next course of action.
🏁 Script executed:
[run_scripts] #!/bin/bash # Perform a comprehensive, case-insensitive search across all C# files for the terms "captcha", "recaptcha", or "token" rg -l "(?i)(captcha|recaptcha|token)" --type csLength of output: 34671
Below is an additional shell script to search the repository for any references to "recaptcha v3" (or similar) to see if updated token validation has been implemented elsewhere:
Next steps for you:
- If references are found: Investigate how the token is expected to behave under reCAPTCHA v3, and update the test and/or validators accordingly.
- If no references are found: Confirm that this test remains the only point for reCAPTCHA-related validation and decide if additional test cases (e.g. new token format checks) are needed for the updated reCAPTCHA v3 behavior.
Please verify the output of the above search so we can decide on the proper adjustment strategy.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for references to "recaptcha v3" rg -i "recaptcha v3"Length of output: 144
Action Required: Confirm ReCAPTCHA Version Implementation
Based on our searches:
- We did not find any explicit references or modifications indicating reCAPTCHA v3–specific handling in the repository.
- The current test in
Streetcode/Streetcode.XUnitTest/Validators/Email/EmailTests.csstill verifies that an empty token produces the expected error "CannotBeEmpty" for the captcha token.Recommendations:
If the update to reCAPTCHA v3 is already implemented elsewhere:
The test should be updated to validate the new expected token format or behavior (e.g., checking token verification rules specific to reCAPTCHA v3).If reCAPTCHA v3 changes have not yet been introduced:
The current test remains valid until the reCAPTCHA service is fully transitioned to v3. Consider adding new tests that specifically cover the valid token format or behavior expected from reCAPTCHA v3 once that implementation is available.Please confirm with the team or compare with the updated requirements to decide whether to revise this test or supplement it with additional cases.
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/Client/Email/EmailClient.cs (1)
9-12: LGTM! Constructor properly initializes the base class.The constructor correctly passes the HttpClient and optional URL to the base class.
Streetcode/Streetcode.BLL/Util/Helpers/UserHelper.cs (1)
10-10: Verify null handling for Email property.The null-forgiving operator (!) assumes Email won't be null, but this should be explicitly validated.
-var cleanEmail = RemoveNonAlphaNumericFromEmail(user.Email!); +if (string.IsNullOrEmpty(user.Email)) +{ + throw new ArgumentException("Email cannot be null or empty", nameof(user)); +} +var cleanEmail = RemoveNonAlphaNumericFromEmail(user.Email);Streetcode/Streetcode.XUnitTest/Mocks/MockFailedToValidateLocalizer.cs (1)
21-23: LGTM! Good addition of validation messagesThe new validation messages for username format, name format, surname format, expertise limit, and length constraints are well-organized within their respective groups.
Also applies to: 40-40, 51-51
Streetcode/Streetcode.BLL/Services/Instagram/InstagramService.cs (1)
11-12: LGTM! Good use of IHttpClientFactoryThe switch from direct HttpClient instantiation to IHttpClientFactory is a best practice that helps manage connection pooling and lifetime.
Also applies to: 17-20
Streetcode/Streetcode.BLL/MediatR/Analytics/StatisticRecord/UpdateCount/UpdateCountStatisticRecordHandler.cs (1)
47-47: LGTM! Good async implementation.The transition to async/await pattern is well-implemented, maintaining consistency with other handlers.
Streetcode/Streetcode.XIntegrationTest/Constants/ControllerTests/AuthConstants.cs (1)
36-39: LGTM! Well-structured normalized fields.The addition of normalized email and username fields follows Identity best practices for case-insensitive comparisons.
Also applies to: 49-52
Streetcode/Streetcode.BLL/MediatR/Users/Update/UpdateUserHandler.cs (3)
27-33: LGTM! Improved constructor readability.The constructor parameters have been nicely formatted with one parameter per line, enhancing code readability.
50-50: LGTM! Added cancellation support.The addition of cancellationToken to SingleOrDefaultAsync improves the handling of async operation cancellations.
81-81: Verify null safety of FirstOrDefault call.The null-forgiving operator (!) suggests you're certain GetRolesAsync will always return a non-null collection with at least one role. Please verify this assumption.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for role assignments to verify if users always have at least one role ast-grep --pattern 'await _userManager.AddToRoleAsync($user, $_)'Length of output: 178
I'll run an updated search that excludes JSON files (which might be tripping up our initial script) to look for any instance of role assignment via
AddToRoleAsyncin the repository. This should give us a better picture of whether users are always assigned a role.
🏁 Script executed:
#!/bin/bash rg -n "await _userManager.AddToRoleAsync" -g '!*.json'Length of output: 388
Action: Confirm that all user creation paths securely assign a role or add a safeguard for missing roles.
Our search shows that role assignment (via AddToRoleAsync) is applied in both the registration and login handlers—suggesting that new users should receive a role. However, this evidence alone doesn’t guarantee that every user, including legacy or exceptional cases, always has at least one role. It might be beneficial to either add explicit null checking/fallback logic or enforce a constraint that prevents any user from existing without a role.
Streetcode/Streetcode.XUnitTest/MediatRTests/Authentication/Logout/LogoutHandlerTests.cs (1)
45-62: LGTM! Well-structured test for successful logout.The test properly verifies the success path with good assertions and mocking setup.
Streetcode/Streetcode.XUnitTest/MediatRTests/Users/DeleteUserHandlerTests.cs (1)
24-51: LGTM! Well-structured test setup.The test constructor properly initializes all required mocks and dependencies.
Streetcode/Streetcode.XUnitTest/MediatRTests/Analytics/UpdateCountStatisticRecordTests.cs (1)
34-60: LGTM! Comprehensive success scenario test.The test properly verifies count increment and repository interactions.
| [Fact] | ||
| public async Task GetAll_Expertises_ReturnsSuccessStatusCode() | ||
| { | ||
| // Act | ||
| var response = await Client.GetAll(); | ||
| var returnedValue = CaseIsensitiveJsonDeserializer.Deserialize<List<ExpertiseDTO>>(response.Content); | ||
|
|
||
| // Assert | ||
| Assert.True(response.IsSuccessStatusCode); | ||
| Assert.NotNull(returnedValue); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
While the basic success case is covered, consider adding:
- Edge cases (empty list, large result set)
- Response content validation
- Error scenarios
Example additions:
[Fact]
public async Task GetAll_WhenNoExpertises_ReturnsEmptyList()
{
// Arrange: Ensure no expertises exist
// Act
var response = await Client.GetAll();
var returnedValue = CaseIsensitiveJsonDeserializer.Deserialize<List<ExpertiseDTO>>(response.Content);
// Assert
Assert.True(response.IsSuccessStatusCode);
Assert.Empty(returnedValue);
}
[Fact]
public async Task GetAll_ValidatesResponseContent()
{
// Arrange: Add test expertise
await Client.Create(_testExpertise);
// Act
var response = await Client.GetAll();
var returnedValue = CaseIsensitiveJsonDeserializer.Deserialize<List<ExpertiseDTO>>(response.Content);
// Assert
Assert.True(response.IsSuccessStatusCode);
Assert.Contains(returnedValue, e => e.Id == _testExpertise.Id);
// Add more specific assertions for expertise properties
}| [Fact] | ||
| public async Task Validate_WhenCalled_ShouldCallBaseValidator() | ||
| { | ||
| // Arrange | ||
| var createValidator = new UpdateUserValidator(mockBaseValidator.Object, _mockRepositoryWrapper.Object, mockUserSharedResourceLocalizer); | ||
| var user = GetValidUser(); | ||
| var createCommand = new UpdateUserCommand(user); | ||
| MockHelpers.SetupMockUserRepositoryGetFirstOfDefaultAsync(_mockRepositoryWrapper, user.UserName); | ||
|
|
||
| // Act | ||
| await createValidator.TestValidateAsync(createCommand); | ||
|
|
||
| // Assert | ||
| mockBaseValidator.Verify(x => x.ValidateAsync(It.IsAny<ValidationContext<UpdateUserDTO>>(), CancellationToken.None), Times.Once); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage with additional test cases.
While the current test verifies the base validator call, consider adding these test cases:
- Validation failure scenarios
- Duplicate username validation
- Invalid email format validation
- Empty required fields validation
Here's an example of an additional test case:
[Fact]
public async Task Validate_WithDuplicateUsername_ShouldReturnValidationError()
{
// Arrange
var createValidator = new UpdateUserValidator(mockBaseValidator.Object, _mockRepositoryWrapper.Object, mockUserSharedResourceLocalizer);
var user = GetValidUser();
var createCommand = new UpdateUserCommand(user);
// Setup mock to simulate existing user
MockHelpers.SetupMockUserRepositoryGetFirstOfDefaultAsync(_mockRepositoryWrapper, user.UserName, returnExistingUser: true);
// Act
var result = await createValidator.TestValidateAsync(createCommand);
// Assert
result.ShouldHaveValidationErrorFor(x => x.User.UserName);
}| { | ||
| 0, new List<string> | ||
| { | ||
| "UserWithSuchUsernameNotExists", | ||
| "UserWithSuchUsernameExists", | ||
| "UserWithSuchEmailNotFound", | ||
| "UserWithSuchEmailExists", | ||
| "UserNotFound", | ||
| "UserNameOrTokenIsEmpty", | ||
| "UserManagerError", | ||
| "IncorrectPassword", | ||
| "EmailNotMatch", | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding reCAPTCHA and Google authentication error messages.
Given the PR's focus on implementing reCAPTCHA v3 and Google authentication, consider adding relevant error messages to handle these scenarios:
{
0, new List<string>
{
"UserWithSuchUsernameNotExists",
"UserWithSuchUsernameExists",
"UserWithSuchEmailNotFound",
"UserWithSuchEmailExists",
"UserNotFound",
"UserNameOrTokenIsEmpty",
"UserManagerError",
"IncorrectPassword",
"EmailNotMatch",
+ "RecaptchaValidationFailed",
+ "RecaptchaScoreTooLow",
+ "GoogleTokenValidationFailed",
+ "GoogleAuthenticationFailed",
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| 0, new List<string> | |
| { | |
| "UserWithSuchUsernameNotExists", | |
| "UserWithSuchUsernameExists", | |
| "UserWithSuchEmailNotFound", | |
| "UserWithSuchEmailExists", | |
| "UserNotFound", | |
| "UserNameOrTokenIsEmpty", | |
| "UserManagerError", | |
| "IncorrectPassword", | |
| "EmailNotMatch", | |
| } | |
| }, | |
| { | |
| 0, new List<string> | |
| { | |
| "UserWithSuchUsernameNotExists", | |
| "UserWithSuchUsernameExists", | |
| "UserWithSuchEmailNotFound", | |
| "UserWithSuchEmailExists", | |
| "UserNotFound", | |
| "UserNameOrTokenIsEmpty", | |
| "UserManagerError", | |
| "IncorrectPassword", | |
| "EmailNotMatch", | |
| "RecaptchaValidationFailed", | |
| "RecaptchaScoreTooLow", | |
| "GoogleTokenValidationFailed", | |
| "GoogleAuthenticationFailed", | |
| } | |
| }, |
| [Fact] | ||
| public async Task Handle_RecordIsNotFound_ShouldReturnError() | ||
| { | ||
| // Arrange | ||
| var qrId = -1; | ||
|
|
||
| _repositoryWrapperMock.Setup(repo => repo.StatisticRecordRepository.GetFirstOrDefaultAsync( | ||
| It.IsAny<Expression<Func<StatisticRecord, bool>>>(), | ||
| It.IsAny<Func<IQueryable<StatisticRecord>, IIncludableQueryable<StatisticRecord, object>>>())) | ||
| .ReturnsAsync((StatisticRecord)null!); | ||
|
|
||
| _localizerCannotFindMock.Setup(loc => loc["CannotFindRecordWithQrId"]).Returns(new LocalizedString("CannotFindRecordWithQrId", "Record not found")); | ||
|
|
||
| var request = new DeleteStatisticRecordCommand(qrId); | ||
|
|
||
| // Act | ||
| var result = await _handler.Handle(request, CancellationToken.None); | ||
|
|
||
| // Assert | ||
| Assert.False(result.IsSuccess); | ||
| Assert.Contains("Record not found", result.Errors[0].Message); | ||
| _repositoryWrapperMock.Verify(repo => repo.StatisticRecordRepository.Delete(It.IsAny<StatisticRecord>()), Times.Never); | ||
| _repositoryWrapperMock.Verify(repo => repo.SaveChanges(), Times.Never); | ||
| } |
There was a problem hiding this comment.
Fix inconsistent async method verification.
There's an inconsistency in the repository mock verification. The test verifies SaveChanges() but the handler uses SaveChangesAsync().
// Assert
Assert.False(result.IsSuccess);
Assert.Contains("Record not found", result.Errors[0].Message);
_repositoryWrapperMock.Verify(repo => repo.StatisticRecordRepository.Delete(It.IsAny<StatisticRecord>()), Times.Never);
- _repositoryWrapperMock.Verify(repo => repo.SaveChanges(), Times.Never);
+ _repositoryWrapperMock.Verify(repo => repo.SaveChangesAsync(), Times.Never);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Fact] | |
| public async Task Handle_RecordIsNotFound_ShouldReturnError() | |
| { | |
| // Arrange | |
| var qrId = -1; | |
| _repositoryWrapperMock.Setup(repo => repo.StatisticRecordRepository.GetFirstOrDefaultAsync( | |
| It.IsAny<Expression<Func<StatisticRecord, bool>>>(), | |
| It.IsAny<Func<IQueryable<StatisticRecord>, IIncludableQueryable<StatisticRecord, object>>>())) | |
| .ReturnsAsync((StatisticRecord)null!); | |
| _localizerCannotFindMock.Setup(loc => loc["CannotFindRecordWithQrId"]).Returns(new LocalizedString("CannotFindRecordWithQrId", "Record not found")); | |
| var request = new DeleteStatisticRecordCommand(qrId); | |
| // Act | |
| var result = await _handler.Handle(request, CancellationToken.None); | |
| // Assert | |
| Assert.False(result.IsSuccess); | |
| Assert.Contains("Record not found", result.Errors[0].Message); | |
| _repositoryWrapperMock.Verify(repo => repo.StatisticRecordRepository.Delete(It.IsAny<StatisticRecord>()), Times.Never); | |
| _repositoryWrapperMock.Verify(repo => repo.SaveChanges(), Times.Never); | |
| } | |
| [Fact] | |
| public async Task Handle_RecordIsNotFound_ShouldReturnError() | |
| { | |
| // Arrange | |
| var qrId = -1; | |
| _repositoryWrapperMock.Setup(repo => repo.StatisticRecordRepository.GetFirstOrDefaultAsync( | |
| It.IsAny<Expression<Func<StatisticRecord, bool>>>(), | |
| It.IsAny<Func<IQueryable<StatisticRecord>, IIncludableQueryable<StatisticRecord, object>>>())) | |
| .ReturnsAsync((StatisticRecord)null!); | |
| _localizerCannotFindMock.Setup(loc => loc["CannotFindRecordWithQrId"]).Returns(new LocalizedString("CannotFindRecordWithQrId", "Record not found")); | |
| var request = new DeleteStatisticRecordCommand(qrId); | |
| // Act | |
| var result = await _handler.Handle(request, CancellationToken.None); | |
| // Assert | |
| Assert.False(result.IsSuccess); | |
| Assert.Contains("Record not found", result.Errors[0].Message); | |
| _repositoryWrapperMock.Verify(repo => repo.StatisticRecordRepository.Delete(It.IsAny<StatisticRecord>()), Times.Never); | |
| _repositoryWrapperMock.Verify(repo => repo.SaveChangesAsync(), Times.Never); | |
| } |
| _userManagerMock.Setup(um => um.Users) | ||
| .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object); | ||
|
|
||
| _userManagerMock.Setup(um => um.Users) | ||
| .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object); | ||
|
|
There was a problem hiding this comment.
Remove duplicate setup code.
The UserManager.Users setup is duplicated. Remove the redundant setup to improve test maintainability.
_userManagerMock.Setup(um => um.Users)
.Returns(new List<User>().AsQueryable().BuildMockDbSet().Object);
-
- _userManagerMock.Setup(um => um.Users)
- .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _userManagerMock.Setup(um => um.Users) | |
| .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object); | |
| _userManagerMock.Setup(um => um.Users) | |
| .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object); | |
| _userManagerMock.Setup(um => um.Users) | |
| .Returns(new List<User>().AsQueryable().BuildMockDbSet().Object); |
| private async Task<RestResponse> SendRequest(RestRequest request, string authToken) | ||
| { | ||
| request.OnBeforeDeserialization = resp => { resp.ContentType = "application/json"; }; | ||
| request.AddHeader("Authorization", $"Bearer {authToken}"); | ||
| request.AddHeader("Content-Type", "application/json"); | ||
|
|
||
| var response = await this.client.ExecuteAsync(request); | ||
| var response = await client.ExecuteAsync(request); | ||
| return response; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement logging for better error tracking
The comment "Add logging" should be addressed to improve error tracking and debugging capabilities.
Consider implementing structured logging:
private async Task<RestResponse> SendRequest(RestRequest request, string authToken)
{
request.OnBeforeDeserialization = resp => { resp.ContentType = "application/json"; };
request.AddHeader("Authorization", $"Bearer {authToken}");
request.AddHeader("Content-Type", "application/json");
+ try {
var response = await client.ExecuteAsync(request);
+ if (!response.IsSuccessful)
+ {
+ _logger.LogWarning("Request failed: {StatusCode} {ErrorMessage}",
+ response.StatusCode, response.ErrorMessage);
+ }
return response;
+ }
+ catch (Exception ex)
+ {
+ _logger.LogError(ex, "Request execution failed");
+ throw;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async Task<RestResponse> SendRequest(RestRequest request, string authToken) | |
| { | |
| request.OnBeforeDeserialization = resp => { resp.ContentType = "application/json"; }; | |
| request.AddHeader("Authorization", $"Bearer {authToken}"); | |
| request.AddHeader("Content-Type", "application/json"); | |
| var response = await this.client.ExecuteAsync(request); | |
| var response = await client.ExecuteAsync(request); | |
| return response; | |
| } | |
| private async Task<RestResponse> SendRequest(RestRequest request, string authToken) | |
| { | |
| request.OnBeforeDeserialization = resp => { resp.ContentType = "application/json"; }; | |
| request.AddHeader("Authorization", $"Bearer {authToken}"); | |
| request.AddHeader("Content-Type", "application/json"); | |
| try { | |
| var response = await client.ExecuteAsync(request); | |
| if (!response.IsSuccessful) | |
| { | |
| _logger.LogWarning("Request failed: {StatusCode} {ErrorMessage}", | |
| response.StatusCode, response.ErrorMessage); | |
| } | |
| return response; | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Request execution failed"); | |
| throw; | |
| } | |
| } |
| RuleFor(dto => dto.UserName) | ||
| .Matches(@"^[a-z0-9'-_]+$").WithMessage(localizer["NameFormat", fieldLocalizer["UserName"], MinLengthSurname]) | ||
| .Matches(@"^[a-z0-9'\-_]+$").WithMessage(localizer["UserNameFormat"]) | ||
| .NotEmpty().WithMessage(localizer["CannotBeEmpty", fieldLocalizer["UserName"]]) | ||
| .MinimumLength(MinLengthName).WithMessage(localizer["MinLength", fieldLocalizer["UserName"], MinLengthUserName]) | ||
| .MaximumLength(MaxLengthName).WithMessage(localizer["MaxLength", fieldLocalizer["UserName"], MaxLengthUserName]); | ||
|
|
||
| RuleFor(dto => dto.Name) | ||
| .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["NameFormat", fieldLocalizer["Name"], MinLengthSurname]) | ||
| .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["NameFormat"]) | ||
| .NotEmpty().WithMessage(localizer["CannotBeEmpty", fieldLocalizer["Name"]]) | ||
| .MinimumLength(MinLengthName).WithMessage(localizer["MinLength", fieldLocalizer["Name"], MinLengthName]) | ||
| .MaximumLength(MaxLengthName).WithMessage(localizer["MaxLength", fieldLocalizer["Name"], MaxLengthName]); | ||
|
|
||
| RuleFor(dto => dto.Surname) | ||
| .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["SurnameFormat", fieldLocalizer["Surname"], MinLengthSurname]) | ||
| .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["SurnameFormat"]) | ||
| .NotEmpty().WithMessage(localizer["CannotBeEmpty", fieldLocalizer["Surname"]]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Enhance validation patterns for better security and usability
The current regex patterns could be improved to better handle international characters and prevent potential security issues.
- .Matches(@"^[a-z0-9'\-_]+$").WithMessage(localizer["UserNameFormat"])
+ .Matches(@"^[\w\-']{2,128}$").WithMessage(localizer["UserNameFormat"])
- .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["NameFormat"])
+ .Matches(@"^[\p{L}'-]+$").WithMessage(localizer["NameFormat"])
- .Matches(@"^[a-zA-Zа-яА-ЯґҐєЄіІїЇ'-]+$").WithMessage(localizer["SurnameFormat"])
+ .Matches(@"^[\p{L}'-]+$").WithMessage(localizer["SurnameFormat"])The updated patterns:
- Use
\wfor username to include all word characters - Use
\p{L}to support all Unicode letters for names - Add length constraints directly in the pattern
🌐 Web query:
What are the best practices for name validation regex patterns in 2024?
💡 Result:
When creating name validation regex patterns in 2024, best practices focus on balancing accuracy with cultural inclusivity while optimizing performance and security. Here are key recommendations:
Core Principles
1. Cultural Sensitivity
- Allow spaces, hyphens/apostrophes (e.g., O'Connor), and multi-part names (e.g., María del Carmen)
- Avoid assuming Western naming conventions
- Use
\p{L}instead of[A-Za-z]to support accented characters:^[\p{L} \-']+(?: [\p{L} \-']+)+$
2. Security & Performance
- Prevent ReDoS attacks by avoiding:
- Nested quantifiers (
(a+)+) - Complex lookaheads/lookbehinds
- Unbounded repetitions
- Nested quantifiers (
- Test edge cases like extremely long names (>50 chars)
Common Patterns
| Use Case | Regex Pattern | Example | Notes |
|---|---|---|---|
| Basic Western names | ^[A-Z][a-z]+(?: [A-Z][a-z]*)+$ |
John Doe | Requires title case |
| International names | ^\p{L}+(?:[ \-']\p{L}+)*$ |
François O'Conner | Unicode support |
| Minimum two-part names | ^\p{L}+(?: \p{L}+){1,}$ |
Li Wei | Blocks single-word entries |
Implementation Tips
Form Validation
- Combine client-side (HTML5) and server-side checks:
<input pattern="^\p{L}+(?: \p{L}+)+$" title="Full name required">
- Use case-insensitive matching where appropriate (
/iflag)
Error Handling
- Provide clear examples:
"Valid format: Firstname Lastname (special characters allowed: - ' )" - Avoid exposing regex specifics in error messages
Testing & Optimization
- Benchmark patterns with tools like Regex101
- Validate against diverse datasets including:
- Multi-generational names (e.g., José María García Lorca)
- Hyphenated surnames (Smith-Johnson)
- Shortened forms (Xiaoling → Xiao Ling)
Alternatives to Consider
For complex validation needs:
// Use dedicated libraries like:
import {isValidName} from 'human-name-validator';These practices ensure your name validation remains robust yet flexible enough to handle global naming conventions while maintaining system performance and security[1][3][5][9].
Citations:
- 1: https://confluence.atlassian.com/proforma/regex-validation-1087521274.html
- 2: https://confluence.atlassian.com/jirakb/using-forms-regex-validation-1255454685.html
- 3: https://arjancodes.com/blog/regex-performance-optimization-and-security-best-practices/
- 4: https://www.servicenow.com/community/developer-articles/common-regular-expressions-and-cheat-sheet/ta-p/2297106
- 5: https://gist.github.com/chrislachance/287e3ebc530bc8ae4513a2e5e994e781
- 6: https://forum.bubble.io/t/15-must-know-regex-patterns-for-bubble-workflows/353476
- 7: https://learn.microsoft.com/ro-ro/dotnet/standard/base-types/best-practices-regex
- 8: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/pattern
- 9: https://openssf.org/blog/2024/06/18/know-your-regular-expressions-securing-input-validation-across-languages/
- 10: https://forum.wixstudio.com/t/simple-regex-for-pattern-validation/11545
Regex Patterns: Consider Allowing Spaces for International Names
While the updated patterns leverage Unicode support with \p{L}, best practices in 2024 recommend permitting spaces in names to accommodate multi-part entries (e.g., "María del Carmen" or "Jean Claude"). You might consider updating the Name and Surname rules from:
- .Matches(@"^[\p{L}'-]+$").WithMessage(localizer["NameFormat"])
+ .Matches(@"^\p{L}+(?:[ \-']\p{L}+)*$").WithMessage(localizer["NameFormat"])Key Points:
- Internationalization: The revised pattern
^\p{L}+(?:[ \-']\p{L}+)*$allows spaces, hyphens, and apostrophes while ensuring that names with multiple parts are accepted. - Consistency: Ensure the regex changes complement the separate minimum/maximum length validations instead of duplicating length constraints in the regex.
- Username Validation: The suggested update for usernames (
^[\w\-']{2,128}$) appears reasonable if the intent is to include word characters; however, verify this aligns with your broader naming requirements.
Adopting these refinements will enhance cultural inclusivity and align the validation logic with contemporary security and usability recommendations.
[found_issues]
| [Fact] | ||
| public async Task ShouldReturnPaginatedNews_CorrectPage() | ||
| { |
There was a problem hiding this comment.
Correct test method name.
The method name ShouldReturnPaginatedNews_CorrectPage is incorrect for a user retrieval test.
-public async Task ShouldReturnPaginatedNews_CorrectPage()
+public async Task Handle_UserExists_ReturnsUserDTO()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Fact] | |
| public async Task ShouldReturnPaginatedNews_CorrectPage() | |
| { | |
| [Fact] | |
| public async Task Handle_UserExists_ReturnsUserDTO() | |
| { |
|
|
||
| namespace Streetcode.XUnitTest.MediatRTests.Users; | ||
|
|
||
| public class GetByUserNameTest |
There was a problem hiding this comment.
Update class name to match functionality.
The class name GetByUserNameTest doesn't match the functionality being tested (GetByEmail). Consider renaming for clarity.
-public class GetByUserNameTest
+public class GetByEmailHandlerTests📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class GetByUserNameTest | |
| public class GetByEmailHandlerTests |
| string tableSchema = this.dbContext.Model.FindEntityType(typeof(T))?.GetSchema() !; | ||
| string tableName = this.dbContext.Model.FindEntityType(typeof(T))?.GetTableName() !; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace null-forgiving operators with proper null checks.
Using the null-forgiving operator (!) is risky. Consider adding proper null checks and error handling.
- string tableSchema = this.dbContext.Model.FindEntityType(typeof(T))?.GetSchema() !;
- string tableName = this.dbContext.Model.FindEntityType(typeof(T))?.GetTableName() !;
+ var entityType = this.dbContext.Model.FindEntityType(typeof(T));
+ if (entityType == null)
+ {
+ throw new InvalidOperationException($"Entity type {typeof(T).Name} not found in the model");
+ }
+ string tableSchema = entityType.GetSchema() ?? throw new InvalidOperationException($"Schema not found for entity {typeof(T).Name}");
+ string tableName = entityType.GetTableName() ?? throw new InvalidOperationException($"Table name not found for entity {typeof(T).Name}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string tableSchema = this.dbContext.Model.FindEntityType(typeof(T))?.GetSchema() !; | |
| string tableName = this.dbContext.Model.FindEntityType(typeof(T))?.GetTableName() !; | |
| var entityType = this.dbContext.Model.FindEntityType(typeof(T)); | |
| if (entityType == null) | |
| { | |
| throw new InvalidOperationException($"Entity type {typeof(T).Name} not found in the model"); | |
| } | |
| string tableSchema = entityType.GetSchema() ?? throw new InvalidOperationException($"Schema not found for entity {typeof(T).Name}"); | |
| string tableName = entityType.GetTableName() ?? throw new InvalidOperationException($"Table name not found for entity {typeof(T).Name}"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs (1)
46-49: 💡 Verification agent🧩 Analysis chain
Verify email content sanitization.
Ensure that the
CreateFeedbackMessageDatamethod properly sanitizes user input to prevent potential XSS or email injection attacks.
🏁 Script executed:
#!/bin/bash # Search for input sanitization in the message factory implementation ast-grep --pattern 'class $_ implements IMessageDataAbstractFactory { $$$ CreateFeedbackMessageData($_) { $$$ } }'Length of output: 126
I'll need to gather additional information to confirm whether user inputs are properly sanitized inside the
CreateFeedbackMessageDatamethod. The initial ast-grep search didn’t return any matches for sanitization code, which means we haven’t yet located any explicit sanitization logic (such as HTML encoding or similar measures) in the factory method implementation.Let's run a more general search to locate the definition of
CreateFeedbackMessageDataand inspect its implementation context:Once you review the output, please verify manually whether the method implementation includes necessary sanitation (e.g., using encoding libraries or sanitization functions) to prevent XSS or injection attacks. If no sanitization is present here, consider adding explicit sanitization to handle user-supplied input before using it.
Please let me know the output of this command to further determine if a code change is required.
🏁 Script executed:
#!/bin/bash # Locate the implementation of CreateFeedbackMessageData and display context to see if sanitization is done. rg -n -A 15 "CreateFeedbackMessageData\s*\(" .Length of output: 6434
Input Sanitization Missing in CreateFeedbackMessageData Implementation
The implementation in
MessageDataConcreteFactory.cspasses inputs (i.e.,from,source, andcontent) directly to the newFeedbackMessageDatawithout any explicit sanitization. This exposes the code to potential XSS or email injection issues if the incoming data contains malicious content.
- Location:
Streetcode/Streetcode.BLL/Factories/MessageDataFactory/Concretes/MessageDataConcreteFactory.cs, lines 19-28- Recommendation: Introduce input sanitization (e.g., using encoding or validation methods) either at the point of input handling in
SendEmailHandler.cs, within the factory method, or via a separate validation/sanitization layer before passing data to the email service.
🧹 Nitpick comments (1)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs (1)
38-44: Consider enhancing error handling with more specific error types.While the error handling is functional, consider creating dedicated error types for different reCAPTCHA validation failures to enable more granular error handling by consumers.
- return Result.Fail(new Error(captchaErrorMessage)); + return Result.Fail(new ReCaptchaValidationError(captchaErrorMessage));Additionally, consider adding debug logging before the validation attempt:
+ _logger.LogDebug("Validating reCAPTCHA token for email from {sender}", request.Email.From); var reCaptchaValidationResult = await _captchaService.ValidateReCaptchaAsync(request.Email.Token, cancellationToken);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs(2 hunks)Streetcode/Streetcode.XUnitTest/MediatRTests/Email/SendEmailHandlerTests.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Streetcode/Streetcode.XUnitTest/MediatRTests/Email/SendEmailHandlerTests.cs
🔇 Additional comments (1)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs (1)
8-8: LGTM! Good separation of concerns.The introduction of
ICaptchaServiceand removal of direct HTTP client dependencies improves modularity and aligns with the upgrade to reCAPTCHA v3.Also applies to: 20-20, 26-27, 33-33
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs (1)
39-44: Consider handling multiple validation errors.Currently, only the first error message is used. Consider aggregating all validation errors to provide more comprehensive feedback.
Here's a suggested improvement:
- if (reCaptchaValidationResult.IsFailed) - { - string captchaErrorMessage = reCaptchaValidationResult.Errors[0].Message; - _logger.LogError(request, captchaErrorMessage); - return Result.Fail(new Error(captchaErrorMessage)); - } + if (reCaptchaValidationResult.IsFailed) + { + var captchaErrorMessages = string.Join("; ", reCaptchaValidationResult.Errors.Select(e => e.Message)); + _logger.LogError(request, captchaErrorMessages); + return Result.Fail(new Error(captchaErrorMessages)); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs(2 hunks)
🔇 Additional comments (2)
Streetcode/Streetcode.BLL/MediatR/Email/SendEmailHandler.cs (2)
8-8: Well-structured dependency management!Good job extracting reCAPTCHA validation into a dedicated service. This improves separation of concerns and makes the code more maintainable.
Also applies to: 20-20, 22-34
38-38: Clean reCAPTCHA validation implementation!The implementation aligns perfectly with the PR objective to update reCAPTCHA. Good job propagating the cancellation token for proper async operation handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/CustomWebApplicationFactory.cs (3)
37-57: Consider enhancing Google login test coverage.While the current implementation handles basic success and failure cases, consider adding test cases for:
- Expired tokens
- Malformed tokens
- Missing required claims
Here's an example of how to add these cases:
public void SetupMockGoogleLogin(User user, string? token = null) { if (token is null) { GoogleServiceMock .Setup(gs => gs.ValidateGoogleToken("invalid_google_id_token")) .ThrowsAsync(new InvalidJwtException("Invalid Google Token")); + + // Add cases for different failure scenarios + GoogleServiceMock + .Setup(gs => gs.ValidateGoogleToken("expired_token")) + .ThrowsAsync(new InvalidJwtException("Token has expired")); + + GoogleServiceMock + .Setup(gs => gs.ValidateGoogleToken("malformed_token")) + .ThrowsAsync(new InvalidJwtException("Malformed token")); } else {
69-99: Reduce duplication in service registration.The service removal logic is repetitive. Consider extracting it into a helper method.
Here's a suggested refactor:
builder.ConfigureServices(services => { - var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IEmailService)); - if (descriptor != null) - { - services.Remove(descriptor); - } - - var googleDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IGoogleService)); - if (googleDescriptor != null) - { - services.Remove(googleDescriptor); - } - - var httpClientFactoryDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IHttpClientFactory)); - if (httpClientFactoryDescriptor != null) - { - services.Remove(httpClientFactoryDescriptor); - } - - var captchaDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(ICaptchaService)); - if (captchaDescriptor != null) - { - services.Remove(captchaDescriptor); - } + RemoveService<IEmailService>(services); + RemoveService<IGoogleService>(services); + RemoveService<IHttpClientFactory>(services); + RemoveService<ICaptchaService>(services); services.AddSingleton(EmailServiceMock.Object); services.AddSingleton(GoogleServiceMock.Object); services.AddSingleton(HttpClientFactoryMock.Object); services.AddSingleton(CaptchaServiceMock.Object); }); +private void RemoveService<T>(IServiceCollection services) +{ + var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(T)); + if (descriptor != null) + { + services.Remove(descriptor); + } +}
122-127: Enhance CaptchaService mock with failure scenarios.The current setup only handles the success case. Consider adding failure scenarios to make the tests more robust.
Here's a suggested enhancement:
private void SetupMockCaptchaService() { this.CaptchaServiceMock .Setup(service => service.ValidateReCaptchaAsync(It.IsAny<string>(), It.IsAny<CancellationToken?>())) .ReturnsAsync(Result.Ok()); + + // Add setup for invalid token scenario + this.CaptchaServiceMock + .Setup(service => service.ValidateReCaptchaAsync("invalid_token", It.IsAny<CancellationToken?>())) + .ReturnsAsync(Result.Fail("Invalid reCAPTCHA token")); + + // Add setup for timeout scenario + this.CaptchaServiceMock + .Setup(service => service.ValidateReCaptchaAsync("timeout_token", It.IsAny<CancellationToken?>())) + .ReturnsAsync(Result.Fail("reCAPTCHA verification timeout")); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/CustomWebApplicationFactory.cs(1 hunks)
🔇 Additional comments (1)
Streetcode/Streetcode.XIntegrationTest/ControllerTests/Utils/CustomWebApplicationFactory.cs (1)
20-29: Well-structured mock declarations!Good job on maintaining encapsulation with private setters and proper initialization of all mock services.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }, | ||
| "ReCaptcha": { | ||
| "SecretKey": "6LeIxAcTAAAAAGG-vFI1TnRWxMZNFuojJ4WifJWe", | ||
| "SecretKey": "6LePO88qAAAAAIP7znxf4kS3OL6W8n1WGuvytyTX", |
Check failure
Code scanning / SonarCloud
reCaptcha secret keys should not be disclosed High
| }, | ||
| "ReCaptcha": { | ||
| "SecretKey": "6LeIxAcTAAAAAGG-vFI1TnRWxMZNFuojJ4WifJWe", | ||
| "SecretKey": "6LePO88qAAAAAIP7znxf4kS3OL6W8n1WGuvytyTX", |
Check failure
Code scanning / SonarCloud
reCaptcha secret keys should not be disclosed High
Summary of issue
Current version of recaptcha uses checkbox that enforces user to click it
Summary of change
Updated captcha to version 3 therefore it authomatically cheks if user is not a robot
Summary by CodeRabbit