Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
OAuth2 Configuration Logic src/app/core/authentication/oauth.config.ts |
Introduces Keycloak realm resolution (uses MIFOS_OAUTH_REALM, default master) and computes resolvedAuthorizeUrl, resolvedTokenUrl, resolvedRedirectUri, resolvedScope, and issuerUrl when explicit endpoints are absent. Maps computed values to public config fields (issuer, loginUrl, tokenEndpoint, redirectUri, postLogoutRedirectUri, clientId/appId, scope) and sets skipIssuerCheck: true and strictDiscoveryDocumentValidation: false. |
Environment Configuration src/environments/environment.ts, src/environments/environment.prod.ts |
Adds backward-compatibility for legacy MIFOS_OAUTH_* env vars: oauth.enabled accepts MIFOS_OAUTH_SERVER_ENABLED, oauth.serverUrl/appId may come from legacy vars, redirectUri defaults to ${window.location.origin}/#/callback if missing, and scope defaults to 'openid profile email'. |
Sequence Diagram(s)
(omitted)
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- WEB-487: Implements OAuth 2.1 PKCE flow for authentication #2864: Modifies OAuth configuration and environment OAuth fields in the same files (
src/app/core/authentication/oauth.config.tsand environment files).
Suggested reviewers
- IOhacker
- adamsaghy
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title concisely describes the main change—fixing OAuth (Keycloak) configuration—and directly aligns with the PR's primary objectives of adding backward compatibility and Keycloak realm detection. |
| Docstring Coverage | ✅ Passed | Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. |
✨ Finishing touches
- 📝 Generate docstrings
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 help to get the list of available commands and usage tips.
![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
Fix all issues with AI Agents 🤖
In @src/app/core/authentication/oauth.config.ts:
- Line 79: The current OAuth config sets issuer to serverUrl; update the issuer
to the Keycloak realm URL by using the realm-based value (e.g.,
`${normalizedServerUrl}/auth/realms/${keycloakRealm}` or if you didn't apply
normalization, `${serverUrl}/auth/realms/${keycloakRealm}`) so the issuer symbol
points to the realm endpoint rather than the root server; keep or remove
skipIssuerCheck as desired, but ensure issuer is corrected in the object where
issuer: serverUrl is set so OAuth2 semantics match Keycloak.
- Around line 68-76: Validate and normalize environment.oauth.serverUrl before
constructing endpoints: ensure serverUrl is non-empty (throw or fallback if
missing), trim trailing slashes, and ensure it includes a scheme (e.g., add
'https://' if absent) so that resolvedAuthorizeUrl, resolvedTokenUrl and any
derived URLs are well-formed; update the logic around keycloakRealm,
resolvedAuthorizeUrl and resolvedTokenUrl to use the normalized serverUrl and
return or log a clear error when serverUrl is invalid.
🧹 Nitpick comments (1)
src/environments/environment.ts (1)
34-44: Prefer consistent property access notation across environment files.The dev environment uses dot notation (
loadedEnv.MIFOS_OAUTH_SERVER_ENABLED) while the production environment uses bracket notation (loadedEnv['MIFOS_OAUTH_SERVER_ENABLED']) for the same properties. While both work, consistent notation improves maintainability.🔎 Proposed fix for consistency
oauth: { // Support legacy MIFOS_OAUTH_* variable names for backward compatibility with Keycloak - enabled: loadedEnv.oauthServerEnabled === true || loadedEnv.MIFOS_OAUTH_SERVER_ENABLED === 'true', - serverUrl: loadedEnv.oauthServerUrl || loadedEnv.MIFOS_OAUTH_SERVER_URL || '', + enabled: loadedEnv.oauthServerEnabled === true || loadedEnv['MIFOS_OAUTH_SERVER_ENABLED'] === 'true', + serverUrl: loadedEnv.oauthServerUrl || loadedEnv['MIFOS_OAUTH_SERVER_URL'] || '', logoutUrl: loadedEnv.oauthServerLogoutUrl || '', - appId: loadedEnv.oauthAppId || loadedEnv.MIFOS_OAUTH_CLIENT_ID || '', + appId: loadedEnv.oauthAppId || loadedEnv['MIFOS_OAUTH_CLIENT_ID'] || '', authorizeUrl: loadedEnv.oauthAuthorizeUrl || '', tokenUrl: loadedEnv.oauthTokenUrl || '', redirectUri: loadedEnv.oauthRedirectUri || `${window.location.origin}/#/callback`, scope: loadedEnv.oauthScope || 'openid profile email' },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/core/authentication/oauth.config.tssrc/environments/environment.prod.tssrc/environments/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/core/authentication/oauth.config.ts
🧬 Code graph analysis (1)
src/app/core/authentication/oauth.config.ts (3)
src/environments/environment.prod.ts (1)
environment(7-82)src/environments/environment.ts (1)
environment(12-86)src/app/settings/settings.service.ts (1)
serverUrl(175-177)
🔇 Additional comments (2)
src/environments/environment.prod.ts (1)
28-38: LGTM! Backward compatibility implementation looks solid.The legacy environment variable mapping is well-implemented with appropriate fallback chains and sensible defaults for redirect URI and scope.
src/app/core/authentication/oauth.config.ts (1)
88-96: No action needed—validation flags are appropriate for OAuth2 mode, not workarounds.The issuer is correctly set to
serverUrl(line 79), and theskipIssuerCheck: trueandstrictDiscoveryDocumentValidation: falseflags are legitimate settings for OAuth2 (non-OIDC) mode. The angular-oauth2-oidc library is designed for OIDC but supports OAuth2 without full OIDC compliance. For Keycloak OAuth2 integration, these relaxations are necessary and appropriate. The code already documents why these flags are required.
JaySoni1
force-pushed
the
WEB-551-fix-oauth-keycloak
branch
from
00504ea to
8d5f0fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/environments/environment.ts:
- Around line 35-36: The enabled assignment in environment.ts mixes boolean and
string checks causing inconsistent behavior; update the enabled expression for
oauthServerEnabled to accept both boolean true and the string "true" (and do the
same for the legacy MIFOS_OAUTH_SERVER_ENABLED) following the OIDC block pattern
— e.g. coerce values to strings or explicitly check both types for
loadedEnv.oauthServerEnabled and loadedEnv.MIFOS_OAUTH_SERVER_ENABLED when
setting the enabled property so both new and legacy env vars are handled
consistently.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/core/authentication/oauth.config.ts (1)
96-98: Clarify that security-relaxed flags are in the OAuth2 (non-OIDC) path only; recommend OIDC mode for Keycloak production.The
skipIssuerCheckandstrictDiscoveryDocumentValidationflags appear ingetOAuth2Config()(lines 96–98) withoidc: false, which is appropriate for OAuth2-only flows. However, for Keycloak in production, prefer thegetOIDCConfig()path (oidc: true), which applies strict validation defaults and does not override these flags. If using pure OAuth2 mode with custom endpoints, document the specific provider constraints that justify disabling discovery validation.
♻️ Duplicate comments (1)
src/app/core/authentication/oauth.config.ts (1)
70-79: Missing validation for emptyserverUrlcan produce malformed OAuth endpoints.When
serverUrlis empty (which is the default in environment.ts),normalizedServerUrlbecomes'', and the resolved URLs become malformed (e.g.,/auth/realms/master/protocol/openid-connect/auth). This will cause OAuth flows to fail silently or redirect incorrectly.Consider adding validation or a guard when OAuth2 is enabled but
serverUrlis not configured.🔎 Proposed fix with validation
function getOAuth2Config(): AuthConfig { const frontendUrl = window.location.origin; const { serverUrl, authorizeUrl, tokenUrl, redirectUri, scope, appId } = environment.oauth; - const normalizedServerUrl = serverUrl?.replace(/\/$/, '') || ''; + const normalizedServerUrl = serverUrl?.trim().replace(/\/$/, '') || ''; + + // Warn if OAuth2 is enabled but serverUrl is missing (unless custom endpoints are provided) + if (!normalizedServerUrl && (!authorizeUrl || !tokenUrl)) { + console.warn('OAuth2 is enabled but serverUrl is not configured. OAuth flows may fail.'); + } // Allow custom Keycloak realm via MIFOS_OAUTH_REALM (defaults to master) const keycloakRealm = (window as any)['env']?.['MIFOS_OAUTH_REALM'] || 'master';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/core/authentication/oauth.config.tssrc/environments/environment.prod.tssrc/environments/environment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/environments/environment.prod.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/core/authentication/oauth.config.ts
🧬 Code graph analysis (1)
src/app/core/authentication/oauth.config.ts (3)
src/environments/environment.prod.ts (1)
environment(7-82)src/environments/environment.ts (1)
environment(12-86)src/app/settings/settings.service.ts (1)
serverUrl(175-177)
🔇 Additional comments (2)
src/environments/environment.ts (1)
37-43: Backward compatibility and defaults look good.The fallback chain for
serverUrl,appId,redirectUri, andscopeprovides sensible defaults and maintains backward compatibility with legacyMIFOS_OAUTH_*environment variables. The default scope'openid profile email'is appropriate for standard OIDC/OAuth2 flows.src/app/core/authentication/oauth.config.ts (1)
81-92: Issuer URL logic correctly handles Keycloak realm semantics.The conditional issuer logic appropriately uses the realm-based URL (
${normalizedServerUrl}/auth/realms/${keycloakRealm}) for standard Keycloak deployments while falling back tonormalizedServerUrlwhen customauthorizeUrlis provided. This addresses proper OAuth2 semantics for Keycloak.
JaySoni1
force-pushed
the
WEB-551-fix-oauth-keycloak
branch
from
8d5f0fa to
fc9a888
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/core/authentication/oauth.config.ts (1)
70-70: Validate serverUrl to prevent malformed OAuth endpoint URLs.The normalization on Line 70 removes trailing slashes but doesn't validate that
serverUrlis non-empty or includes a scheme. IfserverUrlis empty, the constructed OAuth endpoints will be relative URLs (e.g.,/auth/realms/master/protocol/openid-connect/auth) without a host, causing OAuth flow failures.🔎 Proposed fix with validation and normalization
function getOAuth2Config(): AuthConfig { const frontendUrl = window.location.origin; const { serverUrl, authorizeUrl, tokenUrl, redirectUri, scope, appId } = environment.oauth; - const normalizedServerUrl = serverUrl?.replace(/\/$/, '') || ''; + + // Validate and normalize serverUrl + const trimmedServerUrl = serverUrl?.trim() || ''; + if (!trimmedServerUrl) { + throw new Error('OAuth2 serverUrl is required but was not provided. Please configure environment.oauth.serverUrl.'); + } + const normalizedServerUrl = trimmedServerUrl.replace(/\/$/, ''); // Allow custom Keycloak realm via MIFOS_OAUTH_REALM (defaults to master) const keycloakRealm = (window as any)['env']?.['MIFOS_OAUTH_REALM'] || 'master'; const resolvedAuthorizeUrl = authorizeUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/auth`; const resolvedTokenUrl = tokenUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/token`;Based on learnings from past review comments.
Also applies to: 74-77
🧹 Nitpick comments (1)
src/app/core/authentication/oauth.config.ts (1)
81-82: Consider clarifying the issuer logic for better maintainability.The issuer logic is functionally correct and addresses the past review comment about setting the correct Keycloak realm URL. However, the conditional logic could be clearer:
- When
authorizeUrlis provided (custom OAuth provider): issuer =normalizedServerUrl- When
authorizeUrlis NOT provided (Keycloak default): issuer = realm URL🔎 Suggested refactor for improved clarity
- // For Keycloak, issuer should be the realm URL for correct OAuth2 semantics - const issuerUrl = authorizeUrl ? normalizedServerUrl : `${normalizedServerUrl}/auth/realms/${keycloakRealm}`; + // Determine issuer based on OAuth provider type: + // - Custom OAuth provider (custom authorizeUrl provided): use serverUrl as issuer + // - Keycloak (default endpoints): use realm URL as issuer for correct OAuth2 semantics + const issuerUrl = authorizeUrl + ? normalizedServerUrl + : `${normalizedServerUrl}/auth/realms/${keycloakRealm}`;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/core/authentication/oauth.config.tssrc/environments/environment.prod.tssrc/environments/environment.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/environments/environment.prod.ts
- src/environments/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/core/authentication/oauth.config.ts
🧬 Code graph analysis (1)
src/app/core/authentication/oauth.config.ts (3)
src/environments/environment.prod.ts (1)
environment(7-85)src/environments/environment.ts (1)
environment(12-89)src/app/settings/settings.service.ts (1)
serverUrl(175-177)
🔇 Additional comments (3)
src/app/core/authentication/oauth.config.ts (3)
64-66: LGTM!The updated comment accurately reflects the function's support for both Fineract and Keycloak OAuth2 providers.
85-92: LGTM!The return statement correctly uses all the resolved values with appropriate defaults. The configuration structure is correct for the
angular-oauth2-oidclibrary, and the OAuth2-specific settings (skipIssuerCheck: true,strictDiscoveryDocumentValidation: false,oidc: false) are appropriate for non-OIDC OAuth2 flows.
72-73: Code correctly implements Keycloak realm detection with consistent usage.The
keycloakRealmvariable is properly used in all three required OAuth endpoint constructions:
- authorize endpoint (line 75)
- token endpoint (line 77)
- issuer URL (line 82)
The optional chaining and 'master' fallback are appropriate for this runtime configuration pattern. Type assertion is justified for the non-TypeScript window.env pattern.
| redirectUri: loadedEnv.oauthRedirectUri || '', | ||
| scope: loadedEnv.oauthScope || '' | ||
| redirectUri: loadedEnv.oauthRedirectUri || `${window.location.origin}/#/callback`, | ||
| scope: loadedEnv.oauthScope || 'openid profile email' |
There was a problem hiding this comment.
Please dont mix oauth and openid together!
| redirectUri: loadedEnv.oauthRedirectUri || '', | ||
| scope: loadedEnv.oauthScope || '' | ||
| redirectUri: loadedEnv.oauthRedirectUri || `${window.location.origin}/#/callback`, | ||
| scope: loadedEnv.oauthScope || 'openid profile email' |
There was a problem hiding this comment.
Please dont mix oauth and openid together!
| const normalizedServerUrl = serverUrl?.replace(/\/$/, '') || ''; | ||
|
|
||
| // Allow custom Keycloak realm via MIFOS_OAUTH_REALM (defaults to master) | ||
| const keycloakRealm = (window as any)['env']?.['MIFOS_OAUTH_REALM'] || 'master'; |
There was a problem hiding this comment.
I dont think we should have hardcoded dependencies for authentication implementation. Keep it generic and avoid any hardcoding!
| // Allow custom Keycloak realm via MIFOS_OAUTH_REALM (defaults to master) | ||
| const keycloakRealm = (window as any)['env']?.['MIFOS_OAUTH_REALM'] || 'master'; | ||
| const resolvedAuthorizeUrl = | ||
| authorizeUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/auth`; |
There was a problem hiding this comment.
Do not mix oauth and openid and hardcode any URI!
| const resolvedAuthorizeUrl = | ||
| authorizeUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/auth`; | ||
| const resolvedTokenUrl = | ||
| tokenUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/token`; |
| const resolvedScope = scope || 'openid profile email'; | ||
|
|
||
| // For Keycloak, issuer should be the realm URL for correct OAuth2 semantics | ||
| const issuerUrl = authorizeUrl ? normalizedServerUrl : `${normalizedServerUrl}/auth/realms/${keycloakRealm}`; |
There was a problem hiding this comment.
Dont hardcode anything for keycloak please. Keycloak is one of many implementation!
adamsaghy
left a comment
adamsaghy
left a comment
There was a problem hiding this comment.
I think the UI project should NOT:
mix OAuth and OpenId!
Hardcode any URI and anything that is for a particular implementation (like keycloak)
use "keycloak" and "openId" specific configuration
alberto-art3ch
left a comment
alberto-art3ch
left a comment
There was a problem hiding this comment.
Let's discuss this solution for the different options and scenarios, like with some env variable present one LoginForm or other one
- Auto-construct Keycloak authorize/token endpoints from serverUrl + realm - Default realm to 'fineract' if not specified - Add sensible defaults for redirectUri and scope - Fix oauth.enabled to handle string 'true' from environment variables - Add realm property to oauth configuration Based on upstream PR openMF#2967 (WEB-551 Fix OAuth Keycloak) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
This pull request seems to be stale. Are you still planning to work on it? We will automatically close it in 30 days. |
|
@IOhacker @adamsaghy @alberto-art3ch any updates? can we close this PR? |
|
@Ádám Sághy ***@***.***> is there any Apache Fineract updated docs
about how to do the setup for using the OAUTH?
El dom, 15 feb 2026 a las 17:13, Gopi Kishan ***@***.***>)
escribió:
… *gkbishnoi07* left a comment (openMF/web-app#2967)
<#2967 (comment)>
@IOhacker <https://github.com/IOhacker> @adamsaghy
<https://github.com/adamsaghy> @alberto-art3ch
<https://github.com/alberto-art3ch> any updates? can we close this PR?
—
Reply to this email directly, view it on GitHub
<#2967 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALD2ZASKMWK43XIURYQE4XL4MD4SRAVCNFSM6AAAAACQV7JCQKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTSMBVGM4DCMRUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I dont recall, but probably we should have... |
|
@IOhacker I dont see any changes on this PR... |
|
@adamsaghy is there any updated doc for setting up a Keycloak instance + Apache Fineract ? so then we can verify |
We dont have documentation for setting up Keycloak, since that's just one auth server implementation from the many. Also we are lacking proper documentation for Apache Fineract acting as a resource server only. We are using |
5 participants
Changes Made :-
WEB :- 551
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.