feat: Implement external login functionality and user profile management#53
Conversation
- Added ExternalLoginCommand and its handler to support external provider logins (e.g., Google, Facebook). - Created ExternalLoginCommandValidator for input validation of external login requests. - Developed GetProfileQuery and its handler to retrieve the current user's profile information. - Implemented UpdateProfileCommand and its handler for updating user profile details. - Introduced necessary response models for external login and user profile operations. - Updated AuthController to include endpoints for external login and user logout. - Refactored UsersController to manage user profile retrieval and updates.
WalkthroughRefactors external login terminology from "LoginExternal" to "ExternalLogin" across multiple layers, adds user profile management endpoints (GetProfile, UpdateProfile), implements logout with refresh token revocation, removes centralized authorization filters, and updates API routing accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/Application/Features/User/UpdateProfile/UpdateProfileCommand.cs (1)
6-20: Clarify null/empty semantics for profile fields
FullNameandAvatarUrlare nullable, which is good for partial updates, but it’d be useful to ensure the handler clearly distinguishes:
null→ “leave as is”- non-null (including empty string) → “update/clear value”
Just confirm the handler follows this contract and that it’s documented for clients so they don’t accidentally wipe values.
src/Application/Features/Auth/ExternalLogin/ExternalLoginCommandHandler.cs (2)
22-175: ExternalLogin rename and flow look consistent; validate repository/unit-of-work assumptionsThe handler’s rename to
ExternalLoginCommandHandlerand switch toResult<ExternalLoginResponse>is consistent across all branches (NotFound,Deactivated,IdentityFailed, and success paths), and the three scenarios (existing external login, existing user by email, new user) read logically.A couple of points to double‑check rather than change blindly here:
- The code assumes
request.Email,request.FirstName,request.LastName, andrequest.ProviderKeyare non‑null/valid. Please confirmExternalLoginCommandValidatorenforces that so this handler never gets bad inputs.- User creation + external login + refresh token creation appear to rely on a shared unit of work / context. You call
unitOfWork.SaveChangesAsyncfor external login changes but not after adding refresh tokens; if you also have a pipeline behavior that saves at the end of a request, just ensure you’re not inadvertently doing multiple commits or leaving refresh tokens unpersisted when no outer save runs.If those assumptions hold, the logic itself looks sound.
180-227: Name splitting inGenerateTokenResponseis simplistic; consider future‑proofing
GenerateTokenResponsederives first/last names by splittinguser.FullNameon the first space. This works for many Western‑style names but degrades for multi‑word or non‑Western names and loses the original external provider’s first/last breakdown.Not urgent, but for better i18n/name handling you might later:
- Store first/last names explicitly on the user (or reuse
TblUserExternalLogin’s first/last when available), and- Treat
FullNameas a display-only field not used to reconstruct structured name parts.For now, the existing behavior is consistent with the rest of the model.
src/Web.Api/Program.cs (1)
25-30: Confirm removal of global authorization filter is intentionalSwitching to plain
AddControllers()(without the former globalCustomAuthorizeFilter) changes how authorization failures are shaped/handled. If the goal is to rely purely on[Authorize]attributes + JWT middleware and default 401/403 responses, this is fine—just make sure:
- All sensitive endpoints have proper
[Authorize]attributes, and- Any custom error/response formatting previously in the filter is now covered elsewhere (e.g., middleware) if still required.
src/Application/Features/User/UpdateProfile/UpdateProfileCommandHandler.cs (1)
42-42: Consider conditional UpdatedAt.
UpdatedAtis set unconditionally even when no fields change. Consider setting it only whenFullNameorAvatarUrlis actually modified.src/Application/Features/User/GetProfile/GetUserProfileResponse.cs (1)
30-30: Consider using IReadOnlyList for immutable collection properties.The
List<string>andList<ExternalLoginInfo>properties expose mutable collections, allowing callers to modify the lists after construction. For DTOs withinit-only setters, usingIReadOnlyList<T>better conveys immutability and prevents unintended modifications.Apply this diff to use immutable collection types:
- public required List<string> Roles { get; init; } + public required IReadOnlyList<string> Roles { get; init; }- public required List<ExternalLoginInfo> ExternalLogins { get; init; } + public required IReadOnlyList<ExternalLoginInfo> ExternalLogins { get; init; }Also applies to: 45-45
src/Application/Features/User/GetProfile/GetProfileQueryHandler.cs (1)
46-55: Consider providing a fallback for null DisplayName.The DisplayName construction at lines 50-52 returns
nullwhen bothFirstNameandLastNameare null. This could result in a nullDisplayNamein the response, which may lead to poor UX when displaying external login information.Consider adding a fallback to improve the user experience:
DisplayName = el.FirstName != null && el.LastName != null ? $"{el.FirstName} {el.LastName}" - : el.FirstName ?? el.LastName, + : el.FirstName ?? el.LastName ?? el.Provider.ToString(),This ensures that at minimum, the provider name is shown when personal names are unavailable.
src/Web.Api/Controllers/V1/AuthController.cs (1)
110-117: Consider adding [Authorize] attribute to the Logout endpoint.The logout endpoint is not protected by the
[Authorize]attribute. While theLogoutCommandlikely contains a refresh token for validation, it's a best practice to require authentication for logout operations to prevent potential abuse (e.g., token enumeration attacks).Apply this diff to add authorization:
/// <summary> /// Logout user by invalidating refresh token /// </summary> /// <param name="command">Logout command with refresh token</param> /// <returns>Success response</returns> [HttpPost("logout")] + [Authorize] [ProducesResponseType(typeof(ApiResponse<object>), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ApiResponse<object>), StatusCodes.Status400BadRequest)] public async Task<IActionResult> Logout([FromBody] LogoutCommand command)If there's a specific reason to keep it unauthenticated (e.g., allowing logout with only a refresh token without an access token), please document this design decision.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Application/Common/Models/UserInfo.cs(1 hunks)src/Application/Features/Auth/ExternalLogin/ExternalLoginCommand.cs(1 hunks)src/Application/Features/Auth/ExternalLogin/ExternalLoginCommandHandler.cs(7 hunks)src/Application/Features/Auth/ExternalLogin/ExternalLoginCommandValidator.cs(1 hunks)src/Application/Features/Auth/ExternalLogin/ExternalLoginInfo.cs(1 hunks)src/Application/Features/Auth/ExternalLogin/ExternalLoginlResponse.cs(1 hunks)src/Application/Features/Auth/Logout/LogoutCommandHandler.cs(1 hunks)src/Application/Features/User/GetProfile/GetProfileQuery.cs(1 hunks)src/Application/Features/User/GetProfile/GetProfileQueryHandler.cs(1 hunks)src/Application/Features/User/GetProfile/GetUserProfileResponse.cs(1 hunks)src/Application/Features/User/UpdateProfile/UpdateProfileCommand.cs(1 hunks)src/Application/Features/User/UpdateProfile/UpdateProfileCommandHandler.cs(1 hunks)src/Application/Features/User/UpdateProfile/UpdateProfileCommandValidator.cs(1 hunks)src/Application/Features/User/UpdateProfile/UpdateProfileResponse.cs(1 hunks)src/Web.Api/Controllers/V1/AuthController.cs(3 hunks)src/Web.Api/Controllers/V1/UsersController.cs(2 hunks)src/Web.Api/Filters/AuthorizationLoggingFilter.cs(0 hunks)src/Web.Api/Filters/CustomAuthorizeFilter.cs(0 hunks)src/Web.Api/Program.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Web.Api/Filters/CustomAuthorizeFilter.cs
- src/Web.Api/Filters/AuthorizationLoggingFilter.cs
🧰 Additional context used
🧬 Code graph analysis (4)
src/Application/Features/User/UpdateProfile/UpdateProfileCommandHandler.cs (1)
src/Application/Features/User/UpdateProfile/UpdateProfileResponse.cs (1)
UpdateProfileResponse(3-24)
src/Application/Features/User/GetProfile/GetProfileQuery.cs (1)
src/Application/Features/User/GetProfile/GetUserProfileResponse.cs (1)
GetUserProfileResponse(5-46)
src/Application/Features/User/GetProfile/GetProfileQueryHandler.cs (1)
src/Application/Features/User/GetProfile/GetUserProfileResponse.cs (1)
GetUserProfileResponse(5-46)
src/Application/Features/User/UpdateProfile/UpdateProfileCommand.cs (1)
src/Application/Features/User/UpdateProfile/UpdateProfileResponse.cs (1)
UpdateProfileResponse(3-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (15)
src/Application/Common/Models/UserInfo.cs (1)
3-6: Doc comment rename aligns with new ExternalLogin terminologyThe summary now correctly mentions
ExternalLoginalongside other auth responses; no behavioral impact and keeps docs consistent with the new feature naming.src/Application/Features/Auth/ExternalLogin/ExternalLoginlResponse.cs (1)
3-39: Response type rename is consistent and non‑breaking at payload levelRenaming to
ExternalLoginResponsein theApplication.Features.Auth.ExternalLoginnamespace keeps the JSON shape (tokens, user info, flags, provider) intact while aligning with the new command/handler naming. No issues from an API contract perspective as long as clients are updated to the new type name in strongly‑typed consumers.src/Application/Features/Auth/ExternalLogin/ExternalLoginInfo.cs (1)
1-32: LGTM!The record structure is well-designed with appropriate immutability, clear documentation, and correct use of required/optional properties.
src/Application/Features/User/GetProfile/GetProfileQuery.cs (1)
1-12: LGTM!The query structure correctly follows CQRS pattern for retrieving the current user's profile, delegating user identification to
ICurrentUserService.src/Application/Features/User/UpdateProfile/UpdateProfileCommandValidator.cs (1)
1-30: LGTM!The validation logic is correct and appropriate. The URL validation helper properly handles null/empty values and validates absolute HTTP/HTTPS URLs.
src/Application/Features/User/UpdateProfile/UpdateProfileResponse.cs (1)
1-24: LGTM!The response DTO structure is well-defined with appropriate required/optional properties and clear documentation.
src/Application/Features/Auth/ExternalLogin/ExternalLoginCommandValidator.cs (1)
1-47: LGTM!The rename from
LoginExternalCommandValidatortoExternalLoginCommandValidatorimproves naming consistency. Validation rules remain correct and comprehensive.src/Application/Features/Auth/ExternalLogin/ExternalLoginCommand.cs (1)
1-47: LGTM!The rename from
LoginExternalCommandtoExternalLoginCommandimproves naming consistency across the authentication feature.src/Application/Features/Auth/Logout/LogoutCommandHandler.cs (1)
10-28: LGTM!The logout implementation correctly validates the refresh token (existence, revocation status, expiration) and revokes it with an appropriate reason. The error handling is secure and prevents replay attacks.
src/Web.Api/Controllers/V1/UsersController.cs (3)
23-31: LGTM! Clean implementation of GetProfile endpoint.The endpoint correctly retrieves the current user's profile by delegating to the mediator with
GetProfileQuery. The use of a parameterless query is appropriate since the handler will retrieve the current user's ID fromICurrentUserService.
38-47: LGTM! UpdateProfile endpoint follows standard patterns.The endpoint correctly accepts the update command from the request body and delegates to the mediator for processing.
16-16: No security issues identified—review comment based on incorrect premise.The review assumes a "removed GetUsers endpoint" exists, but the current UsersController contains only two endpoints:
GetProfileandUpdateProfile. Both are self-service profile operations appropriately protected by class-level[Authorize]. No role-based authorization was needed for these endpoints. The import ofGetUsersQueryon line 3 is unused and orphaned, not evidence of a removed endpoint. The authorization model aligns correctly with the controller's current security requirements.Likely an incorrect or invalid review comment.
src/Application/Features/User/GetProfile/GetProfileQueryHandler.cs (2)
21-26: LGTM! Proper authentication check.The handler correctly validates that the user is authenticated before proceeding, returning an appropriate unauthorized error if not.
28-33: LGTM! Proper user existence validation.The handler correctly checks if the user exists and returns a not found error if the user doesn't exist in the repository.
src/Web.Api/Controllers/V1/AuthController.cs (1)
57-65: Verify that the route change is acceptable as a breaking change.The external login endpoint route changed from
login-externaltoexternal-login. This is a breaking API change that will affect existing clients. Ensure that:
- This change is intentional and documented
- Clients are notified of the update
- Consider if versioning or a deprecation period is needed
…e-endpoint feat: Implement external login functionality and user profile management
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.