diff --git a/apps/api/src/routes/oidc.ts b/apps/api/src/routes/oidc.ts index 5c894c4..bfbfc3a 100644 --- a/apps/api/src/routes/oidc.ts +++ b/apps/api/src/routes/oidc.ts @@ -1,6 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Sign in with OpenID Connect, connect and disconnect an OpenID account (12 §3.2, §3.3). -import { and, eq, gt, isNull, notExists } from 'drizzle-orm'; +import { and, eq, gt, isNull, ne, notExists } from 'drizzle-orm'; import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { z } from 'zod'; import { decrypt, encrypt } from '../crypto/secrets.ts'; @@ -15,7 +15,7 @@ import { } from '../errors.ts'; import { requireAdmin, requireAuth } from '../auth/guards.ts'; import { isLoginBlocked, recordLoginFailure } from '../auth/login-limiter.ts'; -import { createSession, setSessionCookie } from '../auth/sessions.ts'; +import { COOKIE, createSession, getSessionUser, setSessionCookie } from '../auth/sessions.ts'; import { audit } from '../services/audit.ts'; import { getSetting } from '../services/settings.ts'; import { decide, verifiedEmail } from '../sso/claims.ts'; @@ -146,7 +146,13 @@ export async function oidcRoutes(app: FastifyInstance) { .where(and(eq(userIdentities.kind, 'oidc'), eq(userIdentities.subject, r.subject))); if (connecting) { - // Connect (12 §3.3): to the user of the session that asked, if it's still signed in. + // Connect (12 §3.3): to the user of the session that asked, if it's still signed in, and + // only in that browser: a planted sp_oidc can't connect someone else's account (08 §7 #8). + const here = req.cookies[COOKIE] + ? await getSessionUser(db, req.cookies[COOKIE], false) + : null; + if (here?.sessionId !== connecting) + return fail('expired', 'connect finished in another browser than the one that asked'); const [sess] = await db .select({ userId: sessions.userId, email: users.email }) .from(sessions) @@ -186,7 +192,8 @@ export async function oidcRoutes(app: FastifyInstance) { let userId = owner?.userId; if (!userId) { - // A user without a password or an OpenID account yet, by verified email (12 §3.3 step 2). + // A user without a password or any connected account yet, by verified email (12 §3.3 + // step 2). Never an admin: a provider account with their email isn't them (08 §7 #2). const v = verifiedEmail(r.claims, s.trustEmail); const [u] = v ? await db @@ -196,13 +203,12 @@ export async function oidcRoutes(app: FastifyInstance) { and( eq(users.email, v), isNull(users.passwordHash), + ne(users.role, 'admin'), notExists( db .select({ id: userIdentities.id }) .from(userIdentities) - .where( - and(eq(userIdentities.userId, users.id), eq(userIdentities.kind, 'oidc')), - ), + .where(eq(userIdentities.userId, users.id)), ), ), ) @@ -322,8 +328,15 @@ export async function oidcRoutes(app: FastifyInstance) { recordLoginFailure(req.ip, login); throw badRequest('The directory refused that username or password'); } - if ((await connectDirectory(db, req.log, req.user!.id, entry, req.ip)) === 'taken') + const r = await connectDirectory(db, req.log, req.user!.id, entry, req.ip); + if (r === 'taken') throw new HttpError(409, 'taken', 'That directory account belongs to another user'); + if (r === 'connected_other') + throw new HttpError( + 409, + 'connected_other', + 'Another directory account is connected. Ask an admin to disconnect it first', + ); return { ok: true }; }); app.delete('/api/users/:id/identities/:kind', { preHandler: requireAdmin }, (req, reply) => { diff --git a/apps/api/src/sso/directory.ts b/apps/api/src/sso/directory.ts index efbcbe5..3f0adeb 100644 --- a/apps/api/src/sso/directory.ts +++ b/apps/api/src/sso/directory.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // A directory entry → its SelfPhoneNG user (12 §4.3): connected before, or by the directory's // email. Never a new user (owner, 2026-10-25). -import { and, eq } from 'drizzle-orm'; +import { and, eq, notExists } from 'drizzle-orm'; import { deleteUserSessions } from '../auth/sessions.ts'; import type { Db } from '../db/client.ts'; import { userIdentities, users } from '../db/schema.ts'; @@ -22,7 +22,21 @@ export async function directoryUserFor(db: Db, e: DirectoryEntry): Promise u.role !== 'admin' && (u.passwordHash === null || u.mustChangePassword); -/** Connect a directory account the person proved (My settings, 12 §4.3). */ +/** + * Connect a directory account the person proved (My settings, 12 §4.3). Not another one in place + * of the one they have: the leaver check follows it, so swapping is an admin's job (08 §7 #5). + */ export async function connectDirectory( db: Db, log: { info(o: object, m: string): void }, userId: string, e: DirectoryEntry, ip: string, -): Promise<'connected' | 'taken'> { +): Promise<'connected' | 'taken' | 'connected_other'> { const [owner] = await db .select({ userId: userIdentities.userId }) .from(userIdentities) .where(and(eq(userIdentities.kind, 'ldap'), eq(userIdentities.subject, e.id))); if (owner && owner.userId !== userId) return 'taken'; + const [mine] = await db + .select({ subject: userIdentities.subject }) + .from(userIdentities) + .where(and(eq(userIdentities.userId, userId), eq(userIdentities.kind, 'ldap'))); + if (mine && mine.subject !== e.id) return 'connected_other'; await db.transaction(async (tx) => { await tx .delete(userIdentities) @@ -101,11 +123,8 @@ export async function directorySignIn( entityId: u.id, summary: `${u.email} connected directory account ${e.dn}`, }); - } else if (mine.subject !== e.id) { - // Their email matches, but another directory account is already theirs: not this one. - log.info({ userId: u.id }, `directory entry ${e.dn} matches by email but isn't theirs`); - return null; } else + // Found by this account's id (directoryUserFor never matches by email someone connected). await db .update(userIdentities) .set({ email: e.email, lastUsedAt: new Date() }) diff --git a/apps/api/test/directory-signin.test.ts b/apps/api/test/directory-signin.test.ts index 273f73d..7c89647 100644 --- a/apps/api/test/directory-signin.test.ts +++ b/apps/api/test/directory-signin.test.ts @@ -1,12 +1,13 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // 12 §4.3: who a directory entry may connect to by email, who may disconnect it (§4.5), and a -// directory that's down counting as a failed sign-in (§4.2). +// directory that's down counting as a failed sign-in (§4.2). 08 §7: no OpenID user by email, no +// swapping the connected account. import { eq } from 'drizzle-orm'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { resetLoginLimiter } from '../src/auth/login-limiter.ts'; import { userIdentities, users } from '../src/db/schema.ts'; import { setSetting } from '../src/services/settings.ts'; -import { directorySignIn } from '../src/sso/directory.ts'; +import { connectDirectory, directorySignIn } from '../src/sso/directory.ts'; import type { DirectoryEntry } from '../src/sso/ldap.ts'; import { hasTestDb } from './db.ts'; import { ADMIN, H, createTestApp } from './app.ts'; @@ -52,6 +53,38 @@ describe.skipIf(!hasTestDb)('directory accounts by email (12 §4.3)', () => { } }); + it('never connects by email someone who signs in with OpenID (08 §7 #1)', async () => { + // A directory that lets people edit their own mail: Mallory's entry, the victim's email. + const victim = await t.addUser('oidc-only@example.com', 'unused-pw-2'); + await t.db.update(users).set({ passwordHash: null }).where(eq(users.id, victim.id)); + await t.db + .insert(userIdentities) + .values({ userId: victim.id, kind: 'oidc', subject: 'https://idp victim-sub' }); + expect(await directorySignIn(t.db, log, entry(victim.email), '127.0.0.1')).toBeNull(); + expect(await connected(victim.id)).toBe(1); // the OpenID account only + }); + + it('a connected directory account is only swapped by an admin (08 §7 #5)', async () => { + const u = await t.addUser('swapper@example.com', 'swapper-password-1'); + expect(await connectDirectory(t.db, log, u.id, entry(u.email), '127.0.0.1')).toBe('connected'); + const [first] = await t.db.select().from(userIdentities).where(eq(userIdentities.userId, u.id)); + // The same account again is fine; another one (a shared mailbox) is not. + expect( + await connectDirectory( + t.db, + log, + u.id, + { ...entry(u.email), id: first!.subject }, + '127.0.0.1', + ), + ).toBe('connected'); + expect(await connectDirectory(t.db, log, u.id, entry('shared@example.com'), '127.0.0.1')).toBe( + 'connected_other', + ); + const now = await t.db.select().from(userIdentities).where(eq(userIdentities.userId, u.id)); + expect(now.map((i) => i.subject)).toEqual([first!.subject]); + }); + it('only an admin can disconnect a directory account (a leaver must stay checked)', async () => { const u = await t.addUser('leaver@example.com', 'leaver-password-1'); await t.db.insert(userIdentities).values({ userId: u.id, kind: 'ldap', subject: 'leaver-id' }); diff --git a/apps/api/test/oidc.test.ts b/apps/api/test/oidc.test.ts index 92fa741..40eff5e 100644 --- a/apps/api/test/oidc.test.ts +++ b/apps/api/test/oidc.test.ts @@ -207,6 +207,56 @@ for (const alg of ['RS256', 'ES256'] as const) expect(ids).toMatchObject([{ kind: 'oidc', subject: `${idp.issuer} u-cara` }]); }); + it('never matches by email an admin, or someone with a directory account (08 §7 #2)', async () => { + const [boss, dir] = await t.db + .insert(users) + .values([ + { email: 'diradmin@example.com', displayName: 'Boss', passwordHash: null, role: 'admin' }, + { email: 'diruser@example.com', displayName: 'Dir', passwordHash: null }, + ]) + .returning(); + await t.db + .insert(userIdentities) + .values({ userId: dir!.id, kind: 'ldap', subject: 'dir-user-id' }); + for (const u of [boss!, dir!]) { + idp.st.user = { sub: `u-${u.email}`, email: u.email, email_verified: true }; + const { callback } = await flow(); + expect(errorOf(callback!)).toBe('not_connected'); + expect(session(callback!)).toBeNull(); + } + }); + + it('connect finishes only in the browser whose session asked (08 §7 #8)', async () => { + // Mallory starts Connect, then plants her sp_oidc cookie in Vic's browser and sends him on. + await t.addUser('mallory@example.com', 'mallory-password-1'); + const [m] = await t.db.select().from(users).where(eq(users.email, 'mallory@example.com')); + await t.addUser('vic@example.com', 'vic-password-123'); + const mallory = await t.login('mallory@example.com', 'mallory-password-1'); + const vic = await t.login('vic@example.com', 'vic-password-123'); + idp.st.user = { sub: 'u-vic', email: 'vic@example.com', email_verified: true }; + for (const theirs of [vic, '']) { + const start = await t.app.inject({ + method: 'POST', + url: '/api/auth/oidc/connect', + headers: { ...H, cookie: mallory }, + payload: {}, + }); + const pending = start.cookies.find((c) => c.name === 'sp_oidc')!; + const atIdp = await fetch((start.json() as { url: string }).url, { redirect: 'manual' }); + const back = new URL(atIdp.headers.get('location')!); + const callback = await t.app.inject({ + url: back.pathname + back.search, + headers: { cookie: [`sp_oidc=${pending.value}`, theirs].filter(Boolean).join('; ') }, + }); + expect(errorOf(callback)).toBe('expired'); + const ids = await t.db + .select() + .from(userIdentities) + .where(eq(userIdentities.userId, m!.id)); + expect(ids).toEqual([]); + } + }); + it('trustEmail matches an unverified email', async () => { await t.db .insert(users) diff --git a/docs/spec/12-sso.md b/docs/spec/12-sso.md index 7634a83..0ea1b85 100644 --- a/docs/spec/12-sso.md +++ b/docs/spec/12-sso.md @@ -199,11 +199,12 @@ Failed callbacks count in the IP login limiter (04 §2.2). Signing in, in this order: 1. A `user_identities` row (`oidc`, `" "`) → its user. -2. Else, a user with that email (case-insensitive) who has **no password and no OpenID account - yet** (made by an admin in `oidc` mode, or whose password an admin removed), **and the email is - verified** (`email_verified === true`, or `trustEmail`) → connect it now (step 3's row) and sign - in. Users who have a password are never matched by email: they connect from My account, so a - provider where people can set their own email can't hand over an account. +2. Else, a user with that email (case-insensitive) who is **not an admin** and has **no password + and no connected account of any kind yet** (made by an admin in `oidc` mode, or whose password + an admin removed), **and the email is verified** (`email_verified === true`, or `trustEmail`) + → connect it now (step 3's row) and sign in. Users who have a password, an admin, or someone + who signs in through the directory are never matched by email: they connect from My account, + so a provider where people can set their own email can't hand over an account (08 §7 #2). 3. Else `not_connected`. Connecting (from My account, already signed in, any mode but `password`): @@ -211,6 +212,9 @@ Connecting (from My account, already signed in, any mode but `password`): - After the callback, the session's user gets the `user_identities` row (email and groups as sent). Refused (`taken`) when that `iss sub` belongs to another user; a user who already has one replaces it (confirm in the UI). Other sessions stay. Audit `sso_connected`. +- The callback must arrive in the browser whose session asked: its `sp_session` is the session in + `sp_oidc`, else `expired`. A planted `sp_oidc` (from a sibling subdomain) can't connect the + victim's provider account to someone else's user (08 §7 #8). - **Disconnect** (`DELETE /api/me/identities/oidc`; admins also on Users, `DELETE /api/users/:id/identities/oidc`): ends that user's `oidc` sessions. Refused (409 `last_way_in`) when it would leave them no way to sign in in the current mode (no password, or @@ -376,11 +380,13 @@ A user can have both a SelfPhoneNG password and a directory account: either pass 1. A `user_identities` row (`ldap`, id) → its user. Their email here follows the directory's when it changes (unless another user has it: kept, logged). -2. Else, a user with the directory email who is **not an admin** and has **no password or only the - temporary one** from Users (`must_change_password`) → connect it (add the row) and sign in, +2. Else, a user with the directory email who is **not an admin**, has **no password or only the + temporary one** from Users (`must_change_password`) and **no connected account of any kind** + (an OpenID user's account is theirs, 08 §7 #1) → connect it (add the row) and sign in, audit `ldap_connected`. Anyone else connects on My settings → Sign-in → **Connect directory account** (`POST /api/me/identities/ldap`, `{ login, password }`, checked by the directory and - counted by the login limiter; 409 `taken` when another user has it). Security audit, + counted by the login limiter; 409 `taken` when another user has it, 409 `connected_other` when + they already have a different one: an admin disconnects it first, 08 §7 #5). Security audit, 2026-10-26: a directory where people can edit their own `mail` (common OpenLDAP access rules) must not hand out the admin's account. 3. Else → 401 "Wrong email or password" (no account is made, §1), with `login_failed` audited as