-
Notifications
You must be signed in to change notification settings - Fork 888
WEB-551 Fix Oauth (Keycloak) #2967
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
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -61,21 +61,35 @@ function getOIDCConfig(): AuthConfig { | |
| } | ||
|
|
||
| /** | ||
| * Creates the configuration required for classic OAuth2 providers (e.g., Fineract). | ||
| * Creates the configuration required for classic OAuth2 providers (e.g., Fineract, Keycloak). | ||
| * @returns {AuthConfig} OAuth2 configuration block. | ||
| */ | ||
| function getOAuth2Config(): AuthConfig { | ||
| const frontendUrl = window.location.origin; | ||
| const { serverUrl, authorizeUrl, tokenUrl, redirectUri, scope, appId } = environment.oauth; | ||
| 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'; | ||
| const resolvedAuthorizeUrl = | ||
| authorizeUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/auth`; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not mix oauth and openid and hardcode any URI! |
||
| const resolvedTokenUrl = | ||
| tokenUrl || `${normalizedServerUrl}/auth/realms/${keycloakRealm}/protocol/openid-connect/token`; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
| const resolvedRedirectUri = redirectUri || `${frontendUrl}/#/callback`; | ||
| const resolvedScope = scope || 'openid profile email'; | ||
JaySoni1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // For Keycloak, issuer should be the realm URL for correct OAuth2 semantics | ||
| const issuerUrl = authorizeUrl ? normalizedServerUrl : `${normalizedServerUrl}/auth/realms/${keycloakRealm}`; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dont hardcode anything for keycloak please. Keycloak is one of many implementation! |
||
|
|
||
| return { | ||
| issuer: environment.oauth.serverUrl, | ||
| loginUrl: environment.oauth.authorizeUrl, | ||
| tokenEndpoint: environment.oauth.tokenUrl, | ||
| redirectUri: environment.oauth.redirectUri, | ||
| issuer: issuerUrl, | ||
| loginUrl: resolvedAuthorizeUrl, | ||
| tokenEndpoint: resolvedTokenUrl, | ||
| redirectUri: resolvedRedirectUri, | ||
| postLogoutRedirectUri: `${frontendUrl}/#/login`, | ||
| clientId: environment.oauth.appId, | ||
| clientId: appId, | ||
| responseType: 'code', | ||
| scope: environment.oauth.scope, | ||
| scope: resolvedScope, | ||
| useSilentRefresh: false, | ||
| oidc: false, | ||
| // Skip issuer validation for OAuth2 (non-OIDC) flows | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,14 +26,18 @@ export const environment = { | |
| apiActuator: loadedEnv.apiActuator || '/fineract-provider', | ||
| serverUrl: '', | ||
| oauth: { | ||
| enabled: loadedEnv.oauthServerEnabled === true, | ||
| serverUrl: loadedEnv.oauthServerUrl || '', | ||
| // Support legacy MIFOS_OAUTH_* variable names for backward compatibility with Keycloak | ||
| enabled: | ||
| loadedEnv.oauthServerEnabled === true || | ||
| String(loadedEnv.oauthServerEnabled).toLowerCase() === 'true' || | ||
| String(loadedEnv['MIFOS_OAUTH_SERVER_ENABLED']).toLowerCase() === 'true', | ||
| serverUrl: loadedEnv.oauthServerUrl || loadedEnv['MIFOS_OAUTH_SERVER_URL'] || '', | ||
| logoutUrl: loadedEnv.oauthServerLogoutUrl || '', | ||
| appId: loadedEnv.oauthAppId || '', | ||
| appId: loadedEnv.oauthAppId || loadedEnv['MIFOS_OAUTH_CLIENT_ID'] || '', | ||
| authorizeUrl: loadedEnv.oauthAuthorizeUrl || '', | ||
| tokenUrl: loadedEnv.oauthTokenUrl || '', | ||
| redirectUri: loadedEnv.oauthRedirectUri || '', | ||
| scope: loadedEnv.oauthScope || '' | ||
| redirectUri: loadedEnv.oauthRedirectUri || `${window.location.origin}/#/callback`, | ||
| scope: loadedEnv.oauthScope || 'openid profile email' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please dont mix oauth and openid together! |
||
| }, | ||
| /** Feature flag for Remember Me functionality */ | ||
| enableRememberMe: false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,14 +32,18 @@ export const environment = { | |
| /** Feature flag for Remember Me functionality */ | ||
| enableRememberMe: false, | ||
| oauth: { | ||
| enabled: loadedEnv.oauthServerEnabled === true, | ||
| serverUrl: loadedEnv.oauthServerUrl || '', | ||
| // Support legacy MIFOS_OAUTH_* variable names for backward compatibility with Keycloak | ||
| enabled: | ||
| loadedEnv.oauthServerEnabled === true || | ||
| String(loadedEnv.oauthServerEnabled).toLowerCase() === 'true' || | ||
| String(loadedEnv.MIFOS_OAUTH_SERVER_ENABLED).toLowerCase() === 'true', | ||
| serverUrl: loadedEnv.oauthServerUrl || loadedEnv.MIFOS_OAUTH_SERVER_URL || '', | ||
| logoutUrl: loadedEnv.oauthServerLogoutUrl || '', | ||
| appId: loadedEnv.oauthAppId || '', | ||
| appId: loadedEnv.oauthAppId || loadedEnv.MIFOS_OAUTH_CLIENT_ID || '', | ||
| authorizeUrl: loadedEnv.oauthAuthorizeUrl || '', | ||
| tokenUrl: loadedEnv.oauthTokenUrl || '', | ||
| redirectUri: loadedEnv.oauthRedirectUri || '', | ||
| scope: loadedEnv.oauthScope || '' | ||
| redirectUri: loadedEnv.oauthRedirectUri || `${window.location.origin}/#/callback`, | ||
| scope: loadedEnv.oauthScope || 'openid profile email' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please dont mix oauth and openid together! |
||
| }, | ||
| warningDialog: { | ||
| title: 'Warning', | ||
|
|
||
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.
I dont think we should have hardcoded dependencies for authentication implementation. Keep it generic and avoid any hardcoding!