fix(plugin-auth)!: an organization cannot require MFA while two-factor enrollment is off (#30201)

.changeset/30201-org-mfa-needs-enrollment.md

@@ -0,0 +1,21 @@
+---
+'@objectstack/plugin-auth': patch
+---
+
+fix(plugin-auth)!: an organization cannot require multi-factor authentication while two-factor enrollment is off, and the MFA gate never holds a user who has no way to enroll (#30201)
+
+Clause-②: no (narrowing) — while the two-factor feature is off, a write that turns `sys_organization.require_mfa` on (an insert with it on, or an update of a row that stores it off) is refused `400 VALIDATION_FAILED`, a value an administrator could save before; re-saving a row that already stores it on passes. No published export, error code or settings key is added; nothing is widened.
+Metadata-protocol: none
+Skills-docs: follow-up owed — content/docs/permissions/authentication.mdx (the `mfa_required` row says `require_mfa` is settable per organization with no condition; it needs two-factor on)
+
+<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, export or stored shape is removed, renamed or re-shaped, and no stored row is read, rewritten, converted or dropped, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is a runtime accept set on one platform-owned data column, `sys_organization.require_mfa`, judged against the deployment's two-factor state at write time; the remedy is a deployment or settings change (turn two-factor on), which no ledger entry can derive. The other categories are closed on facts: the bumped package publishes (not unpublished); no ADR-0087 id covers this column and this diff adds none (not registered, not already-registered); no published export is added, removed or retyped, so no interface or type narrows (not runtime-export-retired, runtime-interface-only or type-surface-only). -->
+
+**BREAKING** accept-set narrowing on `sys_organization.require_mfa` writes, served by every data door (the organization's edit form, REST, MCP), on a deployment where two-factor is off.
+
+- **Before.** An administrator could tick Require Multi-Factor Auth on an organization while two-factor was off (the default, `features.twoFactor: false`), and it saved. Once the grace period ended, every member, the owner included, was held `403 MFA_REQUIRED` and sent to enroll, and the enrollment endpoint (`POST /api/v1/auth/two-factor/enable`) answered 404: the twoFactor plugin that serves it was never turned on. Nobody could undo it from inside the product, because Setup sits behind the same gate. The global switch never had this hole: the `auth.mfa_required` setting turns the requirement and the plugin on as one piece.
+- **Now.** While two-factor is off, a write that turns `require_mfa` on is refused: an insert with it on, or an update of a row whose stored value is off. The refusal carries one `fields[]` entry on `require_mfa` (`invalid_value`) saying why, who would have been locked out, and the ways forward. It applies to every write context, the system's included, on the by-id door, an insert, and a multi-row update, which is judged row by row (one row turning it on refuses the whole write, with nothing written). Re-sending `require_mfa: true` for a row that already stores it (a whole-row update, an import re-run, a save with no change) passes, so other edits of that row are not blocked. Clearing the column and writing other columns pass as before, and every write passes once two-factor is on (read per write, so no restart is needed).
+- **The gate.** ADR-0069 D3's MFA gate holds a user only while they can enroll. With two-factor off, an MFA requirement (an organization's `require_mfa`, or `auth.mfa_required` under an `OS_AUTH_TWO_FACTOR=false` pin, or a host that sets `mfaRequired` without `plugins.twoFactor`) holds nobody and starts no grace clock, and the server says so once in a warning with the remedy. An organization row stored with `require_mfa = true` before this release therefore locks no one out; it starts holding unenrolled members once two-factor is turned on.
+- **One answer.** Whether two-factor is on is decided in one place, the same `OS_AUTH_TWO_FACTOR` env, then `plugins.twoFactor`, then off, for the plugin list, `features.twoFactor` in `/api/v1/auth/config`, the gate and the write refusal.
+- **Unchanged.** An enrolled user is never held. With two-factor on, the requirement and its grace period behave exactly as before. The `auth.mfa_required` setting still turns two-factor on with the requirement.
+
+**The remedy.** To require multi-factor authentication for one organization, turn two-factor enrollment on for the deployment first, with `OS_AUTH_TWO_FACTOR=true`, and then tick the organization's box. Unless `OS_AUTH_TWO_FACTOR` is set to `false` (the env var outranks the settings), turning on the `auth.mfa_required` setting instead also works: it requires multi-factor authentication for every organization and turns enrollment on with it.

packages/plugins/plugin-auth/src/auth-manager-password-session-policy.ts

@@ -62,7 +62,9 @@ export interface PasswordSessionPolicyHost {
   _orgMfaCache: AuthManager['_orgMfaCache'];
   _orgMfaRefreshing: AuthManager['_orgMfaRefreshing'];
   getDataEngine: AuthManager['getDataEngine'];
+  isTwoFactorEnabled: AuthManager['isTwoFactorEnabled'];
   logDurabilityDegradation: AuthManager['logDurabilityDegradation'];
+  noteMfaRequirementWithoutEnrollment: AuthManager['noteMfaRequirementWithoutEnrollment'];
   parseHashes: AuthManager['parseHashes'];
   refreshMustChangeCacheIfStale: AuthManager['refreshMustChangeCacheIfStale'];
   refreshOrgMfaCacheIfStale: AuthManager['refreshOrgMfaCacheIfStale'];
@@ -136,7 +138,7 @@ export function refreshOrgMfaCacheIfStale(this: Pick<PasswordSessionPolicyHost,
 
 /** The body of `AuthManager#resolveUserAuthGates`; the method's docblock there is its contract. */
 export async function resolveUserAuthGates(
-  this: Pick<PasswordSessionPolicyHost, '_mustChangeCache' | '_orgMfaCache' | 'config' | 'getDataEngine' | 'refreshMustChangeCacheIfStale' | 'refreshOrgMfaCacheIfStale'>,
+  this: Pick<PasswordSessionPolicyHost, '_mustChangeCache' | '_orgMfaCache' | 'config' | 'getDataEngine' | 'isTwoFactorEnabled' | 'noteMfaRequirementWithoutEnrollment' | 'refreshMustChangeCacheIfStale' | 'refreshOrgMfaCacheIfStale'>,
   userId: string,
   options: { organizationId?: string } = {},
 ): Promise<AuthGate[]> {
@@ -205,7 +207,21 @@ export async function resolveUserAuthGates(
     // A user without TOTP enrolled is blocked once their grace window
     // elapses. The clock (`mfa_required_at`) starts the first time we see
     // them required-but-unenrolled (stamped lazily, best-effort).
-    if (mfaRequired && !(u?.two_factor_enabled === true || u?.two_factor_enabled === 1)) {
+    //
+    // [#30201] Only while they CAN enroll. The twoFactor plugin's
+    // `/two-factor/*` endpoints are the one enrollment path; with the plugin
+    // off, a hold here answers `MFA_REQUIRED` to a user whose remediation
+    // endpoint does not exist, which locks every member of the organization
+    // out, the owner included, behind the very gate that guards Setup. So an
+    // unenrollable requirement is not enforced (no hold, and no grace clock
+    // started: the window opens when enrollment does) and is reported once.
+    // The `require_mfa` write guard refuses a new requirement in that state
+    // (`organization-mfa-enrollment-guard.ts`); this covers rows stored before
+    // it, and an `OS_AUTH_TWO_FACTOR=false` pin under the global setting.
+    const unenrolled = !(u?.two_factor_enabled === true || u?.two_factor_enabled === 1);
+    if (mfaRequired && unenrolled && !this.isTwoFactorEnabled()) {
+      this.noteMfaRequirementWithoutEnrollment();
+    } else if (mfaRequired && unenrolled) {
       const graceDays = Math.max(0, Math.floor(Number(this.config.mfaGracePeriodDays ?? 7)));
       let requiredAt = u?.mfa_required_at;
       if (!requiredAt) {

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

@@ -71,6 +71,7 @@ import {
   resolveDcrEnabled,
   resolveOidcProviderEnabled,
   resolveScimEnabled,
+  resolveTwoFactorEnabled,
   type AuthManager,
 } from './auth-manager.js';
 import {
@@ -188,11 +189,13 @@ export async function buildPluginList(this: PluginListHost): Promise<any[]> {
   // better-auth build would otherwise mount SCIM with its ADR-0134
   // deprovisioning path silently declined.
   assertScimAdminCoherence(pluginConfig);
-  const twoFactorFromEnv = readBooleanEnv('OS_AUTH_TWO_FACTOR');
   const hibpFromEnv = readBooleanEnv('OS_AUTH_PASSWORD_REJECT_BREACHED');
   const enabled = {
     organization: pluginConfig.organization ?? true,
-    twoFactor: twoFactorFromEnv ?? pluginConfig.twoFactor ?? false,
+    // [#30201] Shared decision point (env → config), see
+    // resolveTwoFactorEnabled — the MFA gate and the require_mfa write guard
+    // ask the same question, so a requirement is never enforced unenrollable.
+    twoFactor: resolveTwoFactorEnabled(pluginConfig),
     passwordRejectBreached: hibpFromEnv ?? pluginConfig.passwordRejectBreached ?? false,
     passkeys: pluginConfig.passkeys ?? false,
     magicLink: pluginConfig.magicLink ?? false,

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

@@ -3876,7 +3876,8 @@ describe('AuthManager', () => {
       const IMPERSONATION = { impersonatedBy: 'u-admin' };
       // Every posture computable at once: enforced MFA with no grace, and a
       // one-day expiry. Each row below decides which postures its user holds.
-      const POLICY = { mfaRequired: true, mfaGracePeriodDays: 0, passwordExpiryDays: 1 };
+      // [#30201] Two-factor on: the gate holds a user for MFA only while they can enroll.
+      const POLICY = { mfaRequired: true, mfaGracePeriodDays: 0, passwordExpiryDays: 1, plugins: { twoFactor: true } };
       const sessionGateFor = async (row: Record<string, unknown>, session: Record<string, unknown>) => {
         const engine = makeDataEngine({ platformAdmin: false });
         // The block's double, its `findOne` answering the posture read of the user row.
@@ -4695,9 +4696,13 @@ describe('AuthManager', () => {
       }),
       update: vi.fn(async () => ({})),
     });
+    // [#30201] Two-factor on by default here: the gate holds a user for MFA only
+    // while they can enroll. The cases that turn it off say so (`plugins: {}`).
     const mgr = (engine: any, extra: any = {}) => {
       const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
-      const m = new AuthManager({ secret: SECRET, baseUrl: 'http://localhost:3000', dataEngine: engine, ...extra });
+      const m = new AuthManager({
+        secret: SECRET, baseUrl: 'http://localhost:3000', dataEngine: engine, plugins: { twoFactor: true }, ...extra,
+      });
       warn.mockRestore();
       return m;
     };
@@ -4749,6 +4754,63 @@ describe('AuthManager', () => {
       const g = await (m as any).computeAuthGate('u1', undefined, false);
       expect(g?.code).toBe('MFA_REQUIRED');
     });
+
+    // [#30201] The twoFactor plugin's endpoints are the only enrollment path.
+    // A hold while it is off answers MFA_REQUIRED to a user whose remediation
+    // endpoint answers 404 — the whole-tenant lockout the card measured.
+    describe('only a user who can enroll is held (#30201)', () => {
+      const OLD = () => new Date(Date.now() - 30 * 86_400_000).toISOString();
+      const logger = () => ({ warn: vi.fn(), info: vi.fn() });
+
+      it('two-factor off: no hold and no grace clock started, said once with the remedy', async () => {
+        const log = logger();
+        const lapsed = makeEngine({ id: 'u1', two_factor_enabled: false, mfa_required_at: OLD() });
+        const m = mgr(lapsed, { mfaRequired: true, mfaGracePeriodDays: 0, plugins: {}, logger: log });
+        expect(m.isTwoFactorEnabled()).toBe(false);
+        await expect((m as any).computeAuthGate('u1', undefined, false)).resolves.toBeUndefined();
+        expect(await m.resolveUserAuthGates('u1')).toEqual([]);
+        const fresh = makeEngine({ id: 'u2', two_factor_enabled: false, mfa_required_at: null });
+        const m2 = mgr(fresh, { mfaRequired: true, plugins: {}, logger: log });
+        expect(await m2.resolveUserAuthGates('u2')).toEqual([]);
+        // The window opens when enrollment does, not before.
+        expect(fresh.update).not.toHaveBeenCalled();
+        // Once per manager, however many requests ask.
+        expect(log.warn).toHaveBeenCalledTimes(2);
+        expect(String(log.warn.mock.calls[0][0])).toMatch(/NOT enforced/);
+        expect(String(log.warn.mock.calls[0][0])).toMatch(/OS_AUTH_TWO_FACTOR=true/);
+        // The settings remedy is conditional: the env var outranks it.
+        expect(String(log.warn.mock.calls[0][0])).toMatch(/unless OS_AUTH_TWO_FACTOR is set to false/);
+      });
+
+      it('an OS_AUTH_TWO_FACTOR=false pin outranks plugins.twoFactor, so the requirement is not enforced', async () => {
+        const prior = process.env.OS_AUTH_TWO_FACTOR;
+        process.env.OS_AUTH_TWO_FACTOR = 'false';
+        try {
+          const m = mgr(makeEngine({ id: 'u1', two_factor_enabled: false, mfa_required_at: OLD() }), {
+            mfaRequired: true, mfaGracePeriodDays: 0, logger: logger(),
+          });
+          expect(m.isTwoFactorEnabled()).toBe(false);
+          // The one answer: what is wired, what is advertised, what the gate asks.
+          expect(m.getPublicConfig().features.twoFactor).toBe(false);
+          expect(await m.resolveUserAuthGates('u1')).toEqual([]);
+        } finally {
+          if (prior === undefined) delete process.env.OS_AUTH_TWO_FACTOR;
+          else process.env.OS_AUTH_TWO_FACTOR = prior;
+        }
+      });
+
+      it('control: the same user is held once two-factor is on, and an enrolled user never is', async () => {
+        const m = mgr(makeEngine({ id: 'u1', two_factor_enabled: false, mfa_required_at: OLD() }), {
+          mfaRequired: true, mfaGracePeriodDays: 0,
+        });
+        expect(m.isTwoFactorEnabled()).toBe(true);
+        expect((await m.resolveUserAuthGates('u1')).map((g) => g.code)).toEqual(['MFA_REQUIRED']);
+        const enrolled = mgr(makeEngine({ id: 'u1', two_factor_enabled: true, mfa_required_at: OLD() }), {
+          mfaRequired: true, mfaGracePeriodDays: 0, plugins: {},
+        });
+        expect(await enrolled.resolveUserAuthGates('u1')).toEqual([]);
+      });
+    });
   });
 
   // ADR-0069 D3 — per-org MFA tightening (an org may require MFA above the
@@ -4766,9 +4828,12 @@ describe('AuthManager', () => {
       update: vi.fn(async () => ({})),
       count: vi.fn(async () => 1),
     });
+    // [#30201] Two-factor on by default, as in the block above.
     const mgr = (engine: any, extra: any = {}) => {
       const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
-      const m = new AuthManager({ secret: SECRET, baseUrl: 'http://localhost:3000', dataEngine: engine, ...extra });
+      const m = new AuthManager({
+        secret: SECRET, baseUrl: 'http://localhost:3000', dataEngine: engine, plugins: { twoFactor: true }, ...extra,
+      });
       warn.mockRestore();
       return m;
     };
@@ -4803,6 +4868,17 @@ describe('AuthManager', () => {
       await expect((m as any).computeAuthGate('u1', 'org1', false)).resolves.toBeUndefined();
     });
 
+    it('[#30201] does NOT block for the active org\'s require_mfa while two-factor is off, nor start the clock', async () => {
+      const engine = makeEngine(
+        { id: 'u1', two_factor_enabled: false, mfa_required_at: null },
+        { id: 'org1', require_mfa: true },
+      );
+      const m = mgr(engine, { mfaRequired: false, mfaGracePeriodDays: 0, plugins: {}, logger: { warn: vi.fn() } });
+      (m as any)._orgMfaCache = { value: true, at: Date.now() };
+      await expect((m as any).computeAuthGate('u1', 'org1', false)).resolves.toBeUndefined();
+      expect(engine.update).not.toHaveBeenCalled();
+    });
+
     it('is a no-op when no org requires MFA and there is no active org', async () => {
       const engine = makeEngine({ id: 'u1', two_factor_enabled: false }, null);
       const m = mgr(engine, { mfaRequired: false });