Don't attempt to adjust for clock skews when calculating group call membership expiry (#4340)

* Use origin server timestamp for calculating group call membership expiry

* Fix tests

* Docs

* Refactor comments to reflect that the logic hasn't changed

* Make comment maintainable

* Fix up merge

* Fix test

spec/unit/matrixrtc/CallMembership.spec.ts

@@ -98,20 +98,6 @@ describe("CallMembership", () => {
             expect(membership.getAbsoluteExpiry()).toEqual(5000 + 1000);
         });
 
-        it("considers memberships unexpired if local age low enough", () => {
-            const fakeEvent = makeMockEvent(1000);
-            fakeEvent.getLocalAge = jest.fn().mockReturnValue(3000);
-            const membership = new CallMembership(fakeEvent, membershipTemplate);
-            expect(membership.isExpired()).toEqual(false);
-        });
-
-        it("considers memberships expired when local age large", () => {
-            const fakeEvent = makeMockEvent(1000);
-            fakeEvent.localTimestamp = Date.now() - 6000;
-            const membership = new CallMembership(fakeEvent, membershipTemplate);
-            expect(membership.isExpired()).toEqual(true);
-        });
-
         it("returns preferred foci", () => {
             const fakeEvent = makeMockEvent();
             const mockFocus = { type: "this_is_a_mock_focus" };
@@ -198,13 +184,6 @@ describe("CallMembership", () => {
         beforeEach(() => {
             // server origin timestamp for this event is 1000
             fakeEvent = makeMockEvent(1000);
-            // our clock would have been at 2000 at the creation time (our clock at event receive time - age)
-            // (ie. the local clock is 1 second ahead of the servers' clocks)
-            fakeEvent.localTimestamp = 2000;
-
-            // for simplicity's sake, we say that the event's age is zero
-            fakeEvent.getLocalAge = jest.fn().mockReturnValue(0);
-
             membership = new CallMembership(fakeEvent!, membershipTemplate);
 
             jest.useFakeTimers();
@@ -215,6 +194,13 @@ describe("CallMembership", () => {
         });
 
         it("converts expiry time into local clock", () => {
+            // our clock would have been at 2000 at the creation time (our clock at event receive time - age)
+            // (ie. the local clock is 1 second ahead of the servers' clocks)
+            fakeEvent.localTimestamp = 2000;
+
+            // for simplicity's sake, we say that the event's age is zero
+            fakeEvent.getLocalAge = jest.fn().mockReturnValue(0);
+
             // for sanity's sake, make sure the server-relative expiry time is what we expect
             expect(membership.getAbsoluteExpiry()).toEqual(6000);
             // therefore the expiry time converted to our clock should be 1 second later
@@ -223,7 +209,8 @@ describe("CallMembership", () => {
 
         it("calculates time until expiry", () => {
             jest.setSystemTime(2000);
-            expect(membership.getMsUntilExpiry()).toEqual(5000);
+            // should be using absolute expiry time
+            expect(membership.getMsUntilExpiry()).toEqual(4000);
         });
     });
 });

spec/unit/matrixrtc/MatrixRTCSession.spec.ts

@@ -73,14 +73,17 @@ describe("MatrixRTCSession", () => {
     });
 
     it("ignores expired memberships events", () => {
+        jest.useFakeTimers();
         const expiredMembership = Object.assign({}, membershipTemplate);
         expiredMembership.expires = 1000;
         expiredMembership.device_id = "EXPIRED";
-        const mockRoom = makeMockRoom([membershipTemplate, expiredMembership], 10000);
+        const mockRoom = makeMockRoom([membershipTemplate, expiredMembership]);
 
+        jest.advanceTimersByTime(2000);
         sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
         expect(sess?.memberships.length).toEqual(1);
         expect(sess?.memberships[0].deviceId).toEqual("AAAAAAA");
+        jest.useRealTimers();
     });
 
     it("ignores memberships events of members not in the room", () => {
@@ -91,12 +94,15 @@ describe("MatrixRTCSession", () => {
     });
 
     it("honours created_ts", () => {
+        jest.useFakeTimers();
+        jest.setSystemTime(500);
         const expiredMembership = Object.assign({}, membershipTemplate);
         expiredMembership.created_ts = 500;
         expiredMembership.expires = 1000;
         const mockRoom = makeMockRoom([expiredMembership]);
         sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
         expect(sess?.memberships[0].getAbsoluteExpiry()).toEqual(1500);
+        jest.useRealTimers();
     });
 
     it("returns empty session if no membership events are present", () => {
@@ -304,6 +310,8 @@ describe("MatrixRTCSession", () => {
 
     describe("getOldestMembership", () => {
         it("returns the oldest membership event", () => {
+            jest.useFakeTimers();
+            jest.setSystemTime(4000);
             const mockRoom = makeMockRoom([
                 Object.assign({}, membershipTemplate, { device_id: "foo", created_ts: 3000 }),
                 Object.assign({}, membershipTemplate, { device_id: "old", created_ts: 1000 }),
@@ -312,12 +320,15 @@ describe("MatrixRTCSession", () => {
 
             sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
             expect(sess.getOldestMembership()!.deviceId).toEqual("old");
+            jest.useRealTimers();
         });
     });
 
     describe("getsActiveFocus", () => {
         const activeFociConfig = { type: "livekit", livekit_service_url: "https://active.url" };
         it("gets the correct active focus with oldest_membership", () => {
+            jest.useFakeTimers();
+            jest.setSystemTime(3000);
             const mockRoom = makeMockRoom([
                 Object.assign({}, membershipTemplate, {
                     device_id: "foo",
@@ -335,6 +346,7 @@ describe("MatrixRTCSession", () => {
                 focus_selection: "oldest_membership",
             });
             expect(sess.getActiveFocus()).toBe(activeFociConfig);
+            jest.useRealTimers();
         });
         it("does not provide focus if the selction method is unknown", () => {
             const mockRoom = makeMockRoom([
@@ -356,6 +368,8 @@ describe("MatrixRTCSession", () => {
             expect(sess.getActiveFocus()).toBe(undefined);
         });
         it("gets the correct active focus legacy", () => {
+            jest.useFakeTimers();
+            jest.setSystemTime(3000);
             const mockRoom = makeMockRoom([
                 Object.assign({}, membershipTemplate, {
                     device_id: "foo",
@@ -370,6 +384,7 @@ describe("MatrixRTCSession", () => {
 
             sess.joinRoomSession([{ type: "livekit", livekit_service_url: "htts://test.org" }]);
             expect(sess.getActiveFocus()).toBe(activeFociConfig);
+            jest.useRealTimers();
         });
     });
 
@@ -513,9 +528,8 @@ describe("MatrixRTCSession", () => {
 
                 const eventContent = await eventSentPromise;
 
-                // definitely should have renewed by 1 second before the expiry!
-                const timeElapsed = 60 * 60 * 1000 - 1000;
-                const event = mockRTCEvent(eventContent.memberships, mockRoom.roomId, timeElapsed);
+                jest.setSystemTime(1000);
+                const event = mockRTCEvent(eventContent.memberships, mockRoom.roomId);
                 const getState = mockRoom.getLiveTimeline().getState(EventTimeline.FORWARDS)!;
                 getState.getStateEvents = jest.fn().mockReturnValue(event);
                 getState.events = new Map([
@@ -538,6 +552,8 @@ describe("MatrixRTCSession", () => {
 
                 sendStateEventMock.mockReset().mockImplementation(resolveFn);
 
+                // definitely should have renewed by 1 second before the expiry!
+                const timeElapsed = 60 * 60 * 1000 - 1000;
                 jest.setSystemTime(Date.now() + timeElapsed);
                 jest.advanceTimersByTime(timeElapsed);
                 await eventReSentPromise;
@@ -685,7 +701,7 @@ describe("MatrixRTCSession", () => {
 
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([membershipTemplate, member2], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([membershipTemplate, member2], mockRoom.roomId));
                 sess.onMembershipUpdate();
 
                 await keysSentPromise2;
@@ -711,7 +727,7 @@ describe("MatrixRTCSession", () => {
                 const mockRoom = makeMockRoom([member1, member2]);
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId));
 
                 sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
                 sess.joinRoomSession([mockFocus], mockFocus, { manageMediaKeys: true });
@@ -760,7 +776,7 @@ describe("MatrixRTCSession", () => {
                 const mockRoom = makeMockRoom([member1, member2]);
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId));
 
                 sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
                 sess.joinRoomSession([mockFocus], mockFocus, { manageMediaKeys: true });
@@ -825,6 +841,7 @@ describe("MatrixRTCSession", () => {
 
         it("Re-sends key if a member changes created_ts", async () => {
             jest.useFakeTimers();
+            jest.setSystemTime(1000);
             try {
                 const keysSentPromise1 = new Promise((resolve) => {
                     sendEventMock.mockImplementation(resolve);
@@ -840,7 +857,7 @@ describe("MatrixRTCSession", () => {
                 const mockRoom = makeMockRoom([member1, member2]);
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([member1, member2], mockRoom.roomId));
 
                 sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
                 sess.joinRoomSession([mockFocus], mockFocus, { manageMediaKeys: true });
@@ -938,7 +955,7 @@ describe("MatrixRTCSession", () => {
 
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([membershipTemplate], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([membershipTemplate], mockRoom.roomId));
                 sess.onMembershipUpdate();
 
                 jest.advanceTimersByTime(10000);
@@ -977,7 +994,7 @@ describe("MatrixRTCSession", () => {
 
                 mockRoom.getLiveTimeline().getState = jest
                     .fn()
-                    .mockReturnValue(makeMockRoomState([membershipTemplate, member2], mockRoom.roomId, undefined));
+                    .mockReturnValue(makeMockRoomState([membershipTemplate, member2], mockRoom.roomId));
                 sess.onMembershipUpdate();
 
                 await new Promise((resolve) => {
@@ -1009,9 +1026,7 @@ describe("MatrixRTCSession", () => {
         const onMembershipsChanged = jest.fn();
         sess.on(MatrixRTCSessionEvent.MembershipsChanged, onMembershipsChanged);
 
-        mockRoom.getLiveTimeline().getState = jest
-            .fn()
-            .mockReturnValue(makeMockRoomState([], mockRoom.roomId, undefined));
+        mockRoom.getLiveTimeline().getState = jest.fn().mockReturnValue(makeMockRoomState([], mockRoom.roomId));
         sess.onMembershipUpdate();
 
         expect(onMembershipsChanged).toHaveBeenCalled();
@@ -1021,7 +1036,7 @@ describe("MatrixRTCSession", () => {
         jest.useFakeTimers();
         try {
             const membership = Object.assign({}, membershipTemplate);
-            const mockRoom = makeMockRoom([membership], 0);
+            const mockRoom = makeMockRoom([membership]);
 
             sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);
             const membershipObject = sess.memberships[0];
@@ -1049,7 +1064,7 @@ describe("MatrixRTCSession", () => {
                     expires: 1000,
                 }),
             ];
-            const mockRoomNoExpired = makeMockRoom(mockMemberships, 0);
+            const mockRoomNoExpired = makeMockRoom(mockMemberships);
 
             sess = MatrixRTCSession.roomSessionForRoom(client, mockRoomNoExpired);
 
@@ -1088,6 +1103,7 @@ describe("MatrixRTCSession", () => {
     it("fills in created_ts for other memberships on update", () => {
         client.sendStateEvent = jest.fn();
         jest.useFakeTimers();
+        jest.setSystemTime(1000);
         const mockRoom = makeMockRoom([
             Object.assign({}, membershipTemplate, {
                 device_id: "OTHERDEVICE",

spec/unit/matrixrtc/mocks.ts

@@ -20,10 +20,10 @@ import { randomString } from "../../../src/randomstring";
 
 type MembershipData = CallMembershipData[] | SessionMembershipData;
 
-export function makeMockRoom(membershipData: MembershipData, localAge: number | null = null): Room {
+export function makeMockRoom(membershipData: MembershipData): Room {
     const roomId = randomString(8);
     // Caching roomState here so it does not get recreated when calling `getLiveTimeline.getState()`
-    const roomState = makeMockRoomState(membershipData, roomId, localAge);
+    const roomState = makeMockRoomState(membershipData, roomId);
     return {
         roomId: roomId,
         hasMembershipState: jest.fn().mockReturnValue(true),
@@ -34,8 +34,8 @@ export function makeMockRoom(membershipData: MembershipData, localAge: number |
     } as unknown as Room;
 }
 
-export function makeMockRoomState(membershipData: MembershipData, roomId: string, localAge: number | null = null) {
-    const event = mockRTCEvent(membershipData, roomId, localAge);
+export function makeMockRoomState(membershipData: MembershipData, roomId: string) {
+    const event = mockRTCEvent(membershipData, roomId);
     return {
         on: jest.fn(),
         off: jest.fn(),
@@ -57,7 +57,7 @@ export function makeMockRoomState(membershipData: MembershipData, roomId: string
     };
 }
 
-export function mockRTCEvent(membershipData: MembershipData, roomId: string, localAge: number | null): MatrixEvent {
+export function mockRTCEvent(membershipData: MembershipData, roomId: string): MatrixEvent {
     return {
         getType: jest.fn().mockReturnValue(EventType.GroupCallMemberPrefix),
         getContent: jest.fn().mockReturnValue(
@@ -68,8 +68,7 @@ export function mockRTCEvent(membershipData: MembershipData, roomId: string, loc
                   },
         ),
         getSender: jest.fn().mockReturnValue("@mock:user.example"),
-        getTs: jest.fn().mockReturnValue(1000),
-        localTimestamp: Date.now() - (localAge ?? 10),
+        getTs: jest.fn().mockReturnValue(Date.now()),
         getRoomId: jest.fn().mockReturnValue(roomId),
         sender: {
             userId: "@mock:user.example",

src/matrixrtc/CallMembership.ts

@@ -156,8 +156,14 @@ export class CallMembership {
         return this.membershipData.created_ts ?? this.parentEvent.getTs();
     }
 
+    /**
+     * Gets the absolute expiry time of the membership if applicable to this membership type.
+     * @returns The absolute expiry time of the membership as a unix timestamp in milliseconds or undefined if not applicable
+     */
     public getAbsoluteExpiry(): number | undefined {
+        // if the membership is not a legacy membership, we assume it is MSC4143
         if (!isLegacyCallMembershipData(this.membershipData)) return undefined;
+
         if ("expires" in this.membershipData) {
             // we know createdTs exists since we already do the isLegacyCallMembershipData check
             return this.createdTs() + this.membershipData.expires;
@@ -167,9 +173,15 @@ export class CallMembership {
         }
     }
 
-    // gets the expiry time of the event, converted into the device's local time
+    /**
+     * Gets the expiry time of the event, converted into the device's local time.
+     * @deprecated This function has been observed returning bad data and is no longer used by MatrixRTC.
+     * @returns The local expiry time of the membership as a unix timestamp in milliseconds or undefined if not applicable
+     */
     public getLocalExpiry(): number | undefined {
+        // if the membership is not a legacy membership, we assume it is MSC4143
         if (!isLegacyCallMembershipData(this.membershipData)) return undefined;
+
         if ("expires" in this.membershipData) {
             // we know createdTs exists since we already do the isLegacyCallMembershipData check
             const relativeCreationTime = this.parentEvent.getTs() - this.createdTs();
@@ -184,10 +196,24 @@ export class CallMembership {
         }
     }
 
+    /**
+     * @returns The number of milliseconds until the membership expires or undefined if applicable
+     */
     public getMsUntilExpiry(): number | undefined {
-        if (isLegacyCallMembershipData(this.membershipData)) return this.getLocalExpiry()! - Date.now();
+        if (isLegacyCallMembershipData(this.membershipData)) {
+            // Assume that local clock is sufficiently in sync with other clocks in the distributed system.
+            // We used to try and adjust for the local clock being skewed, but there are cases where this is not accurate.
+            // The current implementation allows for the local clock to be -infinity to +MatrixRTCSession.MEMBERSHIP_EXPIRY_TIME/2
+            return this.getAbsoluteExpiry()! - Date.now();
+        }
+
+        // Assumed to be MSC4143
+        return undefined;
     }
 
+    /**
+     * @returns true if the membership has expired, otherwise false
+     */
     public isExpired(): boolean {
         if (isLegacyCallMembershipData(this.membershipData)) return this.getMsUntilExpiry()! <= 0;