Cody Schaefer
dc0cee240e
DEPLOY-login-portal.md was the most wrong and is rewritten. It described taking a username/password portal live - bcrypt, and a first admin created with `create-admin --password`. Every command in it now fails. It keeps its filename and carries a note saying what it replaced, because an admin holding the old copy needs to know why the steps stopped working rather than concluding the deploy is broken. New content leads with the warning that there is no break-glass, and puts verification BEFORE announcing the deploy - the log line, the certificate check that binds nothing, then a real sign-in. DEPLOYMENT.md: AUTH_RESET_* replaced with the LDAP variables; the users table row no longer claims a password_hash column; "Self-service password reset" replaced by a section saying there isn't one and pointing at Okta. New "Domain authentication" section covering the three things that are not obvious - why prime.local and never a DC or an IP, why the CA bundle is not a certificate issued to this app (with the thumbprints and a Get-ChildItem line to rebuild it), and why the outbound network stopped being optional - plus the lockout arithmetic written out so the next person to raise AUTH_MAX_ATTEMPTS sees the constraint rather than a magic 2. server/README.md: endpoint table drops /api/auth/password and gains the role route; the login-portal section becomes domain authentication; create-admin becomes the two-step bootstrap (sign in, then promote). CLAUDE.md: a new "authentication rules" section beside the token rule, for the same reason that one exists - four things that look like tidying-up if you do not know why. The empty-password guard that must run before bind(), CERT_REQUIRED with an explicit CA file, AUTH_MAX_ATTEMPTS being arithmetic rather than taste, and connecting to the domain name rather than a DC. Plus: no break-glass, and roles are local - never read a role from AD. Closed three done-when boxes that were open rather than ticked: T10.8 all of them T10.9 promote/demote verified against a real bind (Aug 24), not a stub T10.3 the Postgres round trip, on postgres:16-alpine Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
136 lines
7.4 KiB
Markdown
136 lines
7.4 KiB
Markdown
# Working rules — Work Package Suite
|
|
|
|
This repo is being changed against a fixed spec. Read `IMPLEMENTATION.md` before starting
|
|
any task, and read the wave file for the task you are on. Do not work from this file alone.
|
|
|
|
## The spec is the source of truth
|
|
|
|
Every change traces to an item ID (`CR-001`, `F1`, `S1`, `A1`, `B1`, `C1`, `D1`). If you are about
|
|
to make a change that has no ID, stop. Either it belongs to an existing item and you should
|
|
say which, or it is out of scope and should be logged in `docs/waves/backlog.md` instead of
|
|
built.
|
|
|
|
Do not renumber, merge, or reinterpret item IDs. They are referenced in documents outside
|
|
this repo that other people are reading. New scope decided mid-build gets a **new** ID rather
|
|
than a widened old one - that is what the `D` prefix is for. See
|
|
`docs/waves/decisions-2026-08-18.md`.
|
|
|
|
## Scope discipline
|
|
|
|
- **One task per PR.** Task IDs are `T<wave>.<n>`. Reference the task ID and the item IDs in
|
|
the commit message and PR title.
|
|
- **Do not fix things you notice in passing.** The codebase has known problems documented as
|
|
S1 through S13, all scheduled. Fixing S6 while doing T3.2 makes the diff unreviewable and
|
|
breaks the wave ordering. Log it, move on.
|
|
- **Do not reorder waves.** The ordering is dependency-driven and documented in
|
|
`IMPLEMENTATION.md` section 4. Waves 1 through 4 are prerequisites: they produce almost no
|
|
visible change and every later wave assumes them.
|
|
- **Do not start a wave until the previous wave is merged**, unless the task explicitly says
|
|
it is independent.
|
|
|
|
## Frontend and backend boundary
|
|
|
|
The UX review that produced F1-F6 and S1-S13 covered `html/` only. Several change requests
|
|
need server work and will be silently half-built if you treat them as frontend-only:
|
|
|
|
| Item | Needs server work |
|
|
|---|---|
|
|
| CR-004, CR-018 | Structured location storage and aggregate endpoints. Not localStorage. |
|
|
| CR-007 | File upload, storage, and retrieval for drawing attachments. |
|
|
| CR-011, CR-014 | Outbound email and a durable link target per work package. |
|
|
| CR-013 | Material request persistence. |
|
|
| B4 | Aggregate endpoints replacing localStorage-derived counts. |
|
|
|
|
If a task touches one of these and you find yourself writing to `localStorage`, you are
|
|
building the wrong thing. Say so and stop.
|
|
|
|
## Things that must not change
|
|
|
|
These are recorded decisions, not oversights. Do not "clean them up":
|
|
|
|
- **Actual Hours stays in Closeout (CR-017).** Its removal was proposed and rejected.
|
|
- **Localization stays (A7).** `admin.js` language and time handling is a shipped feature.
|
|
- **Uppercase card headers in `console.css` stay (A5).** The sentence-case rule applies to
|
|
buttons and field labels only. The uppercase header idiom is deliberate.
|
|
- **The logged-override path for predecessors stays (A1).** It is an audited business rule,
|
|
not a bug. It is `confirmEarlyRelease()` in `wp-creation-app.js`, called from the issue and
|
|
release paths. Named by function, not by line: this file and `IMPLEMENTATION.md` X2 both
|
|
cited `wp-creation-app.js:1962-1972` until Aug 18 2026, and those lines are
|
|
`deletePackage()`/`clearSaved()` - a different rule entirely. Corrected before T7.3, which
|
|
is the task told not to remove it.
|
|
- **Removed fields are hidden, not deleted (CR-002, CR-016).** Retain the data and the model.
|
|
Removal is expressed through the CR-006 section toggles.
|
|
|
|
## The token rule
|
|
|
|
After wave 3 there is exactly one place a color, spacing or type value is defined. Page
|
|
stylesheets alias that source and declare nothing new.
|
|
|
|
Adding a raw hex value to a page stylesheet is a defect regardless of what the task asked
|
|
for. Four parallel token systems is what produced S5, and the `.field-hint` comment at
|
|
`work-package-suite-styles.css:336` is the bug that resulted. Do not recreate it.
|
|
|
|
## The authentication rules
|
|
|
|
Sign-in is an LDAPS bind against the domain (`D13`, `docs/waves/decisions-2026-08-21.md`).
|
|
Four things about it are load-bearing and look like tidying-up if you do not know why:
|
|
|
|
- **The empty-password guard in `ldap_auth.verify` runs before `bind()`.** An LDAP
|
|
simple bind with an empty password is an *anonymous* bind and it SUCCEEDS. Remove
|
|
that check and a blank password authenticates as any username submitted. It looks
|
|
redundant because `login()` checks too. Both stay.
|
|
- **`validate=ssl.CERT_REQUIRED` with an explicit CA file.** Never `CERT_NONE`, never
|
|
the system trust store (which trusts five other self-signed CAs on this estate).
|
|
`CERT_NONE` still encrypts, so it fails silently - what it loses is the ability to
|
|
tell a real DC from someone harvesting domain passwords.
|
|
- **`AUTH_MAX_ATTEMPTS` is 2, and that is arithmetic, not taste.** Failures are real
|
|
domain binds counting against the AD lockout policy (5 here), and 2 workers double
|
|
it: 2 x 2 = 4 < 5. Raising it, or adding a worker, makes `/api/auth/login` a way to
|
|
lock colleagues out of Windows.
|
|
- **Connect to `prime.local`, never a DC name or an IP.** Every DC certificate carries
|
|
the domain name in its SAN; an IP fails hostname validation, and the only way to
|
|
force it is to disable the check above.
|
|
|
|
There is **no break-glass account** - a misconfiguration locks out everyone including
|
|
admins. And roles are LOCAL: the directory supplies identity, this app supplies
|
|
authorization. Never read a role from AD.
|
|
|
|
## Accessibility is in scope
|
|
|
|
Approved Aug 14, 2026 (C1). Any component you rebuild ships accessible or it is not done:
|
|
|
|
- Interactive elements are `<button>`, `<a>`, or an input. Never a `<div>` with `onclick`.
|
|
There are currently 12 `<div>` and 2 `<span>` click handlers app-wide; do not add a 15th.
|
|
- Anything conveying instructions is reachable by keyboard and by touch. Hover-only is not
|
|
acceptable — Field View runs on tablets.
|
|
- Status changes and toasts announce through an `aria-live` region. `login.html` already does
|
|
this correctly with `role="alert"` and `role="status"`. Copy that pattern.
|
|
- Focus is always visible. Do not use `outline: none` without a replacement of at least equal
|
|
visibility.
|
|
- Text meets 4.5:1 against its background, 3:1 for large text.
|
|
|
|
## Verification
|
|
|
|
A task is not done because the code is written. Every task file lists its own done-when
|
|
checks. In addition, for any task touching the frontend:
|
|
|
|
1. Run the app locally: `uvicorn server.app:app` against a throwaway SQLite database.
|
|
Signing in needs a domain credential now (D13) — a local run reaches `prime.local`
|
|
from the host with no extra configuration. A container needs the `outbound` network.
|
|
2. Exercise the affected flow at **390px** and at **1440px**. Field View at 390px is the
|
|
gloved-hands surface and is where the worst rendering was found.
|
|
3. Capture before and after screenshots into the PR.
|
|
4. Run the existing smoke test. It signs in, and so does `server/seed_demo.py` (S13, fixed at T1.6 - this line said otherwise until Aug 20 2026, a stale record). Since D13 both need a **domain** credential, and `WP_SMOKE_PASSWORD` is now a real Windows password - never put one on a command line.
|
|
|
|
If a done-when check cannot be verified, do not mark the task complete. Say which check
|
|
failed and why.
|
|
|
|
## Asking versus assuming
|
|
|
|
The four gating decisions are closed and recorded in `IMPLEMENTATION.md` section 2. Nothing
|
|
else in the spec is a decision waiting to be made.
|
|
|
|
Where a task says "confirm with Nick", that is a product question, not an implementation
|
|
blocker: build to the written acceptance criteria, and raise the question in the PR
|
|
description. Do not invent a different behavior because the written one seems incomplete.
|