feat(db): accounts on native Postgres — #251

backend/src/api/admin_routes.py

@@ -31,7 +31,7 @@
 from src.rail import network as rail
 from src.servers import inventory as servers
 from src.servers import control as server_control
-from src.skins.db import connect
+from src import database
 from src.characters.creation_catalog import get_catalog
 from src.auth.users import session_user_in
 from src.scripts.util.dirs import validate_map
@@ -261,11 +261,12 @@ def _server_write(request: Request, server: str, operation: str, body: ServerJob
     actor = None
     config = _config()
     token = request.cookies.get(session_cookie(config))
+    users.require_accounts()
     try:
-        # Recheck role and session under the same write lock as the audit entry.
-        with connect() as conn:
-            conn.execute("BEGIN IMMEDIATE")
-            actor = session_user_in(conn, token)
+        # Recheck role and session in the audit entry's transaction, holding both rows until it
+        # commits: a demotion or revocation waits for the entry, or lands first and is seen here.
+        with database.connect() as conn:
+            actor = session_user_in(conn, token, lock=True)
             if actor is None:
                 raise HTTPException(401, detail="not_signed_in")
             require_same_origin(request, config)
@@ -283,7 +284,6 @@ def _server_write(request: Request, server: str, operation: str, body: ServerJob
             # Record the intent before crossing the process boundary. The host
             # persists the accepted job; the same id can safely be retried.
             audit.record(conn, actor=actor, action=action, outcome="requested", reason=reason, detail=detail)
-            conn.commit()
         result = server_control.request(operation, server=server,
                                         **{"plan_id" if operation == "start" else "job_id": body.id},
                                         # backup_ref is unused, but helpers before the AMP backup require the key.

backend/src/api/auth_routes.py

@@ -173,6 +173,9 @@ def _patreon(user: dict) -> dict | None:
 @auth_router.get("/auth/discord/start")
 def discord_start(request: Request, return_to: str | None = None):
     config = _config()
+    # Without an accounts database (DATABASE_URL unset) sign-in answers 503 here, before
+    # anyone is sent to Discord or to the sign-in site.
+    users.require_accounts()
     if config.via_site:
         return _preview_start(request, config, return_to)
     state, url = users.start_sign_in(config, return_to)
@@ -336,6 +339,8 @@ def preview_callback(
     config = _config()
     if not config.via_site:
         raise HTTPException(404, detail="Not Found")
+    # Before the ticket is spent with dev: without an accounts database there is nowhere to sign in.
+    users.require_accounts()
     cookie_state = request.cookies.get(state_cookie(config)) or ""
     status, token = "expired", None
     if (

backend/src/api/database_routes.py

@@ -7,8 +7,8 @@
 from src.api.auth_routes import _no_store
 from src.api.staff_access import require_staff_account
 from src.auth import audit
+from src import database
 from src.dbstudio import studio
-from src.skins.db import connect
 
 database_router = APIRouter(prefix="/admin/database", tags=["admin"])
 
@@ -55,11 +55,11 @@ def _audited(actor: dict, source_id: str, sql: str, write: bool, run, action: st
 def database_overview(request: Request, response: Response):
     actor = require_staff_account(request, "use_database")
     _no_store(response)
-    with connect() as conn:
-        counts = dict(conn.execute(
-            "SELECT json_extract(detail_json, '$.database'), count(*) FROM admin_audit "
+    with database.connect() as conn:
+        counts = {row["source"]: row["n"] for row in conn.execute(
+            "SELECT detail_json ->> 'database' AS source, count(*) AS n FROM admin_audit "
             "WHERE action IN ('database.read', 'database.write') AND outcome IN ('ok', 'failed') "
-            "GROUP BY 1").fetchall())
+            "GROUP BY 1")}
     return _audited(actor, "*", "list databases", False, lambda: studio.overview(counts), "database.list")
 
 

backend/src/auth/account_overview.py

@@ -69,6 +69,7 @@ def rank(player_uuid: str) -> str | None:
     if key is None:
         return None
     try:
+        # PG-MIGRATION: until the LuckPerms area converts, its mirror is in SQLite.
         with connect() as conn:
             if mirror.user_row(conn, key) is None:
                 return None

backend/src/auth/admin.py

@@ -1,23 +1,30 @@
 """Staff operations on website accounts.
 
-Every write reloads the actor's session and both roles inside one immediate
-transaction, so a demotion or revocation that lands first always wins. The
-change, the target's session revocation and the audit row commit together;
-a refused attempt rolls back and is then logged on its own.
+Every write locks the actor's and the target's account rows, then reloads the
+actor's session and both roles inside that transaction, so a demotion or
+revocation that lands first always wins. The change, the target's session
+revocation and the audit row commit together; a refused attempt rolls back and
+is then logged on its own.
 """
 from __future__ import annotations
 
-from src.skins.db import connect
+import json
+
+from src import database
+from src.skins import db as sqlite_db
+from src.skins.codes import hash_secret
 
 from . import audit, roles
-from .users import drop_view_leftovers, session_user_in
+from .users import _TOKEN_MAX, PLACEHOLDERS_LOCK, api_time, drop_view_leftovers, require_accounts, session_user_in
 
 # A user's own sessions and the views they have open as players.
-_REVOKE = "DELETE FROM user_sessions WHERE user_id = ? OR acting_user_id = ?"
+_REVOKE = "DELETE FROM user_sessions WHERE user_id = %s OR acting_user_id = %s"
 REASON_MIN = 3
 REASON_MAX = 500
 _LOOKUP_MAX = 64
 _LOOKUP_LIMIT = 5
+# Ids are bigint; a larger one from the URL names no account.
+_ID_MAX = 2**63 - 1
 
 _ACCOUNT_COLUMNS = (
     "id AS user_id, discord_user_id, discord_username, discord_global_name, "
@@ -42,39 +49,62 @@ def clean_reason(reason: str | None) -> str:
     return text
 
 
+def _account_json(row) -> dict:
+    """An account row with its times in the API form."""
+    account = dict(row)
+    for key in ("created_at", "last_login_at"):
+        account[key] = api_time(account[key])
+    return account
+
+
 def _account(conn, user_id: int) -> dict | None:
-    row = conn.execute(f"SELECT {_ACCOUNT_COLUMNS} FROM users WHERE id = ? AND placeholder = 0", (user_id,)).fetchone()
-    return dict(row) if row else None
+    if not 0 < user_id <= _ID_MAX:
+        return None
+    row = conn.execute(f"SELECT {_ACCOUNT_COLUMNS} FROM users WHERE id = %s AND NOT placeholder",
+                       (user_id,)).fetchone()
+    return _account_json(row) if row else None
+
+
+def _lock_accounts(conn, token: str | None, target_id: int) -> None:
+    """Lock the actor's and the target's users rows, in id order, before anything reads them.
+
+    Two staff acting on each other then run one after the other, and the second
+    sees the first's change.
+    """
+    ids = {target_id} if 0 < target_id <= _ID_MAX else set()
+    if token and len(token) <= _TOKEN_MAX:
+        session = conn.execute("SELECT user_id FROM user_sessions WHERE token_hash = %s",
+                               (hash_secret(token),)).fetchone()
+        if session is not None:
+            ids.add(session["user_id"])
+    if ids:
+        conn.execute("SELECT id FROM users WHERE id = ANY(%s) ORDER BY id FOR UPDATE", (sorted(ids),))
 
 
 def _staff_write(token: str, action: str, target_id: int, reason: str | None, apply) -> dict:
     """Run apply(conn, actor, target, reason) under the shared staff checks."""
+    require_accounts()
     actor = target = None
     clean = None
     try:
-        with connect() as conn:
-            conn.execute("BEGIN IMMEDIATE")
-            try:
-                actor = session_user_in(conn, token)
-                if actor is None:
-                    raise AdminError(401, "not_signed_in", "denied")
-                if not roles.is_staff(actor["role"]):
-                    raise AdminError(403, "forbidden", "denied")
-                clean = clean_reason(reason)
-                target = _account(conn, target_id)
-                if target is None:
-                    raise AdminError(404, "account_not_found", "not_found")
-                if actor["user_id"] == target["user_id"]:
-                    raise AdminError(403, "cannot_act_on_self", "denied")
-                if not roles.outranks(actor["role"], target["role"]):
-                    raise AdminError(403, "target_outranks_you", "denied")
-                detail = apply(conn, actor, target, clean)
-                audit.record(conn, actor=actor, action=action, outcome="ok", target=target,
-                             reason=clean, detail=detail)
-                conn.commit()
-            except BaseException:
-                conn.rollback()
-                raise
+        with database.transaction(lock=PLACEHOLDERS_LOCK) as conn:
+            _lock_accounts(conn, token, target_id)
+            actor = session_user_in(conn, token, lock=True)
+            if actor is None:
+                raise AdminError(401, "not_signed_in", "denied")
+            if not roles.is_staff(actor["role"]):
+                raise AdminError(403, "forbidden", "denied")
+            clean = clean_reason(reason)
+            target = _account(conn, target_id)
+            if target is None:
+                raise AdminError(404, "account_not_found", "not_found")
+            if actor["user_id"] == target["user_id"]:
+                raise AdminError(403, "cannot_act_on_self", "denied")
+            if not roles.outranks(actor["role"], target["role"]):
+                raise AdminError(403, "target_outranks_you", "denied")
+            detail = apply(conn, actor, target, clean)
+            audit.record(conn, actor=actor, action=action, outcome="ok", target=target,
+                         reason=clean, detail=detail)
     except AdminError as exc:
         if actor is not None:
             audit.record_refusal(actor=actor, action=action, outcome=exc.outcome, target=target,
@@ -91,7 +121,7 @@ def apply(conn, actor, target, _reason):
             raise AdminError(403, "role_not_assignable", "denied")
         if new_role == target["role"]:
             raise AdminError(409, "role_unchanged", "conflict")
-        conn.execute("UPDATE users SET role = ? WHERE id = ?", (new_role, target["user_id"]))
+        conn.execute("UPDATE users SET role = %s WHERE id = %s", (new_role, target["user_id"]))
         # Live role checks already apply; signing out also stops open tabs
         # from carrying on with the old role's view.
         revoked = conn.execute(_REVOKE, (target["user_id"], target["user_id"])).rowcount
@@ -112,37 +142,58 @@ def apply(conn, actor, target, _reason):
     return _staff_write(token, "account.sessions.revoke", target_id, reason, apply)
 
 
-# Read-only view of the Discord link: listing never expires graces or queues notices.
-_WITH_MINECRAFT = (
-    "SELECT u.id AS user_id, u.discord_user_id, u.discord_username, u.discord_global_name, "
-    "u.discord_avatar, u.role, u.created_at, u.last_login_at, dl.minecraft_name "
-    "FROM users u LEFT JOIN discord_links dl ON dl.discord_user_id = u.discord_user_id"
+_ACCOUNTS = (
+    "SELECT id AS user_id, discord_user_id, discord_username, discord_global_name, "
+    "discord_avatar, role, created_at, last_login_at FROM users"
 )
 
 
+def _with_minecraft(rows) -> list[dict]:
+    """Accounts with the Minecraft name of their Discord link, if any.
+
+    Read-only view of the link: listing never expires graces or queues notices.
+    """
+    accounts = [_account_json(row) for row in rows]
+    # PG-MIGRATION: until the Linking area converts, discord_links is in SQLite. Its PR
+    # puts back the join: LEFT JOIN discord_links dl ON dl.discord_user_id = u.discord_user_id.
+    names = {}
+    ids = [a["discord_user_id"] for a in accounts]
+    if ids:
+        with sqlite_db.connect() as lite:
+            names = {
+                row["discord_user_id"]: row["minecraft_name"]
+                for row in lite.execute(
+                    "SELECT discord_user_id, minecraft_name FROM discord_links "
+                    "WHERE discord_user_id IN (SELECT value FROM json_each(?))",
+                    (json.dumps(ids),),
+                )
+            }
+    return [{**a, "minecraft_name": names.get(a["discord_user_id"])} for a in accounts]
+
+
 def list_staff() -> list[dict]:
-    with connect() as conn:
+    with database.connect() as conn:
         rows = conn.execute(
-            f"{_WITH_MINECRAFT} WHERE u.role IN ('mod', 'admin', 'root') "
-            "ORDER BY CASE u.role WHEN 'root' THEN 0 WHEN 'admin' THEN 1 ELSE 2 END, "
-            "lower(COALESCE(u.discord_username, u.discord_global_name, '')), u.id"
+            f"{_ACCOUNTS} WHERE role IN ('mod', 'admin', 'root') "
+            "ORDER BY CASE role WHEN 'root' THEN 0 WHEN 'admin' THEN 1 ELSE 2 END, "
+            "lower(COALESCE(discord_username, discord_global_name, '')), id"
         ).fetchall()
-    return [dict(row) for row in rows]
+    return _with_minecraft(rows)
 
 
 def lookup(query: str) -> list[dict]:
     """Exact matches only: a Discord id, or a username or display name in any case."""
     text = (query or "").strip().lstrip("@")
     if not text or len(text) > _LOOKUP_MAX:
         return []
-    with connect() as conn:
+    with database.connect() as conn:
         rows = conn.execute(
             f"""
-            {_WITH_MINECRAFT}
-            WHERE u.placeholder = 0 AND (u.discord_user_id = ? OR lower(u.discord_username) = lower(?)
-               OR lower(u.discord_global_name) = lower(?))
-            ORDER BY u.id LIMIT ?
+            {_ACCOUNTS}
+            WHERE NOT placeholder AND (discord_user_id = %s OR lower(discord_username) = lower(%s)
+               OR lower(discord_global_name) = lower(%s))
+            ORDER BY id LIMIT %s
             """,
             (text, text, text, _LOOKUP_LIMIT),
         ).fetchall()
-    return [dict(row) for row in rows]
+    return _with_minecraft(rows)

backend/src/auth/admin_cli.py

@@ -15,56 +15,56 @@
 import sys
 from pathlib import Path
 
+from src import database
 from src.skins import db
 
 from . import audit
 from .admin import REASON_MAX, REASON_MIN, AdminError, clean_reason, list_staff
-from .users import drop_view_leftovers
+from .users import PLACEHOLDERS_LOCK, drop_view_leftovers
 
 
 def _set_root(discord_id: str, reason: str | None, *, grant: bool, demote_to: str = "player") -> str:
     clean = clean_reason(reason)
-    with db.connect() as conn:
-        conn.execute("BEGIN IMMEDIATE")
-        try:
-            row = conn.execute(
-                "SELECT id AS user_id, discord_user_id, discord_username, discord_global_name, role "
-                "FROM users WHERE discord_user_id = ? AND placeholder = 0",
-                (discord_id,),
-            ).fetchone()
-            if row is None:
-                raise SystemExit("No website account for that Discord id; sign in to the site once first.")
-            target = dict(row)
-            if grant:
-                if target["role"] == "root":
-                    raise SystemExit("That account is already root.")
-                new_role = "root"
-            else:
-                if target["role"] != "root":
-                    raise SystemExit("That account is not root.")
-                roots = conn.execute("SELECT COUNT(*) FROM users WHERE role = 'root'").fetchone()[0]
-                if roots <= 1:
-                    raise SystemExit("Refusing to remove the last root account.")
-                new_role = demote_to
-            conn.execute("UPDATE users SET role = ? WHERE id = ?", (new_role, target["user_id"]))
-            revoked = conn.execute(
-                "DELETE FROM user_sessions WHERE user_id = ? OR acting_user_id = ?",
-                (target["user_id"], target["user_id"]),
-            ).rowcount
-            drop_view_leftovers(conn)
-            audit.record(
-                conn, actor=None, action="account.role.change", outcome="ok", target=target, reason=clean,
-                detail={"before": target["role"], "after": new_role, "sessions_revoked": revoked},
-            )
-            conn.commit()
-        except BaseException:
-            conn.rollback()
-            raise
+    with database.transaction(lock=PLACEHOLDERS_LOCK) as conn:
+        # Lock the target, and to take root away every root too, in id order, so two
+        # removals cannot each see the other root still there.
+        roots = "" if grant else " OR role = 'root'"
+        locked = conn.execute(
+            "SELECT id AS user_id, discord_user_id, discord_username, discord_global_name, role "
+            f"FROM users WHERE NOT placeholder AND (discord_user_id = %s{roots}) ORDER BY id FOR UPDATE",
+            (discord_id,),
+        ).fetchall()
+        target = next((dict(row) for row in locked if row["discord_user_id"] == discord_id), None)
+        if target is None:
+            raise SystemExit("No website account for that Discord id; sign in to the site once first.")
+        if grant:
+            if target["role"] == "root":
+                raise SystemExit("That account is already root.")
+            new_role = "root"
+        else:
+            if target["role"] != "root":
+                raise SystemExit("That account is not root.")
+            if sum(row["role"] == "root" for row in locked) <= 1:
+                raise SystemExit("Refusing to remove the last root account.")
+            new_role = demote_to
+        conn.execute("UPDATE users SET role = %s WHERE id = %s", (new_role, target["user_id"]))
+        revoked = conn.execute(
+            "DELETE FROM user_sessions WHERE user_id = %s OR acting_user_id = %s",
+            (target["user_id"], target["user_id"]),
+        ).rowcount
+        drop_view_leftovers(conn)
+        audit.record(
+            conn, actor=None, action="account.role.change", outcome="ok", target=target, reason=clean,
+            detail={"before": target["role"], "after": new_role, "sessions_revoked": revoked},
+        )
     return f"{audit.display_name(target)}: {target['role']} -> {new_role}"
 
 
 def main(argv: list[str] | None = None) -> int:
     parser = argparse.ArgumentParser(prog="python -m src.auth.admin_cli", description=__doc__.splitlines()[0])
+    parser.add_argument("--database-url", help="the site's Postgres (default: DATABASE_URL)")
+    # PG-MIGRATION: until the Scripts area converts, Profile sessions and Discord links are
+    # still read from province.db (drop_view_leftovers, list-staff's Minecraft names).
     parser.add_argument("--db", type=Path, help="database file (default: the site's province.db)")
     sub = parser.add_subparsers(dest="command", required=True)
     for name in ("grant-root", "revoke-root"):
@@ -84,6 +84,8 @@ def main(argv: list[str] | None = None) -> int:
         db.DRINKS_DIR = db.DATA_DIR / "drinks"
     print(f"Database: {db.DB_PATH}", file=sys.stderr)
     db.migrate()
+    if args.database_url:
+        database.open_pool(args.database_url, min_size=0, max_size=1)
 
     if args.command == "list-staff":
         for row in list_staff():