diff --git a/apps/board/app.py b/apps/board/app.py index cf08581..61b8bd9 100644 --- a/apps/board/app.py +++ b/apps/board/app.py @@ -91,11 +91,12 @@ def create_app(overrides: dict | None = None) -> Flask: # restart, which is a confusing way to find out the variable is unset. raise RuntimeError("BOARD_SECRET_KEY is required (generate one with `openssl rand -hex 32`).") - from . import auth, board, content, members, uploads + from . import auth, board, content, members, messages, uploads app.register_blueprint(auth.bp, url_prefix=URL_PREFIX) app.register_blueprint(members.bp, url_prefix=URL_PREFIX) app.register_blueprint(content.bp, url_prefix=URL_PREFIX) app.register_blueprint(uploads.bp, url_prefix=URL_PREFIX) + app.register_blueprint(messages.bp, url_prefix=URL_PREFIX) app.register_blueprint(board.bp, url_prefix=URL_PREFIX) app.teardown_appcontext(close_db) @@ -106,6 +107,12 @@ def create_app(overrides: dict | None = None) -> Flask: def _year(): return {"current_year": datetime.now(timezone.utc).year} + @app.context_processor + def _unread(): + # Lazily: base.html is rendered for signed-out pages and error pages + # too, and neither should run a query to draw a badge nobody sees. + return {"unread_private": messages.unread_count} + @app.before_request def _before(): # Order matters: reject forged writes before any handler can act on diff --git a/apps/board/db.py b/apps/board/db.py index 1b81e03..a247864 100644 --- a/apps/board/db.py +++ b/apps/board/db.py @@ -13,6 +13,8 @@ from pathlib import Path from flask import current_app, g +from . import migrations + SCHEMA_PATH = Path(__file__).with_name("schema.sql") # Authorship of a removed member is reassigned to this row rather than deleted, @@ -48,11 +50,20 @@ def close_db(_exception=None) -> None: def init_db(app) -> None: - """Apply the schema and make sure the fixed rows exist.""" + """Apply the schema, bring old databases up to date, seed the fixed rows. + + The order is deliberate. `schema.sql` is all CREATE TABLE IF NOT EXISTS, so + on an empty database it builds everything in its current shape and the + migrations below find nothing to do; on a database that has run before it + adds only what is new and silently leaves existing tables alone — which is + precisely why migrations have to come second and clean up after it. + """ Path(app.config["DB_PATH"]).parent.mkdir(parents=True, exist_ok=True) db = connect(app.config["DB_PATH"]) try: db.executescript(SCHEMA_PATH.read_text(encoding="utf-8")) + for step in migrations.apply(db): + app.logger.info("Applied migration %s", step) _ensure_tombstone(db) _seed_owner(db, app) finally: diff --git a/apps/board/members.py b/apps/board/members.py index e327200..3c3994d 100644 --- a/apps/board/members.py +++ b/apps/board/members.py @@ -21,7 +21,7 @@ import sqlite3 from flask import (Blueprint, Response, abort, current_app, flash, g, redirect, render_template, request, url_for) -from . import auth, gitea, invites, mail +from . import auth, gitea, invites, mail, uploads from .db import TOMBSTONE_LOGIN, get_db from .security import admin_required, login_required, owner_required @@ -91,17 +91,62 @@ REASSIGNED_ON_ERASE = { } CLEARED_ON_ERASE = {("members", "created_by")} +# Private correspondence is not reassigned to the tombstone, it goes. A thread +# outlives its author because other people replied and the conversation would +# otherwise lose its shape; a two-party exchange has no such remainder, and +# keeping half of somebody's erased correspondence is close to the thing +# erasure exists to prevent. The other person's copy goes with it, which is the +# uncomfortable half of that choice and is meant to be. +REMOVED_ON_ERASE = { + ("conversation_members", "member_id"), + ("messages", "author_id"), + ("blocks", "blocker_id"), + ("blocks", "blocked_id"), +} -def erase_member(db: sqlite3.Connection, member_id: int) -> None: - """Remove a member and their personal data, keeping the conversation intact. + +def _conversations_of(member_id: int) -> str: + return "SELECT conversation_id FROM conversation_members WHERE member_id = ?" + + +def erase_member(db: sqlite3.Connection, member_id: int) -> list[str]: + """Remove a member and their personal data, keeping the board intact. GDPR erasure means the name, login and address go. It does not mean the - threads other people replied to should vanish, so authorship moves to the - tombstone row instead of cascading or dangling. + threads other people replied to should vanish, so public authorship moves + to the tombstone row instead of cascading or dangling. Private messages are + the exception and are deleted outright — see REMOVED_ON_ERASE. + + Returns the stored names of pictures whose rows have gone, so the caller can + take them off disk. Deleting files is not done here because this function + is a transaction: a file removed inside one cannot be put back if the + transaction rolls away underneath it. """ ghost = tombstone_id(db) db.execute("BEGIN IMMEDIATE") try: + # Read before deleting: once the conversations are gone there is + # nothing left to work out which files they carried. + orphaned = [row["stored_name"] for row in db.execute( + f"""SELECT a.stored_name FROM attachments a + JOIN messages m ON m.id = a.message_id + WHERE m.conversation_id IN ({_conversations_of(member_id)})""", + (member_id,), + )] + # Attachments first: they point at messages without cascading, so + # deleting the conversation while they exist is refused. + db.execute( + f"""DELETE FROM attachments WHERE message_id IN ( + SELECT id FROM messages + WHERE conversation_id IN ({_conversations_of(member_id)}))""", + (member_id,), + ) + db.execute( + f"DELETE FROM conversations WHERE id IN ({_conversations_of(member_id)})", + (member_id,), + ) + db.execute("DELETE FROM blocks WHERE blocker_id = ? OR blocked_id = ?", + (member_id, member_id)) # Table and column names come from the module constants above, never # from a request, so the interpolation is not a place user input can # reach. @@ -116,6 +161,7 @@ def erase_member(db: sqlite3.Connection, member_id: int) -> None: except Exception: db.execute("ROLLBACK") raise + return orphaned def _load(member_id: int): @@ -306,7 +352,10 @@ def erase(member_id: int): target = _load(member_id) if target["role"] == "owner": abort(403) - erase_member(get_db(), member_id) + # Files only after the transaction has committed: a rollback can put the + # rows back, and nothing can put the pictures back. + for stored_name in erase_member(get_db(), member_id): + uploads.remove(stored_name) flash(f"{target['display_name']} eliminado. Sus mensajes quedan como «Miembro eliminado».", "ok") return redirect(url_for("members.index")) diff --git a/apps/board/messages.py b/apps/board/messages.py new file mode 100644 index 0000000..ff3eac0 --- /dev/null +++ b/apps/board/messages.py @@ -0,0 +1,260 @@ +"""Private messages between two members. + +An inbox, not a chat. Each conversation has exactly two people, each keeps +their own unread mark, and either can block the other — after which neither can +write. That symmetry is deliberate: a block that silences only one side is a +one-way megaphone, which is worse than having no block at all. + +Every rule here is enforced in the handler, not only in the template. A hidden +button is a courtesy to somebody using the site normally; it is not a rule, and +the difference matters most for exactly the person a block exists to stop. +""" + +from __future__ import annotations + +from flask import (Blueprint, abort, flash, g, redirect, render_template, + request, url_for) + +from . import uploads +from .db import TOMBSTONE_LOGIN, get_db +from .render import to_html +from .security import login_required + +bp = Blueprint("messages", __name__) + +BODY_MAX = 20_000 + + +def _other_party(conversation_id: int): + """The member on the far side, or None if they have been erased.""" + return get_db().execute( + """SELECT m.* FROM conversation_members cm + JOIN members m ON m.id = cm.member_id + WHERE cm.conversation_id = ? AND cm.member_id != ?""", + (conversation_id, g.member["id"]), + ).fetchone() + + +def _mine_or_404(conversation_id: int): + """Membership is the entire access rule, and it is checked on every hit. + + Not "does this conversation exist" — a member who is not in it must not be + able to tell the difference between a conversation that is not theirs and + one that does not exist. + """ + row = get_db().execute( + """SELECT c.* FROM conversations c + JOIN conversation_members cm ON cm.conversation_id = c.id + WHERE c.id = ? AND cm.member_id = ?""", + (conversation_id, g.member["id"]), + ).fetchone() + if row is None: + abort(404) + return row + + +def blocked_between(a: int, b: int) -> bool: + """A block in either direction stops both directions.""" + return get_db().execute( + """SELECT 1 FROM blocks + WHERE (blocker_id = ? AND blocked_id = ?) + OR (blocker_id = ? AND blocked_id = ?)""", + (a, b, b, a), + ).fetchone() is not None + + +def conversation_with(other_id: int) -> int: + """The conversation between the signed-in member and `other_id`, made if + it does not exist yet.""" + db = get_db() + row = db.execute( + """SELECT a.conversation_id AS id + FROM conversation_members a + JOIN conversation_members b ON b.conversation_id = a.conversation_id + WHERE a.member_id = ? AND b.member_id = ?""", + (g.member["id"], other_id), + ).fetchone() + if row: + return row["id"] + + conversation_id = db.execute( + "INSERT INTO conversations DEFAULT VALUES").lastrowid + for member_id in (g.member["id"], other_id): + db.execute( + "INSERT INTO conversation_members (conversation_id, member_id) VALUES (?, ?)", + (conversation_id, member_id), + ) + return conversation_id + + +def unread_count() -> int: + """For the badge in the navigation. One query, not one per conversation.""" + if g.member is None: + return 0 + return get_db().execute( + """SELECT COUNT(*) AS n + FROM messages m + JOIN conversation_members cm + ON cm.conversation_id = m.conversation_id AND cm.member_id = ? + WHERE m.author_id != ? + AND m.deleted_at IS NULL + AND (cm.last_read_at IS NULL OR m.created_at > cm.last_read_at)""", + (g.member["id"], g.member["id"]), + ).fetchone()["n"] + + +# --- routes --------------------------------------------------------------- + +@bp.route("/privados") +@login_required +def inbox(): + rows = get_db().execute( + """SELECT c.id, + other.display_name AS other_name, + other.id AS other_id, + (SELECT body_md FROM messages + WHERE conversation_id = c.id AND deleted_at IS NULL + ORDER BY created_at DESC LIMIT 1) AS last_body, + (SELECT created_at FROM messages + WHERE conversation_id = c.id AND deleted_at IS NULL + ORDER BY created_at DESC LIMIT 1) AS last_at, + (SELECT COUNT(*) FROM messages m + WHERE m.conversation_id = c.id AND m.author_id != mine.member_id + AND m.deleted_at IS NULL + AND (mine.last_read_at IS NULL OR m.created_at > mine.last_read_at) + ) AS unread + FROM conversations c + JOIN conversation_members mine + ON mine.conversation_id = c.id AND mine.member_id = :me + JOIN conversation_members theirs + ON theirs.conversation_id = c.id AND theirs.member_id != :me + JOIN members other ON other.id = theirs.member_id + ORDER BY last_at DESC NULLS LAST""", + {"me": g.member["id"]}, + ).fetchall() + + # Anyone you could start writing to: every active member but yourself, the + # tombstone, and anyone either of you has blocked. + people = get_db().execute( + """SELECT m.id, m.display_name FROM members m + WHERE m.id != :me AND m.active = 1 + AND m.role IN ('owner', 'admin', 'user') + AND m.gitea_login != :ghost + AND NOT EXISTS (SELECT 1 FROM blocks + WHERE (blocker_id = :me AND blocked_id = m.id) + OR (blocker_id = m.id AND blocked_id = :me)) + ORDER BY m.display_name COLLATE NOCASE""", + {"me": g.member["id"], "ghost": TOMBSTONE_LOGIN}, + ).fetchall() + + return render_template("inbox.html", conversations=rows, people=people, + excerpt=lambda text: (text or "")[:120]) + + +@bp.route("/privados/nueva", methods=["POST"]) +@login_required +def start(): + """The member comes from the form, not the path, so the select can post + straight here. A path parameter would need JavaScript to rewrite the + action, and the Content-Security-Policy makes inline handlers silently + inert — a class of bug this codebase has already paid for once.""" + member_id = request.form.get("member_id", type=int) + if member_id is None or member_id == g.member["id"]: + abort(400) + target = get_db().execute( + """SELECT * FROM members + WHERE id = ? AND active = 1 AND role IN ('owner', 'admin', 'user')""", + (member_id,), + ).fetchone() + if target is None: + abort(404) + if blocked_between(g.member["id"], member_id): + abort(403) + return redirect(url_for("messages.conversation", + conversation_id=conversation_with(member_id))) + + +@bp.route("/privados/") +@login_required +def conversation(conversation_id: int): + _mine_or_404(conversation_id) + db = get_db() + rows = db.execute( + """SELECT m.*, a.display_name AS author + FROM messages m JOIN members a ON a.id = m.author_id + WHERE m.conversation_id = ? AND m.deleted_at IS NULL + ORDER BY m.created_at""", + (conversation_id,), + ).fetchall() + + # Marked read on opening, which is what the member just did. Written before + # rendering so a slow page does not leave the badge stale. + db.execute( + """UPDATE conversation_members SET last_read_at = datetime('now') + WHERE conversation_id = ? AND member_id = ?""", + (conversation_id, g.member["id"]), + ) + + other = _other_party(conversation_id) + return render_template( + "conversation.html", conversation_id=conversation_id, messages=rows, + other=other, to_html=to_html, + images=uploads.for_messages([row["id"] for row in rows]), + blocked=other is not None and blocked_between(g.member["id"], other["id"]), + ) + + +@bp.route("/privados//enviar", methods=["POST"]) +@login_required +def send(conversation_id: int): + _mine_or_404(conversation_id) + other = _other_party(conversation_id) + if other is None: + flash("Esa persona ya no está en la comunidad.", "error") + return redirect(url_for("messages.conversation", conversation_id=conversation_id)) + # Checked here and not only hidden in the template: the template is a + # courtesy, this is the rule. + if blocked_between(g.member["id"], other["id"]): + abort(403) + + body = request.form.get("body", "").strip()[:BODY_MAX] + try: + staged = uploads.stage(request.files.getlist("pictures")) + except uploads.RejectedUpload as exc: + flash(str(exc), "error") + return redirect(url_for("messages.conversation", conversation_id=conversation_id)) + if not body and not staged: + flash("Escribe algo o adjunta una imagen.", "error") + return redirect(url_for("messages.conversation", conversation_id=conversation_id)) + + cursor = get_db().execute( + "INSERT INTO messages (conversation_id, author_id, body_md) VALUES (?, ?, ?)", + (conversation_id, g.member["id"], body), + ) + uploads.save(staged, g.member["id"], message_id=cursor.lastrowid) + return redirect(url_for("messages.conversation", conversation_id=conversation_id) + "#final") + + +@bp.route("/privados/bloquear/", methods=["POST"]) +@login_required +def block(member_id: int): + if member_id == g.member["id"]: + abort(400) + get_db().execute( + """INSERT INTO blocks (blocker_id, blocked_id) VALUES (?, ?) + ON CONFLICT DO NOTHING""", + (g.member["id"], member_id), + ) + flash("Bloqueado. Ninguno de los dos puede escribir al otro.", "ok") + return redirect(url_for("messages.inbox")) + + +@bp.route("/privados/desbloquear/", methods=["POST"]) +@login_required +def unblock(member_id: int): + """Only your own block. Removing somebody else's would let the blocked + person undo the thing that was done to protect against them.""" + get_db().execute("DELETE FROM blocks WHERE blocker_id = ? AND blocked_id = ?", + (g.member["id"], member_id)) + flash("Desbloqueado.", "ok") + return redirect(url_for("messages.inbox")) diff --git a/apps/board/migrations.py b/apps/board/migrations.py new file mode 100644 index 0000000..6a951c1 --- /dev/null +++ b/apps/board/migrations.py @@ -0,0 +1,115 @@ +"""Changes to tables that already exist. + +`schema.sql` is all `CREATE TABLE IF NOT EXISTS`, which handles exactly one +kind of change: a brand-new table. It silently does nothing to a table that is +already there, so every alteration to an existing one has to happen here. + +That was fine until now because every change so far had been a new table. The +first change that is not — giving `attachments` a third possible parent — also +happens to be one SQLite cannot do in place, because the old row carries a +CHECK constraint and SQLite has no `DROP CONSTRAINT`. Verified rather than +assumed: `ALTER TABLE ... ADD COLUMN` succeeds, and the next insert is refused +by a constraint that can no longer be removed. + +**The order matters.** `init_db` applies `schema.sql` first and then these. On +an empty database the schema creates everything in its current shape and each +step below finds its work already done, so every step must be written to check +before it acts and return quietly. + +Steps are numbered, applied once, in order, each in its own transaction, and +recorded in SQLite's own `PRAGMA user_version`. Never renumber one and never +edit one that has shipped: a server that has already run it will not run it +again, so a correction is a new step. +""" + +from __future__ import annotations + +import sqlite3 + + +def _columns(db: sqlite3.Connection, table: str) -> set[str]: + return {row[1] for row in db.execute(f"PRAGMA table_info({table})")} + + +def _attachments_accept_messages(db: sqlite3.Connection) -> None: + """Let an attachment hang off a private message. + + The table is rebuilt rather than altered because of its CHECK constraint: + `(thread_id IS NULL) <> (comment_id IS NULL)` insists that exactly one of + those two is set, so a row belonging to a message — with both of them null + — is refused. Adding the column is allowed; using it is not. + + This is SQLite's documented procedure for changing a constraint: build the + new table beside the old one, copy the rows, drop the old, rename. The + foreign keys are switched off around it because dropping a table with them + on can cascade, and switched back on after, with a check that nothing was + broken in between. + """ + if "message_id" in _columns(db, "attachments"): + return # a new database: schema.sql already created it this way + + db.execute("PRAGMA foreign_keys = OFF") + try: + db.execute(""" + CREATE TABLE attachments_new ( + id INTEGER PRIMARY KEY, + thread_id INTEGER REFERENCES threads(id), + comment_id INTEGER REFERENCES comments(id), + message_id INTEGER REFERENCES messages(id), + stored_name TEXT NOT NULL UNIQUE, + original_name TEXT NOT NULL, + content_type TEXT NOT NULL, + bytes INTEGER NOT NULL, + uploaded_by INTEGER NOT NULL REFERENCES members(id), + created_at TEXT NOT NULL DEFAULT (datetime('now')), + CHECK ((thread_id IS NOT NULL) + (comment_id IS NOT NULL) + + (message_id IS NOT NULL) = 1) + )""") + # Named columns, not SELECT *: the order has to survive somebody adding + # a column to one of the two tables later. + db.execute(""" + INSERT INTO attachments_new + (id, thread_id, comment_id, stored_name, original_name, + content_type, bytes, uploaded_by, created_at) + SELECT id, thread_id, comment_id, stored_name, original_name, + content_type, bytes, uploaded_by, created_at + FROM attachments""") + db.execute("DROP TABLE attachments") + db.execute("ALTER TABLE attachments_new RENAME TO attachments") + db.execute("CREATE INDEX IF NOT EXISTS attachments_thread ON attachments(thread_id)") + db.execute("CREATE INDEX IF NOT EXISTS attachments_comment ON attachments(comment_id)") + db.execute("CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id)") + + broken = db.execute("PRAGMA foreign_key_check").fetchall() + if broken: + raise RuntimeError(f"migration left dangling references: {broken}") + finally: + db.execute("PRAGMA foreign_keys = ON") + + +# (number, description, function). The number is the value written to +# user_version once the step succeeds. +STEPS = [ + (1, "attachments can belong to a private message", _attachments_accept_messages), +] + + +def apply(db: sqlite3.Connection) -> list[str]: + """Run whatever this database has not run yet. Returns what was applied.""" + version = db.execute("PRAGMA user_version").fetchone()[0] + done = [] + for number, description, step in sorted(STEPS): + if number <= version: + continue + db.execute("BEGIN IMMEDIATE") + try: + step(db) + # Not a parameter: PRAGMA does not take them. The value is an int + # from the list above, never from anything a request can reach. + db.execute(f"PRAGMA user_version = {int(number)}") + db.execute("COMMIT") + except Exception: + db.execute("ROLLBACK") + raise + done.append(f"{number}: {description}") + return done diff --git a/apps/board/schema.sql b/apps/board/schema.sql index b46b7a6..e1964ff 100644 --- a/apps/board/schema.sql +++ b/apps/board/schema.sql @@ -55,7 +55,62 @@ CREATE TABLE IF NOT EXISTS comments ( CREATE INDEX IF NOT EXISTS comments_thread ON comments(thread_id, created_at) WHERE deleted_at IS NULL; --- Pictures attached to a thread or a comment. +-- Private messages between two members. +-- +-- An inbox, not live chat: gunicorn's sync workers cannot hold a connection +-- open per signed-in member, and that would be the first thing on this box +-- with a real scaling limit. +-- +-- Membership is its own table rather than two columns on `conversations` +-- because the unread mark is per person: each side keeps its own +-- `last_read_at`, and the badge counts messages newer than it that somebody +-- else wrote. Two columns would need two last-read fields and a rule about +-- which is which. +CREATE TABLE IF NOT EXISTS conversations ( + id INTEGER PRIMARY KEY, + created_at TEXT NOT NULL DEFAULT (datetime('now')) +); + +CREATE TABLE IF NOT EXISTS conversation_members ( + conversation_id INTEGER NOT NULL REFERENCES conversations(id) ON DELETE CASCADE, + member_id INTEGER NOT NULL REFERENCES members(id), + last_read_at TEXT, + PRIMARY KEY (conversation_id, member_id) +); + +CREATE INDEX IF NOT EXISTS conversation_members_member + ON conversation_members(member_id); + +CREATE TABLE IF NOT EXISTS messages ( + id INTEGER PRIMARY KEY, + conversation_id INTEGER NOT NULL REFERENCES conversations(id) ON DELETE CASCADE, + author_id INTEGER NOT NULL REFERENCES members(id), + body_md TEXT NOT NULL, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + deleted_at TEXT +); + +CREATE INDEX IF NOT EXISTS messages_conversation + ON messages(conversation_id, created_at) WHERE deleted_at IS NULL; + +-- Blocking is symmetric: one row stops messages in both directions. +-- +-- The alternative — the blocker may still write, the blocked may not reply — +-- turns a safety feature into a one-way megaphone, which is worse than not +-- having one. Somebody who blocks a person and then wants to talk to them can +-- unblock. The CHECK is there because blocking yourself is meaningless and +-- would quietly disable your own inbox. +CREATE TABLE IF NOT EXISTS blocks ( + blocker_id INTEGER NOT NULL REFERENCES members(id), + blocked_id INTEGER NOT NULL REFERENCES members(id), + created_at TEXT NOT NULL DEFAULT (datetime('now')), + PRIMARY KEY (blocker_id, blocked_id), + CHECK (blocker_id <> blocked_id) +); + +CREATE INDEX IF NOT EXISTS blocks_blocked ON blocks(blocked_id); + +-- Pictures attached to a thread, a comment or a private message. -- -- The file itself lives in /data/uploads; this is the record of what it is and -- what it belongs to. `stored_name` is generated, never the name the browser @@ -69,17 +124,20 @@ CREATE TABLE IF NOT EXISTS attachments ( id INTEGER PRIMARY KEY, thread_id INTEGER REFERENCES threads(id), comment_id INTEGER REFERENCES comments(id), + message_id INTEGER REFERENCES messages(id), stored_name TEXT NOT NULL UNIQUE, original_name TEXT NOT NULL, content_type TEXT NOT NULL, bytes INTEGER NOT NULL, uploaded_by INTEGER NOT NULL REFERENCES members(id), created_at TEXT NOT NULL DEFAULT (datetime('now')), - CHECK ((thread_id IS NULL) <> (comment_id IS NULL)) + CHECK ((thread_id IS NOT NULL) + (comment_id IS NOT NULL) + + (message_id IS NOT NULL) = 1) ); CREATE INDEX IF NOT EXISTS attachments_thread ON attachments(thread_id); CREATE INDEX IF NOT EXISTS attachments_comment ON attachments(comment_id); +CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id); -- Gitea access tokens for the editor. -- diff --git a/apps/board/templates/base.html b/apps/board/templates/base.html index 594908d..ada599c 100644 --- a/apps/board/templates/base.html +++ b/apps/board/templates/base.html @@ -31,6 +31,11 @@
  • Mensajes
    • {% if g.member.role in ('owner', 'admin') %} +
  • + {# "Mensajes" is the public board, so this cannot be called that. + The badge is only drawn when there is something to see. #} + Privados{% set pending = unread_private() %}{% if pending %} {{ pending }}{% endif %} +
  • Contenido
    • {% endif %}
  • Miembros
    • diff --git a/apps/board/templates/conversation.html b/apps/board/templates/conversation.html new file mode 100644 index 0000000..6c19e86 --- /dev/null +++ b/apps/board/templates/conversation.html @@ -0,0 +1,50 @@ +{% extends "base.html" %} +{% from "_attachments.html" import attachments %} +{% block title %}{{ other.display_name if other else 'Conversación' }}{% endblock %} + +{% block main %} +
    +

    {{ other.display_name if other else 'Miembro eliminado' }}

    + Volver +
    + +{% for m in messages %} +
    +
    + {{ m.author }} · +
    +
    {{ to_html(m.body_md) }}
    + {{ attachments(images.get(m.id, [])) }} +
    +{% endfor %} + + + +{% if other is none %} +

    Esta persona ya no está en la comunidad.

    +{% elif blocked %} +

    + Hay un bloqueo entre vosotros, así que ninguno de los dos puede escribir. +

    +
    + + +
    +

    + Si el bloqueo lo puso la otra persona, esto no lo quita. +

    +{% else %} +
    + + + + + + + + +
    +{% endif %} +{% endblock %} diff --git a/apps/board/templates/inbox.html b/apps/board/templates/inbox.html new file mode 100644 index 0000000..1ecd267 --- /dev/null +++ b/apps/board/templates/inbox.html @@ -0,0 +1,43 @@ +{% extends "base.html" %} +{% block title %}Privados{% endblock %} + +{% block main %} +
    +

    Mensajes privados

    +
    + +{% if people %} +
    + + + + +
    +{% endif %} + +{% if not conversations %} +

    Todavía no tienes conversaciones privadas.

    +{% endif %} + +{% for c in conversations %} + +{% endfor %} +{% endblock %} diff --git a/apps/board/tests/test_members.py b/apps/board/tests/test_members.py index cbe204d..56edb77 100644 --- a/apps/board/tests/test_members.py +++ b/apps/board/tests/test_members.py @@ -268,8 +268,9 @@ def test_every_table_pointing_at_members_is_accounted_for(db): members.py's erase lists. Otherwise erasure breaks — and it breaks at the moment somebody exercises a right they are entitled to, which is the worst possible time to find out.""" - from apps.board.members import CLEARED_ON_ERASE, REASSIGNED_ON_ERASE - handled = REASSIGNED_ON_ERASE | CLEARED_ON_ERASE + from apps.board.members import (CLEARED_ON_ERASE, REASSIGNED_ON_ERASE, + REMOVED_ON_ERASE) + handled = REASSIGNED_ON_ERASE | CLEARED_ON_ERASE | REMOVED_ON_ERASE tables = [row["name"] for row in db.execute( "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'")] diff --git a/apps/board/tests/test_messages.py b/apps/board/tests/test_messages.py new file mode 100644 index 0000000..da59a72 --- /dev/null +++ b/apps/board/tests/test_messages.py @@ -0,0 +1,209 @@ +"""Private messages, unread marks and blocking. + +The access rule is membership of the conversation, and it is worth testing +from the outside rather than trusting the query: the failure mode is not an +error, it is somebody quietly reading correspondence that is not theirs. +""" + +from __future__ import annotations + +import io +from pathlib import Path + + +PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 64 + + +def conversation_between(client, post, a, b, sign_in): + sign_in(a) + response = post("/comunidad/privados/nueva", {"member_id": str(b)}) + return int(response.headers["Location"].rstrip("/").rsplit("/", 1)[1]) + + +# --- who may read what ---------------------------------------------------- + +def test_a_stranger_cannot_read_a_conversation(client, post, make_member, sign_in): + """404 rather than 403, deliberately: a member who is not in a conversation + should not be able to tell it apart from one that does not exist.""" + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + + sign_in(make_member("curiosa")) + assert client.get(f"/comunidad/privados/{conversation}").status_code == 404 + + +def test_a_stranger_cannot_send_into_one_either(client, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + + sign_in(make_member("curiosa")) + response = post(f"/comunidad/privados/{conversation}/enviar", {"body": "Hola"}) + assert response.status_code == 404 + + +def test_the_two_parties_can(client, db, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + post(f"/comunidad/privados/{conversation}/enviar", {"body": "Hola José"}) + + sign_in(jose) + body = client.get(f"/comunidad/privados/{conversation}").get_data(as_text=True) + assert "Hola José" in body + + +def test_opening_the_same_person_twice_reuses_the_conversation( + client, db, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + first = conversation_between(client, post, maria, jose, sign_in) + second = conversation_between(client, post, maria, jose, sign_in) + + assert first == second + assert db.execute("SELECT COUNT(*) AS n FROM conversations").fetchone()["n"] == 1 + + +# --- unread --------------------------------------------------------------- + +def test_unread_is_per_member_and_clears_on_reading(client, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + post(f"/comunidad/privados/{conversation}/enviar", {"body": "¿Vienes?"}) + + badge = '1' + + # The sender has nothing unread — their own message does not count. + assert badge not in client.get("/comunidad/privados").get_data(as_text=True) + + sign_in(jose) + assert badge in client.get("/comunidad/privados").get_data(as_text=True) + + client.get(f"/comunidad/privados/{conversation}") # opening marks it read + assert badge not in client.get("/comunidad/privados").get_data(as_text=True) + + +# --- blocking ------------------------------------------------------------- + +def test_a_block_stops_both_directions(client, post, make_member, sign_in): + """Symmetric on purpose. A block that silences only the blocked person + leaves the blocker able to keep writing, which is a megaphone, not a + safety feature.""" + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + + post(f"/comunidad/privados/bloquear/{jose}") # maria blocks jose + + assert post(f"/comunidad/privados/{conversation}/enviar", + {"body": "Otra cosa"}).status_code == 403 # …and maria too + sign_in(jose) + assert post(f"/comunidad/privados/{conversation}/enviar", + {"body": "¿Hola?"}).status_code == 403 + + +def test_a_block_is_enforced_in_the_handler_not_the_template( + client, post, make_member, sign_in): + """The form is hidden once blocked, but hiding is a courtesy. Somebody who + keeps the old page open, or crafts the request, meets the same refusal.""" + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + sign_in(jose) + post(f"/comunidad/privados/bloquear/{maria}") + + sign_in(maria) # never reloaded the page, still has the form + assert post(f"/comunidad/privados/{conversation}/enviar", + {"body": "Hola"}).status_code == 403 + + +def test_a_blocked_person_cannot_start_a_new_conversation( + client, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + sign_in(maria) + post(f"/comunidad/privados/bloquear/{jose}") + + sign_in(jose) + assert post("/comunidad/privados/nueva", {"member_id": str(maria)}).status_code == 403 + + +def test_unblocking_only_removes_your_own(client, db, post, make_member, sign_in): + """Otherwise the blocked person could lift the block that exists because + of them.""" + maria, jose = make_member("maria"), make_member("jose") + sign_in(maria) + post(f"/comunidad/privados/bloquear/{jose}") + + sign_in(jose) + post(f"/comunidad/privados/desbloquear/{maria}") + + assert db.execute("SELECT COUNT(*) AS n FROM blocks").fetchone()["n"] == 1 + + +def test_a_blocked_person_is_not_offered_in_the_list(client, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + sign_in(maria) + post(f"/comunidad/privados/bloquear/{jose}") + + assert "Jose" not in client.get("/comunidad/privados").get_data(as_text=True) + + +# --- pictures ------------------------------------------------------------- + +def test_a_picture_in_a_message_is_private_to_the_two_of_them( + app, client, db, post, make_member, sign_in): + """The board's pictures are for every member. These are not, and being + signed in is nowhere near enough of a check.""" + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + post(f"/comunidad/privados/{conversation}/enviar", + {"body": "Mira", "pictures": (io.BytesIO(PNG), "foto.png")}, + content_type="multipart/form-data") + + name = db.execute("SELECT stored_name FROM attachments").fetchone()["stored_name"] + assert client.get(f"/comunidad/media/{name}").status_code == 200 + + sign_in(jose) + assert client.get(f"/comunidad/media/{name}").status_code == 200 + + sign_in(make_member("curiosa")) + assert client.get(f"/comunidad/media/{name}").status_code == 404 + + +def test_an_empty_message_with_no_picture_is_refused(client, db, post, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + post(f"/comunidad/privados/{conversation}/enviar", {"body": " "}) + + assert db.execute("SELECT COUNT(*) AS n FROM messages").fetchone()["n"] == 0 + + +# --- erasure -------------------------------------------------------------- + +def test_erasing_a_member_takes_their_private_messages( + app, client, db, post, owner_id, make_member, sign_in): + """Unlike a thread, which survives its author as "Miembro eliminado". A + two-party exchange has no remainder to preserve, and keeping half of + somebody's erased correspondence is what erasure exists to prevent.""" + maria, jose = make_member("maria"), make_member("jose") + conversation = conversation_between(client, post, maria, jose, sign_in) + post(f"/comunidad/privados/{conversation}/enviar", + {"body": "Privado", "pictures": (io.BytesIO(PNG), "f.png")}, + content_type="multipart/form-data") + name = db.execute("SELECT stored_name FROM attachments").fetchone()["stored_name"] + + sign_in(owner_id) + post(f"/comunidad/miembros/{maria}/eliminar") + + for table in ("conversations", "conversation_members", "messages", "attachments"): + assert db.execute(f"SELECT COUNT(*) AS n FROM {table}").fetchone()["n"] == 0, table + assert not (Path(app.config["UPLOAD_DIR"]) / name).exists() + + +def test_erasing_a_member_removes_blocks_either_way( + client, db, post, owner_id, make_member, sign_in): + maria, jose = make_member("maria"), make_member("jose") + sign_in(maria) + post(f"/comunidad/privados/bloquear/{jose}") + sign_in(jose) + post(f"/comunidad/privados/bloquear/{maria}") + + sign_in(owner_id) + post(f"/comunidad/miembros/{maria}/eliminar") + + assert db.execute("SELECT COUNT(*) AS n FROM blocks").fetchone()["n"] == 0 diff --git a/apps/board/tests/test_migrations.py b/apps/board/tests/test_migrations.py new file mode 100644 index 0000000..5017ed9 --- /dev/null +++ b/apps/board/tests/test_migrations.py @@ -0,0 +1,128 @@ +"""Changing a table that already has rows in it. + +The one thing `schema.sql` cannot do. Every test here builds a database in the +*old* shape first and then runs the migration over it, because a migration +tested only against a fresh database is tested against the one case it was +never needed for. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from apps.board import migrations + +# The attachments table exactly as it shipped in Phase B, CHECK and all. Kept +# here as a literal rather than imported: the point is to reproduce what is on +# the server, and schema.sql has moved on. +OLD_SCHEMA = """ +CREATE TABLE members (id INTEGER PRIMARY KEY, gitea_login TEXT); +CREATE TABLE threads (id INTEGER PRIMARY KEY, deleted_at TEXT); +CREATE TABLE comments (id INTEGER PRIMARY KEY, deleted_at TEXT); +CREATE TABLE messages (id INTEGER PRIMARY KEY, deleted_at TEXT); +CREATE TABLE attachments ( + id INTEGER PRIMARY KEY, + thread_id INTEGER REFERENCES threads(id), + comment_id INTEGER REFERENCES comments(id), + stored_name TEXT NOT NULL UNIQUE, + original_name TEXT NOT NULL, + content_type TEXT NOT NULL, + bytes INTEGER NOT NULL, + uploaded_by INTEGER NOT NULL REFERENCES members(id), + created_at TEXT NOT NULL DEFAULT (datetime('now')), + CHECK ((thread_id IS NULL) <> (comment_id IS NULL)) +); +""" + + +@pytest.fixture +def old_db(tmp_path): + """A database as it existed before this migration, with real rows in it.""" + db = sqlite3.connect(tmp_path / "old.db", isolation_level=None) + db.row_factory = sqlite3.Row + db.executescript(OLD_SCHEMA) + db.execute("INSERT INTO members (id, gitea_login) VALUES (1, 'salvador')") + db.execute("INSERT INTO threads (id) VALUES (7)") + db.execute("INSERT INTO comments (id) VALUES (9)") + db.execute( + """INSERT INTO attachments + (thread_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (7, 'foto-abc123abc123.jpg', 'foto.jpg', 'image/jpeg', 2048, 1)""") + db.execute( + """INSERT INTO attachments + (comment_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (9, 'otra-def456def456.png', 'otra.png', 'image/png', 512, 1)""") + yield db + db.close() + + +def test_the_old_check_really_does_block_this(old_db): + """The premise the whole migration rests on, asserted rather than assumed. + + If SQLite ever lets this insert through, the rebuild is unnecessary and + this file should shrink to nothing.""" + old_db.execute("ALTER TABLE attachments ADD COLUMN message_id INTEGER") + with pytest.raises(sqlite3.IntegrityError): + old_db.execute( + """INSERT INTO attachments + (message_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (1, 'x-000000000000.png', 'x.png', 'image/png', 1, 1)""") + + +def test_the_rows_survive(old_db): + """The one that would hurt: Salvador's photo is a real file on a real + server, and a migration that drops it loses something nobody can rebuild.""" + before = old_db.execute( + "SELECT stored_name, bytes, uploaded_by FROM attachments ORDER BY id").fetchall() + + migrations.apply(old_db) + + after = old_db.execute( + "SELECT stored_name, bytes, uploaded_by FROM attachments ORDER BY id").fetchall() + assert [tuple(row) for row in after] == [tuple(row) for row in before] + + +def test_a_message_can_carry_a_picture_afterwards(old_db): + migrations.apply(old_db) + old_db.execute("INSERT INTO messages (id) VALUES (3)") + old_db.execute( + """INSERT INTO attachments + (message_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (3, 'nueva-111111111111.png', 'n.png', 'image/png', 10, 1)""") + assert old_db.execute( + "SELECT message_id FROM attachments WHERE stored_name LIKE 'nueva%'" + ).fetchone()["message_id"] == 3 + + +def test_an_attachment_still_needs_exactly_one_parent(old_db): + """The rebuilt CHECK has to be as strict as the one it replaced, in three + directions instead of two — otherwise the migration quietly removes a + constraint while appearing to widen it.""" + migrations.apply(old_db) + for columns, values in [("", ""), ("thread_id, comment_id", "7, 9")]: + with pytest.raises(sqlite3.IntegrityError): + old_db.execute( + f"""INSERT INTO attachments + ({columns + ', ' if columns else ''}stored_name, + original_name, content_type, bytes, uploaded_by) + VALUES ({values + ', ' if values else ''} + 'bad-{len(columns)}00000000000.png', 'b.png', + 'image/png', 1, 1)""") + + +def test_running_it_twice_changes_nothing(old_db): + first = migrations.apply(old_db) + second = migrations.apply(old_db) + + assert first and not second # applied once, then nothing to do + assert old_db.execute("PRAGMA user_version").fetchone()[0] == max( + number for number, _, _ in migrations.STEPS) + + +def test_a_fresh_database_skips_it(app, db): + """schema.sql already builds the new shape, so the step must find its work + done and return quietly rather than rebuilding a table it just created.""" + assert "message_id" in {row[1] for row in db.execute("PRAGMA table_info(attachments)")} + assert migrations.apply(db) == [] diff --git a/apps/board/uploads.py b/apps/board/uploads.py index 10a1a31..a80f006 100644 --- a/apps/board/uploads.py +++ b/apps/board/uploads.py @@ -28,7 +28,7 @@ import re import secrets from pathlib import Path -from flask import Blueprint, abort, current_app, send_from_directory +from flask import Blueprint, abort, current_app, g, send_from_directory from .db import get_db from .security import login_required @@ -142,8 +142,20 @@ def stage(files) -> list[dict]: return staged +def remove(stored_name: str) -> None: + """Take a picture off disk. Missing is not an error. + + Called after a row has already gone, so the file being absent means an + earlier attempt got this far — which is the state we wanted anyway. + """ + if not STORED_NAME.match(stored_name): + return + directory().joinpath(stored_name).unlink(missing_ok=True) + + def save(staged: list[dict], member_id: int, - thread_id: int | None = None, comment_id: int | None = None) -> None: + thread_id: int | None = None, comment_id: int | None = None, + message_id: int | None = None) -> None: """Write the files, then record them. In that order. A row pointing at a file that does not exist renders as a broken image on @@ -155,11 +167,12 @@ def save(staged: list[dict], member_id: int, (folder / item["stored_name"]).write_bytes(item["data"]) get_db().execute( """INSERT INTO attachments - (thread_id, comment_id, stored_name, original_name, - content_type, bytes, uploaded_by) - VALUES (?, ?, ?, ?, ?, ?, ?)""", - (thread_id, comment_id, item["stored_name"], item["original_name"], - item["content_type"], len(item["data"]), member_id), + (thread_id, comment_id, message_id, stored_name, + original_name, content_type, bytes, uploaded_by) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)""", + (thread_id, comment_id, message_id, item["stored_name"], + item["original_name"], item["content_type"], len(item["data"]), + member_id), ) @@ -171,6 +184,10 @@ def for_comments(comment_ids: list[int]) -> dict[int, list]: return _grouped("comment_id", comment_ids) +def for_messages(message_ids: list[int]) -> dict[int, list]: + return _grouped("message_id", message_ids) + + def _grouped(column: str, ids: list[int]) -> dict[int, list]: """One query for a whole page rather than one per comment.""" if not ids: @@ -187,6 +204,14 @@ def _grouped(column: str, ids: list[int]) -> dict[int, list]: return grouped +def _in_conversation(conversation_id: int) -> bool: + return get_db().execute( + """SELECT 1 FROM conversation_members + WHERE conversation_id = ? AND member_id = ?""", + (conversation_id, g.member["id"]), + ).fetchone() is not None + + @bp.route("/media/") @login_required def serve(name: str): @@ -199,16 +224,22 @@ def serve(name: str): if not STORED_NAME.match(name): abort(404) row = get_db().execute( - """SELECT a.content_type + """SELECT a.content_type, m.conversation_id FROM attachments a - LEFT JOIN threads t ON t.id = a.thread_id - LEFT JOIN comments c ON c.id = a.comment_id + LEFT JOIN threads t ON t.id = a.thread_id + LEFT JOIN comments c ON c.id = a.comment_id + LEFT JOIN messages m ON m.id = a.message_id WHERE a.stored_name = ? - AND COALESCE(t.deleted_at, c.deleted_at) IS NULL""", + AND COALESCE(t.deleted_at, c.deleted_at, m.deleted_at) IS NULL""", (name,), ).fetchone() if row is None: abort(404) + # A picture on the board is for every member; one in a private message is + # for the two people in that conversation and nobody else. Being signed in + # is the whole check for the first and not nearly enough for the second. + if row["conversation_id"] is not None and not _in_conversation(row["conversation_id"]): + abort(404) # mimetype from our own column, never guessed from the name on disk, and # paired with the X-Content-Type-Options: nosniff set in app.py. return send_from_directory(directory(), name, mimetype=row["content_type"]) diff --git a/docs/server-setup.md b/docs/server-setup.md index 1838054..bf3dc5d 100644 --- a/docs/server-setup.md +++ b/docs/server-setup.md @@ -613,6 +613,51 @@ Limits: 4 images per message, and `BOARD_UPLOAD_MAX_BYTES` (8MB by default) each. Adding a picture to a post *after* publishing it means posting a reply — editing changes the words, and leaves the pictures alone. +### 11.10 Private messages (`/comunidad/privados/`) + +An inbox between two members: conversations, unread badges, photos, blocking. +Deliberately not live chat — that needs a connection held open per signed-in +member, which gunicorn's sync workers cannot do, and it would be the first +thing on this box with a real scaling limit. + +**Blocking is symmetric.** One block stops messages in both directions, and +either person can only remove their own. A block that silenced just the blocked +person would leave the blocker able to keep writing, which is a megaphone +rather than a safety feature. + +**Erasing a member deletes their private messages, both sides.** A thread +outlives its author as *Miembro eliminado*, because other people replied and +the conversation would lose its shape; a two-party exchange has no such +remainder. This does destroy the other person's copy — the uncomfortable half +of the choice, and deliberate. + +### 11.11 Schema changes: `PRAGMA user_version` + +`schema.sql` is all `CREATE TABLE IF NOT EXISTS`, which handles exactly one +kind of change — a brand-new table — and silently ignores every other. Until +private messages, every change happened to be a new table, so nothing noticed. + +`apps/board/migrations.py` holds numbered steps applied once each, in order, +recorded in SQLite's own `user_version`. `init_db` runs the schema first and +the migrations second: on an empty database the schema builds the current +shape and each step finds its work done; on an existing one the schema adds +what is new and the steps fix up what it could not touch. + +**Never edit a step that has shipped**, and never renumber one. A server that +has run it will not run it again, so a correction is a new step. + +The first step rebuilds `attachments` so a picture can belong to a private +message. It has to be a rebuild rather than an `ALTER`, because the table +carries a CHECK constraint and SQLite has no `DROP CONSTRAINT` — adding the +column works and the next insert is refused by a constraint that can no longer +be removed. That is tested against a database built in the old shape, with rows +in it, because a migration tested only on a fresh database is tested against +the one case it was never needed for. + +**After deploying this, check that an existing photo still renders.** That is +the proof the rebuild kept real rows. Back up first — `scripts/backup-board.sh` +— as with any migration. + ## 12. Make Gitea look like the site Members sign in to `/comunidad/` through Gitea, so Gitea's sign-in form and its