fix(auth): make the second factor obey the operator's lockout policy (#3690)

.changeset/two-factor-lockout-follows-settings.md

@@ -0,0 +1,42 @@
+---
+"@objectstack/plugin-auth": patch
+"@objectstack/service-settings": patch
+---
+
+fix(auth): the second factor now obeys the operator's lockout policy instead of better-auth's defaults (#3690)
+
+`auth-manager.ts` constructed `twoFactor()` with a schema and nothing else, so
+better-auth's built-in `accountLockout` defaults — on, 10 attempts, 15 minutes —
+governed two-factor verification no matter what the admin configured. An operator
+who tightened **Setup → Authentication → Account lockout threshold** to 3 got a
+password stage that locked at 3 and a second factor that still locked at 10: the
+stricter door was the looser one, with nothing in the UI saying so.
+
+`lockout_threshold` / `lockout_duration_minutes` are now projected onto
+better-auth's own `accountLockout` shape (`enabled` / `maxFailedAttempts` /
+`durationSeconds`, minutes converted to seconds) rather than growing a parallel
+`two_factor_lockout_*` pair — one policy, one mental model, and a future upstream
+field arrives as a new option instead of a conflict. The projection goes through
+`applyConfigPatch`, which resets the cached better-auth instance, so a settings
+change takes effect without a restart.
+
+Threshold `0` is deliberately **not** forwarded as `enabled: false`. It is the
+password stage's "off", and a deployment may leave that stage unlocked because
+rate limiting or an IdP covers it; the second factor is the last check before a
+session is issued, so it keeps better-auth's default rather than being switched
+off by a setting that never mentioned it.
+
+The threshold field is also no longer hidden behind `email_password_enabled` —
+two-factor verification exists in passwordless deployments, where the setting was
+previously unreachable.
+
+The admin **Unlock Account** action now clears both stages. It only ever reset
+`sys_user`, so a user locked at the second factor had no admin escape hatch and
+had to wait the duration out — survivable while that lock needed 10 failures,
+routine once an operator can set the threshold to 3. The second-factor clear is
+best-effort and runs after the primary write, so an account with no enrolment
+still unlocks normally.
+
+Note the plugin caps attempts at 5 per challenge (`beginAttempt(5)`), which no
+option reaches; a threshold above 5 forces a fresh challenge rather than raising
+that cap.

content/docs/permissions/administrator-guide.mdx

@@ -83,7 +83,8 @@ only on reorgs. Why the tree matters:
 
 Day-to-day lifecycle actions live on each user's row menu and record header:
 **Ban / Unban** (blocks sign-in), **Unlock Account** (clears a brute-force
-lockout early), **Set Password** (also mints one-time temporary passwords),
+lockout early, at both the password and two-factor stages), **Set Password**
+(also mints one-time temporary passwords),
 and **Impersonate User** (see [Step 4](#step-4--verify)). *(Rough edge today:
 users' own self-service password recovery depends on a configured email or
 SMS delivery service — wiring tracked in cloud#580. Until then, admin **Set

content/docs/permissions/authentication.mdx

@@ -455,10 +455,23 @@ code that reads it). Changes take effect immediately; no restart is required.
 
 | Setting | Effect |
 |---|---|
-| `lockout_threshold` | Lock an account after this many consecutive failed sign-ins. |
-| `lockout_duration_minutes` | How long a lockout lasts. Admins can clear it early with the **Unlock** action on the user record. |
+| `lockout_threshold` | Lock an account after this many consecutive failed sign-in attempts. Counts **both** stages: wrong passwords and wrong two-factor codes. |
+| `lockout_duration_minutes` | How long a lockout lasts, at either stage. Admins can clear it early with the **Unlock** action on the user record, which releases both stages. |
 | `rate_limit_max` / `rate_limit_window_seconds` | Per-IP request throttle on auth endpoints. |
 
+The two stages keep **separate counters** — `sys_user.failed_login_count` for the
+password check, `sys_two_factor.failed_verification_count` for the second factor —
+so a locked password stage and a locked second factor are independent states. Each
+counter resets on a success at its own stage, and the admin **Unlock Account**
+action clears both at once.
+
+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
+additionally caps attempts at 5 per challenge, which is not configurable: a
+threshold above 5 simply forces the attacker to restart the sign-in flow more
+often before the account-level lock engages.
+
 ### Multi-factor (enforced MFA)
 
 | Setting | Effect |

packages/plugins/plugin-auth/src/auth-manager.test.ts

@@ -885,6 +885,62 @@ describe('AuthManager', () => {
       expect(tfPlugin._opts.schema.user.fields.twoFactorEnabled).toBe('two_factor_enabled');
     });
 
+    // #3690 — the operator's lockout policy has to reach the SECOND factor too.
+    // Before this, `twoFactor()` got only a schema, so the second factor sat on
+    // better-auth's built-in 10/900s no matter how the admin tuned sign-in:
+    // tightening the threshold to 3 left the last door before a session at 10.
+    describe('two-factor account lockout follows the operator settings (#3690)', () => {
+      const buildTwoFactor = async (extra: Record<string, unknown>) => {
+        let capturedConfig: any;
+        (betterAuth as any).mockImplementation((config: any) => {
+          capturedConfig = config;
+          return { handler: vi.fn(), api: {} };
+        });
+        const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
+        const manager = new AuthManager({
+          secret: 'test-secret-at-least-32-chars-long',
+          baseUrl: 'http://localhost:3000',
+          plugins: { twoFactor: true },
+          ...extra,
+        });
+        await manager.getAuthInstance();
+        warnSpy.mockRestore();
+        return capturedConfig.plugins.find((p: any) => p.id === 'two-factor')._opts.accountLockout;
+      };
+
+      it('projects lockoutThreshold / lockoutDurationMinutes onto better-auth\'s contract', async () => {
+        // Minutes → seconds is the whole reason this needs a test: the two
+        // sides use different units for the same policy.
+        expect(await buildTwoFactor({ lockoutThreshold: 3, lockoutDurationMinutes: 30 })).toEqual({
+          enabled: true,
+          maxFailedAttempts: 3,
+          durationSeconds: 1800,
+        });
+      });
+
+      it('falls back to the password stage\'s 15-minute default when only a threshold is set', async () => {
+        expect(await buildTwoFactor({ lockoutThreshold: 5 })).toEqual({
+          enabled: true,
+          maxFailedAttempts: 5,
+          durationSeconds: 900,
+        });
+      });
+
+      // Threshold 0 is the password stage's "off". It is NOT forwarded as
+      // `enabled: false`: a deployment may leave the password stage unlocked
+      // because rate limiting or an IdP covers it, while the second factor —
+      // the last check before a session — keeps better-auth's default. Leaving
+      // the option unset is what hands that default back.
+      for (const [label, extra] of [
+        ['threshold 0 (lockout explicitly off)', { lockoutThreshold: 0 }],
+        ['no lockout settings at all', {}],
+      ] as Array<[string, Record<string, unknown>]>) {
+        it(`leaves better-auth's own default in place with ${label}`, async () => {
+          expect(await buildTwoFactor(extra)).toBeUndefined();
+        });
+      }
+    });
+
     it('should register magicLink plugin when enabled', async () => {
       let capturedConfig: any;
       (betterAuth as any).mockImplementation((config: any) => {
@@ -2416,6 +2472,46 @@ describe('AuthManager', () => {
       );
     });
 
+    // #3690 — sign-in can lock at either stage, and the counters are
+    // independent. Unlocking only `sys_user` left a 2FA-locked user with no
+    // admin escape hatch, which the now-configurable threshold makes routine.
+    it('unlockUser also clears the second factor\'s lock', async () => {
+      const engine = {
+        ...makeEngine({ id: 'u1' }),
+        find: vi.fn(async () => [{ id: 'tf1' }, { id: 'tf2' }]),
+      };
+      const m = mgr(engine);
+      await expect(m.unlockUser('u1')).resolves.toBe(true);
+      expect(engine.find).toHaveBeenCalledWith(
+        'sys_two_factor',
+        expect.objectContaining({ where: { user_id: 'u1' } }),
+      );
+      for (const id of ['tf1', 'tf2']) {
+        expect(engine.update).toHaveBeenCalledWith(
+          'sys_two_factor',
+          { id, failed_verification_count: 0, locked_until: null },
+          expect.anything(),
+        );
+      }
+    });
+
+    it('unlockUser still succeeds when the second-factor clear fails', async () => {
+      // No enrolment, a store that cannot serve the lookup, 2FA never switched
+      // on — none of that may turn the password-stage unlock the admin asked
+      // for into a failure.
+      const engine = {
+        ...makeEngine({ id: 'u1' }),
+        find: vi.fn(async () => { throw new Error('no such object: sys_two_factor'); }),
+      };
+      const m = mgr(engine);
+      await expect(m.unlockUser('u1')).resolves.toBe(true);
+      expect(engine.update).toHaveBeenCalledWith(
+        'sys_user',
+        { id: 'u1', failed_login_count: 0, locked_until: null },
+        expect.anything(),
+      );
+    });
+
     it('unlockUser returns false for an unknown user', async () => {
       const engine = makeEngine(null);
       const m = mgr(engine);

packages/plugins/plugin-auth/src/auth-manager.ts

@@ -1855,6 +1855,9 @@ export class AuthManager {
       const { twoFactor } = await import('better-auth/plugins/two-factor');
       plugins.push(twoFactor({
         schema: buildTwoFactorPluginSchema(),
+        // ADR-0069 D2 (#3690) — the operator's lockout policy governs BOTH
+        // stages of sign-in, not just the password one.
+        accountLockout: this.resolveTwoFactorAccountLockout(),
       }));
     }
 
@@ -3893,6 +3896,46 @@ export class AuthManager {
     }
   }
 
+  /**
+   * ADR-0069 D2 (#3690) — project the operator's lockout settings onto
+   * better-auth's own `twoFactor({ accountLockout })` contract, so the second
+   * factor obeys the same policy as the password stage above.
+   *
+   * Deliberately reuses `lockoutThreshold` / `lockoutDurationMinutes` rather
+   * than introducing a parallel `two_factor_lockout_*` pair: an admin who
+   * tightens sign-in to 3 attempts reasonably reads that as "the login flow is
+   * tightened", and before this the second factor silently stayed at
+   * better-auth's 10 — the stricter door was the looser one, with nothing in
+   * the UI saying so. Following better-auth's own field shape (rather than
+   * inventing ObjectStack settings on top of it) also means a future upstream
+   * addition here shows up as a new option, not a conflict.
+   *
+   * `undefined` when the threshold is 0/absent — better-auth's defaults (on,
+   * 10 attempts, 15 minutes) then stand. That is NOT mapped to
+   * `enabled: false`: `0` is the password stage's "off", and a deployment may
+   * leave THAT stage unlocked because other controls cover it (per-IP rate
+   * limiting, a captcha, an IdP in front). The second factor is the last door
+   * before a session is issued, so it is not turned off by a setting that
+   * never mentioned it. Both stages remain explicitly configurable; only the
+   * "unset" case differs, and it differs toward locking.
+   *
+   * Note the plugin ALSO caps attempts per challenge at a hardcoded 5
+   * (`beginAttempt(5)` in its totp / backup-code verifiers), which no option
+   * reaches. A threshold above 5 therefore still forces a fresh challenge
+   * every 5 guesses; the account budget is what accumulates across them.
+   */
+  private resolveTwoFactorAccountLockout():
+  { enabled: boolean; maxFailedAttempts: number; durationSeconds: number } | undefined {
+    const threshold = Number(this.config.lockoutThreshold) || 0;
+    if (threshold <= 0) return undefined;
+    const minutes = Number(this.config.lockoutDurationMinutes) || 15;
+    return {
+      enabled: true,
+      maxFailedAttempts: threshold,
+      durationSeconds: minutes * 60,
+    };
+  }
+
   /**
    * ADR-0069 D7 — stamp `last_login_at` (+ `last_login_ip` when known) on a
    * successful sign-in. Best-effort and always fire-and-forget safe: a login
@@ -3918,6 +3961,14 @@ export class AuthManager {
    * ADR-0069 D2 — clear a user's lockout state (admin "Unlock" action).
    * Resets `failed_login_count` and `locked_until`. Returns false when no data
    * engine is wired or the user does not exist.
+   *
+   * [#3690] Clears BOTH stages. Sign-in can lock at the password check
+   * (`sys_user`) or at the second factor (`sys_two_factor`), and the two keep
+   * independent counters — so unlocking only the first left a 2FA-locked user
+   * with no admin escape hatch at all, waiting out the duration. That was
+   * survivable while the second factor sat on better-auth's 10-attempt default;
+   * with the threshold now operator-configurable (3 is a reasonable choice),
+   * it is a lock admins will hit routinely.
    */
   public async unlockUser(userId: string): Promise<boolean> {
     const engine = this.getDataEngine();
@@ -3932,6 +3983,25 @@ export class AuthManager {
       { id: userId, failed_login_count: 0, locked_until: null },
       { context: SYSTEM_CTX } as any,
     );
+    // Best-effort and deliberately after the primary write: a user with no
+    // enrolment (or an environment where 2FA was never switched on) must still
+    // get a successful unlock, and the password-stage clear is the part the
+    // admin asked for.
+    try {
+      const enrolments = await engine.find('sys_two_factor', {
+        where: { user_id: String(userId) }, fields: ['id'], context: SYSTEM_CTX,
+      } as any);
+      for (const row of (enrolments ?? []) as Array<{ id?: string }>) {
+        if (!row?.id) continue;
+        await engine.update(
+          'sys_two_factor',
+          { id: row.id, failed_verification_count: 0, locked_until: null },
+          { context: SYSTEM_CTX } as any,
+        );
+      }
+    } catch {
+      // Never turn a successful password-stage unlock into a failure.
+    }
     return true;
   }