fix(migrate): classify by directory identity before local password (H16)

[alghanim]

Fifth of six. Stacked on #62.

⚠️ Wire break — read first

SchemaRev 1 → 2. Classification happens on the SOURCE side, so a patched central cannot trust the Kind values in a rev-1 bundle. guardMigrationCall exact-matches the rev, so this surfaces as a clear "incompatible migration bundle" rather than a silent import of buggy classification.

Any in-flight migration must upgrade the source deployment first. This belongs in the release notes.

The bug

classifyUser tested u.PasswordHash != "" before the directory signal, so any AD-backed user who also carried a local hash migrated as an app-local user keyed by that credential. Three things went wrong at once:

• The central ended up holding a standing password for someone whose identity is directory-governed, so the account outlived AD-side disablement, lockout and password policy — exactly what this package's doc comment promises never happens ("no record or password is copied").
• Classify never blocks a local user, so an AD population misclassified this way sailed straight past the "central is on a different AD" guard.
• Apply flipped allow_local_users on the target for accounts that must never use the app-local login path.

This composes with SA-7 (#61): that bug was one way an AD user acquired a local hash in the first place. Fixing SA-7 reduces the population going forward but neither eliminates it (master-admin break-glass stays legitimate) nor cleans existing data, so the precedence inversion is independently necessary.

Approach

Resolve the directory key first; only a user with no directory identity is local. A KindAD entry no longer carries a password hash — the central re-binds that person from the same AD, so copying the credential would recreate the very shadow account this prevents. A HadLocalPassword boolean (never the hash) travels instead, so the dry run can tell the operator which break-glass logins do not survive.

directoryKey prefers SAMAccountName but falls back to ldap/kerberos mappings, because handleImportLDAPUsers provisions a user with an ldap mapping and no SAMAccountName until first login. The local provider is deliberately not consulted: every JIT provision writes both an ldap and a local mapping, which is exactly how localUsername happily returned an AD username under the old precedence.

Selection is deterministic across backends — GetMappingsForUser returns insertion order on Bolt and unordered rows on Postgres, so the bare (non-@) form wins and ties break lexicographically.

Two issues an adversarial review caught in the first cut

1. directoryKey failed open. A swallowed error from GetMappingsForUser reads as "no directory identity" → classifies LOCAL → exports the password hash. It now fails closed; TestClassifyFailsClosedOnMappingError pins it (and is written so it genuinely fails against the old code — the first draft of that test could not fail).
2. allow_local_users flipped for zero-role entries that Apply skips and Classify no longer counts — opening the target's local-login gate for a user never created.

Also fixed: dishonest preflight counts

Apply skips a user with no effective roles entirely while Classify counted them as migrating, so preflight promised N and Apply delivered fewer with nothing explaining the gap. Report.NoRoles now counts them separately, with a note.

Note on the two directory predicates

handler.isDirectoryBacked (SA-7) is deny-by-default; migrate.directoryKey is an allow-list of ldap/kerberos. The asymmetry is intentional — one returns a verdict, the other must return a KEY. Do not unify them; internal/migrate importing from internal/handler would invert the dependency direction.

🤖 Generated with Claude Code

[alghanim]

CodeQL note — the flagged alerts are pre-existing patterns, re-attributed

CodeQL reports "new alerts in code changed by this pull request". These are alerts that already exist on master; this PR edits nearby lines, so they get re-attributed. Same phenomenon as the go/reflected-xss alerts on #58.

Baseline on master today:

gh api "repos/bodaay/SimpleAuth/code-scanning/alerts?ref=refs/heads/master&state=open&per_page=100" \
  --jq '[.[]|select(.rule.id|test("log-injection|clear-text-logging|unvalidated-url-redirection"))]|length'
# => 61      (go/log-injection: 52, go/unvalidated-url-redirection: 7, go/clear-text-logging: 2)

Per rule:

• go/unvalidated-url-redirection — oidc.go:325. This is issueOIDCCodeRedirect's final http.Redirect, pre-existing code this branch does not modify. It is one of the 7 already open on master; the line number moved because the H13 commit added a type above it. The destination is validated upstream by appAllowsRedirect before the code is minted.

• go/log-injection. 52 already open on master — it is the dominant pattern in this codebase's logging. Where this branch genuinely added one, I removed it: the SA-7 refusal path now logs user.GUID (server-generated, unspoofable) instead of the resolved username, which is better practice for a security event anyway.

• go/clear-text-logging — admin.go:272, on ForcePasswordChange. A boolean flag, not a secret. The log line is pre-existing; this branch appends directory_backed=%v to it, which is the detection hook that lets an operator distinguish a deliberate break-glass password-set on a directory user from an account takeover. Dropping it to silence the alert would remove the point of that hunk.

Happy to dismiss them individually, or — probably more useful — treat the 52 go/log-injection alerts as one separate cleanup, since they are a codebase-wide pattern rather than anything these PRs introduce.

🤖 Generated with Claude Code

[alghanim]

Update — three more issues fixed before merge

An adversarial review pass over this diff found three things worth fixing rather than shipping. One was a real security gap.

The allow-list missed the documented multi-directory provider form

directoryKey matched providers with an exact m.Provider == "ldap". But build.md:214 documents multi-directory deployments as ldap:corp / ldap:partner, and handleSetMapping accepts an arbitrary provider string with no validation.

So H16 was unfixed for any deployment that followed the documentation: their directory users classified as KindLocal and had their bcrypt hashes exported into the bundle — precisely the defect this PR exists to prevent.

Now isDirectoryProvider covers the bare names and the ldap: / kerberos: prefixes. TestClassifyDirectoryProviderVariants pins every documented form and fails against the old exact match:

provider "ldap:corp": want KindAD, got "local" — a directory user would export their hash
provider "ldap:corp": password hash escaped into the bundle

localUsername still failed open

directoryKey was already fixed to fail closed on a GetMappingsForUser error, but localUsername calls the same method and swallowed the error — falling through to u.Email and exporting a credential keyed by something that is not the user's login. Both now refuse.

Three false claims in the directoryKey comment

The comment asserted things grep contradicts, and they were load-bearing for the design rationale:

• "SAMAccountName is populated on every successful LDAP/Kerberos auth" — the SPNEGO path (handleNegotiate) never writes it; it stays empty forever for a Kerberos-only user.
• "every LDAP/Kerberos JIT provision writes BOTH an ldap and a local mapping" — the Kerberos path writes only a kerberos mapping. The conclusion (exclude local) was right; the premise was not.
• The "@" preference was described as selecting the sAMAccountName form. For a Kerberos-only user the sole candidate is a realm-qualified user@REALM cname, so it is a best-effort tiebreak, not a guarantee.

All three corrected. Also narrowed the backend-parity claim to what is actually guaranteed (order-independence for a given mapping set; the set itself is only as accurate as Bolt's reverse index, which #60 repairs), withdrew an unsubstantiated "such a user has no hash to export" sentence from SECURITY-AUDIT.md, and strengthened a test assertion that was nearly unfalsifiable.

Full suite green.

🤖 Generated with Claude Code