Skip to content

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

Merged
objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/project-thread-mjj479
Nov 5, 2026
Merged

objectstack-fleet[bot] merged 7 commits into
mainfrom
claude/project-thread-mjj479

Conversation

@objectstack-fleet

@objectstack-fleet objectstack-fleet Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Requested by Bill · project thread

domain:server#2 · os-bill · session_01Utp6Cbvcmv8YWFhFJwzjKL

维护者速读

  • 改了什么:两步验证(twoFactor)没开时,组织上的「强制多因素认证」(require_mfa)不能从关改成开(新建即开同样不行),返回 400 并说明原因和出路(先开两步验证;环境变量没把它钉成关时,也可以开全局的 auth.mfa_required,它会连同注册入口一起打开)。已经存了开的组织行原样回存照常放行,不挡这一行的其它编辑。同时 MFA 闸门只拦「能去注册的人」:两步验证关着时,任何 MFA 要求都不拦人、不启动宽限期倒计时,服务端只打一条带修法的警告。
  • 为什么改:之前管理员在 Setup 勾上这个框就能存,宽限期一过,全组织所有人(包括所有者)都被 403 MFA_REQUIRED 拦住并被引去注册,而注册接口 POST /api/v1/auth/two-factor/enable 因为插件没开直接 404;Setup 本身也在同一道闸后面,产品里没人能撤回,整个租户锁死。全局开关一直把「要求」和「注册入口」作为一整块打开,组织字段没有这层配对。
  • 风险与回滚:这是一次收窄(Clause-② no (narrowing)):两步验证关着的部署上,以前能存的 require_mfa = true 现在被拒。已经存了 true 的旧组织行不改动、也不再锁人;一旦打开两步验证,它就开始照常要求未注册成员(宽限期从那时起算)。两步验证开着的部署行为完全不变。回滚即撤本 PR。
  • 先看:packages/plugins/plugin-auth/src/organization-mfa-enrollment-guard.ts(拒存规则与文案);auth-manager-password-session-policy.ts 里 resolveUserAuthGates 的 MFA 分支(两步验证关时不拦、不盖戳);auth-manager.ts 的 resolveTwoFactorEnabled(插件列表、features.twoFactor、闸门、拒存四处同一个答案)。
  • 两分钟验证:pnpm --filter @objectstack/plugin-auth exec vitest run src/organization-mfa-enrollment-guard.test.ts,预期 29/29 通过,其中端到端一组断言「两步验证关:注册接口 404,会话不被 MFA_REQUIRED 拦住」。
  • 需要你做的:无,席位落地。界面半边(Setup 文案、注册失败的本地化提示)不在本卡,已列给席位另立。

Fixes #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). Not edited here: outside this card's file surface.

Why

sys_organization.require_mfa is a generically editable extension column, so an administrator could tick Require Multi-Factor Auth on an organization's edit form and it saved, with two-factor off (the default, features.twoFactor: false). Once the grace period ended, the ADR-0069 D3 gate held every unenrolled member 403 MFA_REQUIRED and sent them to enroll, and the enrollment endpoint (POST /api/v1/auth/two-factor/enable) answered 404 because the twoFactor plugin that serves it was never wired. Every member, the owner included, was locked out, and nobody could undo it from inside the product, because Setup sits behind the same gate. The global auth.mfa_required setting never had this hole: its settings piece applies { mfaRequired: true, plugins: { twoFactor: true } } as one refusable patch. The organization column had no such pairing.

What changes

  • The refusal (route b). A new beforeInsert / beforeUpdate hook on sys_organization (organization-mfa-enrollment-guard.ts, priority 15, right after the ADR-0092 D2 identity write guard at 10) refuses, while two-factor is off, a write that turns require_mfa on: an insert with it on, or an update of a row whose stored value is off. validationFailure from @objectstack/types, 400 VALIDATION_FAILED, one fields[] entry { field: 'require_mfa', code: 'invalid_value' } (the closed ADR-0114 catalog). The message says what was refused, why, who would have been locked out (every member, the owner included) and the ways forward; the auth.mfa_required remedy is offered only "unless OS_AUTH_TWO_FACTOR is set to false", because the env var outranks the settings. It is judged for every write context, the system's included (the lockout does not depend on who wrote the row). The stored value is the update context's previous, which the engine binds before beforeUpdate on both update paths (the by-id row it read, and one per-row context per matched row on a multi-row update, before anything is written), so a multi-row update is judged row by row and one row turning it on refuses the whole write with nothing written. An update context without a previous is judged as a change. Re-sending require_mfa: true for a row that already stores it (a whole-row SDK or MCP update, an import re-run, a save with no change) passes, so the row's other edits are not blocked; the gate does not enforce that stored value while two-factor is off anyway. Clearing the column and writing other columns pass untouched. Two-factor state is read per write from the live AuthManager, so a settings pass that turns two-factor on admits the next write with no restart.
  • Why refuse rather than pair (route a was measured and not taken). The twoFactor plugin's switch is process configuration: the settings pass and the OS_AUTH_TWO_FACTOR env, re-applied at every boot and read by the plugin-list builder. An organization row is data. Turning the plugin on from an organization write would hold only in the process that took the write, would not survive a restart or reach another node (nothing that builds the plugin list reads organization rows), and turning it back off when the last organization clears the column would silently drop every enrolled member's second factor. So the card's fallback is taken; the global switch keeps its existing pairing.
  • The gate fail-safe (ADR-0069 D3). resolveUserAuthGates now holds a user for MFA only while they can enroll. With two-factor off, an MFA requirement (an organization's require_mfa, auth.mfa_required under an OS_AUTH_TWO_FACTOR=false pin, or a host config with mfaRequired but no plugins.twoFactor) yields no MFA_REQUIRED and stamps no mfa_required_at; the manager logs one warning per process naming the remedy. An organization row stored require_mfa = true before this lands therefore locks nobody out. Enrolled users were never held and still are not.
  • One answer for "is two-factor on". resolveTwoFactorEnabled (env OS_AUTH_TWO_FACTOR, then plugins.twoFactor, then off) is now the single decision point for the better-auth plugin list, features.twoFactor in /api/v1/auth/config, the gate and the refusal; the two copies of the env read it replaces are gone. It is not re-exported from index.ts. The live reading is AuthManager.isTwoFactorEnabled(), a new public method on an exported class, marked @internal (read by the gate and by the guard AuthPlugin registers; not a supported entry point).
  • Registration. AuthPlugin registers the guard in the kernel:ready identity-guard block, right after registerApiKeyRestoreGuard. This hunk is far from the email_password_enabled settings apply that Authentication settings: turning off email and password with no other sign-in method saves without warning, and everyone is locked out #30202 edits.
  • Existing tests that relied on the dead end. Tests that turned on mfaRequired without plugins.twoFactor (two auth-manager.test.ts helpers, the impersonation policy fixture, and two packages/verify held-session pins) now set plugins: { twoFactor: true }, which is the only configuration in which the hold they pin can be complied with. In held-session-vendor-admin-doors, the block whose hold only the session's organization declares turns two-factor on in its own beforeAll (and puts it back after) instead of inheriting it from the held state that ran before; run alone it now passes 4/4, and without that line its organization write is refused by the guard. held-session-platform-admin already set OS_AUTH_TWO_FACTOR=true.
  • Changeset .changeset/30201-org-mfa-needs-enrollment.md (patch, ! summary, BREAKING accept-set banner, ADR-0087 not-required (no-migration-prescription) marker).

Tests

  • organization-mfa-enrollment-guard.test.ts (new, 29 tests):
    • the rule on a capture engine: registration options, the off/on predicate (false, 0, '0', 'false', null, undefined are off), refusal while off, admission while on, an update refused for a row stored off or with no previous, re-sending it for a row already on admitted (an insert never is), other writes never judged, the message's facts (the conditional settings remedy included) and no tracker number;
    • AuthPlugin registration: kernel:ready registers both hooks on sys_organization above D2's priority, and the captured handler refuses while two-factor is off and admits after applyConfigPatch({ plugins: { twoFactor: true } });
    • a real SQLite ObjectQL engine with SysOrganization and the D2 guard: the administrator's by-id save is refused and the row keeps its value, admitted once two-factor is on, refused for a system write, a multi-row update and an insert; a row that already requires MFA re-saved with it and another column passes while two-factor is off (the row stored off is still refused); a multi-row update over one row on and one off is refused with nothing written, and over rows all on it lands; other writes pass;
    • end to end on real better-auth: an organization stored require_mfa = true, a member past the grace window, active_organization_id set. Two-factor off: POST /two-factor/enable answers 404, the session's authGate is not MFA_REQUIRED, resolveUserAuthGates returns [], the warning fires once and the clock is not restamped. Two-factor on: authGate is MFA_REQUIRED and /two-factor/enable answers 200 with a totpURI.
  • auth-manager.test.ts: "only a user who can enroll is held" (no hold, no stamp, one warning per manager whose settings remedy is conditional on the env var; an OS_AUTH_TWO_FACTOR=false pin outranks plugins.twoFactor and features.twoFactor reads false; control with two-factor on), plus the per-org case (no block and no clock while off).
  • Ablations on 9b256807 (every negative pin, via scripts/ablation-replace.mjs in wrap mode, each restore proven blob == HEAD; plugin-auth tests import their source, so no build sits between mutation and run):
    • A, guard throw deleted: 5 red (the rule's refusal, the registration pin, the real-engine admin, system and multi-row refusals).
    • B, gate fail-safe neutralized: 4 red, including the end-to-end pin expected 'MFA_REQUIRED' not to be 'MFA_REQUIRED'.
    • C, registration in auth-plugin.ts deleted: the registration pin red (expected [] to deeply equal [[beforeInsert…],[beforeUpdate…]]).
    • D, env precedence dropped from resolveTwoFactorEnabled: the env-pin test red (expected true to be false).
  • Ablations, review round on f643025e, same method:
    • F, the unchanged-value pass deleted: 3 red (the rule's re-send case, the real-engine re-save, the multi-row case's landing half).
    • G, a context without previous treated as already on: 3 red (the write was admitted).
    • H, the settings remedy made unconditional: the message pin red.
    • E, the organization block's own two-factor arming deleted (packages/verify, run alone): its beforeAll fails on the guard's ValidationError, 28 skipped.

Acceptance notes

  • The domain:ui half is not here (the UI lane is off duty): the Setup copy on the organization form (who is affected, when the block starts), and a localized enrollment refusal saying what to do. The server's refusal text above is what the form shows until then.
  • Under an OS_AUTH_TWO_FACTOR=false pin, the settings console still shows auth.mfa_required as saved; the gate now does not enforce it (and says so in the log). Before this PR such a deployment held users it could not enroll.
  • A user whose mfa_required_at was stamped before this lands, on a deployment that later turns two-factor on, is held as soon as it is on, because the stamp is older than the grace window. They can enroll at that point, which is the difference from today.
  • This relaxes a security hold on deployments where two-factor is off: a configured MFA requirement is no longer enforced there. That is deliberate (the dispatch's fail-safe): the hold could not be complied with and locked the tenant out; the warning names the remedy.

Verification

Suites, typecheck, gates and the full dogfood battery ran on the final tree, head 05e6e0bd (the review fixes f643025e merged with main at 8b117c0e; neither main merge touched a file in plugin-auth or packages/verify). The first-round ablations ran on 9b256807, the review-round ones on f643025e.

Suites and typecheck (head 05e6e0b, under scripts/pm/os-verify-lock.sh)
  • pnpm --filter @objectstack/plugin-auth exec vitest run --maxWorkers=2: Test Files 221 passed (221), Tests 3909 passed, 10 skipped. VERDICT command-exit 0.
  • pnpm --filter @objectstack/plugin-auth typecheck: VERDICT command-exit 0 (the test-typecheck ratchet holds).
  • pnpm --filter @objectstack/verify exec vitest run on held-session-impersonation-target, held-session-vendor-admin-doors and held-session-platform-admin: Test Files 3 passed, Tests 49 passed. VERDICT command-exit 0. The organization-declared block alone (-t 'organization declares'): 4 passed, 24 skipped.
Full dogfood battery (owed by the narrowing arm)

pnpm --filter @objectstack/dogfood test on 05e6e0bd, under scripts/pm/os-verify-lock.sh: VERDICT command-exit 0 (held the lock 1182s, shared box); Test Files 419 passed, 3 skipped (422); Tests 3492 passed, 36 skipped (3528); no failure. The one red the first round saw on 8c4430f5 (permission-set-clone-door.dogfood.test.ts, from #30222's member_default grant, measured green with that file put back) is gone: main reverted #30222 (#30262) and this head carries the revert.

Gates

node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack on 05e6e0bd: 64 derived, each run with its exit code recorded before any pipe. A session restart cut the first pass after 60 (all exit 0); the other 4 were re-run: check:type-check-debt exit 0 under the lock, check:undeclared-dep-imports and check:where-matcher exit 0, and check:watch-hint-literal exit 1 on a packages/spec/tsup.config.bundled_*.mjs temp file that the concurrent locked build wrote and removed mid-scan (ENOENT), then exit 0 re-run with no build in flight. --ran over the final record: 64 derived, 64 run, 0 NOT-MEASURED, 0 UNRUN, exit 0. pnpm lint exit 0.

Human Minimum

  • 1. Schemas start as Zod; packages/spec holds no runtime logic (packages/spec not touched).
  • 2. Every name I added uses the casing for its kind.
  • 3. No formatter ran; pnpm lint passes (reading under Verification).
  • 4. One issue, one branch off an up-to-date main, one pull request.
  • 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset.
  • 6. packages/spec touched: n/a.
  • 7. No hand edits to content/docs/references/, packages/*/CHANGELOG.md or content/docs/releases/.
  • 8. Title, body and comments are in English (the 维护者速读 block is Chinese by the Communication rule).

Generated by Claude Code

…or is off; the MFA gate holds only enrollable users

Claude-Session: https://claude.ai/code/session_01Utp6CYRZxPEjGeTbGfW4YC
Co-authored-by: Claude <noreply@anthropic.com>
…nt guard on kernel:ready (#30201)

The guard is registered on sys_organization for insert and update, after
the ADR-0092 D2 identity write guard, and it asks the live auth manager
per write, so a settings pass that turns two-factor on admits the next
write.

Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Utp6CYRZxPEjGeTbGfW4YC
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/plugin-auth, touching 29 documentable anchor(s).

46 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json bfca0efb79ac5df2049f392cdecd3bbeeea9ec61.

⛔ 9 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 cross-cutting symbol(s) contributed no route anchor: packageId (4 routes)
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 103 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 22 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json bfca0efb79ac5df2049f392cdecd3bbeeea9ec61 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from b0cadad1dc48de8323409ab84244e096bf13b914 — the merge of head 05e6e0bd2330f2cc10f98d0bbb0d1e9241885529 into base bfca0efb79ac5df2049f392cdecd3bbeeea9ec61, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin b0cadad1dc48de8323409ab84244e096bf13b914 && git checkout b0cadad1dc48de8323409ab84244e096bf13b914
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin bfca0efb79ac5df2049f392cdecd3bbeeea9ec61 05e6e0bd2330f2cc10f98d0bbb0d1e9241885529 && git checkout -B drift-repro bfca0efb79ac5df2049f392cdecd3bbeeea9ec61 && git merge --no-ff 05e6e0bd2330f2cc10f98d0bbb0d1e9241885529

node scripts/docs-audit/affected-docs.mjs --json bfca0efb79ac5df2049f392cdecd3bbeeea9ec61

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs bfca0efb79ac5df2049f392cdecd3bbeeea9ec61 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-bill · session_01Utp6Cbvcmv8YWFhFJwzjKL

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 8c4430f5fdf9d92204dd4f13a523f5e95d15b55a
Local-runs: none
Scope: packages/plugins/plugin-auth/src/auth-manager-password-session-policy.ts, packages/plugins/plugin-auth/src/auth-manager-plugin-list.ts, packages/plugins/plugin-auth/src/auth-manager.ts, packages/plugins/plugin-auth/src/auth-manager.test.ts, packages/plugins/plugin-auth/src/auth-plugin.ts, packages/plugins/plugin-auth/src/organization-mfa-enrollment-guard.ts, packages/plugins/plugin-auth/src/organization-mfa-enrollment-guard.test.ts, packages/verify/src/held-session-impersonation-target.pin.test.ts, packages/verify/src/held-session-vendor-admin-doors.pin.test.ts + .changeset/30201-org-mfa-needs-enrollment.md, packages/platform-objects/src/identity/sys-organization.object.ts (require_mfa), packages/plugins/plugin-auth/src/managed-extension-fields.ts (the D2 editable tier), packages/plugins/plugin-auth/src/identity-write-guard.ts (D2, priority 10, no object filter), packages/plugins/plugin-auth/src/last-admin-guard.ts (ctx.input.data precedent), packages/plugins/plugin-auth/src/auth-plugin.ts lines 2214-2225 on main (the global mfa_required settings piece), packages/plugins/plugin-auth/src/auth-manager.ts applyConfigPatch and readBooleanEnv on main, packages/plugins/plugin-auth/src/index.ts, packages/objectql/src/engine.ts (by-id, multi-row and insert hook contexts), packages/types/src/validation-failure.ts, packages/rest/src/error-response.ts (mapDataError), packages/spec/src/api/errors.zod.ts (ADR-0114 catalog), docs/adr/0069-enterprise-authentication-hardening.md D3, ui/packages/plugin-form/src/sanitize.ts and occSave.tsx (what the Console's edit form sends), packages/qa/dogfood/test/upgrade-harness fixtures (stored require_mfa rows), scripts/pm/clause2-line.mjs and scripts/check-adr-0087-registration.mjs (the declaration readers).

Inputs read: card #30201 body and all four comments (6648424134, 6648643507, 6650356190, 6650376678); PR #30293 body, file list and the net diff against main at merge-base bb86ff9 (10 files, +692/-12); the check-runs on this head. Check-runs at judgement (05:29:46Z, after a ten-minute wait from 05:18Z): 32 success, 13 skipped, 0 failure; still in progress: Test Core (1/8) and Test Core (3/8). Every other required context is green (Lint and Repo Gates, Build Core, Governed Surface Queue Guard, Test UI, Live E2E UI; Type Check UI, Lint UI and Build and E2E UI skipped by path filter; Type Check workspace, consumer, source and debt gates success; Test Core shards 2, 4, 5, 6, 7, 8 success). The landing still waits on the two shards; this record judges the diff.

① Derived judgments

Accept-set change, the one narrowing. On a deployment where two-factor is off (resolveTwoFactorEnabled: OS_AUTH_TWO_FACTOR env, then plugins.twoFactor, then off), a write to sys_organization whose payload sets require_mfa to anything but false, 0, '0', 'false', null or undefined is refused. Right: the card's own fallback, and the value an administrator could save before is the one that produced the whole-tenant lockout.

Doors, read in the tree, each right:

  • REST data API by-id PATCH and the Setup organization form (the Console's generic record editor, which sends dirty fields through dataSource.update): ObjectQL by-id update builds one hookContext with input.data = the persist image (engine.ts near line 9220); the D2 identity-write guard at priority 10 admits a user-context write of a whitelisted column (require_mfa is in MANAGED_EXTENSION_EDITABLE_FIELDS for sys_organization), then this guard at 15 judges it. Pinned on a real SQLite engine with the real SysOrganization and the real D2 guard.
  • Multi-row update (where plus multi): dispatchUnscopedMultiWriteHooks and dispatchPerRowBeforeHooks both carry the same hookContext whose input.data is the shared payload (engine.ts near lines 9526 and 9566), so one judgement covers every matched row. Pinned.
  • Insert: the per-row beforeInsert context carries input.data (engine.ts near line 7626); a user-context insert of sys_organization is already refused by D2 at 10, a system insert reaches this guard. Pinned.
  • System context: judged, not bypassed. Right, and deliberate (the lockout does not depend on who wrote the row); the last-admin guard is the precedent.
  • Metadata or Setup edit outside the data API: sys_organization is a data object, there is no metadata door for its rows; the Setup form is the data API door above.
  • Not covered and not needed: raw SQL restores (the upgrade-harness fixtures load database.sql directly; their stored rows carry require_mfa = 0).

Gate fail-safe. resolveUserAuthGates now holds a user for MFA only while mfaRequired and unenrolled and isTwoFactorEnabled(); otherwise it calls noteMfaRequirementWithoutEnrollment() (one warn per manager naming the remedy) and neither holds nor stamps mfa_required_at. Right: the hold answered MFA_REQUIRED to a user whose remediation endpoint did not exist; the twoFactor plugin is pushed directly into the better-auth plugin list (auth-manager-plugin-list.ts line 271, not through addOptionalPlugin), so config on means the endpoint is wired and the four readers (plugin list, features.twoFactor, gate, guard) cannot disagree. Enrolled users were never held and still are not.

Response code and shape. validationFailure from @objectstack/types builds name ValidationError, code VALIDATION_FAILED, one fields[] entry { field: 'require_mfa', code: 'invalid_value' }; mapDataError in packages/rest keys 400 on that code; invalid_value is a member of the closed ADR-0114 catalog (errors.zod.ts line 285). Right.

Public surface. resolveTwoFactorEnabled is exported from auth-manager.ts but not re-exported from index.ts (the named list is unchanged), right. AuthManager.isTwoFactorEnabled() is a NEW public method on a class index.ts does export, so the published surface of @objectstack/plugin-auth gains one method; the PR body's "No published export ... is added; nothing is widened" misses it. A should-fix on the prose (or mark the method internal), not a semver change: the narrowing arm already carries the BREAKING banner, and an additive method on a patch-level narrowing changes no level.

② Semver level

Changeset .changeset/30201-org-mfa-needs-enrollment.md: @objectstack/plugin-auth patch, summary line with the ! marker, a BREAKING banner naming the accept-set narrowing, Before / Now / The gate / One answer / Unchanged, and a Remedy paragraph. Level is right by the Post-Task Checklist rule: Metadata-protocol none (sys_organization.require_mfa is a data row value, nothing a metadata author writes changes) takes patch; a non-protocol narrowing ships patch and keeps its BREAKING banner and ADR-0087 disposition, which it does.

Declaration lines. PR body and changeset both read Clause-②: no (narrowing) and Metadata-protocol: none; the one reader (scripts/pm/clause2-line.mjs) reads the value and the arm, and Check Changeset is green. The two Clause-② lines are not byte-identical beyond the value: the body's tail ends "No published export, error code or settings key is added; nothing is widened." and the changeset's ends "Nothing is widened." Should-fix, cosmetic: make the tails one sentence (and drop the "no published export" claim, see ① on isTwoFactorEnabled).

ADR-0087 marker: exactly one HTML-comment marker, adr-0087: not-required (no-migration-prescription), a spelling check-adr-0087-registration.mjs recognises (its line 931). The justification holds against the tree: no spec key, export, stored shape or stored row is removed, renamed, rewritten or dropped; the remedy is a deployment setting (OS_AUTH_TWO_FACTOR or auth.mfa_required), which no migration ledger can derive; the other categories are closed on facts the diff shows (the package publishes, no ADR-0087 id, no export removed or retyped). Right.

③ Boundary flags

  • Clause-② re-declared no (narrowing) against the Claim's no: right, the dev's measurement of route (a) holds in the tree (plugin-list membership is process config re-applied at boot; nothing that builds the plugin list reads organization rows), so the refusal is the only route that survives a restart and a second node. The claim correction 6650376678 accepts it subject to this review.
  • Two packages/verify pins outside the claimed surface, one key each (plugins: { twoFactor: true } beside mfaRequired). The edits do not weaken the pins: each still arms the MFA_REQUIRED posture and asserts the 403, the code and the effect never landing; two-factor on is now the only configuration in which the hold they pin exists. Should-fix, test fragility introduced by the fail-safe: in held-session-vendor-admin-doors.pin.test.ts the describe "a hold only the session's organization declares" (line 333) arms nothing of its own; its ql.update(require_mfa: true) and its expectArmed(MFA_REQUIRED) both pass only because the earlier HELD_STATES arm (line 260) left plugins.twoFactor true in the manager (its disarm at line 265 resets mfaRequired alone). Run alone or reordered, that block now fails on this head. It should arm plugins.twoFactor itself in its beforeAll. held-session-impersonation-target has no such dependent block (disarmMfa leaves twoFactor on, nothing later asserts it off).
  • Skills-docs follow-up (content/docs/permissions/authentication.mdx, the mfa_required row): with this head the sentence "also settable per organization via require_mfa" is wrong on a two-factor-off deployment; should-fix, carried by the seat as the report says; also docs/HARDENING.md line 242 reads the same way.
  • Route (a) not taken: answered in ① and ④.
  • dispatch-gates without paths; unlocked dogfood and verify builds; the depth-1 fetch; the recreated commits with the model-free trailer pair: process notes, none touches the diff.
  • main moved 20 commits after the pre-PR merge, none in plugin-auth or packages/verify: the seat re-pulls before landing; the two Test Core shards still running are the only open CI state.
  • Write budget 6, not 3: accounted (four pushes, one pr_create, one comment).
  • open_questions: none in the report; none left by this review.

④ Design judgment

The narrowing against the published spec on main. ADR-0069 D3 declares the per-organization capability as "an org may require MFA above the global floor", with the mechanism row for mfa_required resting on the twoFactor plugin ("twoFactor plugin already provides TOTP"), and its hard constraint (ADR-0049, line 10) says a Require-MFA toggle that does not actually block is worse than nothing. On main the organization toggle met neither half honestly: it blocked everyone, and the enrollment half of D3's mechanism was absent, so what the consumer had was not the declared capability but a lockout. This diff takes nothing the spec declares away at the consumer: the org still tightens above the floor whenever the floor's own mechanism (the twoFactor plugin) is present, and it makes the spec's own precondition real by refusing the write when it is not, with the remedy in the refusal. The gate fail-safe is the same reading applied to rows that already exist. PASS on the design, with the findings below, none must-fix.

Breaking attempts:

  • Two-factor turned OFF after organizations saved require_mfa = true (global switch turned off then restarted, or an env pin): no lockout; the gate warns once and holds nobody; the row keeps true, so the organization form and GET /api/v1/data/sys_organization still show Require Multi-Factor Auth ticked while features.twoFactor is false. Nothing on the read side says "not enforced". Should-fix (the domain:ui half already listed for the seat, plus the API: a reader has to join two surfaces to learn the truth). Not a must-fix here: the write door now refuses the lie going forward, the server says so in the log, and the alternative was the lockout.
  • OS_AUTH_TWO_FACTOR=false pinned: the org write is refused, right; the global auth.mfa_required setting still saves and shows on while the gate does not enforce it (the PR's acceptance note). Two should-fixes, same file surface: (i) the refusal message's second remedy, "turn on the auth.mfa_required setting, which ... turns enrollment on with it", is false under exactly this pin, since the env outranks the settings patch (readBooleanEnv first in resolveTwoFactorEnabled); under a pin the only remedy is the env, and the message should say so or drop the second remedy when the env is set. (ii) the settings piece at auth-plugin.ts line 2220 could refuse mfa_required = true the same way this guard refuses the org write when the env pins two-factor off, instead of saving an unenforced switch; pre-existing shape, not introduced here.
  • OS_AUTH_TWO_FACTOR=true pinned: plugin list, features, gate and guard all read true; every write admitted, every unenrolled member held after grace. Holds.
  • Global mfa_required interplay: on turns plugins.twoFactor on in the same refusable patch, so org writes are admitted from then on and a stored org row starts holding; off patches mfaRequired alone, twoFactor stays on in-process until restart, so held members stay held and still can enroll until the next boot, when the settings pass no longer turns two-factor on and the fail-safe takes over. No window in which a user is held without an enrollment path. Holds.
  • A seeded or package-installed organization row with require_mfa = true at boot: the guard binds at kernel:ready; a system insert after that is refused with the same 400, which is a correct, located refusal of a row the deployment cannot honour (the install fails on that row with the remedy in the message), not a silent drop. Nothing in this repository seeds such a row (the three upgrade-harness fixtures store 0 and load by raw SQL, so no hook runs). Holds.
  • Multi-node: the guard and the gate read the node's own config; the settings pass runs on every node at boot, and a runtime settings change propagates exactly as every other auth setting does today (not by this diff). A node where two-factor is on holds and admits writes, a node where it is off neither holds nor admits them; no node holds a user it cannot enroll. Holds; the cross-node settings lag is pre-existing and out of scope.
  • mfa_required_at no longer stamped while two-factor is off: its readers on main are the gate itself and documentation (the QA checklist describes the lazy stamp); no code reads it elsewhere. Consequence pinned by the diff's own e2e: a user stamped during the old dead end is held the moment two-factor turns on (the stamp is older than the window) and POST /two-factor/enable answers 200 for that held session, so the hold is now compliable. A user never stamped gets a fresh window when enrollment opens. Holds; the "held at once" case is a disclosed, correct outcome, since the window existed to allow enrollment and the user could not enroll then.
  • Judged on the payload, not on the transition. A writer that re-sends a stored require_mfa = true unchanged on a two-factor-off deployment is refused, with a message saying the organization "cannot require multi-factor authentication yet" although the row already says it does: full-row clients (an SDK or MCP update that echoes the record, an import re-run) and the Console's no-change save (dirtyEditPayload falls back to the whole payload when nothing changed) cannot touch such an organization until they untick the box or turn two-factor on. The by-id context carries previous, so an unchanged true could pass while a false-to-true transition is refused; the multi-row path would need the prior rows it already reads. Should-fix; not a must-fix because the Console's edit form sends only changed fields, so the card's door is unaffected, and the refusal names a real remedy.
  • turnsMfaRequirementOn treats 'off' and 'no' as on (readBooleanEnv treats them as off): the column is boolean, the engine refuses those strings later anyway, and the guard's docblock says so. Not a finding.
  • The refusal reaches a user-context caller only after D2 admitted the write, so a non-administrator never learns the deployment's two-factor state from this message. Holds.

Findings, each named:

  • should-fix: vendor-admin-doors org-only block depends on an earlier state's leftover plugins.twoFactor (③).
  • should-fix: refuse on the false-to-true transition rather than on an echoed true (④).
  • should-fix: the refusal's second remedy is false under an OS_AUTH_TWO_FACTOR=false pin (④).
  • should-fix: the global mfa_required settings door saves an unenforced switch under the same pin, the symmetric gap (④, pre-existing shape).
  • should-fix: a stored require_mfa = true reads as required on every surface while unenforced; the UI half and an API-side signal (④).
  • should-fix: PR body says no published export is added, but AuthManager.isTwoFactorEnabled() is a new public method on an exported class (①).
  • should-fix: Clause-② line tails differ between PR body and changeset (②).
  • should-fix: authentication.mdx and HARDENING.md still say require_mfa is settable per organization unconditionally (③).
  • must-fix: none.

Implemented-by: session_01Utp6CYRZxPEjGeTbGfW4YC
Reviewed-by: session_01Utp6Cbvcmv8YWFhFJwzjKL

VERDICT: PASS

…e settings remedy works

Contract review follow-ups:

- The guard judges the change, not the payload: an update is refused only
  for a row whose stored value (the engine's `previous`, bound on the by-id
  and the per-row multi-row paths) is off. Re-sending `require_mfa: true`
  for a row that already stores it passes, so a whole-row update, an import
  re-run or a save with no change no longer blocks the row's other edits.
  A multi-row update is judged row by row; a context with no `previous` is
  judged as a change.
- The refusal and the gate's warning offer the `auth.mfa_required` remedy
  only where it can work: the OS_AUTH_TWO_FACTOR env var outranks it.
- AuthManager.isTwoFactorEnabled is marked @internal.
- The organization-declared held-session block turns two-factor on itself
  instead of inheriting it from the held state that ran before.

Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Utp6CYRZxPEjGeTbGfW4YC
@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-bill · session_01Utp6Cbvcmv8YWFhFJwzjKL

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 05e6e0bd2330f2cc10f98d0bbb0d1e9241885529
Local-runs: none
Scope: packages/plugins/plugin-auth/src/auth-manager-password-session-policy.ts, packages/plugins/plugin-auth/src/auth-manager-plugin-list.ts, packages/plugins/plugin-auth/src/auth-manager.ts, packages/plugins/plugin-auth/src/auth-manager.test.ts, packages/plugins/plugin-auth/src/auth-plugin.ts, packages/plugins/plugin-auth/src/organization-mfa-enrollment-guard.ts, packages/plugins/plugin-auth/src/organization-mfa-enrollment-guard.test.ts, packages/verify/src/held-session-impersonation-target.pin.test.ts, packages/verify/src/held-session-vendor-admin-doors.pin.test.ts + .changeset/30201-org-mfa-needs-enrollment.md, packages/objectql/src/engine.ts (update(): the by-id prior read, its not-found gate and the previous binding before the beforeUpdate dispatch; the predicate branch's matched-row read, hasHooksFor and the per-row dispatch before updateMany; the unscoped-multi dispatch), packages/objectql/src/engine-hook-dispatch.ts (dispatchPerRowBeforeHooks, the per-row previous binding), packages/objectql/src/engine-insert.ts (readUpdatePriorRow, declared columns only), packages/objectql/src/engine-top-level.ts (update.options.upsert retired), packages/core/src/utils/import-runner.ts (update and upsert rows written through updateData by id), packages/plugins/plugin-auth/src/auth-manager.ts applyConfigPatch (plugins merged shallowly) and resolveTwoFactorEnabled over readBooleanEnv, packages/plugins/plugin-auth/src/managed-extension-fields.ts (require_mfa in the editable tier), packages/plugins/plugin-auth/tsup.config.ts (dts emitted, no stripInternal anywhere in the repo), packages/platform-objects/src/identity/sys-organization.object.ts (require_mfa declared boolean), scripts/check-adr-0087-registration.mjs (the not-required categories), scripts/pm/clause2-line.mjs (the declaration reader), packages/qa/dogfood/test/upgrade-harness fixtures (stored require_mfa rows).

Delta review of the head the full review passed (record 6650579682, head 8c4430f). Inputs read: card #30201 body and its four comments; PR #30293 body as it stands and its pending replacement (server-seat2/30201/pr-body-v2.md); the file list, the net diff against main at this head (10 files, +794/-12) and the delta 8c4430f..f643025 (6 files, +127/-25); the check-runs on this head. The range 8c4430f..05e6e0b holds exactly two commits: f643025 (the fix) and 05e6e0b, one merge of main at 8b117c0 (the earlier head was itself the merge at bb86ff9, so this PR carries two main merges in all). The diff f643025..05e6e0b over plugin-auth, packages/verify and the changeset is empty: the merge touches no path of this PR; what it brings to packages/objectql/src/engine.ts is main's own, and every engine citation below was read at this head. Check-runs at judgement (2026-11-05T06:49Z): after five readings over ten minutes from the first at 2026-11-05T06:40Z to the last at 2026-11-05T06:49Z: 29 success, 13 skipped, 0 failure or other; still in progress: 5 (Test Core (1/8), Test Core (2/8), Test Core (3/8), Test Core (4/8), Test Core (7/8)). Green and complete: Governed Surface Queue Guard, Check Changeset, PR-body guards, every Lint job, Type Check consumer, source and debt gates, Build Core, Test UI, Live E2E UI, Dogfood Regression Gate; Type Check UI, Lint UI and Build and E2E UI skipped by path filter. The shards still running are the landing's wait; this record judges the diff, as the earlier one did.

① Derived judgments

The delta replaces the payload rule with a transition rule. Every accept-set change it makes, judged:

  • A by-id update whose payload carries require_mfa on for a row that already stores it on, two-factor off: REFUSED before, PASSES now. Right. The write turns nothing on; the gate does not enforce the stored value while two-factor is off (resolveUserAuthGates, unchanged in the delta); refusing it blocked every other edit of that row through a whole-record door, which was the earlier record's should-fix. The engine binds previous unconditionally on this path (readUpdatePriorRow before the dispatch) and throws recordNotFound before any hook when the row is absent, so the guard never meets a by-id context without previous.
  • A multi-row update whose payload carries on and whose every matched row stores on: REFUSED before, PASSES now. Right, same reasoning. The predicate branch reads the matched rows (readPriorRows over the composed AST and the same driver options updateMany uses) whenever hasHooksFor('beforeUpdate', object) is true, which the guard's own registration makes true, and dispatchPerRowBeforeHooks binds previous = that row, coerced, on each per-row context, all before updateMany. The dev's real-engine pin covers it (where slug acme, both rows on, lands with timezone written).
  • A multi-row update with at least one matched row stored off: refused before and now, with nothing written. Right; now judged per row, and the throw from any row's chain leaves updateMany unreached. Pinned (org_a on, org_b off: refused, org_b still off, org_a's timezone not written).
  • An insert with on, and a by-id update of a row stored off, null or never set: refused before and now. Right. previous.require_mfa null or absent is off, so an update that sets it is a change.
  • A beforeUpdate context with no previous: refused. Right as a fail-safe; no engine path produces one (by-id: unconditional read, 404 first; predicate: per row), so it is reachable only by a hand-built context, and a context that cannot prove the prior state is not evidence the value was on.
  • A multi-row update matching zero rows: zero per-row dispatches (D1) before and now; the unscoped-multi dispatch reaches only registrations that declare for it, which this guard does not. No change.
  • Stored forms: previous arrives through coerceBooleanFields on both paths, and turnsMfaRequirementOn reads 1, '1', 'true' as on and 0, '0', 'false', null, undefined as off, so a SQLite 0/1 or an uncoerced string judges the same way.
  • The reverse direction, a write that passed before and is refused now: none. The transition rule accepts a strict superset of the payload rule, so the narrowing declared on the earlier head is now narrower, not wider, and no new refusal appears.
  • Copy: the refusal and the once-per-manager warning now offer the auth.mfa_required remedy only unless OS_AUTH_TWO_FACTOR is set to false. Right: resolveTwoFactorEnabled reads the env first, so under a false pin the settings patch does not turn enrollment on. Both strings are pinned (the message pin in the guard test, the warn pin in auth-manager.test.ts).

② Semver level

Changeset .changeset/30201-org-mfa-needs-enrollment.md at this head: @objectstack/plugin-auth patch, the summary with the ! marker, the BREAKING banner, Before / Now / The gate / One answer / Unchanged, the Remedy paragraph, and exactly one HTML-comment ADR-0087 marker reading not-required (no-migration-prescription), a category check-adr-0087-registration.mjs lists. The delta rewrites the Now bullet, the Remedy paragraph and the Clause-② line to the transition rule; nothing moves the level. Patch stays right: Metadata-protocol none (a data-row value, nothing an author writes), a non-protocol narrowing ships patch and keeps its banner and disposition, and the delta adds no export, code or key. The disposition's justification still holds against the tree: no spec key, export, stored shape or stored row is removed, renamed or rewritten, and the remedy is a deployment or settings change no ledger can derive.

Declaration lines. The two line-initial declarations in the replacement body (pr-body-v2.md) and in the changeset are byte-identical: Clause-②: no (narrowing) with the transition-rule tail, and Metadata-protocol: none. The earlier record's tail-mismatch should-fix is closed in the replacement. The PR body as it stands on GitHub still carries the earlier head's Clause-② line (the payload rule, 25 tests), so until the replacement is written in, the live body and the changeset differ in their tails and the body describes a rule the head no longer implements; the reader (clause2-line.mjs) reads the value and the arm, both unchanged, so Check Changeset is unaffected. Should-fix, process: write pr-body-v2.md in once its DOGFOOD_PLACEHOLDER is filled, and read the body back.

The @internal mark. No tsconfig or tsup config in the repository sets stripInternal, and plugin-auth's tsup emits dts, so AuthManager.isTwoFactorEnabled() is still present in the published .d.ts; @internal changes the published-surface reading by convention (a TSDoc contract, visible to a consumer who reads it) and not mechanically. The Clause-② line's "No published export ... is added" is literally true (the named list in index.ts is unchanged; a method on an exported class is not an export) and the replacement body now discloses the method and its mark in the same paragraph, which is the honest reading. The level is unchanged either way: an additive method on a patch-level narrowing. Closed. packages/verify's pin calls it on an any-typed handle, as it already does _orgMfaCache; no published consumer binds to it.

③ Boundary flags

Each should-fix of record 6650579682:

  • vendor-admin-doors organization-declared block depends on leftover plugins.twoFactor: CLOSED. Its beforeAll reads isTwoFactorEnabled(), applies plugins.twoFactor true itself and the afterAll restores false only when it was false; applyConfigPatch merges plugins shallowly over the standing config, so no sibling plugin flag is dropped by either patch. In file order the earlier MFA_REQUIRED arm leaves two-factor on and the block restores nothing; run alone it arms and restores. The dev's E ablation (arming deleted, run alone) reports the beforeAll failing on the guard's refusal.
  • Refuse on the transition rather than on an echoed true: CLOSED (①).
  • The refusal's second remedy false under an OS_AUTH_TWO_FACTOR=false pin: CLOSED, in the refusal and in the warning, both pinned.
  • The global auth.mfa_required settings door saves an unenforced switch under the same pin: STILL OPEN. Not in the delta; pre-existing shape; the replacement body's acceptance note discloses it. Should-fix, carried.
  • A stored require_mfa true reads as required on every surface while unenforced (the UI half and an API-side signal): STILL OPEN. Carried by the seat as the domain:ui half; not a code finding of this diff.
  • "No published export is added" against isTwoFactorEnabled(): CLOSED by the @internal mark and the disclosure in the replacement body (②).
  • Clause-② tails differ between PR body and changeset: CLOSED in the replacement and the changeset; OPEN on the live body until the replacement is written (②).
  • authentication.mdx and HARDENING.md still say require_mfa is settable per organization unconditionally: STILL OPEN. The Skills-docs line names authentication.mdx only; docs/HARDENING.md line 242 reads the same way and is not named. Should-fix, carried, name both.

New flags in the delta:

  • The two packages/verify edits of the earlier head are unchanged; the delta adds seven lines to one of them, inside the already-accepted file surface (claim correction 6650376678). No new path.
  • The replacement body's dogfood section is DOGFOOD_PLACEHOLDER: the full battery the narrowing arm owes has run on the earlier head only (one red attributed to fix(plugin-security): every member reads and posts comments, and reads the activity history, of the records they can read #30222, reverted on main at revert: PR #30222 (main red #30255) #30262, and main is now merged into this head). A landing precondition the seat holds, not a code finding; this record does not substitute for it.
  • Draft, auto-merge off, head repo is the base repo. plugin-auth and packages/verify are not governed surfaces; landing is the seat's once every check is green.
  • Check-runs: judged on the state named above; the still-running contexts, if any, are the landing's wait, not this record's.

④ Design judgment

The transition rule against the published spec on main. ADR-0069 D3 declares the per-organization requirement above the global floor on the twoFactor plugin's mechanism; the earlier record found the refusal makes that precondition real at the write door and the gate fail-safe applies the same reading to stored rows. The delta narrows the refusal to the writes that create the state the spec cannot honour and lets every write that leaves it as stored through, which is the right boundary: a value the gate does not enforce is not made more dangerous by being echoed, and refusing the echo took the row's other edits hostage. PASS on the design. Attempts to break it:

  • An update path that does not bind previous. Bulk update by filter: the predicate branch reads the matched rows before the per-row dispatch whenever the object has a beforeUpdate hook, and this guard's registration is one, so previous is bound per row through the same AST and driver options updateMany then uses, and the rows judged are the rows written. Import and seed: the import runner (packages/core/src/utils/import-runner.ts) resolves an update or upsert row's target by its match fields and writes it with updateData by id, so a re-run echoing true passes and an off-to-on row is refused per row; insert mode goes through insert and is refused when on. Upsert: the engine's update.options.upsert is a retired key (ENGINE_UPDATE_UPSERT_REMOVED); there is no upsert door that could bypass the by-id read. Raw driver writes and raw SQL run no hooks at all: no non-test writer of require_mfa exists outside the engine at this head, the harness fixtures store 0, and this exposure is the earlier head's, unchanged by the transition rule. Holds.
  • A race of two concurrent updates. previous is read before the write with no lock or precondition, as every previous-reading guard here (last-admin included). The one ordering that admits a value the rule would otherwise refuse: writer A clears true to false, writer B read true before A landed and echoes true, B passes, the row ends true with two-factor off. The outcome is a row stored true, which the gate does not enforce while two-factor is off, which holds nobody, and which the next off-to-on write is judged against; no lockout path opens. The other orderings refuse (B's previous read false after A cleared it) or admit only while two-factor is on. Holds; not a finding.
  • true to false to true within one batch. A multi-row update carries one shared payload for every matched row (D3 pins the payload by reference identity), so a batch cannot carry both values; a batch insert judges each row's own payload with no previous; sequential by-id writes are each judged against the row as stored at that moment (true to false passes, false to true is refused). A hook earlier in the per-row loop that rewrote the shared payload to off would make later rows pass, which is the clearing direction and is also what the key-divergence refusal exists for. Holds.
  • An update that leaves require_mfa unchanged but is judged as a change. Needs a beforeUpdate context without previous or with previous.require_mfa off while the row stores on. The by-id read is unconditional and reads declared columns of the whole row (require_mfa is declared); the predicate read is the whole matched row, declared columns only; coercion keeps a 1 as on; a row the caller's row scope hides is a 404 before any hook. None reachable. Holds.
  • Ordering inside the guard: the unchanged check runs before the two-factor read, so an echoed true never consults config; the insert handler ignores a previous it is handed. Right.

Findings, each named:

  • should-fix: the live PR body still carries the earlier head's Clause-② line and rule text; write pr-body-v2.md in after the dogfood reading fills its placeholder, and read it back (②).
  • should-fix, carried: the global auth.mfa_required settings door saves an unenforced switch under an OS_AUTH_TWO_FACTOR=false pin (③, pre-existing).
  • should-fix, carried: a stored require_mfa true reads as required on every surface while unenforced; the UI half and an API-side signal (③).
  • should-fix, carried: authentication.mdx and docs/HARDENING.md line 242 still say require_mfa is settable per organization unconditionally; the Skills-docs line names only the first (③).
  • must-fix: none.

Implemented-by: session_01Utp6CYRZxPEjGeTbGfW4YC
Reviewed-by: session_01Utp6Cbvcmv8YWFhFJwzjKL

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#2 · os-bill · session_01Utp6Cbvcmv8YWFhFJwzjKL

维护者速读

席位核对 — ACCEPT · domain:server#2 · head 05e6e0bd2330f2cc10f98d0bbb0d1e9241885529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Organization require-MFA can be turned on with two-factor off, and after the grace period everyone is locked out with no way to enroll

1 participant