From 97e67e347d01ca7e1e2ce1de6e4b19c17243e442 Mon Sep 17 00:00:00 2001 From: iamdoubz Date: Mon, 26 Oct 2026 09:46:23 -0500 Subject: [PATCH] auth: security audit fixes, rate limits on every route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two separate reviews of the SSO branch and all 207 API routes (spec 12 §8.6). Every route was guarded as meant. Fixed, each with a test: - login: a directory 503 counts as a failed sign-in; a locked directory account is a refused password, not "unavailable" - open redirect after OpenID sign-in (a tab in next) - directory email matching only for non-admins without their own password; others connect on My settings (POST /api/me/identities/ldap); only admins disconnect a directory account - OpenID session check reads the refreshed ID token's claims too - connect is POST /api/auth/oidc/connect (CSRF header) - stored bind password, client secret and SMTP password tied to their server - 409 role_managed when a provider sets the role; empty Contrack PATCH is a 400 - @fastify/rate-limit on every route (RATE_LIMIT_PER_MINUTE), 10/min on Test buttons; a password asked again shares the login limiter Live tests pass against lldap 0.6.3, Samba 4 AD and Keycloak 26.8.1. --- .env.example | 1 + apps/api/package.json | 1 + apps/api/src/app.ts | 12 ++- apps/api/src/auth/guards.ts | 8 +- apps/api/src/config.ts | 2 + apps/api/src/routes/about.ts | 3 +- apps/api/src/routes/auth.ts | 41 +++++--- apps/api/src/routes/contacts.ts | 92 ++++++++++-------- apps/api/src/routes/integrations.ts | 4 +- apps/api/src/routes/me.ts | 3 + apps/api/src/routes/oidc.ts | 117 +++++++++++++++++------ apps/api/src/routes/provisioning.ts | 3 +- apps/api/src/routes/settings.ts | 89 +++++++++++------- apps/api/src/routes/system.ts | 22 +++-- apps/api/src/routes/trunks.ts | 4 +- apps/api/src/routes/users.ts | 38 +++++++- apps/api/src/sso/directory.ts | 44 ++++++++- apps/api/src/sso/ldap.test.ts | 42 ++++++++- apps/api/src/sso/ldap.ts | 6 +- apps/api/src/sso/oidcCheck.ts | 27 +++++- apps/api/test/ad-live.test.ts | 8 +- apps/api/test/app.ts | 1 + apps/api/test/directory-signin.test.ts | 119 ++++++++++++++++++++++++ apps/api/test/fake-oidc.ts | 11 ++- apps/api/test/ldap-live.test.ts | 38 ++++++-- apps/api/test/oidc-live.test.ts | 20 +++- apps/api/test/oidc.test.ts | 85 +++++++++++++++-- apps/api/test/rate-limit.test.ts | 71 ++++++++++++++ apps/api/test/sign-in.test.ts | 119 ++++++++++++++++++++++++ apps/web/src/api/client.ts | 5 + apps/web/src/components/SignInCard.tsx | 84 +++++++++++++++-- apps/web/src/pages/admin/UserDrawer.tsx | 13 ++- docs/admin-guide/sign-in.md | 19 +++- docs/spec/01-infrastructure.md | 1 + docs/spec/04-backend-api.md | 7 +- docs/spec/08-security-ops.md | 2 + docs/spec/12-sso.md | 62 ++++++++++-- packages/shared/src/users.ts | 2 + pnpm-lock.yaml | 19 ++++ 39 files changed, 1059 insertions(+), 186 deletions(-) create mode 100644 apps/api/test/directory-signin.test.ts create mode 100644 apps/api/test/rate-limit.test.ts diff --git a/.env.example b/.env.example index 063e200..2deb592 100644 --- a/.env.example +++ b/.env.example @@ -15,6 +15,7 @@ APP_UID=1000 APP_GID=1000 LOG_LEVEL=info TRUST_PROXY=true # true when behind Caddy/nginx +RATE_LIMIT_PER_MINUTE=600 # requests per minute per client IP, every route; 0 = off RTP_PORT_START=10000 # call audio (UDP), both ends included: open this range on the RTP_PORT_END=20000 # firewall and router. Two ports per call; 200 is plenty for a small office # Local AI (admin guide: AI). Starting values for Settings → AI; that page wins once saved. diff --git a/apps/api/package.json b/apps/api/package.json index 324c4e7..fc387d0 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -12,6 +12,7 @@ }, "dependencies": { "@fastify/cookie": "^11.1.3", + "@fastify/rate-limit": "^11.2.1", "@fastify/static": "^10.1.6", "@fastify/websocket": "^11.3.4", "@node-rs/argon2": "^2.2.2", diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 74fc3b6..496e119 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -3,6 +3,7 @@ import { existsSync } from 'node:fs'; import { resolve } from 'node:path'; import Fastify from 'fastify'; import fastifyCookie from '@fastify/cookie'; +import fastifyRateLimit from '@fastify/rate-limit'; import fastifyStatic from '@fastify/static'; import fastifyWebsocket from '@fastify/websocket'; import { sql } from 'drizzle-orm'; @@ -11,7 +12,7 @@ import type { Db } from './db/client.ts'; import type { Generator } from './generator/index.ts'; import type { AmiClient } from './ami/client.ts'; import { settingsRoutes } from './routes/settings.ts'; -import { errorHandler, forbidden, notFound, sendError } from './errors.ts'; +import { HttpError, errorHandler, forbidden, notFound, sendError } from './errors.ts'; import { authRoutes } from './routes/auth.ts'; import { userRoutes } from './routes/users.ts'; import { oidcRoutes } from './routes/oidc.ts'; @@ -92,6 +93,15 @@ export function buildApp(config: Config, db: Db, publicDir = resolve('public')) app.decorateRequest('sessionId', null); app.register(fastifyCookie); app.register(fastifyWebsocket); + // Every route, per client IP (08 §2.4). Before the routes, so each one gets it; routes with + // their own limits (login, /prov) or a stricter one say so in `config.rateLimit`. + if (config.RATE_LIMIT_PER_MINUTE > 0) + app.register(fastifyRateLimit, { + max: config.RATE_LIMIT_PER_MINUTE, + timeWindow: 60_000, + errorResponseBuilder: (_req, ctx) => + new HttpError(429, 'rate_limited', `Too many requests, try again in ${ctx.after}`), + }); app.setErrorHandler(errorHandler); // CSRF (04 §2.2): browsers can't add this header cross-site without CORS, and CORS is off. diff --git a/apps/api/src/auth/guards.ts b/apps/api/src/auth/guards.ts index 12c2b16..925aa73 100644 --- a/apps/api/src/auth/guards.ts +++ b/apps/api/src/auth/guards.ts @@ -1,5 +1,6 @@ // SPDX-License-Identifier: AGPL-3.0-or-later -// preHandler guards (04 §2.4). Routes list them in `preHandler`. +// preHandler guards (04 §2.4). Routes list them in `preHandler`; the rate limit presets go in +// `config`. import type { FastifyReply, FastifyRequest } from 'fastify'; import type { users } from '../db/schema.ts'; import { forbidden, unauthorized } from '../errors.ts'; @@ -47,3 +48,8 @@ export async function requireAdmin(req: FastifyRequest, reply: FastifyReply): Pr await requireAuth(req, reply); if (req.user!.role !== 'admin') throw forbidden('Admins only'); } + +/** Route `config` for what calls out or does real work (Test buttons, downloads): 10 a minute. */ +export const SLOW = { rateLimit: { max: 10, timeWindow: 60_000 } }; +/** Route `config` for routes with their own limit (/prov) or only Asterisk on this machine. */ +export const NO_LIMIT = { rateLimit: false as const }; diff --git a/apps/api/src/config.ts b/apps/api/src/config.ts index 59421f6..a41589f 100644 --- a/apps/api/src/config.ts +++ b/apps/api/src/config.ts @@ -35,6 +35,8 @@ const EnvSchema = z API_PORT: z.coerce.number().int().min(1).max(65535).default(8080), LOG_LEVEL: z.enum(['fatal', 'error', 'warn', 'info', 'debug', 'trace']).default('info'), TRUST_PROXY: z.stringbool().default(true), + /** Requests per minute per client IP, every route (08 §2.4); stricter ones per route. 0: off. */ + RATE_LIMIT_PER_MINUTE: z.coerce.number().int().min(0).default(600), /** Call audio (RTP) ports, both ends included: the firewall opens this UDP range (01 §3.6). */ RTP_PORT_START: z.coerce.number().int().min(1024).max(65534).default(10000), RTP_PORT_END: z.coerce.number().int().min(1025).max(65535).default(20000), diff --git a/apps/api/src/routes/about.ts b/apps/api/src/routes/about.ts index b03d756..dc4a5aa 100644 --- a/apps/api/src/routes/about.ts +++ b/apps/api/src/routes/about.ts @@ -7,6 +7,7 @@ import { readFile, stat } from 'node:fs/promises'; import type { FastifyInstance } from 'fastify'; import type { AboutDto } from '@selfphoneng/shared'; import { notFound } from '../errors.ts'; +import { SLOW } from '../auth/guards.ts'; export const REPO_URL = 'https://git.dou.bet/spng/selfphoneng'; /** In the api's working directory (/app in the image), like ./public. */ @@ -35,7 +36,7 @@ export async function aboutRoutes(app: FastifyInstance) { }; }); - app.get('/api/about/source', async (_req, reply) => { + app.get('/api/about/source', { config: SLOW }, async (_req, reply) => { if (!(await stat(SOURCE_ARCHIVE).catch(() => null))) throw notFound('The source archive is only in builds made with Docker'); return reply diff --git a/apps/api/src/routes/auth.ts b/apps/api/src/routes/auth.ts index 4feccc6..564ea74 100644 --- a/apps/api/src/routes/auth.ts +++ b/apps/api/src/routes/auth.ts @@ -1,7 +1,7 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // /api/auth/* (04 §2, §7.3). import { and, eq, isNull, lt, or, sql } from 'drizzle-orm'; -import type { FastifyInstance } from 'fastify'; +import type { FastifyInstance, FastifyRequest } from 'fastify'; import { ChangePasswordSchema, LoginSchema, @@ -140,14 +140,24 @@ export async function authRoutes(app: FastifyInstance) { return { secret, otpauthUrl: url, qr: await QRCode.toDataURL(url, { margin: 2, width: 240 }) }; }); + /** + * A signed-in person's own password, asked again (two-step on/off, recovery codes, a new + * password). Wrong ones count in the login limiter, so a stolen session can't guess it. + */ + async function myPasswordOk(req: FastifyRequest, hash: string | null, password: string) { + if (isLoginBlocked(req.ip, req.user!.email)) throw rateLimited(); + if (hash && (await verifyPassword(hash, password))) return true; + recordLoginFailure(req.ip, req.user!.email); + return false; + } + app.post('/api/auth/totp/enable', { preHandler: requireAuth }, async (req) => { adminOnly(req.user!); const { secret, code, password } = TotpEnableSchema.parse(req.body); const u = await myRow(req.user!.id); if (u.totpSecretEnc) throw conflict('Two-step sign-in is already on'); // Like turning it off: a stolen session alone can't lock the owner out with its own secret. - if (!u.passwordHash || !(await verifyPassword(u.passwordHash, password))) - throw badRequest('Wrong password'); + if (!(await myPasswordOk(req, u.passwordHash, password))) throw badRequest('Wrong password'); const step = checkTotp(secret, code, null); if (step === null) throw badRequest('That code doesn’t match. Check the phone’s clock and try again.'); @@ -168,16 +178,15 @@ export async function authRoutes(app: FastifyInstance) { }); /** Turning it off or making new recovery codes needs the password again (on does too). */ - async function confirmPassword(id: string, body: unknown) { - const { password } = PasswordConfirmSchema.parse(body); - const u = await myRow(id); - if (!u.passwordHash || !(await verifyPassword(u.passwordHash, password))) - throw badRequest('Wrong password'); + async function confirmPassword(req: FastifyRequest) { + const { password } = PasswordConfirmSchema.parse(req.body); + const u = await myRow(req.user!.id); + if (!(await myPasswordOk(req, u.passwordHash, password))) throw badRequest('Wrong password'); return u; } app.post('/api/auth/totp/disable', { preHandler: requireAuth }, async (req) => { - await confirmPassword(req.user!.id, req.body); + await confirmPassword(req); await db .update(users) .set({ totpSecretEnc: null, totpLastStep: null, totpRecoveryHashes: [] }) @@ -194,7 +203,7 @@ export async function authRoutes(app: FastifyInstance) { }); app.post('/api/auth/totp/recovery-codes', { preHandler: requireAuth }, async (req) => { - const u = await confirmPassword(req.user!.id, req.body); + const u = await confirmPassword(req); if (!u.totpSecretEnc) throw badRequest('Two-step sign-in is off'); const { codes, hashes } = newRecoveryCodes(); await db.update(users).set({ totpRecoveryHashes: hashes }).where(eq(users.id, u.id)); @@ -236,6 +245,16 @@ export async function authRoutes(app: FastifyInstance) { try { entry = await ldapAuthenticate(ldap, login, body.password); } catch (err) { + // Counted like a wrong password: a 503 must not be a free guess at the local one. + recordLoginFailure(req.ip, login); + await audit(db, { + actorUserId: user?.id, + actorIp: req.ip, + action: 'login_failed', + entityType: 'user', + entityId: user?.id, + summary: `Failed login for ${login} (the directory can't be reached)`, + }); req.log.warn({ event: 'auth.ldap', err: (err as Error).message }, 'directory unavailable'); await directoryWarning( db, @@ -345,7 +364,7 @@ export async function authRoutes(app: FastifyInstance) { const user = req.user!; if (!user.passwordHash) throw badRequest('You have no password here. Ask an admin to reset it to get one.'); - if (!(await verifyPassword(user.passwordHash, body.currentPassword))) + if (!(await myPasswordOk(req, user.passwordHash, body.currentPassword))) throw badRequest('Current password is wrong'); if (body.newPassword === body.currentPassword) throw badRequest('Choose a password different from the current one'); diff --git a/apps/api/src/routes/contacts.ts b/apps/api/src/routes/contacts.ts index 01f821b..4d021e6 100644 --- a/apps/api/src/routes/contacts.ts +++ b/apps/api/src/routes/contacts.ts @@ -16,7 +16,7 @@ import { type ContactShareDto, type ListResponse, } from '@selfphoneng/shared'; -import { requireAdmin, requireAuth } from '../auth/guards.ts'; +import { NO_LIMIT, SLOW, requireAdmin, requireAuth } from '../auth/guards.ts'; import { addToBook, assertBuiltin, @@ -162,33 +162,39 @@ export async function contactRoutes(app: FastifyInstance) { * The dialplan's caller-name lookup (11 §6 C4): only Asterisk on this machine (no proxy in * between) with the token. Never waits on Contrack. Not logged: the token is in the query. */ - app.get('/api/internal/caller-name', { logLevel: 'silent' }, async (req, reply) => { - const q = z - .object({ - ext: z.string().regex(/^\d{1,10}$/), - number: z.string().regex(/^\d{3,20}$/), - token: z.string().max(100), - }) - .safeParse(req.query); - const local = ['127.0.0.1', '::1', '::ffff:127.0.0.1'].includes(req.socket.remoteAddress ?? ''); - if ( - !local || - req.headers['x-forwarded-for'] || - !q.success || - !sameToken(q.data.token, callerNameToken(app.config.APP_SECRET)) - ) - return reply.code(404).send(''); - const [e] = await db - .select({ userId: extensions.userId }) - .from(extensions) - .where(eq(extensions.number, q.data.ext)); - const names = await contactNames(db, e?.userId ?? null, [q.data.number], { wait: false }).catch( - () => new Map(), - ); - return reply - .type('text/plain; charset=utf-8') - .send(phoneSafeName(names.get(q.data.number) ?? '')); - }); + app.get( + '/api/internal/caller-name', + { logLevel: 'silent', config: NO_LIMIT }, + async (req, reply) => { + const q = z + .object({ + ext: z.string().regex(/^\d{1,10}$/), + number: z.string().regex(/^\d{3,20}$/), + token: z.string().max(100), + }) + .safeParse(req.query); + const local = ['127.0.0.1', '::1', '::ffff:127.0.0.1'].includes( + req.socket.remoteAddress ?? '', + ); + if ( + !local || + req.headers['x-forwarded-for'] || + !q.success || + !sameToken(q.data.token, callerNameToken(app.config.APP_SECRET)) + ) + return reply.code(404).send(''); + const [e] = await db + .select({ userId: extensions.userId }) + .from(extensions) + .where(eq(extensions.number, q.data.ext)); + const names = await contactNames(db, e?.userId ?? null, [q.data.number], { + wait: false, + }).catch(() => new Map()); + return reply + .type('text/plain; charset=utf-8') + .send(phoneSafeName(names.get(q.data.number) ?? '')); + }, + ); /** Copy the builtin book into Contrack (11 §5, C6): mine, or (admin) the company's. */ app.post( @@ -214,19 +220,23 @@ export async function contactRoutes(app: FastifyInstance) { ); /** Settings → Contacts → Test (11 §4.3): the form's address, and its token or the saved one. */ - app.post('/api/contacts/contrack/test', { preHandler: requireAdmin }, async (req) => { - const b = z - .object({ url: z.string().trim().min(1), companyToken: z.string().trim().optional() }) - .parse(req.body); - const saved = await getSetting(db, 'contacts.contrack'); - const token = - b.companyToken || (saved?.companyTokenEnc ? decrypt(saved.companyTokenEnc) : null); - try { - return { ok: true, detail: await contrackCheck(b.url, token) }; - } catch (err) { - return { ok: false, error: (err as Error).message }; - } - }); + app.post( + '/api/contacts/contrack/test', + { preHandler: requireAdmin, config: SLOW }, + async (req) => { + const b = z + .object({ url: z.string().trim().min(1), companyToken: z.string().trim().optional() }) + .parse(req.body); + const saved = await getSetting(db, 'contacts.contrack'); + const token = + b.companyToken || (saved?.companyTokenEnc ? decrypt(saved.companyTokenEnc) : null); + try { + return { ok: true, detail: await contrackCheck(b.url, token) }; + } catch (err) { + return { ok: false, error: (err as Error).message }; + } + }, + ); // ───── sharing: an offer each, accepted or denied by the recipient (11 §2.1) ───── app.post('/api/me/contacts/share', { preHandler: requireAuth }, async (req) => { diff --git a/apps/api/src/routes/integrations.ts b/apps/api/src/routes/integrations.ts index fd2cda3..48c62c2 100644 --- a/apps/api/src/routes/integrations.ts +++ b/apps/api/src/routes/integrations.ts @@ -13,7 +13,7 @@ import { type WebhookDto, type WebhookEvent, } from '@selfphoneng/shared'; -import { requireAdmin } from '../auth/guards.ts'; +import { SLOW, requireAdmin } from '../auth/guards.ts'; import { generateToken, hashToken } from '../auth/tokens.ts'; import { decrypt, encrypt } from '../crypto/secrets.ts'; import { apiTokens, users, webhookDeliveries, webhooks } from '../db/schema.ts'; @@ -103,7 +103,7 @@ export async function integrationRoutes(app: FastifyInstance) { }); /** Sends a `webhook.test` event now, not queued: the answer shows right away. */ - app.post('/api/webhooks/:id/test', async (req) => { + app.post('/api/webhooks/:id/test', { config: SLOW }, async (req) => { const h = await getHook(IdParams.parse(req.params).id); const body = JSON.stringify({ id: crypto.randomUUID(), diff --git a/apps/api/src/routes/me.ts b/apps/api/src/routes/me.ts index 02e35c1..25a16da 100644 --- a/apps/api/src/routes/me.ts +++ b/apps/api/src/routes/me.ts @@ -180,6 +180,9 @@ export async function meRoutes(app: FastifyInstance) { /** Log my calls in Contrack, with or without their summaries (11 §4.4). */ app.patch('/api/me/contrack', async (req) => { const o = ContrackOptionsSchema.parse(req.body); + // An empty update would be `UPDATE … SET` with nothing to set: a 500, not a 400. + if (o.logCalls === undefined && o.logSummaries === undefined) + throw badRequest('Nothing to update'); const [u] = await db .select({ logCalls: users.contrackLogCalls }) .from(users) diff --git a/apps/api/src/routes/oidc.ts b/apps/api/src/routes/oidc.ts index 1fcf4d4..5c894c4 100644 --- a/apps/api/src/routes/oidc.ts +++ b/apps/api/src/routes/oidc.ts @@ -5,13 +5,22 @@ import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { z } from 'zod'; import { decrypt, encrypt } from '../crypto/secrets.ts'; import { sessions, userIdentities, users } from '../db/schema.ts'; -import { HttpError, notFound, unauthorized } from '../errors.ts'; +import { + HttpError, + badRequest, + forbidden, + notFound, + rateLimited, + unauthorized, +} 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 { audit } from '../services/audit.ts'; import { getSetting } from '../services/settings.ts'; import { decide, verifiedEmail } from '../sso/claims.ts'; +import { connectDirectory } from '../sso/directory.ts'; +import { ldapAuthenticate } from '../sso/ldap.ts'; import { applyClaims } from '../sso/oidcCheck.ts'; import { finishSignIn, @@ -24,8 +33,14 @@ import { export const OIDC_COOKIE = 'sp_oidc'; const COOKIE_PATH = '/api/auth/oidc'; -/** Same-site paths only: never send the browser to another origin. */ -const safeNext = (p: string | undefined) => (p && /^\/(?![/\\])/.test(p) ? p : undefined); +/** + * Same-site paths only: never send the browser to another origin. No backslash or control + * character anywhere: browsers drop tabs and newlines from URLs, so "//evil.example" + * would become "//evil.example". + */ +export const safeNext = (p: string | undefined) => + // eslint-disable-next-line no-control-regex -- control characters are what it refuses + p && /^\/(?!\/)[^\\\x00-\x1f\x7f]*$/.test(p) ? p : undefined; const withParam = (path: string, k: string, v: string) => `${path}${path.includes('?') ? '&' : '?'}${k}=${v}`; @@ -41,34 +56,46 @@ export async function oidcRoutes(app: FastifyInstance) { return s; } + /** The sp_oidc cookie: what the callback checks the provider's answer against. */ + async function begin(reply: FastifyReply, s: OidcSettings, next: string, connect: string | null) { + const { url, pending } = await startSignIn(s, host, next, connect); + reply.setCookie(OIDC_COOKIE, encrypt(JSON.stringify(pending)), { + httpOnly: true, + secure: true, + sameSite: 'lax', + path: COOKIE_PATH, + maxAge: 600, + }); + return url.href; + } + app.get('/api/auth/oidc/start', async (req, reply) => { const s = await enabled(); - const q = z - .object({ next: z.string().max(500).optional(), connect: z.literal('1').optional() }) - .parse(req.query); - let connectSessionId: string | null = null; - let next = safeNext(q.next); - if (q.connect) { - await requireAuth(req, reply); - if (!req.sessionId) throw unauthorized(); // an API token can't connect an account - connectSessionId = req.sessionId; - next ??= req.user!.role === 'admin' ? '/admin/account' : '/me/settings'; - } + const q = z.object({ next: z.string().max(500).optional() }).parse(req.query); try { - const { url, pending } = await startSignIn(s, host, next ?? '/', connectSessionId); - reply.setCookie(OIDC_COOKIE, encrypt(JSON.stringify(pending)), { - httpOnly: true, - secure: true, - sameSite: 'lax', - path: COOKIE_PATH, - maxAge: 600, - }); - return reply.redirect(url.href); + return reply.redirect(await begin(reply, s, safeNext(q.next) ?? '/', null)); } catch (err) { req.log.warn({ event: 'auth.oidc', err: oidcError(err) }, 'OpenID discovery failed'); - return reply.redirect( - withParam(connectSessionId ? next! : '/login', 'sso_error', 'provider'), - ); + return reply.redirect(withParam('/login', 'sso_error', 'provider')); + } + }); + + /** + * Connect from My account (12 §3.3). A POST, so it needs the CSRF header: another site can't + * start it and connect whatever provider account the browser is signed in to. The page then + * sends the browser to `url`. + */ + app.post('/api/auth/oidc/connect', { preHandler: requireAuth }, async (req, reply) => { + const s = await enabled(); + if (!req.sessionId) throw unauthorized(); // an API token can't connect an account + const q = z.object({ next: z.string().max(500).optional() }).parse(req.body ?? {}); + const next = + safeNext(q.next) ?? (req.user!.role === 'admin' ? '/admin/account' : '/me/settings'); + try { + return { url: await begin(reply, s, next, req.sessionId) }; + } catch (err) { + req.log.warn({ event: 'auth.oidc', err: oidcError(err) }, 'OpenID discovery failed'); + throw new HttpError(502, 'provider', `${s.label} can't be reached`); } }); @@ -264,9 +291,41 @@ export async function oidcRoutes(app: FastifyInstance) { } const Kind = z.object({ kind: z.enum(['oidc', 'ldap']) }); - app.delete('/api/me/identities/:kind', { preHandler: requireAuth }, (req, reply) => - disconnect(req, reply, req.user!.id, Kind.parse(req.params).kind), - ); + app.delete('/api/me/identities/:kind', { preHandler: requireAuth }, (req, reply) => { + const { kind } = Kind.parse(req.params); + // The directory account is how a leaver gets disabled (12 §4.5): only an admin drops it. + if (kind === 'ldap' && req.user!.role !== 'admin') + throw forbidden('Ask an admin to disconnect your directory account', 'admin_only'); + return disconnect(req, reply, req.user!.id, kind); + }); + + /** + * Connect my directory account (12 §4.3): its username and password, checked by the directory, + * while signed in here. Counted like a sign-in by the login limiter. + */ + app.post('/api/me/identities/ldap', { preHandler: requireAuth }, async (req) => { + if (!req.sessionId) throw unauthorized(); // an API token can't connect an account + const { login, password } = z + .object({ login: z.string().trim().min(1).max(254), password: z.string().min(1).max(200) }) + .parse(req.body); + const ldap = await getSetting(db, 'auth.ldap'); + if (!ldap?.enabled) throw notFound(); + if (isLoginBlocked(req.ip, login)) throw rateLimited(); + let entry; + try { + entry = await ldapAuthenticate(ldap, login, password); + } catch { + recordLoginFailure(req.ip, login); + throw new HttpError(503, 'directory_unavailable', 'The directory can’t be reached'); + } + if (!entry) { + 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') + throw new HttpError(409, 'taken', 'That directory account belongs to another user'); + return { ok: true }; + }); app.delete('/api/users/:id/identities/:kind', { preHandler: requireAdmin }, (req, reply) => { const p = Kind.extend({ id: z.uuid() }).parse(req.params); return disconnect(req, reply, p.id, p.kind); diff --git a/apps/api/src/routes/provisioning.ts b/apps/api/src/routes/provisioning.ts index 17529db..b129b1c 100644 --- a/apps/api/src/routes/provisioning.ts +++ b/apps/api/src/routes/provisioning.ts @@ -14,6 +14,7 @@ import { allowProvRequest, recordProvFailure } from '../provisioning/limiter.ts' import { grandstream, grandstreamPhonebook } from '../provisioning/templates/grandstream.ts'; import { yealink, yealinkCommon, yealinkPhonebook } from '../provisioning/templates/yealink.ts'; import { getSetting } from '../services/settings.ts'; +import { NO_LIMIT } from '../auth/guards.ts'; /** File name → what it is. Anything else is a plain 404 (phones ask for files we don't serve). */ const FILES: { @@ -73,7 +74,7 @@ export async function provisioningRoutes(app: FastifyInstance) { const deny = (reply: FastifyReply, code: number, text: string) => reply.code(code).header('Cache-Control', 'no-store').type('text/plain').send(text); - app.get('/prov/*', async (req, reply) => { + app.get('/prov/*', { config: NO_LIMIT }, async (req, reply) => { if (!(await getSetting(db, 'provisioning.enabled'))) return deny(reply, 404, 'Not found'); const local = await isLocal(req.ip); if ((await getSetting(db, 'provisioning.lanOnly')) && !local) diff --git a/apps/api/src/routes/settings.ts b/apps/api/src/routes/settings.ts index b016179..5ab0a51 100644 --- a/apps/api/src/routes/settings.ts +++ b/apps/api/src/routes/settings.ts @@ -24,7 +24,7 @@ import { import type { Db } from '../db/client.ts'; import { dids, extensions, trunks } from '../db/schema.ts'; import { HttpError, badRequest } from '../errors.ts'; -import { requireAdmin } from '../auth/guards.ts'; +import { SLOW, requireAdmin } from '../auth/guards.ts'; import { encrypt } from '../crypto/secrets.ts'; import { audit } from '../services/audit.ts'; import { assertDestinationExists } from '../services/destinations.ts'; @@ -152,15 +152,21 @@ export async function settingsRoutes(app: FastifyInstance) { throw badRequest((err as Error).message); } const old = await getSetting(db, 'auth.ldap'); + const url = v.url.replace(/\/$/, ''); + // The stored password only goes back to the server and account it was typed for: else a + // changed address (or Test) would send it to whoever answers there. + const same = old?.url === url && old.bindDn === v.bindDn; + if (!v.bindPassword && !v.clearBindPassword && !same && v.bindDn && old?.bindPasswordEnc) + throw badRequest('The address or service account changed: enter its password again'); return { enabled: v.enabled, preset: v.preset, - url: v.url.replace(/\/$/, ''), + url, startTls: v.startTls, bindDn: v.bindDn, bindPasswordEnc: v.bindPassword ? encrypt(v.bindPassword) - : v.clearBindPassword + : v.clearBindPassword || !same ? null : (old?.bindPasswordEnc ?? null), baseDn: v.baseDn, @@ -181,6 +187,10 @@ export async function settingsRoutes(app: FastifyInstance) { throw badRequest((err as Error).message); } const old = await getSetting(db, 'auth.oidc'); + // As for the directory: the stored secret only goes to the provider it was typed for. + const same = old?.issuer === input.issuer && old.clientId === input.clientId; + if (!input.clientSecret && !input.clearClientSecret && !same && old?.clientSecretEnc) + throw badRequest('The issuer or client ID changed: enter the client secret again'); return { label: input.label, issuer: input.issuer, @@ -189,7 +199,9 @@ export async function settingsRoutes(app: FastifyInstance) { ? encrypt(input.clientSecret) : input.clearClientSecret ? null - : (old?.clientSecretEnc ?? null), + : same + ? (old.clientSecretEnc ?? null) + : null, scopes: input.scopes, groupsClaim: input.groupsClaim, adminGroups: input.adminGroups, @@ -217,7 +229,12 @@ export async function settingsRoutes(app: FastifyInstance) { secure: input.secure, username: input.username, from: input.from, - passwordEnc: input.password ? encrypt(input.password) : (old?.passwordEnc ?? ''), + // Kept only for the same server and account: a new host doesn't get the old password. + passwordEnc: input.password + ? encrypt(input.password) + : old?.host === input.host && old.username === input.username + ? old.passwordEnc + : '', }; } else if (key === 'contacts.contrack') { // The company token is write-only too (11 §4.3). @@ -311,7 +328,7 @@ export async function settingsRoutes(app: FastifyInstance) { return toDto(db); }); - app.post('/api/settings/smtp/test', async (req) => { + app.post('/api/settings/smtp/test', { config: SLOW }, async (req) => { const { to } = z.object({ to: z.email() }).parse(req.body); try { await app.sendMail({ @@ -326,7 +343,7 @@ export async function settingsRoutes(app: FastifyInstance) { }); /** Settings → Emergency → Test notifications (06 §6.5): email + ring-out, never dials 911. */ - app.post('/api/settings/emergency/test', async (req) => { + app.post('/api/settings/emergency/test', { config: SLOW }, async (req) => { const [ext] = await db.select().from(extensions).where(eq(extensions.userId, req.user!.id)); const notified = await notifyEmergency( { db, ami: app.ami, live: app.live, send: app.sendMail, log: req.log }, @@ -352,34 +369,42 @@ export async function settingsRoutes(app: FastifyInstance) { * Settings → Sign-in → Directory → Test, from the unsaved form (12 §4.1). The test password * is used once and never stored or logged. */ - app.post('/api/settings/auth/ldap/test', async (req): Promise => { - const { testLogin, testPassword, ...form } = LdapInputSchema.extend({ - testLogin: z.string().trim().max(254).optional(), - testPassword: z.string().max(200).optional(), - }).parse(req.body); - const s = (await ldapFromInput(form))!; - try { - const r = await testLdap(s, testLogin || undefined, testPassword); - if (!r.user) return { ok: true, matching: r.matching }; - const matches = await directoryUserFor(db, r.user); - return { - ok: true, - matching: r.matching, - user: { ...r.user, matches: matches?.email ?? null }, - }; - } catch (err) { - return { ok: false, error: (err as Error).message }; - } - }); + app.post( + '/api/settings/auth/ldap/test', + { config: SLOW }, + async (req): Promise => { + const { testLogin, testPassword, ...form } = LdapInputSchema.extend({ + testLogin: z.string().trim().max(254).optional(), + testPassword: z.string().max(200).optional(), + }).parse(req.body); + const s = (await ldapFromInput(form))!; + try { + const r = await testLdap(s, testLogin || undefined, testPassword); + if (!r.user) return { ok: true, matching: r.matching }; + const matches = await directoryUserFor(db, r.user); + return { + ok: true, + matching: r.matching, + user: { ...r.user, matches: matches?.email ?? null }, + }; + } catch (err) { + return { ok: false, error: (err as Error).message }; + } + }, + ); /** Settings → Sign-in → Test, from the unsaved form (12 §3.1). */ - app.post('/api/settings/auth/oidc/test', async (req): Promise => { - const s = await oidcFromInput(OidcInputSchema.parse(req.body)); - return testOidc(s!, app.config.PUBLIC_HOSTNAME); - }); + app.post( + '/api/settings/auth/oidc/test', + { config: SLOW }, + async (req): Promise => { + const s = await oidcFromInput(OidcInputSchema.parse(req.body)); + return testOidc(s!, app.config.PUBLIC_HOSTNAME); + }, + ); /** The server's public IPv4, as an outside service sees it (only sent when asked). */ - app.post('/api/settings/network/detect-public-ip', async () => { + app.post('/api/settings/network/detect-public-ip', { config: SLOW }, async () => { const ip = await detectPublicIp(); if (ip) return { ip }; throw new HttpError(502, 'unavailable', "Couldn't detect the public IP. Enter it by hand."); @@ -401,7 +426,7 @@ export async function settingsRoutes(app: FastifyInstance) { }); /** Settings → AI → Test (spec 10): the values in the form, saved or not. */ - app.post('/api/ai/test', async (req): Promise => { + app.post('/api/ai/test', { config: SLOW }, async (req): Promise => { const { kind } = z.object({ kind: z.enum(['llm', 'stt', 'tts']) }).parse(req.body); const ep = (kind === 'tts' ? AiTtsEndpointSchema : AiEndpointSchema).parse( (req.body as { endpoint?: unknown }).endpoint, diff --git a/apps/api/src/routes/system.ts b/apps/api/src/routes/system.ts index a5fc66b..82c9208 100644 --- a/apps/api/src/routes/system.ts +++ b/apps/api/src/routes/system.ts @@ -13,7 +13,7 @@ import type { SystemStatus, } from '@selfphoneng/shared'; import type { FastifyInstance } from 'fastify'; -import { requireAdmin } from '../auth/guards.ts'; +import { SLOW, requireAdmin } from '../auth/guards.ts'; import { audit } from '../services/audit.ts'; import { getAllSettings } from '../services/settings.ts'; import { systemEvents } from '../db/schema.ts'; @@ -175,14 +175,18 @@ export async function systemRoutes(app: FastifyInstance) { app.get('/api/system/diagnostics', async (): Promise => ({ checks: await insideChecks(diag()), })); - app.post('/api/system/diagnostics/network', async (req): Promise => { - const { mode } = z - .object({ mode: z.enum(['quick', 'complete']).default('quick') }) - .parse(req.body ?? {}); - if (!(await getAllSettings(app.db))['network.probeUrl']) - throw badRequest('No network test probe set (Settings → Network)'); - return networkTest(diag(), mode); - }); + app.post( + '/api/system/diagnostics/network', + { config: SLOW }, + async (req): Promise => { + const { mode } = z + .object({ mode: z.enum(['quick', 'complete']).default('quick') }) + .parse(req.body ?? {}); + if (!(await getAllSettings(app.db))['network.probeUrl']) + throw badRequest('No network test probe set (Settings → Network)'); + return networkTest(diag(), mode); + }, + ); app.get('/api/system/generated-files', async () => ({ files: await Promise.all( diff --git a/apps/api/src/routes/trunks.ts b/apps/api/src/routes/trunks.ts index 7844f4e..6bb3e2d 100644 --- a/apps/api/src/routes/trunks.ts +++ b/apps/api/src/routes/trunks.ts @@ -18,7 +18,7 @@ import { import type { Db } from '../db/client.ts'; import { dids, outboundRoutes, trunks } from '../db/schema.ts'; import { badRequest, conflict, HttpError, notFound } from '../errors.ts'; -import { requireAdmin } from '../auth/guards.ts'; +import { SLOW, requireAdmin } from '../auth/guards.ts'; import { decrypt, encrypt } from '../crypto/secrets.ts'; import { getDids, @@ -202,7 +202,7 @@ export async function trunkRoutes(app: FastifyInstance) { return { ok: true }; }); - app.post('/api/trunks/:id/test', async (req): Promise => { + app.post('/api/trunks/:id/test', { config: SLOW }, async (req): Promise => { const { id } = IdParams.parse(req.params); const t = await getTrunk(db, id); if (!t.enabled) throw badRequest('Turn the trunk on first'); diff --git a/apps/api/src/routes/users.ts b/apps/api/src/routes/users.ts index 4fe8491..371d095 100644 --- a/apps/api/src/routes/users.ts +++ b/apps/api/src/routes/users.ts @@ -11,7 +11,7 @@ import { } from '@selfphoneng/shared'; import type { Db } from '../db/client.ts'; import { extensions, userIdentities, users } from '../db/schema.ts'; -import { badRequest, conflict, notFound } from '../errors.ts'; +import { HttpError, badRequest, conflict, notFound } from '../errors.ts'; import { requireAdmin } from '../auth/guards.ts'; import { hashPassword } from '../auth/passwords.ts'; import { deleteUserSessions } from '../auth/sessions.ts'; @@ -32,6 +32,7 @@ export const toUserDto = ( u: UserRow, extension: UserDto['extension'] = null, ids: IdentityRow[] = [], + from: RoleSources = { oidc: null, ldap: false }, ): UserDto => ({ id: u.id, email: u.email, @@ -44,8 +45,26 @@ export const toUserDto = ( createdAt: u.createdAt.toISOString(), extension, signIn: signInWays(u, ids), + roleFrom: roleFrom(ids, from), }); +/** The providers that set roles now (12 §1): adminGroups / adminFilter set and in use. */ +type RoleSources = { oidc: string | null; ldap: boolean }; +async function roleSources(db: Db): Promise { + const [oidc, ldap, mode] = await Promise.all([ + getSetting(db, 'auth.oidc'), + getSetting(db, 'auth.ldap'), + getSetting(db, 'auth.mode'), + ]); + return { + oidc: oidc && mode !== 'password' && oidc.adminGroups.length > 0 ? oidc.label : null, + ldap: Boolean(ldap?.enabled && ldap.adminFilter), + }; +} +const roleFrom = (ids: { kind: string }[], from: RoleSources) => + (from.oidc && ids.some((i) => i.kind === 'oidc') && from.oidc) || + (from.ldap && ids.some((i) => i.kind === 'ldap') ? 'the directory' : null); + /** Connected accounts (12 §2), for one user or all of them. */ const identitiesOf = (db: Db, userId?: string): Promise => db @@ -116,12 +135,14 @@ export async function userRoutes(app: FastifyInstance) { .leftJoin(extensions, eq(extensions.userId, users.id)) .orderBy(asc(users.displayName), asc(users.id)); const ids = await identitiesOf(db); + const from = await roleSources(db); return { items: rows.map((r) => toUserDto( r.u, r.extId && r.extNumber ? { id: r.extId, number: r.extNumber } : null, ids.filter((i) => i.userId === r.u.id), + from, ), ), nextCursor: null, @@ -130,7 +151,12 @@ export async function userRoutes(app: FastifyInstance) { app.get('/api/users/:id', async (req) => { const { id } = IdParams.parse(req.params); - return toUserDto(await getUser(db, id), await extOf(db, id), await identitiesOf(db, id)); + return toUserDto( + await getUser(db, id), + await extOf(db, id), + await identitiesOf(db, id), + await roleSources(db), + ); }); app.post('/api/users', async (req, reply) => { @@ -169,6 +195,12 @@ export async function userRoutes(app: FastifyInstance) { const body = UserUpdateSchema.parse(req.body); if (Object.keys(body).length === 0) throw badRequest('Nothing to update'); const before = await getUser(db, id); + const ids = await identitiesOf(db, id); + const from = await roleSources(db); + // 12 §1: a provider that sets the role would undo this at the next sign-in or check. + const managed = roleFrom(ids, from); + if (body.role !== undefined && body.role !== before.role && managed) + throw new HttpError(409, 'role_managed', `The role comes from ${managed}`); const losesAdmin = before.role === 'admin' && before.isActive && @@ -199,7 +231,7 @@ export async function userRoutes(app: FastifyInstance) { after: Object.fromEntries(changed.map((k) => [k, after[k]])), }, }); - return toUserDto(after, await extOf(db, id), await identitiesOf(db, id)); + return toUserDto(after, await extOf(db, id), ids, from); }); app.delete('/api/users/:id', async (req) => { diff --git a/apps/api/src/sso/directory.ts b/apps/api/src/sso/directory.ts index c116de1..efbcbe5 100644 --- a/apps/api/src/sso/directory.ts +++ b/apps/api/src/sso/directory.ts @@ -23,7 +23,49 @@ 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). */ +export async function connectDirectory( + db: Db, + log: { info(o: object, m: string): void }, + userId: string, + e: DirectoryEntry, + ip: string, +): Promise<'connected' | 'taken'> { + 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'; + await db.transaction(async (tx) => { + await tx + .delete(userIdentities) + .where(and(eq(userIdentities.userId, userId), eq(userIdentities.kind, 'ldap'))); + await tx + .insert(userIdentities) + .values({ userId, kind: 'ldap', subject: e.id, email: e.email, lastUsedAt: new Date() }); + }); + await audit(db, { + actorUserId: userId, + actorIp: ip, + action: 'ldap_connected', + entityType: 'user', + entityId: userId, + summary: `Connected directory account ${e.dn}`, + }); + await applyRole(db, log, userId, e.admin, 'the directory', ip); + return 'connected'; } /** diff --git a/apps/api/src/sso/ldap.test.ts b/apps/api/src/sso/ldap.test.ts index 763c935..3ce48a6 100644 --- a/apps/api/src/sso/ldap.test.ts +++ b/apps/api/src/sso/ldap.test.ts @@ -1,10 +1,11 @@ // SPDX-License-Identifier: AGPL-3.0-or-later -import { BerWriter } from 'ldapts'; -import { describe, expect, it } from 'vitest'; +import { BerWriter, Client, UnwillingToPerformError } from 'ldapts'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { anyLogin, assertLdap, byIdFilter, + DirectoryUnavailable, ldapAuthenticate, withLogin, type LdapSettings, @@ -61,3 +62,40 @@ describe('LDAP (12 §4)', () => { expect(await ldapAuthenticate(s, 'ann', '')).toBeNull(); }); }); + +describe('a directory that answers no (12 §4.2)', () => { + afterEach(() => vi.restoreAllMocks()); + const s = { + url: 'ldaps://dir.example.com', + startTls: false, + bindDn: null, + baseDn: 'dc=example,dc=com', + userFilter: '(uid={login})', + idAttribute: 'entryUUID', + emailAttribute: 'mail', + nameAttribute: 'displayName', + adminFilter: null, + } as LdapSettings; + + it('a locked account (389 DS, FreeIPA: not code 49) is a refused password, not "unavailable"', async () => { + vi.spyOn(Client.prototype, 'search').mockResolvedValue({ + searchEntries: [{ dn: 'uid=ann,dc=example,dc=com', entryUUID: 'id-1' }], + searchReferences: [], + }); + vi.spyOn(Client.prototype, 'unbind').mockResolvedValue(); + vi.spyOn(Client.prototype, 'bind').mockRejectedValue( + new UnwillingToPerformError('Account locked'), + ); + expect(await ldapAuthenticate(s, 'ann', 'right-but-locked')).toBeNull(); + }); + + it('no answer at all is "unavailable"', async () => { + vi.spyOn(Client.prototype, 'search').mockResolvedValue({ + searchEntries: [{ dn: 'uid=ann,dc=example,dc=com', entryUUID: 'id-1' }], + searchReferences: [], + }); + vi.spyOn(Client.prototype, 'unbind').mockResolvedValue(); + vi.spyOn(Client.prototype, 'bind').mockRejectedValue(new Error('connect ECONNREFUSED')); + await expect(ldapAuthenticate(s, 'ann', 'pw')).rejects.toThrow(DirectoryUnavailable); + }); +}); diff --git a/apps/api/src/sso/ldap.ts b/apps/api/src/sso/ldap.ts index 93ef82a..300b047 100644 --- a/apps/api/src/sso/ldap.ts +++ b/apps/api/src/sso/ldap.ts @@ -8,6 +8,7 @@ import { FilterParser, InvalidCredentialsError, NoSuchObjectError, + ResultCodeError, SizeLimitExceededError, escapeFilter, type Entry, @@ -150,7 +151,10 @@ async function passwordWorks(s: LdapSettings, dn: string, password: string): Pro await u.bind(dn, password); return true; } catch (err) { - if (err instanceof InvalidCredentialsError) return false; + // The server answered (wrong password, locked, expired: 389 DS and FreeIPA say 19 or 53 + // for a locked account): a refused password. Only no answer at all is "unavailable", so + // locking someone's account can't turn their sign-in into a 503. + if (err instanceof ResultCodeError) return false; throw new DirectoryUnavailable((err as Error).message); } finally { await u.unbind().catch(() => {}); diff --git a/apps/api/src/sso/oidcCheck.ts b/apps/api/src/sso/oidcCheck.ts index 32e2d64..f86ec9d 100644 --- a/apps/api/src/sso/oidcCheck.ts +++ b/apps/api/src/sso/oidcCheck.ts @@ -8,7 +8,7 @@ import { decrypt, encrypt } from '../crypto/secrets.ts'; import { sessions, userIdentities, users } from '../db/schema.ts'; import { audit } from '../services/audit.ts'; import { getSetting } from '../services/settings.ts'; -import { decide, type ClaimsDecision } from './claims.ts'; +import { claimAt, decide, type ClaimsDecision } from './claims.ts'; import { applyRole } from './role.ts'; import { oidcConfig, oidcError, type OidcSettings } from './oidc.ts'; @@ -95,9 +95,13 @@ export async function checkOidcSessions(d: { db: Db; log: Log; now?: Date }): Pr try { tokens = await oidc.refreshTokenGrant(config, decrypt(row.refreshEnc!)); const sub = row.subject.slice(row.subject.indexOf(' ') + 1); - claims = config.serverMetadata().userinfo_endpoint - ? { ...(await oidc.fetchUserInfo(config, tokens.access_token, sub)) } - : { ...tokens.claims() }; + // As at sign-in: the ID token's claims, userinfo's on top. Entra's app roles and + // "ID token only" mappers are in the ID token alone. + claims = { + ...tokens.claims(), + ...(config.serverMetadata().userinfo_endpoint && + (await oidc.fetchUserInfo(config, tokens.access_token, sub))), + }; } catch (err) { // The provider said no: disabled, signed out there, or the refresh token expired. if (err instanceof oidc.ResponseBodyError && err.error === 'invalid_grant') { @@ -108,6 +112,21 @@ export async function checkOidcSessions(d: { db: Db; log: Log; now?: Date }): Pr log.warn?.({ err: oidcError(err) }, `OpenID check of a session failed`); continue; } + // No groups claim at all (the refresh brought no ID token, and userinfo doesn't carry it): + // nothing to decide groups on, so not "in no groups". Checked again next run. + const groupsKnown = claimAt(claims, s.groupsClaim) !== undefined; + const ruled = s.allowedGroups.length > 0 || s.adminGroups.length > 0; + if (ruled && !groupsKnown) { + log.info({ userId: row.userId }, 'OpenID check: no groups claim, role and access kept'); + await db + .update(sessions) + .set({ + idpCheckedAt: now, + ...(tokens.refresh_token && { idpRefreshEnc: encrypt(tokens.refresh_token) }), + }) + .where(eq(sessions.id, row.id)); + continue; + } const verdict = decide(s, claims); if (!verdict.allowed) { await end(row, `no longer in ${s.label}'s allowed groups`); diff --git a/apps/api/test/ad-live.test.ts b/apps/api/test/ad-live.test.ts index 854cb45..4f5fe94 100644 --- a/apps/api/test/ad-live.test.ts +++ b/apps/api/test/ad-live.test.ts @@ -94,8 +94,14 @@ describe.skipIf(!hasTestDb || !URL_)('Active Directory with a real Samba AD DC ( }); t = await createTestApp(); - for (const sam of [ann, bob, dan, cat]) + // New users with the temporary password: the directory email may connect them (12 §4.3). + for (const sam of [ann, bob, dan, cat]) { await t.addUser(`${sam}@example.com`, 'local-password-1'); + await t.db + .update(users) + .set({ mustChangePassword: true }) + .where(eq(users.email, `${sam}@example.com`)); + } await setSetting(t.db, 'auth.ldap', { enabled: true, preset: 'ad', diff --git a/apps/api/test/app.ts b/apps/api/test/app.ts index 41a1c3c..e6acf24 100644 --- a/apps/api/test/app.ts +++ b/apps/api/test/app.ts @@ -66,6 +66,7 @@ export async function createTestApp( INITIAL_ADMIN_PASSWORD: ADMIN.password, AMI_SECRET: 'ami-secret-123', LOG_LEVEL: 'fatal', + RATE_LIMIT_PER_MINUTE: '0', // the rate limit has its own test RECORDINGS_DIR: mkdtempSync(join(tmpdir(), 'sp-rec-')), SOUNDS_DIR: mkdtempSync(join(tmpdir(), 'sp-sounds-')), MOH_DIR: mkdtempSync(join(tmpdir(), 'sp-moh-')), diff --git a/apps/api/test/directory-signin.test.ts b/apps/api/test/directory-signin.test.ts new file mode 100644 index 0000000..273f73d --- /dev/null +++ b/apps/api/test/directory-signin.test.ts @@ -0,0 +1,119 @@ +// 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). +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 type { DirectoryEntry } from '../src/sso/ldap.ts'; +import { hasTestDb } from './db.ts'; +import { ADMIN, H, createTestApp } from './app.ts'; + +const log = { info: () => {} }; + +describe.skipIf(!hasTestDb)('directory accounts by email (12 §4.3)', () => { + let t: Awaited>; + let n = 0; + const entry = (email: string): DirectoryEntry => ({ + dn: `uid=e${++n}`, + id: `id-${n}`, + email, + name: email, + admin: null, + }); + const connected = async (userId: string) => + (await t.db.select().from(userIdentities).where(eq(userIdentities.userId, userId))).length; + + beforeAll(async () => { + t = await createTestApp(); + await t.clearMustChange(ADMIN.email); + }); + afterAll(() => t?.close()); + + it('never connects an admin, or someone who set their own password', async () => { + const admin = await t.addUser('boss@example.com', 'boss-password-1', 'admin'); + const own = await t.addUser('own@example.com', 'own-password-1'); + for (const u of [admin, own]) { + expect(await directorySignIn(t.db, log, entry(u.email), '127.0.0.1')).toBeNull(); + expect(await connected(u.id)).toBe(0); + } + }); + + it('connects someone with no password, or only the temporary one', async () => { + const fresh = await t.addUser('fresh@example.com', 'temporary-pw-1'); + await t.db.update(users).set({ mustChangePassword: true }).where(eq(users.id, fresh.id)); + const none = await t.addUser('none@example.com', 'unused-pw-1'); + await t.db.update(users).set({ passwordHash: null }).where(eq(users.id, none.id)); + for (const u of [fresh, none]) { + expect((await directorySignIn(t.db, log, entry(u.email), '127.0.0.1'))?.id).toBe(u.id); + expect(await connected(u.id)).toBe(1); + } + }); + + 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' }); + const mine = await t.login(u.email, 'leaver-password-1'); + const self = await t.app.inject({ + method: 'DELETE', + url: '/api/me/identities/ldap', + headers: { ...H, cookie: mine }, + }); + expect(self.statusCode).toBe(403); + expect(await connected(u.id)).toBe(1); + const admin = await t.login(ADMIN.email, ADMIN.password); + const byAdmin = await t.app.inject({ + method: 'DELETE', + url: `/api/users/${u.id}/identities/ldap`, + headers: { ...H, cookie: admin }, + }); + expect(byAdmin.statusCode).toBe(200); + expect(await connected(u.id)).toBe(0); + }); + + it('a directory that is down still counts each failed password: no free guesses', async () => { + resetLoginLimiter(); + await setSetting(t.db, 'auth.ldap', { + enabled: true, + preset: 'lldap', + url: 'ldap://127.0.0.1:1', // nothing listens there + startTls: false, + bindDn: null, + bindPasswordEnc: null, + baseDn: 'dc=example,dc=com', + userFilter: '(uid={login})', + idAttribute: 'entryUUID', + emailAttribute: 'mail', + nameAttribute: 'displayName', + adminFilter: null, + }); + try { + const codes: number[] = []; + for (let i = 0; i < 6; i++) + codes.push( + ( + await t.app.inject({ + method: 'POST', + url: '/api/auth/login', + headers: H, + payload: { login: ADMIN.email, password: `guess-${i}` }, + }) + ).statusCode, + ); + expect(codes).toEqual([503, 503, 503, 503, 503, 429]); + // Even the right password waits now: the guesses were counted. + const right = await t.app.inject({ + method: 'POST', + url: '/api/auth/login', + headers: H, + payload: { login: ADMIN.email, password: ADMIN.password }, + }); + expect(right.statusCode).toBe(429); + } finally { + await setSetting(t.db, 'auth.ldap', null); + resetLoginLimiter(); + } + }); +}); diff --git a/apps/api/test/fake-oidc.ts b/apps/api/test/fake-oidc.ts index 5930f42..8a738b2 100644 --- a/apps/api/test/fake-oidc.ts +++ b/apps/api/test/fake-oidc.ts @@ -11,6 +11,8 @@ export type FakeUser = { email_verified?: boolean; /** Only in userinfo, as Keycloak's mappers and Authelia send them. */ groups?: string[]; + /** Only in the ID token, as Entra's app roles. */ + roles?: string[]; }; const b64u = (b: Buffer | string) => Buffer.from(b).toString('base64url'); @@ -38,6 +40,8 @@ export async function startFakeOidc(alg: 'RS256' | 'ES256' = 'RS256') { /** true: the token endpoint answers 503, as a provider that's down. */ down: false, refreshes: 0, + /** false: a refresh brings no new ID token (allowed by OIDC Core §12.2). */ + refreshIdToken: true, }; const codes = new Map(); const access = new Map(); @@ -101,7 +105,8 @@ export async function startFakeOidc(alg: 'RS256' | 'ES256' = 'RS256') { st.refreshes++; if (!user || !st.refreshOk) return json(400, { error: 'invalid_grant' }); // Today's claims, as a real provider gives on refresh. - return json(200, tokens(st.user.sub === user.sub ? { ...st.user } : user, null)); + const t = tokens(st.user.sub === user.sub ? { ...st.user } : user, null); + return json(200, st.refreshIdToken ? t : { ...t, id_token: undefined }); } const c = codes.get(form.get('code') ?? ''); codes.delete(form.get('code') ?? ''); @@ -113,7 +118,8 @@ export async function startFakeOidc(alg: 'RS256' | 'ES256' = 'RS256') { if (url.pathname === '/userinfo') { const user = access.get((req.headers.authorization ?? '').replace(/^Bearer /, '')); if (!user) return json(401, { error: 'invalid_token' }); - return json(200, { ...user, ...(st.tamper.userinfoSub && { sub: 'someone-else' }) }); + const { roles: _, ...info } = user; + return json(200, { ...info, ...(st.tamper.userinfoSub && { sub: 'someone-else' }) }); } if (url.pathname === '/logout') return json(200, {}); json(404, {}); @@ -130,6 +136,7 @@ export async function startFakeOidc(alg: 'RS256' | 'ES256' = 'RS256') { exp: t.expired ? now - 3600 : now + 300, ...(nonce && { nonce: t.nonce ? 'wrong-nonce' : nonce }), ...(user.email && { email: user.email, email_verified: user.email_verified }), + ...(user.roles && { roles: user.roles }), }; const at = b64u(randomBytes(16)); const rt = b64u(randomBytes(16)); diff --git a/apps/api/test/ldap-live.test.ts b/apps/api/test/ldap-live.test.ts index 35f838a..7446c7d 100644 --- a/apps/api/test/ldap-live.test.ts +++ b/apps/api/test/ldap-live.test.ts @@ -38,6 +38,7 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', const ann = `ann${tag}`; const bob = `bob${tag}`; const cat = `cat${tag}`; + const dee = `dee${tag}`; const group = `pbx-admins-${tag}`; let t: Awaited>; let jwt: string; @@ -86,6 +87,7 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', [ann, 'Ann Lee'], [bob, 'Bob Roe'], [cat, 'Cat Poe'], + [dee, 'Dee Fox'], ]) await gql( `mutation { createUser(user: { id: "${id}", email: "${id}@example.com", displayName: "${name}" }) { id } }`, @@ -96,7 +98,7 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', await gql(`mutation { addUserToGroup(userId: "${ann}", groupId: ${g.createGroup.id}) { ok } }`); const c = new Client({ url: URL_! }); await c.bind(`uid=admin,ou=people,${BASE}`, ADMIN_PASS); - for (const id of [ann, bob, cat]) + for (const id of [ann, bob, cat, dee]) await c.exop( '1.3.6.1.4.1.4203.1.11.1', passwordModify(`uid=${id},ou=people,${BASE}`, `${id}-directory-pw`), @@ -106,14 +108,21 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', t = await createTestApp(); await t.clearMustChange(ADMIN.email); admin = await t.login(ADMIN.email, ADMIN.password); - await t.addUser(`${ann}@example.com`, 'ann-local-pw-12'); - await t.addUser(`${cat}@example.com`, 'cat-local-pw-12'); + // New users with the temporary password from Users: the directory email may connect them. + for (const who of [ann, cat]) { + await t.addUser(`${who}@example.com`, `${who}-local-pw-12`); + await t.db + .update(users) + .set({ mustChangePassword: true }) + .where(eq(users.email, `${who}@example.com`)); + } + await t.addUser(`${dee}@example.com`, 'dee-own-password-12'); // chose her own password await setSetting(t.db, 'auth.ldap', settings()); }, 60_000); afterAll(async () => { await t?.close(); - for (const id of [ann, bob, cat]) + for (const id of [ann, bob, cat, dee]) await gql(`mutation { deleteUser(userId: "${id}") { ok } }`).catch(() => {}); }); @@ -142,7 +151,7 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', it('signs in by email too, and the local password still works', async () => { resetLoginLimiter(); expect((await login(`${ann}@example.com`, `${ann}-directory-pw`)).statusCode).toBe(200); - expect((await login(`${ann}@example.com`, 'ann-local-pw-12')).statusCode).toBe(200); + expect((await login(`${ann}@example.com`, `${ann}-local-pw-12`)).statusCode).toBe(200); }); it('a wrong password, an empty one, and a stranger are refused alike', async () => { @@ -170,7 +179,7 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', const r = await login(ann, `${ann}-directory-pw`); expect(r.statusCode).toBe(503); expect(r.json().error.code).toBe('directory_unavailable'); - expect((await login(`${ann}@example.com`, 'ann-local-pw-12')).statusCode).toBe(200); + expect((await login(`${ann}@example.com`, `${ann}-local-pw-12`)).statusCode).toBe(200); } finally { await setSetting(t.db, 'auth.ldap', settings()); } @@ -198,6 +207,23 @@ describe.skipIf(!hasTestDb || !URL_ || !HTTP)('LDAP with a real lldap (12 S4)', expect(bad.json()).toMatchObject({ ok: false, error: expect.stringContaining('refused') }); }); + it('someone with their own password connects on My settings, never by email alone', async () => { + resetLoginLimiter(); + expect((await login(dee, `${dee}-directory-pw`)).statusCode).toBe(401); + const mine = await t.login(`${dee}@example.com`, 'dee-own-password-12'); + const connect = (password: string) => + t.app.inject({ + method: 'POST', + url: '/api/me/identities/ldap', + headers: { ...H, cookie: mine }, + payload: { login: dee, password }, + }); + expect((await connect('wrong-password')).statusCode).toBe(400); + expect((await connect(`${dee}-directory-pw`)).statusCode).toBe(200); + resetLoginLimiter(); + expect((await login(dee, `${dee}-directory-pw`)).statusCode).toBe(200); + }); + it('directoryCheck disables someone removed from the directory, and only them (§4.5)', async () => { resetLoginLimiter(); expect((await login(cat, `${cat}-directory-pw`)).statusCode).toBe(200); // connects cat diff --git a/apps/api/test/oidc-live.test.ts b/apps/api/test/oidc-live.test.ts index 494dd2a..1b0efae 100644 --- a/apps/api/test/oidc-live.test.ts +++ b/apps/api/test/oidc-live.test.ts @@ -128,12 +128,22 @@ describe.skipIf(!hasTestDb || !KC)('OpenID Connect with a real Keycloak (12 S3)' /** start → Keycloak's login form, filled in like a person would → our callback. */ async function flow(cookie?: string, query = '') { - const start = await t.app.inject({ - url: `/api/auth/oidc/start${query}`, - headers: cookie ? { cookie } : {}, - }); + const connecting = query === '?connect=1'; + // Connect is a POST from My account (with the CSRF header); sign-in a GET from the button. + const start = connecting + ? await t.app.inject({ + method: 'POST', + url: '/api/auth/oidc/connect', + headers: { ...H, cookie: cookie! }, + payload: {}, + }) + : await t.app.inject({ + url: `/api/auth/oidc/start${query}`, + headers: cookie ? { cookie } : {}, + }); + const location = connecting ? (start.json() as { url: string }).url : start.headers.location; const pending = start.cookies.find((c) => c.name === 'sp_oidc')!; - let res = await browser.get(start.headers.location as string); + let res = await browser.get(location as string); if (res.status === 200) { // Not signed in at Keycloak yet: post its login form. const html = await res.text(); diff --git a/apps/api/test/oidc.test.ts b/apps/api/test/oidc.test.ts index f881295..92fa741 100644 --- a/apps/api/test/oidc.test.ts +++ b/apps/api/test/oidc.test.ts @@ -3,7 +3,7 @@ // provider (test/fake-oidc.ts). import { and, desc, eq } from 'drizzle-orm'; import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; -import { encrypt } from '../src/crypto/secrets.ts'; +import { decrypt, encrypt } from '../src/crypto/secrets.ts'; import { sessions, userIdentities, users } from '../src/db/schema.ts'; import { resetLoginLimiter } from '../src/auth/login-limiter.ts'; import { setSetting } from '../src/services/settings.ts'; @@ -49,18 +49,29 @@ for (const alg of ['RS256', 'ES256'] as const) idp.st.user = { sub: 'u-1', email: 'ann@example.com', email_verified: true }; idp.st.refreshOk = true; idp.st.down = false; + idp.st.refreshIdToken = true; }); /** The browser's round trip: start → provider → callback. */ async function flow(cookie?: string, query = '') { - const start = await t.app.inject({ - url: `/api/auth/oidc/start${query}`, - headers: cookie ? { cookie } : {}, - }); - expect(start.statusCode).toBe(302); + const connecting = query === '?connect=1'; + // Connect is a POST from My account (with the CSRF header); sign-in a GET from the button. + const start = connecting + ? await t.app.inject({ + method: 'POST', + url: '/api/auth/oidc/connect', + headers: { ...H, cookie: cookie! }, + payload: {}, + }) + : await t.app.inject({ + url: `/api/auth/oidc/start${query}`, + headers: cookie ? { cookie } : {}, + }); + const location = connecting ? (start.json() as { url: string }).url : start.headers.location; + expect(start.statusCode).toBe(connecting ? 200 : 302); const pending = start.cookies.find((c) => c.name === 'sp_oidc'); if (!pending) return { start, callback: null }; - const atIdp = await fetch(start.headers.location as string, { redirect: 'manual' }); + const atIdp = await fetch(location as string, { redirect: 'manual' }); const back = new URL(atIdp.headers.get('location')!); expect(back.origin).toBe('https://pbx.test'); const callback = await t.app.inject({ @@ -81,6 +92,43 @@ for (const alg of ['RS256', 'ES256'] as const) expect(r.json()).toEqual({ mode: 'mixed', ldap: false, oidc: { label: 'Keycloak' } }); }); + it('never sends the browser to another site after sign-in (a tab, a backslash, //)', async () => { + await t.addUser('nextcheck@example.com', 'next-check-pw-1'); + idp.st.user = { sub: 'u-next', email: 'nextcheck@example.com', email_verified: true }; + const pw = await t.login('nextcheck@example.com', 'next-check-pw-1'); + await flow(pw, '?connect=1'); + for (const next of [ + '/\t/evil.example', + '/\n/evil.example', + '/\\evil.example', + '//evil.example', + ]) + expect( + (await flow(undefined, `?next=${encodeURIComponent(next)}`)).callback!.headers.location, + ).toBe('/'); + expect((await flow(undefined, '?next=/me/calls')).callback!.headers.location).toBe( + '/me/calls', + ); + }); + + it('connect is a POST with the CSRF header; a GET with connect=1 only signs in', async () => { + const ann = await t.login(ADMIN.email, ADMIN.password); + const forged = await t.app.inject({ + method: 'POST', + url: '/api/auth/oidc/connect', + headers: { cookie: ann }, + payload: {}, + }); + expect(forged.statusCode).toBe(403); + expect(forged.json().error.code).toBe('csrf'); + const r = await t.app.inject({ + url: '/api/auth/oidc/start?connect=1', + headers: { cookie: ann }, + }); + const pending = r.cookies.find((c) => c.name === 'sp_oidc')!; + expect(JSON.parse(decrypt(pending.value)).connectSessionId).toBeNull(); + }); + it('starts with PKCE S256, state and nonce, and a short-lived cookie', async () => { const r = await t.app.inject({ url: '/api/auth/oidc/start?next=/me/calls' }); const to = new URL(r.headers.location as string); @@ -403,4 +451,27 @@ for (const alg of ['RS256', 'ES256'] as const) await setSetting(t.db, 'auth.oidc', oidcSettings); } }); + + it('the session check reads roles that are only in the ID token, and keeps a session whose refresh brings no groups', async () => { + idp.st.user = { ...idp.st.user, roles: ['pbx-users'] }; // Entra: app roles, ID token only + await setSetting(t.db, 'auth.oidc', { + ...oidcSettings, + groupsClaim: 'roles', + allowedGroups: ['pbx-users'], + }); + try { + const { id } = await annSession(); + await check(20); + expect(await row(id)).toBeTruthy(); + idp.st.refreshIdToken = false; // no new ID token: no roles claim at all + await check(40); + expect(await row(id)).toBeTruthy(); + idp.st.refreshIdToken = true; + idp.st.user.roles = []; // the claim is there, and empty: ended + await check(60); + expect(await row(id)).toBeUndefined(); + } finally { + await setSetting(t.db, 'auth.oidc', oidcSettings); + } + }); }); diff --git a/apps/api/test/rate-limit.test.ts b/apps/api/test/rate-limit.test.ts new file mode 100644 index 0000000..700f8a8 --- /dev/null +++ b/apps/api/test/rate-limit.test.ts @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later +// 08 §2.4: every route is rate limited per client IP, Test buttons more, and a signed-in +// person's password asked again counts like a sign-in. +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { resetLoginLimiter } from '../src/auth/login-limiter.ts'; +import { hasTestDb } from './db.ts'; +import { ADMIN, H, createTestApp } from './app.ts'; + +describe.skipIf(!hasTestDb)('rate limits (08 §2.4)', () => { + let t: Awaited>; + let admin: string; + beforeAll(async () => { + t = await createTestApp(undefined, { RATE_LIMIT_PER_MINUTE: '30' }); + await t.clearMustChange(ADMIN.email); + admin = await t.login(ADMIN.email, ADMIN.password); + }); + afterAll(() => t?.close()); + + it('Test buttons: 10 a minute', async () => { + const codes: number[] = []; + for (let i = 0; i < 11; i++) + codes.push( + ( + await t.app.inject({ + method: 'POST', + url: '/api/settings/smtp/test', + headers: { ...H, cookie: admin }, + payload: {}, + }) + ).statusCode, + ); + expect(codes.slice(0, 10)).not.toContain(429); + expect(codes[10]).toBe(429); + }); + + it('a password asked again counts like a sign-in: guessing it is stopped', async () => { + resetLoginLimiter(); + const codes: number[] = []; + for (let i = 0; i < 6; i++) + codes.push( + ( + await t.app.inject({ + method: 'POST', + url: '/api/auth/change-password', + headers: { ...H, cookie: admin }, + payload: { currentPassword: `guess-${i}-wrong`, newPassword: 'another-password-1' }, + }) + ).statusCode, + ); + expect(codes.slice(0, 5)).toEqual([400, 400, 400, 400, 400]); + expect(codes[5]).toBe(429); + resetLoginLimiter(); + }); + + it('/prov keeps its own limits (07 §2), outside the global one', async () => { + const prov = await t.app.inject({ url: '/prov/001565000000.cfg' }); + expect(prov.headers['x-ratelimit-limit']).toBeUndefined(); + const api = await t.app.inject({ url: '/api/auth/methods' }); + expect(api.headers['x-ratelimit-limit']).toBe('30'); + }); + + it('every other route: the global limit per IP, as a 429 rate_limited', async () => { + let r; + for (let i = 0; i < 40; i++) { + r = await t.app.inject({ url: '/api/auth/methods' }); + if (r.statusCode === 429) break; + } + expect(r!.statusCode).toBe(429); + expect(r!.json().error.code).toBe('rate_limited'); + }); +}); diff --git a/apps/api/test/sign-in.test.ts b/apps/api/test/sign-in.test.ts index bc3e0cc..5531847 100644 --- a/apps/api/test/sign-in.test.ts +++ b/apps/api/test/sign-in.test.ts @@ -111,4 +111,123 @@ describe.skipIf(!hasTestDb)('sign-in (12 S1)', () => { }); expect(del.statusCode).toBe(200); }); + + const patch = (payload: object, cookie = admin) => + t.app.inject({ method: 'PATCH', url: '/api/settings', payload, headers: { ...H, cookie } }); + + it('a stored secret never goes to a new server: the directory, OpenID and email', async () => { + const ldap = { + enabled: false, + preset: 'lldap', + url: 'ldaps://dir.example.com', + startTls: false, + bindDn: 'uid=svc,dc=example,dc=com', + bindPassword: 'service-pw-1', + baseDn: 'dc=example,dc=com', + userFilter: '(uid={login})', + idAttribute: 'entryUUID', + emailAttribute: 'mail', + nameAttribute: 'displayName', + adminFilter: '', + }; + expect((await patch({ 'auth.ldap': ldap })).statusCode).toBe(200); + const { bindPassword: _, ...keep } = ldap; + expect((await patch({ 'auth.ldap': keep })).statusCode).toBe(200); // same server: kept + const moved = await patch({ 'auth.ldap': { ...keep, url: 'ldaps://evil.example' } }); + expect(moved.statusCode).toBe(400); + expect(moved.json().error.message).toMatch(/enter its password again/); + const test = await post( + '/api/settings/auth/ldap/test', + { ...keep, url: 'ldaps://evil.example' }, + admin, + ); + expect(test.statusCode).toBe(400); // Test too: it would bind there with the stored password + const otherDn = await patch({ + 'auth.ldap': { ...keep, bindDn: 'uid=other,dc=example,dc=com' }, + }); + expect(otherDn.statusCode).toBe(400); + + const oidc = { + label: 'Keycloak', + issuer: 'https://kc.example.com/realms/x', + clientId: 'selfphoneng', + clientSecret: 'client-secret-1', + scopes: 'openid email profile', + groupsClaim: 'groups', + adminGroups: [], + allowedGroups: [], + trustEmail: false, + }; + expect((await patch({ 'auth.oidc': oidc })).statusCode).toBe(200); + const { clientSecret: __, ...noSecret } = oidc; + const newIssuer = await patch({ 'auth.oidc': { ...noSecret, issuer: 'https://evil.example' } }); + expect(newIssuer.statusCode).toBe(400); + expect(newIssuer.json().error.message).toMatch(/client secret again/); + expect((await patch({ 'auth.oidc': noSecret })).json()['auth.oidc'].hasClientSecret).toBe(true); + + const smtp = { + host: 'mail.example.com', + port: 587, + secure: false, + username: 'pbx', + password: 'smtp-pw-1', + from: 'PBX ', + }; + expect((await patch({ smtp })).statusCode).toBe(200); + const { password: ___, ...smtpKeep } = smtp; + expect((await patch({ smtp: smtpKeep })).json().smtp.hasPassword).toBe(true); + expect( + (await patch({ smtp: { ...smtpKeep, host: 'evil.example' } })).json().smtp.hasPassword, + ).toBe(false); + await patch({ 'auth.ldap': null, 'auth.oidc': null, smtp: null }); + }); + + it('a role set by the directory or the provider is read-only here (409 role_managed)', async () => { + const u = await t.addUser('managed@example.com', 'managed-pw-12'); + await t.db.insert(userIdentities).values({ userId: u.id, kind: 'ldap', subject: 'managed-id' }); + const ldap = { + enabled: true, + preset: 'lldap' as const, + url: 'ldaps://dir.example.com', + startTls: false, + bindDn: null, + bindPasswordEnc: null, + baseDn: 'dc=example,dc=com', + userFilter: '(uid={login})', + idAttribute: 'entryUUID', + emailAttribute: 'mail', + nameAttribute: 'displayName', + adminFilter: null, + }; + await setSetting(t.db, 'auth.ldap', ldap); + const setRole = (role: string) => + t.app.inject({ + method: 'PATCH', + url: `/api/users/${u.id}`, + payload: { role }, + headers: { ...H, cookie: admin }, + }); + try { + expect((await setRole('admin')).statusCode).toBe(200); // no admin filter: set here + await setRole('user'); + await setSetting(t.db, 'auth.ldap', { ...ldap, adminFilter: '(memberOf=cn=pbx-admins)' }); + const r = await setRole('admin'); + expect(r.statusCode).toBe(409); + expect(r.json().error.code).toBe('role_managed'); + const dto = await t.app.inject({ url: `/api/users/${u.id}`, headers: { cookie: admin } }); + expect(dto.json().roleFrom).toBe('the directory'); + } finally { + await setSetting(t.db, 'auth.ldap', null); + } + }); + + it('an empty Contrack update is a 400, not a 500', async () => { + const r = await t.app.inject({ + method: 'PATCH', + url: '/api/me/contrack', + payload: {}, + headers: { ...H, cookie: admin }, + }); + expect(r.statusCode).toBe(400); + }); }); diff --git a/apps/web/src/api/client.ts b/apps/web/src/api/client.ts index f085d09..4885981 100644 --- a/apps/web/src/api/client.ts +++ b/apps/web/src/api/client.ts @@ -172,6 +172,11 @@ export const authApi = { methods: () => api('GET', '/api/auth/methods'), /** 12 §3.3: disconnect my OpenID account. */ disconnectOidc: () => api<{ ok: true }>('DELETE', '/api/me/identities/oidc'), + /** 12 §3.3: where to send the browser to connect my OpenID account. */ + connectOidc: (next: string) => api<{ url: string }>('POST', '/api/auth/oidc/connect', { next }), + /** 12 §4.3: connect my directory account with its username and password. */ + connectLdap: (login: string, password: string) => + api<{ ok: true }>('POST', '/api/me/identities/ldap', { login, password }), changePassword: (currentPassword: string, newPassword: string) => api<{ ok: true }>('POST', '/api/auth/change-password', { currentPassword, newPassword }), }; diff --git a/apps/web/src/components/SignInCard.tsx b/apps/web/src/components/SignInCard.tsx index e476790..398e392 100644 --- a/apps/web/src/components/SignInCard.tsx +++ b/apps/web/src/components/SignInCard.tsx @@ -1,10 +1,12 @@ // SPDX-License-Identifier: AGPL-3.0-or-later -// My sign-in ways (12 §5): the password, and connecting or disconnecting an OpenID account. +// My sign-in ways (12 §5): the password, connecting or disconnecting an OpenID account, and +// connecting a directory account. +import { useState } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { Link, useLocation, useSearchParams } from 'react-router'; import { authApi } from '../api/client.ts'; import { useMe } from '../auth/RequireAuth.tsx'; -import { Alert, Button } from './ui.tsx'; +import { Alert, Button, Field, inputClass } from './ui.tsx'; /** What `?sso_error=` means (12 §3.2), for the login page and here. */ export function ssoErrorText(code: string, label: string): string { @@ -34,6 +36,10 @@ export function SignInCard() { mutationFn: authApi.disconnectOidc, onSuccess: () => qc.invalidateQueries({ queryKey: ['me'] }), }); + const connect = useMutation({ + mutationFn: () => authApi.connectOidc(loc.pathname), + onSuccess: (r) => window.location.assign(r.url), + }); const ways = me.data?.signIn; const oidc = methods.data?.oidc; if (!ways) return null; @@ -59,10 +65,13 @@ export function SignInCard() {

)} - {ways.ldap && ( + {ways.ldap ? (

- Your company directory account is connected: its password works here too. + Your company directory account is connected: its password works here too. Only an admin + can disconnect it.

+ ) : ( + methods.data?.ldap && )} {oidc && (
@@ -87,14 +96,15 @@ export function SignInCard() { Connect your {oidc.label} account to sign in with it as well.

- {/* A full page load: the browser goes to the provider and comes back. */} - connect.mutate()} + disabled={connect.isPending || connect.isSuccess} > Connect {oidc.label} - +
+ {connect.error && {connect.error.message}} )}
@@ -102,3 +112,59 @@ export function SignInCard() { ); } + +/** Connect my directory account (12 §4.3): its username and password, checked by the directory. */ +function DirectoryConnect() { + const qc = useQueryClient(); + const [v, setV] = useState({ login: '', password: '' }); + const connect = useMutation({ + mutationFn: () => authApi.connectLdap(v.login, v.password), + onSuccess: () => { + setV({ login: '', password: '' }); + return qc.invalidateQueries({ queryKey: ['me'] }); + }, + }); + return ( +
{ + e.preventDefault(); + connect.mutate(); + }} + > +

Company directory

+

+ Connect your directory account to sign in with its password as well. +

+ + {(a) => ( + setV({ ...v, login: e.target.value })} + /> + )} + + + {(a) => ( + setV({ ...v, password: e.target.value })} + /> + )} + +
+ +
+ {connect.error && {connect.error.message}} +
+ ); +} diff --git a/apps/web/src/pages/admin/UserDrawer.tsx b/apps/web/src/pages/admin/UserDrawer.tsx index 2843d5f..5782335 100644 --- a/apps/web/src/pages/admin/UserDrawer.tsx +++ b/apps/web/src/pages/admin/UserDrawer.tsx @@ -192,11 +192,20 @@ function GeneralTab({ label="Role" error={errors.role?.message} hint={ - isSelf ? 'You cannot remove your own admin role if you are the last admin.' : undefined + user?.roleFrom + ? `From ${user.roleFrom}: change it there.` + : isSelf + ? 'You cannot remove your own admin role if you are the last admin.' + : undefined } > {(a) => ( - diff --git a/docs/admin-guide/sign-in.md b/docs/admin-guide/sign-in.md index 7d68491..a9ef175 100644 --- a/docs/admin-guide/sign-in.md +++ b/docs/admin-guide/sign-in.md @@ -118,9 +118,22 @@ Keycloak (26.8.1) is the one SelfPhoneNG is tested against. ## Directory (LDAP) With a directory, people sign in with their **directory username or email and password**. When -the password isn't the one set in SelfPhoneNG, SelfPhoneNG asks the directory. The first time, -they're matched to their user here by the directory's email; nobody gets a user just for being in -the directory. Their SelfPhoneNG password, if they have one, keeps working too. +the password isn't the one set in SelfPhoneNG, SelfPhoneNG asks the directory. Nobody gets a user +just for being in the directory, and their SelfPhoneNG password, if they have one, keeps working +too. + +The first time, the directory account is connected to their user here: + +- **New users** (still on the temporary password from Users, or with none): automatically, by the + directory's email. +- **Everyone else, and every admin**: they sign in with their SelfPhoneNG password once, then + **My settings** (admins: **My account**) → **Sign-in** → **Connect directory account** with their + directory username and password. An email alone never connects an admin or someone who chose + their own password, so a directory that lets people edit their own email can't hand out an + account. + +Only an admin can disconnect a directory account (Users → the user → Sign-in): it's what the +hourly leaver check follows. **Settings → Sign-in → Directory (LDAP)**: diff --git a/docs/spec/01-infrastructure.md b/docs/spec/01-infrastructure.md index 7293db1..2e0defe 100644 --- a/docs/spec/01-infrastructure.md +++ b/docs/spec/01-infrastructure.md @@ -121,6 +121,7 @@ APP_UID=1000 APP_GID=1000 LOG_LEVEL=info TRUST_PROXY=true # true when behind Caddy/nginx +RATE_LIMIT_PER_MINUTE=600 # requests per minute per client IP, every route; 0 = off #NODE_EXTRA_CA_CERTS=/var/lib/selfphoneng/tls/idp-ca.pem # a private CA for the OpenID provider or LDAPS (12 §1) ``` diff --git a/docs/spec/04-backend-api.md b/docs/spec/04-backend-api.md index 5d94422..4d91d9b 100644 --- a/docs/spec/04-backend-api.md +++ b/docs/spec/04-backend-api.md @@ -33,7 +33,12 @@ - **CSRF:** every non-GET `/api/*` request with cookie auth must carry the header `X-Requested-With: selfphoneng`. The web client always sends it. Browsers can't send it cross-site without CORS, and CORS is off. -- Login rate limit: 5 failures per IP per 15 min and 10 per email per hour → HTTP 429. +- Login rate limit: 5 failures per IP per 15 min and 10 per email per hour → HTTP 429. A signed-in + person's password asked again (change password, two-step on/off, recovery codes) counts the same. +- Every route: `@fastify/rate-limit`, `RATE_LIMIT_PER_MINUTE` (default 600) per client IP → 429 + `rate_limited`. Test buttons, diagnostics and the source download: 10 a minute (`SLOW` in + `auth/guards.ts`). `/prov` (its own limits, 07 §2) and the caller-name route (Asterisk on this + machine) are outside it (`NO_LIMIT`). Write `login_failed` to the audit log with the IP. - If `must_change_password`, every route except `/api/auth/*` returns 403 `{code:'password_change_required'}`. - Single sign-on (spec 12, as built in S1): the body is `{login, password, totp?}` (`login`: email, or a diff --git a/docs/spec/08-security-ops.md b/docs/spec/08-security-ops.md index 49231c5..ff21fd3 100644 --- a/docs/spec/08-security-ops.md +++ b/docs/spec/08-security-ops.md @@ -77,6 +77,8 @@ get the proxy's address only (below). The setting is on Settings → Security, w `Permissions-Policy: microphone=(self), camera=()`. - HSTS is set by the reverse proxy (Caddy does it by default; nginx example adds `add_header Strict-Transport-Security "max-age=31536000" always;`). - `TRUST_PROXY=true` → Fastify `trustProxy: '127.0.0.1'` only. +- Rate limits on every route (04 §2.2): `RATE_LIMIT_PER_MINUTE` per client IP, stricter on Test + buttons; login and a password asked again share the login limiter. - Admin TOTP (M11): RFC 6238, 30 s, 6 digits, ±1 window, 10 single-use recovery codes (stored hashed). - As built (M11-T1), TOTP (`auth/totp.ts`, node:crypto only, RFC 6238 test vectors in its test): opt-in for admins on **My account** (header → Account): scan the QR or type the key, confirm one diff --git a/docs/spec/12-sso.md b/docs/spec/12-sso.md index 79c66b8..7634a83 100644 --- a/docs/spec/12-sso.md +++ b/docs/spec/12-sso.md @@ -36,7 +36,9 @@ the softphone's SIP login are untouched: a phone never talks to the identity pro warning). - **Extensions** stay manual: an admin assigns one on Users. - **Our own session**: every sign-in makes the same `sp_session` cookie as today (04 §2.2, 14 days, - sliding). `sessions.method` says how it was made. Provider tokens never reach the browser. + sliding). `sessions.method` says how it was made. Provider tokens never reach the browser, except + the ID token as `id_token_hint` in the sign-out redirect to the provider (§3.5; RP-initiated + logout works that way). - **Two-step sign-in** (08 §2.4, owner 2026-10-25): password sign-ins ask for our TOTP code when it's on, whether the password was checked here or by the directory. OpenID sign-ins don't: MFA there is the provider's job. @@ -155,7 +157,10 @@ rotation, and builds the end-session URL. We don't hand-roll any of that. 1. Login page (`mixed`, `oidc`): **Sign in with