From aa0ebb0c115c0ac8428e83c8cdc32b29770154c1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 31 Jul 2023 11:27:27 +0100 Subject: [PATCH 1/4] Revert "Ensure we don't overinflate the total notification count (#3634)" This reverts commit fd0c4a7f5697b33020362de72d19521c22efd63b. --- spec/unit/event-timeline-set.spec.ts | 18 ++----- spec/unit/notifications.spec.ts | 31 ++++-------- spec/unit/room-state.spec.ts | 7 +-- spec/unit/room.spec.ts | 4 +- src/client.ts | 73 +++++++++++++++------------- src/models/event.ts | 6 +-- 6 files changed, 58 insertions(+), 81 deletions(-) diff --git a/spec/unit/event-timeline-set.spec.ts b/spec/unit/event-timeline-set.spec.ts index e86840ec5b2..a817127569c 100644 --- a/spec/unit/event-timeline-set.spec.ts +++ b/spec/unit/event-timeline-set.spec.ts @@ -303,13 +303,8 @@ describe("EventTimelineSet", () => { messageEventIsDecryptionFailureSpy.mockReturnValue(true); replyEventIsDecryptionFailureSpy.mockReturnValue(true); - messageEvent.emit( - MatrixEventEvent.Decrypted, - messageEvent, - undefined, - messageEvent.getPushDetails(), - ); - replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent, undefined, replyEvent.getPushDetails()); + messageEvent.emit(MatrixEventEvent.Decrypted, messageEvent); + replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent); // simulate decryption messageEventIsDecryptionFailureSpy.mockReturnValue(false); @@ -318,13 +313,8 @@ describe("EventTimelineSet", () => { messageEventShouldAttemptDecryptionSpy.mockReturnValue(false); replyEventShouldAttemptDecryptionSpy.mockReturnValue(false); - messageEvent.emit( - MatrixEventEvent.Decrypted, - messageEvent, - undefined, - messageEvent.getPushDetails(), - ); - replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent, undefined, replyEvent.getPushDetails()); + messageEvent.emit(MatrixEventEvent.Decrypted, messageEvent); + replyEvent.emit(MatrixEventEvent.Decrypted, replyEvent); }); itShouldReturnTheRelatedEvents(); diff --git a/spec/unit/notifications.spec.ts b/spec/unit/notifications.spec.ts index 2f845f34d76..b0ebd23d952 100644 --- a/spec/unit/notifications.spec.ts +++ b/spec/unit/notifications.spec.ts @@ -54,7 +54,7 @@ describe("fixNotificationCountOnDecryption", () => { mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(), isInitialSyncComplete: jest.fn().mockReturnValue(false), - getPushActionsForEvent: jest.fn((ev) => event.getPushActions() ?? mkPushAction(true, true)), + getPushActionsForEvent: jest.fn().mockReturnValue(mkPushAction(true, true)), getRoom: jest.fn().mockImplementation(() => room), decryptEventIfNeeded: jest.fn().mockResolvedValue(void 0), supportsThreads: jest.fn().mockReturnValue(true), @@ -125,15 +125,15 @@ describe("fixNotificationCountOnDecryption", () => { room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 1); room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight, 0); - event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, true)); - threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, true)); + event.getPushActions = jest.fn().mockReturnValue(mkPushAction(false, false)); + threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(false, false)); }); it("changes the room count to highlight on decryption", () => { expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2); expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0); - fixNotificationCountOnDecryption(mockClient, event, {}); + fixNotificationCountOnDecryption(mockClient, event); expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(3); expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(1); @@ -143,7 +143,7 @@ describe("fixNotificationCountOnDecryption", () => { room.setUnreadNotificationCount(NotificationCountType.Total, 0); room.setUnreadNotificationCount(NotificationCountType.Highlight, 0); - fixNotificationCountOnDecryption(mockClient, event, {}); + fixNotificationCountOnDecryption(mockClient, event); expect(room.getRoomUnreadNotificationCount(NotificationCountType.Total)).toBe(1); expect(room.getRoomUnreadNotificationCount(NotificationCountType.Highlight)).toBe(1); @@ -153,7 +153,7 @@ describe("fixNotificationCountOnDecryption", () => { expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(1); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0); - fixNotificationCountOnDecryption(mockClient, threadEvent, {}); + fixNotificationCountOnDecryption(mockClient, threadEvent); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(2); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(1); @@ -163,7 +163,7 @@ describe("fixNotificationCountOnDecryption", () => { room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total, 0); room.setThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight, 0); - fixNotificationCountOnDecryption(mockClient, event, {}); + fixNotificationCountOnDecryption(mockClient, event); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0); @@ -187,7 +187,7 @@ describe("fixNotificationCountOnDecryption", () => { event: true, }); - fixNotificationCountOnDecryption(mockClient, unknownThreadEvent, {}); + fixNotificationCountOnDecryption(mockClient, unknownThreadEvent); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0); @@ -201,7 +201,7 @@ describe("fixNotificationCountOnDecryption", () => { event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false)); mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false)); - fixNotificationCountOnDecryption(mockClient, event, {}); + fixNotificationCountOnDecryption(mockClient, event); expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(0); expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0); }); @@ -213,7 +213,7 @@ describe("fixNotificationCountOnDecryption", () => { threadEvent.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false)); mockClient.getPushActionsForEvent = jest.fn().mockReturnValue(mkPushAction(false, false)); - fixNotificationCountOnDecryption(mockClient, event, {}); + fixNotificationCountOnDecryption(mockClient, event); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Total)).toBe(0); expect(room.getThreadUnreadNotificationCount(THREAD_ID, NotificationCountType.Highlight)).toBe(0); }); @@ -231,15 +231,4 @@ describe("fixNotificationCountOnDecryption", () => { room.setThreadUnreadNotificationCount("$123", NotificationCountType.Highlight, 5); expect(cb).toHaveBeenLastCalledWith({ highlight: 5 }, "$123"); }); - - it("should not increment notification count where event was already contributing notification", () => { - expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2); - expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0); - - event.getPushActions = jest.fn().mockReturnValue(mkPushAction(true, false)); - fixNotificationCountOnDecryption(mockClient, event, { actions: { notify: true, tweaks: {} } }); - - expect(room.getUnreadNotificationCount(NotificationCountType.Total)).toBe(2); - expect(room.getUnreadNotificationCount(NotificationCountType.Highlight)).toBe(0); - }); }); diff --git a/spec/unit/room-state.spec.ts b/spec/unit/room-state.spec.ts index 261c7b120e8..c8d249870d8 100644 --- a/spec/unit/room-state.spec.ts +++ b/spec/unit/room-state.spec.ts @@ -1037,12 +1037,7 @@ describe("RoomState", function () { // this event is a message after decryption decryptingRelatedEvent.event.type = EventType.RoomMessage; - decryptingRelatedEvent.emit( - MatrixEventEvent.Decrypted, - decryptingRelatedEvent, - undefined, - decryptingRelatedEvent.getPushDetails(), - ); + decryptingRelatedEvent.emit(MatrixEventEvent.Decrypted, decryptingRelatedEvent); expect(addLocationsSpy).not.toHaveBeenCalled(); }); diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index df57ea7355b..5dee474459f 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3428,13 +3428,13 @@ describe("Room", function () { expect(room.polls.get(pollStartEventId)).toBeUndefined(); // now emit a Decrypted event but keep the decryption failure - pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent, undefined, pollStartEvent.getPushDetails()); + pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent); // still do not expect a poll to show up for the room expect(room.polls.get(pollStartEventId)).toBeUndefined(); // clear decryption failure and emit a Decrypted event again isDecryptionFailureSpy.mockRestore(); - pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent, undefined, pollStartEvent.getPushDetails()); + pollStartEvent.emit(MatrixEventEvent.Decrypted, pollStartEvent); // the poll should now show up in the room's polls const poll = room.polls.get(pollStartEventId); diff --git a/src/client.ts b/src/client.ts index 7dc4d211f14..f50a1c74058 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1371,8 +1371,8 @@ export class MatrixClient extends TypedEventEmitter { - fixNotificationCountOnDecryption(this, event, pushDetails); + this.on(MatrixEventEvent.Decrypted, (event) => { + fixNotificationCountOnDecryption(this, event); }); // Like above, we have to listen for read receipts from ourselves in order to @@ -9851,23 +9851,26 @@ export class MatrixClient extends TypedEventEmitter 0) { + // TODO: Handle mentions received while the client is offline + // See also https://github.com/vector-im/element-web/issues/9069 + let newCount = currentHighlightCount; + if (newHighlight && !oldHighlight) newCount++; + if (!newHighlight && oldHighlight) newCount--; + + if (isThreadEvent) { + room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Highlight, newCount); } else { - oldValue = !!oldActions?.tweaks?.highlight; - newValue = !!actions?.tweaks?.highlight; + room.setUnreadNotificationCount(NotificationCountType.Highlight, newCount); } + } - if (oldValue !== newValue || count > 0) { - if (newValue && !oldValue) count++; - if (!newValue && oldValue) count--; + // Total count is used to typically increment a room notification counter, but not loudly highlight it. + const currentTotalCount = room.getUnreadCountForEventContext(NotificationCountType.Total, event); - if (isThreadEvent) { - room.setThreadUnreadNotificationCount(event.threadRootId, type, count); - } else { - room.setUnreadNotificationCount(type, count); - } + // `notify` is used in practice for incrementing the total count + const newNotify = !!actions?.notify; + + // The room total count is NEVER incremented by the server for encrypted rooms. We basically ignore + // the server here as it's always going to tell us to increment for encrypted events. + if (newNotify) { + if (isThreadEvent) { + room.setThreadUnreadNotificationCount( + event.threadRootId, + NotificationCountType.Total, + currentTotalCount + 1, + ); + } else { + room.setUnreadNotificationCount(NotificationCountType.Total, currentTotalCount + 1); } } } diff --git a/src/models/event.ts b/src/models/event.ts index 693fa87c2eb..72c6212db9a 100644 --- a/src/models/event.ts +++ b/src/models/event.ts @@ -226,7 +226,7 @@ export type MatrixEventHandlerMap = { * @param event - The matrix event which has been decrypted * @param err - The error that occurred during decryption, or `undefined` if no error occurred. */ - [MatrixEventEvent.Decrypted]: (event: MatrixEvent, err: Error | undefined, pushDetails: PushDetails) => void; + [MatrixEventEvent.Decrypted]: (event: MatrixEvent, err?: Error) => void; [MatrixEventEvent.BeforeRedaction]: (event: MatrixEvent, redactionEvent: MatrixEvent) => void; [MatrixEventEvent.VisibilityChange]: (event: MatrixEvent, visible: boolean) => void; [MatrixEventEvent.LocalEventIdReplaced]: (event: MatrixEvent) => void; @@ -916,8 +916,6 @@ export class MatrixEvent extends TypedEventEmitter Date: Mon, 31 Jul 2023 12:33:19 +0100 Subject: [PATCH 2/4] Fix wrong handling of encrypted rooms when loading them from sync accumulator --- src/sync.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/sync.ts b/src/sync.ts index 49bddc045f1..f2ec1b32b0f 100644 --- a/src/sync.ts +++ b/src/sync.ts @@ -28,8 +28,7 @@ import { Optional } from "matrix-events-sdk"; import type { SyncCryptoCallbacks } from "./common-crypto/CryptoBackend"; import { User, UserEvent } from "./models/user"; import { NotificationCountType, Room, RoomEvent } from "./models/room"; -import { promiseMapSeries, defer, deepCopy } from "./utils"; -import { IDeferred, noUnsafeEventProps, unsafeProp } from "./utils"; +import { deepCopy, defer, IDeferred, noUnsafeEventProps, promiseMapSeries, unsafeProp } from "./utils"; import { Filter } from "./filter"; import { EventTimeline } from "./models/event-timeline"; import { logger } from "./logger"; @@ -1316,7 +1315,7 @@ export class SyncApi { const ephemeralEvents = this.mapSyncEventsFormat(joinObj.ephemeral); const accountDataEvents = this.mapSyncEventsFormat(joinObj.account_data); - const encrypted = client.isRoomEncrypted(room.roomId); + const encrypted = this.isRoomEncrypted(room, stateEvents, events); // We store the server-provided value first so it's correct when any of the events fire. if (joinObj.unread_notifications) { /** @@ -1324,6 +1323,9 @@ export class SyncApi { * bother setting it here. We trust our calculations better than the * server's for this case, and therefore will assume that our non-zero * count is accurate. + * XXX: this is known faulty as the push rule for `.m.room.encrypted` may be disabled so server + * may issue notification counts of 0 which we wrongly trust. + * https://github.com/matrix-org/matrix-spec-proposals/pull/2654 would fix this * * @see import("./client").fixNotificationCountOnDecryption */ @@ -1721,6 +1723,20 @@ export class SyncApi { }); } + private findEncryptionEvent(events?: MatrixEvent[]): MatrixEvent | undefined { + return events?.find((e) => e.getType() === EventType.RoomEncryption && e.getStateKey() === ""); + } + + // When processing the sync response we cannot rely on MatrixClient::isRoomEncrypted before we actually + // inject the events into the room object, so we have to inspect the events themselves. + private isRoomEncrypted(room: Room, stateEventList: MatrixEvent[], timelineEventList?: MatrixEvent[]): boolean { + return ( + this.client.isRoomEncrypted(room.roomId) || + !!this.findEncryptionEvent(stateEventList) || + !!this.findEncryptionEvent(timelineEventList) + ); + } + /** * Injects events into a room's model. * @param stateEventList - A list of state events. This is the state From 1e9648412033d027ba8bb427b4f066040003cc29 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 31 Jul 2023 12:33:43 +0100 Subject: [PATCH 3/4] Tidy up code, removing sections which didn't make any difference --- src/client.ts | 38 +++++++++++++------------------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/src/client.ts b/src/client.ts index f50a1c74058..a50acbc2ad6 100644 --- a/src/client.ts +++ b/src/client.ts @@ -9858,19 +9858,8 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri const room = cli.getRoom(event.getRoomId()); if (!room || !ourUserId || !eventId) return; - const oldActions = event.getPushActions(); - const actions = cli.getPushActionsForEvent(event, true); - const isThreadEvent = !!event.threadRootId && !event.isThreadRoot; - const currentHighlightCount = room.getUnreadCountForEventContext(NotificationCountType.Highlight, event); - - // Ensure the unread counts are kept up to date if the event is encrypted - // We also want to make sure that the notification count goes up if we already - // have encrypted events to avoid other code from resetting 'highlight' to zero. - const oldHighlight = !!oldActions?.tweaks?.highlight; - const newHighlight = !!actions?.tweaks?.highlight; - let hasReadEvent; if (isThreadEvent) { const thread = room.getThread(event.threadRootId); @@ -9892,13 +9881,17 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri return; } - if (oldHighlight !== newHighlight || currentHighlightCount > 0) { + const actions = cli.getPushActionsForEvent(event, true); + + // Ensure the unread counts are kept up to date if the event is encrypted + // We also want to make sure that the notification count goes up if we already + // have encrypted events to avoid other code from resetting 'highlight' to zero. + const newHighlight = !!actions?.tweaks?.highlight; + + if (newHighlight) { // TODO: Handle mentions received while the client is offline // See also https://github.com/vector-im/element-web/issues/9069 - let newCount = currentHighlightCount; - if (newHighlight && !oldHighlight) newCount++; - if (!newHighlight && oldHighlight) newCount--; - + const newCount = room.getUnreadCountForEventContext(NotificationCountType.Highlight, event) + 1; if (isThreadEvent) { room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Highlight, newCount); } else { @@ -9906,23 +9899,18 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri } } - // Total count is used to typically increment a room notification counter, but not loudly highlight it. - const currentTotalCount = room.getUnreadCountForEventContext(NotificationCountType.Total, event); - // `notify` is used in practice for incrementing the total count const newNotify = !!actions?.notify; // The room total count is NEVER incremented by the server for encrypted rooms. We basically ignore // the server here as it's always going to tell us to increment for encrypted events. if (newNotify) { + // Total count is used to typically increment a room notification counter, but not loudly highlight it. + const newCount = room.getUnreadCountForEventContext(NotificationCountType.Total, event) + 1; if (isThreadEvent) { - room.setThreadUnreadNotificationCount( - event.threadRootId, - NotificationCountType.Total, - currentTotalCount + 1, - ); + room.setThreadUnreadNotificationCount(event.threadRootId, NotificationCountType.Total, newCount); } else { - room.setUnreadNotificationCount(NotificationCountType.Total, currentTotalCount + 1); + room.setUnreadNotificationCount(NotificationCountType.Total, newCount); } } } From 4169ccb0aef6a96cd5c1b67304867c53fd795f71 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Mon, 31 Jul 2023 12:53:01 +0100 Subject: [PATCH 4/4] Add test --- spec/integ/matrix-client-syncing.spec.ts | 63 ++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/spec/integ/matrix-client-syncing.spec.ts b/spec/integ/matrix-client-syncing.spec.ts index baec3039ff4..45b6f42b4a6 100644 --- a/spec/integ/matrix-client-syncing.spec.ts +++ b/spec/integ/matrix-client-syncing.spec.ts @@ -38,6 +38,7 @@ import { Room, IndexedDBStore, RelationType, + EventType, } from "../../src"; import { ReceiptType } from "../../src/@types/read_receipts"; import { UNREAD_THREAD_NOTIFICATIONS } from "../../src/@types/sync"; @@ -1590,6 +1591,68 @@ describe("MatrixClient syncing", () => { }); }); }); + + it("should apply encrypted notification logic for events within the same sync blob", async () => { + const roomId = "!room123:server"; + const syncData = { + rooms: { + join: { + [roomId]: { + ephemeral: { + events: [], + }, + timeline: { + events: [ + utils.mkEvent({ + room: roomId, + event: true, + skey: "", + type: EventType.RoomEncryption, + content: {}, + }), + utils.mkMessage({ + room: roomId, + user: otherUserId, + msg: "hello", + }), + ], + }, + state: { + events: [ + utils.mkMembership({ + room: roomId, + mship: "join", + user: otherUserId, + }), + utils.mkMembership({ + room: roomId, + mship: "join", + user: selfUserId, + }), + utils.mkEvent({ + type: "m.room.create", + room: roomId, + user: selfUserId, + content: { + creator: selfUserId, + }, + }), + ], + }, + }, + }, + }, + } as unknown as ISyncResponse; + + httpBackend!.when("GET", "/sync").respond(200, syncData); + client!.startClient(); + + await Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]); + + const room = client!.getRoom(roomId)!; + expect(room).toBeInstanceOf(Room); + expect(room.getRoomUnreadNotificationCount(NotificationCountType.Total)).toBe(0); + }); }); describe("of a room", () => {