Delay SSO initialization , until user clicks on the GUI.#1131
Delay SSO initialization , until user clicks on the GUI.#1131vanmilleru wants to merge 5 commits intoblinkospace:mainfrom
Conversation
DLHTX
left a comment
There was a problem hiding this comment.
Code Review
The approach here is solid — deferring SSO initialization until the user actually clicks login addresses #1004 and #1086. However, there are two issues that need to be fixed:
1. Import name mismatch (will cause a runtime error)
server/routerExpress/auth/index.ts line 2:
import passport, { ensureOAuthStrategiesInitialized } from './config';But the exported function in config.ts is named ensureOAuthStrategies, not ensureOAuthStrategiesInitialized. This will fail at import time.
Fix: Change to import passport, { ensureOAuthStrategies } from './config'; and update all call sites accordingly.
2. facebook / twitter / discord routes are not handled
The PR only adds lazy initialization to /github, /google, /callback/:providerId, and /:providerId, but these routes still call passport.authenticate() directly without ensuring strategies are initialized:
router.get('/facebook', logOAuthRequest('Facebook'), passport.authenticate('facebook', { scope: ['email'] }));
router.get('/twitter', logOAuthRequest('Twitter'), passport.authenticate('twitter'));
router.get('/discord', logOAuthRequest('Discord'), passport.authenticate('discord', { scope: ['identify', 'email'] }));If a user configures any of these providers, clicking login will fail because the strategy hasn't been initialized yet.
Fix: These three routes also need to be converted to async handlers with await ensureOAuthStrategies() before calling passport.authenticate().
Happy to approve once these are addressed 👍
blinko-space
left a comment
There was a problem hiding this comment.
Review: A few small things 🔧
Nice fix — this addresses a real boot-time problem (#1004, #1086) where an unreachable SSO provider blocks server startup. The lazy-init pattern is the right approach.
What's good
- ✓
ensureOAuthStrategies()is properly idempotent: tracksoauthStrategiesInitializedAND de-dupes concurrent callers via the sharedoauthInitializationPromise. The race is safe because JS is single-threaded between theif (!oauthInitializationPromise)check and the assignment. - ✓
.finally()clears the promise so a failed init can be retried on the next request — good recovery semantics. - ✓
reinitializeOAuthStrategiesresets both gates before re-init — keeps the lazy-init contract intact through reconfiguration. - ✓ Removing
await initOAuthStrategies()fromconfigureSessionis the actual fix for the boot hang.
Things to address before merge
-
Copy-paste typo on the Discord handler (
server/routerExpress/auth/index.ts:74) — the log message says'twitter authentication route accessed'instead of'discord ...'. Easy fix, but it'll confuse anyone debugging Discord SSO. -
Indentation is off in two places (lints would catch this):
server/routerExpress/auth/index.ts:59— facebook handler has a 3-space indent onpassport.authenticate(...)and a stray blank line before});.server/routerExpress/auth/index.ts:222—:providerIdhandler has a 4-space indent onawait ensureOAuthStrategies();(should be 2).
Suggestions (not blockers)
-
Error path: route handlers do
await ensureOAuthStrategies()without try/catch. If init throws (e.g., transient DNS failure on the OAuth provider), Express returns a 500 to the user instead of a friendly redirect. Worth wrapping with a try/catch that responds with a redirect to the login page + an error toast param, since the whole point of this PR is graceful handling of unreachable providers. -
Tests: an integration test covering "server boots with unreachable SSO" + "first OAuth request triggers init" + "concurrent requests de-dup" would lock in the contract this PR establishes. Not a hard requirement for this PR.
Once the typo and indentation are fixed I'll re-approve. 🚀
|
@blinko-space done. @blinko-space done. |
fixing #1004 #1086