diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 0190e16..296fe49 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -146,20 +146,37 @@ docker compose exec db psql -U wpsuite -d wpsuite -c "select id, name from proje ### Automated smoke test -`server/smoketest.py` exercises the whole stack end-to-end (health → project → -SOP → Work Package → the AWP issue gate → status → metrics → comments → cascade -cleanup). Stdlib only — no pip/jq. +`server/smoketest.py` exercises the whole stack end-to-end (health → sign-in → +project → SOP → Work Package → the AWP issue gate → status → metrics → comments → +archive round trip → cascade cleanup → sign-out). Stdlib only — no pip/jq. + +It **signs in first**, because every `/api/` route except `/api/health` requires a +session. Credentials come from the environment so a password stays out of shell +history, and the account must be an **admin**: the run creates a project and deletes +it again, and archiving or deleting one takes Project Admin on it. The script checks +the signed-in role up front and warns if it is too low rather than letting you find +out in the cleanup step. ```bash +export WP_SMOKE_USER= +export WP_SMOKE_PASSWORD='…' + # Through the proxy (use --insecure for a self-signed internal cert): python3 server/smoketest.py https://wp-suite.company.local --insecure -# Or from inside the api container (hits FastAPI directly): -docker compose exec api python /app/server/smoketest.py http://localhost:8000 +# Or from inside the api container (hits FastAPI directly). Pass the vars through: +docker compose exec -e WP_SMOKE_USER -e WP_SMOKE_PASSWORD api \ + python /app/server/smoketest.py http://localhost:8000 # Add --keep to leave a demo project in the DB so you can open it in the UI. +# --user / --password override the environment if you'd rather be explicit. ``` +Exit codes: **0** all checks passed · **1** one or more checks failed · **2** the run +could not start (host unreachable, or credentials missing or rejected). The last is +kept separate on purpose — "I could not test this" is a different answer from "this is +broken", and automation should not treat them alike. + Exit code 0 and "ALL PASS" means the API, the Python logic, and SQL are all working. It cleans up after itself (the test project and its SOP/WPs are deleted via cascade); a single tagged test comment remains (there's no comment diff --git a/KNOWN-ISSUES.md b/KNOWN-ISSUES.md index ede8208..b0545e1 100644 --- a/KNOWN-ISSUES.md +++ b/KNOWN-ISSUES.md @@ -12,8 +12,7 @@ Close an entry by deleting it in the same commit that fixes it. |---|-------|----------|--------|--------| | 1 | XSS via SOP discipline names in the WP creator | Medium (internal), High if externally reachable | 2026-08-05 | Open | | 2 | Archived projects: the two big apps don't grey out their own controls | Low | 2026-08-05 | Open | -| 3 | `server/smoketest.py` cannot authenticate — every run fails with 401 | Medium | 2026-08-05 | Open | -| 4 | User Directory and nav drawer have not been run in a browser | Low (verification gap, not a known defect) | 2026-08-05 | Open | +| 3 | User Directory and nav drawer have not been run in a browser | Low (verification gap, not a known defect) | 2026-08-05 | Open | --- @@ -166,72 +165,7 @@ app bar — and therefore the banner — is deliberately skipped. --- -## 3. `server/smoketest.py` cannot authenticate — every run fails with 401 - -**Files:** `server/smoketest.py` · documented in `DEPLOYMENT.md` §"Automated smoke -test" (line ~147) -**Predates:** the login portal. The script was written against an open API and was -never updated when authentication landed. - -### What is wrong - -The script has no login step — no call to `/api/auth/login`, no cookie jar, no -credential arguments. `auth_gate` (`server/app.py`) refuses every `/api/` route -without a session cookie, so every check after the first fails: - -``` -- create project (status=401) -- fetch project by id (status=401) -... -Result: FAIL -``` - -Only `/api/health` passes, because it is on `auth._EXEMPT_EXACT`. - -### What it costs - -**The documented end-to-end verification path does not work, and hasn't for some -time.** `DEPLOYMENT.md` presents this as the way to prove "NGINX → FastAPI → -PostgreSQL all work", including a `docker compose exec` invocation for use inside the -api container. Anyone following it after a deploy gets a wall of 401s and has to work -out for themselves whether the stack is broken or the script is. - -The stack itself is fine, and there is a working equivalent: the **Admin Console → -End-to-end smoke test** card runs the same sequence from the browser, where the -session cookie already exists. `html/admin.html` describes it as mirroring -`smoketest.py`, which is now the only place that sequence actually runs. - -Rated Medium rather than Low because it is a verification tool that reports failure -on a healthy system — the failure mode most likely to be believed and acted on. - -### Why it is still open - -It was found while testing unrelated work (the User Directory) and is not a defect in -the product. Fixing it means choosing how the script gets credentials, which is a -small design decision about a deploy-time tool rather than a code fix, and it should -not ride along inside a feature branch. - -### What closing it takes - -Small — under an hour, stdlib only, matching the script's existing constraint. - -1. Add `--user` / `--password` arguments, defaulting to `WP_SMOKE_USER` / - `WP_SMOKE_PASSWORD` from the environment so the runbook does not put a password on - a command line. -2. Install an `http.cookiejar.CookieJar` on the opener in `call()`, then POST - `/api/auth/login` before the first check and assert it returned 200. There is a - working reference for both steps in the throwaway harness written for this branch - (`Client` in the scratch `http_check.py`), or in `html/admin.js`'s in-browser - version of the same flow. -3. Note in `DEPLOYMENT.md` that the account needs access to the project the script - creates — simplest is an admin account, since a `project_user` cannot create a - project. -4. Decide whether a missing credential is a hard failure or a skip with a clear - message. A silent 401 wall is what caused this entry. - ---- - -## 4. User Directory and nav drawer have not been run in a browser +## 3. User Directory and nav drawer have not been run in a browser **Files:** `html/users.html`, `html/users.js`, `html/wp-sidenav.js`, `html/wp-sidenav.css`, `html/console.css` diff --git a/server/app.py b/server/app.py index f336fa0..3b56662 100644 --- a/server/app.py +++ b/server/app.py @@ -187,7 +187,19 @@ def require_project_writable(db: Session, user_or_none, project_id: Optional[str if not project_id: return proj = db.get(models.Project, project_id) - if proj is not None and proj.archived_at is not None: + if proj is None: + # The project this write targets is gone — usually an outbox op queued before + # someone deleted the job. Refusing here is what keeps it a clean 409 instead + # of a foreign-key violation surfacing as a 500: the row could never be + # inserted anyway now that both engines enforce their FKs (see db.py). 409 + # also matters because project-data.js retires a 4xx op and would retry a 5xx + # forever, so this is the difference between one quiet failure and a loop. + raise HTTPException( + status_code=409, + detail=f"{what} — this project no longer exists. It was deleted, so there is " + f"nothing to save it against.", + ) + if proj.archived_at is not None: raise HTTPException( status_code=409, detail=(f"{what} — this project is archived (read-only). An administrator " @@ -959,6 +971,14 @@ def create_user(body: NewUserIn, actor: models.User = Depends(require_user_manag project_role=body.project_role.strip()[:120], ) db.add(u) + # Flush the account before adding memberships that point at it. The ORM decides + # flush order from relationship() declarations, and models.py deliberately has + # none (plain columns + ForeignKey), so it will happily emit the project_members + # INSERT before the users one — which the database then rejects. Without this the + # whole call fails with a foreign-key violation on any engine that actually + # enforces them, which is every engine we run: Postgres always, and SQLite since + # db.py started setting `PRAGMA foreign_keys=ON`. + db.flush() log_event(db, actor, "user_created", "user", u.id, summary=u.username, detail={"role": u.role, "project_role": u.project_role, "projects": len(valid)}) diff --git a/server/db.py b/server/db.py index a47c283..5e1f602 100644 --- a/server/db.py +++ b/server/db.py @@ -12,7 +12,7 @@ Connection precedence: The schema is identical either way (SQLAlchemy handles dialect differences). """ import os -from sqlalchemy import create_engine, URL +from sqlalchemy import create_engine, event, URL from sqlalchemy.orm import sessionmaker, DeclarativeBase # Load a local .env if present (dev convenience). @@ -46,6 +46,25 @@ _is_sqlite = isinstance(DATABASE_URL, str) and DATABASE_URL.startswith("sqlite") connect_args = {"check_same_thread": False} if _is_sqlite else {} engine = create_engine(DATABASE_URL, connect_args=connect_args, pool_pre_ping=True, future=True) + +if _is_sqlite: + # SQLite ships with foreign keys DISABLED and the pragma is per-connection, so + # without this every `ondelete="CASCADE"` in models.py is silently a no-op on a + # dev database while working correctly on Postgres. That divergence is worse than + # it sounds: deleting a project left its SOPs, work packages and membership rows + # behind as orphans pointing at an id that no longer exists, and deleting a user + # left their project_members rows — and the smoke test's cascade assertion failed + # on dev while passing in production, which is the exact failure that makes a + # smoke test worth ignoring. + # + # Registered on the engine, not a session, because the pragma has to be set on + # each new DBAPI connection as the pool creates it. + @event.listens_for(engine, "connect") + def _sqlite_enforce_foreign_keys(dbapi_connection, _record): + cur = dbapi_connection.cursor() + cur.execute("PRAGMA foreign_keys=ON") + cur.close() + SessionLocal = sessionmaker(bind=engine, autoflush=False, autocommit=False, future=True) diff --git a/server/models.py b/server/models.py index 4f262e2..3050cfb 100644 --- a/server/models.py +++ b/server/models.py @@ -9,6 +9,21 @@ The full client document for a SOP or WP is kept verbatim in a JSON `data` column, with the most-queried fields promoted to real columns for listing and filtering. IDs are short strings (client- or server-generated) so the browser can upsert without round-tripping a sequence. + +NO relationship() DECLARATIONS, ON PURPOSE — and one consequence to know about. +Every link here is a plain column plus a ForeignKey; nothing is navigable as +`project.work_packages`. Queries are explicit selects, which suits an API that +mostly reads one scoped list at a time and never wants a lazy load firing inside +a response. + +The consequence: SQLAlchemy's unit of work derives FLUSH ORDER from relationships, +not from ForeignKey metadata. With none declared it has no dependency edge to +follow, so if you add a parent and its child in the SAME flush it may emit the +child's INSERT first and the database will reject it. Both engines enforce foreign +keys (Postgres always; SQLite since db.py sets `PRAGMA foreign_keys=ON`), so this +is a real error, not a dev-only quirk. Call `db.flush()` after adding the parent — +see `create_user` in app.py, which creates an account and its ProjectMember rows +together. """ from datetime import datetime, timezone from typing import Optional diff --git a/server/smoketest.py b/server/smoketest.py index c4425af..ea29323 100644 --- a/server/smoketest.py +++ b/server/smoketest.py @@ -5,6 +5,24 @@ Exercises the real HTTP endpoints the way the front end does, proving that NGINX → FastAPI → PostgreSQL all work and that the Python logic (the AWP release gate, metrics, cascade delete) behaves. Stdlib only — no pip, no jq. +AUTHENTICATION + Every /api/ route except /api/health requires a session (auth_gate in + server/app.py), so the script signs in first and keeps the session cookie for + the rest of the run. Credentials come from the environment by preference, so a + password never has to appear in a command line or shell history: + + export WP_SMOKE_USER=smoketest + export WP_SMOKE_PASSWORD='…' + python3 server/smoketest.py https://wp-suite.company.local + + …or pass --user / --password explicitly. + + Use an ADMIN account. The script creates a project and deletes it again at the + end, and deleting one takes Project Admin on that project (require_project_admin); + a plain project_user can create a project but not clean it up. The script checks + the signed-in role up front and warns if it is too low, rather than letting you + discover it in the cleanup step. + USAGE # Against the deployed site (through the NGINX proxy): python3 server/smoketest.py https://wp-suite.company.local @@ -13,16 +31,24 @@ USAGE python3 server/smoketest.py https://wp-suite.company.local --insecure # From inside the api container (hits FastAPI directly): - docker compose exec api python /app/server/smoketest.py http://localhost:8000 + docker compose exec -e WP_SMOKE_USER -e WP_SMOKE_PASSWORD api \ + python /app/server/smoketest.py http://localhost:8000 # Leave the demo project in the database so you can open it in the UI: python3 server/smoketest.py https://wp-suite.company.local --keep The base URL is the SITE root (no /api). Default: http://localhost:8000 -Exit code 0 = all checks passed, 1 = one or more failed. + +Exit codes: 0 = all checks passed · 1 = one or more checks failed · 2 = the run +could not start (unreachable host, missing or rejected credentials). 2 is kept +distinct on purpose: "I could not test this" is not the same answer as "this is +broken", and conflating them is what made an unauthenticated version of this +script report a wall of failures against a perfectly healthy stack. """ import argparse +import http.cookiejar import json +import os import ssl import sys import urllib.error @@ -40,6 +66,18 @@ def check(name, cond, detail=""): BASE = "" CTX = None +# One opener for the whole run, carrying the cookie jar that holds the session +# issued by /api/auth/login. urlopen() has no cookie support, which is why the +# session used to be dropped on the floor and every data route answered 401. +OPENER = None + + +def build_opener(ctx=None): + handlers = [urllib.request.HTTPCookieProcessor(http.cookiejar.CookieJar())] + if ctx is not None: + handlers.append(urllib.request.HTTPSHandler(context=ctx)) + return urllib.request.build_opener(*handlers) + def call(method, path, body=None): """Returns (status_code, parsed_body). Never raises on HTTP status.""" @@ -50,7 +88,7 @@ def call(method, path, body=None): headers={"Content-Type": "application/json", "Accept": "application/json"}, ) try: - with urllib.request.urlopen(req, context=CTX, timeout=20) as r: + with OPENER.open(req, timeout=20) as r: raw = r.read().decode(); status = r.status except urllib.error.HTTPError as e: raw = e.read().decode(); status = e.code @@ -61,33 +99,95 @@ def call(method, path, body=None): return status, parsed +def abort(msg, hint=""): + """Could not run — distinct from 'ran and found problems'. See exit codes above.""" + print(_c("\nABORT", "31") + " " + msg) + if hint: + print(hint) + print() + return 2 + + def main(): - global BASE, CTX + global BASE, CTX, OPENER ap = argparse.ArgumentParser(description="Work Package Suite API smoke test") ap.add_argument("base_url", nargs="?", default="http://localhost:8000", help="Site root, no /api (default: http://localhost:8000)") ap.add_argument("--insecure", action="store_true", help="skip TLS verification") ap.add_argument("--keep", action="store_true", help="keep the demo project (don't delete)") + ap.add_argument("--user", default=os.getenv("WP_SMOKE_USER", ""), + help="account to sign in as (default: $WP_SMOKE_USER). Use an admin account.") + ap.add_argument("--password", default=os.getenv("WP_SMOKE_PASSWORD", ""), + help="its password (default: $WP_SMOKE_PASSWORD — preferred, " + "so it stays out of shell history)") args = ap.parse_args() BASE = args.base_url.rstrip("/") if args.insecure: CTX = ssl.create_default_context(); CTX.check_hostname = False; CTX.verify_mode = ssl.CERT_NONE + OPENER = build_opener(CTX) print(f"\nWork Package Suite — API smoke test\nTarget: {BASE}\n") + # Refuse to start without credentials rather than running headlong into 401s. + if not args.user or not args.password: + missing = " and ".join( + n for n, v in (("WP_SMOKE_USER", args.user), ("WP_SMOKE_PASSWORD", args.password)) if not v) + return abort( + f"no credentials — {missing} not set.", + " Every /api/ route except /api/health needs a session, so there is nothing\n" + " meaningful to test without one. Set them and re-run:\n\n" + " export WP_SMOKE_USER=\n" + " export WP_SMOKE_PASSWORD='…'\n\n" + " Or pass --user/--password. Use an admin account: the run creates a project\n" + " and deletes it again, and the delete needs Project Admin on it.") + project_id = None + # Guards the sign-out in `finally`. Without it an ABORT on a rejected login still + # ran the logout checks, which "passed" — a session that never existed is trivially + # refused after logout — and printed PASS lines underneath an abort message. + logged_in = False try: - # 1) Health — API is up and reachable through the proxy. + # 1) Health — API is up and reachable through the proxy. Exempt from auth, + # so this also isolates "host unreachable" from "credentials rejected". try: st, body = call("GET", "/api/health") except urllib.error.URLError as e: - print(_c("\nABORT", "31") + f" cannot reach {BASE}/api/health — {e}\n" - " Is the stack up (docker compose ps) and the URL correct?\n") - return 1 + return abort(f"cannot reach {BASE}/api/health — {e}", + " Is the stack up (docker compose ps) and the URL correct?") check("health endpoint returns ok", st == 200 and isinstance(body, dict) and body.get("ok") is True, f"status={st} body={body}") - # 2) Create a project (writes to the projects table). + # 2) Sign in. The cookie the response sets is held by OPENER's jar and rides + # every request after this one. + st, body = call("POST", "/api/auth/login", + {"username": args.user, "password": args.password}) + if st != 200: + detail = body.get("detail") if isinstance(body, dict) else body + hint = (" The account may be locked: the API locks an account for a while after\n" + " a few consecutive failures (AUTH_MAX_ATTEMPTS / AUTH_LOCKOUT_MINUTES),\n" + " so re-running with the wrong password makes this worse, not better.\n" + " Check the password, then wait out the lockout window." + if st in (401, 403, 423, 429) else + " Unexpected status from the login endpoint — check the API logs.") + return abort(f"could not sign in as '{args.user}' (HTTP {st}): {detail}", hint) + logged_in = True + check("login issues a session", st == 200) + + # 3) Prove the session actually travels — this is the check whose absence let + # an unauthenticated version of this script look like a broken stack. + st, me = call("GET", "/api/auth/me") + who = (me or {}).get("user", {}) if isinstance(me, dict) else {} + check("session is accepted on an authenticated route", + st == 200 and who.get("username", "").lower() == args.user.lower(), + f"status={st} body={me}") + role = who.get("role", "?") + print(f" ..... signed in as {who.get('username', args.user)} (role: {role})") + if role not in ("admin", "project_super_user", "project_admin"): + print(_c(" NOTE", "33") + f" '{role}' cannot archive or delete a project, so the " + "archive checks and the\n cleanup step will fail and a stray test project " + "will be left behind.\n Re-run with an admin account for a clean pass.") + + # 4) Create a project (writes to the projects table). st, proj = call("POST", "/api/projects", { "name": "ZZ Smoke Test Project", "number": "SMOKE-001", "client": "Internal QA", "division": "Controls", "site": "Test Host", @@ -96,14 +196,14 @@ def main(): project_id = proj.get("id") if isinstance(proj, dict) else None check("create project", st == 200 and bool(project_id), f"status={st}") - # 3) Read it back + confirm it's in the list (SQL round-trip). + # 5) Read it back + confirm it's in the list (SQL round-trip). st, got = call("GET", f"/api/projects/{project_id}") check("fetch project by id", st == 200 and got.get("number") == "SMOKE-001", f"status={st}") st, lst = call("GET", "/api/projects") check("project appears in list", st == 200 and any(p.get("id") == project_id for p in lst), f"status={st} count={len(lst) if isinstance(lst, list) else '?'}") - # 4) Create a SOP linked to the project. + # 6) Create a SOP linked to the project. st, sop = call("POST", "/api/sops", { "project_id": project_id, "name": "ZZ Smoke SOP", "number": "SMOKE-001", "complete": True, "created_by": "smoketest", @@ -116,7 +216,7 @@ def main(): st, latest = call("GET", f"/api/sops/latest?project_id={project_id}") check("latest SOP for project resolves", st == 200 and latest.get("id") == sop_id, f"status={st}") - # 5) Create a Work Package with one OPEN constraint (not release-ready). + # 7) Create a Work Package with one OPEN constraint (not release-ready). st, wp = call("POST", "/api/wps", { "project_id": project_id, "sop_id": sop_id, "number": "WP01-SMOKE", "subject": "Smoke test package", "type": "Conduit Install", @@ -128,11 +228,11 @@ def main(): wp_id = wp.get("id") if isinstance(wp, dict) else None check("create work package", st == 200 and bool(wp_id), f"status={st}") - # 6) The AWP release gate: issuing with an open constraint must be REFUSED (409). + # 8) The AWP release gate: issuing with an open constraint must be REFUSED (409). st, refused = call("POST", f"/api/wps/{wp_id}/issue") check("issue is blocked while a constraint is open (409)", st == 409, f"status={st} body={refused}") - # 7) Clear the constraint (upsert), then issue must SUCCEED (200, status Issued). + # 9) Clear the constraint (upsert), then issue must SUCCEED (200, status Issued). call("POST", "/api/wps", { "id": wp_id, "project_id": project_id, "sop_id": sop_id, "number": "WP01-SMOKE", "subject": "Smoke test package", "type": "Conduit Install", @@ -146,16 +246,16 @@ def main(): f"status={st}") check("issued_at timestamp is set", isinstance(issued, dict) and bool(issued.get("issued_at"))) - # 8) Status transition endpoint. + # 10) Status transition endpoint. st, prog = call("POST", f"/api/wps/{wp_id}/status", {"status": "In Progress"}) check("status transition endpoint", st == 200 and prog.get("status") == "In Progress", f"status={st}") - # 9) Metrics aggregate for the project (Python aggregation over SQL rows). + # 11) Metrics aggregate for the project (Python aggregation over SQL rows). st, m = call("GET", f"/api/wps/metrics?project_id={project_id}") check("metrics endpoint aggregates", st == 200 and isinstance(m, dict) and m.get("total", 0) >= 1, f"status={st} metrics={m}") - # 10) Comment / feedback write + read. + # 12) Comment / feedback write + read. st, c = call("POST", "/api/feedback", { "type": "wp_review_comment", "name": "smoketest", "wp_id": wp_id, "text": "SMOKE TEST comment — safe to delete", "page": "/smoketest"}) @@ -164,11 +264,11 @@ def main(): check("comment is queryable", st == 200 and any("SMOKE TEST" in (x.get("text") or "") for x in comments), f"status={st}") - # 11) WPs filter by project. + # 13) WPs filter by project. st, wps = call("GET", f"/api/wps?project_id={project_id}") check("list WPs by project", st == 200 and any(w.get("id") == wp_id for w in wps), f"status={st}") - # 12) Archiving a project: it leaves the default list, stays reachable with + # 14) Archiving a project: it leaves the default list, stays reachable with # archived=all, and freezes read-only — then unarchiving restores all three. # The freeze is the whole point of the feature, so it is asserted, not assumed. st, arch = call("POST", f"/api/projects/{project_id}/archive", {"archived": True}) @@ -193,7 +293,7 @@ def main(): check("writing succeeds again once unarchived", st == 200, f"status={st}") finally: - # 13) Cleanup — deleting the project cascades to its SOPs and WPs (FK ON DELETE CASCADE). + # 15) Cleanup — deleting the project cascades to its SOPs and WPs (FK ON DELETE CASCADE). if project_id and not args.keep: st, _ = call("DELETE", f"/api/projects/{project_id}") check("delete project (cascades SOP + WPs)", st == 200, f"status={st}") @@ -203,6 +303,15 @@ def main(): elif project_id and args.keep: print(f"\n --keep: left demo project {project_id} ('ZZ Smoke Test Project') in the database.") + # 16) Sign out. Exercises the logout endpoint, and means a run does not end + # holding a live session — which matters when this is run from a shared + # jump host or a CI worker. Only if we got one: see `logged_in`. + if logged_in: + st, _ = call("POST", "/api/auth/logout") + check("logout clears the session", st == 200, f"status={st}") + st, _ = call("GET", "/api/auth/me") + check("session is refused after logout (401)", st == 401, f"status={st}") + # ── summary ──────────────────────────────────────────────────────────────── total = len(_PASS) + len(_FAIL) print(f"\n{'-'*52}\n{len(_PASS)}/{total} checks passed.")