Implement changes to MSC2285 (private read receipts) (#2221)

* Add `ReceiptType`

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Implement changes to MSC2285

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Improve tests

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Apply suggestions from review

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Update `getEventReadUpTo()` to handle private read receipts

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Write tests for `getEventReadUpTo()`

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Give `getReadReceiptForUserId()` a JSDOC

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Types!

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

* Try to use receipt `ts`s

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>

spec/unit/matrix-client.spec.ts

@@ -29,6 +29,7 @@ import {
 import { MEGOLM_ALGORITHM } from "../../src/crypto/olmlib";
 import { EventStatus, MatrixEvent } from "../../src/models/event";
 import { Preset } from "../../src/@types/partials";
+import { ReceiptType } from "../../src/@types/read_receipts";
 import * as testUtils from "../test-utils/test-utils";
 import { makeBeaconInfoContent } from "../../src/content-helpers";
 import { M_BEACON_INFO } from "../../src/@types/beacon";
@@ -992,6 +993,46 @@ describe("MatrixClient", function() {
         });
     });
 
+    describe("read-markers and read-receipts", () => {
+        it("setRoomReadMarkers", () => {
+            client.setRoomReadMarkersHttpRequest = jest.fn();
+            const room = {
+                hasPendingEvent: jest.fn().mockReturnValue(false),
+                addLocalEchoReceipt: jest.fn(),
+            };
+            const rrEvent = new MatrixEvent({ event_id: "read_event_id" });
+            const rpEvent = new MatrixEvent({ event_id: "read_private_event_id" });
+            client.getRoom = () => room;
+
+            client.setRoomReadMarkers(
+                "room_id",
+                "read_marker_event_id",
+                rrEvent,
+                rpEvent,
+            );
+
+            expect(client.setRoomReadMarkersHttpRequest).toHaveBeenCalledWith(
+                "room_id",
+                "read_marker_event_id",
+                "read_event_id",
+                "read_private_event_id",
+            );
+            expect(room.addLocalEchoReceipt).toHaveBeenCalledTimes(2);
+            expect(room.addLocalEchoReceipt).toHaveBeenNthCalledWith(
+                1,
+                client.credentials.userId,
+                rrEvent,
+                ReceiptType.Read,
+            );
+            expect(room.addLocalEchoReceipt).toHaveBeenNthCalledWith(
+                2,
+                client.credentials.userId,
+                rpEvent,
+                ReceiptType.ReadPrivate,
+            );
+        });
+    });
+
     describe("beacons", () => {
         const roomId = '!room:server.org';
         const content = makeBeaconInfoContent(100, true);

spec/unit/room.spec.ts

@@ -23,6 +23,7 @@ import * as utils from "../test-utils/test-utils";
 import {
     DuplicateStrategy,
     EventStatus,
+    EventTimelineSet,
     EventType,
     JoinRule,
     MatrixEvent,
@@ -31,11 +32,12 @@ import {
     RoomEvent,
 } from "../../src";
 import { EventTimeline } from "../../src/models/event-timeline";
-import { Room } from "../../src/models/room";
+import { IWrappedReceipt, Room } from "../../src/models/room";
 import { RoomState } from "../../src/models/room-state";
 import { UNSTABLE_ELEMENT_FUNCTIONAL_USERS } from "../../src/@types/event";
 import { TestClient } from "../TestClient";
 import { emitPromise } from "../test-utils/test-utils";
+import { ReceiptType } from "../../src/@types/read_receipts";
 import { Thread, ThreadEvent } from "../../src/models/thread";
 
 describe("Room", function() {
@@ -2286,4 +2288,29 @@ describe("Room", function() {
             expect(responseRelations[0][1].has(threadReaction)).toBeTruthy();
         });
     });
+
+    describe("getEventReadUpTo()", () => {
+        const client = new TestClient(userA).client;
+        const room = new Room(roomId, client, userA);
+
+        it("handles missing receipt type", () => {
+            room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
+                return receiptType === ReceiptType.ReadPrivate ? { eventId: "eventId" } as IWrappedReceipt : null;
+            };
+
+            expect(room.getEventReadUpTo(userA)).toEqual("eventId");
+        });
+
+        it("prefers older receipt", () => {
+            room.getReadReceiptForUserId = (userId, ignore, receiptType) => {
+                return (receiptType === ReceiptType.Read
+                    ? { eventId: "eventId1" }
+                    : { eventId: "eventId2" }
+                    ) as IWrappedReceipt;
+            };
+            room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => 1 } as EventTimelineSet);
+
+            expect(room.getEventReadUpTo(userA)).toEqual("eventId1");
+        });
+    });
 });

spec/unit/sync-accumulator.spec.js

@@ -15,6 +15,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+import { ReceiptType } from "../../src/@types/read_receipts";
 import { SyncAccumulator } from "../../src/sync-accumulator";
 
 // The event body & unsigned object get frozen to assert that they don't get altered
@@ -294,10 +295,13 @@ describe("SyncAccumulator", function() {
             room_id: "!foo:bar",
             content: {
                 "$event1:localhost": {
-                    "m.read": {
+                    [ReceiptType.Read]: {
                         "@alice:localhost": { ts: 1 },
                         "@bob:localhost": { ts: 2 },
                     },
+                    [ReceiptType.ReadPrivate]: {
+                        "@dan:localhost": { ts: 4 },
+                    },
                     "some.other.receipt.type": {
                         "@should_be_ignored:localhost": { key: "val" },
                     },
@@ -309,7 +313,7 @@ describe("SyncAccumulator", function() {
             room_id: "!foo:bar",
             content: {
                 "$event2:localhost": {
-                    "m.read": {
+                    [ReceiptType.Read]: {
                         "@bob:localhost": { ts: 2 }, // clobbers event1 receipt
                         "@charlie:localhost": { ts: 3 },
                     },
@@ -337,12 +341,15 @@ describe("SyncAccumulator", function() {
             room_id: "!foo:bar",
             content: {
                 "$event1:localhost": {
-                    "m.read": {
+                    [ReceiptType.Read]: {
                         "@alice:localhost": { ts: 1 },
                     },
+                    [ReceiptType.ReadPrivate]: {
+                        "@dan:localhost": { ts: 4 },
+                    },
                 },
                 "$event2:localhost": {
-                    "m.read": {
+                    [ReceiptType.Read]: {
                         "@bob:localhost": { ts: 2 },
                         "@charlie:localhost": { ts: 3 },
                     },

src/@types/read_receipts.ts

@@ -0,0 +1,21 @@
+/*
+Copyright 2022 Šimon Brandner <simon.bra.ag@gmail.com>
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+export enum ReceiptType {
+    Read = "m.read",
+    FullyRead = "m.fully_read",
+    ReadPrivate = "org.matrix.msc2285.read.private"
+}

src/client.ts

@@ -181,6 +181,7 @@ import { CryptoStore } from "./crypto/store/base";
 import { MediaHandler } from "./webrtc/mediaHandler";
 import { IRefreshTokenResponse } from "./@types/auth";
 import { TypedEventEmitter } from "./models/typed-event-emitter";
+import { ReceiptType } from "./@types/read_receipts";
 import { Thread, THREAD_RELATION_TYPE } from "./models/thread";
 import { MBeaconInfoEventContent, M_BEACON_INFO } from "./@types/beacon";
 
@@ -1078,7 +1079,13 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
                 // Figure out if we've read something or if it's just informational
                 const content = event.getContent();
                 const isSelf = Object.keys(content).filter(eid => {
-                    return Object.keys(content[eid]['m.read']).includes(this.getUserId());
+                    const read = content[eid][ReceiptType.Read];
+                    if (read && Object.keys(read).includes(this.getUserId())) return true;
+
+                    const readPrivate = content[eid][ReceiptType.ReadPrivate];
+                    if (readPrivate && Object.keys(readPrivate).includes(this.getUserId())) return true;
+
+                    return false;
                 }).length > 0;
 
                 if (!isSelf) return;
@@ -4491,13 +4498,14 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     /**
      * Send a receipt.
      * @param {Event} event The event being acknowledged
-     * @param {string} receiptType The kind of receipt e.g. "m.read"
+     * @param {ReceiptType} receiptType The kind of receipt e.g. "m.read". Other than
+     * ReceiptType.Read are experimental!
      * @param {object} body Additional content to send alongside the receipt.
      * @param {module:client.callback} callback Optional.
      * @return {Promise} Resolves: to an empty object {}
      * @return {module:http-api.MatrixError} Rejects: with an error response.
      */
-    public sendReceipt(event: MatrixEvent, receiptType: string, body: any, callback?: Callback): Promise<{}> {
+    public sendReceipt(event: MatrixEvent, receiptType: ReceiptType, body: any, callback?: Callback): Promise<{}> {
         if (typeof (body) === 'function') {
             callback = body as any as Callback; // legacy
             body = {};
@@ -4524,32 +4532,19 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     /**
      * Send a read receipt.
      * @param {Event} event The event that has been read.
-     * @param {object} opts The options for the read receipt.
-     * @param {boolean} opts.hidden True to prevent the receipt from being sent to
-     * other users and homeservers. Default false (send to everyone). <b>This
-     * property is unstable and may change in the future.</b>
+     * @param {ReceiptType} receiptType other than ReceiptType.Read are experimental! Optional.
      * @param {module:client.callback} callback Optional.
      * @return {Promise} Resolves: to an empty object
      * @return {module:http-api.MatrixError} Rejects: with an error response.
      */
-    public async sendReadReceipt(event: MatrixEvent, opts?: { hidden?: boolean }, callback?: Callback): Promise<{}> {
-        if (typeof (opts) === 'function') {
-            callback = opts as any as Callback; // legacy
-            opts = {};
-        }
-        if (!opts) opts = {};
-
+    public async sendReadReceipt(event: MatrixEvent, receiptType = ReceiptType.Read, callback?: Callback): Promise<{}> {
         const eventId = event.getId();
         const room = this.getRoom(event.getRoomId());
         if (room && room.hasPendingEvent(eventId)) {
             throw new Error(`Cannot set read receipt to a pending event (${eventId})`);
         }
 
-        const addlContent = {
-            "org.matrix.msc2285.hidden": Boolean(opts.hidden),
-        };
-
-        return this.sendReceipt(event, "m.read", addlContent, callback);
+        return this.sendReceipt(event, receiptType, {}, callback);
     }
 
     /**
@@ -4562,16 +4557,15 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param {MatrixEvent} rrEvent the event tracked by the read receipt. This is here for
      * convenience because the RR and the RM are commonly updated at the same time as each
      * other. The local echo of this receipt will be done if set. Optional.
-     * @param {object} opts Options for the read markers
-     * @param {object} opts.hidden True to hide the receipt from other users and homeservers.
-     * <b>This property is unstable and may change in the future.</b>
+     * @param {MatrixEvent} rpEvent the m.read.private read receipt event for when we don't
+     * want other users to see the read receipts. This is experimental. Optional.
      * @return {Promise} Resolves: the empty object, {}.
      */
     public async setRoomReadMarkers(
         roomId: string,
         rmEventId: string,
-        rrEvent: MatrixEvent,
-        opts: { hidden?: boolean },
+        rrEvent?: MatrixEvent,
+        rpEvent?: MatrixEvent,
     ): Promise<{}> {
         const room = this.getRoom(roomId);
         if (room && room.hasPendingEvent(rmEventId)) {
@@ -4579,18 +4573,26 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         }
 
         // Add the optional RR update, do local echo like `sendReceipt`
-        let rrEventId;
+        let rrEventId: string;
         if (rrEvent) {
             rrEventId = rrEvent.getId();
-            if (room && room.hasPendingEvent(rrEventId)) {
+            if (room?.hasPendingEvent(rrEventId)) {
                 throw new Error(`Cannot set read receipt to a pending event (${rrEventId})`);
             }
-            if (room) {
-                room.addLocalEchoReceipt(this.credentials.userId, rrEvent, "m.read");
-            }
+            room?.addLocalEchoReceipt(this.credentials.userId, rrEvent, ReceiptType.Read);
         }
 
-        return this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, opts);
+        // Add the optional private RR update, do local echo like `sendReceipt`
+        let rpEventId: string;
+        if (rpEvent) {
+            rpEventId = rpEvent.getId();
+            if (room?.hasPendingEvent(rpEventId)) {
+                throw new Error(`Cannot set read receipt to a pending event (${rpEventId})`);
+            }
+            room?.addLocalEchoReceipt(this.credentials.userId, rpEvent, ReceiptType.ReadPrivate);
+        }
+
+        return this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, rpEventId);
     }
 
     /**
@@ -7381,25 +7383,24 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param {string} rrEventId ID of the event tracked by the read receipt. This is here
      * for convenience because the RR and the RM are commonly updated at the same time as
      * each other. Optional.
-     * @param {object} opts Options for the read markers.
-     * @param {object} opts.hidden True to hide the read receipt from other users. <b>This
-     * property is currently unstable and may change in the future.</b>
+     * @param {string} rpEventId rpEvent the m.read.private read receipt event for when we
+     * don't want other users to see the read receipts. This is experimental. Optional.
      * @return {Promise} Resolves: the empty object, {}.
      */
     public setRoomReadMarkersHttpRequest(
         roomId: string,
         rmEventId: string,
         rrEventId: string,
-        opts: { hidden?: boolean },
+        rpEventId: string,
     ): Promise<{}> {
         const path = utils.encodeUri("/rooms/$roomId/read_markers", {
             $roomId: roomId,
         });
 
         const content = {
-            "m.fully_read": rmEventId,
-            "m.read": rrEventId,
-            "org.matrix.msc2285.hidden": Boolean(opts ? opts.hidden : false),
+            [ReceiptType.FullyRead]: rmEventId,
+            [ReceiptType.Read]: rrEventId,
+            [ReceiptType.ReadPrivate]: rpEventId,
         };
 
         return this.http.authedRequest(undefined, Method.Post, path, undefined, content);

src/models/room.ts

@@ -47,6 +47,7 @@ import {
     ThreadFilterType,
 } from "./thread";
 import { TypedEventEmitter } from "./typed-event-emitter";
+import { ReceiptType } from "../@types/read_receipts";
 import { IStateEventWithRoomId } from "../@types/search";
 
 // These constants are used as sane defaults when the homeserver doesn't support
@@ -58,7 +59,7 @@ import { IStateEventWithRoomId } from "../@types/search";
 const KNOWN_SAFE_ROOM_VERSION = '9';
 const SAFE_ROOM_VERSIONS = ['1', '2', '3', '4', '5', '6', '7', '8', '9'];
 
-function synthesizeReceipt(userId: string, event: MatrixEvent, receiptType: string): MatrixEvent {
+function synthesizeReceipt(userId: string, event: MatrixEvent, receiptType: ReceiptType): MatrixEvent {
     // console.log("synthesizing receipt for "+event.getId());
     return new MatrixEvent({
         content: {
@@ -93,13 +94,13 @@ interface IReceipt {
     ts: number;
 }
 
-interface IWrappedReceipt {
+export interface IWrappedReceipt {
     eventId: string;
     data: IReceipt;
 }
 
 interface ICachedReceipt {
-    type: string;
+    type: ReceiptType;
     userId: string;
     data: IReceipt;
 }
@@ -108,7 +109,7 @@ type ReceiptCache = {[eventId: string]: ICachedReceipt[]};
 
 interface IReceiptContent {
     [eventId: string]: {
-        [type: string]: {
+        [key in ReceiptType]: {
             [userId: string]: IReceipt;
         };
     };
@@ -1792,7 +1793,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         // Don't synthesize RR for m.room.redaction as this causes the RR to go missing.
         if (event.sender && event.getType() !== EventType.RoomRedaction) {
             this.addReceipt(synthesizeReceipt(
-                event.sender.userId, event, "m.read",
+                event.sender.userId, event, ReceiptType.Read,
             ), true);
 
             // Any live events from a user could be taken as implicit
@@ -2314,14 +2315,23 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
      */
     public getUsersReadUpTo(event: MatrixEvent): string[] {
         return this.getReceiptsForEvent(event).filter(function(receipt) {
-            return receipt.type === "m.read";
+            return [ReceiptType.Read, ReceiptType.ReadPrivate].includes(receipt.type);
         }).map(function(receipt) {
             return receipt.userId;
         });
     }
 
-    public getReadReceiptForUserId(userId: string, ignoreSynthesized = false): IWrappedReceipt | null {
-        const [realReceipt, syntheticReceipt] = this.receipts["m.read"]?.[userId] ?? [];
+    /**
+     * Gets the latest receipt for a given user in the room
+     * @param userId The id of the user for which we want the receipt
+     * @param ignoreSynthesized Whether to ignore synthesized receipts or not
+     * @param receiptType Optional. The type of the receipt we want to get
+     * @returns the latest receipts of the chosen type for the chosen user
+     */
+    public getReadReceiptForUserId(
+        userId: string, ignoreSynthesized = false, receiptType = ReceiptType.Read,
+    ): IWrappedReceipt | null {
+        const [realReceipt, syntheticReceipt] = this.receipts[receiptType]?.[userId] ?? [];
         if (ignoreSynthesized) {
             return realReceipt;
         }
@@ -2339,8 +2349,25 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
      * @return {String} ID of the latest event that the given user has read, or null.
      */
     public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | null {
-        const readReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized);
-        return readReceipt?.eventId ?? null;
+        const timelineSet = this.getUnfilteredTimelineSet();
+        const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read);
+        const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate);
+
+        // If we have both, compare them
+        let comparison: number | undefined;
+        if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) {
+            comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId);
+        }
+
+        // If we didn't get a comparison try to compare the ts of the receipts
+        if (!comparison) comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts;
+
+        // The public receipt is more likely to drift out of date so the private
+        // one has precedence
+        if (!comparison) return privateReadReceipt?.eventId ?? publicReadReceipt?.eventId ?? null;
+
+        // If public read receipt is older, return the private one
+        return (comparison < 0) ? privateReadReceipt?.eventId : publicReadReceipt?.eventId;
     }
 
     /**
@@ -2493,7 +2520,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
                     }
                     this.receiptCacheByEventId[eventId].push({
                         userId: userId,
-                        type: receiptType,
+                        type: receiptType as ReceiptType,
                         data: receipt,
                     });
                 });
@@ -2506,9 +2533,9 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
      * client the fact that we've sent one.
      * @param {string} userId The user ID if the receipt sender
      * @param {MatrixEvent} e The event that is to be acknowledged
-     * @param {string} receiptType The type of receipt
+     * @param {ReceiptType} receiptType The type of receipt
      */
-    public addLocalEchoReceipt(userId: string, e: MatrixEvent, receiptType: string): void {
+    public addLocalEchoReceipt(userId: string, e: MatrixEvent, receiptType: ReceiptType): void {
         this.addReceipt(synthesizeReceipt(userId, e, receiptType), true);
     }
 

src/sync-accumulator.ts

@@ -24,6 +24,7 @@ import { deepCopy } from "./utils";
 import { IContent, IUnsigned } from "./models/event";
 import { IRoomSummary } from "./models/room-summary";
 import { EventType } from "./@types/event";
+import { ReceiptType } from "./@types/read_receipts";
 
 interface IOpts {
     maxTimelineEntries?: number;
@@ -157,6 +158,7 @@ interface IRoom {
     _readReceipts: {
         [userId: string]: {
             data: IMinimalEvent;
+            type: ReceiptType;
             eventId: string;
         };
     };
@@ -416,16 +418,31 @@ export class SyncAccumulator {
                 // of a hassle to work with. We'll inflate this back out when
                 // getJSON() is called.
                 Object.keys(e.content).forEach((eventId) => {
-                    if (!e.content[eventId]["m.read"]) {
+                    if (!e.content[eventId][ReceiptType.Read] && !e.content[eventId][ReceiptType.ReadPrivate]) {
                         return;
                     }
-                    Object.keys(e.content[eventId]["m.read"]).forEach((userId) => {
-                        // clobber on user ID
-                        currentData._readReceipts[userId] = {
-                            data: e.content[eventId]["m.read"][userId],
-                            eventId: eventId,
-                        };
-                    });
+                    const read = e.content[eventId][ReceiptType.Read];
+                    if (read) {
+                        Object.keys(read).forEach((userId) => {
+                            // clobber on user ID
+                            currentData._readReceipts[userId] = {
+                                data: e.content[eventId][ReceiptType.Read][userId],
+                                type: ReceiptType.Read,
+                                eventId: eventId,
+                            };
+                        });
+                    }
+                    const readPrivate = e.content[eventId][ReceiptType.ReadPrivate];
+                    if (readPrivate) {
+                        Object.keys(readPrivate).forEach((userId) => {
+                            // clobber on user ID
+                            currentData._readReceipts[userId] = {
+                                data: e.content[eventId][ReceiptType.ReadPrivate][userId],
+                                type: ReceiptType.ReadPrivate,
+                                eventId: eventId,
+                            };
+                        });
+                    }
                 });
             });
         }
@@ -552,11 +569,12 @@ export class SyncAccumulator {
             Object.keys(roomData._readReceipts).forEach((userId) => {
                 const receiptData = roomData._readReceipts[userId];
                 if (!receiptEvent.content[receiptData.eventId]) {
-                    receiptEvent.content[receiptData.eventId] = {
-                        "m.read": {},
-                    };
+                    receiptEvent.content[receiptData.eventId] = {};
                 }
-                receiptEvent.content[receiptData.eventId]["m.read"][userId] = (
+                if (!receiptEvent.content[receiptData.eventId][receiptData.type]) {
+                    receiptEvent.content[receiptData.eventId][receiptData.type] = {};
+                }
+                receiptEvent.content[receiptData.eventId][receiptData.type][userId] = (
                     receiptData.data
                 );
             });