From c5540ce6da6955a1e5424b59e79634c86f583b5a Mon Sep 17 00:00:00 2001 From: Cody Schaefer Date: Fri, 21 Aug 2026 15:40:56 -0500 Subject: [PATCH] T10.9 D14 - the CLI authenticates against the domain; create-admin/create removed Accounts are not created here any more. D13 provisions them on first successful sign-in, so create-admin and create were redundant - and worse than redundant, because a hand-typed username can end up matching no directory identity at all. Removing them means every row now originates from a bind, which closes that class of problem for everything except the rows the old CLI already made. promote and demote replace them. Bootstrapping the first admin is now two steps in order: sign in once, which provisions the account at project_user, then promote your own sAMAccountName. Every state-changing command requires a prompted domain bind. No --password flag on anything, deliberately: that would put a live domain password into shell history and into ps output for every other user on the box. `list` needs no credential so an outage stays diagnosable. Two deliberate divergences from the API, both commented at the code: - The bind does NOT apply the login group gate. If a mistyped required group locks everyone out of the console, this tool must still work, or the only route to fixing the lockout is the thing the lockout prevents. - Changing your OWN role is permitted. set_user_role in app.py forbids it to stop an admin locking themselves out of the console; here it is the entire bootstrap path. Allowed, and recorded with {"self": true}. Kept from set_user_role: the last-admin guard, and clearing auto_add_projects on promotion to admin (an admin already reaches every project, so the flag would sit there invisible and spring back on demotion). What this is worth, said plainly in the module docstring rather than implied: anyone with a shell here can still write to the users table with psql or sqlite3, so the bind is defence in depth and mostly ACCOUNTABILITY. Before this, every role change from a shell was invisible in AuditLog while the same change through the console was recorded. Now both are recorded and both name a person. Any-domain-user was accepted as sufficient knowing that. Verified: the three removed commands are rejected as invalid choices; list runs with no credential; a state-changing command with LDAP misconfigured refuses rather than proceeding unauthenticated; promote, demote, self-promotion, the last-admin guard, the unknown-account message, and one audit row per change all behave, with the bind stubbed. Left open rather than ticked: none of this has been run against a real bind. authenticate_operator was stubbed for the logic tests. Co-Authored-By: Claude Opus 5 (1M context) --- docs/waves/decisions-2026-08-21.md | 53 +++++++ docs/waves/wave-10.md | 35 ++++- server/manage_users.py | 233 +++++++++++++++++++++-------- 3 files changed, 254 insertions(+), 67 deletions(-) diff --git a/docs/waves/decisions-2026-08-21.md b/docs/waves/decisions-2026-08-21.md index fcd2624..59d5545 100644 --- a/docs/waves/decisions-2026-08-21.md +++ b/docs/waves/decisions-2026-08-21.md @@ -155,3 +155,56 @@ Two consequences worth knowing: - Group-to-role mapping (e.g. an AD group that confers `project_admin`). Criterion 4 keeps authorization local on purpose. Worth its own item later; logged in `backlog.md`. - Replacing the Let's Encrypt certificate or changing anything on the OpenResty host. + +--- + +## D14 — The CLI authenticates against the domain, and stops creating accounts + +- **Amends:** `D13` criterion 4, which said role granting keeps working *from the Admin + console*. It said nothing about `manage_users.py`, which had no authentication of any + kind. Requiring one is a new requirement, so it gets its own id rather than widening + criterion 4. +- **Surface:** `server/manage_users.py` +- **Task:** `T10.9` + +### The decision + +1. **`create-admin` and `create` are removed.** `D13` provisions accounts on first + successful sign-in, so creating them by hand is redundant. Removing them also closes a + class of problem: every row now originates from a bind, so a username cannot be typed + in wrong and end up orphaned from the directory identity it was meant to match. That + risk now applies only to rows the old CLI already created. +2. **`promote` and `demote` replace them.** The directory supplies identity; this app + supplies authorization, and this is where authorization is assigned from a shell. +3. **Every state-changing command requires a domain bind.** Prompted, via `getpass`. + There is deliberately no `--password` flag: that would put a live domain password into + shell history and into `ps` output for every other user on the box. +4. **`list` needs no credential**, so an outage stays diagnosable. + +### Bootstrapping the first admin, which changed shape + +Two steps, in order: **sign in once** (which provisions the account at `project_user`), +then **`promote `**. Before D14 the first admin was created with a +password; there is no password now, and no account to create. + +### What this is worth, stated plainly + +Anyone with a shell on the api container can still write to the `users` table directly +with `psql` or `sqlite3`. So the bind is **defence in depth and, mostly, +ACCOUNTABILITY** — not a security boundary. Before D14 every role change made from a +shell was invisible in `AuditLog` while the same change through the console was recorded; +now both are recorded and both name a person. Any-domain-user was accepted as sufficient +(no privileged group exists on this estate, and machine access is already restricted to a +few people), which was decided knowing the above. + +### Two deliberate divergences from the API + +- **The bind does NOT apply the login group gate.** If a mistyped required group locks + everyone out of the console, this tool has to still work — otherwise the only route to + fixing the lockout is the thing the lockout prevents. +- **Changing your OWN role is permitted here.** `set_user_role` in `app.py` forbids it to + stop an admin locking themselves out of the console. Here it is the entire bootstrap + path, so it is allowed and recorded with `{"self": true}` in the audit detail. + +The last-admin guard is kept, matching `set_user_role`: an app with no admin cannot be +administered, and there is no password login left to recover through. diff --git a/docs/waves/wave-10.md b/docs/waves/wave-10.md index d6b4e2b..aa14446 100644 --- a/docs/waves/wave-10.md +++ b/docs/waves/wave-10.md @@ -1,6 +1,6 @@ # Wave 10 — Domain authentication over LDAPS -**Items:** `D13` +**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 @@ -337,3 +337,36 @@ happens when the DC is unreachable, whatever `T10.2` decides. - [ ] the DC-unreachable behaviour is stated explicitly - [ ] `IMPLEMENTATION.md` section 4 lists wave 10 - [ ] no doc still claims passwords are stored as bcrypt hashes + +--- + +### 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 +- [ ] verified against a real domain bind rather than a stubbed `authenticate_operator` diff --git a/server/manage_users.py b/server/manage_users.py index e52e2cb..ed18acf 100644 --- a/server/manage_users.py +++ b/server/manage_users.py @@ -1,118 +1,219 @@ """Command-line user management for the Work Package Suite. -Use this to bootstrap the FIRST admin (the /api/auth/users endpoint needs an -existing admin, so one has to be made here) and for occasional account maintenance -from a shell on the server. +Accounts are not created here any more. D13 provisions them on first successful +sign-in, so this tool exists to do the one thing the directory cannot decide: +assign the app's PERMISSIONS role. The directory supplies identity; this supplies +authorization. -NO PASSWORDS. D13 moved authentication to an LDAPS bind against the domain, so -this tool never sets or resets a credential — it only creates accounts and assigns -roles. People sign in with their Windows password; the login page sends anyone who -has forgotten it to Okta. - -Run from the PROJECT ROOT (same place you run uvicorn), so the package imports and -.env resolve the same way the API does: - - python -m server.manage_users create-admin alice --name "Alice Smith" - python -m server.manage_users create bob --role user --name "Bob Jones" python -m server.manage_users list + python -m server.manage_users promote alice # -> admin + python -m server.manage_users promote bob --role project_admin + python -m server.manage_users demote alice # -> project_user python -m server.manage_users disable bob python -m server.manage_users enable bob -`username` must be the person's sAMAccountName, because that is what the bind and -the account match use. Creating an account is optional: anyone who authenticates -against the domain and is not already here is provisioned on first sign-in, at -project_user with no project access. +Run from the PROJECT ROOT (same place you run uvicorn) so the package imports and +.env resolve the way the API does. + +`create-admin` and `create` are GONE (D14). They were redundant once accounts +provision themselves, and removing them closes a whole class of problem: every +row now originates from a successful bind, so a username can no longer be typed +in wrong and end up orphaned from the directory identity it was meant to match. + +BOOTSTRAPPING THE FIRST ADMIN is therefore two steps, in this order: + 1. Sign in to the app once. That provisions your account at project_user. + 2. Run `promote ` here. + +EVERY COMMAND THAT CHANGES ANYTHING REQUIRES A DOMAIN BIND (D14). Shell access +alone is no longer enough to mint an admin. Be clear about what that is and is +not worth: anyone with a shell on this container can still write to the `users` +table directly with psql or sqlite3, so this is defence in depth and — mostly — +ACCOUNTABILITY. Before D14 every role change made from a shell was invisible in +the audit trail while the same change through the console was recorded. Now both +are recorded, and both name a person. + +The bind here deliberately does NOT apply the login group gate. If a mistyped +required group locks everyone out of the console, this tool has to still work — +otherwise the only way to fix the lockout is the only thing the lockout prevents. + +`list` needs no credential, so an outage stays diagnosable. """ import argparse +import getpass import sys import uuid from .db import SessionLocal, Base, engine -from . import models, auth +from . import models, auth, ldap_auth -def _gen_id() -> str: - return f"user_{uuid.uuid4().hex[:12]}" +def _gen_id(prefix: str = "user") -> str: + return f"{prefix}_{uuid.uuid4().hex[:12]}" -def cmd_create(args, role: str | None = None) -> None: - role = role or args.role - # 'user' is the pre-roles spelling of 'project_user' and is still accepted so the - # documented one-liners keep working; anything else has to be a current role. - if role == "user": - role = auth.ROLE_PROJECT_USER - if role not in auth.ROLES: - sys.exit(f"role must be one of {', '.join(auth.ROLES)}") - with SessionLocal() as db: - if auth.find_user(db, args.username): - sys.exit(f"A user named '{args.username}' already exists.") - u = models.User( - id=_gen_id(), - username=args.username.strip(), - full_name=(args.name or "").strip(), - email=(args.email or "").strip(), - role=role, - ) - db.add(u) - db.commit() - print(f"Created {role}: {u.username} (id={u.id})") +def _audit(db, actor: str, action: str, user: "models.User", detail: dict) -> None: + """Append an audit row in the caller's transaction. + + Written by hand rather than via app.py's log_event: importing that would drag + FastAPI and the entire application into a CLI startup for one INSERT. + """ + db.add(models.AuditLog( + id=_gen_id("ev"), actor=actor, action=action, entity_type="user", + entity_id=user.id, summary=user.username, detail=detail, + )) + + +def authenticate_operator() -> str: + """Prompt for a domain credential, bind, and return the operator's sAMAccountName. + + Exits on failure — a command that changes a role must not proceed unauthenticated. + The password is only ever read from a hidden prompt: there is no --password flag, + because that would put a live domain password into shell history and into the + output of `ps` for every other user on the box. + """ + if not ldap_auth.is_configured(): + sys.exit(f"Cannot authenticate: {ldap_auth.describe()}\n" + f"This command needs a domain bind. Fix the LDAP configuration first.") + who = input("Your domain username: ").strip() + if not who: + sys.exit("Cancelled.") + pw = getpass.getpass("Your domain password: ") + if not pw: + sys.exit("Cancelled.") + # required_group="" on purpose: see the module docstring. The login group must + # not be able to lock an operator out of the tool that fixes the login group. + result = ldap_auth.verify(who, pw, required_group="") + if not result.ok: + if result.is_config_problem: + sys.exit(f"Could not reach the domain ({result.reason}): {result.detail}") + sys.exit("Authentication failed.") + print(f"Authenticated as {result.sam}.") + return result.sam + + +def _load(db, username: str) -> "models.User": + u = auth.find_user(db, username) + if not u: + sys.exit(f"No account named '{username}'. Accounts are created on first " + f"sign-in — has this person signed in yet? `list` shows who exists.") + return u def cmd_list(args) -> None: + """Read-only, and deliberately needs no credential: during an outage this is + how you find out what the app thinks the world looks like.""" with SessionLocal() as db: rows = db.query(models.User).order_by(models.User.username).all() if not rows: - print("No users yet. Create one with: create-admin ") + print("No accounts yet. They are created on first successful sign-in.") return - print(f"{'USERNAME':<24}{'ROLE':<20}{'ACTIVE':<8}{'NAME'}") + print(f"{'USERNAME':<24}{'ROLE':<20}{'ACTIVE':<8}{'LAST LOGIN':<22}{'NAME'}") for u in rows: + last = u.last_login_at.strftime("%Y-%m-%d %H:%M") if u.last_login_at else "never" print(f"{u.username:<24}{auth.normalize_role(u.role):<20}" - f"{('yes' if u.is_active else 'no'):<8}{u.full_name}") + f"{('yes' if u.is_active else 'no'):<8}{last:<22}{u.full_name}") + + +def _set_role(username: str, role: str) -> None: + if role not in auth.ROLES: + sys.exit(f"role must be one of {', '.join(auth.ROLES)}") + operator = authenticate_operator() + with SessionLocal() as db: + u = _load(db, username) + old = auth.normalize_role(u.role) + if old == role: + print(f"{u.username} is already {role}. Nothing to do.") + return + # The last-admin guard, matching set_user_role in app.py: an app with no + # admin cannot be administered, and there is no password login left to + # recover through. + if old == auth.ROLE_ADMIN and role != auth.ROLE_ADMIN: + others = db.query(models.User).filter( + models.User.role == auth.ROLE_ADMIN, + models.User.id != u.id, + models.User.is_active.is_(True), + ).count() + if not others: + sys.exit("Refusing: that is the last active admin account. Promote " + "someone else first.") + detail = {"from": old, "to": role, "via": "manage_users"} + u.role = role + # Mirrors set_user_role: an admin already reaches every project, so the + # default-member flag would sit there doing nothing and spring back to life + # on demotion. + if role == auth.ROLE_ADMIN and (u.auto_add_projects or u.auto_add_role): + u.auto_add_projects = False + u.auto_add_role = "" + detail["auto_add_cleared"] = True + # NOTE: unlike the API, this permits changing your OWN role. The endpoint + # forbids it to stop an admin locking themselves out of the console; here it + # is the entire bootstrap path — sign in, then promote yourself. + if u.username.lower() == operator.lower(): + detail["self"] = True + _audit(db, operator, "role_changed", u, detail) + db.commit() + print(f"{u.username}: {old} -> {role}") + + +def cmd_promote(args) -> None: + _set_role(args.username, args.role) + + +def cmd_demote(args) -> None: + _set_role(args.username, auth.ROLE_PROJECT_USER) def _set_active(username: str, active: bool) -> None: + operator = authenticate_operator() with SessionLocal() as db: - u = auth.find_user(db, username) - if not u: - sys.exit(f"No user named '{username}'.") + u = _load(db, username) + if bool(u.is_active) == active: + print(f"{u.username} is already {'enabled' if active else 'disabled'}.") + return u.is_active = active + # Disabling has to take effect on sessions already issued, and role reads go + # through the database on every request — but token_version is what get_current_user + # checks, so bump it to sign them out now rather than at session expiry. + u.token_version = (u.token_version or 0) + 1 + _audit(db, operator, "user_active_changed", u, + {"is_active": active, "via": "manage_users"}) db.commit() print(f"{u.username} is now {'enabled' if active else 'disabled'}.") def main() -> None: - # Ensure the users table exists even on a fresh database. + # Ensure tables exist on a fresh local database (SQLite dev). Production owns + # its schema through alembic. Base.metadata.create_all(bind=engine) - p = argparse.ArgumentParser(prog="manage_users", description="Work Package Suite user management") + p = argparse.ArgumentParser( + prog="manage_users", + description="Work Package Suite user management. Accounts are created on " + "first sign-in (D13); this assigns roles.") sub = p.add_subparsers(dest="cmd", required=True) - def add_create(name, help_): - sp = sub.add_parser(name, help=help_) - sp.add_argument("username", help="the person's sAMAccountName") - sp.add_argument("--name", default="", help="full name") - sp.add_argument("--email", default="") - return sp + sub.add_parser("list", help="list all accounts (no credential needed)") - add_create("create-admin", "create an admin account") - c = add_create("create", "create an account") - c.add_argument("--role", choices=list(auth.ROLES) + ["user"], default=auth.ROLE_PROJECT_USER, - help="permissions role ('user' is the legacy name for project_user)") + pr = sub.add_parser("promote", help="raise an account's permissions role (needs a domain bind)") + pr.add_argument("username", help="the person's sAMAccountName") + pr.add_argument("--role", default=auth.ROLE_ADMIN, choices=list(auth.ROLES), + help="target role (default: admin)") - sub.add_parser("list", help="list all accounts") + dm = sub.add_parser("demote", help=f"set an account back to {auth.ROLE_PROJECT_USER}") + dm.add_argument("username", help="the person's sAMAccountName") - dp = sub.add_parser("disable", help="disable an account (blocks login)") + dp = sub.add_parser("disable", help="disable an account (blocks sign-in)") dp.add_argument("username") ep = sub.add_parser("enable", help="re-enable an account") ep.add_argument("username") args = p.parse_args() - if args.cmd == "create-admin": - cmd_create(args, role="admin") - elif args.cmd == "create": - cmd_create(args) - elif args.cmd == "list": + if args.cmd == "list": cmd_list(args) + elif args.cmd == "promote": + cmd_promote(args) + elif args.cmd == "demote": + cmd_demote(args) elif args.cmd == "disable": _set_active(args.username, False) elif args.cmd == "enable":