Merge pull request #47 from jagerman/bans

Bans, unbans

Author: jagerman

api.yaml

@@ -759,9 +759,14 @@ paths:
   /user/{sessionId}/ban:
     post:
       tags: [Users]
-      summary: Bans or unbans a user.
+      summary: Bans a user.
       description: >
-        Applies or removes a ban of a user from specific rooms, or from the server globally.
+        Applies a ban of a user from specific rooms, or from the server globally.
+        
+        
+        The invoking user must have moderator (or admin) permission in all given rooms when
+        specifying `rooms`, and must be a global server moderator (or admin) if using the `global`
+        parameter.
         
         
         Note that the given session ID does not have to exist: it is possible to preemptively ban
@@ -774,7 +779,7 @@ paths:
       parameters:
       - $ref: "#/components/parameters/pathSessionId"
       requestBody:
-        description: Details of the ban to apply. To unban a user, specify a negative timeout.
+        description: Details of the ban to apply.
         required: true
         content:
           application/json:
@@ -791,6 +796,12 @@ paths:
                     be a moderator (or admin) of all of the given rooms.
                     
                     
+                    You can specify a single element list `["*"]` to ban the user from all rooms on
+                    the server to which the calling user has moderator permissions.  This differs
+                    from a global ban in that it doesn't apply to non-moderator-permission rooms,
+                    nor does it apply to newly created rooms or non-room endpoints.
+                    
+                    
                     Exclusive of `global`.
                 global:
                   type: boolean
@@ -799,22 +810,20 @@ paths:
                     user must be a server-level moderator or admin.
                     
                     
+                    The ban applies immediately to all server requests initiated by the user (not
+                    just room access).
+                    
+                    
                     Exclusive of `rooms`.
                 timeout:
                   type: number
                   format: double
                   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 or globally this updates the existing expiry to the
-                    given value. If omitted or `null` the ban does not expire.
-                    
-                    If this value is set to a negative value (`-1` is suggested) then any existing
-                    bans for this user are *removed* from the given rooms/server.  Note, however,
-                    that server bans and room bans are independent: removing a server-level ban does
-                    not remove room-specific bans, and removing a room-level ban will not grant room
-                    access to a user who also has a server-level ban.
+                    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"
@@ -825,21 +834,77 @@ paths:
                 summary: "Permanent server ban"
                 value:
                   global: true
-                  timeout: null
-                  delete_all: true
-              unban:
-                summary: "Unban a user from a room"
-                value:
-                  rooms: ["lokinet"]
-                  global: false,
-                  timeout: -1
       responses:
         200:
           description: Ban applied successfully.
           content: {}
         403:
           description: >
-            Permission denied.  The user attempting to set the ban does not have moderator
+            Permission denied.  The user attempting to set the ban does not have the required
+            moderator permissions for one or more of the given rooms (or server moderator permission
+            for a global ban).
+          content: {}
+  /user/{sessionId}/unban:
+    post:
+      tags: [Users]
+      summary: Removes a user ban.
+      description: >
+        Removes a room-specific or global ban of a user.
+        
+        
+        Note that removing a room-specific ban does not affect an existing global ban, and removing
+        a global ban does not affect existing room-specific bans.
+      parameters:
+      - $ref: "#/components/parameters/pathSessionId"
+      requestBody:
+        description: Details of the ban to remove.
+        required: true
+        content:
+          application/json:
+            schema:
+              type: object
+              properties:
+                rooms:
+                  type: array
+                  items:
+                    $ref: "#/components/schemas/RoomToken"
+                  minItems: 1
+                  description: >
+                    List of room tokens from which the ban should be removed (if present). The
+                    invoking user must be a moderator (or admin) of all of the given rooms.
+                    
+                    
+                    You can specify a single element list `["*"]` to unban the user from all rooms
+                    on the server to which the calling user has moderator permissions.  This isn't
+                    the same as removing a global ban; this just removes any room-specific bans that
+                    may currently apply from moderated rooms.
+                    
+                    
+                    Exclusive of `global`.
+                global:
+                  type: boolean
+                  description: >
+                    If true then remove a global server ban of this user. The invoking user must be
+                    a server-level moderator or admin.
+                    
+                    
+                    Exclusive of `rooms`.
+            examples:
+              tworooms:
+                summary: "Remove ban from two rooms"
+                value:
+                  rooms: ["session", "lokinet"]
+              permaban:
+                summary: "Remove server ban"
+                value:
+                  global: true
+      responses:
+        200:
+          description: Ban removed successfully.
+          content: {}
+        403:
+          description: >
+            Permission denied.  The user attempting to remove the ban does not have moderator
             permissions for one or more of the given rooms (or server moderator permission for a
             global ban).
           content: {}

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

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

sogs/model/room.py

@@ -1040,28 +1040,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):
         """