Add tests to ensure publicity syncing works for +s modes (#1698)

* PgDataStore: Fix syntax of array parameters

Arrays passed as query parameters become PostgreSQL arrays. However,
`foo IN $1` is not valid syntax, as `IN` can only be used with
subqueries (`foo IN (SELECT ...)`) and lists (`foo IN ($1, $2)`), which
require parentheses.

`foo IN ($1)` is interpreted as a list and the expression is compared
with the whole array, not checked against its members.

The correct syntax for checking if a value matches any member of an
array parameter is `foo = ANY($1)`, described in
https://www.postgresql.org/docs/15/functions-comparisons.html#id-1.5.8.30.16.

Fix `getIrcChannelsForRoomIds` and `getRoomsVisibility`. While we're at
it, replace `getMappingsForChannelByOrigin`'s workable but awkward
building of a parameter list.

Signed-off-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>

* Add tests for publicity-syncing

* Await publicity

* Refactor to fix publicity sync

* Refactor

* changelog

* Ensure PgDataStore returns private for missing entries

---------

Signed-off-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>
Co-authored-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>

changelog.d/1660.bugfix

@@ -0,0 +1 @@
+Fix SQL syntax errors in the PostgreSQL data store.

changelog.d/1698.misc

@@ -0,0 +1 @@
+Add tests that cover publicity syncing behaviour.

spec/integ/publicity.spec.ts

@@ -0,0 +1,65 @@
+/* eslint-disable @typescript-eslint/no-non-null-assertion */
+import { Request } from "matrix-appservice-bridge";
+import { IrcBridge } from "../../lib/bridge/IrcBridge";
+import { BridgeRequest } from "../../lib/models/BridgeRequest";
+import envBundle from "../util/env-bundle";
+
+describe("Publicity Syncing", function() {
+    const {env, roomMapping, test} = envBundle();
+
+    beforeEach(async () => {
+        await test.beforeEach(env);
+
+        env.ircMock._autoConnectNetworks(
+            roomMapping.server, roomMapping.botNick, roomMapping.server
+        );
+        env.ircMock._autoJoinChannels(
+            roomMapping.server, roomMapping.botNick, roomMapping.channel
+        );
+
+        await test.initEnv(env);
+    });
+
+    afterEach(async () => test.afterEach(env));
+
+    it("should ensure rooms with no visibility state are private", async () => {
+        const ircBridge: IrcBridge = env.ircBridge as IrcBridge;
+        const store = ircBridge.getStore();
+        const roomVis = await store.getRoomsVisibility([roomMapping.roomId]);
+        expect(roomVis.get(roomMapping.roomId)).toBe('private');
+    });
+
+    it("should ensure rooms with +s channels are set to private visibility", async () => {
+        const ircBridge: IrcBridge = env.ircBridge as IrcBridge;
+        const store = ircBridge.getStore();
+        // Ensure opposite state
+        await store.setRoomVisibility(roomMapping.roomId, "public");
+        const req = new BridgeRequest(new Request({
+            data: {
+                isFromIrc: true,
+            }
+        }));
+        const server = ircBridge.getServer(roomMapping.server)!;
+        await ircBridge.ircHandler.roomAccessSyncer.onMode(
+            req, server, roomMapping.channel, "", "s", true, null
+        );
+        const roomVis = await store.getRoomsVisibility([roomMapping.roomId]);
+        expect(roomVis.get(roomMapping.roomId)).toBe('private');
+    });
+
+    it("should ensure rooms with -s channels are set to public visibility", async () => {
+        const ircBridge: IrcBridge = env.ircBridge as IrcBridge;
+        const store = ircBridge.getStore();
+        const req = new BridgeRequest(new Request({
+            data: {
+                isFromIrc: true,
+            }
+        }));
+        const server = ircBridge.getServer(roomMapping.server)!;
+        await ircBridge.ircHandler.roomAccessSyncer.onMode(
+            req, server, roomMapping.channel, "", "s", false, null
+        );
+        const roomVis = await store.getRoomsVisibility([roomMapping.roomId]);
+        expect(roomVis.get(roomMapping.roomId)).toBe('public');
+    });
+});

src/bridge/IrcBridge.ts

@@ -802,7 +802,7 @@ export class IrcBridge {
         log.info("Connecting to IRC networks...");
         await this.connectToIrcNetworks();
 
-        promiseutil.allSettled(this.ircServers.map((server) => {
+        await promiseutil.allSettled(this.ircServers.map((server) => {
             // Call MODE on all known channels to get modes of all channels
             return Bluebird.cast(this.publicitySyncer.initModes(server));
         })).catch((err) => {

src/bridge/PublicitySyncer.ts

@@ -48,8 +48,8 @@ export class PublicitySyncer {
 
     public async initModes (server: IrcServer) {
         //Get all channels and call modes for each one
-
         const channels = await this.ircBridge.getStore().getTrackedChannelsForServer(server.domain);
+        log.info(`Syncing modes for ${channels.length} on ${server.domain}`);
         await Promise.all(channels.map((channel) =>
             this.initModeQueue.enqueue(`${channel}@${server.domain}`, {
                 channel,
@@ -61,47 +61,40 @@ export class PublicitySyncer {
     /**
      * Returns the key used when calling `updateVisibilityMap` for updating an IRC channel
      * visibility mode (+s or -s).
-     * ```
-     * // Set channel on server to be +s
-     * const key = publicitySyncer.getIRCVisMapKey(server.getNetworkId(), channel);
-     * publicitySyncer.updateVisibilityMap(true, key, true);
-     * ```
      * @param {string} networkId
      * @param {string} channel
      * @returns {string}
      */
-    public getIRCVisMapKey(networkId: string, channel: string) {
+    private getIRCVisMapKey(networkId: string, channel: string) {
         return `${networkId} ${channel}`;
     }
 
-    public updateVisibilityMap(isMode: boolean, key: string, value: boolean, channel: string, server: IrcServer) {
-        log.debug(`updateVisibilityMap: isMode:${isMode} k:${key} v:${value} chan:${channel} srv:${server.domain}`);
+    /**
+     * Update the visibility of a given channel
+     *
+     * @param isSecret Is the channel secret.
+     * @param channel Channel name
+     * @param server Server the channel is part of.
+     * @returns If the channel publicity was synced.
+     */
+    public async updateVisibilityMap(channel: string, server: IrcServer, isSecret: boolean): Promise<boolean> {
+        const key = this.getIRCVisMapKey(server.getNetworkId(), channel);
+        log.debug(`updateVisibilityMap ${key} isSecret:${isSecret}`);
         let hasChanged = false;
-        if (isMode) {
-            if (typeof value !== 'boolean') {
-                throw new Error('+s state must be indicated with a boolean');
-            }
-            if (this.visibilityMap.channelIsSecret.get(key) !== value) {
-                this.visibilityMap.channelIsSecret.set(key, value);
-                hasChanged = true;
-            }
-        }
-        else {
-            if (typeof value !== 'string' || (value !== "private" && value !== "public")) {
-                throw new Error('Room visibility must = "private" | "public"');
-            }
-
-            if (this.visibilityMap.roomVisibilities.get(key) !== value) {
-                this.visibilityMap.roomVisibilities.set(key, value);
-                hasChanged = true;
-            }
+        if (this.visibilityMap.channelIsSecret.get(key) !== isSecret) {
+            this.visibilityMap.channelIsSecret.set(key, isSecret);
+            hasChanged = true;
         }
 
         if (hasChanged) {
-            this.solveVisibility(channel, server).catch((err: Error) => {
-                log.error(`Failed to sync publicity for ${channel}: ` + err.message);
-            });
+            try {
+                await this.solveVisibility(channel, server)
+            }
+            catch (err) {
+                throw Error(`Failed to sync publicity for ${channel}: ${err.message}`);
+            }
         }
+        return hasChanged;
     }
 
     /* Solve any inconsistencies between the currently known state of channels '+s' modes
@@ -126,17 +119,8 @@ export class PublicitySyncer {
         this.visibilityMap.mappings = new Map();
 
         // Update rooms to correct visibilities
-        let currentStates: Map<string, MatrixDirectoryVisibility> = new Map();
-
-        // Assume private by default
-        for (const roomId of roomIds) {
-            currentStates.set(roomId, "private");
-        }
-
-        currentStates = {
-            ...currentStates,
-            ...await this.ircBridge.getStore().getRoomsVisibility(roomIds),
-        };
+        const currentStates: Map<string, MatrixDirectoryVisibility>
+            = await this.ircBridge.getStore().getRoomsVisibility(roomIds);
 
         const correctState = this.visibilityMap.channelIsSecret.get(visKey) ? 'private' : 'public';
 

src/bridge/RoomAccessSyncer.ts

@@ -423,17 +423,23 @@ export class RoomAccessSyncer {
                 req.log.info("Not syncing publicity: shouldPublishRooms is false");
                 return;
             }
-            const key = this.ircBridge.publicitySyncer.getIRCVisMapKey(
-                server.getNetworkId(), channel
-            );
 
+            try {
+                // Update the visibility for all rooms connected to this channel
+                await this.ircBridge.publicitySyncer.updateVisibilityMap(
+                    channel, server, enabled,
+                );
+            }
+            catch (ex) {
+                log.error(
+                    `Failed to update visibility map for ${channel} ${server.getNetworkId()}: ${ex}`
+                );
+            }
+
+            // Only set this after we've applied the changes.
             matrixRooms.map((room) => {
                 this.ircBridge.getStore().setModeForRoom(room.getId(), "s", enabled);
             });
-            // Update the visibility for all rooms connected to this channel
-            this.ircBridge.publicitySyncer.updateVisibilityMap(
-                true, key, enabled, channel, server,
-            );
         }
         // "k" and "i"
         await Promise.all(matrixRooms.map((room) =>

src/datastore/NedbDataStore.ts

@@ -305,8 +305,8 @@ export class NeDBDataStore implements DataStore {
      */
     public async getMatrixRoomsForChannel(server: IrcServer, channel: string): Promise<MatrixRoom[]> {
         const ircRoom = new IrcRoom(server, channel);
-        return await this.roomStore.getLinkedMatrixRooms(
-            IrcRoom.createId(ircRoom.getServer(), ircRoom.getChannel())
+        return this.roomStore.getLinkedMatrixRooms(
+            ircRoom.getId()
         );
     }
 

src/datastore/postgres/PgDataStore.ts

@@ -255,7 +255,7 @@ export class PgDataStore implements DataStore, ProvisioningStore {
 
     public async getIrcChannelsForRoomIds(roomIds: string[]): Promise<{ [roomId: string]: IrcRoom[] }> {
         const entries = await this.pgPool.query(
-            "SELECT room_id, irc_domain, irc_channel FROM rooms WHERE room_id IN $1",
+            "SELECT room_id, irc_domain, irc_channel FROM rooms WHERE room_id = ANY($1)",
             [roomIds]
         );
         const mapping: { [roomId: string]: IrcRoom[] } = {};
@@ -292,14 +292,14 @@ export class PgDataStore implements DataStore, ProvisioningStore {
         if (!Array.isArray(origin)) {
             origin = [origin];
         }
-        const inStatement = origin.map((_, i) => `\$${i + 3}`).join(", ");
         const entries = await this.pgPool.query<RoomRecord>(
-            `SELECT * FROM rooms WHERE irc_domain = $1 AND irc_channel = $2 AND origin IN (${inStatement})`,
+            "SELECT * FROM rooms WHERE irc_domain = $1 AND irc_channel = $2 AND origin = ANY($3)",
             [
                 server.domain,
                 // Channels must be lowercase
                 toIrcLowerCase(channel),
-            ].concat(origin));
+                origin,
+            ]);
         return entries.rows.map((e) => PgDataStore.pgToRoomEntry(e));
     }
 
@@ -663,10 +663,13 @@ export class PgDataStore implements DataStore, ProvisioningStore {
     }
 
     public async getRoomsVisibility(roomIds: string[]): Promise<Map<string, MatrixDirectoryVisibility>> {
-        const map: Map<string, MatrixDirectoryVisibility> = new Map();
-        const res = await this.pgPool.query("SELECT room_id, visibility FROM room_visibility WHERE room_id IN $1", [
-            roomIds,
-        ]);
+        const map: Map<string, MatrixDirectoryVisibility> = new Map(
+            roomIds.map(r => [r, 'private'])
+        );
+        const res = await this.pgPool.query(
+            "SELECT room_id, visibility FROM room_visibility WHERE room_id = ANY($1)",
+            [roomIds]
+        );
         for (const row of res.rows) {
             map.set(row.room_id, row.visibility ? "public" : "private");
         }