fix(plugin-auth)!: the global mfa_required switch cannot be saved on while OS_AUTH_TWO_FACTOR=false keeps two-factor enrollment off (#30302)

.changeset/30302-mfa-enforcement-follow-ups.md

@@ -0,0 +1,19 @@
+---
+'@objectstack/plugin-auth': patch
+---
+
+fix(plugin-auth)!: the global `auth.mfa_required` switch cannot be saved on while `OS_AUTH_TWO_FACTOR=false` keeps two-factor enrollment off (#30302)
+
+Clause-②: no (narrowing) — while `OS_AUTH_TWO_FACTOR` is set to off, a save that turns the `auth.mfa_required` setting on is refused `400 SETTINGS_VALIDATION` (`invalid_value` at `mfa_required`), a value an administrator could save before; saving it off, or re-saving a value already in force, passes. No published export, error code, settings key or `/auth/config` member is added; nothing is widened.
+Metadata-protocol: none
+Skills-docs: updated here — `content/docs/permissions/authentication.mdx` (the `mfa_required` row) and `docs/HARDENING.md` (the MFA row); `skills/**` teaches neither `mfa_required` nor `require_mfa`, so no skill or eval moves.
+
+<!-- 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 settings key, `auth.mfa_required`, judged against the deployment's `OS_AUTH_TWO_FACTOR` env var at save time; the remedy is a deployment change (set the env var on, or remove it), 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 key 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 the `auth.mfa_required` settings save (Setup → Authentication, `PUT /api/settings/auth`, `SettingsService.setMany`), on a deployment that sets `OS_AUTH_TWO_FACTOR` to off.
+
+- **Before.** Turning on "Require multi-factor authentication" saved and read as on. The settings pass turns the requirement and the two-factor plugin on as one piece, but the `OS_AUTH_TWO_FACTOR` env var outranks the plugin flag, so under `OS_AUTH_TWO_FACTOR=false` nobody could enroll an authenticator app, and the MFA gate, which holds only a user who can enroll, enforced nothing. An organization's own `require_mfa` already refused this state; the global switch did not.
+- **Now.** That save is refused before anything is written: the whole save, other keys in it included, with one field error at `mfa_required` naming `OS_AUTH_TWO_FACTOR`, in the caller's language (English, Simplified Chinese, Japanese, Spanish), and the constraint `{ twoFactor: false, overriddenBy: 'OS_AUTH_TWO_FACTOR' }`.
+- **Unchanged.** With `OS_AUTH_TWO_FACTOR` unset or on, the switch saves and turns enrollment on with the requirement, as before. Turning it off is never refused. A switch saved on before this release is left as stored and still not enforced; `GET /api/v1/auth/config` answers `features.twoFactor: false` there, the same decision the gate makes, which is the reading a console uses to say the requirement is not enforced.
+
+**The remedy.** Set `OS_AUTH_TWO_FACTOR=true` on the deployment (or remove it) and restart, then turn on `auth.mfa_required`.

content/docs/permissions/authentication.mdx

@@ -604,7 +604,7 @@ often before the account-level lock engages.
 
 | Setting | Effect |
 |---|---|
-| `mfa_required` | Require MFA org-wide (also settable per organization via `require_mfa`). |
+| `mfa_required` | Require MFA org-wide; saving it on also turns two-factor enrollment on, and the save is refused while `OS_AUTH_TWO_FACTOR=false` pins enrollment off. An organization can require it on its own (`require_mfa`) only while two-factor enrollment is on; a requirement stored while enrollment is off is not enforced, which `GET /api/v1/auth/config` reports as `features.twoFactor: false`. |
 | `mfa_grace_period_days` | Grace window for existing users to enroll. After it elapses, the session is allowed to sign in but **gated from data access** until TOTP is enrolled — the Console surfaces a remediation overlay that walks the user through enrollment. |
 
 ### Sessions

docs/HARDENING.md

@@ -239,7 +239,7 @@ production deployment, enable at least:
 | ----------- | ------------------------------------------------------------------------------------ |
 | Password    | `password_reject_breached: true`, `password_require_complexity: true`, `password_history_count: 3–5`, `password_expiry_days: 90` |
 | Anti-abuse  | `lockout_threshold: 5`, `lockout_duration_minutes: 15`, `rate_limit_max` / `rate_limit_window_seconds` |
-| MFA         | `mfa_required: true` (or per-org `require_mfa`), `mfa_grace_period_days: 7`            |
+| MFA         | `mfa_required: true` (or per-org `require_mfa`, which needs two-factor enrollment on), `mfa_grace_period_days: 7`; neither saves while `OS_AUTH_TWO_FACTOR=false` |
 | Sessions    | `session_idle_timeout_minutes: 30–60`, `session_absolute_max_hours: 8–12`, `max_concurrent_sessions_per_user` |
 | Network     | `allowed_ip_ranges` (IPv4 CIDR) for tenant- or office-scoped access                   |
 

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

@@ -69,6 +69,7 @@ import {
 import { registerLastAdminGuard } from './last-admin-guard.js';
 import { registerApiKeyRestoreGuard } from './api-key-restore-guard.js';
 import { registerOrganizationMfaEnrollmentGuard } from './organization-mfa-enrollment-guard.js';
+import { registerMfaRequiredEnrollmentPredicate } from './mfa-required-enrollment-predicate.js';
 import { registerPersonalViewHooks } from './personal-view-hooks.js';
 import { registerUserPreferenceHooks } from './user-preference-hooks.js';
 import { personalViewCallerResolver, registerPersonalViewRoutes, PERSONAL_VIEW_REMOVE_ROUTE, type PersonalViewEngine } from './personal-view-routes.js';
@@ -2271,6 +2272,9 @@ export class AuthPlugin implements Plugin {
         // gated users would have no way to comply. [#20412] So the enforcement
         // and the plugin are ONE piece: a refused plugins block leaves MFA at
         // its standing value too, never enforced without its enrollment path.
+        // The env var OS_AUTH_TWO_FACTOR outranks this piece's plugin flag, so
+        // under a pin to off the save itself is refused, before this pass
+        // (mfa-required-enrollment-predicate.ts).
         if (isExplicit('mfa_required')) {
           const on = asBoolean(values.mfa_required, false);
           if (on) {
@@ -2591,6 +2595,11 @@ export class AuthPlugin implements Plugin {
       }
     }
 
+    // The save door refuses turning `mfa_required` on while OS_AUTH_TWO_FACTOR
+    // pins enrollment off; the pass below would store it and enforce nothing.
+    // See mfa-required-enrollment-predicate.ts.
+    registerMfaRequiredEnrollmentPredicate(settings, ctx.logger);
+
     await applySettings();
     if (typeof settings.subscribe === 'function') {
       settings.subscribe('auth', () => {

packages/plugins/plugin-auth/src/mfa-required-enrollment-predicate.test.ts

@@ -0,0 +1,241 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * The global `auth.mfa_required` switch cannot be saved on while
+ * `OS_AUTH_TWO_FACTOR` pins enrollment off (`mfa-required-enrollment-predicate.ts`).
+ *
+ * Through the REAL settings service (`@objectstack/service-settings`) and the
+ * real `AuthPlugin` / `AuthManager`: the plugin registers the predicate when it
+ * binds the `auth` namespace, and a save goes through `setMany`, or through the
+ * real `PUT /api/settings/:ns` route for the envelope. No data engine is
+ * composed; the settings service keeps its in-memory store.
+ *
+ * Pinned:
+ *   1. under `OS_AUTH_TWO_FACTOR=false`, turning the switch on is refused
+ *      before anything is written: the stored value, the batch it rode in, the
+ *      runtime requirement and `features.twoFactor` all stay as they were;
+ *   2. the HTTP door answers 400 `SETTINGS_VALIDATION` at the key, in the
+ *      caller's language;
+ *   3. controls: with the env var unset or on, the same save is accepted and
+ *      the pass turns the requirement and enrollment on; turning it off is
+ *      never refused;
+ *   4. a switch stored on before the check is not re-judged: saving it again
+ *      passes, and the gate's own reading stays off.
+ */
+
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import type { PluginContext } from '@objectstack/core';
+import type { IHttpRequest, IHttpResponse, IHttpServer, RouteHandler } from '@objectstack/spec/contracts';
+import {
+  SettingsService,
+  SettingsValidationError,
+  builtinSettingsManifests,
+  registerSettingsRoutes,
+} from '@objectstack/service-settings';
+import { AuthPlugin } from './auth-plugin.js';
+import type { AuthManager } from './auth-manager.js';
+import {
+  MFA_REQUIRED_ENROLLMENT_COPY,
+  mfaRequiredEnrollmentPredicate,
+  registerMfaRequiredEnrollmentPredicate,
+} from './mfa-required-enrollment-predicate.js';
+
+const SECRET = 'test-secret-at-least-32-chars-long';
+const ENV = 'OS_AUTH_TWO_FACTOR';
+
+/** The error `setMany` refused with, or a failure naming what happened instead. */
+async function refusalOf(write: Promise<unknown>): Promise<SettingsValidationError> {
+  const outcome = await write.then(
+    () => undefined,
+    (err: unknown) => err,
+  );
+  expect(outcome, 'the save went through').toBeInstanceOf(SettingsValidationError);
+  return outcome as SettingsValidationError;
+}
+
+describe('the global mfa_required switch needs two-factor enrollment', () => {
+  let saved: string | undefined;
+
+  beforeEach(() => {
+    saved = process.env[ENV];
+    delete process.env[ENV];
+  });
+
+  afterEach(() => {
+    if (saved === undefined) delete process.env[ENV];
+    else process.env[ENV] = saved;
+  });
+
+  const makeSettings = () => {
+    const settings = new SettingsService({ env: {} });
+    for (const manifest of builtinSettingsManifests) {
+      if (manifest.namespace === 'auth') settings.registerManifest(manifest);
+    }
+    return settings;
+  };
+
+  /** Boot as a host does: init → start → kernel:ready (the auth namespace binds there). */
+  const boot = async (settings: SettingsService = makeSettings()) => {
+    const hooks = new Map<string, Array<() => Promise<void>>>();
+    const warn = vi.fn();
+    const ctx = {
+      registerService: vi.fn(),
+      getService: vi.fn((name: string) => {
+        if (name === 'manifest') return { register: vi.fn() };
+        if (name === 'settings') return settings;
+        return undefined;
+      }),
+      getServices: vi.fn(() => new Map()),
+      hook: vi.fn((name: string, handler: () => Promise<void>) => {
+        if (!hooks.has(name)) hooks.set(name, []);
+        hooks.get(name)!.push(handler);
+      }),
+      trigger: vi.fn(),
+      logger: { info: vi.fn(), error: vi.fn(), warn, debug: vi.fn() },
+      getKernel: vi.fn(),
+    } as unknown as PluginContext;
+    const plugin = new AuthPlugin({ secret: SECRET, baseUrl: 'http://localhost:3000' });
+    await plugin.init(ctx);
+    const manager = (ctx.registerService as any).mock.calls.find(([name]: [string]) => name === 'auth')?.[1] as AuthManager;
+    await plugin.start(ctx);
+    for (const handler of hooks.get('kernel:ready') ?? []) await handler();
+    return { settings, manager, warn };
+  };
+
+  const required = (manager: AuthManager) => (manager as any).config.mfaRequired === true;
+
+  // ── 1. The refusal ──────────────────────────────────────────────────────────
+
+  it('OS_AUTH_TWO_FACTOR=false: turning it on is refused; nothing in the batch is written', async () => {
+    process.env[ENV] = 'false';
+    const { settings, manager } = await boot();
+
+    const refusal = await refusalOf(settings.setMany('auth', { mfa_required: true, mfa_grace_period_days: 3 }));
+    expect(refusal.code).toBe('SETTINGS_VALIDATION');
+    expect(refusal.fields).toEqual([
+      expect.objectContaining({
+        field: 'mfa_required',
+        code: 'invalid_value',
+        constraint: { twoFactor: false, overriddenBy: 'OS_AUTH_TWO_FACTOR' },
+      }),
+    ]);
+
+    expect(await settings.get('auth', 'mfa_required')).toMatchObject({ value: false, source: 'default' });
+    expect(await settings.get('auth', 'mfa_grace_period_days')).toMatchObject({ source: 'default' });
+    expect(required(manager)).toBe(false);
+    expect(manager.isTwoFactorEnabled()).toBe(false);
+    expect(manager.getPublicConfig().features.twoFactor).toBe(false);
+    // Off, or the value in force, is never refused.
+    await expect(settings.setMany('auth', { mfa_required: false })).resolves.toBeDefined();
+  });
+
+  it('the settings PUT door answers 400 SETTINGS_VALIDATION at the key, in the caller language', async () => {
+    process.env[ENV] = '0';
+    const { settings } = await boot();
+    const routes = new Map<string, RouteHandler>();
+    const add = (method: string) => (path: string, handler: RouteHandler) => {
+      routes.set(`${method} ${path}`, handler);
+      return http;
+    };
+    const http = {
+      get: add('GET'),
+      post: add('POST'),
+      put: add('PUT'),
+      delete: add('DELETE'),
+      patch: add('PATCH'),
+      use: () => http,
+      listen: async () => {},
+      close: async () => {},
+      getInstance: () => null,
+    } as unknown as IHttpServer;
+    registerSettingsRoutes(http, settings, {
+      contextFromRequest: () => ({ enforced: true, permissions: ['manage_platform_settings'], userId: 'usr_admin' }),
+    });
+
+    const state: { status: number; body?: any } = { status: 200 };
+    const res = {
+      json: vi.fn((data: unknown) => {
+        state.body = data;
+      }),
+      send: vi.fn(),
+      status: vi.fn((code: number) => {
+        state.status = code;
+        return res;
+      }),
+      header: vi.fn(() => res),
+    } as unknown as IHttpResponse;
+    const req = {
+      params: { namespace: 'auth' },
+      query: {},
+      body: { mfa_required: true },
+      headers: { 'accept-language': 'zh-CN,zh;q=0.9' },
+      method: 'PUT',
+      path: '/api/settings/auth',
+    } as unknown as IHttpRequest;
+    await routes.get('PUT /api/settings/:namespace')!(req, res);
+
+    expect(state.status).toBe(400);
+    expect(state.body.error.code).toBe('SETTINGS_VALIDATION');
+    const [field] = state.body.error.details.fields;
+    expect(field).toMatchObject({ field: 'mfa_required', code: 'invalid_value' });
+    expect(field.message).toBe(MFA_REQUIRED_ENROLLMENT_COPY['zh-CN']);
+  });
+
+  // ── 3. Controls ─────────────────────────────────────────────────────────────
+
+  it.each([
+    ['unset', undefined],
+    ['on', 'true'],
+  ])('control: OS_AUTH_TWO_FACTOR %s — the save is accepted and the pass turns enrollment on with it', async (_label, value) => {
+    if (value !== undefined) process.env[ENV] = value;
+    const { settings, manager } = await boot();
+
+    await settings.setMany('auth', { mfa_required: true });
+    expect(await settings.get('auth', 'mfa_required')).toMatchObject({ value: true });
+    await vi.waitFor(() => expect(required(manager)).toBe(true));
+    expect(manager.isTwoFactorEnabled()).toBe(true);
+    expect(manager.getPublicConfig().features.twoFactor).toBe(true);
+  });
+
+  // ── 4. A switch stored on before the check ──────────────────────────────────
+
+  it('a switch stored on before the check is not re-judged, and the gate still reads enrollment off', async () => {
+    const settings = makeSettings();
+    await settings.setMany('auth', { mfa_required: true });
+    process.env[ENV] = 'false';
+    const { manager } = await boot(settings);
+
+    // The value in force: the settings service asks no predicate about it.
+    await expect(settings.setMany('auth', { mfa_required: true })).resolves.toBeDefined();
+    // Read as on, enforced by nobody: `features.twoFactor` is what says so.
+    expect(required(manager)).toBe(true);
+    expect(manager.isTwoFactorEnabled()).toBe(false);
+    expect(manager.getPublicConfig().features.twoFactor).toBe(false);
+  });
+
+  // ── The predicate and its registration, alone ───────────────────────────────
+
+  it('judges a change to on only, and only while enrollment stays off', () => {
+    const off = mfaRequiredEnrollmentPredicate(() => false);
+    expect(off({ next: true, current: false })?.constraint).toEqual({ twoFactor: false, overriddenBy: 'OS_AUTH_TWO_FACTOR' });
+    // Stored toggles read as `bindAuthSettings` reads them.
+    expect(off({ next: 'true', current: null })).toBeDefined();
+    expect(off({ next: 1, current: undefined })).toBeDefined();
+    expect(off({ next: false, current: true })).toBeUndefined();
+    expect(off({ next: null, current: true })).toBeUndefined();
+    expect(off({ next: true, current: 'true' })).toBeUndefined();
+    expect(mfaRequiredEnrollmentPredicate(() => true)({ next: true, current: false })).toBeUndefined();
+    // Unknown locales fall back to English.
+    expect(off({ next: true, current: false, locale: 'fr-FR' })?.message).toBe(MFA_REQUIRED_ENROLLMENT_COPY.en);
+  });
+
+  it('a settings service without write predicates is said at warn, only while the pin makes it matter', () => {
+    const warn = vi.fn();
+    registerMfaRequiredEnrollmentPredicate({}, { warn });
+    expect(warn).not.toHaveBeenCalled();
+    process.env[ENV] = 'false';
+    registerMfaRequiredEnrollmentPredicate({}, { warn });
+    expect(warn).toHaveBeenCalledTimes(1);
+    expect(String(warn.mock.calls[0][0])).toMatch(/OS_AUTH_TWO_FACTOR/);
+  });
+});