Create adr-004-external-auth-user-provisioning-policy.md

[ejaronne]

https://github.com/mitre/heimdall2/blob/external_auth_provisioning_policy/docs/adr-004-external-auth-user-provisioning-policy.md

[ejaronne]

REGISTRATION_DISABLED=false, true, local, SSO
track down all samples, helm charts, of environment variables to update behavior descriptions
PR name - update_REGISTRATION_DISABLED_behavior_breaking_change
Eugene - update ADR
Amndeep/Will - set up cards and initial LOE
Nick, Sean, Olivia - review cards and feedback on LOE and any problems seen

[aaronlippold]

ADR-004 Review — verification, industry comparison, and recommended revisions

Reviewed against the heimdall2 source (617ea4388). Overall: sound decision, right placement (policy in AuthnService, not per-strategy), honest alternatives. The recommendations below strengthen it into a long-term design — several review questions in §11 can be answered definitively from the code.

---

1. Verification findings (against actual source)

ADR claim	Verified
"Current behavior" snippet for validateOrCreateUser	✅ Matches authn.service.ts exactly
REGISTRATION_DISABLED does not gate the external-auth path	✅ Confirmed
Proposed config helper pattern	✅ isLocalLoginAllowed/isRegistrationAllowed both use ?.toLowerCase() !== 'true' — identical
§9.3 risk / §11 Q2: "LDAP may need a separate review if its login path does not call validateOrCreateUser"	🔑 LDAP already calls validateOrCreateUser (ldap.strategy.ts:78), same as all five OAuth strategies

The LDAP finding settles review questions 1 and 2 together: the gate covers LDAP automatically — and therefore the variable name SSO_AUTO_CREATE_USERS_DISABLED is a misnomer that will surprise LDAP-only deployments. The ADR's own glossary has the right term: EXTERNAL_AUTH_*.

2. Industry comparison

Product	Design	Notes
GitLab	signup_enabled (local) + allow_single_sign_on (per-provider array, defaults deny) + block_auto_created_users (defaults true: JIT users created but blocked pending admin approval)	The approval-queue middle state is absent from this ADR's alternatives
Grafana	Per-provider allow_sign_up under each [auth.*] section	This is the ADR's deferred Option C, shipped as a major product's primary design
SonarQube	Provisioning mode: JIT (default) vs automatic/SCIM — mutually exclusive states, one switch	Mode enum, not boolean accretion
Harbor	oidc_auto_onboard boolean	Closest to this ADR's proposal

Every product surveyed agrees with this ADR's core call: local registration and external-auth JIT are separate controls (rejecting Option A is industry-consistent). The gap: GitLab and SonarQube show two patterns this ADR doesn't consider — the approval queue and the mode enum.

Sources: GitLab OmniAuth · Grafana auth · SonarQube provisioning · Harbor OIDC

3. Recommended design changes

3a. Mode enum instead of boolean — GitLab's lesson is that booleans accrete into a config maze. One enum absorbs the future without migration:

EXTERNAL_AUTH_USER_PROVISIONING=jit | approval | disabled    # default: jit

• jit — current behavior (backwards-compatible default; existing deployments upgrade with zero change)
• disabled — this ADR's goal: unknown external identities rejected
• approval — reserved for phase 2 (GitLab's block_auto_created_users pattern): account created but blocked pending admin approval. Ships later as one switch case, not a new flag.

Fail-fast on unknown values at startup. Note the current boolean pattern silently treats a typo (ture) as permissive — a security-policy variable should refuse to start on garbage, not default open.

3b. State the choke point as an architectural invariant. All six strategies (google, github, gitlab, okta, oidc, ldap) funnel through validateOrCreateUser — that is the real standardization mechanism. Add to the ADR: "All external-auth strategies MUST provision through AuthnService; any future strategy (e.g., SAML) inherits this policy by construction." Suggested shape:

async provisionExternalUser(identity: ExternalIdentity): Promise<User> {
  const existing = await this.findByEmail(identity.email);
  if (existing) return this.refreshMetadata(existing, identity);

  switch (this.config.externalAuthProvisioning()) {   // exhaustive
    case 'jit':      return this.createActiveUser(identity);
    case 'approval': return this.createPendingUser(identity);  // phase 2
    case 'disabled': throw new AccountNotProvisionedError();
  }
}

3c. Promote audit logging from review question (§11 Q3) to in-scope AC. §1.3's justification is "auditors expect account creation to be traceable" — the rejection event IS the audit artifact this feature exists to produce. Structured events at the choke point: external_auth.user.jit_created, external_auth.login.rejected_not_provisioned, (later) user.pending_approval / user.approved.

3d. Specify the frontend error contract now (§11 Q4, §9.3 risk). UnauthorizedException inside an OAuth callback (a redirect flow) surfaces as a generic failure unless handled. Carry a structured code on the redirect (?error=account_not_provisioned) and render remediation on the login page.

3e. Add the misconfiguration advisory — app + docs + defaults. The ADR's own §9.2 negative ("admins can set REGISTRATION_DISABLED=true but forget the SSO flag") has a mitigation:

• Boot: log a coherence warning when REGISTRATION_DISABLED=true while external auth is configured with jit.
• Admin UI: the same check feeds a small reusable "configuration advisories" mechanism — computed at boot, served on an authenticated admin endpoint (note: the existing /server settings payload is fetched pre-auth and must not carry config posture), rendered as a dismissible post-login notification + persistent admin-panel entry with the exact variable, current effective behavior, and the options. This is GitLab's admin-area-warnings pattern, and it is the upgrade/transition path: admins discover the new knob in-app rather than in release notes.
• Public login modal: user remediation ONLY, never config posture — advertising "this instance auto-creates accounts" pre-auth is reconnaissance for attackers.

4. UX sketches

Login modal — rejected (disabled mode), via redirect error code:

┌────────────────────────────────────────────────────────┐
│  ⚠  Sign-in not completed                              │
│     Your Okta identity was verified, but no Heimdall   │
│     account exists for you. Contact your Heimdall      │
│     administrator to request an account.               │
├────────────────────────────────────────────────────────┤
│   Email    [                    ]                      │
│   Password [                    ]        [ LOGIN ]     │
│  ───────────────── or sign in with ─────────────────   │
│        [ Okta ] [ GitHub ] [ Google ] [ LDAP ]         │
└────────────────────────────────────────────────────────┘

Admin panel — persistent configuration advisory (post-login, dismissible):

┌─ Admin ▸ Configuration Advisories ───────────────────────────────┐
│  ⚠ External auth auto-creates users while registration disabled │
│  Your settings:  REGISTRATION_DISABLED=true                      │
│                  EXTERNAL_AUTH_USER_PROVISIONING=(unset → jit)   │
│  Effective: identities from Okta/GitHub/Google/LDAP still get    │
│  Heimdall accounts automatically on first login.                 │
│                                                                  │
│  To change, set EXTERNAL_AUTH_USER_PROVISIONING to:              │
│    jit       auto-create on first login         (current)        │
│    approval  create, block until you approve     ◀ recommended   │
│    disabled  reject sign-ins with no account                     │
│  [ VIEW DOCS ]                                     [ DISMISS ]   │
└──────────────────────────────────────────────────────────────────┘

Admin ▸ Users — approval queue (phase 2 payoff: default-deny with one-click approvals, no manual email matching):

│  ● PENDING APPROVAL (2)                                          │
│  │ jdoe@agency.gov   Jane Doe   via okta   2h ago                │
│  │                             [ APPROVE ]  [ DENY ]             │

5. Migration story this enables

1. Release N: enum ships, default jit — zero behavior change for every existing deployment; the advisory makes the previously-silent behavior visible in-app.
2. Docs: one table mapping the three controls (LOCAL_LOGIN_DISABLED, REGISTRATION_DISABLED, EXTERNAL_AUTH_USER_PROVISIONING) to the three account paths.
3. Phase 2: approval mode + pending-user state + admin queue — a switch case, not a redesign.
4. Future (optional): if the default ever moves toward deny, the advisory channel announces it a release ahead — standard two-release deprecation, already built.

6. Direct answers to §11 review questions

1. Name: EXTERNAL_AUTH_USER_PROVISIONING (enum) — or at minimum EXTERNAL_AUTH_AUTO_CREATE_USERS_DISABLED; "SSO" is wrong because LDAP is provably covered.
2. LDAP: already covered — ldap.strategy.ts:78 calls validateOrCreateUser. Rewrite §9.3 as a settled statement.
3. Audit logging: yes, in scope — it's the artifact §1.3 promises.
4. Frontend: specific message via structured redirect error code; never a generic failure for an intentional policy rejection.

Smaller items

• Behavior matrix: add the top operational failure row — pre-provisioned user whose local email doesn't exactly match the IdP email, policy on → rejected (§8.2 warns about it; the matrix is where operators will debug it).
• §6.2: one sentence on email-binding risk — an IdP allowing user-editable/unverified emails can bind to someone else's pre-provisioned account.
• Header: Author: TBD needs filling; branch name in the header will be stale after a rename.

[aaronlippold]

Re-review of the revised ADR — endorsed, with answers to §12 and a set of edits

The REGISTRATION_DISABLED enum rework is the better design: one variable, four policies, and true finally means what admins always assumed it meant — it repairs the original expectation gap at the root instead of adding a knob beside it. Fail-closed breakage is the right direction to break. Endorsed.

Answers to the §12 review questions, plus edits being applied directly to the ADR in a follow-up commit:

Q1 — unknown values: fail fast. The enum makes permissive-on-unknown more dangerous than before: REGISTRATION_DISABLED=locel (typo) silently opens both account-creation paths — in a variable whose purpose is deny. A security-policy variable should refuse to start on garbage, not default open. (Telling detail: the §7.1 test table has no unknown-value rows.) Edit: startup validation rejects anything outside false|true|local|sso (empty/unset = false), with tests.

Q2 — yes, it's a breaking change, and release notes are the weakest channel. The cohort that breaks (true + relying on SSO JIT) can be targeted precisely at upgrade time: boot check — REGISTRATION_DISABLED=true AND any external auth strategy enabled → one WARN: "As of vX.Y, true also disables SSO/LDAP auto-account creation. Set REGISTRATION_DISABLED=local for the previous behavior." Edit: new implementation subsection + sp:1 work-order phase, and the release/migration note promoted from "should we?" to a required deliverable.

Q3 — split it. Admin user creation while LOCAL_LOGIN_DISABLED=true is a separate concern from registration policy; keeping it in this ADR's scope would tangle two policies. Left as the one open question.

Also restored: the frontend rendering contract. The revision keeps the account_not_provisioned error code (good) but dropped the original Q4 — how the login page renders it. Without a rendering spec, an intentional policy rejection surfaces as a generic auth failure, which §9.3 itself lists as the risk. Edit: short subsection specifying the login-page alert (identity verified / no account / contact admin), carried as a redirect query param consistent with existing OAuth failure handling.

Two hygiene edits:

• §4 gains Option E (GitLab-style approval queue: auto-created-but-blocked-pending-approval), rejected for this ADR with the structural note that "approval" is not a kind of "disabled" and can never be a value of this enum — if ever wanted it's a separate future mechanism. Recorded so nobody invents REGISTRATION_DISABLED=sso-approval later.
• The §2 value table and §5 behavior matrix get explicit LDAP callouts — the glossary covers it, but the value table is what operators actually read.

Edits incoming on this branch.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[aaronlippold]

Deep review + research pass applied — seven commits on this branch

Following up on my earlier endorsement: I ran a full verification pass over the revised ADR (every finding independently re-checked against the code before being applied), plus a targeted research pass with every citation verified against primary sources. Net result: the enum design held up across all of it — nothing overturned the core decision. What changed is precision, evidence, and scope-honesty. Highlights worth your eyes:

The two things that were blocking

1. §3.6's error transport didn't exist. OAuth failures actually reach the login page via the authenticationError cookie set by AuthenticationExceptionFilter (rendered as a generic snackbar) — there is no query-parameter channel. The contract is respecced onto real transports: a second authenticationErrorCode cookie for OAuth callbacks, and the 401 JSON body for the LDAP/local POST flows (which have no exception filter — under the old spec an LDAP user would have gotten exactly the generic failure §9.3 warns about).
2. The 401 body shape is now verified, not assumed (@nestjs/common@11.1.28 + current docs): an object passed to UnauthorizedException replaces the entire body, so the snippets include statusCode explicitly. Exact wire body is pinned in §3.5.

Other substantive additions

• Second breaking cohort acknowledged (§6.2): values like 1/yes/typos run silently permissive today and will refuse to boot after fail-fast lands — plus an upgrade-ordering/rollback rule (local/sso are fully permissive on pre-upgrade code).
• §8.2 runbook conflict: LOCAL_LOGIN_DISABLED=true blocks POST /users for admins too — pre-provision before disabling local login.
• Email handling: findByEmail is case-sensitive with zero normalization anywhere — the ADR now requires trim+lowercase at the choke point (mixed-case IdP emails would otherwise break pre-provisioning). Multi-valued LDAP mail and missing-email behavior specified.
• sso's guarantee stated precisely (§2/§6.3): it blocks silent JIT, not self-provisioning — with registration open and no email verification, register-then-SSO-bind works (this is the Classic-Federated-Merge class from the USENIX 2022 pre-hijacking paper; mitigation carded as open question 12.4).
• NIST mapping (§6.1): AC-2/IA-4/800-63C-4 §4.6.3 — deployers can cite the ADR in ATO docs.
• LDAP strategy await fix, filter log redaction, pinned audit-event shape, §7.5 filter tests + §7.6 frontend tests, and standards/precedent citations throughout (PostgreSQL/Django for the fail-fast pattern, GitLab/Grafana/Mattermost for the error-code UX).

ADR-005 + work tracking

• ADR-005 (docs/adr-005-vitepress-documentation-site.md): VitePress docs site on the vulcan pattern — because the wiki can't be PR'd, which is exactly the Phase 9 problem. Wiki remains the channel until it lands.
• Beads epics ready: heimdall2-4qg (this ADR, 10 cards = the 10 phases) and heimdall2-yvx (ADR-005, 6 cards) — full card templates with ACs, TDD starting points, and verification commands. @Amndeep7 / Will — this should map straight onto the card/LOE task from the kickoff comment.
• Board setup: install bd (https://github.com/gastownhall/beads), then bd dolt pull from a heimdall2 checkout (fresh machine: bd bootstrap). Note: the board schema migrated v49→v54 today — if you have an existing beads clone, bd dolt pull on your current bd binary before upgrading bd. Team workflow skills: https://github.com/mitre/mitre-saf-skills. Details are in both ADRs' work-order sections.

Eugene — the seven commits are individually scoped if you want to review pass-by-pass; happy to walk through any finding. If anything reads as over-reach, flag it and we'll cut it back.