Test and fix ban timeouts, and allow global timeouts

jagerman · jagerman · commit 7315f6d1989b · 2022-01-28T20:09:32.000-04:00
Adds more tests for ban timeouts, which revealed a permission futures
bug that wouldn't properly apply updates when `banned` was NULL.

Fixing this required splitting permission updates and bans into separate
tables, which makes things easier to manage.

Moreover this allows the new user_ban_futures table to accept a NULL
room so that we can use it to also have global ban timeouts, which is
implemented here.
diff --git a/api.yaml b/api.yaml
@@ -743,7 +743,8 @@ paths:
                     user must be a server-level moderator or admin.
                     
                     
-                    The ban applies immediately to all server requests as early as possible.
+                    The ban applies immediately to all server requests initiated by the user (not
+                    just room access).
                     
                     
                     Exclusive of `rooms`.
@@ -753,13 +754,9 @@ paths:
                   nullable: true
                   example: 86400
                   description: >
-                    How long the ban should apply, in seconds.  If there is an existing ban on the
-                    user in the given rooms this updates the existing expiry to the given value. If
-                    omitted or `null` then the ban does not expire.
-                    
-                    
-                    May only be used with the `rooms` argument; timeouts are not supported on global
-                    bans.
+                    How long the ban should apply, in seconds.  If omitted (or `null`) then the ban
+                    applies forever.  If an unban was previously scheduled then it is replaced (if a
+                    timeout is given) or deleted (if no timeout is provided).
             examples:
               tworooms:
                 summary: "1-day ban from two rooms"
diff --git a/sogs/cleanup.py b/sogs/cleanup.py
@@ -94,24 +94,53 @@ def expire_nonce_history():
 def apply_permission_updates():
     with db.transaction():
         now = time.time()
-        num_applied = query(
+        num_perms = query(
             """
-            INSERT INTO user_permission_overrides (room, "user", read, write, upload, banned)
-            SELECT room, "user", read, write, upload, banned FROM user_permission_futures
+            INSERT INTO user_permission_overrides (room, "user", read, write, upload)
+            SELECT room, "user", read, write, upload
+                FROM user_permission_futures
                 WHERE at <= :now
+                ORDER BY at
             ON CONFLICT (room, "user") DO UPDATE SET
                 read = COALESCE(excluded.read, user_permission_overrides.read),
                 write = COALESCE(excluded.write, user_permission_overrides.write),
-                upload = COALESCE(excluded.upload, user_permission_overrides.upload),
-                banned = COALESCE(excluded.banned, user_permission_overrides.banned)
+                upload = COALESCE(excluded.upload, user_permission_overrides.upload)
             """,
             now=now,
         ).rowcount
-        if not num_applied:
-            return 0
 
-        query("DELETE FROM user_permission_futures WHERE at <= :now", now=now)
+        if num_perms > 0:
+            query("DELETE FROM user_permission_futures WHERE at <= :now", now=now)
+
+        num_room_bans, num_user_bans = 0, 0
+        for uid, rid, banned in query(
+            'SELECT "user", room, banned FROM user_ban_futures WHERE at <= :now ORDER BY at',
+            now=now,
+        ):
+            if rid is None:
+                query("UPDATE users SET banned = :b WHERE id = :u", u=uid, b=banned)
+                num_user_bans += 1
+            else:
+                query(
+                    """
+                INSERT INTO user_permission_overrides (room, "user", banned)
+                VALUES (:r, :u, :b)
+                ON CONFLICT (room, "user") DO UPDATE SET banned = excluded.banned
+                """,
+                    r=rid,
+                    u=uid,
+                    b=banned,
+                )
+                num_room_bans += 1
+
+        if num_room_bans > 0 or num_user_bans > 0:
+            query("DELETE FROM user_ban_futures WHERE at <= :now", now=now)
+
+        num_applied = num_perms + num_room_bans + num_user_bans
 
     if num_applied > 0:
-        app.logger.info("Applied {} scheduled user permission updates".format(num_applied))
+        app.logger.info(
+            f"Applied {num_applied} permission updates ({num_perms} perms; "
+            f"{num_room_bans} room (un)bans; {num_user_bans} global (un)bans)"
+        )
     return num_applied
diff --git a/sogs/db.py b/sogs/db.py
@@ -128,6 +128,7 @@ def database_init():
         create_message_details_deleter,
         check_for_hacks,
         seqno_etc_updates,
+        user_perm_future_updates,
     ):
         with transaction(conn):
             if migrate(conn):
@@ -479,6 +480,76 @@ def seqno_etc_updates(conn):
     return True
 
 
+def user_perm_future_updates(conn):
+    """
+    Break up user_permission_futures to not be (room,user) unique, and to move ban futures to a
+    separate table.
+    """
+
+    if 'user_ban_futures' in metadata.tables:
+        return False
+
+    logging.warning("Updating user_permission_futures")
+
+    if engine.name == 'sqlite':
+        # Under sqlite we have to drop and recreate the whole thing.  (Since we didn't have a
+        # release out that was using futures yet, we don't bother trying to migrate data).
+        conn.execute("DROP TABLE user_permission_futures")
+        conn.execute(
+            """
+CREATE TABLE user_permission_futures (
+    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    user INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
+    read BOOLEAN, /* Set this value @ at, if non-null */
+    write BOOLEAN, /* Set this value @ at, if non-null */
+    upload BOOLEAN /* Set this value @ at, if non-null */
+)
+"""
+        )
+        conn.execute("CREATE INDEX user_permission_futures_at ON user_permission_futures(at)")
+        conn.execute(
+            """
+CREATE INDEX user_permission_futures_room_user ON user_permission_futures(room, user)
+"""
+        )
+
+        conn.execute(
+            """
+CREATE TABLE user_ban_futures (
+    room INTEGER REFERENCES rooms ON DELETE CASCADE,
+    user INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
+    banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
+);
+"""
+        )
+        conn.execute("CREATE INDEX user_ban_futures_at ON user_ban_futures(at)")
+        conn.execute("CREATE INDEX user_ban_futures_room_user ON user_ban_futures(room, user)")
+
+    else:  # postgresql
+        conn.execute(
+            """
+CREATE TABLE user_ban_futures (
+    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    "user" INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
+    banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
+);
+CREATE INDEX user_ban_futures_at ON user_ban_futures(at);
+CREATE INDEX user_ban_futures_room_user ON user_ban_futures(room, "user");
+
+INSERT INTO user_ban_futures (room, "user", at, banned)
+    SELECT room, "user", at, banned FROM user_permission_futures WHERE banned is NOT NULL;
+
+ALTER TABLE user_permission_futures DROP PRIMARY KEY;
+ALTER TABLE user_permission_futures DROP COLUMN banned;
+"""
+        )
+
+    return True
+
+
 def create_admin_user(dbconn):
     """
     We create a dummy user (with id 0) for system tasks such as changing moderators from
diff --git a/sogs/model/room.py b/sogs/model/room.py
@@ -1035,28 +1035,36 @@ def ban_user(self, to_ban: User, *, mod: User, timeout: Optional[float] = None):
                 """
                 INSERT INTO user_permission_overrides (room, "user", banned, moderator, admin)
                     VALUES (:r, :ban, TRUE, FALSE, FALSE)
-                ON CONFLICT (room, user) DO
+                ON CONFLICT (room, "user") DO
                     UPDATE SET banned = TRUE, moderator = FALSE, admin = FALSE
                 """,
                 r=self.id,
                 ban=to_ban.id,
             )
 
+            # Replace (or remove) an existing scheduled unban:
+            query(
+                'DELETE FROM user_ban_futures WHERE room = :r AND "user" = :u AND NOT banned',
+                r=self.id,
+                u=to_ban.id,
+            )
             if timeout:
                 query(
                     """
-                    INSERT INTO user_permission_futures (room, "user", at, banned)
-                        VALUES (:r, :banned, :at, FALSE)
+                    INSERT INTO user_ban_futures
+                    (room, "user", banned, at) VALUES (:r, :u, FALSE, :at)
                     """,
                     r=self.id,
-                    banned=to_ban.id,
+                    u=to_ban.id,
                     at=time.time() + timeout,
                 )
 
         if to_ban.id in self._perm_cache:
             del self._perm_cache[to_ban.id]
 
-        app.logger.debug(f"Banned {to_ban} from {self} (banned by {mod})")
+        app.logger.debug(
+            f"Banned {to_ban} from {self} {f'for {timeout}s ' if timeout else ''}(banned by {mod})"
+        )
 
     def unban_user(self, to_unban: User, *, mod: User):
         """
diff --git a/sogs/model/user.py b/sogs/model/user.py
@@ -5,6 +5,7 @@
 from ..web import app
 from .exc import NoSuchUser, BadPermission
 
+from typing import Optional
 import time
 
 
@@ -168,11 +169,18 @@ def remove_moderator(self, *, removed_by: User, remove_admin_only: bool = False)
         self.global_admin = False
         self.global_moderator = False
 
-    def ban(self, *, banned_by: User):
-        """Globally bans this user from the server; can only be applied by a global moderator or
-        global admin, and cannot be applied to another global moderator or admin (to prevent
-        accidental mod/admin banning; to ban them, first explicitly remove them as moderator/admin
-        and then ban)."""
+    def ban(self, *, banned_by: User, timeout: Optional[float] = None):
+        """
+        Globally bans this user from the server; can only be applied by a global moderator or global
+        admin, and cannot be applied to another global moderator or admin (to prevent accidental
+        mod/admin banning; to ban them, first explicitly remove them as moderator/admin and then
+        ban).
+
+        timeout should be None for a non-expiring ban and otherwise should be the duration of the
+        ban, in seconds; an unban will be scheduled to occur after the interval.  In either case,
+        any existing scheduled global unbans for this user will be deleted (and replaced, if a new
+        timeout is provided).
+        """
 
         if not banned_by.global_moderator:
             app.logger.warning(f"Cannot ban {self}: {banned_by} is not a global mod/admin")
@@ -182,17 +190,43 @@ def ban(self, *, banned_by: User):
             app.logger.warning(f"Cannot ban {self}: user is a global moderator/admin")
             raise BadPermission()
 
-        query("UPDATE users SET banned = TRUE WHERE id = :u", u=self.id)
-        app.logger.debug(f"{banned_by} globally banned {self}")
+        with db.transaction():
+            query("UPDATE users SET banned = TRUE WHERE id = :u", u=self.id)
+            query(
+                'DELETE FROM user_ban_futures WHERE room IS NULL AND "user" = :u AND NOT banned',
+                u=self.id,
+            )
+
+            if timeout:
+                query(
+                    """
+                    INSERT INTO user_ban_futures
+                    ("user", room, banned, at) VALUES (:u, NULL, FALSE, :at)
+                    """,
+                    u=self.id,
+                    at=time.time() + timeout,
+                )
+
+        app.logger.debug(
+            f"{banned_by} globally banned {self}{f' for {timeout}s' if timeout else ''}"
+        )
         self.banned = True
 
     def unban(self, *, unbanned_by: User):
-        """Undoes a global ban.  `unbanned_by` must be a global mod/admin."""
+        """
+        Undoes a global ban.  `unbanned_by` must be a global mod/admin.
+
+        Any currently scheduled global ban futures for this user will be removed as well.
+        """
         if not unbanned_by.global_moderator:
             app.logger.warning(f"Cannot unban {self}: {unbanned_by} is not a global mod/admin")
             raise BadPermission()
 
         query("UPDATE users SET banned = FALSE WHERE id = :u", u=self.id)
+        query(
+            'DELETE FROM user_ban_futures WHERE room IS NULL AND "user" = :u AND banned', u=self.id
+        )
+
         app.logger.debug(f"{unbanned_by} removed global ban on {self}")
         self.banned = False
 
diff --git a/sogs/routes/users.py b/sogs/routes/users.py
@@ -156,16 +156,12 @@ def ban_user(sid):
         app.logger.warning("Invalid ban request: timeout must be numeric")
         abort(http.BAD_REQUEST)
 
-    if timeout and global_ban:
-        app.logger.warning("Invalid ban request: global server bans do not support timeouts")
-        abort(http.BAD_REQUEST)
-
     if rooms:
         with db.transaction():
             for room in rooms:
                 room.ban_user(to_ban=user, mod=g.user, timeout=timeout)
     else:
-        user.ban(banned_by=g.user)
+        user.ban(banned_by=g.user, timeout=timeout)
 
     return {}
 
diff --git a/sogs/schema.pgsql b/sogs/schema.pgsql
@@ -367,11 +367,22 @@ CREATE TABLE user_permission_futures (
     at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
     read BOOLEAN, /* Set this value @ at, if non-null */
     write BOOLEAN, /* Set this value @ at, if non-null */
-    upload BOOLEAN, /* Set this value @ at, if non-null */
-    banned BOOLEAN, /* Set this value @ at, if non-null */
-    PRIMARY KEY(room, "user")
+    upload BOOLEAN /* Set this value @ at, if non-null */
 );
 CREATE INDEX user_permissions_future_at ON user_permission_futures(at);
+CREATE INDEX user_permissions_future_room_user ON user_permission_futures(room, "user");
+
+-- Similar to the above, but for ban/unbans.  For example to implement a 2-day ban you would set
+-- their user_permissions.banned to TRUE then add a row here with banned = FALSE to schedule the
+-- unban.  (You can also schedule a future *ban* here, but the utility of that is less clear).
+CREATE TABLE user_ban_futures (
+    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    "user" INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
+    banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
+);
+CREATE INDEX user_ban_futures_at ON user_ban_futures(at);
+CREATE INDEX user_ban_futures_room_user ON user_ban_futures(room, "user");
 
 
 -- Nonce tracking to prohibit request signature nonce reuse (thus prevent replay attacks)
diff --git a/sogs/schema.sqlite b/sogs/schema.sqlite
@@ -322,16 +322,27 @@ FROM
 -- Or to implement a join delay you could set room defaults to false then insert a value here to be
 -- applied after a delay.
 CREATE TABLE user_permission_futures (
-    room INTEGER NOT NULL REFERENCES rooms(id) ON DELETE CASCADE,
-    user INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
+    room INTEGER NOT NULL REFERENCES rooms ON DELETE CASCADE,
+    user INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
     at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
     read BOOLEAN, /* Set this value @ at, if non-null */
     write BOOLEAN, /* Set this value @ at, if non-null */
-    upload BOOLEAN, /* Set this value @ at, if non-null */
-    banned BOOLEAN, /* Set this value @ at, if non-null */
-    PRIMARY KEY(room, user)
-) WITHOUT ROWID;
-CREATE INDEX user_permissions_future_at ON user_permission_futures(at);
+    upload BOOLEAN /* Set this value @ at, if non-null */
+);
+CREATE INDEX user_permission_futures_at ON user_permission_futures(at);
+CREATE INDEX user_permission_futures_room_user ON user_permission_futures(room, "user");
+
+-- Similar to the above, but for ban/unbans.  For example to implement a 2-day ban you would set
+-- their user_permissions.banned to TRUE then add a row here with banned = FALSE to schedule the
+-- unban.  (You can also schedule a future *ban* here, but the utility of that is less clear).
+CREATE TABLE user_ban_futures (
+    room INTEGER REFERENCES rooms ON DELETE CASCADE,
+    user INTEGER NOT NULL REFERENCES users ON DELETE CASCADE,
+    at FLOAT NOT NULL, /* when the change should take effect (unix epoch) */
+    banned BOOLEAN NOT NULL /* if true then ban at `at`, if false then unban */
+);
+CREATE INDEX user_ban_futures_at ON user_ban_futures(at);
+CREATE INDEX user_ban_futures_room_user ON user_ban_futures(room, user);
 
 
 -- Nonce tracking to prohibit request signature nonce reuse (thus prevent replay attacks)
diff --git a/tests/test_user_routes.py b/tests/test_user_routes.py
