Skip to content

Commit

Permalink
Fix wrong handling of encrypted rooms when loading them from sync acc…
Browse files Browse the repository at this point in the history 
…umulator (#3640)

* Revert "Ensure we don't overinflate the total notification count (#3634)"

This reverts commit fd0c4a7.

* Fix wrong handling of encrypted rooms when loading them from sync accumulator

* Tidy up code, removing sections which didn't make any difference

* Add test
  • Loading branch information
@t3chguy
t3chguy authored Aug 2, 2023
1 parent 2e9b34e commit 2172f28
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 28 deletions.
63 changes: 63 additions & 0 deletions spec/integ/matrix-client-syncing.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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", () => {
Expand Down
38 changes: 13 additions & 25 deletions src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9862,19 +9862,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);
Expand All @@ -9896,37 +9885,36 @@ 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 {
room.setUnreadNotificationCount(NotificationCountType.Highlight, newCount);
}
}

// 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);
}
}
}
22 changes: 19 additions & 3 deletions src/sync.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -1316,14 +1315,17 @@ 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) {
/**
* We track unread notifications ourselves in encrypted rooms, so don't
* 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
*/
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2172f28

Please sign in to comment.