feat: Implement Google Login API#41
Conversation
WalkthroughAdds external OAuth login (Google/Facebook/GitHub/Microsoft) with domain and persistence support, splits FullName into FirstName/LastName, adds avatars and external login entities, reorganizes service namespaces, removes the local password hasher, and introduces related migrations, DI registrations, and a new API endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant Mediator
participant Handler as LoginExternalCommandHandler
participant UserRepo as IUserRepository
participant ExternalRepo as IExternalLoginRepository
participant AuthService as IAuthService
participant TokenService as ITokenService
participant UnitOfWork
Client->>AuthController: POST /login-external (provider, providerKey, email, firstName, lastName)
AuthController->>Mediator: Send LoginExternalCommand
Mediator->>Handler: Handle(command)
alt External login exists
Handler->>ExternalRepo: GetByProviderAsync(provider, providerKey)
ExternalRepo-->>Handler: externalLogin
Handler->>UserRepo: GetByIdAsync(userId)
UserRepo-->>Handler: user (validate)
Handler->>TokenService: Generate tokens
TokenService-->>Handler: tokens
Handler-->>AuthController: LoginExternalResponse (IsNewUser=false)
else Email exists (no external)
Handler->>UserRepo: GetByEmailAsync(email)
UserRepo-->>Handler: user
Handler->>ExternalRepo: CreateAsync(new external login)
Handler->>UnitOfWork: SaveChangesAsync
Handler->>TokenService: Generate tokens
TokenService-->>Handler: tokens
Handler-->>AuthController: LoginExternalResponse (IsNewUser=false)
else New user
Handler->>UserRepo: CreateAsync(new user)
Handler->>AuthService: CreateIdentityUserWithoutPasswordAsync(...)
AuthService-->>Handler: success
Handler->>ExternalRepo: CreateAsync(new external login)
Handler->>UnitOfWork: SaveChangesAsync
Handler->>TokenService: Generate tokens
TokenService-->>Handler: tokens
Handler-->>AuthController: LoginExternalResponse (IsNewUser=true)
end
AuthController-->>Client: 200 OK / LoginExternalResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
- Split FullName into FirstName and LastName in RegisterCommand and LoginExternalCommandValidator. - Update validation rules for FirstName and LastName with appropriate messages and length constraints. - Introduce AvatarHelper for generating avatar URLs based on user names. - Implement secure external login with token verification in LoginExternalSecureCommand and LoginExternalSecureCommandHandler. - Add IExternalAuthService for verifying external provider tokens. - Create ExternalLoginRepository for managing external login data. - Enhance UserInfo model to include FirstName, LastName, and AvatarUrl. - Update IdentityService to support creating Identity users without passwords for external logins. - Modify appsettings.json to include Google authentication configuration. - Add ExternalProviderJsonConverter for handling external provider enum serialization.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.editorconfig (2)
59-59: Remove duplicate conflicting rule for namespace declarations.Lines 59 and 101 both define
csharp_style_namespace_declarationswith conflicting severities:file_scoped:error(line 59) andfile_scoped:none(line 101). Since the[*.{cs,vb}]section is evaluated after[*.cs], line 101 overrides line 59 for.csfiles, making line 59 ineffective and confusing. Either remove line 59 or consolidate the rules.- csharp_style_namespace_declarations = file_scoped:error csharp_style_expression_bodied_methods = false:silentOr, if
.csand.vbfiles should have different enforcement:
- Keep line 59 and document the distinction clearly
- Ensure the
[*.{cs,vb}]section only applies to.vbfiles or separate them into distinct sectionsAlso applies to: 101-101
69-69: Remove duplicate conflicting rule for lambda expression bodies.Lines 69 and 112 both define
csharp_style_expression_bodied_lambdaswith different severities:true:error(line 69) andtrue:suggestion(line 112). Line 112 overrides line 69 for.csfiles, making line 69 ineffective. Remove the duplicate to clarify intent.csharp_style_expression_bodied_indexers = true:error csharp_style_expression_bodied_accessors = true:error - csharp_style_expression_bodied_lambdas = true:error csharp_style_expression_bodied_local_functions = true:errorAlso applies to: 112-112
src/Application/Features/Auth/Register/RegisterCommandHandler.cs (1)
35-44: Avatar URL is dropped at persistence
Domain.Entities.Userin the updated EF Core snapshot (20251111165001_AddUserExternalLoginTable.Designer) still exposes only Email/FullName/PhoneNumber/Roles/etc.—there is no mappedAvatarUrlproperty. Because of that, this assignment never reaches the database, so any subsequent load of the user will returnAvatarUrl = null, undoing the avatar you just generated. Please extend the model configuration and migration to add theAvatarUrlcolumn (and wire it through the repository) before relying on this field.
♻️ Duplicate comments (1)
src/Application/Features/Auth/LoginExternal/LoginExternalResponse.cs (1)
8-39: Duplicate response structure noted.This response has an identical structure to
LoginExternalSecureResponse. See the review comment onLoginExternalSecureResponse.csregarding consolidation.
🧹 Nitpick comments (12)
src/Application/Interfaces/Services/Auth/IExternalAuthService.cs (2)
19-29: Consider using the ExternalProvider enum instead of string for type safety.The
Providerproperty is defined asstring, butDomain.Constants.ExternalProvidersdefines anExternalProviderenum. Using the enum would provide:
- Compile-time type safety
- Prevention of invalid provider values
- Better IDE support and refactoring
+using Domain.Constants; + namespace Application.Interfaces.Services.Auth; // ... public sealed record ExternalUserInfo { /// <summary> - /// Provider name (Google, Facebook, etc.) + /// External authentication provider /// </summary> - public required string Provider { get; init; } + public required ExternalProvider Provider { get; init; }Note: This change would require updates in the implementation and consumers of this type.
19-34: Consider adding validation for required fields.While the
requiredkeyword ensures properties are initialized, it doesn't prevent empty strings. Consider adding validation to ensureProvider,ProviderKey, andYou could add a private constructor with validation or leverage the
Domain.ValueObjects.Emailfor the Email property to ensure format validity.src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureCommand.cs (1)
9-21: Consider using ExternalProvider enum and add validation.Two recommendations:
Type safety: The
Providerproperty usesstringinstead of theExternalProviderenum fromDomain.Constants. Using the enum would prevent invalid provider values and improve type safety.Input validation: While
requiredprevents null values, it allows empty strings. Consider adding validation attributes or a FluentValidation validator to ensureProviderandIdTokencontain meaningful values.+using Domain.Constants; + namespace Application.Features.Auth.LoginExternalSecure; /// <summary> /// Secure external login command using token verification /// </summary> public sealed record LoginExternalSecureCommand : ICommand<Result<LoginExternalSecureResponse>> { /// <summary> - /// Provider type (currently only Google is supported) + /// External authentication provider /// </summary> - public required string Provider { get; init; } + public required ExternalProvider Provider { get; init; } /// <summary> /// ID token from Google OAuth (JWT) /// This token will be verified with Google's servers to ensure authenticity /// </summary> public required string IdToken { get; init; } }Additionally, create a corresponding
LoginExternalSecureCommandValidatorusing FluentValidation to ensureIdTokenis not empty and follows expected token format patterns.src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (1)
19-19: Consider returning IReadOnlyList for better flexibility.Returning a concrete
List<UserExternalLogin>couples callers to a specific collection type. Consider returningIReadOnlyList<UserExternalLogin>orIReadOnlyCollection<UserExternalLogin>for a more flexible contract.Apply this diff:
- Task<List<UserExternalLogin>> GetByUserIdAsync(Guid userId, CancellationToken cancellationToken = default); + Task<IReadOnlyList<UserExternalLogin>> GetByUserIdAsync(Guid userId, CancellationToken cancellationToken = default);src/Infrastructure/Extensions/ServiceCollectionExtensions.cs (1)
101-101: Consider validating Google configuration at startup.The
ExternalAuthServicevalidates the Google ClientId at runtime (inVerifyGoogleTokenAsync), which could lead to runtime exceptions if the configuration is missing. Consider adding configuration validation during service registration for fail-fast behavior.Add validation in the
AddInfrastructureApplicationServicesmethod:private static IServiceCollection AddInfrastructureApplicationServices(this IServiceCollection services) { // Authentication & Security services.AddScoped<ITokenService, TokenService>(); services.AddScoped<ITokenGenerationService, TokenGenerationService>(); services.AddScoped<IExternalAuthService, ExternalAuthService>(); + + // Validate external auth configuration at startup + services.AddOptions<ExternalAuthOptions>() + .BindConfiguration("Authentication:Google") + .ValidateDataAnnotations() + .ValidateOnStart(); // Email Services services.AddScoped<IEmailService, EmailService>();Alternatively, add a simple check:
+private static void ValidateExternalAuthConfiguration(IConfiguration configuration) +{ + var googleClientId = configuration["Authentication:Google:ClientId"]; + if (string.IsNullOrEmpty(googleClientId)) + { + throw new InvalidOperationException("Google ClientId must be configured in appsettings.json under Authentication:Google:ClientId"); + } +}src/Application/Features/Auth/LoginExternal/LoginExternalCommand.cs (1)
26-26: Consider using the Email value object for consistency.The domain includes an
src/Domain/ValueObjects/Email.cs) with built-in validation logic. Usingstringhere creates inconsistency across the codebase and bypasses domain validation. Consider usingDomain.ValueObjects.Emailinstead ofstringfor the Email property.If applied consistently across commands:
+using Domain.ValueObjects; + namespace Application.Features.Auth.LoginExternal; public sealed record LoginExternalCommand : ICommand<Result<LoginExternalResponse>> { ... - public required string Email { get; init; } + public required Email Email { get; init; } ... }src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureResponse.cs (1)
8-39: Consider consolidating duplicate response types.
LoginExternalSecureResponseis nearly identical toLoginExternalResponse(insrc/Application/Features/Auth/LoginExternal/LoginExternalResponse.cs). Both have the same properties:AccessToken,RefreshToken,ExpiresAt,User,IsNewUser, andProvider. This duplication increases maintenance burden.Consider:
- Using a single shared response type for both secure and non-secure external login flows
- Using a shared base record if the flows truly need separate response types
- Document the distinction between "secure" and "non-secure" flows if they are meaningfully different
Example consolidation:
-namespace Application.Features.Auth.LoginExternalSecure; +namespace Application.Features.Auth.Common; /// <summary> -/// Secure external login response +/// External login response (used for both secure and non-secure flows) /// </summary> -public sealed record LoginExternalSecureResponse +public sealed record ExternalLoginResponse { ... }src/Application/Features/Auth/Login/LoginCommandHandler.cs (2)
38-40: Name parsing logic is fragile.The current name parsing logic (
user.FullName.Split(' ', 2)) doesn't handle:
- Leading/trailing whitespace
- Multiple consecutive spaces
- Names with more than 2 parts (e.g., "John Middle Last")
- Culture-specific name formats (e.g., "Last, First")
Consider creating a dedicated helper method with more robust parsing, or better yet, update the domain model to store FirstName and LastName separately instead of parsing at runtime.
Example helper method:
private static (string FirstName, string LastName) ParseFullName(string fullName) { var trimmed = fullName?.Trim() ?? string.Empty; if (string.IsNullOrWhiteSpace(trimmed)) { return (string.Empty, string.Empty); } var parts = trimmed.Split(' ', StringSplitOptions.RemoveEmptyEntries); return parts.Length switch { 0 => (string.Empty, string.Empty), 1 => (parts[0], string.Empty), _ => (parts[0], string.Join(' ', parts.Skip(1))) }; }
38-50: Consider aligning domain and application models.There's a mismatch between the domain and application layers:
- The
Userentity (domain) storesFullNameas a single string- The
UserInfomodel (application) uses separateFirstNameandLastNameproperties- External logins store
FirstNameandLastNameseparately in theUserExternalLoginentityThis creates inconsistency where:
- Traditional users are stored with
FullNameand parsed at runtime- External login users have proper
FirstName/LastNameseparationConsider updating the
Userdomain entity to useFirstNameandLastNameproperties instead ofFullName, or add both withFullNameas a computed property. This would eliminate the need for fragile runtime parsing and align with the external login model.src/Application/Common/Models/UserInfo.cs (1)
7-43: Well-structured authentication response model.The record is clean and properly documented. The computed
FullNameproperty is a good design choice for consistent formatting.Consider using
IReadOnlyList<string>instead ofList<string>for theRolesproperty to prevent external modification and better align with the immutability of records:- public required List<string> Roles { get; init; } + public required IReadOnlyList<string> Roles { get; init; }This ensures that once a
UserInfoinstance is created, its roles cannot be modified through the list reference.src/Infrastructure/Repositories/ExternalLoginRepository.cs (1)
26-38: Minor formatting inconsistency in XML documentation.There's inconsistent spacing between the method on line 30 and its XML documentation. The closing brace on line 29 should have consistent spacing before the next method's documentation.
Apply this formatting fix:
el => el.Provider == provider && el.ProviderKey == providerKey, cancellationToken); - } /// <summary> - /// Get external logins by user ID from database - /// </summary> + } + + /// <summary> + /// Get external logins by user ID from database + /// </summary> public async Task<List<UserExternalLogin>> GetByUserIdAsync(Guid userId, CancellationToken cancellationToken = default)src/Application/Interfaces/Services/Auth/IAuthService.cs (1)
1-51: Excellent interface design with clear documentation.The extensive XML documentation clearly explains the purpose and boundaries of this interface, especially the distinction between
IAuthService(Identity operations) andIUserRepository(domain operations). The addition ofCreateIdentityUserWithoutPasswordAsyncproperly supports the external login flow.Consider returning more detailed result types instead of
boolfor authentication methods to support better error handling and user feedback:// Example: Instead of bool, could return an enum or result type public enum SignInResult { Success, InvalidCredentials, LockedOut, NotFound } Task<SignInResult> SignInAsync(string email, string password, bool rememberMe = false);This would allow callers to provide more specific error messages to users (e.g., "Account locked" vs "Invalid password").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.editorconfig(3 hunks)src/Application/Common/Helpers/AvatarHelper.cs(1 hunks)src/Application/Common/Models/UserInfo.cs(1 hunks)src/Application/EventHandlers/EmailVerifiedEventHandler.cs(1 hunks)src/Application/EventHandlers/UserCreatedEventHandler.cs(1 hunks)src/Application/Features/Auth/GetProfile/GetProfileQueryHandler.cs(1 hunks)src/Application/Features/Auth/Login/LoginCommand.cs(1 hunks)src/Application/Features/Auth/Login/LoginCommandHandler.cs(2 hunks)src/Application/Features/Auth/LoginExternal/LoginExternalCommand.cs(1 hunks)src/Application/Features/Auth/LoginExternal/LoginExternalCommandHandler.cs(1 hunks)src/Application/Features/Auth/LoginExternal/LoginExternalCommandValidator.cs(1 hunks)src/Application/Features/Auth/LoginExternal/LoginExternalResponse.cs(1 hunks)src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureCommand.cs(1 hunks)src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureCommandHandler.cs(1 hunks)src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureResponse.cs(1 hunks)src/Application/Features/Auth/Register/RegisterCommand.cs(1 hunks)src/Application/Features/Auth/Register/RegisterCommandHandler.cs(4 hunks)src/Application/Features/Auth/Register/RegisterCommandValidator.cs(1 hunks)src/Application/Features/Auth/Register/RegisterResponse.cs(1 hunks)src/Application/Features/Auth/VerifyEmail/VerifyEmailCommandHandler.cs(1 hunks)src/Application/Interfaces/IAuthService.cs(0 hunks)src/Application/Interfaces/IPasswordHasher.cs(0 hunks)src/Application/Interfaces/IUserRepository.cs(0 hunks)src/Application/Interfaces/Repositories/IExternalLoginRepository.cs(1 hunks)src/Application/Interfaces/Repositories/IUserRepository.cs(1 hunks)src/Application/Interfaces/Services/Auth/IAuthService.cs(1 hunks)src/Application/Interfaces/Services/Auth/IExternalAuthService.cs(1 hunks)src/Application/Interfaces/Services/Auth/ITokenGenerationService.cs(1 hunks)src/Application/Interfaces/Services/Auth/ITokenService.cs(1 hunks)src/Application/Interfaces/Services/Email/IEmailService.cs(1 hunks)src/Application/Interfaces/Services/Email/IEmailTemplateService.cs(1 hunks)src/Domain/Constants/ExternalProviders.cs(1 hunks)src/Domain/Entities/User.cs(1 hunks)src/Domain/Entities/UserExternalLogin.cs(1 hunks)src/Infrastructure/Data/Configurations/UserExternalLoginConfiguration.cs(1 hunks)src/Infrastructure/Data/Contexts/DataContext.cs(1 hunks)src/Infrastructure/Data/Migrations/20251111165001_AddUserExternalLoginTable.Designer.cs(1 hunks)src/Infrastructure/Data/Migrations/20251111165001_AddUserExternalLoginTable.cs(1 hunks)src/Infrastructure/Data/Migrations/DataContextModelSnapshot.cs(3 hunks)src/Infrastructure/Extensions/ServiceCollectionExtensions.cs(3 hunks)src/Infrastructure/Identity/IdentityService.cs(2 hunks)src/Infrastructure/Infrastructure.csproj(1 hunks)src/Infrastructure/Repositories/ExternalLoginRepository.cs(1 hunks)src/Infrastructure/Repositories/UserRepository.cs(1 hunks)src/Infrastructure/Services/Email/EmailService.cs(1 hunks)src/Infrastructure/Services/Email/EmailTemplateService.cs(1 hunks)src/Infrastructure/Services/ExternalAuthService.cs(1 hunks)src/Infrastructure/Services/PasswordHasher.cs(0 hunks)src/Infrastructure/Services/TokenGenerationService.cs(2 hunks)src/Infrastructure/Services/TokenService.cs(2 hunks)src/Web.Api/Controllers/V1/AuthController.cs(2 hunks)src/Web.Api/Converters/ExternalProviderJsonConverter.cs(1 hunks)src/Web.Api/Program.cs(1 hunks)src/Web.Api/appsettings.Development.json(1 hunks)src/Web.Api/appsettings.json(1 hunks)
💤 Files with no reviewable changes (4)
- src/Infrastructure/Services/PasswordHasher.cs
- src/Application/Interfaces/IUserRepository.cs
- src/Application/Interfaces/IAuthService.cs
- src/Application/Interfaces/IPasswordHasher.cs
🧰 Additional context used
🧬 Code graph analysis (22)
src/Application/Interfaces/Services/Email/IEmailService.cs (1)
src/Domain/ValueObjects/Email.cs (1)
src/Infrastructure/Data/Contexts/DataContext.cs (1)
src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)
src/Application/EventHandlers/UserCreatedEventHandler.cs (1)
src/Domain/ValueObjects/Email.cs (1)
src/Application/Interfaces/Services/Auth/IExternalAuthService.cs (1)
src/Domain/ValueObjects/Email.cs (1)
src/Infrastructure/Data/Configurations/UserExternalLoginConfiguration.cs (1)
src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)
src/Application/Features/Auth/LoginExternal/LoginExternalCommandValidator.cs (1)
src/Domain/ValueObjects/Email.cs (1)
src/Infrastructure/Services/ExternalAuthService.cs (1)
src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureCommandHandler.cs (2)
Task(27-196)Task(201-231)
src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureResponse.cs (1)
src/Domain/Entities/User.cs (1)
User(9-43)
src/Web.Api/Program.cs (2)
src/Web.Api/Extensions/SerilogExtensions.cs (2)
SerilogExtensions(10-44)ConfigureSerilog(15-35)src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProviderJsonConverter(11-57)
src/Application/Interfaces/Repositories/IUserRepository.cs (2)
src/Domain/Aggregates/Conversation/ConversationAggregate.cs (1)
Entities(32-32)src/Domain/Entities/User.cs (1)
User(9-43)
src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (2)
src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProvider(13-50)
src/Domain/Constants/ExternalProviders.cs (1)
src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProvider(13-50)
src/Application/Features/Auth/LoginExternal/LoginExternalCommand.cs (2)
src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProvider(13-50)src/Domain/ValueObjects/Email.cs (1)
src/Infrastructure/Repositories/ExternalLoginRepository.cs (2)
src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (3)
Task(14-14)Task(19-19)Task(24-24)src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)
src/Application/Features/Auth/LoginExternal/LoginExternalCommandHandler.cs (9)
src/Domain/Entities/User.cs (1)
User(9-43)src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (3)
Task(14-14)Task(19-19)Task(24-24)src/Application/Interfaces/Repositories/IUserRepository.cs (6)
Task(33-33)Task(38-38)Task(43-43)Task(48-48)Task(53-53)Task(58-58)src/Application/Interfaces/Services/Auth/IAuthService.cs (5)
Task(25-25)Task(30-30)Task(35-35)Task(40-40)Task(45-45)src/Application/Common/Helpers/AvatarHelper.cs (1)
AvatarHelper(6-19)src/Domain/Constants/UserRoles.cs (1)
UserRoles(3-38)src/Application/Interfaces/Services/Auth/ITokenService.cs (2)
GenerateAccessToken(13-13)GenerateRefreshToken(18-18)src/Infrastructure/Services/TokenService.cs (2)
GenerateAccessToken(30-69)GenerateRefreshToken(74-80)
src/Application/Features/Auth/LoginExternalSecure/LoginExternalSecureCommandHandler.cs (9)
src/Domain/Entities/User.cs (1)
User(9-43)src/Domain/Aggregates/Conversation/ConversationAggregate.cs (1)
Entities(32-32)src/Domain/Entities/UserExternalLogin.cs (1)
UserExternalLogin(9-43)src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (3)
Task(14-14)Task(19-19)Task(24-24)src/Application/Interfaces/Services/Auth/IAuthService.cs (3)
Task(25-25)Task(30-30)Task(35-35)src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProvider(13-50)src/Domain/Constants/UserRoles.cs (1)
UserRoles(3-38)src/Domain/Events/User/UserCreatedEvent.cs (1)
UserCreatedEvent(17-25)src/Application/Interfaces/Services/Auth/ITokenService.cs (2)
GenerateAccessToken(13-13)GenerateRefreshToken(18-18)
src/Application/Features/Auth/Register/RegisterCommandHandler.cs (2)
src/Domain/ValueObjects/Email.cs (2)
ToString(34-34)src/Application/Common/Helpers/AvatarHelper.cs (1)
AvatarHelper(6-19)
src/Application/Interfaces/Services/Auth/IAuthService.cs (1)
src/Web.Api/Services/CurrentUserService.cs (1)
List(54-86)
src/Application/Common/Models/UserInfo.cs (2)
src/Domain/ValueObjects/Email.cs (1)
src/Web.Api/Services/CurrentUserService.cs (1)
List(54-86)
src/Domain/Entities/UserExternalLogin.cs (2)
src/Domain/Aggregates/Conversation/ConversationAggregate.cs (1)
Entities(32-32)src/Web.Api/Converters/ExternalProviderJsonConverter.cs (1)
ExternalProvider(13-50)
src/Application/Interfaces/Services/Email/IEmailTemplateService.cs (1)
src/Domain/ValueObjects/Email.cs (1)
src/Infrastructure/Extensions/ServiceCollectionExtensions.cs (2)
src/Infrastructure/Repositories/ExternalLoginRepository.cs (2)
ExternalLoginRepository(12-49)ExternalLoginRepository(16-19)src/Infrastructure/Services/ExternalAuthService.cs (2)
ExternalAuthService(10-65)ExternalAuthService(14-17)
🔇 Additional comments (34)
src/Domain/Entities/User.cs (1)
14-14: LGTM!The addition of the
AvatarUrlproperty is appropriate for supporting user avatars in external authentication flows. The nullable type correctly reflects that avatars are optional.src/Infrastructure/Services/Email/EmailTemplateService.cs (1)
1-1: LGTM!The namespace reorganization improves code organization by grouping email-related interfaces under
Application.Interfaces.Services.Email.src/Infrastructure/Services/Email/EmailService.cs (1)
3-3: LGTM!Consistent namespace reorganization with other email services in the PR.
src/Application/Features/Auth/VerifyEmail/VerifyEmailCommandHandler.cs (1)
2-3: LGTM!The more specific using directives improve code clarity by making dependencies explicit and align with the namespace reorganization throughout the PR.
src/Application/Interfaces/Services/Email/IEmailService.cs (1)
1-1: LGTM!The namespace change improves organization by consolidating email-related interfaces.
src/Infrastructure/Repositories/UserRepository.cs (1)
1-1: LGTM!The namespace change improves organization by grouping repository interfaces together, consistent with the broader interface reorganization in this PR.
src/Application/EventHandlers/EmailVerifiedEventHandler.cs (1)
2-2: LGTM!The namespace update aligns with the email service interface reorganization throughout the PR.
src/Application/EventHandlers/UserCreatedEventHandler.cs (1)
2-3: LGTM! Namespace reorganization improves code structure.The refactor from the broad
Application.Interfacesto more specificApplication.Interfaces.Services.AuthandApplication.Interfaces.Services.Emailnamespaces improves code organization and makes dependencies more explicit.src/Application/Features/Auth/GetProfile/GetProfileQueryHandler.cs (1)
2-2: LGTM! Namespace refactor aligns with repository organization.The updated namespace
Application.Interfaces.Repositoriescorrectly organizes repository interfaces separately from other application interfaces.src/Infrastructure/Services/TokenService.cs (2)
1-2: LGTM! Namespace updates align with authentication service reorganization.The updated imports correctly reference the new locations for
UserInfo(inApplication.Common.Models) andITokenService(inApplication.Interfaces.Services.Auth), supporting the centralization of authentication-related types.
47-49: LGTM! Formatting adjustment improves readability.The line break adjustment for the
Iatclaim is a minor formatting improvement with no functional impact.src/Infrastructure/Services/TokenGenerationService.cs (1)
1-1: LGTM! Namespace consolidation improves authentication service organization.The updated namespace
Application.Interfaces.Services.Authgroups authentication-related services logically.src/Application/Features/Auth/Login/LoginCommand.cs (1)
2-2: LGTM! Centralization of UserInfo reduces duplication.Moving
UserInfotoApplication.Common.Modelseliminates duplication across authentication flows and ensures consistency in user data representation across login, registration, and external authentication features.src/Infrastructure/Data/Contexts/DataContext.cs (1)
34-34: LGTM! External login entity properly integrated.The new
DbSet<UserExternalLogin>property correctly exposes the external login entity to EF Core, following the established pattern and enabling the tracking of OAuth/social authentication associations.src/Application/Interfaces/Services/Email/IEmailTemplateService.cs (1)
1-1: LGTM! Namespace refactor groups email services logically.Moving to
Application.Interfaces.Services.Emailimproves organization by grouping all email-related service interfaces together.src/Application/Interfaces/Services/Auth/ITokenGenerationService.cs (1)
1-1: LGTM! Namespace reorganization improves code structure.The namespace reorganization aligns authentication-related interfaces under a dedicated
Authsubfolder, improving discoverability and maintainability.src/Application/Interfaces/Services/Auth/ITokenService.cs (1)
1-3: LGTM! Improved namespace organization and shared model usage.The changes correctly refactor to use the shared
UserInfomodel fromApplication.Common.Modelsand align the namespace with the authentication services structure.src/Domain/Constants/ExternalProviders.cs (1)
1-27: LGTM! Well-structured enum with explicit values for database persistence.The enum is properly defined with explicit integer values (important for database storage) and comprehensive XML documentation. Note that while the enum defines four providers, based on the PR scope, only Google authentication is currently implemented. The additional providers appear to be defined for future expansion.
src/Infrastructure/Infrastructure.csproj (1)
10-10: Google.Apis.Auth version 1.72.0 is current and secure.The latest version of Google.Apis.Auth on NuGet is 1.72.0, which matches the version specified in the project file. No security vulnerabilities were found in the GitHub security database for this package. The dependency is up-to-date and poses no known security risks.
src/Infrastructure/Data/Migrations/DataContextModelSnapshot.cs (1)
201-254: UserExternalLogin snapshot mapping looks consistentProperties, indexes, and cascade delete wiring in the snapshot line up with the new configuration, so the model snapshot stays in sync with the entity mapping.
src/Application/Interfaces/Repositories/IExternalLoginRepository.cs (2)
14-14: LGTM!The method signature is well-designed for external login lookups with appropriate nullable return type.
24-24: Duplicate handling is properly implemented.Verification confirms:
- ✅ Unique constraint exists on
(Provider, ProviderKey)in database configuration (UserExternalLoginConfiguration.cs)- ✅ Both handlers calling
CreateAsync(LoginExternalSecureCommandHandlerandLoginExternalCommandHandler) check for duplicates viaGetByProviderAsyncbefore creation- ✅ Database and application layers both protect against duplicate external logins
No changes needed.
src/Application/Features/Auth/Register/RegisterCommandValidator.cs (1)
28-36: LGTM!The validator correctly splits name validation into separate FirstName and LastName rules with appropriate constraints (NotEmpty, MinimumLength 2, MaximumLength 100). This aligns well with the updated RegisterCommand structure.
src/Infrastructure/Extensions/ServiceCollectionExtensions.cs (2)
84-84: LGTM!The
IExternalLoginRepositoryregistration with Scoped lifetime is appropriate for EF Core repository pattern.
81-91: IPasswordHasher removal is correctly implemented and doesn't break existing flows.Verification confirms:
- No orphaned IPasswordHasher references remain in the codebase
IdentityService.CheckPasswordAsync()properly delegates to ASP.NET Core Identity'sUserManager.CheckPasswordAsync(), which handles password verification internally- Both register and login flows use
IAuthServicefor password operations, which correctly abstracts to the Identity framework- Password hashing is now fully managed by ASP.NET Core Identity rather than through direct dependency injection
The removal is safe and complete.
src/Application/Features/Auth/Register/RegisterResponse.cs (1)
19-31: LGTM!The refactoring from a single
FullNameproperty to separateFirstNameandLastNameproperties with a computedFullNameis well-designed. The.Trim()on the computed property properly handles edge cases whereLastNamemight be empty.src/Domain/Entities/UserExternalLogin.cs (1)
9-43: Domain entity structure is clean and appropriate.The entity correctly models the external login relationship with proper nullable fields for optional provider data (avatar, names). The use of the
ExternalProviderenum for type safety is good practice.src/Infrastructure/Repositories/ExternalLoginRepository.cs (1)
24-48: Repository implementation is correct and well-documented.The methods properly use async/await patterns, support cancellation tokens, and follow the UnitOfWork pattern. The XML documentation clearly explains the SaveChanges responsibility.
src/Application/Features/Auth/LoginExternal/LoginExternalCommandValidator.cs (2)
10-46: Comprehensive validation with proper constraints.The validator enforces all required fields and length constraints that align with the database migration. The use of
When()for optionalAvatarUrlvalidation is correct. The field length limits match the migration constraints exactly.
22-28: Duplicate error message won't be used.The error message on line 26 "Email must be a valid email address" will be overridden by the message on line 28 "Email must not exceed 256 characters" since both
WithMessage()calls apply to the sameEmailAddress()rule.Restructure to properly associate each message with its respective validation:
RuleFor(x => x.Email) .NotEmpty() .WithMessage("Email is required") .EmailAddress() .WithMessage("Email must be a valid email address") .MaximumLength(256) .WithMessage("Email must not exceed 256 characters");Actually, looking more carefully, each
WithMessage()should apply to the immediately preceding rule. However, FluentValidation chains applyWithMessage()to the last rule in the chain before it. Let me verify this is correct:Actually, on review, this appears to be correct FluentValidation syntax where each
WithMessage()applies to the preceding rule. The structure should work as intended. Let me retract my concern.src/Web.Api/Program.cs (2)
25-30: JSON converter registration for ExternalProvider enum is correct.Adding the
ExternalProviderJsonConverterto the JSON serialization options ensures proper (de)serialization of theExternalProviderenum in API requests and responses. This is necessary for the external login flow.
14-92: Verify removal of startup error handling is intentional.The previous version likely had try/catch/finally wrapper around the startup sequence to handle and log startup failures. Removing this wrapper means unhandled exceptions during startup may not be logged by Serilog before the application terminates.
Please confirm this change is intentional. If startup error logging is still desired, consider wrapping the main execution in a try/catch:
try { Log.Information("Starting Legal Assistant API"); var builder = WebApplication.CreateBuilder(args); // ... rest of startup code ... await app.RunAsync(); Log.Information("Legal Assistant API stopped"); } catch (Exception ex) { Log.Fatal(ex, "Application startup failed"); throw; } finally { await Log.CloseAndFlushAsync(); }src/Infrastructure/Data/Migrations/20251111165001_AddUserExternalLoginTable.cs (1)
14-55: Migration structure and entity initialization are correct—no issues found.Verification confirms that
CreatedAtis properly initialized by application code before persistence. BothLoginExternalCommandHandlerandLoginExternalSecureCommandHandlerexplicitly setCreatedAt = DateTime.UtcNowwhen creatingUserExternalLogininstances, ensuring theNOT NULLcolumn receives a value before the entity is passed to the repository.The unique composite index on
(Provider, ProviderKey)prevents duplicates, and cascade delete onUserIdis appropriate for this dependent relationship.src/Infrastructure/Services/ExternalAuthService.cs (1)
26-31: Google ClientId configuration is properly set and validated.The configuration exists in
appsettings.jsonwith a placeholder value, and the code correctly validates its presence at runtime, throwing anInvalidOperationExceptionif the value is missing or empty. This is appropriate defensive programming for a required configuration value.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Application/Common/Helpers/AvatarHelper.cs (2)
37-37: Consider making the avatar service URL configurable.The hardcoded
ui-avatars.comURL creates a tight coupling to an external service. While the documentation mentions this concern, making it configurable would improve flexibility for production environments.Consider:
- Adding an
IOptions<AvatarOptions>parameter (or similar configuration)- Allowing the base URL to be overridden via
appsettings.json- Providing the current URL as a sensible default
This would enable:
- Easy switching to a self-hosted avatar service
- Testing with mock services
- Compliance with organizational policies about external dependencies
29-29: Consider extracting magic numbers to named constants.The values
1000(max size),16(color range), and200(default size) are magic numbers that could benefit from named constants for clarity and maintainability.public static class AvatarHelper { + private const int DefaultAvatarSize = 200; + private const int MaxAvatarSize = 1000; + private const int ColorRange = 16; + - public static Uri GenerateAvatarUrl(string fullName, int size = 200) + public static Uri GenerateAvatarUrl(string fullName, int size = DefaultAvatarSize) { // ... validation code ... - if (size <= 0 || size > 1000) + if (size <= 0 || size > MaxAvatarSize) { - throw new ArgumentOutOfRangeException(nameof(size), "Size must be between 1 and 1000"); + throw new ArgumentOutOfRangeException(nameof(size), $"Size must be between 1 and {MaxAvatarSize}"); } // ... in the hash calculation ... - var backgroundSeed = Math.Abs(fullName.GetHashCode() % 16).ToString("X", CultureInfo.InvariantCulture); + var backgroundSeed = Math.Abs(fullName.GetHashCode() % ColorRange).ToString("X", CultureInfo.InvariantCulture); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Application/Common/Helpers/AvatarHelper.cs(1 hunks)src/Web.Api/Controllers/V1/AuthController.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Web.Api/Controllers/V1/AuthController.cs
* feat: Create UserExternalLogin table * chore: Refactor code * feat(auth): Refactor user registration and external login handling - Split FullName into FirstName and LastName in RegisterCommand and LoginExternalCommandValidator. - Update validation rules for FirstName and LastName with appropriate messages and length constraints. - Introduce AvatarHelper for generating avatar URLs based on user names. - Implement secure external login with token verification in LoginExternalSecureCommand and LoginExternalSecureCommandHandler. - Add IExternalAuthService for verifying external provider tokens. - Create ExternalLoginRepository for managing external login data. - Enhance UserInfo model to include FirstName, LastName, and AvatarUrl. - Update IdentityService to support creating Identity users without passwords for external logins. - Modify appsettings.json to include Google authentication configuration. - Add ExternalProviderJsonConverter for handling external provider enum serialization. * Update .editorconfig Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Remove external login secure command and handler * fix: Enhance avatar URL generation and add external login endpoint --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit