Skip to content

Commit

Permalink
Merge branch 'develop' into andybalaam/move-related-of-redacted
Browse files Browse the repository at this point in the history
  • Loading branch information
andybalaam authored Oct 18, 2023
2 parents f540515 + 11661bb commit 9fd2bb6
Show file tree
Hide file tree
Showing 9 changed files with 351 additions and 256 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/cypress.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:
# from creeping in. They take a long time to run and consume 4 concurrent runners.
if: github.event.workflow_run.event == 'merge_group'

uses: matrix-org/matrix-react-sdk/.github/workflows/cypress.yaml@66854039a33ed6cfe1fc635ff2daa8bb261c0b56
uses: matrix-org/matrix-react-sdk/.github/workflows/cypress.yaml@v1.11.47-rc.1
permissions:
actions: read
issues: read
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/pull_request.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:
if: github.event.action == 'opened'
steps:
- name: Check membership
uses: tspascoal/get-user-teams-membership@37c08f7b52a72ca95d12af2e7ab2553ca9adf13b # v2
uses: tspascoal/get-user-teams-membership@ba78054988f58bea69b7c6136d563236f8ed2fc0 # v3
id: teams
with:
username: ${{ github.event.pull_request.user.login }}
Expand Down
8 changes: 8 additions & 0 deletions docs/SUMMARY.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Summary

- [Introduction](../README.md)

# Deep dive

- [Storage notes](storage-notes.md)
- [Unverified devices](warning-on-unverified-devices.md)
Original file line number Diff line number Diff line change
@@ -1,31 +1,29 @@
Random notes from Matthew on the two possible approaches for warning users about unexpected
unverified devices popping up in their rooms....

Original idea...
================
# Original idea...

Warn when an existing user adds an unknown device to a room.

Warn when a user joins the room with unverified or unknown devices.

Warn when you initial sync if the room has any unverified devices in it.
^ this is good enough if we're doing local storage.
OR, better:
^ this is good enough if we're doing local storage.
OR, better:
Warn when you initial sync if the room has any new undefined devices since you were last there.
=> This means persisting the rooms that devices are in, across initial syncs.
=> This means persisting the rooms that devices are in, across initial syncs.


Updated idea...
===============
# Updated idea...

Warn when the user tries to send a message:
- If the room has unverified devices which the user has not yet been told about in the context of this room
...or in the context of this user? currently all verification is per-user, not per-room.

- If the room has unverified devices which the user has not yet been told about in the context of this room
...or in the context of this user? currently all verification is per-user, not per-room.
...this should be good enough.

- so track whether we have warned the user or not about unverified devices - blocked, unverified, verified, unverified_warned.
- so track whether we have warned the user or not about unverified devices - blocked, unverified, verified, unverified_warned.
throw an error when trying to encrypt if there are pure unverified devices there
app will have to search for the devices which are pure unverified to warn about them - have to do this from MembersList anyway?
- or megolm could warn which devices are causing the problems.
- or megolm could warn which devices are causing the problems.

Why do we wait to establish outbound sessions? It just makes a horrible pause when we first try to send a message... but could otherwise unnecessarily consume resources?
Why do we wait to establish outbound sessions? It just makes a horrible pause when we first try to send a message... but could otherwise unnecessarily consume resources?
63 changes: 63 additions & 0 deletions spec/unit/models/thread.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,69 @@ describe("Thread", () => {
});
});
});

describe("addEvent", () => {
describe("Given server support for threads", () => {
let previousThreadHasServerSideSupport: FeatureSupport;

beforeAll(() => {
previousThreadHasServerSideSupport = Thread.hasServerSideSupport;
Thread.hasServerSideSupport = FeatureSupport.Stable;
});

afterAll(() => {
Thread.hasServerSideSupport = previousThreadHasServerSideSupport;
});

it("Adds events even if they appear out of order", async () => {
// Given a thread exists
const client = createClient();
const user = "@alice:matrix.org";
const room = "!room:z";
const thread = await createThread(client, user, room);
const prevNumEvents = thread.timeline.length;

// When two messages come in but the later one has an older timestamp
const message1 = createThreadMessage(thread.id, user, room, "message1");
const message2 = createThreadMessage(thread.id, user, room, "message2");
message2.localTimestamp -= 10000;

await thread.addEvent(message1, false);
await thread.addEvent(message2, false);

// Then both events end up in the timeline
expect(thread.timeline.length - prevNumEvents).toEqual(2);
const lastEvent = thread.timeline.at(-1)!;
const secondLastEvent = thread.timeline.at(-2)!;
expect(lastEvent).toBe(message2);
expect(secondLastEvent).toBe(message1);
});

it("Adds events to start even if they appear out of order", async () => {
// Given a thread exists
const client = createClient();
const user = "@alice:matrix.org";
const room = "!room:z";
const thread = await createThread(client, user, room);
const prevNumEvents = thread.timeline.length;

// When two messages come in but the later one has an older timestamp
const message1 = createThreadMessage(thread.id, user, room, "message1");
const message2 = createThreadMessage(thread.id, user, room, "message2");
message2.localTimestamp -= 10000;

await thread.addEvent(message1, false);
await thread.addEvent(message2, true);

// Then both events end up in the timeline
expect(thread.timeline.length - prevNumEvents).toEqual(2);
const lastEvent = thread.timeline.at(-1)!;
const firstEvent = thread.timeline.at(0)!;
expect(lastEvent).toBe(message1);
expect(firstEvent).toBe(message2);
});
});
});
});

/**
Expand Down
10 changes: 4 additions & 6 deletions src/matrixrtc/MatrixRTCSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,6 @@ export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, M
memberships: this.makeNewMemberships(memberships, myCallMemberEvent, myPrevMembership),
};

let resendDelay = 0;
try {
await this.client.sendStateEvent(
this.room.roomId,
Expand All @@ -438,13 +437,12 @@ export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, M
logger.info(`Sent updated call member event.`);

// check periodically to see if we need to refresh our member event
if (this.isJoined()) resendDelay = MEMBER_EVENT_CHECK_PERIOD;
if (this.isJoined()) {
this.memberEventTimeout = setTimeout(this.triggerCallMembershipEventUpdate, MEMBER_EVENT_CHECK_PERIOD);
}
} catch (e) {
resendDelay = CALL_MEMBER_EVENT_RETRY_DELAY_MIN + Math.random() * 2000;
const resendDelay = CALL_MEMBER_EVENT_RETRY_DELAY_MIN + Math.random() * 2000;
logger.warn(`Failed to send call member event: retrying in ${resendDelay}`);
}

if (resendDelay) {
await new Promise((resolve) => setTimeout(resolve, resendDelay));
await this.triggerCallMembershipEventUpdate();
}
Expand Down
21 changes: 15 additions & 6 deletions src/models/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,9 +416,12 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
}

/**
* Gets the event as though it would appear unencrypted. If the event is already not
* encrypted, it is simply returned as-is.
* @returns The event in wire format.
* Gets the event as it would appear if it had been sent unencrypted.
*
* If the event is encrypted, we attempt to mock up an event as it would have looked had the sender not encrypted it.
* If the event is not encrypted, a copy of it is simply returned as-is.
*
* @returns A shallow copy of the event, in wire format, as it would have been had it not been encrypted.
*/
public getEffectiveEvent(): IEvent {
const content = Object.assign({}, this.getContent()); // clone for mutation
Expand Down Expand Up @@ -1633,15 +1636,21 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
}

/**
* Summarise the event as JSON. This is currently used by React SDK's view
* event source feature and Seshat's event indexing, so take care when
* adjusting the output here.
* Summarise the event as JSON.
*
* If encrypted, include both the decrypted and encrypted view of the event.
*
* This is named `toJSON` for use with `JSON.stringify` which checks objects
* for functions named `toJSON` and will call them to customise the output
* if they are defined.
*
* **WARNING** Do not log the result of this method; otherwise, it will end up
* in rageshakes, leading to a privacy violation.
*
* @deprecated Prefer to use {@link MatrixEvent#getEffectiveEvent} or similar.
* This method will be removed soon; it is too easy to use it accidentally
* and cause a privacy violation (cf https://github.com/vector-im/element-web/issues/26380).
* In any case, the value it returns is not a faithful serialization of the object.
*/
public toJSON(): object {
const event = this.getEffectiveEvent();
Expand Down
29 changes: 13 additions & 16 deletions src/models/thread.ts
Original file line number Diff line number Diff line change
Expand Up @@ -430,22 +430,19 @@ export class Thread extends ReadReceipt<ThreadEmittedEvents, ThreadEventHandlerM
// If initial events have not been fetched, we are OK to throw away
// this event, because we are about to fetch all the events for this
// thread from the server.
//
// If not, this looks like a bug - we should always add the event to
// the thread. See https://github.com/vector-im/element-web/issues/26254
logger.warn(
`Not adding event ${event.getId()} to thread timeline!
isNewestReply=${isNewestReply}
event.localTimestamp=${event.localTimestamp}
!!lastReply=${!!lastReply}
lastReply?.getId()=${lastReply?.getId()}
lastReply?.localTimestamp=${lastReply?.localTimestamp}
toStartOfTimeline=${toStartOfTimeline}
Thread.hasServerSideSupport=${Thread.hasServerSideSupport}
event.isRelation(RelationType.Annotation)=${event.isRelation(RelationType.Annotation)}
event.isRelation(RelationType.Replace)=${event.isRelation(RelationType.Replace)}
`,
);

// Otherwise, we should add it, but we suspect it is out of order.
if (toStartOfTimeline) {
// If we're adding at the start of the timeline, it doesn't
// matter that it's out of order.
this.addEventToTimeline(event, toStartOfTimeline);
} else {
// We think this event might be out of order, because isNewestReply
// is false (otherwise we would have gone into the earlier if
// clause), so try to insert it in the right place based on
// timestamp.
this.insertEventIntoTimeline(event);
}
}

if (
Expand Down
Loading

0 comments on commit 9fd2bb6

Please sign in to comment.