diff --git a/.changeset/auth-lockout-resolved-account.md b/.changeset/auth-lockout-resolved-account.md new file mode 100644 index 0000000000..1c15c4c3cb --- /dev/null +++ b/.changeset/auth-lockout-resolved-account.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-auth': patch +--- + +fix(plugin-auth): the account lockout resolves the account the sign-in credential check resolves + +Clause-②: no + +- **One account per sign-in.** The ADR-0069 D2 lock gate, its failed sign-in counter and the `login_failed` / `account_locked` account events now share one resolver. It looks the account up exactly as better-auth's `/sign-in/email` credential check does, so all four name the same account for every identifier the credential check resolves to it. +- **What that guarantees.** Every refused sign-in that resolves to an account counts toward that account's one lock. An existing lock refuses every sign-in that resolves to the account, including one with the correct password. A successful sign-in clears the counter, as before. An identifier that resolves to no account changes no account and writes no account event. +- **Nothing to migrate.** The lockout state stays on the account's own `sys_user` row (`failed_login_count`, `locked_until`), written by its id, and `lockoutThreshold` / `lockoutDurationMinutes` keep their meaning. diff --git a/content/docs/permissions/authentication.mdx b/content/docs/permissions/authentication.mdx index f8eb34fa71..acd4b87626 100644 --- a/content/docs/permissions/authentication.mdx +++ b/content/docs/permissions/authentication.mdx @@ -564,6 +564,13 @@ so a locked password stage and a locked second factor are independent states. Ea counter resets on a success at its own stage, and the admin **Unlock Account** action clears both at once. +The password-stage counter and lock belong to the **account**, not to the text +typed into the sign-in form. Both live on the `sys_user` row that the sign-in's own +credential check resolves the address to. So every refused sign-in on that account +counts toward its one lock, whichever spelling of the address it used. A lock refuses +every sign-in that resolves to the account, the correct password included, and a +successful sign-in under any spelling clears the counter. + Setting `lockout_threshold` to `0` disables the **password-stage** lockout only. Two-factor verification keeps a built-in limit of 10 attempts per 15 minutes, because it is the last check before a session is issued. Two-factor verification diff --git a/packages/plugins/plugin-auth/src/account-lockout-resolved-account.test.ts b/packages/plugins/plugin-auth/src/account-lockout-resolved-account.test.ts new file mode 100644 index 0000000000..c14b262184 --- /dev/null +++ b/packages/plugins/plugin-auth/src/account-lockout-resolved-account.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// ADR-0069 D2 — the account lockout belongs to the ACCOUNT, never to the text +// a sign-in typed. +// +// ## The invariant +// +// A password sign-in names an account through its credential check +// (better-auth's `/sign-in/email`). The lockout's counter and its lock gate +// must name that same account for every identifier the credential check +// resolves to it: +// +// - every refused sign-in that resolves to the account counts toward its one +// lock, whichever spelling of the address it carried; +// - an existing lock refuses every sign-in that resolves to the account, the +// correct password included; +// - a successful sign-in under any spelling clears the counter, as before; +// - an identifier that resolves to no account changes no account and writes +// no account event. +// +// The account events (`login_failed` / `account_locked`) name the account the +// same resolution names, so the ledger and the lock never disagree. +// +// ## Why the server here is the real one +// +// "Which account does this identifier name" is answered by the credential +// check, so every case drives a real `AuthManager` through +// `AuthManager.handleRequest` over the in-memory engine this package's +// better-auth suites share, whose equality is exact — the comparison a SQL +// column makes by default. The lock state is read back from the `sys_user` +// row itself. + +import { describe, it, expect, afterAll } from 'vitest'; +import { AuthManager } from './auth-manager'; +import { createMemoryEngine } from './impersonation-bearer-rotation.test'; +import { inviteForAudienceGate } from './audience-gate-test-support'; +import { memoizeScrypt } from './__tests__/scrypt-memo.testkit'; +import type { AuthSessionAuditEventInput } from './auth-session-audit'; + +const scrypt = memoizeScrypt(); +afterAll(scrypt.restore); + +const SECRET = 'test-secret-at-least-32-chars-long!!'; +const BASE = 'http://localhost:3000/api/v1/auth'; +/** The address the account signed up with. */ +const EMAIL = 'holder.lockout-account@example.com'; +/** + * Other spellings of the same address, each of which the credential check + * resolves to the account above (a correct password under each signs in). + */ +const SPELLINGS = [ + 'Holder.Lockout-Account@Example.com', + 'HOLDER.LOCKOUT-ACCOUNT@EXAMPLE.COM', +] as const; +const PASSWORD = 'Corr3ct!Horse-Battery'; +const WRONG_PASSWORD = 'Wr0ng!Guess-Zebra-7731'; +const THRESHOLD = 3; + +function deployment() { + const engine = createMemoryEngine(); + const events: AuthSessionAuditEventInput[] = []; + const manager = new AuthManager({ + secret: SECRET, + baseUrl: 'http://localhost:3000', + dataEngine: engine, + lockoutThreshold: THRESHOLD, + lockoutDurationMinutes: 15, + getAuditSink: () => ({ + recordAuthEvent: async (event: AuthSessionAuditEventInput) => { + events.push(event); + }, + }), + } as any); + return { engine, manager, events }; +} + +const post = (manager: AuthManager, path: string, body: unknown) => + manager.handleRequest( + new Request(`${BASE}${path}`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }), + ); + +async function signUp(manager: AuthManager): Promise { + inviteForAudienceGate(manager, EMAIL); + const res = await post(manager, '/sign-up/email', { email: EMAIL, password: PASSWORD, name: 'Holder' }); + expect(res.status, await res.clone().text()).toBe(200); +} + +const signIn = (manager: AuthManager, email: string, password: string) => + post(manager, '/sign-in/email', { email, password }); + +/** The status and machine code a sign-in answered. */ +async function outcome(res: Response): Promise<{ status: number; code: string | undefined }> { + const body = (await res.clone().json().catch(() => ({}))) as { code?: unknown }; + return { status: res.status, code: typeof body.code === 'string' ? body.code : undefined }; +} + +const REFUSED = { status: 401, code: 'INVALID_EMAIL_OR_PASSWORD' }; +const LOCKED = { status: 403, code: 'ACCOUNT_LOCKED' }; + +function accountRow(engine: any): any { + const row = ((engine.tables.get('sys_user') ?? []) as any[]).find((r) => r.email === EMAIL); + if (!row) throw new Error(`no sys_user row for ${EMAIL}`); + return row; +} + +const isLocked = (row: any) => !!row.locked_until && new Date(row.locked_until).getTime() > Date.now(); + +/** The account events, in order — `login` / `logout` filtered out. */ +const accountEvents = (events: AuthSessionAuditEventInput[]) => + events.filter((e) => e.action === 'login_failed' || e.action === 'account_locked'); + +describe('ADR-0069 D2: the lockout counts and holds the account the credential check resolves', () => { + it('each spelling below is one the credential check resolves to the account', async () => { + const { engine, manager } = deployment(); + await signUp(manager); + for (const spelling of SPELLINGS) { + const res = await signIn(manager, spelling, PASSWORD); + expect(res.status, spelling).toBe(200); + const body = (await res.json()) as { user?: { id?: string } }; + expect(body.user?.id, spelling).toBe(accountRow(engine).id); + } + }); + + it('refusals under different spellings of one address accumulate toward its one lock', async () => { + const { engine, manager, events } = deployment(); + await signUp(manager); + const userId = String(accountRow(engine).id); + events.length = 0; + + const attempts = [SPELLINGS[0], EMAIL, SPELLINGS[1]]; + for (const [i, spelling] of attempts.entries()) { + expect(await outcome(await signIn(manager, spelling, WRONG_PASSWORD)), spelling).toEqual(REFUSED); + expect(Number(accountRow(engine).failed_login_count), spelling).toBe(i + 1); + } + + expect(isLocked(accountRow(engine))).toBe(true); + const recorded = accountEvents(events); + expect(recorded.map((e) => e.action)).toEqual([ + 'login_failed', + 'login_failed', + 'login_failed', + 'account_locked', + ]); + expect(recorded.every((e) => e.userId === userId)).toBe(true); + expect(recorded[3]!.context?.threshold).toBe(THRESHOLD); + expect(new Date(String(recorded[3]!.context?.locked_until)).getTime()) + .toBe(new Date(accountRow(engine).locked_until).getTime()); + }); + + it('a locked account refuses the correct password under every spelling', async () => { + const { engine, manager, events } = deployment(); + await signUp(manager); + const userId = String(accountRow(engine).id); + for (let i = 0; i < THRESHOLD; i++) { + expect(await outcome(await signIn(manager, EMAIL, WRONG_PASSWORD))).toEqual(REFUSED); + } + expect(isLocked(accountRow(engine))).toBe(true); + const lockedUntil = new Date(accountRow(engine).locked_until).getTime(); + events.length = 0; + + for (const spelling of [EMAIL, ...SPELLINGS]) { + expect(await outcome(await signIn(manager, spelling, PASSWORD)), spelling).toEqual(LOCKED); + } + + expect(events.some((e) => e.action === 'login'), 'no session was minted').toBe(false); + expect(accountEvents(events)).toEqual( + [EMAIL, ...SPELLINGS].map(() => expect.objectContaining({ + action: 'login_failed', + userId, + context: { endpoint: '/sign-in/email', reason: 'ACCOUNT_LOCKED' }, + })), + ); + // The lock is the one the account already held: the refusals neither lift nor move it. + expect(new Date(accountRow(engine).locked_until).getTime()).toBe(lockedUntil); + }); + + it('a lock reached under one spelling refuses the correct password under another', async () => { + const { engine, manager } = deployment(); + await signUp(manager); + for (let i = 0; i < THRESHOLD; i++) { + expect(await outcome(await signIn(manager, SPELLINGS[1], WRONG_PASSWORD))).toEqual(REFUSED); + } + expect(isLocked(accountRow(engine))).toBe(true); + + expect(await outcome(await signIn(manager, EMAIL, PASSWORD))).toEqual(LOCKED); + expect(await outcome(await signIn(manager, SPELLINGS[0], PASSWORD))).toEqual(LOCKED); + }); + + it('a successful sign-in under any spelling clears the counter', async () => { + const { engine, manager } = deployment(); + await signUp(manager); + + for (const spelling of SPELLINGS) { + for (let i = 0; i < THRESHOLD - 1; i++) { + expect(await outcome(await signIn(manager, EMAIL, WRONG_PASSWORD))).toEqual(REFUSED); + } + expect(Number(accountRow(engine).failed_login_count)).toBe(THRESHOLD - 1); + + expect((await signIn(manager, spelling, PASSWORD)).status, spelling).toBe(200); + expect(Number(accountRow(engine).failed_login_count), spelling).toBe(0); + expect(isLocked(accountRow(engine)), spelling).toBe(false); + } + }); + + it('an identifier that resolves to no account changes no account and writes no account event', async () => { + const { engine, manager, events } = deployment(); + await signUp(manager); + const before = structuredClone(engine.tables.get('sys_user')); + events.length = 0; + + const unknown = ['nobody.lockout-account@example.com', 'Nobody.Lockout-Account@Example.com']; + for (let i = 0; i < THRESHOLD + 1; i++) { + for (const identifier of unknown) { + expect(await outcome(await signIn(manager, identifier, WRONG_PASSWORD)), identifier).toEqual(REFUSED); + } + } + + expect(engine.tables.get('sys_user')).toEqual(before); + expect(accountEvents(events)).toEqual([]); + for (const identifier of unknown) expect(JSON.stringify(events)).not.toContain(identifier); + // …and the real account still signs in. + expect((await signIn(manager, EMAIL, PASSWORD)).status).toBe(200); + }); +}); diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 886ff6a119..83337bf031 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -9284,6 +9284,10 @@ export class AuthManager { * (`lockoutThreshold <= 0`) or no data engine is wired. Fails OPEN on a * lookup error: an infra hiccup must never block every login. * + * The lock it enforces is the one held by the account the credential check + * resolves the identifier to ({@link findSignInAccount}), so it holds for + * every spelling of the address that signs in to that account. + * * The refusal is a refused sign-in of the account it names, so — given the * endpoint context `ctx` — it writes that account's `login_failed` row with * the refusal's own code before throwing (`auth-session-audit.ts`). @@ -9296,13 +9300,10 @@ export class AuthManager { let locked = false; let lockedUserId: string | undefined; try { - const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] }; - const u = await engine.findOne('sys_user', { - where: { email }, fields: ['id', 'locked_until'], context: SYSTEM_CTX, - } as any); - const lu = u?.locked_until; + const u = await this.findSignInAccount(engine, email, ['locked_until']); + const lu = u?.locked_until as string | number | Date | null | undefined; locked = !!(lu && new Date(lu).getTime() > Date.now()); - if (locked && typeof u?.id === 'string' && u.id) lockedUserId = u.id; + if (locked && u) lockedUserId = u.id; } catch { return; // fail-open } @@ -9335,6 +9336,12 @@ export class AuthManager { * history row on every login). No-op when lockout is disabled. Never throws — * a counter write must not turn a valid login into an error. * + * The counter it moves is the one held by the account the credential check + * resolves the identifier to ({@link findSignInAccount}) — the account the + * gate above enforces — so every refused attempt on that account counts + * toward one lock, and every success on it clears that lock, whatever + * spelling of the address the attempt carried. + * * Resolves to the lock it stamped — the account, the expiry and the * threshold — when THIS call wrote `locked_until` and the write landed, and * to `undefined` otherwise. That is the lockout transition the @@ -9351,12 +9358,8 @@ export class AuthManager { if (!engine) return undefined; try { const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] }; - const u = await engine.findOne('sys_user', { - where: { email }, - fields: ['id', 'failed_login_count', 'locked_until'], - context: SYSTEM_CTX, - } as any); - if (!u?.id) return undefined; + const u = await this.findSignInAccount(engine, email, ['failed_login_count', 'locked_until']); + if (!u) return undefined; if (success) { if ((Number(u.failed_login_count) || 0) !== 0 || u.locked_until) { await engine.update( @@ -9376,9 +9379,7 @@ export class AuthManager { patch.locked_until = lockedUntil; } await engine.update('sys_user', patch, { context: SYSTEM_CTX } as any); - return lockedUntil && typeof u.id === 'string' - ? { userId: u.id, lockedUntil, threshold } - : undefined; + return lockedUntil ? { userId: u.id, lockedUntil, threshold } : undefined; } catch (e) { // Never break the auth response. [#12981] But lockout accounting that // silently stops accounting is brute-force protection that is OFF. @@ -9443,22 +9444,15 @@ export class AuthManager { /** * The id of the account a password sign-in for `email` is checked against, - * or `undefined`. Resolved the way better-auth's `/sign-in/email` resolves - * it — the address lower-cased (`findUserByEmail(email.toLowerCase())`), - * matching the lower-cased identifier the adapter stores — so the - * `login_failed` row names the account the credential check actually judged. + * or `undefined` — {@link findSignInAccount}'s answer, so the `login_failed` + * row names the account the credential check actually judged and the lock + * gate and counter hold. */ private async resolveSignInAccountId(email: string): Promise { const engine = this.getDataEngine(); if (!engine) return undefined; try { - const query: EngineQueryOptions = { - where: { email: email.toLowerCase() }, - fields: ['id'], - context: { isSystem: true, positions: [], permissions: [] }, - }; - const u = await engine.findOne('sys_user', query); - return typeof u?.id === 'string' && u.id ? u.id : undefined; + return (await this.findSignInAccount(engine, email, []))?.id; } catch (e) { // The refusal stands either way; what is lost is its ledger row. this.logDurabilityDegradation( @@ -9473,6 +9467,45 @@ export class AuthManager { } } + /** + * ADR-0069 D2 — the ONE answer to "which account does this password sign-in + * name", read by the lock gate ({@link assertAccountNotLocked}), the counter + * ({@link recordSignInOutcome}) and the account events + * ({@link resolveSignInAccountId}): the `sys_user` row with `fields` + * (plus `id`), or `undefined` when the identifier names no account. + * + * It asks exactly what better-auth's `/sign-in/email` credential check asks + * (1.7.3: `internalAdapter.findUserByEmail(email.toLowerCase())`, an + * equality match of the `email` column against the lower-cased address, + * which is also the spelling `internalAdapter.createUser` stores), so the + * gate, the counter, the ledger and the credential check name one account + * for every identifier the credential check resolves to it. + * + * The lockout state lives on that row (`failed_login_count` / + * `locked_until`) and every write to it is by the row's `id`, so one account + * holds one counter. ⛔ Never resolve the account any other way on this + * path: the lockout holds exactly as far as it names the account the + * credential check names. + * + * Throws what the engine throws; each caller keeps its own failure posture + * (the gate fails open, the counter and the ledger report the lost write). + */ + private async findSignInAccount( + engine: IDataEngine, + email: string, + fields: readonly string[], + ): Promise<({ id: string } & Record) | undefined> { + const query: EngineQueryOptions = { + where: { email: email.toLowerCase() }, + fields: ['id', ...fields], + context: { isSystem: true, positions: [], permissions: [] }, + }; + const row = await engine.findOne('sys_user', query); + return row && typeof row.id === 'string' && row.id + ? (row as { id: string } & Record) + : undefined; + } + /** * ADR-0069 D2 (#3690) — project the operator's lockout settings onto * better-auth's own `twoFactor({ accountLockout })` contract, so the second diff --git a/scripts/query-options-erasure-baseline.json b/scripts/query-options-erasure-baseline.json index c2b55fc8e1..b0cd51c419 100644 --- a/scripts/query-options-erasure-baseline.json +++ b/scripts/query-options-erasure-baseline.json @@ -40,7 +40,7 @@ "packages/plugins/plugin-approvals/src/approver-org-scope.ts": 2, "packages/plugins/plugin-approvals/src/lifecycle-hooks.ts": 4, "packages/plugins/plugin-auth/src/admin-import-users.ts": 1, - "packages/plugins/plugin-auth/src/auth-manager.ts": 14, + "packages/plugins/plugin-auth/src/auth-manager.ts": 12, "packages/plugins/plugin-sharing/src/share-link-routes.ts": 3, "packages/plugins/plugin-sharing/src/share-link-service.ts": 5, "packages/runtime/src/action-execution.ts": 1,