Conversation
Co-authored-by: marcominerva <3522534+marcominerva@users.noreply.github.com>
…property Co-authored-by: marcominerva <3522534+marcominerva@users.noreply.github.com>
…ce-public Make JwtBearerService public with virtual methods for extensibility
- Throw ArgumentNullException if JwtBearerSettings options are null in JwtBearerService constructor. - Always enable token lifetime validation in JWT Bearer authentication, regardless of ExpirationTime setting.
Bump Swashbuckle.AspNetCore.SwaggerUI and SwaggerGen from 10.1.5 to 10.1.7 across all projects. Update Swashbuckle.AspNetCore in Net8JwtBearerSample to 10.1.7. Also update Microsoft.AspNetCore.Authentication.JwtBearer and Microsoft.AspNetCore.OpenApi for .NET 10.0 to their latest patch versions. No other changes included.
Add a check to ensure absoluteExpiration is not earlier than the current UTC time when creating a JWT token. Throw an ArgumentException if an invalid expiration is provided to prevent issuing already-expired tokens.
There was a problem hiding this comment.
Pull request overview
This PR updates several ASP.NET Core / Swashbuckle NuGet dependencies across the library and samples, and makes JwtBearerService a public, more extensible implementation of IJwtBearerService with updated documentation and settings encapsulation.
Changes:
- Bump
Microsoft.AspNetCore.OpenApi,Microsoft.AspNetCore.Authentication.JwtBearer, and Swashbuckle package versions across projects/samples. - Refactor
JwtBearerServiceto bepublic, add XML docs, and make key methodsvirtual. - Change JWT bearer middleware configuration to always validate token lifetime (
ValidateLifetime = true).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/SimpleAuthentication/SimpleAuthenticationExtensions.cs | Always enables JWT lifetime validation in the middleware setup. |
| src/SimpleAuthentication/SimpleAuthentication.csproj | Updates Microsoft.AspNetCore.OpenApi package version. |
| src/SimpleAuthentication/JwtBearer/JwtBearerService.cs | Makes service public/virtual, centralizes settings access, adds validation/docs. |
| src/SimpleAuthentication.Swashbuckle/SimpleAuthentication.Swashbuckle.csproj | Updates Swashbuckle.AspNetCore.SwaggerGen version. |
| src/SimpleAuthentication.Abstractions/SimpleAuthentication.Abstractions.csproj | Updates Microsoft.AspNetCore.Authentication.JwtBearer version. |
| samples/MinimalApis/Net8JwtBearerSample/Net8JwtBearerSample.csproj | Updates Swashbuckle dependency in minimal API sample. |
| samples/MinimalApis/JwtBearerSample/JwtBearerSample.csproj | Updates OpenAPI + SwaggerUI dependencies in minimal API sample. |
| samples/MinimalApis/BasicAuthenticationSample/BasicAuthenticationSample.csproj | Updates OpenAPI + SwaggerUI dependencies in minimal API sample. |
| samples/MinimalApis/ApiKeySample/ApiKeySample.csproj | Updates OpenAPI + SwaggerUI dependencies in minimal API sample. |
| samples/Controllers/JwtBearerSample/JwtBearerSample.csproj | Updates OpenAPI + SwaggerUI dependencies in controllers sample. |
| samples/Controllers/BasicAuthenticationSample/BasicAuthenticationSample.csproj | Updates OpenAPI + SwaggerUI dependencies in controllers sample. |
| samples/Controllers/ApiKeySample/ApiKeySample.csproj | Updates OpenAPI + SwaggerUI dependencies in controllers sample. |
| throw new ArgumentException("The expiration date must be greater than or equal to the current date and time.", nameof(absoluteExpiration)); | ||
| } | ||
|
|
||
| claims ??= []; |
There was a problem hiding this comment.
claims ??= [] will most likely materialize as an array assigned to IList<Claim>. Arrays implement IList<T> but are fixed-size, so subsequent calls to claims.Update(...) (which uses Add/Remove) can throw NotSupportedException when claims is initially null. Use a mutable collection (e.g., initialize to a List<Claim>) before mutating the list.
| claims ??= []; | |
| claims = claims switch | |
| { | |
| null => new global::System.Collections.Generic.List<Claim>(), | |
| { IsReadOnly: true } => new global::System.Collections.Generic.List<Claim>(claims), | |
| _ => claims | |
| }; |
| protected JwtBearerSettings JwtBearerSettings { get; } = jwtBearerSettingsOptions?.Value ?? throw new ArgumentNullException(nameof(jwtBearerSettingsOptions)); | ||
|
|
There was a problem hiding this comment.
JwtBearerSettings initialization uses jwtBearerSettingsOptions?.Value ?? throw new ArgumentNullException(nameof(jwtBearerSettingsOptions)), which throws an ArgumentNullException for the options parameter even when jwtBearerSettingsOptions is non-null but .Value is null. Consider validating jwtBearerSettingsOptions explicitly (per project style) and failing with a more accurate exception if the bound settings instance is null.
| protected JwtBearerSettings JwtBearerSettings { get; } = jwtBearerSettingsOptions?.Value ?? throw new ArgumentNullException(nameof(jwtBearerSettingsOptions)); | |
| protected JwtBearerSettings JwtBearerSettings { get; } = GetJwtBearerSettings(jwtBearerSettingsOptions); | |
| private static JwtBearerSettings GetJwtBearerSettings(IOptions<JwtBearerSettings> jwtBearerSettingsOptions) | |
| { | |
| ArgumentNullException.ThrowIfNull(jwtBearerSettingsOptions); | |
| return jwtBearerSettingsOptions.Value ?? throw new InvalidOperationException($"{nameof(JwtBearerSettings)} options must be configured."); | |
| } |
| var now = DateTime.UtcNow; | ||
|
|
||
| if (absoluteExpiration.HasValue && absoluteExpiration.Value < now) | ||
| { | ||
| throw new ArgumentException("The expiration date must be greater than or equal to the current date and time.", nameof(absoluteExpiration)); | ||
| } |
There was a problem hiding this comment.
The absoluteExpiration validation compares a user-provided DateTime against DateTime.UtcNow without normalizing time zones. If callers pass a DateTime with Kind = Local/Unspecified, the comparison can be wrong (since DateTime comparisons don't convert between kinds). Consider requiring UTC explicitly (e.g., validate Kind and/or convert to UTC) and clarifying the exception message accordingly.
| var claims = (principal.Identity as ClaimsIdentity)!.Claims.ToList(); | ||
|
|
||
| var userName = claims.First(c => c.Type == jwtBearerSettings.NameClaimType).Value; | ||
| var userName = claims.First(c => c.Type == JwtBearerSettings.NameClaimType).Value; |
There was a problem hiding this comment.
RefreshTokenAsync assumes the validated token always contains a claim with Type == JwtBearerSettings.NameClaimType and uses First(...), which will throw InvalidOperationException if the claim is missing. Since ValidateTokenAsync can succeed even when that claim isn't present, consider handling the missing-claim case explicitly and throwing a more appropriate exception (e.g., SecurityTokenException/ArgumentException) with a clear message.
| var userName = claims.First(c => c.Type == JwtBearerSettings.NameClaimType).Value; | |
| var nameClaim = claims.FirstOrDefault(c => c.Type == JwtBearerSettings.NameClaimType); | |
| if (string.IsNullOrEmpty(nameClaim?.Value)) | |
| { | |
| throw new SecurityTokenException($"Token does not contain the required '{JwtBearerSettings.NameClaimType}' claim."); | |
| } | |
| var userName = nameClaim.Value; |
This pull request primarily upgrades several NuGet package dependencies across the solution and refactors the
JwtBearerServiceclass to improve clarity, extensibility, and documentation. The changes focus on keeping dependencies up to date and making the JWT Bearer service implementation more robust and maintainable.Dependency Updates:
Microsoft.AspNetCore.OpenApito version10.0.5andSwashbuckle.AspNetCore.SwaggerUIto10.1.7in all sample project files under bothControllersandMinimalApisfolders. [1] [2] [3]Swashbuckle.AspNetCoreandSwashbuckle.AspNetCore.SwaggerGento version10.1.7in relevant sample and library projects. [1] [2]Microsoft.AspNetCore.Authentication.JwtBearerto version10.0.5inSimpleAuthentication.Abstractions.JwtBearerService Refactoring and Improvements:
JwtBearerServicefrominternaltopublic, added XML documentation, and made key methods (CreateTokenAsync,ValidateTokenAsync,RefreshTokenAsync)virtualfor easier extension and testing. [1] [2] [3]jwtBearerSettingsfield with the protected propertyJwtBearerSettingsthroughout the class for consistency and improved encapsulation. Added null checks and argument validation. [1] [2] [3]Behavioral Change:
ValidateLifetime = true) instead of conditionally based on expiration time settings.