Caching tokens with redis & reset Password #28
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR refactors auth and token handling to use Redis, adds password reset email workflows, updates startup configuration (JWT, Identity, Swagger, Redis, email), modifies DTOs for refresh tokens, deactivates EF RefreshToken model usage, and introduces configuration-backed email settings and cache key helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant AC as AuthController
participant AS as AuthService
participant R as RedisCacheService
participant UM as UserManager
Note over AC,AS: Login / Token issuance
C->>AC: POST /login (credentials)
AC->>AS: GetAuthToken(...)
AS->>UM: Validate user + password
AS->>AS: Generate JWT with jti
AS->>R: Set JwtCacheKey(jti) -> userId
AS->>R: Set UserJwtCacheKey(userId) -> jti (replace old)
AS->>R: Get UserRefreshTokenCacheKey(userId)
alt No refresh token in Redis
AS->>AS: generateRefreshToken()
AS->>R: Set UserRefreshTokenCacheKey(userId) -> RefreshTokenDto
end
AS-->>AC: AuthResponseDTO (JWT, RefreshTokenDto)
AC-->>C: 200 OK
sequenceDiagram
autonumber
participant C as Client
participant API as API (JWT Bearer)
participant R as RedisCacheService
Note over API,R: JWT validation (per request)
C->>API: Request with Bearer token (jti)
API->>R: Get JwtCacheKey(jti)
alt jti found
API-->>C: Proceed (token valid)
else jti missing
API-->>C: 401 (token revoked/invalid)
end
sequenceDiagram
autonumber
participant C as Client
participant AC as AuthController
participant AS as AuthService
participant R as RedisCacheService
Note over AC,AS: Logout
C->>AC: POST /logout/{userId} (refreshToken)
AC->>AS: Logout(userId, refreshToken)
AS->>R: Remove UserJwtCacheKey(userId) + JwtCacheKey(current jti)
AS->>R: Validate and remove UserRefreshTokenCacheKey(userId) if matches
AS-->>AC: Success
AC-->>C: 200 OK
sequenceDiagram
autonumber
participant C as Client
participant AC as AuthController
participant AS as AuthService
participant UM as UserManager
participant ES as IEmailSender
Note over AC,AS: Forgot/Reset password
C->>AC: POST /forgot-password (email)
AC->>AS: ForgotPasswordAsync(email)
AS->>UM: Generate password reset token
AS->>ES: SendPasswordResetLinkAsync(email, resetUrl)
AS-->>AC: Success
AC-->>C: 200 Check Your Email
C->>AC: POST /reset-password (email, token, newPassword)
AC->>AS: ResetPasswordAsync(dto)
AS->>UM: ResetPasswordAsync(user, token, newPassword)
AS-->>AC: Result
AC-->>C: 200/400
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (20)
AskFm/Shared/AppConstants.cs (1)
3-20: Make the container class static; optional: add environment scoping.
This is a pure utility; mark the class static to prevent instantiation. Consider adding an env/application prefix to avoid cross-environment key collisions.Apply:
-public class AppConstants +public static class AppConstants {AskFm/AskFm.BLL/AskFm.BLL.csproj (1)
17-17: Remove unused SendGrid PackageReference or wire up SendGridAskFm/AskFm.BLL/Services/EmailSender.cs contains only
using SendGrid/using SendGrid.Helpers.Mail(lines 10–11) but instantiatesSmtpClient(line 83) — SendGrid types aren’t used. Remove the PackageReference in AskFm/AskFm.BLL/AskFm.BLL.csproj (line 17) and delete the unused usings, or replace the SmtpClient implementation with a SendGrid client integration.AskFm/AskFm.API/appsettings.json (1)
12-15: Clarify expiration units and naming.
Verified: ExpireTimes:Jwt_Token_Exp is treated as minutes (JwtOptions.AccessExpiration; AuthService uses AddMinutes/TimeSpan.FromMinutes) and ExpireTimes:Refresh_Token_Exp is treated as days (JwtOptions.AccessRefreshTokenExpiration; AuthService uses AddDays). Recommend renaming keys to be explicit (e.g., AccessExpirationMinutes, RefreshExpirationDays) or documenting the units in the README.AskFm/AskFm.BLL/Services/IEmailSender.cs (2)
1-3: Remove unused using.
Microsoft.AspNetCore.Identityisn’t used here.-using Microsoft.AspNetCore.Identity;
6-12: Add CancellationToken and fix minor spacing.Async APIs should accept a CancellationToken; also remove extra spaces after
(.-public interface IEmailSender -{ - public Task<ServiceResult<bool>> SendPasswordResetLinkAsync( string email, string resetLink); - public Task<ServiceResult<bool>> SendConfirmationLinkAsync(string email, string confirmationLink); - public Task<ServiceResult<bool>> SendPasswordResetCodeAsync( string email, string resetCode); - Task<ServiceResult<bool>> SendEmailAsync (string email, string subject, string message); -} +public interface IEmailSender +{ + Task<ServiceResult<bool>> SendPasswordResetLinkAsync(string email, string resetLink, CancellationToken ct = default); + Task<ServiceResult<bool>> SendConfirmationLinkAsync(string email, string confirmationLink, CancellationToken ct = default); + Task<ServiceResult<bool>> SendPasswordResetCodeAsync(string email, string resetCode, CancellationToken ct = default); + Task<ServiceResult<bool>> SendEmailAsync(string email, string subject, string message, CancellationToken ct = default); +}Add this import if not present:
using System.Threading;AskFm/AskFm.BLL/Services/RedisCacheService.cs (1)
15-24: Add CancellationToken, guard inputs, and harden deserialization.Prevents silent hangs and crashes on corrupt cache entries; enables cooperative cancellation.
- public async Task SetCacheAsync<T>(string key, T value, TimeSpan expirationTime) + public async Task SetCacheAsync<T>(string key, T value, TimeSpan expirationTime, CancellationToken ct = default) { + if (string.IsNullOrWhiteSpace(key)) throw new ArgumentException("key cannot be null/empty", nameof(key)); var options = new DistributedCacheEntryOptions { AbsoluteExpirationRelativeToNow = expirationTime }; var jsonData = JsonSerializer.Serialize(value); - await _cache.SetStringAsync(key, jsonData, options); + await _cache.SetStringAsync(key, jsonData, options, ct); } - public async Task<T?> GetCacheAsync<T>(string key) + public async Task<T?> GetCacheAsync<T>(string key, CancellationToken ct = default) { - var jsonData = await _cache.GetStringAsync(key); - return jsonData is null ? default : JsonSerializer.Deserialize<T?>(jsonData); + if (string.IsNullOrWhiteSpace(key)) throw new ArgumentException("key cannot be null/empty", nameof(key)); + var jsonData = await _cache.GetStringAsync(key, ct); + if (string.IsNullOrEmpty(jsonData)) return default; + try + { + return JsonSerializer.Deserialize<T>(jsonData); + } + catch (JsonException) + { + return default; + } } - public async Task RemoveCacheAsync(string key) + public async Task RemoveCacheAsync(string key, CancellationToken ct = default) { - await _cache.RemoveAsync(key); + if (string.IsNullOrWhiteSpace(key)) return; + await _cache.RemoveAsync(key, ct); }Note: Update
IRedisServiceaccordingly (see its comment).Also applies to: 26-30, 32-35
AskFm/AskFm.BLL/DTO/UserDTOs/AuthResponseDTO.cs (1)
1-1: Remove unused using.
AskFm.DAL.Modelsisn’t referenced in this DTO anymore.-using AskFm.DAL.Models;AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs (3)
15-16: Async naming consistency.Rename to include
Asyncto match the rest of the interface.- Task<ServiceResult<bool>> UpdatePassword(int userId, UpdatePasswordDTO updatePasswordDto); - Task<ServiceResult<ReadUserDTO>> ResetEmail(int userId, string updatedEmail); + Task<ServiceResult<bool>> UpdatePasswordAsync(int userId, UpdatePasswordDTO updatePasswordDto); + Task<ServiceResult<ReadUserDTO>> ResetEmailAsync(int userId, string updatedEmail);
6-17: Consider CancellationToken across service methods.Add
CancellationToken ct = defaultto public async methods for consistency and cooperative cancellation.
21-24: Remove stray comment block or move to XML docs.Inline endpoint notes in an interface are noise.
AskFm/AskFm.BLL/DTO/UserDTOs/ForgotPasswordDto.cs (1)
7-9: Optional: bound email length.Prevents excessively large payloads; 256 is common for emails.
[Required] [EmailAddress] - public string Email { get; set; } + [StringLength(256)] + public string Email { get; set; }AskFm/AskFm.BLL/Services/IRedisService.cs (2)
1-3: Remove unused usings.These aren’t needed in the interface.
-using Microsoft.Extensions.Caching.Distributed; -using System.Text.Json;
6-13: Expose CancellationToken in the contract.Aligns with
RedisCacheServiceand enables cooperative cancellation.-public interface IRedisService -{ - public Task SetCacheAsync<T>(string key, T value, TimeSpan expirationTime); - - public Task<T?> GetCacheAsync<T>(string key); - - public Task RemoveCacheAsync(string key); -} +public interface IRedisService +{ + Task SetCacheAsync<T>(string key, T value, TimeSpan expirationTime, CancellationToken ct = default); + Task<T?> GetCacheAsync<T>(string key, CancellationToken ct = default); + Task RemoveCacheAsync(string key, CancellationToken ct = default); +}Add if not present:
using System.Threading;AskFm/AskFm.BLL/DTO/UserDTOs/ResetPasswordDto.cs (1)
14-19: Add password metadata annotations.Helpful for UI scaffolding and basic length constraints; server-side policy can still enforce complexity.
[Required] - public string NewPassword { get; set; } + [DataType(DataType.Password)] + [StringLength(100, MinimumLength = 8)] + public string NewPassword { get; set; } [Required] [Compare("NewPassword", ErrorMessage = "The new password and confirmation password do not match.")] - public string ConfirmPassword { get; set; } + [DataType(DataType.Password)] + public string ConfirmPassword { get; set; }AskFm/AskFm.BLL/Services/UserIdentityService/IAuthService.cs (1)
12-14: Rename Logout → LogoutAsync, add CancellationToken, and clarify overlap with RevokeRefreshTokenAsync.
- Change signatures: IAuthService.AuthService Logout → LogoutAsync(int userId, string refreshToken, CancellationToken ct = default) and update AuthService implementation and the call site (AskFm/AskFm.API/Controllers/AuthController.cs:97).
- Add optional CancellationToken to ForgotPasswordAsync and ResetPasswordAsync (interface + implementation) to allow cancellation.
- RevokeRefreshTokenAsync is a public API (IAuthService.cs:11 / AuthService.cs:151) and Logout currently calls it then revokes the JWT; either (a) propagate CancellationToken into RevokeRefreshTokenAsync and pass it from LogoutAsync, or (b) document the distinct semantics (RevokeRefreshTokenAsync = refresh-token-only; LogoutAsync = refresh + JWT) or consolidate to avoid overlapping public APIs.
AskFm/AskFm.BLL/Services/EmailSender.cs (2)
10-11: Remove unused SendGrid referencesThe SendGrid imports are not used in this implementation. Remove these unnecessary dependencies.
-using SendGrid; -using SendGrid.Helpers.Mail;
83-87: SmtpClient should be disposedSmtpClient implements IDisposable and should be properly disposed to release network resources.
The fix is included in the error handling suggestion above with the
usingstatement.AskFm/AskFm.API/Controllers/AuthController.cs (1)
127-130: Redundant null checkThe null check on line 127 is redundant since model validation should handle this through data annotations.
- if (resetPasswordDto == null) - { - return BadRequest("Invalid Data"); - } - var result = await _authService.ResetPasswordAsync(resetPasswordDto);AskFm/AskFm.API/Program.cs (1)
140-144: Consider more specific error message for revoked tokensThe error message "Token revoked or expired" could be split to distinguish between revocation and expiration for better debugging.
var cachedToken = await redis.GetCacheAsync<int>(AppConstants.JwtCacheKey(jti)); if (cachedToken <= 0) { - context.Fail("Token revoked or expired"); + context.Fail("Token has been revoked"); }AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs (1)
280-282: ExpireAfter should use days, not the raw config valueThe ExpireAfter field appears to represent days but is being set to the raw configuration value which might be confusing.
Consider renaming the configuration key or the property for clarity:
- ExpireAfter = _configuration.GetValue<int>("ExpireTimes:Refresh_Token_Exp") + ExpireAfter = _configuration.GetValue<int>("ExpireTimes:Refresh_Token_Days")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
AskFm/AskFm.API/Controllers/AuthController.cs(4 hunks)AskFm/AskFm.API/Program.cs(6 hunks)AskFm/AskFm.API/appsettings.json(1 hunks)AskFm/AskFm.BLL/AskFm.BLL.csproj(1 hunks)AskFm/AskFm.BLL/DTO/EmailSettings.cs(1 hunks)AskFm/AskFm.BLL/DTO/RefreshTokenDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/UserDTOs/AuthResponseDTO.cs(1 hunks)AskFm/AskFm.BLL/DTO/UserDTOs/ForgotPasswordDto.cs(1 hunks)AskFm/AskFm.BLL/DTO/UserDTOs/ResetPasswordDto.cs(1 hunks)AskFm/AskFm.BLL/Services/EmailSender.cs(1 hunks)AskFm/AskFm.BLL/Services/IEmailSender.cs(1 hunks)AskFm/AskFm.BLL/Services/IRedisService.cs(1 hunks)AskFm/AskFm.BLL/Services/RedisCacheService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs(8 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/IAuthService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/IUserService.cs(1 hunks)AskFm/AskFm.BLL/Services/UserIdentityService/JwtOptions.cs(1 hunks)AskFm/AskFm.DAL/Models/ApplicationUser.cs(1 hunks)AskFm/AskFm.DAL/Models/RefreshToken.cs(1 hunks)AskFm/Shared/AppConstants.cs(1 hunks)
🔇 Additional comments (13)
AskFm/AskFm.BLL/Services/UserIdentityService/JwtOptions.cs (1)
5-7: Enforce required JwtOptions (Issuer/Audience/SigningKey) and enable options validationFile: AskFm/AskFm.BLL/Services/UserIdentityService/JwtOptions.cs (lines 5-7)
Make Issuer/Audience/SigningKey non-nullable and validated to avoid broken token generation/validation.+using System.ComponentModel.DataAnnotations; public class JwtOptions { - public string? Issuer { get; set; } - public string? Audience { get; set; } - public string? SigningKey { get; set; } + [Required, MinLength(1)] + public string Issuer { get; set; } = default!; + [Required, MinLength(1)] + public string Audience { get; set; } = default!; + [Required, MinLength(32)] + public string SigningKey { get; set; } = default!; public int AccessExpiration { get; set; } // Minutes public int AccessRefreshTokenExpiration { get; set; } // days }Ensure options validation is enabled in DI:
services.AddOptions().Bind(config.GetSection("JwtOptions")).ValidateDataAnnotations().ValidateOnStart();Verify the DI registration above exists in Program/Startup and that configuration values are present.
AskFm/AskFm.BLL/DTO/UserDTOs/ForgotPasswordDto.cs (1)
7-9: LGTM.Validation attributes are appropriate.
AskFm/AskFm.API/Controllers/AuthController.cs (3)
62-63: Good improvement: Changed refresh-token from GET to POSTChanging the refresh token endpoint from GET to POST is a security best practice, as it prevents token leakage through URL logging.
86-90: User ID validation prevents privilege escalationGood security practice - validating that the path parameter matches the authenticated user prevents users from logging out other users' sessions.
113-119: Generic success message prevents user enumerationGood security practice - returning "Check Your Email" regardless of whether the email exists prevents attackers from discovering valid email addresses.
AskFm/AskFm.API/Program.cs (4)
95-96: Token expiration times moved to configurationGood practice - moving token expiration times from environment variables to configuration makes them easier to manage.
Also applies to: 108-109
133-147: JWT revocation check implementationExcellent implementation of JWT revocation using Redis. The check for revoked tokens in OnTokenValidated prevents the use of invalidated JWTs.
184-190: Redis configuration uses connection stringGood - using a connection string for Redis configuration is more flexible than hardcoding host/port.
111-111: Password reset token lifespan configuredSetting DataProtectionTokenProviderOptions.TokenLifespan to 2 hours is reasonable for password reset tokens.
AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs (4)
239-240: JWT tracking in Redis enables single-device loginThe implementation stores only one JWT per user in Redis, effectively implementing single-device login. This is a valid design choice but should be documented.
249-250: Refresh token cache expiration aligns with token lifetimeGood practice - setting the Redis cache expiration to match the refresh token's actual expiration ensures consistency.
178-187: Logout implementation properly revokes both tokensThe logout flow correctly revokes both the refresh token and JWT, ensuring complete session termination.
308-315: RevokeJwtToken helper properly cleans up Redis entriesThe helper method correctly removes both the JWT mapping and user's current JWT reference from Redis.
| "EmailOption": { | ||
| "client": "smtp.gmail.com", | ||
| "password": "password", | ||
| "from": "email", | ||
| "port":567 | ||
| }, |
There was a problem hiding this comment.
SMTP port is likely wrong; fix 567 → 587 (or 465 for SSL). Also avoid storing plaintext passwords.
Port 567 will fail for Gmail. Use 587 (STARTTLS) or 465 (SSL) and move EmailOption.password to environment variables or user-secrets.
Apply:
"EmailOption": {
"client": "smtp.gmail.com",
- "password": "password",
+ "password": "",
"from": "email",
- "port":567
+ "port": 587
},Consider removing EmailOption from source-controlled appsettings.json entirely and sourcing via appsettings.Development.json + secrets.
📝 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.
| "EmailOption": { | |
| "client": "smtp.gmail.com", | |
| "password": "password", | |
| "from": "email", | |
| "port":567 | |
| }, | |
| "EmailOption": { | |
| "client": "smtp.gmail.com", | |
| "password": "", | |
| "from": "email", | |
| "port": 587 | |
| }, |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/appsettings.json around lines 16 to 21 the SMTP port is
incorrect (567) and the password is stored in plaintext; change the port to 587
(or 465 if you intend to use SSL) and remove the plaintext password from this
source-controlled file, instead reading EmailOption.password from environment
variables or user-secrets (or move the whole EmailOption into
appsettings.Development.json and protect secrets with dotnet user-secrets); also
update the configuration code to prefer environment/user-secret values for the
email password and host/port so no sensitive credentials remain in source
control.
| "from": "email", | ||
| "port":567 | ||
| }, | ||
| "ClientUrYour App Namel": " http://localhost:5180", |
There was a problem hiding this comment.
Fix typo and leading space in client URL key/value.
"ClientUrYour App Namel" won’t bind; likely intended "ClientUrl". Also remove the leading space in the URL.
Apply:
-"ClientUrYour App Namel": " http://localhost:5180",
+"ClientUrl": "http://localhost:5180",📝 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.
| "ClientUrYour App Namel": " http://localhost:5180", | |
| "ClientUrl": "http://localhost:5180", |
🤖 Prompt for AI Agents
In AskFm/AskFm.API/appsettings.json around line 22, the JSON key and value are
mistyped and have a leading space; change the key "ClientUrYour App Namel" to
the intended "ClientUrl" (or the exact config key your app expects) and remove
the leading space from the URL value so it reads "http://localhost:5180" (ensure
quotes and JSON formatting remain valid).
| public async Task<IActionResult> ForgotPassword(ForgotPasswordDto forgotPasswordDto) | ||
| { | ||
|
|
||
| var result = await _authService.ForgotPasswordAsync(forgotPasswordDto.Email); | ||
| if (!result.success) | ||
| { | ||
| return BadRequest(result.Errors); | ||
| } | ||
|
|
||
| return Ok("Check Your Email"); | ||
| } |
There was a problem hiding this comment.
Password reset endpoint lacks rate limiting
The forgot-password endpoint is vulnerable to abuse without rate limiting. Attackers could spam users with reset emails.
Consider implementing rate limiting at the API level or tracking reset attempts per email address in Redis with a time window.
🤖 Prompt for AI Agents
In AskFm/AskFm.API/Controllers/AuthController.cs around lines 110 to 120, the
ForgotPassword action currently calls _authService.ForgotPasswordAsync without
any throttling; add rate limiting to prevent abuse by either (1) applying
API-level rate limit middleware (e.g., AspNetCoreRateLimit or built-in endpoint
rate limiting) for this route, or (2) tracking per-email reset attempts in
Redis/CDS: inject IDistributedCache or a Redis client, increment a counter keyed
by normalized email with a TTL (e.g., 1 hour), return 429 TooManyRequests when
the count exceeds the threshold, and only call _authService.ForgotPasswordAsync
when under limit; also keep the endpoint response non-enumerative (always return
a generic success message) and log rate-limit events for monitoring.
| public class EmailSettings | ||
| { | ||
| public string From {get; set;} | ||
| public string Client {get; set;} | ||
| public string Password {get;set;} | ||
| public int Port {get; set; } | ||
|
|
||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add validation attributes; ensure the Port from config is actually used by EmailSender.
Guard against empty From/Client/Password and invalid Port.
Apply:
+using System.ComponentModel.DataAnnotations;
public class EmailSettings
{
- public string From {get; set;}
- public string Client {get; set;}
- public string Password {get;set;}
- public int Port {get; set; }
+ [Required, EmailAddress]
+ public string From { get; set; } = default!;
+ [Required]
+ public string Client { get; set; } = default!;
+ [Required]
+ public string Password { get; set; } = default!;
+ [Range(1, 65535)]
+ public int Port { get; set; }
}🏁 Script executed:
#!/bin/bash
# Verify EmailSender uses settings.Port and not a hard-coded 587
rg -n -C2 --type cs 'new\s+SmtpClient\(|\.Port\s*=' AskFm | sed -n '1,120p'Length of output: 324
Use configured SMTP port and add validation to EmailSettings
- Replace hard-coded 587 in AskFm/AskFm.BLL/Services/EmailSender.cs (line 83):
var client = new SmtpClient(_emailSettings.Client, 587) -> use _emailSettings.Port (e.g., new SmtpClient(_emailSettings.Client, _emailSettings.Port)). - Apply DataAnnotations to AskFm/AskFm.BLL/DTO/EmailSettings.cs. Apply the following diff:
+using System.ComponentModel.DataAnnotations;
public class EmailSettings
{
- public string From {get; set;}
- public string Client {get; set;}
- public string Password {get;set;}
- public int Port {get; set; }
+ [Required, EmailAddress]
+ public string From { get; set; } = default!;
+ [Required]
+ public string Client { get; set; } = default!;
+ [Required]
+ public string Password { get; set; } = default!;
+ [Range(1, 65535)]
+ public int Port { get; set; }
}📝 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 EmailSettings | |
| { | |
| public string From {get; set;} | |
| public string Client {get; set;} | |
| public string Password {get;set;} | |
| public int Port {get; set; } | |
| } | |
| using System.ComponentModel.DataAnnotations; | |
| public class EmailSettings | |
| { | |
| [Required, EmailAddress] | |
| public string From { get; set; } = default!; | |
| [Required] | |
| public string Client { get; set; } = default!; | |
| [Required] | |
| public string Password { get; set; } = default!; | |
| [Range(1, 65535)] | |
| public int Port { get; set; } | |
| } |
| using Microsoft.EntityFrameworkCore; | ||
|
|
||
| namespace AskFm.BLL.DTO; | ||
| public class RefreshTokenDto | ||
| { | ||
| public string Token { get; set; } | ||
| public DateTime ExpireOn { get; set; } | ||
| public bool IsExpired => DateTime.Now >= ExpireOn; | ||
| public int ExpireAfter { get; set; } | ||
| public DateTime CreatedOn { get; set; } | ||
| } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Remove unused EF using; prefer UTC for expiry checks; consider making the DTO immutable.
Avoid EF dependency in DTOs. Use UtcNow for consistency and serialization safety.
Apply:
-using Microsoft.EntityFrameworkCore;
-
namespace AskFm.BLL.DTO;
public class RefreshTokenDto
{
public string Token { get; set; }
public DateTime ExpireOn { get; set; }
- public bool IsExpired => DateTime.Now >= ExpireOn;
+ public bool IsExpired => DateTime.UtcNow >= ExpireOn;
public int ExpireAfter { get; set; }
public DateTime CreatedOn { get; set; }
}🏁 Script executed:
#!/bin/bash
# Ensure all token timestamps are generated and stored in UTC
rg -n -C2 --type cs 'DateTime\.Now|DateTimeOffset\.Now|DateTime\.UtcNow' AskFm | sed -n '1,200p'Length of output: 5709
Remove unused EF using; prefer UTC for expiry checks; unify timestamps.
Remove Microsoft.EntityFrameworkCore from RefreshTokenDto and change IsExpired to use DateTime.UtcNow; replace other DateTime.Now usages with UtcNow (or document intentional local-time use).
-using Microsoft.EntityFrameworkCore;
-
namespace AskFm.BLL.DTO;
public class RefreshTokenDto
{
public string Token { get; set; }
public DateTime ExpireOn { get; set; }
- public bool IsExpired => DateTime.Now >= ExpireOn;
+ public bool IsExpired => DateTime.UtcNow >= ExpireOn;
public int ExpireAfter { get; set; }
public DateTime CreatedOn { get; set; }
}-
Fix other DateTime.Now occurrences found by the search:
- AskFm/AskFm.BLL/Services/CommentLikeService.cs:122 — existingLike.CreatedAt = DateTime.Now -> DateTime.UtcNow
- AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs:173 — appUser.LastSeen = DateTime.Now -> DateTime.UtcNow
- AskFm/AskFm.API/Controllers/WeatherForecastController.cs:26 — DateOnly.FromDateTime(DateTime.Now.AddDays(index)) — confirm whether local time is intended; otherwise use UTC-based conversion.
-
Optional: make RefreshTokenDto immutable (readonly properties + ctor).
📝 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.
| using Microsoft.EntityFrameworkCore; | |
| namespace AskFm.BLL.DTO; | |
| public class RefreshTokenDto | |
| { | |
| public string Token { get; set; } | |
| public DateTime ExpireOn { get; set; } | |
| public bool IsExpired => DateTime.Now >= ExpireOn; | |
| public int ExpireAfter { get; set; } | |
| public DateTime CreatedOn { get; set; } | |
| } | |
| namespace AskFm.BLL.DTO; | |
| public class RefreshTokenDto | |
| { | |
| public string Token { get; set; } | |
| public DateTime ExpireOn { get; set; } | |
| public bool IsExpired => DateTime.UtcNow >= ExpireOn; | |
| public int ExpireAfter { get; set; } | |
| public DateTime CreatedOn { get; set; } | |
| } |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/DTO/RefreshTokenDto.cs lines 1-11, remove the unused "using
Microsoft.EntityFrameworkCore;" and change the IsExpired calc to use
DateTime.UtcNow instead of DateTime.Now; also update other DateTime.Now
occurrences noted by the reviewer: in
AskFm/AskFm.BLL/Services/CommentLikeService.cs line 122 set
existingLike.CreatedAt = DateTime.UtcNow, in
AskFm/AskFm.BLL/Services/UserIdentityService/UserService.cs line 173 set
appUser.LastSeen = DateTime.UtcNow, and review
AskFm/AskFm.API/Controllers/WeatherForecastController.cs line 26 to use a
UTC-based conversion (e.g.,
DateOnly.FromDateTime(DateTime.UtcNow.AddDays(index)) if local time is not
required); optionally consider making RefreshTokenDto immutable by converting
properties to readonly and adding a constructor.
| var resetUrl = | ||
| $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={email}&token={token}"; | ||
|
|
There was a problem hiding this comment.
Reset URL should send the actual link in email body
The reset URL is being sent as the email body directly instead of being used in the email template. The EmailSender has a SendPasswordResetLinkAsync method that should be used.
- var resetUrl =
- $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={email}&token={token}";
-
- _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl);
+ var resetUrl =
+ $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={Uri.EscapeDataString(email)}&token={Uri.EscapeDataString(token)}";
+
+ await _emailSender.SendPasswordResetLinkAsync(email, resetUrl);📝 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.
| var resetUrl = | |
| $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={email}&token={token}"; | |
| var resetUrl = | |
| $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={Uri.EscapeDataString(email)}&token={Uri.EscapeDataString(token)}"; | |
| await _emailSender.SendPasswordResetLinkAsync(email, resetUrl); |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs around lines
198-200, the code builds a resetUrl and sends it as the raw email body; instead
call the EmailSender's SendPasswordResetLinkAsync method so the link is placed
into the proper email template. Replace the manual send with a call to
SendPasswordResetLinkAsync (passing the recipient email and either the token or
the fully-built url per the method signature), ensure query parameters are
URL-encoded (email and token) when constructing the url, and remove the direct
SendEmailAsync usage so the template-based sender is used.
| } | ||
| var token = await _userManager.GeneratePasswordResetTokenAsync(user); | ||
| var resetUrl = | ||
| $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={email}&token={token}"; |
There was a problem hiding this comment.
URL parameters need encoding
Email and token parameters in the reset URL should be URL-encoded to handle special characters.
The fix is included in the suggestion above using Uri.EscapeDataString.
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs around line 199,
the reset-password URL is built without encoding the email and token query
parameters; update the code to URL-encode both email and token (e.g., wrap each
with Uri.EscapeDataString or equivalent) before concatenating into the query
string so special characters are safely transmitted.
| var resetUrl = | ||
| $"{_configuration.GetValue<string>("ClientUrl")}/app/Auth/reset-password?email={email}&token={token}"; | ||
|
|
||
| _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl); |
There was a problem hiding this comment.
Missing await on email sending
The SendEmailAsync call is not awaited, which means the method will complete before the email is actually sent, and any exceptions will be lost.
- _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl);
+ await _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl);📝 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.
| _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl); | |
| await _emailSender.SendEmailAsync(email, "AskFm: Reset Password", resetUrl); |
🤖 Prompt for AI Agents
In AskFm/AskFm.BLL/Services/UserIdentityService/AuthService.cs around line 201,
the call to _emailSender.SendEmailAsync(...) is not awaited; update the method
to await the async call (await _emailSender.SendEmailAsync(...)) so the email
send completes before the method returns, ensure the enclosing method signature
is async and returns Task or Task<T> as appropriate, and propagate or handle
exceptions from the awaited call (optionally use ConfigureAwait(false) if
context capture should be avoided).
|
|
||
| // tokens | ||
| public virtual ICollection<RefreshToken>? RefreshTokens { get; set; } | ||
| // public virtual ICollection<RefreshToken>? RefreshTokens { get; set; } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removing RefreshTokens nav requires DB migration; confirm none of the code still expects it.
If EF previously owned this collection, generate a migration to drop the owned type/table. Also verify no references remain.
Consider fully removing related config instead of leaving commented code.
🏁 Script executed:
#!/bin/bash
rg -n '\bRefreshTokens\b'Length of output: 1291
Generate a migration to remove the RefreshTokens owned collection and delete remaining references.
Migrations and the model snapshot still include ownsMany("AskFm.DAL.Models.RefreshToken", "RefreshTokens"); add a migration to drop the owned type/table (or restore the navigation if you intend to keep it) and remove the commented nav/config.
Locations:
- AskFm/AskFm.DAL/Models/ApplicationUser.cs:34 (commented nav).
- AskFm/AskFm.DAL/Migrations/20250824164944_AddRefreshTokenTable.Designer.cs (ownsMany / navigation).
- AskFm/AskFm.DAL/Migrations/20250824210328_AddColumnToRefreshTokenTable.Designer.cs (ownsMany / navigation).
- AskFm/AskFm.DAL/Migrations/20250824224832_FixIsActiveColumn_Follow.Designer.cs (ownsMany / navigation).
- AskFm/AskFm.DAL/Migrations/AppDbContextModelSnapshot.cs (ownsMany / navigation).
| // using Microsoft.EntityFrameworkCore; | ||
| // | ||
| // namespace AskFm.DAL.Models; | ||
| // // [Owned] | ||
| // public class RefreshToken | ||
| // { | ||
| // public string Token { get; set; } | ||
| // public DateTime ExpireOn { get; set; } | ||
| // public bool IsExpired => DateTime.Now >= ExpireOn; | ||
| // public DateTime? RevokedOn { get; set; } | ||
| // public bool IsActive => RevokedOn == null && !IsExpired; | ||
| // | ||
| // public DateTime CreatedOn { get; set; } | ||
| // } No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Delete dead code file instead of commenting it out; verify migrations.
Keeping a fully commented entity is confusing and may mislead EF model readers. Remove the file and ensure a migration reflects the removal.
🏁 Script executed:
#!/bin/bash
rg -n '\bRefreshToken\b'
rg -n 'Owned\(|\[Owned\]' || true
# Inspect pending migrations referencing RefreshToken
rg -n --glob '!**/bin/**' --glob '!**/obj/**' '(HasMany|OwnsMany|OwnsOne).*RefreshToken' || trueLength of output: 2769
Restore or remove RefreshToken consistently — do not leave the entity commented out.
Migrations and the model snapshot still reference AskFm.DAL.Models.RefreshToken; the commented-out class will cause compile/migration inconsistencies.
- Keep it: un-comment AskFm.DAL/Models/RefreshToken.cs and ApplicationUser.RefreshTokens so EF mappings compile.
- Remove it: delete the model, remove OwnsMany/ToTable and other references (DbContext, DTOs, controllers) and add a migration to drop the RefreshToken table and update the model snapshot.
Relevant files: AskFm/AskFm.DAL/Models/RefreshToken.cs, AskFm/AskFm.DAL/Models/ApplicationUser.cs, AskFm/AskFm.DAL/Migrations/20250824164944_AddRefreshTokenTable.cs, AskFm/AskFm.DAL/Migrations/20250824210328_AddColumnToRefreshTokenTable.cs, AskFm/AskFm.DAL/Migrations/20250824224832_FixIsActiveColumn_Follow.Designer.cs, AskFm/AskFm.DAL/Migrations/AppDbContextModelSnapshot.cs (and related Designer.cs migration files).
🤖 Prompt for AI Agents
In AskFm/AskFm.DAL/Models/RefreshToken.cs lines 1-14: the RefreshToken entity
was left commented-out causing EF model/migration mismatch; either restore it or
fully remove all usages. To keep it: un-comment the RefreshToken class and
ensure ApplicationUser.RefreshTokens is un-commented, restore any
[Owned]/mapping attributes and DbContext OwnsMany/ToTable configuration, rebuild
and add a new migration so the snapshot matches the model. To remove it: delete
RefreshToken.cs, remove all OwnsMany/ToTable and property references in
ApplicationUser, DbContext, DTOs and controllers, then create a migration that
drops the RefreshToken table and update the model snapshot and Designer files
before committing.
1 participant
Summary by CodeRabbit
New Features
Refactor