-
-
Notifications
You must be signed in to change notification settings - Fork 254
bit Boilerplate now retrieves keycloak claims dynamically (#11837) #11838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR migrates the boilerplate identity provider from IdentityServerDemo to EnterpriseSso and adds Keycloak integration. Changes include updating UI components, configuring OpenID Connect with Keycloak HTTP client, dynamically retrieving and refreshing Keycloak JWT claims in the claims factory, updating Keycloak realm configuration, and refreshing localized resource strings across 12 languages. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Browser
participant Server as ASP.NET Server
participant KC as Keycloak
participant Store as User Store
participant JWT as JWT Parser
Client->>Server: OpenID Connect Redirect (EnterpriseSso)
Server->>KC: Authorization Request
KC-->>Server: Authorization Code
Server->>KC: Token Request
KC-->>Server: Tokens (access, refresh, expires_at)
Server->>Store: SetAuthenticationTokenAsync (persist tokens)
Note over Server: GenerateClaimsAsync Called
alt Token Expired
Server->>Server: Check token expiry (expires_at)
Server->>KC: Refresh Token Request (via HTTP Client)
KC-->>Server: New Access Token
Server->>JWT: Parse JWT Claims
JWT-->>Server: Keycloak Claims
Server->>Server: Add Claims to Identity
else Token Valid
Server->>JWT: Parse Existing JWT
JWT-->>Server: Keycloak Claims
Server->>Server: Add Claims to Identity
end
Server-->>Client: User Principal with Claims
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/demo-realm.json (1)
335-348: Confirm intent to drop client roles from the consolidatedrolesclaimThe new
rolesscope mapper now exposes only realm roles via a multivalued"roles"claim across id/access/userinfo tokens, which is a good fit for consumption on the .NET side, provided your OpenID Connect configuration maps"roles"into the framework’s role claim type.However, replacing the prior dual mappers with a single realm-role mapper means client roles will no longer flow through this claim if/when you define them. Please confirm this is intentional, or consider re-introducing client-role mapping (or a second claim) if you plan to rely on client roles as well.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json (1)
189-195: Enterprise SSO config block matches demo Keycloak client; consider emphasizing overrideThe new
Authentication:EnterpriseSsosection (withClientId=interactive.confidentialandClientSecret=secret) lines up with the demo Keycloak client used elsewhere, which is appropriate for a boilerplate.To reduce the chance these leak into non-demo environments, you might consider:
- Setting these to
nullby default and documenting the demo values, or- Adding a short note in
EnterpriseSso_Commentexplicitly stating that the ID/secret are demo-only and must be overridden for any real deployment.Not blocking, but it could help prevent misconfigured production setups.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json (1)
125-129: EnterpriseSso auth block is structurally fine; ensure it’s overridden per-environmentThe new
Authentication:EnterpriseSsosection is well‑formed and fits the existing pattern. SinceClientId/ClientSecretare demo values, make sure real deployments override them via environment‑specific appsettings or secrets management rather than relying on these defaults.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs (1)
90-107: Persisting external provider tokens is correct; be explicit about usage and storage guaranteesThe new block that pulls
access_token,refresh_token, andexpires_atfrominfo.AuthenticationTokensand saves them viaSetAuthenticationTokenAsyncis functionally sound and will enable later Enterprise SSO / Keycloak calls on behalf of the user.Two follow‑ups to keep in mind:
- Make sure the same token names (
"access_token","refresh_token","expires_at") are used consistently wherever you later read them (e.g., inAppUserClaimsPrincipalFactoryor other services), otherwise you’ll silently fail to locate the tokens.- Because you now persist long‑lived provider tokens for all external schemes, ensure your user store / database is treated as sensitive (encryption at rest, backups, and a story for revoking tokens if the account is compromised). If only EnterpriseSso needs this, you could optionally scope the persistence to
info.LoginProvider == "EnterpriseSso".src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (2)
38-42: Consider defensive parsing forexpires_at.
DateTimeOffset.Parsecan throwFormatExceptionif the stored value is malformed. Consider usingTryParseor wrapping in a try-catch to handle corrupted token data gracefully.- var keycloakTokenExpiryDate = DateTimeOffset.Parse(await UserManager.GetAuthenticationTokenAsync(user, "EnterpriseSso", "expires_at") ?? throw new InvalidOperationException("expires_at token is missing")); + var expiresAtString = await UserManager.GetAuthenticationTokenAsync(user, "EnterpriseSso", "expires_at"); + if (string.IsNullOrEmpty(expiresAtString) || !DateTimeOffset.TryParse(expiresAtString, out var keycloakTokenExpiryDate)) + { + return; // Skip Keycloak claims if token metadata is invalid + }
66-77: Consider filtering standard JWT claims.Adding all Keycloak JWT claims includes standard claims like
iat,exp,nbf,jti,iss,aud, andazpthat are token metadata rather than user identity claims. These could cause confusion or conflicts.+ var excludedClaimTypes = new HashSet<string>(StringComparer.OrdinalIgnoreCase) + { + "iat", "exp", "nbf", "jti", "iss", "aud", "azp", "typ", "auth_time", "session_state", "sid", "acr" + }; var keycloakClaims = parsedKeycloakAccessToken.Claims + .Where(claim => !excludedClaimTypes.Contains(claim.Type)) .Select(claim => new Claim( JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.TryGetValue(claim.Type, out var mapped) ? mapped : claim.Type, claim.Value)) .ToList();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/Components/SocialRow.razor(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/Components/SocialRow.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/appsettings.json(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/README.md(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/demo-realm.json(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/appsettings.json(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.ar.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.de.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.es.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fr.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.hi.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.nl.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.resx(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.sv.resx(2 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.zh.resx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs (1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Services/AppEnvironment.cs (1)
IsDevelopment(29-32)
🪛 markdownlint-cli2 (0.18.1)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/README.md
35-35: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
⏰ 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: Agent
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: build Bit.Templates
🔇 Additional comments (23)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/README.md (1)
82-82: Documentation updates appropriately support the Keycloak migration.The additions on line 82 (admin realm navigation note) and line 93 (specific 2FA security recommendation) enhance the guidance for developers and operators working with this setup. These changes align well with the migration to EnterpriseSso and Keycloak integration.
Also applies to: 93-93
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.resx (1)
636-637: Enterprise SSO sign-in text is consistent with other providersKey name and wording match the existing pattern for social sign-in buttons (Google, Facebook, etc.), so this looks good.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.de.resx (2)
1-1: XML declaration normalizationStandardizing the XML declaration (and effectively removing the BOM) is fine and should not affect RESX loading.
636-637: German Enterprise SSO sign-in text aligns with existing patternKey and translation (
Mit Enterprise SSO anmelden) are consistent with other social sign-in button texts in this file.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.nl.resx (2)
1-1: XML header cleanup is safeRemoving the BOM / normalizing the XML declaration is safe for RESX usage.
636-637: Dutch Enterprise SSO sign-in string is consistent
EnterpriseSsoSignInButtonTextwith “Aanmelden met Enterprise SSO” matches the phrasing style of the other providers.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/Components/SocialRow.razor.cs (1)
30-37: Enterprise SSO handler wiring looks correct; verify scheme name consistencyThe new
HandleEnterpriseSsomirrors the pattern of the other handlers and passes"EnterpriseSso"throughOnClick, which matches the naming used elsewhere in this PR.Just make sure the external login scheme / authentication configuration (e.g., in
Program.ServicesandSocialRow.razor) uses exactly the same"EnterpriseSso"identifier so the correct handler is invoked end-to-end.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.sv.resx (2)
1-1: RESX XML header normalizationThe cleaned XML declaration is fine and keeps the file consistent with other locale resources.
636-637: Swedish Enterprise SSO sign-in text fits existing wording
EnterpriseSsoSignInButtonTextwith “Logga in med Enterprise SSO” is consistent with the other provider strings.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.hi.resx (2)
1-1: XML declaration cleanupStandardizing the XML declaration (without BOM) is fine and keeps RESX tooling happy.
636-637: Hindi Enterprise SSO sign-in text is aligned with other providers
EnterpriseSsoSignInButtonTextwith “एंटरप्राइज़ SSO से साइन इन करें” follows the same phrasing as the existing Google/Azure Entra sign-in strings.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Identity/Components/SocialRow.razor (1)
14-18: EnterpriseSso provider wiring looks consistent; just confirm scheme name alignmentThe new EnterpriseSso branch mirrors the existing social providers and uses the localized
EnterpriseSsoSignInButtonTextplusEnterpriseSsoIcon, which is good. Please just double‑check that"EnterpriseSso"matches the authentication scheme name returned byGetSupportedSocialAuthSchemes, otherwise this button might never appear.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.es.resx (1)
1-1: Spanish Enterprise SSO label and RESX cleanup look goodThe XML header normalization is harmless, and the new
EnterpriseSsoSignInButtonText(“Iniciar sesión con SSO empresarial”) matches the tone and structure of the existing social sign‑in strings.Also applies to: 636-638
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.zh.resx (1)
1-1: Chinese Enterprise SSO translation is consistent with existing sign‑in labels
EnterpriseSsoSignInButtonText(“使用企业SSO登录”) fits the existing pattern for social sign‑in buttons and the RESX header fix is benign.Also applies to: 636-638
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.ar.resx (1)
1-1: Arabic Enterprise SSO resource entry is appropriate
EnterpriseSsoSignInButtonText(“تسجيل الدخول بـ SSO المؤسسي”) aligns with the existing Google/Facebook/etc sign‑in phrases and keeps terminology clear for enterprise SSO.Also applies to: 636-638
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fr.resx (1)
1-1: French Enterprise SSO label matches existing localization styleThe new
EnterpriseSsoSignInButtonText(“Se connecter avec SSO d'entreprise”) follows the same pattern as other social sign‑in button texts and reads naturally in French; header cleanup is fine.Also applies to: 636-638
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Resources/AppStrings.fa.resx (1)
639-641: Farsi Enterprise SSO text is clear and consistent
EnterpriseSsoSignInButtonText(“ورود با SSO سازمانی”) accurately conveys enterprise SSO sign‑in and is consistent with the existing social provider button labels.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Controllers/Identity/IdentityController.SocialSignIn.cs (1)
122-123: Local HTTP redirect for native clients remains correctThe
if (localHttpPort is not null)branch still correctly prefers the localhost callback for desktop/mobile hosts, falling back to the web app URL otherwise; no issues here.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs (2)
646-668: LGTM!The updated comments clearly explain the Keycloak integration flow, and the conditional authority configuration with fallback to Duende IdentityServer is well-structured.
677-680: LGTM!The
offline_accessscope is required for obtaining refresh tokens used inAppUserClaimsPrincipalFactory.RetrieveKeycloakClaims, andMapInboundClaims = trueensures proper claim type translation from JWT to .NET claim types.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs (3)
1-8: LGTM!The constructor expansion to include
IConfigurationandIServiceProvideris appropriate for the Keycloak integration. UsingIServiceProviderfor lazyIHttpClientFactoryresolution avoids circular dependency issues.
25-26: LGTM!The Keycloak claims retrieval is properly integrated after session claims, allowing Keycloak-sourced claims to augment the user's identity.
53-53: Consider extracting hardcoded realm "demo" to configuration.The realm "demo" appears hardcoded in this file and
Program.Services.cs. For production deployments with different realm names, extract this to a configuration setting for better flexibility and maintainability.
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs
Show resolved
Hide resolved
...erplate/src/Server/Boilerplate.Server.Api/Services/Identity/AppUserClaimsPrincipalFactory.cs
Outdated
Show resolved
Hide resolved
...Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/README.md
Outdated
Show resolved
Hide resolved
...Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.AppHost/Realms/README.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request enhances the bit Boilerplate's Keycloak integration by implementing dynamic claims retrieval from the identity provider. The changes rename "IdentityServerDemo" to "EnterpriseSso" for better clarity and add functionality to fetch and refresh Keycloak claims during user authentication.
Key changes:
- Implements dynamic Keycloak claims retrieval with automatic token refresh in
AppUserClaimsPrincipalFactory - Stores OAuth tokens (access, refresh, expires_at) from external providers for future use
- Updates Keycloak realm configuration to use a simplified roles mapper that places roles directly in the "roles" claim
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| AppStrings.*.resx (10 files) | Renamed resource key from IdentityServerDemoSignInButtonText to EnterpriseSsoSignInButtonText across all language files |
| Server.Web/appsettings.json | Added EnterpriseSso configuration with ClientId and ClientSecret |
| Server.Api/appsettings.json | Added EnterpriseSso configuration section with explanatory comment |
| demo-realm.json | Simplified Keycloak roles mapper to directly map realm roles to "roles" claim instead of nested structure |
| Realms/README.md | Updated documentation with improved user table formatting and clearer security notes |
| AppUserClaimsPrincipalFactory.cs | Added RetrieveKeycloakClaims method to dynamically fetch and refresh claims from Keycloak |
| Program.Services.cs | Configured EnterpriseSso OpenID Connect authentication and added Keycloak HttpClient |
| IdentityController.SocialSignIn.cs | Stores authentication tokens (access, refresh, expires_at) from external providers |
| SocialRow.razor.cs | Renamed HandleIdentityServerDemo to HandleEnterpriseSso |
| SocialRow.razor | Updated component to reference EnterpriseSso and new icon |
| EnterpriseSsoIcon.razor | New SVG icon component for Enterprise SSO sign-in button |
closes #11837
Summary by CodeRabbit
New Features
Documentation
Localization
✏️ Tip: You can customize this high-level summary in your review settings.