From 184ff71dee63b7be619b72af2faae6cc625949a2 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Wed, 12 Aug 2026 11:48:11 -0500 Subject: [PATCH] Let an existing account move to SSO, and ask who you are before how Two halves of the same problem: an account created with a password could never use single sign-on, and the login page offered a credential before it knew which one applied. LINKING. Signing in with a provider never adopts an account that already has a password -- that is the takeover the login path exists to refuse. The README promised the way out ("the owner signs in locally and links from Settings") but nothing had ever been built, so the refusal was a dead end rather than a redirection. Settings now has a Sign-in method block: an account with a password can link an instance-wide provider, and one on a provider can unlink back to a password. The account being linked comes from the SIGNED TRANSACTION -- the session that started it -- never from the email in the returned token. That distinction is the whole feature: taking it from the token would be the same email-keyed takeover under a friendlier name. The email must still match the account's own, because login resolves accounts by the asserted address, and one provider subject may not be linked to two accounts. Linking DELETES the password rather than keeping it alongside. One credential at a time, and the confirmation says so in those words, because a password left behind is a second way in that the user believes they replaced. Unlink therefore takes the new password up front and writes it in the SAME statement as the unlink -- never unlink now and set a password after, which leaves an account briefly, or on failure permanently, with no way in. Instance-wide providers only. An organization's provider is chosen by a customer; letting one attach itself to a platform account would hand that customer whatever the account can do. IDENTIFIER-FIRST. The password box now appears only after an address has been submitted, which is what lets the organization lookup happen before a credential is offered: someone whose company requires its own provider is shown that, rather than a password box that will be refused. Editing the address returns to the identifier step so a corrected domain gets a fresh answer. The per-keystroke lookup is gone with it. It answered for half-typed domains, changed the form under someone mid-address, and spent a 10/min per-IP budget on people who had not finished typing -- an office behind one address could exhaust it without a single sign-in attempt. Instance-wide providers stay visible at all times now, by decision: the server refuses them for an SSO-only organization anyway, and hiding them made the page change shape while typing. Verified in a real browser, not only by rendering: password hidden -> submit -> visible and focused -> edit the address -> hidden again, with no page errors. Four mutations of the linking rules fail the tests (account from the email instead of the session, keeping the password, allowing org providers, dropping requireAuth). --- frontend/js/api.js | 3 + frontend/js/i18n/en.js | 29 +++++ frontend/js/views/login.js | 131 ++++++++++++++++----- frontend/js/views/settings.js | 117 ++++++++++++++++++ server/routes/auth.js | 116 ++++++++++++++++-- server/test/login-identifier-first.test.js | 63 ++++++++++ server/test/oidc-account-linking.test.js | 99 ++++++++++++++++ 7 files changed, 521 insertions(+), 37 deletions(-) create mode 100644 server/test/login-identifier-first.test.js create mode 100644 server/test/oidc-account-linking.test.js diff --git a/frontend/js/api.js b/frontend/js/api.js index 112a7f9..574398b 100644 --- a/frontend/js/api.js +++ b/frontend/js/api.js @@ -210,6 +210,9 @@ export const api = { // TOTP 2FA (#100) — opt-in per-user, local accounts only. See routes/auth.js. totpStatus: () => request('/auth/totp/status'), + // Unlink an instance-wide SSO provider. The new password is required in the same call: + // the account must never sit between credentials. + ssoUnlink: (password) => request('/auth/oidc/unlink', { method: 'POST', body: JSON.stringify({ password }) }), totpSetup: () => request('/auth/totp/setup', { method: 'POST' }), totpEnable: (code) => request('/auth/totp/enable', { method: 'POST', body: JSON.stringify({ code }) }), totpDisable: (code) => request('/auth/totp/disable', { method: 'POST', body: JSON.stringify({ code }) }), diff --git a/frontend/js/i18n/en.js b/frontend/js/i18n/en.js index 25fc544..375981c 100644 --- a/frontend/js/i18n/en.js +++ b/frontend/js/i18n/en.js @@ -116,6 +116,8 @@ export default { 'common.unknown': 'Unknown', // Auth (login view) + 'auth.next': 'Next', + 'auth.error_email_required': 'Enter your email address', 'auth.sign_in': 'Sign In', 'auth.sign_out': 'Sign out', 'auth.create_account': 'Create Account', @@ -836,6 +838,33 @@ export default { 'settings.save_profile': 'Save Profile', 'settings.email_alerts': 'Email me when devices go offline', 'settings.change_password': 'Change Password', + // Sign-in method (#258). The link warning is deliberately explicit about destruction of the + // password — that is the part users miss, and it is not reversible without setting a new one. + 'settings.signin_method': 'Sign-in method', + 'settings.signin_password_now': 'This account signs in with a password. You can link it to a single sign-on provider instead.', + 'settings.signin_password_only': 'This account signs in with a password. No single sign-on providers are configured on this server.', + 'settings.signin_link': 'Link {provider}', + 'settings.signin_link_warning': 'You are linking this account to {provider}.\n\nYour local password will be DELETED. After this you sign in with {provider} only.\n\nTo go back to a password later, unlink {provider} and set a new one.', + 'settings.signin_linked': 'This account signs in with {provider}. It has no password.', + 'settings.signin_unlink': 'Unlink {provider}', + 'settings.signin_unlink_desc': 'Set a password to sign in with instead. It takes effect immediately and {provider} is unlinked in the same step.', + 'settings.signin_unlink_confirm': 'Set password and unlink', + 'settings.signin_unlinked_toast': 'Unlinked. You now sign in with your password.', + 'settings.passwords_dont_match': 'The two passwords do not match', + 'settings.signin_linked_toast': 'Linked. You now sign in with {provider}, and your password has been removed.', + 'settings.signin_err_link_email_mismatch': 'That provider account uses a different email address than this account. Sign in to the provider with the same address and try again.', + 'settings.signin_err_link_already_used': 'That provider account is already linked to a different ScreenTinker account.', + 'settings.signin_err_not_linkable': 'Only the providers configured on this server can be linked to an account.', + 'settings.signin_err_no_email': 'The provider did not supply an email address, so the account could not be linked.', + 'settings.signin_err_email_unverified': 'The provider would not confirm that email address is verified.', + 'settings.signin_err_verification_failed': 'The sign-in could not be verified. Nothing was changed.', + 'settings.signin_err_provider_unavailable': 'The provider could not be reached. Nothing was changed.', + 'settings.signin_err_provider_refused': 'The provider refused the request. Nothing was changed.', + 'settings.signin_err_unknown_provider': 'That provider is not configured on this server.', + 'settings.signin_err_expired': 'That took too long. Start the link again.', + 'settings.signin_err_bad_state': 'The response did not match the request. Start the link again.', + 'settings.signin_err_no_code': 'The provider returned no authorization code. Start the link again.', + 'settings.signin_err_server_error': 'Something went wrong. Nothing was changed.', 'settings.password_min_8': 'Must be at least 8 characters.', 'settings.current_password': 'Current Password', 'settings.new_password': 'New Password', diff --git a/frontend/js/views/login.js b/frontend/js/views/login.js index f1585c0..1acf2e2 100644 --- a/frontend/js/views/login.js +++ b/frontend/js/views/login.js @@ -287,7 +287,15 @@ function setupHandlers(config, isSetup) { if (isSetup) { document.getElementById('loginBtn')?.addEventListener('click', () => doRegister(true)); } else { - document.getElementById('loginBtn')?.addEventListener('click', doLogin); + /* + * Identifier-first. The button is "Next" until an address has been submitted: we ask the server + * what that address uses BEFORE offering a credential, so an SSO-only user is never shown a + * password box that is going to be refused, and the org lookup has somewhere to happen. + */ + document.getElementById('loginBtn')?.addEventListener('click', () => { + if (identified && !ssoOnlyDomain) return doLogin(); + identify(); + }); document.getElementById('showRegisterBtn')?.addEventListener('click', () => { document.getElementById('localAuthForm').style.display = 'none'; document.getElementById('registerForm').style.display = 'block'; @@ -304,6 +312,40 @@ function setupHandlers(config, isSetup) { if (e.key === 'Enter') isSetup ? doRegister(true) : doLogin(); }); + /* + * Enter in the EMAIL field advances rather than submitting. During first-run setup both fields + * are needed at once, so identifier-first is skipped entirely there. + */ + document.getElementById('loginEmail')?.addEventListener('keydown', (e) => { + if (e.key !== 'Enter') return; + if (isSetup) return doRegister(true); + if (identified && !ssoOnlyDomain) return doLogin(); + identify(); + }); + + /* + * Editing the address after identifying returns to the identifier step. Someone who mistypes + * their domain must get a fresh answer rather than keep the previous domain's one. + */ + document.getElementById('loginEmail')?.addEventListener('input', () => { + if (!identified) return; + identified = false; + applyFormState(); + }); + + /* + * Ask what this address uses, then show the right thing. The lookup itself sets ssoOnlyDomain via + * setPasswordVisible(), so this only has to decide that we now know who is signing in. + */ + async function identify() { + const email = document.getElementById('loginEmail').value.trim(); + if (!email || !email.includes('@')) { showError(t('auth.error_email_required')); return; } + try { await lookupOrgSso(email); } catch { /* lookup failures fall through to the password box */ } + identified = true; + applyFormState(); + if (!ssoOnlyDomain) document.getElementById('loginPassword')?.focus(); + } + async function doLogin() { const email = document.getElementById('loginEmail').value.trim(); const password = document.getElementById('loginPassword').value; @@ -509,7 +551,6 @@ function setupHandlers(config, isSetup) { * Debounced because this fires while someone types, and the endpoint is rate limited; asking on * every keystroke would spend a user's whole budget before they finished their own address. */ - let ssoLookupTimer = null; let lastDomainAsked = ''; const orgSlot = () => document.getElementById('orgSsoSlot'); @@ -520,44 +561,68 @@ function setupHandlers(config, isSetup) { * on every negative answer matters as much as hiding it: someone who types an SSO-only address, * then corrects it to their own, must get the password box back. */ - function setPasswordVisible(visible) { + /* + * Password visibility has TWO independent drivers, and conflating them is how this got confusing: + * + * identified — identifier-first. The password box does not exist until an address has been + * submitted, because until then we do not know whether this account uses a + * password at all. This is what lets the org lookup happen before we offer the + * wrong thing. + * ssoOnlyDomain — the address belongs to an organization that REQUIRES its own provider. Then a + * password box is not merely going to fail, it is the wrong thing to show. + * + * The field appears only when identified AND not SSO-only. Kept as one function so the two can + * never disagree about what is on screen. + */ + let identified = false; + let ssoOnlyDomain = false; + + function applyFormState() { + const showPassword = identified && !ssoOnlyDomain; + const show = showPassword ? '' : 'none'; /* * ⚠️ Hide the password FIELD, never its .form-group — the organization SSO slot lives inside - * that same group, so hiding the container took the single sign-on button down with it and left - * a login page whose only action was "Create Account". Found by looking at a screenshot. + * that same group, so hiding the container took the single sign-on button down with it. */ - const show = visible ? '' : 'none'; - for (const id of ['loginPassword', 'loginPasswordLabel', 'loginBtn']) { + for (const id of ['loginPassword', 'loginPasswordLabel']) { const el = document.getElementById(id); if (el) el.style.display = show; } + /* - * The instance's own providers go too. They are the operator's, not this organization's, and - * they are not domain-confined — so offering "Continue with Google" to someone whose company - * requires its own identity provider is offering them the bypass. The server refuses it either - * way; this stops the page inviting it. + * The primary button is "Next" until an address has been submitted, then "Sign in". One button + * rather than two, so there is never a choice about which to press. */ - const instance = document.getElementById('instanceProviders'); - if (instance) instance.style.display = show; + const btn = document.getElementById('loginBtn'); + if (btn) btn.textContent = identified && !ssoOnlyDomain ? t('auth.sign_in') : t('auth.next'); + if (btn) btn.style.display = ssoOnlyDomain ? 'none' : ''; + /* - * "Create Account" goes too. Registration at an SSO-only domain is refused by the server, and - * leaving the button was worse than useless: it was the ONLY action left on the card, so the - * page invited the one thing that cannot work. + * The instance's own providers stay visible at ALL times, by explicit decision: they are the + * operator's, they are offered to everyone, and the server refuses them for an SSO-only + * organization anyway. (Previously they were hidden for such domains so the page would not + * invite the bypass; the cost was a login page that changed shape while you typed.) + */ + + /* + * "Create Account" and "Forgot your password?" DO go for an SSO-only domain: registration there + * is refused by the server, and a password reset produces one that can never be used. */ const reg = document.getElementById('showRegisterBtn'); - if (reg) reg.style.display = show; - // The OR divider sits outside #instanceProviders, so hiding those alone left a dangling rule - // with nothing beneath it. - const divider = document.getElementById('ssoDivider'); - if (divider) divider.style.display = show; - // "Forgot your password?" sits in its own

; hide the wrapper so no empty gap is left. + if (reg) reg.style.display = ssoOnlyDomain ? 'none' : ''; const forgot = document.getElementById('forgotLink'); if (forgot) { const wrap = forgot.parentElement && forgot.parentElement.tagName === 'P' ? forgot.parentElement : forgot; - wrap.style.display = show; + wrap.style.display = ssoOnlyDomain ? 'none' : ''; } } + // Kept for the org lookup below, which reasons about SSO-only rather than about identification. + function setPasswordVisible(visible) { + ssoOnlyDomain = !visible; + applyFormState(); + } + async function lookupOrgSso(email) { const at = String(email || '').lastIndexOf('@'); const domain = at === -1 ? '' : email.slice(at + 1).trim().toLowerCase(); @@ -650,11 +715,21 @@ function setupHandlers(config, isSetup) { } } - document.getElementById('loginEmail')?.addEventListener('input', (e) => { - clearTimeout(ssoLookupTimer); - const value = e.target.value; - ssoLookupTimer = setTimeout(() => lookupOrgSso(value), 400); - }); + /* + * The lookup now runs on SUBMIT (identify()), not on every keystroke. + * + * Identifier-first made the debounced version both redundant and wrong: redundant because nothing + * is shown until an address is submitted anyway, and wrong because it would answer for a + * half-typed domain and change the form under someone mid-address. It also spent a rate-limit + * budget of 10/min per IP on people who had not finished typing — an office behind one address + * could exhaust it without a single sign-in attempt. + * + * ⚠️ Applied HERE, after the `let identified` / `let ssoOnlyDomain` declarations above. Called any + * earlier it would throw on the temporal dead zone, which on this page means a login form that + * never renders. + */ + if (isSetup) identified = true; // first-run setup needs both fields at once + applyFormState(); /* * Completing an SSO login. diff --git a/frontend/js/views/settings.js b/frontend/js/views/settings.js index 7a01f6d..5819cb0 100644 --- a/frontend/js/views/settings.js +++ b/frontend/js/views/settings.js @@ -62,6 +62,16 @@ export async function render(container) {

${t('settings.sso_note', { provider: esc(user.auth_provider || 'SSO') })}

`} + +
+

${t('settings.signin_method')}

+

…

+
+

${t('settings.2fa_title')}

@@ -526,6 +536,87 @@ export async function render(container) { // ==================== Two-factor authentication (#100) ==================== // Drives the merged TOTP backend (/api/auth/totp/*). Re-renders #twoFactorBlock // for each state: SSO note / disabled+enroll / recovery-codes / enabled+manage. + /* + * Sign-in method: password OR one instance-wide provider, never both. + * + * The warning on the link button is the whole UX: the local password is DELETED, not kept as a + * fallback, and someone who does not read that will think they gained a second way in. Unlink + * asks for the new password up front for the same reason — the account must never sit between + * credentials. + * + * Only instance-wide providers appear. An organization's provider is chosen by a customer and + * must not be attachable to a platform account; the server refuses it too. + */ + async function loadSsoLink() { + const block = document.getElementById('ssoLinkBlock'); + if (!block) return; + const head = `

${t('settings.signin_method')}

`; + const muted = 'color:var(--text-muted);font-size:12px'; + const paint = (inner) => { block.innerHTML = head + inner; }; + + let me; + try { me = await api.getMe(); } + catch (e) { paint(`

${esc(e.message)}

`); return; } + + let providers = []; + try { + const res = await fetch('/api/auth/providers'); + if (res.ok) providers = (await res.json()).providers || []; + } catch { /* offline: fall through to the no-providers copy */ } + + if (me.auth_provider && me.auth_provider !== 'local') { + const name = providers.find((p) => p.slug === me.auth_provider)?.name || me.auth_provider; + paint(` +

${t('settings.signin_linked', { provider: esc(name) })}

+ + + `); + document.getElementById('unlinkBtn').onclick = () => { + document.getElementById('unlinkForm').style.display = ''; + document.getElementById('unlinkBtn').style.display = 'none'; + document.getElementById('unlinkPw').focus(); + }; + document.getElementById('unlinkConfirmBtn').onclick = async () => { + const pw = document.getElementById('unlinkPw').value; + const pw2 = document.getElementById('unlinkPw2').value; + if (pw !== pw2) return showToast(t('settings.passwords_dont_match'), 'error'); + try { + await api.ssoUnlink(pw); + showToast(t('settings.signin_unlinked_toast'), 'success'); + loadSsoLink(); + } catch (e) { showToast(e.message, 'error'); } + }; + return; + } + + if (!providers.length) { + paint(`

${t('settings.signin_password_only')}

`); + return; + } + paint(` +

${t('settings.signin_password_now')}

+
+ ${providers.map((p) => ``).join('')} +
+ `); + block.querySelectorAll('[data-link-slug]').forEach((btn) => { + btn.onclick = () => { + const slug = btn.dataset.linkSlug; + const name = providers.find((p) => p.slug === slug)?.name || slug; + // Deliberately blunt: the password is destroyed, and that is the part people miss. + if (!window.confirm(t('settings.signin_link_warning', { provider: name }))) return; + window.location.href = `/api/auth/oidc/${encodeURIComponent(slug)}/link/start`; + }; + }); + } + async function load2FA() { const block = document.getElementById('twoFactorBlock'); if (!block) return; @@ -660,6 +751,32 @@ export async function render(container) { loadTokens(); load2FA(); + loadSsoLink(); + + /* + * Report the outcome of a link round trip. + * + * The callback returns to #/settings rather than the login page — an authenticated user bounced + * to a login screen to be told "that did not work" reads as having been signed out. Params are + * stripped afterwards so a refresh or a copied URL does not replay the message. + */ + (function reportLinkOutcome() { + const q = new URLSearchParams((location.hash.split('?')[1] || '')); + const linked = q.get('sso_linked'); + const err = q.get('sso_error'); + if (!linked && !err) return; + if (linked) { + showToast(t('settings.signin_linked_toast', { provider: linked }), 'success'); + } else { + const known = ['link_email_mismatch', 'link_already_used', 'not_linkable', 'no_email', + 'email_unverified', 'verification_failed', 'provider_unavailable', 'provider_refused', + 'unknown_provider', 'expired', 'bad_state', 'no_code', 'server_error']; + const key = known.includes(err) ? `settings.signin_err_${err}` : 'auth.sso_failed'; + showToast(t(key), 'error'); + } + history.replaceState(null, '', location.pathname + location.search + '#/settings'); + loadSsoLink(); + }()); // #73: agency scope reveals a playlist picker (the token's allowlist). Loaded lazily once. const tokScopeSel = document.getElementById('tokScope'); diff --git a/server/routes/auth.js b/server/routes/auth.js index 6b7fa53..1155179 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -1158,6 +1158,13 @@ function backToApp(res, params) { res.redirect(`/app#/login?${qs}`); } +// A link attempt starts from Settings while signed in, so it must end there — bouncing an +// authenticated user to the login page to report the outcome reads as "you were signed out". +function backToSettings(res, params) { + const qs = new URLSearchParams(params).toString(); + res.redirect(`/app#/settings?${qs}`); +} + // Which providers this instance offers. Public: it is what draws the login buttons. router.get('/providers', (req, res) => { res.json({ providers: oidcProviders.publicList() }); @@ -1233,10 +1240,15 @@ router.post('/sso/start', express.urlencoded({ extended: false }), (req, res) => res.redirect(startUrl); }); -router.get('/oidc/:slug/start', asyncRoute(async (req, res) => { - const provider = oidcProviders.get(req.params.slug); - if (!provider) return backToApp(res, { sso_error: 'unknown_provider' }); - +/** + * Begin an OIDC round trip. + * + * `extra` is merged into the signed transaction, which is how LINK mode is carried: the tx is + * server-signed and lives in an httpOnly cookie, so the browser can neither read nor forge which + * account a link is for. Login and link therefore share one flow — the same PKCE, state, nonce and + * verification — instead of a second copy that drifts. + */ +async function beginOidc(req, res, provider, extra = {}, onError = backToApp) { try { const doc = await oidc.discover(provider.issuer); const pkce = oidc.createPkce(); @@ -1244,7 +1256,7 @@ router.get('/oidc/:slug/start', asyncRoute(async (req, res) => { const state = oidc.randomToken(); const tx = jwt.sign( - { typ: 'oidc-tx', slug: provider.slug, nonce, verifier: pkce.verifier, state }, + { typ: 'oidc-tx', slug: provider.slug, nonce, verifier: pkce.verifier, state, ...extra }, config.jwtSecret, // HS256 explicitly, and a `typ` the session verifier does not accept: two token kinds signed // with one secret must never be interchangeable, even if today only `slug` happens to stop it. @@ -1269,9 +1281,61 @@ router.get('/oidc/:slug/start', asyncRoute(async (req, res) => { url.searchParams.set('code_challenge_method', pkce.method); res.redirect(url.toString()); } catch (err) { - console.error(`[oidc] ${req.params.slug} start failed:`, err.message); - backToApp(res, { sso_error: 'provider_unavailable' }); + console.error(`[oidc] ${provider.slug} start failed:`, err.message); + onError(res, { sso_error: 'provider_unavailable' }); } +} + +router.get('/oidc/:slug/start', asyncRoute(async (req, res) => { + const provider = oidcProviders.get(req.params.slug); + if (!provider) return backToApp(res, { sso_error: 'unknown_provider' }); + await beginOidc(req, res, provider); +})); + +/* + * Link an EXISTING account to an instance-wide provider. + * + * Signing in with a provider never adopts an account that has a password — that would let anyone who + * can make a provider assert an address inherit the account behind it. So the owner proves they are + * the owner first, by being signed in, and starts the link themselves. The account is taken from the + * SESSION, never from the email in the returned token. + * + * ⚠️ INSTANCE-WIDE PROVIDERS ONLY. An organization's provider is chosen by a customer; letting one + * attach itself to a platform account would hand that customer whatever the account can do. Org + * membership arrives through the normal org SSO path, which is domain-confined. + */ +/* + * Unlink, and set a password in the SAME operation. + * + * Not two steps. An account whose only credential is a provider has nothing to fall back on the + * moment that link is removed, so "unlink now, set a password next" leaves a window — and a failure + * in between leaves an account nobody can sign into at all. The new password is therefore required + * up front and written in one transaction with the unlink. + */ +router.post('/oidc/unlink', requireAuth, (req, res) => { + const password = String((req.body || {}).password || ''); + const user = db.prepare('SELECT id, email, auth_provider, password_hash FROM users WHERE id = ?').get(req.user.id); + if (!user) return res.status(404).json({ error: 'Account not found' }); + if (user.auth_provider === 'local') { + return res.status(400).json({ error: 'This account already signs in with a password' }); + } + if (password.length < passwordReset.MIN_PASSWORD_LENGTH) { + return res.status(400).json({ error: `Password must be at least ${passwordReset.MIN_PASSWORD_LENGTH} characters` }); + } + + const was = user.auth_provider; + db.prepare("UPDATE users SET auth_provider = 'local', provider_id = NULL, password_hash = ? WHERE id = ?") + .run(bcrypt.hashSync(password, 10), user.id); + logActivity(user.id, 'auth:sso_unlinked', `was ${was}`, null, getClientIp(req)); + console.log(`[oidc] ${was} unlinked from ${user.email} (password set)`); + res.json({ ok: true, auth_provider: 'local' }); +}); + +router.get('/oidc/:slug/link/start', requireAuth, asyncRoute(async (req, res) => { + const provider = oidcProviders.get(req.params.slug); + if (!provider) return backToSettings(res, { sso_error: 'unknown_provider' }); + if (provider.organizationId) return backToSettings(res, { sso_error: 'not_linkable' }); + await beginOidc(req, res, provider, { link: req.user.id }, backToSettings); })); router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => { @@ -1338,7 +1402,9 @@ router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => { } const email = String(claims.email || '').toLowerCase().trim(); - if (!email) return backToApp(res, { sso_error: 'no_email' }); + const linking = !!tx.link; + const fail = linking ? backToSettings : backToApp; + if (!email) return fail(res, { sso_error: 'no_email' }); /* * ⚠️ AN ORGANIZATION'S PROVIDER MAY ONLY SPEAK FOR ITS OWN DOMAINS. @@ -1387,7 +1453,39 @@ router.get('/oidc/:slug/callback', asyncRoute(async (req, res) => { // explicit false is still refused, and an org-configured provider still cannot assume anything. // See oidcProviders.emailIsVerified() for why that division is the safe one. if (!oidcProviders.emailIsVerified(claims, provider)) { - return backToApp(res, { sso_error: 'email_unverified' }); + return fail(res, { sso_error: 'email_unverified' }); + } + + /* + * LINK: attach this provider to the account that STARTED the link, and drop its password. + * + * The account comes from the signed transaction (i.e. from the session that began this), never + * from the returned email — otherwise "linking" would be the very email-keyed takeover the login + * path refuses. The email must still match the account's own, because login resolves an account by + * the address the provider asserts: linking a different address would produce an account that + * cannot be signed into, or would collide with someone else's. + * + * The password is DELETED rather than kept alongside. One credential at a time is the whole point + * — a password left behind is a second way in that the user believes they replaced. + */ + if (linking) { + const target = db.prepare('SELECT id, email, auth_provider FROM users WHERE id = ?').get(tx.link); + if (!target) return backToSettings(res, { sso_error: 'server_error' }); + if (target.email.toLowerCase() !== email) { + console.warn(`[oidc] link refused: ${provider.slug} asserted ${email} for account ${target.email}`); + return backToSettings(res, { sso_error: 'link_email_mismatch' }); + } + // Someone else already signed in with this provider identity. Two accounts must never share one + // provider subject, or whoever signs in second silently takes the first one's place. + const taken = db.prepare('SELECT id FROM users WHERE provider_id = ? AND auth_provider = ? AND id != ?') + .get(String(claims.sub), provider.slug, target.id); + if (taken) return backToSettings(res, { sso_error: 'link_already_used' }); + + db.prepare('UPDATE users SET auth_provider = ?, provider_id = ?, password_hash = NULL, avatar_url = COALESCE(?, avatar_url) WHERE id = ?') + .run(provider.slug, String(claims.sub), claims.picture || null, target.id); + logActivity(target.id, 'auth:sso_linked', `provider=${provider.slug}`, null, getClientIp(req)); + console.log(`[oidc] ${provider.slug} linked to ${target.email} (password cleared)`); + return backToSettings(res, { sso_linked: provider.slug }); } try { diff --git a/server/test/login-identifier-first.test.js b/server/test/login-identifier-first.test.js new file mode 100644 index 0000000..3a8d872 --- /dev/null +++ b/server/test/login-identifier-first.test.js @@ -0,0 +1,63 @@ +'use strict'; + +/* + * Identifier-first login (#258). + * + * The password box does not exist until an address has been submitted. That is what lets the + * organization lookup happen BEFORE a credential is offered, so someone whose company requires its + * own identity provider is never shown a password box that is going to be refused. + * + * Verified in a real browser as well (password hidden -> submit -> visible + focused -> edit the + * address -> hidden again); these assertions stop the wiring being removed silently. + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const LOGIN = fs.readFileSync(path.join(__dirname, '..', '..', 'frontend', 'js', 'views', 'login.js'), 'utf8'); + +test('password visibility depends on BOTH identification and SSO-only', () => { + assert.match(LOGIN, /const showPassword = identified && !ssoOnlyDomain;/, + 'the two drivers must be combined in one place so they cannot disagree'); +}); + +test('the primary button advances before it signs in', () => { + assert.match(LOGIN, /if \(identified && !ssoOnlyDomain\) return doLogin\(\);\s*\n\s*identify\(\);/, + 'the button must identify first and only sign in once an address is known'); + assert.match(LOGIN, /btn\.textContent = identified && !ssoOnlyDomain \? t\('auth\.sign_in'\) : t\('auth\.next'\)/); +}); + +test('editing the address returns to the identifier step', () => { + assert.match(LOGIN, /if \(!identified\) return;\s*\n\s*identified = false;/, + 'a corrected address must get a fresh answer, not the previous domain\'s'); +}); + +test('the per-keystroke lookup is gone', () => { + assert.doesNotMatch(LOGIN, /ssoLookupTimer/, + 'the debounced lookup answered for half-typed domains and burned a 10/min budget'); + assert.match(LOGIN, /async function identify\(\)[\s\S]{0,400}await lookupOrgSso\(email\)/, + 'the lookup now runs on submit'); +}); + +test('instance-wide providers are never hidden', () => { + // Deliberate: they are the operator's, offered to everyone, and the server refuses them for an + // SSO-only organization anyway. Hiding them made the page change shape while typing. + assert.doesNotMatch(LOGIN, /getElementById\('instanceProviders'\)[\s\S]{0,120}style\.display/, + 'nothing may hide #instanceProviders'); +}); + +test('first-run setup skips identifier-first', () => { + assert.match(LOGIN, /if \(isSetup\) identified = true;/, + 'creating the first admin needs both fields at once'); +}); + +test('the initial state is applied after its declarations (temporal dead zone)', () => { + const decl = LOGIN.indexOf('let identified = false;'); + const call = LOGIN.lastIndexOf('\n applyFormState();'); + assert.ok(decl !== -1 && call !== -1, 'both the declaration and the init call must exist'); + assert.ok(call > decl, + 'applyFormState() must be called AFTER the let declarations — earlier throws on the TDZ, which ' + + 'on this page means a login form that never renders'); +}); diff --git a/server/test/oidc-account-linking.test.js b/server/test/oidc-account-linking.test.js new file mode 100644 index 0000000..a128e64 --- /dev/null +++ b/server/test/oidc-account-linking.test.js @@ -0,0 +1,99 @@ +'use strict'; + +/* + * Linking an existing account to an instance-wide provider (#258). + * + * Signing in with a provider never adopts an account that already has a password — that is the + * takeover the login path exists to refuse. The README promised an escape hatch ("the owner signs + * in locally and links from Settings") that was never built, so an account created with a password + * could never use SSO at all. + * + * The rules this pins down, all of which are load-bearing: + * - the account being linked comes from the SIGNED TRANSACTION (i.e. the session that started the + * link), never from the email in the returned token. Otherwise "linking" is the same email-keyed + * takeover under a friendlier name; + * - the provider's email must still equal the account's, because login resolves accounts by the + * asserted address; + * - one provider subject may not be linked to two accounts; + * - linking DELETES the password: one credential at a time; + * - unlinking SETS a password in the same statement, so the account is never between credentials; + * - ORG providers are not linkable at all. + */ + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const AUTH = fs.readFileSync(path.join(__dirname, '..', 'routes', 'auth.js'), 'utf8'); + +/** Body of a route handler, from its `router.(''` to the next `router.`. */ +function handler(verb, route) { + const start = AUTH.indexOf(`router.${verb}('${route}'`); + assert.notEqual(start, -1, `route ${verb.toUpperCase()} ${route} not found`); + const rest = AUTH.slice(start + 1); + const end = rest.indexOf('\nrouter.'); + return end === -1 ? rest : rest.slice(0, end); +} + +test('link start requires authentication and refuses org providers', () => { + const body = handler('get', '/oidc/:slug/link/start'); + assert.match(AUTH, /router\.get\('\/oidc\/:slug\/link\/start', requireAuth/, + 'the link must be startable only by someone already signed in — that is the proof of ownership'); + assert.match(body, /provider\.organizationId.*not_linkable/s, + "an organization's provider must never attach itself to a platform account"); + assert.match(body, /link: req\.user\.id/, + 'the account must come from the session, not from anything the browser can set'); +}); + +test('the linked account is taken from the transaction, never from the returned email', () => { + const cb = handler('get', '/oidc/:slug/callback'); + assert.match(cb, /WHERE id = \?'\)\.get\(tx\.link\)/, + 'the target account is looked up by tx.link (the session that started it)'); + // The email is still checked, but as a constraint on the link — not as the way the account is found. + assert.match(cb, /target\.email\.toLowerCase\(\) !== email/, 'email must match the account being linked'); + assert.match(cb, /link_email_mismatch/); +}); + +test('one provider subject cannot be linked to two accounts', () => { + const cb = handler('get', '/oidc/:slug/callback'); + assert.match(cb, /provider_id = \? AND auth_provider = \? AND id != \?/, + 'must check whether this provider identity already belongs to another account'); + assert.match(cb, /link_already_used/); +}); + +test('linking deletes the password — one credential at a time', () => { + const cb = handler('get', '/oidc/:slug/callback'); + assert.match(cb, /UPDATE users SET auth_provider = \?, provider_id = \?, password_hash = NULL/, + 'the password must be cleared in the same statement that attaches the provider'); +}); + +test('unlinking sets a password in the SAME statement', () => { + const body = handler('post', '/oidc/unlink'); + assert.match(body, /UPDATE users SET auth_provider = 'local', provider_id = NULL, password_hash = \?/, + 'unlink and set-password must be one write — never unlink first and set a password after'); + assert.match(body, /password\.length < passwordReset\.MIN_PASSWORD_LENGTH/, + 'the replacement password must meet the same minimum as a reset'); + assert.match(body, /auth_provider === 'local'/, 'refuse unlinking an account that has no provider'); +}); + +test('both link and unlink are recorded in the activity log', () => { + assert.match(handler('get', '/oidc/:slug/callback'), /logActivity\([^)]*'auth:sso_linked'/); + assert.match(handler('post', '/oidc/unlink'), /logActivity\([^)]*'auth:sso_unlinked'/); +}); + +test('link failures return to Settings, not the login page', () => { + const cb = handler('get', '/oidc/:slug/callback'); + assert.match(cb, /const fail = linking \? backToSettings : backToApp/, + 'an authenticated user must not be bounced to a login screen to be told the link failed'); + assert.match(AUTH, /function backToSettings\(res, params\)[\s\S]{0,200}#\/settings/); +}); + +test('login and link share one flow, so verification cannot drift between them', () => { + // beginOidc is the single place PKCE/state/nonce are minted; both entry points call it. + assert.match(AUTH, /async function beginOidc\(req, res, provider, extra = \{\}/); + const login = handler('get', '/oidc/:slug/start'); + const link = handler('get', '/oidc/:slug/link/start'); + assert.match(login, /beginOidc\(req, res, provider\)/); + assert.match(link, /beginOidc\(req, res, provider, \{ link: req\.user\.id \}/); +});