Repository navigation
Conversation
Contributor
E2E — mocked backend🎭 E2E Tests✅ All green — 431 passed · 0 failed · 1 skipped, across 40 spec files in 10m 9s. By spec file
All 432 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 39414607359 from |
Contributor
E2E — real Fineract🎭 E2E Tests✅ All green — 171 passed · 0 failed · 0 skipped, across 50 spec files in 10m 42s. By spec file
All 171 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 39414607359 from |
Adds an opt-in "Sign in with your identity provider" path to the login page: the
authorization-code flow with PKCE (S256) as a public client. It is driven by the
config.json keys oidcLoginEnabled, oidc { issuer, clientId, scopes, redirectPath,
postLogoutPath } and passwordLoginEnabled. Without them the login page is unchanged.
- OidcService: discovery (endpoints must share the issuer's origin), state, nonce and
PKCE verifier, code exchange, ID token nonce/iss/aud check, refresh before expiry,
RP-initiated logout. offline_access is never requested. No new dependency; it uses
fetch, so the tenant-header interceptors do not touch provider traffic.
- authInterceptor sends Authorization: Bearer for an OIDC session.
- The user context comes from GET /v1/userdetails with the Bearer token, mapped to the
session a Basic login produces, so menus, guards and rbacEnabled work unchanged.
Fineract 1.15.0 serves that endpoint only with fineract.security.oauth2.enabled, so
with OIDC federation alone it answers 404. The callback page then says so and opens
no session.
- /auth/callback route, session storage keys, mocked e2e coverage of the round trip.
- passwordLoginEnabled: false hides the password form; it is ignored unless OIDC
sign-in is configured, so a mistaken config cannot lock everyone out.
- DOCS/OIDC.md documents the config keys, the Keycloak client settings and the
/v1/userdetails platform requirement.
Closes apache#370
Adds the six login.sso keys this change introduces, drops the notImplemented key it removes, and renames ON_HOLD_TRANSACTIONS.REASON to LOAN to follow en.json. npm run i18n:check is clean (fr 2752/2752).
guymoyo
force-pushed
the
feat/oidc-sign-in
branch
from
October 19, 2026 18:03
71fd35a to
6e4bcc7
Compare
Contributor
🔄 What this change talks toFrom the 19 application TypeScript file(s) changed in this pull request. sequenceDiagram
autonumber
participant FineractRoleApi as FineractRoleApi (Service)
participant RolesService as RolesService (API client)
participant Fineract as Fineract (REST API)
participant FineractUserApi as FineractUserApi (Service)
participant UsersService as UsersService (API client)
participant PasswordPreferencesService as PasswordPreferencesService (API client)
participant AuthService as AuthService (Service)
participant STORAGE as STORAGE (Injected)
participant ConfigService as ConfigService (Service)
participant Injector as Injector (Injected)
participant OidcService as OidcService (Injected)
participant HttpClient as HttpClient (Injected)
FineractRoleApi->>RolesService: getRoles()
RolesService->>Fineract: getRoles()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: getRolesRoleId()
RolesService->>Fineract: getRolesRoleId()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: getRolesRoleIdPermissions()
RolesService->>Fineract: getRolesRoleIdPermissions()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: postRoles()
RolesService->>Fineract: postRoles()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: putRolesRoleId()
RolesService->>Fineract: putRolesRoleId()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: putRolesRoleIdPermissions()
RolesService->>Fineract: putRolesRoleIdPermissions()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: postRolesRoleId()
RolesService->>Fineract: postRolesRoleId()
Fineract-->>RolesService: response
FineractRoleApi->>RolesService: deleteRolesRoleId()
RolesService->>Fineract: deleteRolesRoleId()
Fineract-->>RolesService: response
FineractUserApi->>UsersService: getUsers()
UsersService->>Fineract: getUsers()
Fineract-->>UsersService: response
FineractUserApi->>UsersService: getUsersUserId()
UsersService->>Fineract: getUsersUserId()
Fineract-->>UsersService: response
FineractUserApi->>UsersService: getUsersTemplate()
UsersService->>Fineract: getUsersTemplate()
Fineract-->>UsersService: response
FineractUserApi->>UsersService: postUsers()
UsersService->>Fineract: postUsers()
Fineract-->>UsersService: response
FineractUserApi->>UsersService: putUsersUserId()
UsersService->>Fineract: putUsersUserId()
Fineract-->>UsersService: response
FineractUserApi->>PasswordPreferencesService: getPasswordpreferences()
PasswordPreferencesService->>Fineract: getPasswordpreferences()
Fineract-->>PasswordPreferencesService: response
FineractUserApi->>UsersService: postUsersUserIdPwd()
UsersService->>Fineract: postUsersUserIdPwd()
Fineract-->>UsersService: response
Truncated for readability: 16 further participant(s) and 15 further call(s) not shown.
Generated by run 39414607265 from |
Aman-Mittal
requested changes
Oct 27, 2026
| completeOidcLogin(): Observable<UserSession> { | ||
| const tenantId = this.currentTenantId(); | ||
| return this.http | ||
| .get<UserDetailsResponse>(`${this.configService.apiUrl}/userdetails`, { |
Member
There was a problem hiding this comment.
Blocking (platform dependency). On current Fineract this call 404s in the mode an external IdP uses. UserDetailsApiResource is @ConditionalOnProperty("fineract.security.oauth2.enabled") (Fineract as its own authorization server), and federation (fineract.security.oidc-federation.enabled) doesn't load it. So every sign-in ends on "server cannot identify users".
Please link the Fineract change (Jira plus PR) this depends on, and gate or document the feature by minimum Fineract version until it's released.
| - `passwordLoginEnabled: false` is ignored unless OIDC sign-in is enabled and complete, so a mistaken | ||
| config cannot lock everyone out. It hides the form only; it does not stop Fineract from accepting | ||
| Basic credentials, which is a server setting. | ||
| - The Content-Security-Policy must allow the provider: add its origin to `connect-src`. The |
Member
There was a problem hiding this comment.
The shipped container can't do this. deploy/nginx.conf.template:50 hard-codes connect-src 'self', and deploy/entrypoint.sh only substitutes FINERACT_UPSTREAM, FINERACT_PROXY_SSL_VERIFY and FINERACT_FORWARDED_PROTO. With the official image, the discovery fetch and the token fetch to the IdP are blocked, so users always see "The identity provider could not be reached". The authorize redirect still works because it's a navigation.
Please add an entrypoint setting (e.g. OIDC_ISSUER_ORIGIN) that's appended to connect-src, or support proxying the IdP's .well-known and token endpoints through the same origin. Either way, document it here instead of asking operators to hand-edit the CSP.
| accessToken: body.access_token, | ||
| refreshToken: body.refresh_token ?? previous?.refreshToken, | ||
| idToken: body.id_token ?? previous?.idToken, | ||
| expiresAt: now + (body.expires_in ?? 60) * 1000, |
Member
There was a problem hiding this comment.
Refresh loop. The fallback lifetime (expires_in ?? 60) equals REFRESH_SKEW_MS (60 000), so:
ensureFresh'sexpiresAt - now > REFRESH_SKEW_MSis never true, and every API call waits on a refresh;scheduleRefreshcomputesMath.max(0, 1000)and re-fires every 1 s.
The same happens for any provider configured with an access-token lifetime of 60 s or less (expires_in is only RECOMMENDED in RFC 6749). The result is an endless refresh_token grant loop against the IdP.
Suggested:
- fall back to the JWT
exp, or something like 300 s; - use
skew = min(60 s, lifetime / 2); - floor the timer delay (e.g. 15 s);
- add unit tests for
expires_inmissing and set to 30.
| } catch (e) { | ||
| if (generation !== this.generation) return; | ||
| // invalid_grant and friends: the provider session is gone. A network error is not. | ||
| if (e instanceof OidcError) this.endLocally(); |
Member
There was a problem hiding this comment.
A transient outage signs everyone out. tokenRequest (L414–418) throws OidcError for any !response.ok, which includes 500, 502 and 503, and an HTML gateway page whose body parses as {} and becomes token_error. This line then calls endLocally(), so a short IdP or proxy blip during a background refresh sends every signed-in teller to /login?reason=session-expired. That contradicts the comment above and the docs ("a network failure does not").
Please end the session only on 400 or 401 with error ∈ {invalid_grant, invalid_client, unauthorized_client}; treat 5xx and unparseable responses like a network error and retry. Please also add a unit test; the current ones cover only a refusal and fetch rejecting.
| } | ||
| }; | ||
| if ( | ||
| doc.issuer !== issuer || |
Member
There was a problem hiding this comment.
Interoperability:
- Trailing-slash issuers. L425 strips trailing slashes from the configured issuer, then this compares it to
doc.issuer, which must match exactly. Providers that publish a trailing slash, e.g. Auth0's"issuer": "https://tenant.auth0.com/", can never pass. Please compare against the configured value as-is, and normalise only the URL used to build.well-known. - Endpoint origins. The same-origin requirement on every endpoint (L454) rules out providers whose token endpoint lives on another host (e.g. Google's
oauth2.googleapis.com). Either allow an explicitallowedEndpointOrigins, or state the limitation inOIDC.md.
| // as id_token_hint to end its session without asking the user to confirm. | ||
| const idToken = this.oidc.getIdToken(); | ||
| this.clearSession(); | ||
| if (viaProvider) void this.oidc.endSession(idToken); |
Member
There was a problem hiding this comment.
OIDC users lose the sign-out reason. idle.service.ts and error.interceptor.ts call logout() and then navigate to /login?reason=…. For OIDC sessions, endSession does a window.location.assign(end_session_endpoint…) that returns to the fixed postLogoutPath, without the reason. So someone timed out for inactivity, or whose token Fineract rejected, sees a plain login page with no explanation; basic-auth users still get the message.
Please carry the reason through, e.g. stash it in session storage before redirecting and read it on /login, or append it to post_logout_redirect_uri if the IdP allows that.
| /** The authenticated session, including the Basic auth key. Tab-scoped, never persisted. */ | ||
| session: { key: 'fineract_session', scope: 'session' }, | ||
| /** The OIDC tokens of the signed-in user. Tab-scoped, never persisted. */ | ||
| oidcTokens: { key: 'fineract_oidc_tokens', scope: 'session' }, |
Member
There was a problem hiding this comment.
Security hardening, non-blocking. This matches how the Basic key is stored, and the strict CSP limits XSS. But a refresh token taken from the session-scoped store is worse than a stolen Basic key: the client is public and nothing ties the token to this browser, so whoever holds it can mint new access tokens from anywhere until it expires.
At minimum, DOCS/security.md should require refresh-token rotation at the IdP (Keycloak "Revoke Refresh Token", reuse 0). Better options are keeping the refresh token in memory only (accepting a re-login on reload) or DPoP.
Also, when the IdP has no end_session_endpoint, logout returns early (oidc.service.ts:288) without calling the revocation endpoint, so the refresh token stays valid at the IdP.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Adds an opt-in "Sign in with your identity provider" path to the login page: authorization-code flow with PKCE (S256) as a public client, driven by an
oidcblock inconfig.json(oidcLoginEnabled,oidc.issuer,oidc.clientId,oidc.scopes,passwordLoginEnabled). Without those keys the login page is unchanged, and password login stays on unless a deployment turns it off (which is ignored unless OIDC is fully configured). No new dependency;offline_accessis never requested.The UI loads the user context from
GET /v1/userdetailswith the Bearer token and maps it to the same session a Basic login produces, so menus, guards andrbacEnabledwork unchanged. Platform note: in Fineract 1.15.0 that endpoint is only loaded withfineract.security.oauth2.enabled, so with OIDC federation (fineract.security.oidc-federation.enabled) alone it answers 404. Fineract needs a change to also load it under the federation flag; that upstream work is tracked separately (Jira to be filed). The UI does not fail open: on a 404 it drops the tokens, opens no session and shows a clear message on the callback page.DOCS/OIDC.mddocuments the config keys, the Keycloak client settings (public client, PKCE S256, exact redirect URIs, nooffline_access, audience mapper) and the userdetails requirement.Closes #370
Verification
npm ci,npm run lint(0 errors, no change to the suppressions baseline),npm run test:unit(312 files, 2546 tests),npm run build,npm run i18n:check,check:icons,check:a11y-names,check:route-permissionsall pass.--project=mocked e2e/login.spec.ts e2e/oidc-login.spec.ts) passes. The OIDC spec covers the config matrix (absent, wrong-typed or incomplete config shows nothing new), password form hiding, the full round trip against a mocked provider (discovery, PKCE S256 redirect, code exchange, Bearer/v1/userdetails), the 404 case, and a mismatchedstate.backendPlaywright project (oidc-login-backend.spec.tswas updated for the new behaviour but needs Docker).Screenshots
Available on request.
Checklist
src/app/api/.src/app/core/adapters/instead of direct browser globals or imperative third-party APIs.dev@fineract.apache.org first —
or it is a bug fix, refactor or test change, where that does not apply.