diff --git a/server/db/database.js b/server/db/database.js index 1c6ff1c..bd5a087 100644 --- a/server/db/database.js +++ b/server/db/database.js @@ -482,7 +482,6 @@ const migrations = [ * self-service switch gets flipped under pressure. So removal goes through the operator: the * request is recorded here and a platform admin has to approve it. */ - "ALTER TABLE organizations ADD COLUMN sso_only INTEGER NOT NULL DEFAULT 0", `CREATE TABLE IF NOT EXISTS org_sso_only_requests ( id TEXT PRIMARY KEY, organization_id TEXT NOT NULL, @@ -959,6 +958,27 @@ migrateGroupSchedules(); // updates workspace_id. ensureMultitenancyMigration(); +/* + * `organizations.sso_only` — added HERE, not in the migrations array above. + * + * That array runs BEFORE ensureMultitenancyMigration(), which is what creates the organizations + * table, so on a fresh install the ALTER hit a table that did not exist yet: `[migrate] FAILED … + * no such table: organizations`, one console.error among ~85 migration lines. The instance then + * ran its entire first boot with the SSO settings screen 500ing and — far worse — + * ssoOnlyForEmail() catching `no such column` and returning "not SSO-only", which is password + * login proceeding for an organization that had switched it off. It self-healed on the second + * boot, which is exactly what makes it easy to miss. + */ +try { + const orgCols = db.prepare('PRAGMA table_info(organizations)').all().map((c) => c.name); + if (orgCols.length && !orgCols.includes('sso_only')) { + db.exec('ALTER TABLE organizations ADD COLUMN sso_only INTEGER NOT NULL DEFAULT 0'); + console.log('[migrate] added organizations.sso_only'); + } +} catch (e) { + console.error('[migrate] could not add organizations.sso_only:', e.message); +} + // Phase 2.2c migration: backfill content_folders.workspace_id from owner's // default workspace. The ALTER lives in the migrations array above; this // one-shot populates the column for any rows that pre-date it. diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index a28a598..e4b51e8 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -321,11 +321,64 @@ function ssoOnlyForEmail(email) { WHERE d.domain = ? AND d.verified_at IS NOT NULL AND p.enabled = 1 AND o.sso_only = 1 `).get(domain) || null; } catch (e) { - if (/no such table|no such column/i.test(e.message)) return null; + /* + * ⚠️ FAIL CLOSED. This used to swallow `no such column` and return null — and null means "not + * SSO-only", i.e. password login proceeds. It is the single control stopping a password from + * bypassing a customer's identity provider, so a schema problem must never be the thing that + * quietly switches it off. The sibling forEmail() carries the same warning for the same reason. + * + * `no such table` on the DOMAINS table is different and genuinely means "this instance has no + * per-org SSO at all", so it stays a null. + */ + /* + * "The feature is not installed" and "the schema drifted" are different answers. + * + * A missing per-org SSO table, or no organizations table at all, means this instance has no + * per-organization SSO — nothing is being bypassed, so null is correct and a single-tenant + * install must keep working. A missing sso_only COLUMN on a table that does exist is drift, and + * that is the case that must never quietly answer "not required". + */ + if (/no such table: (org_sso_domains|org_sso_providers|organizations|organization_members)/i.test(e.message)) return null; + console.error('[sso] could not determine SSO-only status, refusing password login:', e.message); throw e; } } +/** + * Must THIS USER use single sign-on? + * + * ⚠️ Membership, not just the address. ssoOnlyForEmail() answers about a DOMAIN, and a review used + * that gap to walk straight in: any account in the tenant whose address sits outside the verified + * domains kept password login — a contractor, an MSP, the one address nobody remembered. Worse, it + * could be manufactured on demand, because an org admin can create a local password account at any + * address and bind it to their workspace. Enforcing on the domain alone protects the domain; it + * does not protect the ORGANIZATION, which is what the setting claims to do. + * + * So both are asked: the address's domain (which catches people who are not members yet) and every + * organization the user actually belongs to. + */ +function ssoOnlyForUser(user) { + if (!user) return null; + const byDomain = ssoOnlyForEmail(user.email); + if (byDomain) return byDomain; + + const conn = db(); + if (!conn) return null; + try { + return conn.prepare(` + SELECT o.id AS organization_id, o.name AS organization_name + FROM organization_members m + JOIN organizations o ON o.id = m.organization_id + WHERE m.user_id = ? AND o.sso_only = 1 + LIMIT 1 + `).get(user.id) || null; + } catch (e) { + if (/no such table: (organization_members|organizations)/i.test(e.message)) return null; + throw e; // drift on a table that exists — fail closed; the caller refuses the login + } +} + module.exports = { - list, get, publicList, getOrgProvider, ownerOf, forEmail, ssoOnlyForEmail, DEFAULT_SCOPES, SLUG_RE, + list, get, publicList, getOrgProvider, ownerOf, forEmail, + ssoOnlyForEmail, ssoOnlyForUser, DEFAULT_SCOPES, SLUG_RE, }; diff --git a/server/routes/admin.js b/server/routes/admin.js index ebf05a3..7701935 100644 --- a/server/routes/admin.js +++ b/server/routes/admin.js @@ -55,6 +55,32 @@ router.post('/users', (req, res) => { if (!canAdminWorkspace(db, req.user, ws)) { return res.status(403).json({ error: 'Admin access required' }); } + /* + * ⚠️ An SSO-only organization must not have password accounts minted into it. + * + * This route creates a LOCAL account with an admin-chosen password, and it accepts any address — + * so on a tenant that requires single sign-on it was a one-call backdoor: create + * `contractor@somewhere-else.test` bound to the workspace, log in with the password, and every + * control the customer turned SSO-only on for is behind you. A review did exactly that, and the + * account it created could then mint another. + * + * platform_admin keeps the ability, because that is the operator break-glass — the same + * exemption the login gate makes, for the same reason. + */ + if (req.user.role !== 'platform_admin' && ws.organization_id) { + // The table is absent on a single-tenant install; that simply means no organization requires + // single sign-on, so creation proceeds. + let org = null; + try { org = db.prepare('SELECT sso_only, name FROM organizations WHERE id = ?').get(ws.organization_id); } + catch { org = null; } + if (org && org.sso_only) { + return res.status(400).json({ + error: `${org.name || 'This organization'} requires single sign-on, so password accounts cannot be created. Invite the person through your identity provider instead.`, + code: 'sso_only_org', + }); + } + } + // Stamp the target workspace so the activityLogger middleware (and our // explicit audit row) attribute to the right tenant. req.workspaceId = ws.id; diff --git a/server/routes/auth.js b/server/routes/auth.js index 8bf3693..4e2bc46 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -188,7 +188,21 @@ router.post('/login', (req, res) => { * already answered `sso: true` publicly, so naming it reveals nothing new. */ if (user.role !== 'platform_admin') { - const enforced = oidcProviders.ssoOnlyForEmail(user.email); + /* + * A throw here means we could not determine the answer (schema drift, a broken read). Treat + * that as "SSO is required" rather than letting a 500 escape or, worse, letting the login + * through: the whole point of this gate is that a password must not be an alternative way in, + * and "we could not check" is not "there is nothing to check". + */ + let enforced = null; + try { + // By MEMBERSHIP as well as by domain — an account inside the tenant at an outside address + // was the demonstrated way around this. + enforced = oidcProviders.ssoOnlyForUser(user); + } catch (e) { + console.error('[login] SSO-only status unavailable, refusing password login:', e && e.message); + enforced = { unavailable: true }; + } if (enforced) { logFailedLogin(email, getClientIp(req), 'Password login refused: organization requires SSO'); return res.status(403).json({ diff --git a/server/routes/org-sso.js b/server/routes/org-sso.js index f6a71b7..fd857b4 100644 --- a/server/routes/org-sso.js +++ b/server/routes/org-sso.js @@ -316,6 +316,36 @@ function notifyOperatorOfRemovalRequest(req, { id, orgId, orgName, reason }) { } } +/* + * An SSO-only organization may not dismantle its own enforcement sideways. + * + * `sso_only` is honoured only while a provider is enabled AND a domain is verified, so disabling + * the provider, clearing its domains, or deleting it all switch enforcement off — with `sso_only` + * still reading `true`, no request filed and the operator never told. A review used each of the + * three, and the delete variant additionally rewrites every federated account to `local`, after + * which a password reset takes over accounts the identity provider was supposed to own. + * + * That made the approval workflow decorative: anyone who could file a request could instead just + * turn the provider off. So the same interlock guards every route that would leave the tenant with + * nothing enforcing, and points at the request as the way through. + */ +function assertNotLastEnforcingProvider(orgId, providerId, what) { + const org = db.prepare('SELECT sso_only FROM organizations WHERE id = ?').get(orgId); + if (!org || !org.sso_only) return; + const remaining = db.prepare(` + SELECT COUNT(*) AS n FROM org_sso_domains d + JOIN org_sso_providers p ON p.id = d.provider_id + WHERE d.organization_id = ? AND d.verified_at IS NOT NULL AND p.enabled = 1 + AND p.id != ? + `).get(orgId, providerId).n; + if (remaining > 0) return; // another provider still enforces; this one may go + const e = new Error(`Your organization requires single sign-on, so ${what} would leave nobody able to sign in. ` + + 'Ask the people who run this server to approve stopping the requirement first.'); + e.status = 409; + e.code = 'sso_only_locked'; + throw e; +} + /** A provider's domains, with the DNS record each unverified one still needs. */ function domainsFor(providerId) { return db.prepare('SELECT * FROM org_sso_domains WHERE provider_id = ? ORDER BY domain').all(providerId) @@ -471,6 +501,15 @@ router.put('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, asyncRoute( * returns the secret, so a UI that round-trips a form would otherwise blank it on every save — * the classic way a settings page silently breaks the thing it is editing. */ + // Disabling this provider, or removing the domains it enforces through, is the same act as + // turning the requirement off — and that needs the operator. + try { + if (enabled !== undefined && !enabled) assertNotLastEnforcingProvider(req.orgId, existing.id, 'disabling this provider'); + if (domainsSupplied && !cleanDomains) assertNotLastEnforcingProvider(req.orgId, existing.id, 'removing every sign-in domain'); + } catch (e) { + return res.status(e.status || 400).json({ error: e.message, code: e.code }); + } + let newlyClaimed = []; const secretEnc = clientSecret === undefined ? existing.client_secret_enc : (clientSecret === '' ? null : secretbox.encrypt(String(clientSecret))); @@ -785,6 +824,12 @@ router.delete('/:orgId/sso/:id', requireOrgAdmin, requireVerifiedAdmin, (req, re * Both are handled here, at the moment the intent is known, rather than inferred later from the * absence of configuration — which is what made an unset GOOGLE_CLIENT_ID look like a deletion. */ + try { + assertNotLastEnforcingProvider(req.orgId, existing.id, 'deleting this provider'); + } catch (e) { + return res.status(e.status || 400).json({ error: e.message, code: e.code }); + } + const freed = db.transaction(() => { const domains = db.prepare('DELETE FROM org_sso_domains WHERE provider_id = ?').run(existing.id).changes; // Back to a local account, so the owner can recover it by proving the mailbox — strictly diff --git a/server/test/admin-users.test.js b/server/test/admin-users.test.js index 4a690bf..0abf80d 100644 --- a/server/test/admin-users.test.js +++ b/server/test/admin-users.test.js @@ -72,7 +72,11 @@ db.exec(` ); CREATE TABLE organizations ( id TEXT PRIMARY KEY, name TEXT NOT NULL, - owner_user_id TEXT, plan_id TEXT, subscription_status TEXT + owner_user_id TEXT, plan_id TEXT, subscription_status TEXT, + -- Mirrors the real schema. Login refuses when it cannot determine whether an organization + -- requires single sign-on, so a fixture missing this column fails closed — correctly, but it + -- is the fixture that is wrong, not the guard. + sso_only INTEGER NOT NULL DEFAULT 0 ); CREATE TABLE activity_log ( id INTEGER PRIMARY KEY AUTOINCREMENT, diff --git a/server/test/oidc-sso.test.js b/server/test/oidc-sso.test.js index dd1fe04..d5fd37a 100644 --- a/server/test/oidc-sso.test.js +++ b/server/test/oidc-sso.test.js @@ -768,7 +768,7 @@ test('the login gate exempts platform_admin, and that exemption is deliberate', * must not be "tidied away" by someone who reads it as a convenience. */ const src = fs.readFileSync(require.resolve('../routes/auth.js'), 'utf8'); - assert.match(src, /user\.role !== 'platform_admin'[\s\S]{0,200}ssoOnlyForEmail/, - 'the break-glass exemption must guard the ssoOnlyForEmail check'); + assert.match(src, /user\.role !== 'platform_admin'[\s\S]{0,600}ssoOnlyForUser/, + 'the break-glass exemption must guard the SSO-only check'); assert.match(src, /code: 'sso_required'/, 'and the refusal must be distinguishable from a bad password'); });