feat(oidc): refresh the access token in-place when it expires - #10371
feat(oidc): refresh the access token in-place when it expires#10371perfectra1n wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new middleware, refreshOidcTokenIfNeeded, to automatically refresh expired OIDC access tokens when the offline_access scope is configured. It also includes comprehensive unit tests and updates the user documentation. The reviewer suggested restricting Google-specific authorization parameters (access_type=offline and prompt=consent) to only be sent when the provider is Google, as unconditionally sending them causes disruptive consent prompts on other standard OIDC providers. The documentation should also be updated to reflect this behavior.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const offlineAccessParams = wantsOfflineAccess | ||
| ? { access_type: "offline", prompt: "consent" } | ||
| : {}; |
There was a problem hiding this comment.
The access_type=offline and prompt=consent parameters are Google-specific workarounds to force Google to return a refresh token on subsequent logins. However, for standard OpenID Connect providers (like Keycloak, Authentik, Okta, or Auth0), requesting the offline_access scope is sufficient to obtain a refresh token. Unconditionally sending prompt=consent to these standard providers forces the user to see the consent screen on every single login, which is a disruptive user experience.
We should restrict these parameters to only be sent when the configured issuer is Google.
const issuer = config.MultiFactorAuthentication.oauthIssuerBaseUrl.replace(/\/+$/, "");
const isGoogle = issuer === GOOGLE_ISSUER;
const offlineAccessParams = (wantsOfflineAccess && isGoogle)
? { access_type: "offline", prompt: "consent" }
: {};There was a problem hiding this comment.
Implemented in 218c7c9, with one addition beyond the suggestion: when the issuer is Google, offline_access is also stripped from the scope string, since Google rejects it as invalid_scope — leaving it in would break the authorization request for the very users opting in. So: spec providers get the scope passed through with no extra params; Google gets the scope minus offline_access plus access_type=offline/prompt=consent. Reuses the existing GOOGLE_ISSUER comparison (extracted as isGoogleIssuer(), shared with resolveClientAuthMethod).
| 1. Add `offline_access` to the `oauthScope` setting (e.g. `oauthScope=openid profile email offline_access`, or the `TRILIUM_MULTIFACTORAUTHENTICATION_OAUTHSCOPE` / `TRILIUM_OAUTH_SCOPE` environment variable). | ||
| 2. Restart the server and reconnect your account so the provider issues a refresh token. | ||
|
|
||
| When offline access is requested, Trilium additionally sends `access_type=offline` and `prompt=consent` to the authorization endpoint (required for Google to issue and re-issue a refresh token). These parameters are **not** sent otherwise, so the normal sign-in flow is unaffected. |
There was a problem hiding this comment.
If we restrict access_type=offline and prompt=consent to Google only (as suggested in the code review), we should update this documentation to clarify that these parameters are only sent when the provider is Google, preventing confusion for users of other OIDC providers.
| When offline access is requested, Trilium additionally sends `access_type=offline` and `prompt=consent` to the authorization endpoint (required for Google to issue and re-issue a refresh token). These parameters are **not** sent otherwise, so the normal sign-in flow is unaffected. | |
| When offline access is requested and the provider is Google, Trilium additionally sends access_type=offline and prompt=consent to the authorization endpoint (required for Google to issue and re-issue a refresh token). For other providers, these Google-specific parameters are omitted to avoid unnecessary consent prompts. |
There was a problem hiding this comment.
Updated in 218c7c9 to describe the per-issuer behavior, including the Google scope-stripping.
| if (oauthError === "invalid_grant") { | ||
| getLog().info(`OIDC refresh token rejected by IdP (invalid_grant): ${message}. Forcing re-authentication.`); |
There was a problem hiding this comment.
invalid_grant from the IdP signals a deliberate revocation (admin-forced logout, consent withdrawal, IdP-side session invalidation). Logging it at info buries a security-relevant event alongside routine operational chatter; warn makes it visible to operators monitoring logs for anomalies without requiring a log-level change.
| if (oauthError === "invalid_grant") { | |
| getLog().info(`OIDC refresh token rejected by IdP (invalid_grant): ${message}. Forcing re-authentication.`); | |
| if (oauthError === "invalid_grant") { | |
| getLog().warn(`OIDC refresh token rejected by IdP (invalid_grant): ${message}. Forcing re-authentication.`); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not applied: the core logger (packages/trilium-core/src/services/log.ts) has no warn() method — only log/info/error — so the suggested code doesn't compile. The repo also logs comparable security-relevant OAuth rejections at info (e.g. OAuth login rejected: the authenticated account is not the enrolled one in afterCallback), so info matches the established convention here. Happy to bump to error if the maintainer prefers louder revocation events, but warn isn't available.
There was a problem hiding this comment.
Fair point — if warn() doesn't exist on the logger, my suggestion would break the build. And with info being the established convention for OAuth rejection events in this codebase, info is the right call here. Withdrawing this suggestion.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
Adds refreshOidcTokenIfNeeded middleware (mounted after the reactive OIDC middleware in app.ts) that refreshes an expired OIDC access token using the stored refresh token: - No-ops unless the user is OIDC-authenticated, the access token is expired, and a refresh token is present (so it is inert for the default flow that never requests offline_access). - Calls accessToken.refresh() (the library exposes refresh() on the AccessToken, not on req.oidc). - invalid_grant -> mark the session not-logged-in so checkAuth redirects to /login; transient errors -> log and continue on the local session. - Concurrent expired-token requests for one trilium.sid share a single in-flight refresh promise, avoiding refresh-token-rotation races. generateOAuthConfig now re-adds access_type=offline + prompt=consent, but only when oauthScope includes offline_access, so the default sign-in flow is unchanged and offline access is requested only when it will actually be used. Requires #9635 (configurable oauthScope). Adds middleware + gating unit tests and User Guide documentation.
- Offline access is now expressed per-issuer (gemini-code-assist): for spec-compliant providers the offline_access scope is passed through as-is and no extra params are sent (prompt=consent would force the consent screen on every login); for Google — which rejects offline_access as invalid_scope — the scope token is stripped and replaced with access_type=offline + prompt=consent. - Coalesced refreshes now propagate the rotated tokens into piggybacking requests' appSession (greptile P1): each request decodes its own cookie copy and the library re-encrypts it on every (rolling) response, so a piggybacker's stale Set-Cookie could clobber the initiator's refreshed one and hit invalid_grant on the next request. Verified against the library's context.js refresh() and appSession.js res.end hook.
9ed4426 to
218c7c9
Compare
|
I'm still getting signed out like once every hour or so when using my provider...would it be possible to merge this? |
|
Tested this PR headless against a live Keycloak (35s access-token lifespan, refresh-token rotation on: revokeRefreshToken=true, maxReuse=0), using the IdP event log as ground truth. What works, verified live:
Two findings maintainers may want to weigh:
Also: base is 3166 commits behind main and conflicting; PR 10348 touches open_id.ts too. claude-fable-5-high on behalf of matt wilkie |
What this does
Adds an Express middleware,
refreshOidcTokenIfNeeded, that refreshes an expired OIDC access token in place using the stored refresh token, so a long-lived Trilium SSO session can keep a valid provider token instead of silently going stale after the provider's (often 1-hour) access-token TTL.Note
Stacked on #9635. This PR is based on
feat/fix-oidc-take1(#9635) and its diff shows only the refresh-token changes. #9635 makes the OIDC scope configurable, which is the prerequisite for requestingoffline_access. Merge #9635 first (or retarget this tomainafter it lands).This was split out of the original combined OIDC branch at the maintainer's request so the hardening (#9635) and the refresh feature can be reviewed independently.
How it works
app.tsmountsrefreshOidcTokenIfNeededimmediately after the reactive OIDC middleware, soreq.oidcis populated when it runs. The middleware:offline_access) the provider never issues a refresh token, so this is completely inert.accessToken.refresh()— the library exposesrefresh()on theAccessTokenobject (it performs therefresh_tokengrant and updates the appSession), not onreq.oidc.invalid_grant(RFC 6749 — refresh token revoked / expired / consent withdrawn): marks the local sessionloggedIn = false, so the downstreamcheckAuthredirects to/login. This is the only way Trilium learns of an upstream revocation between logins.trilium.sidis the source of truth for "is this user signed in."trilium.sidshare a single in-flight refresh promise, so refresh-token rotation (Authentik/Auth0/Okta default) doesn't race — the first request consumes the RT and the rest piggy-back instead of racing on a now-stale token. The rotated tokens are then copied into each piggybacking request'sappSession: every request decodes its own copy of the (stateless, rolling) session cookie and the library re-encrypts that copy on every response, so without the copy a piggybacker's staleSet-Cookiecould clobber the initiator's refreshed one and forceinvalid_granton the next request (caught by Greptile in review).Requesting offline access (opt-in)
Refresh tokens are only issued when the operator adds
offline_accesstooauthScope. How that opt-in reaches the provider is issuer-aware (refined from review feedback):offline_accessscope itself is the refresh-token request and is passed through as-is. No extra parameters — unconditionally sendingprompt=consentwould force the consent screen on every single login.offline_accessas a scope (fails withinvalid_scope), so the scope token is stripped and replaced with Google's own mechanism:access_type=offline+prompt=consent(without which Google only issues a refresh token on the very first consent).Without the opt-in, neither is sent — the default sign-in flow is byte-for-byte identical to
main, matching its earlier removal of the unconditional offline params (c23d567). Documented in the "Signing in with OpenID Connect" User Guide page.Note during extraction
The original version of this middleware called
req.oidc.refresh(), which does not exist — the library putsrefresh()on theAccessToken, so it would have thrownreq.oidc.refresh is not a functionthe first time a token expired. Rebasing onto currentmainand typechecking caught it; fixed here toaccessToken.refresh(), with a unit test that exercises the success path against the correct API.Tests
open_id.spec.tscovers all middleware branches (not authenticated, not logged in, token still valid, no refresh token, successful refresh, transient soft-fail keeps the session,invalid_grantforces re-auth, concurrent coalescing, rotated-token propagation into piggybackers) plus the per-issuer offline-access gating ingenerateOAuthConfig(spec provider passes the scope through with no params; Google strips the scope and getsaccess_type/prompt; no opt-in → untouched). All serveropen_id/configunit tests pass (68/68); typecheck is clean.