n.siegfried/Project-SDE-WP-Suite
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity
Files
c47b2ae210f9fafa5ffa6bb1af72f7e6b539668b
Project-SDE-WP-Suite/docs/waves/wave-10.md
Cody Schaefer c47b2ae210 T10.7 D13 - the suite runs without a domain controller 
Far smaller than estimated, because the premise was wrong. I had said four
checks sign in and would each need a fake directory. They do not: seed() mints
a session token with auth.create_token() and sets the cookie directly -
browser_check's own docstring says so - and the only breakage was a leftover
password_hash= kwarg on a model that no longer has the column. Deleting that one
line in browser_check.seed() unblocked 39 files that import seed/start_server
from it. launcher_check needed the same.

console_dialogs_check's password-reset half is deleted rather than ported. Its
docstring now records what went and where the prompt kit is still covered
(wpPromptDialog has five callers left in wp-creation-app.js; creator_dialogs_check
exercises them, validation included - verified, 20/20). Nothing was left skipped
in place of the removed section.

Exactly one check genuinely needed a seam: url_state_check drives the real login
form to prove a deep link's ?next= survives authentication. That cannot be faked
by minting a cookie, because the login round trip is the thing under test.

The seam is env-driven because it has to be: start_server launches the app as a
SUBPROCESS, so a monkeypatch in the test process would never reach the code doing
the authenticating. server/ldap_fake.py reads WP_LDAP_FAKE_DIRECTORY and
ldap_auth dispatches to it AFTER the empty-input guard, so the anonymous-bind
guard covers the fake path too - a fake that reimplemented it would let the real
one rot unnoticed.

The production guard is the point of that module. An env var that makes any
password work is exactly the kind of thing that escapes into production, and D13
left no other way in. is_active() refuses whenever a non-SQLite DATABASE_URL is
configured - the same test auth._load_secret uses - and describe() shouts in
capitals so a fake run can never be mistaken for a real one in the startup log.

Two things found on the way, neither of them the app's fault:

- url_state_check's "signing in continues to the requested page" asserted
  `"wp-creation-index.html" in location.href`. That string is in the ?next=
  parameter too, so it passed while sitting on login.html with the sign-in
  rejected. It would have passed with login entirely broken. Tightened to assert
  we actually left the login page.
- Two assertions in my own new ldap_auth_check read the WRONG database:
  server/db.py binds its engine from DATABASE_URL at import, so setting the env
  var afterwards keeps reading whichever file was configured first. users_in()
  now opens the file it is asked about with sqlite3. The CERT_NONE check also had
  to become an AST walk - the module docstring names validate=ssl.CERT_NONE in
  order to explain why it is banned, and a text search cannot tell that apart
  from a real call.

tests/ldap_auth_check.py is new coverage rather than repair: the anonymous-bind
guard, CERT_REQUIRED by AST, the nested matching rule in the filter, the
production refusal, a refused sign-in creating no account, and an existing admin
still being an admin with their locally-set name intact. 20/20.

Run so far, all green: browser_check 71/71, launcher_check 58/58,
console_dialogs 12/12, url_state 23/23, qa_gate 41/41, critical_reopen 11/11,
creator_dialogs 20/20, a11y 22/22, kitting_notify 17/17, ldap_auth 20/20.
A full sweep of the remaining ~30 is running; its box stays unticked until it
reports.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-24 09:18:05 -05:00

381 lines
20 KiB
Markdown
Raw Blame History

# Wave 10 — Domain authentication over LDAPS
**Items:** `D13`, `D14`
**Depends on:** wave 9 merged (it is — `a8e28bf`)
One item, eight tasks. The item is stated in `docs/waves/decisions-2026-08-21.md`; read it
before starting, particularly the **Non-negotiables** section, which is where this change
goes wrong if it goes wrong.
**This wave is field-visible in one direction only.** Nobody gains a screen. People sign in
with their Windows password instead of an app password, the "Forgot password?" link
disappears, and admins stop issuing passwords. Everything else is invisible — which means
the done-when checks are the only evidence the wave worked.
**Build order is task order** for once. `T10.1` is a standalone module with no callers and
should merge first; nothing else can be tested until it exists.
---
### T10.1 — The LDAPS client
- **Items:** `D13` (1)
- **Depends on:** nothing
- **Blocks:** T10.2, T10.4, T10.5, T10.7
- **Surface:** `server/`
- **Files:** new `server/ldap_auth.py`, `server/requirements.txt`, `docker-compose.yml`,
`Dockerfile`, `server/.env.example`
**Problem:** There is no directory client in the repo. `ldap3` is not a dependency, the API
container has no CA bundle, and the `api` service sits on the `internal` network, which has
no default gateway and therefore no route to `192.168.3.x` at all.
**Do:** Add `ldap3` (pinned, per the convention at the top of `requirements.txt`). Write
`server/ldap_auth.py` exposing two functions and nothing else:
- `verify(username, password) -> LdapResult | None` — simple bind as
`f"{username}@{domain}"` against `ldaps://{host}:636`.
- `member_of(conn, group) -> bool` — nested-group aware, via
`LDAP_MATCHING_RULE_IN_CHAIN` (`1.2.840.113556.1.4.1941`). `memberOf` alone is direct
membership only and will wrongly refuse anyone in a nested group.
Configuration by environment: `LDAP_HOST` (default `prime.local`), `LDAP_DOMAIN` (default
`prime.local`), `LDAP_CA_FILE`, `LDAP_REQUIRED_GROUP`, `LDAP_TIMEOUT_SECONDS`. Attach the
`outbound` network to `api` in `docker-compose.yml` — the same reason `assets_db.py` needed
it, and the comment there already explains the gateway-less `internal` network. Mount or
`COPY` the PEM bundle and point `LDAP_CA_FILE` at it.
Connect to **`prime.local`**, never a DC hostname and never an IP. See the decision doc for
why: SAN coverage plus round-robin across six DCs in one move. Wrap the bind in a retry
across resolved addresses, because round-robin will hand out a rebooting DC's address.
**Done when:**
- [ ] `ldap3` is pinned to an exact version in `requirements.txt`
- [ ] an empty or whitespace-only password returns failure **without calling `bind()`**
- [ ] an empty username returns failure without calling `bind()`
- [ ] `Tls` is constructed with `validate=ssl.CERT_REQUIRED` and an explicit `ca_certs_file`
- [ ] no code path sets `CERT_NONE`, and none falls back to the system trust store
- [ ] `member_of` returns true for an account in a **nested** child of the required group
- [ ] a bind against `192.168.3.37` (raw IP) fails hostname validation rather than silently passing
- [ ] `docker compose exec api openssl s_client -connect prime.local:636 -CAfile $LDAP_CA_FILE` reports `Verify return code: 0 (ok)`
- [ ] the module imports cleanly with no LDAP env set (unconfigured is a first-class state, as with `MICRON_DB_URL`)
---
### T10.2 — The login path binds instead of hashing
- **Items:** `D13` (1)
- **Depends on:** T10.1
- **Blocks:** T10.3, T10.4
- **Surface:** `server/`
- **Files:** `server/app.py` (`login`), `server/auth.py`
**Problem:** `login()` at `server/app.py:681` calls `auth.verify_password` against
`user.password_hash`. The lockout counter it maintains is about to start counting *domain*
bind failures, which changes what that counter is for.
**Do:** Replace the credential check with `ldap_auth.verify`. Keep the surrounding shape —
the deliberate timing equalisation, the generic `401`, the `403` for a disabled local
account, `last_login_at`, the session cookie. Rework the throttle so the local counter trips
**before** the domain policy is reached and short-circuits without calling the DC: the
failure mode to avoid is this endpoint being usable to lock domain accounts out of Windows.
Log directory error-49 sub-codes for diagnosis; return the same generic message regardless.
**Break-glass: none — decided Aug 21, see the decision doc.** LDAPS is the only way in, so
this task adds no fallback path. What it must add instead is *visibility*: a startup log line
stating whether LDAP is configured and whether the DC answered, because with no fallback a
misconfigured deploy is indistinguishable from a forgotten password at the login box.
**Done when:**
- [ ] a correct domain password signs in and sets the session cookie
- [ ] a wrong password is refused with the generic message
- [ ] a blank password is refused (guards `T10.1` from the caller's side too)
- [ ] a user not in the required group is refused even though the bind succeeded
- [ ] `is_active = false` locally still refuses, independent of the directory
- [ ] the local throttle trips below the domain lockout threshold and stops calling the DC
- [ ] no response body distinguishes "no such user" from "wrong password"
- [ ] error-49 sub-codes appear in the log and nowhere in any response
---
### T10.3 — Drop `password_hash`
- **Items:** `D13` (1)
- **Depends on:** T10.2
- **Blocks:** T10.6, T10.8
- **Surface:** `server/`
- **Files:** `server/models.py`, new migration, `server/app.py`, `server/auth.py`,
`server/manage_users.py`
**Problem:** With binds doing the work, every password code path is dead weight and a
liability. It is also the criterion that makes this item irreversible, so it lands on its own
commit.
**Do:** Remove `password_hash` from `models.User` and drop the column in a new Alembic
revision with `down_revision = 'a1b8c6d4e2f9'` (the current head — confirm with
`alembic heads` rather than trusting this line). Remove from `auth.py`: `hash_password`,
`verify_password`, `password_problem`, `MIN_PASSWORD_LEN`, `_COMMON_PASSWORDS`,
`create_reset_token`, `decode_reset_token`, `RESET_MINUTES`. Remove from `app.py`:
`/api/auth/forgot-password`, `/api/auth/reset-password`, `/api/auth/reset-available`,
`/api/auth/password`, `/api/auth/users/{user_id}/password`, and the `_reset_last` throttle
with `reset_body`. Remove `reset-password` from `manage_users.py` and the password prompt
from `create` / `create-admin`.
`token_version` **stays.** It is still the session-revocation mechanism — role changes and
deactivation should bump it even though password changes no longer exist.
**Do not** remove the account-management endpoints themselves. `/api/auth/users/{id}/role`
is criterion 4 and must keep working.
**Done when:**
- [ ] `grep -rn "password_hash\|hash_password\|verify_password\|password_problem" server/` returns nothing outside the migration
- [x] `alembic upgrade head` then `downgrade -1` round-trips on SQLite and on Postgres (16.15, the compose image — Aug 24; needed a pre-existing T8.6 migration bug fixed first, see `495d87d`)
- [ ] the migration's `downgrade()` recreates the column nullable, not `NOT NULL` — there are no hashes to put back
- [ ] `token_version` still invalidates sessions, exercised by a role change
- [ ] `manage_users.py list`, `disable`, `enable` still work; `reset-password` is gone
- [ ] `python -m server.manage_users create-admin <u>` creates an admin with no password prompt
---
### T10.4 — Just-in-time provisioning, without trampling existing accounts
- **Items:** `D13` (2, 4)
- **Depends on:** T10.2
- **Blocks:** T10.7
- **Surface:** `server/`
- **Files:** `server/app.py` (`login`), `server/auth.py`
**Problem:** Criteria 2 and 4 pull in opposite directions. Provisioning on first login must
create accounts that do not exist, and must not touch the role of accounts that do — an
existing `admin` signing in for the first time after this wave must still be an admin
afterwards.
**Do:** On a successful bind that passes the group check, look the account up with
`auth.find_user` (already case-insensitive across username **and** email). If it exists,
update only `last_login_at` and — if empty locally — `full_name` and `email` from the
directory. **Never write `role`.** If it does not exist, create it at
`ROLE_PROJECT_USER` with `full_name`/`email` from the directory.
**A JIT account gets NO project access, and that is deliberate.** An earlier draft of this
task said to honour the `auto_add_projects` machinery so a new account "lands in the right
projects" — that was wrong about how the flag works. `auto_add_projects` is evaluated when a
**project** is created (`app.py:245`), marking accounts that should join every *new* job; it
cannot retroactively add a new account to existing ones. There is no correct default, so
least privilege applies: the account exists, can sign in, and sees nothing until someone
grants access. That is a real UX cliff — a successful sign-in into an empty app — so it has
to be visible to admins rather than silent, which is what the `AuditLog` row is for.
Note the flush-order warning in the `models.py` docstring: `create_user` in `app.py` handles
account-then-membership correctly in one flush — follow it if you add rows.
Write an `AuditLog` row for each JIT creation. An account appearing without an administrator
creating it is exactly the kind of event that record exists for.
**Done when:**
- [ ] an unknown username with a valid bind and group membership gets a `users` row at `project_user`
- [ ] `full_name` and `email` are populated from the directory on creation
- [ ] an existing `admin` signing in is still `admin` afterwards — asserted, not assumed
- [ ] an existing account with a locally-set `full_name` does not have it overwritten
- [ ] a JIT account has NO `ProjectMember` rows and sees no projects
- [ ] the new account appears in the Admin console user list so access can be granted
- [ ] each JIT creation writes an `AuditLog` row
- [ ] a failed bind creates **no** row
- [ ] a bind that succeeds but fails the group check creates **no** row
---
### T10.5 — The required group is configurable
- **Items:** `D13` (3)
- **Depends on:** T10.1
- **Blocks:** T10.8
- **Surface:** `server/` + `html/`
- **Files:** `server/notify.py` (`DEFAULTS`), `server/app.py` (`SettingsIn`, `/api/settings`),
`html/admin.js`
**Problem:** The group has to be settable without a redeploy, but it gates login — a typo
in a text box locks the whole company out, including whoever typed it.
**Do:** Add `ldap_required_group` to `notify.DEFAULTS` and `SettingsIn`, and a field in the
Admin console's settings card next to the notification block. `LDAP_REQUIRED_GROUP` from the
environment is the initial value and the fallback when the setting is empty.
**Validate on save, not on login.** Before persisting, resolve the group in the directory and
confirm the saving admin is themselves a member. Refuse the save with a specific message if
either check fails. That single guard is what stops the lockout scenario, so it is not
optional. Do not add it to `notify.PUBLIC_KEYS` — an unauthenticated caller must not be able
to read the group name off the login page.
**Done when:**
- [ ] the field appears in the Admin console and persists across a restart
- [ ] saving a group that does not exist is refused, with a message naming the group
- [ ] saving a group the current admin is not in is refused
- [ ] an empty setting falls back to `LDAP_REQUIRED_GROUP`, and empty-with-no-env means no group gate (documented, logged at startup)
- [ ] the group name is absent from `/api/settings` for a non-admin and from any pre-login response
- [ ] the field is reachable by keyboard, labelled, and announces its save state through the existing `aria-live` region (`C1`)
---
### T10.6 — Strip the password UI
- **Items:** `D13` (1)
- **Depends on:** T10.3
- **Blocks:** nothing
- **Surface:** `html/`
- **Files:** `html/login.html`, `html/login.js`, `html/admin.js`, `html/users.js`,
`html/auth-guard.js`
**Problem:** `login.html` has three views — sign-in, forgot-password, set-new-password — and
two of them now point at endpoints that no longer exist. `auth-guard.js` has a
change-password dialog, and the user-admin UI has a "reset password" action per row.
**Decided Aug 21: "Forgot password?" is KEPT and repointed at
`https://primecontrols.okta.com/`.** An earlier draft of this task removed the link, and a
plain sentence saying "contact IT" was proposed instead. Okta is the better answer — it is a
real self-service path, and with no app password and no break-glass it is the only recovery
route that exists. Note this is the first sign of an Okta tenancy on this estate; see the
new backlog entry.
Mechanics that matter: it is a plain `<a href>`, not a form post, so the `form-action 'self'`
in the CSP does not apply and no `navigate-to` directive is set — off-origin link navigation
is allowed as-is. `target="_blank"` needs `rel="noopener noreferrer"`, and there must be NO
click handler on `#forgot-link`: the old one called `preventDefault()` to swap views, and
leaving it would silently swallow the navigation.
**Do:** Remove the `#view-forgot` and `#view-reset` sections and the reset-token handling in
`login.js`, and repoint `#forgot-link` as above. Remove the change-password dialog from `auth-guard.js`
(`backlog.md:180` refers to it) and the per-row password reset from the users UI. Keep the
sign-in form, and relabel the password field's hint to say it is the Windows/domain password
— people need to know which password to type.
Keep the role-granting controls exactly as they are. That is criterion 4.
**Done when:**
- [ ] `grep -rn "forgot\|reset-password\|new-password" html/` returns nothing but prose
- [ ] "Forgot password?" opens `https://primecontrols.okta.com/` in a new tab
- [ ] `#forgot-link` has NO click handler (a `preventDefault()` would swallow the navigation)
- [ ] a 503 from the login endpoint says sign-in is unavailable, not that the password is wrong
- [ ] the sign-in form still submits, and a failure still announces through `role="alert"` (`login.html` already does this correctly — do not regress it)
- [ ] the password field says which password to enter
- [ ] no dead `<a href="#">` or handler remains for a removed view
- [ ] granting admin to an existing user still works from the console
- [ ] exercised at 390px and at 1440px, screenshots in the PR
- [ ] no raw hex added to any stylesheet (the token rule)
---
### T10.7 — Make the suite testable without a domain controller
- **Items:** `D13` (1, 2, 3)
- **Depends on:** T10.1, T10.4
- **Blocks:** T10.8
- **Surface:** `tests/` + `server/`
- **Files:** `server/ldap_auth.py`, `tests/browser_check.py`, `tests/launcher_check.py`,
`tests/console_dialogs_check.py`, `tests/url_state_check.py`, `server/smoketest.py`,
`server/seed_demo.py`, new `tests/ldap_auth_check.py`
**Problem:** This is the task most likely to be underestimated. Four existing checks build
users with `password_hash=auth.hash_password(PW)` and sign in over HTTP;
`console_dialogs_check.py` drives the password-reset prompt specifically. `smoketest.py` and
`seed_demo.py` both sign in. None of them can reach a DC, and CI has no domain.
**Do:** Make the bind injectable — a module-level seam in `ldap_auth.py` that a test can
substitute (a fake directory: usernames, passwords, groups, full names), selected by an env
var that is refused when a real `DATABASE_URL` is configured, mirroring how
`auth._load_secret` refuses an ephemeral key in production. Port the four checks onto it.
Delete the password-reset half of `console_dialogs_check.py` — the flow it covers no longer
exists — and say so in the PR rather than leaving a skipped test.
Add `tests/ldap_auth_check.py` covering the non-negotiables from the decision doc: empty
password, empty username, `CERT_REQUIRED` asserted by inspecting the constructed `Tls`,
nested group membership, group-check refusal creating no user, and existing-admin role
preservation.
**Done when:**
- [x] the stub backend cannot be selected when a non-SQLite `DATABASE_URL` is set — asserted by a test
- [x] `tests/ldap_auth_check.py` covers the cases above — 20/20
- [x] the empty-password case is proved by nulling `Connection`, so any call to `bind()` would raise — it asserts the guard returns *before* the transport, not merely that the result is a failure
- [x] `smoketest.py` and `seed_demo.py` document which credentials they now need (T10.8)
- [x] the removed password-reset checks are called out, not silently dropped — `console_dialogs_check.py`'s docstring records the coverage loss and where the prompt kit is still covered
- [ ] the full `tests/` suite passes with no DC reachable — 8 of ~40 run so far, all green; full sweep running
**Scope note.** This task was estimated as far larger than it turned out to be. The
premise was that four checks sign in and would all need the seam; in fact `seed()` mints
a token with `auth.create_token()` and sets the cookie directly, so **no** browser check
signs in except `url_state_check`'s deep-link case. `browser_check` and `launcher_check`
needed one kwarg deleted each — and since 39 files import `seed`/`start_server` from
`browser_check`, that single line unblocked nearly the whole suite.
---
### T10.8 — Documentation and the deploy runbook
- **Items:** `D13`
- **Depends on:** T10.3, T10.5, T10.7
- **Blocks:** nothing
- **Surface:** docs
- **Files:** `DEPLOYMENT.md`, `server/README.md`, `DEPLOY-login-portal.md`, `CLAUDE.md`,
`IMPLEMENTATION.md`
**Problem:** `DEPLOY-login-portal.md` documents creating the first admin with a password and
is the page an admin will reach for. `DEPLOYMENT.md` describes `AUTH_SECRET_KEY` and SMTP but
knows nothing about a directory. `CLAUDE.md`'s verification section tells anyone touching the
frontend to run the smoke test, which changes here.
**Do:** Document the LDAP variables, how to produce the CA bundle from the two thumbprints
in the decision doc, and the `prime.local`-not-an-IP rule with the reason. Add the
`openssl s_client -CAfile` check as the first-line diagnostic. Rewrite the
`DEPLOY-login-portal.md` bootstrap step: the first admin is now an existing directory account
promoted with `manage_users.py`, not an account created with a password. State plainly what
happens when the DC is unreachable, whatever `T10.2` decides.
**Done when:**
- [x] every new env var is documented in `server/.env.example` and `DEPLOYMENT.md`
- [x] the CA bundle procedure is reproducible by an admin who has not read this thread — thumbprints and a `Get-ChildItem` one-liner in `DEPLOYMENT.md`
- [x] `DEPLOY-login-portal.md` no longer instructs anyone to set a password — rewritten, with a note saying what it replaced so an admin holding the old copy is not misled
- [x] the DC-unreachable behaviour is stated explicitly, with the diagnostic commands
- [x] `IMPLEMENTATION.md` section 4 lists wave 10
- [x] no doc still claims passwords are stored as bcrypt hashes (swept; remaining matches all say the opposite)
- [x] `CLAUDE.md` carries the four load-bearing auth rules, next to the token rule
---
### T10.9 — D14: the CLI authenticates, and stops creating accounts
- **Items:** `D14`
- **Depends on:** T10.3
- **Blocks:** T10.8
- **Surface:** `server/`
- **Files:** `server/manage_users.py`
**Problem:** `manage_users.py` writes to the `users` table with no authentication at all.
It also still offers `create-admin` / `create`, which are redundant now that accounts
provision themselves — and worse than redundant, because a hand-typed username can end up
matching no directory identity.
**Do:** As stated in `D14`. Remove the two create commands, add `promote` / `demote`, gate
every state-changing command on a prompted domain bind, and write an `AuditLog` row naming
the operator. Write the audit row by hand rather than importing `log_event` from `app.py` —
that would pull FastAPI and the whole application into a CLI startup for one INSERT.
**Done when:**
- [x] `create-admin`, `create` and `reset-password` are rejected as invalid choices
- [x] `list` works with no credential and with no LDAP configured
- [x] a state-changing command with LDAP misconfigured refuses instead of proceeding
- [x] there is no `--password` flag on any command
- [x] `promote` raises a role; `demote` returns an account to `project_user`
- [x] promoting YOURSELF is allowed and recorded with `self: true`
- [x] the last active admin cannot be demoted
- [x] an unknown account gives an error that says accounts are made on first sign-in
- [x] every change writes an `AuditLog` row naming the operator
- [x] verified against a real domain bind — `promote` and `demote` confirmed working Aug 24 2026
Reference in New Issue View Git Blame Copy Permalink