bit Boilerplate must always store auth method in jwt token (#11939) #11940
Conversation
WalkthroughThe changes introduce a METHOD claim to track authentication methods (External, SMS, Email, Password, etc.) throughout the authentication and token lifecycle. The METHOD claim is stored during sign-in, propagated during token refresh, and used to conditionally gate Keycloak claim retrieval. The previous amr claim-based TFA validation is replaced with this new approach. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SignIn as SignIn Flow
participant Context as HttpContext.Items
participant ClaimsFactory as AppUserClaimsPrincipalFactory
participant JWT as JWT Token
User->>SignIn: Authenticate with method (External/SMS/Password)
SignIn->>Context: Store METHOD claim
SignIn->>SignIn: SignInAsync (no amr claim)
rect rgb(200, 220, 240)
Note over ClaimsFactory: Claims Generation
ClaimsFactory->>Context: Read METHOD from Items
ClaimsFactory->>ClaimsFactory: Add METHOD claim to principal
alt METHOD == "External"
ClaimsFactory->>ClaimsFactory: Retrieve & merge Keycloak claims
else Other methods
ClaimsFactory->>ClaimsFactory: Skip Keycloak integration
end
end
ClaimsFactory->>JWT: Build token with METHOD claim
JWT->>User: Return token with auth method preserved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (1)
28-32: Consider defensive null handling forHttpContext.Line 30 uses the null-forgiving operator on
HttpContext. While this code path runs during HTTP request processing whereHttpContextshould always exist, using?.with a fallback would be more defensive:🔎 Defensive null handling
- string? authenticationMethod = httpContextAccessor.HttpContext!.Items[AppClaimTypes.METHOD]?.ToString() ?? "Password"; + string authenticationMethod = httpContextAccessor.HttpContext?.Items[AppClaimTypes.METHOD]?.ToString() ?? "Password";
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/SignInManagerExtensions.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ISharedServiceCollectionExtensions.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppClaimTypes.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AuthPolicies.cs
💤 Files with no reviewable changes (2)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AuthPolicies.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ISharedServiceCollectionExtensions.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/SignInManagerExtensions.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppClaimTypes.cs (1)
AppClaimTypes(4-37)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppClaimTypes.cs (1)
AppClaimTypes(4-37)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppClaimTypes.cs (1)
AppClaimTypes(4-37)
⏰ 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). (3)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
- GitHub Check: build Bit.Templates
🔇 Additional comments (5)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Extensions/SignInManagerExtensions.cs (2)
68-69: LGTM - Auth method stored for downstream claim generation.The authentication method is correctly stored in the HTTP context after successful OTP verification. By this point,
authenticationMethodis guaranteed non-null since we've passed thetokenIsValid is falseguard at line 62.
91-91: Verify the removal of explicit claim assignment.The change from
SignInWithClaimsAsync(withamrclaim) toSignInAsyncis consistent with the new design whereAppUserClaimsPrincipalFactoryhandles METHOD claim generation. Confirm this aligns with the removal ofamr-based TFA validation mentioned in the PR summary.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppClaimTypes.cs (1)
31-36: LGTM - Well-documented claim constant.The METHOD claim constant is properly defined with comprehensive documentation covering all supported authentication methods.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (2)
45-47: Keycloak claim retrieval guard is well-designed.The early return for non-external authentication prevents unnecessary Keycloak token operations for password/OTP-based sign-ins. This correctly limits Keycloak claim merging to users who authenticated via external providers.
9-10: Constructor change looks good.The addition of
IHttpContextAccessorenables access to the authentication method stored inHttpContext.Itemsduring claim generation.
| HttpContext.Items[AppClaimTypes.METHOD] = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD); | ||
|
|
There was a problem hiding this comment.
Backward compatibility: Existing external auth tokens will lose Keycloak claims after refresh.
When refreshing tokens issued before this change (lacking the METHOD claim), GetClaimValue<string?> returns null, causing AppUserClaimsPrincipalFactory to default to "Password". This breaks RetrieveKeycloakClaims which early-exits unless METHOD is "External".
Existing Keycloak/external users will lose their Keycloak-derived claims after their first token refresh post-deployment.
Consider deriving the method from stored Keycloak tokens as a fallback:
🔎 Suggested fallback approach
- HttpContext.Items[AppClaimTypes.METHOD] = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD);
+ var method = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD);
+ // Fallback for tokens issued before METHOD claim was introduced
+ if (string.IsNullOrEmpty(method))
+ {
+ // Check if user has Keycloak tokens stored, implying external auth
+ var keycloakToken = await userManager.GetAuthenticationTokenAsync(userSession.User!, "Keycloak", "refresh_token");
+ method = string.IsNullOrEmpty(keycloakToken) ? "Password" : "External";
+ }
+ HttpContext.Items[AppClaimTypes.METHOD] = method;Note: This fix would need to be applied after line 254 where user is available, or refactored accordingly.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Pull request overview
This PR refactors authentication method tracking in JWT tokens by replacing the ASP.NET Core Identity standard "amr" (Authentication Methods Reference) claim with a custom METHOD claim. The change removes the TFA_ENABLED policy that relied on the "amr" claim and ensures all authentication methods (Password, Email, SMS, Push, External, WebAuthn) are consistently tracked throughout the authentication lifecycle.
Key Changes
- Introduced a new custom claim type
AppClaimTypes.METHODto store the authentication method used - Removed the
TFA_ENABLEDauthorization policy that checked for the "amr" claim with "mfa" value - Updated token refresh flow to preserve the authentication method across token renewals
- Modified claims generation to always include the METHOD claim with appropriate fallback
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/Shared/Services/AuthPolicies.cs |
Removed the TFA_ENABLED constant and policy definition |
src/Shared/Services/AppClaimTypes.cs |
Added METHOD claim type constant with documentation of supported authentication methods |
src/Shared/Extensions/ISharedServiceCollectionExtensions.cs |
Removed TFA_ENABLED policy registration from authorization configuration |
src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs |
Added logic to inject METHOD claim during claims generation, with HttpContext access and early return for non-Keycloak authentications |
src/Server/Boilerplate.Server.Api/Extensions/SignInManagerExtensions.cs |
Added HttpContext.Items storage for authentication method in OTP sign-in flow and removed explicit "amr" claim |
src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.cs |
Added METHOD claim preservation during token refresh to maintain authentication context |
| if (aspnetCoreIdentityClaims.HasClaim(AppClaimTypes.METHOD, "External") is false) | ||
| return; // User was not authenticated via Keycloak |
There was a problem hiding this comment.
The early return when the METHOD claim is not "External" means RetrieveKeycloakClaims will be skipped for all non-external authentications. However, this logic assumes that Keycloak is only used for external authentication. If there's a possibility of Keycloak being used as an authentication provider in other scenarios, this check might be too restrictive. Consider whether this assumption is always valid, or if additional configuration should be checked.
|
|
||
| /// <summary> | ||
| /// The method used for user authentication. | ||
| /// External (Social), Sms (Web-OTP), Email (Magic Link or 6 digit code), Push notification (6 digit code), WebAuthn (Face-Id, Fingerprint etc), Password. |
There was a problem hiding this comment.
The documentation mentions "Web-OTP" for SMS, but the actual authentication method value stored is "Sms" (as seen in SignInManagerExtensions.cs). Consider clarifying the documentation to indicate that "Web-OTP" is the technology used for SMS-based authentication, while the stored value is "Sms", to avoid confusion between the implementation detail and the stored claim value.
| /// External (Social), Sms (Web-OTP), Email (Magic Link or 6 digit code), Push notification (6 digit code), WebAuthn (Face-Id, Fingerprint etc), Password. | |
| /// Stored values: External (Social), Sms, Email (Magic Link or 6 digit code), Push notification (6 digit code), WebAuthn (Face-Id, Fingerprint etc), Password. | |
| /// Web-OTP is the technology used for SMS-based authentication; the stored claim value is "Sms". |
| HttpContext.Items[AppClaimTypes.METHOD] = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD); | ||
|
|
There was a problem hiding this comment.
The use of the null-forgiving operator on HttpContext here assumes it's always non-null. While this is likely safe in the Refresh endpoint context, it would be more defensive to handle the null case explicitly or add a null check, especially since this is a critical authentication flow. Consider using httpContextAccessor.HttpContext?.Items or adding an explicit null check with an appropriate exception.
| HttpContext.Items[AppClaimTypes.METHOD] = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD); | |
| var httpContext = HttpContext; | |
| if (httpContext is null) | |
| { | |
| throw new InvalidOperationException("HTTP context is not available for the refresh token request."); | |
| } | |
| httpContext.Items[AppClaimTypes.METHOD] = refreshTicket.Principal.GetClaimValue<string?>(AppClaimTypes.METHOD); |
|
|
||
| if (result.Claims.Any(c => c.Type == AppClaimTypes.METHOD) is false) | ||
| { | ||
| string? authenticationMethod = httpContextAccessor.HttpContext!.Items[AppClaimTypes.METHOD]?.ToString() ?? "Password"; |
There was a problem hiding this comment.
The use of the null-forgiving operator on HttpContext here assumes it's always non-null. While this may be safe during claims generation, it would be more robust to handle the null case explicitly or add a null check. If HttpContext is expected to always be available at this point, consider adding a descriptive exception when it's null. Alternatively, the fallback to "Password" could be adjusted to account for scenarios where HttpContext might be unavailable.
| string? authenticationMethod = httpContextAccessor.HttpContext!.Items[AppClaimTypes.METHOD]?.ToString() ?? "Password"; | |
| var httpContext = httpContextAccessor.HttpContext; | |
| var authenticationMethod = httpContext?.Items[AppClaimTypes.METHOD]?.ToString() ?? "Password"; |
1 participant
closes #11939
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.