diff --git a/apps/board/members.py b/apps/board/members.py index 09a3ce9..e327200 100644 --- a/apps/board/members.py +++ b/apps/board/members.py @@ -74,6 +74,24 @@ def transfer_ownership(db: sqlite3.Connection, owner_id: int, target_id: int) -> raise +# Every column pointing at members(id) that does not cascade has to be dealt +# with before the row can go, or SQLite refuses the delete and the admin gets a +# 500 with nothing to read. These two sets are the list, and +# test_members.py checks them against the schema's actual foreign keys — so a +# table added later fails a test here rather than a button in production. That +# is not hypothetical: `attachments` was added and forgotten, and the first +# person who tried to remove a member who had posted a photo got the 500. +REASSIGNED_ON_ERASE = { + ("threads", "author_id"), + ("comments", "author_id"), + # The picture belongs to the thread, which survives as "Miembro eliminado", + # so it is reassigned rather than deleted, exactly like the words around it. + # Removing the picture itself means deleting the post it is attached to. + ("attachments", "uploaded_by"), +} +CLEARED_ON_ERASE = {("members", "created_by")} + + def erase_member(db: sqlite3.Connection, member_id: int) -> None: """Remove a member and their personal data, keeping the conversation intact. @@ -84,9 +102,15 @@ def erase_member(db: sqlite3.Connection, member_id: int) -> None: ghost = tombstone_id(db) db.execute("BEGIN IMMEDIATE") try: - db.execute("UPDATE threads SET author_id = ? WHERE author_id = ?", (ghost, member_id)) - db.execute("UPDATE comments SET author_id = ? WHERE author_id = ?", (ghost, member_id)) - db.execute("UPDATE members SET created_by = NULL WHERE created_by = ?", (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. + for table, column in sorted(REASSIGNED_ON_ERASE): + db.execute(f"UPDATE {table} SET {column} = ? WHERE {column} = ?", + (ghost, member_id)) + for table, column in sorted(CLEARED_ON_ERASE): + db.execute(f"UPDATE {table} SET {column} = NULL WHERE {column} = ?", + (member_id,)) db.execute("DELETE FROM members WHERE id = ? AND role != 'owner'", (member_id,)) db.execute("COMMIT") except Exception: diff --git a/apps/board/tests/test_members.py b/apps/board/tests/test_members.py index 13f4e64..cbe204d 100644 --- a/apps/board/tests/test_members.py +++ b/apps/board/tests/test_members.py @@ -227,3 +227,61 @@ def test_ticking_it_anyway_still_creates_nothing(app, client, db, post, owner_id assert "GITEA_ADMIN_TOKEN" in response.get_data(as_text=True) assert db.execute("SELECT 1 FROM members WHERE gitea_login = 'maria'").fetchone() is None + + +# --- erasing somebody who has left traces -------------------------------- + +def test_erasing_a_member_who_posted_a_picture(app, db, post, owner_id, make_member, sign_in): + """This failed in production with a 500 the first time it was tried. + + `attachments.uploaded_by` is NOT NULL and does not cascade, so with a + picture in the database SQLite refuses to delete the member — and the admin + sees "Internal Server Error" with nothing to act on.""" + author = make_member("saliente") + db.execute("INSERT INTO threads (author_id, title, body_md) VALUES (?, 'Hola', 'Texto')", + (author,)) + db.execute( + """INSERT INTO attachments + (thread_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (1, 'foto-abc123abc123.jpg', 'foto.jpg', 'image/jpeg', 10, ?)""", + (author,), + ) + sign_in(owner_id) + + response = post(f"/comunidad/miembros/{author}/eliminar") + + assert response.status_code == 302 + assert db.execute("SELECT 1 FROM members WHERE id = ?", (author,)).fetchone() is None + # The picture stays with the thread, which survives as "Miembro eliminado" — + # the same rule the words follow. Deleting the post removes the picture. + row = db.execute( + """SELECT m.display_name FROM attachments a JOIN members m ON m.id = a.uploaded_by""" + ).fetchone() + assert row["display_name"] == "Miembro eliminado" + + +def test_every_table_pointing_at_members_is_accounted_for(db): + """The guard that makes the bug above unrepeatable. + + Read out of the live schema rather than written down twice: any future + table with a foreign key to members(id) either cascades, or is named in + 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 + + tables = [row["name"] for row in db.execute( + "SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'")] + unhandled = [ + f"{table}.{fk['from']}" + for table in tables + for fk in db.execute(f"PRAGMA foreign_key_list('{table}')").fetchall() + if fk["table"] == "members" + and (fk["on_delete"] or "").upper() != "CASCADE" + and (table, fk["from"]) not in handled + ] + assert not unhandled, ( + "these point at members(id), do not cascade, and erase_member does not " + f"touch them, so erasing a member will fail: {unhandled}" + )