Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add expire_ts compatibility to matrixRTC #4032

Merged
merged 10 commits into from
Jan 29, 2024
Merged

Add expire_ts compatibility to matrixRTC #4032

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bafb2de
add expire_ts compatibility to matrixRTC
toger5 Jan 23, 2024
dd2d0c9
add expire_ts
toger5 Jan 24, 2024
e34c22b
rename expire_ts -> expires_ts
toger5 Jan 24, 2024
80aab5d
allow events without `expires`
toger5 Jan 24, 2024
91e3f5b
fix test for expires_ts
toger5 Jan 26, 2024
b7fbcfd
comment clarification
toger5 Jan 26, 2024
d797ac2
add comment where one needs to use the origin_server_ts
toger5 Jan 26, 2024
5f94cd7
add additional expires_ts tests
toger5 Jan 26, 2024
348c37f
fix fake timer
toger5 Jan 26, 2024
251c6ee
change priority order to favor expires
toger5 Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions spec/unit/matrixrtc/CallMembership.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,12 @@ function makeMockEvent(originTs = 0): MatrixEvent {
}

describe("CallMembership", () => {
it("rejects membership with no expiry", () => {
it("rejects membership with no expiry and no expires_ts", () => {
expect(() => {
new CallMembership(makeMockEvent(), Object.assign({}, membershipTemplate, { expires: undefined }));
new CallMembership(
makeMockEvent(),
Object.assign({}, membershipTemplate, { expires: undefined, expires_ts: undefined }),
);
}).toThrow();
});

Expand All @@ -57,6 +60,16 @@ describe("CallMembership", () => {
new CallMembership(makeMockEvent(), Object.assign({}, membershipTemplate, { scope: undefined }));
}).toThrow();
});
it("rejects with malformatted expires_ts", () => {
expect(() => {
new CallMembership(makeMockEvent(), Object.assign({}, membershipTemplate, { expires_ts: "string" }));
}).toThrow();
});
it("rejects with malformatted expires", () => {
expect(() => {
new CallMembership(makeMockEvent(), Object.assign({}, membershipTemplate, { expires: "string" }));
}).toThrow();
});

it("uses event timestamp if no created_ts", () => {
const membership = new CallMembership(makeMockEvent(12345), membershipTemplate);
Expand All @@ -71,11 +84,19 @@ describe("CallMembership", () => {
expect(membership.createdTs()).toEqual(67890);
});

it("computes absolute expiry time", () => {
it("computes absolute expiry time based on expires", () => {
const membership = new CallMembership(makeMockEvent(1000), membershipTemplate);
expect(membership.getAbsoluteExpiry()).toEqual(5000 + 1000);
});

it("computes absolute expiry time based on expires_ts", () => {
const membership = new CallMembership(
makeMockEvent(1000),
Object.assign({}, membershipTemplate, { expires: undefined, expires_ts: 6000 }),
);
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);
Expand Down
12 changes: 9 additions & 3 deletions spec/unit/matrixrtc/MatrixRTCSession.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ describe("MatrixRTCSession", () => {
});

it("sends a membership event when joining a call", () => {
jest.useFakeTimers();
sess!.joinRoomSession([mockFocus]);

expect(client.sendStateEvent).toHaveBeenCalledWith(
mockRoom!.roomId,
EventType.GroupCallMemberPrefix,
Expand All @@ -227,13 +227,15 @@ describe("MatrixRTCSession", () => {
call_id: "",
device_id: "AAAAAAA",
expires: 3600000,
expires_ts: Date.now() + 3600000,
foci_active: [{ type: "mock" }],
membershipID: expect.stringMatching(".*"),
},
],
},
"@alice:example.org",
);
jest.useRealTimers();
});

it("does nothing if join called when already joined", () => {
Expand Down Expand Up @@ -291,6 +293,7 @@ describe("MatrixRTCSession", () => {
call_id: "",
device_id: "AAAAAAA",
expires: 3600000 * 2,
expires_ts: 1000 + 3600000 * 2,
foci_active: [{ type: "mock" }],
created_ts: 1000,
membershipID: expect.stringMatching(".*"),
Expand Down Expand Up @@ -510,7 +513,7 @@ describe("MatrixRTCSession", () => {
});
});

it("Does not emits if no membership changes", () => {
it("Does not emit if no membership changes", () => {
const mockRoom = makeMockRoom([membershipTemplate]);
sess = MatrixRTCSession.roomSessionForRoom(client, mockRoom);

Expand Down Expand Up @@ -591,6 +594,7 @@ describe("MatrixRTCSession", () => {
call_id: "",
device_id: "AAAAAAA",
expires: 3600000,
expires_ts: Date.now() + 3600000,
foci_active: [mockFocus],
membershipID: expect.stringMatching(".*"),
},
Expand All @@ -605,7 +609,7 @@ describe("MatrixRTCSession", () => {

it("fills in created_ts for other memberships on update", () => {
client.sendStateEvent = jest.fn();

jest.useFakeTimers();
const mockRoom = makeMockRoom([
Object.assign({}, membershipTemplate, {
device_id: "OTHERDEVICE",
Expand Down Expand Up @@ -635,13 +639,15 @@ describe("MatrixRTCSession", () => {
call_id: "",
device_id: "AAAAAAA",
expires: 3600000,
expires_ts: Date.now() + 3600000,
foci_active: [mockFocus],
membershipID: expect.stringMatching(".*"),
},
],
},
"@alice:example.org",
);
jest.useRealTimers();
});

it("collects keys from encryption events", () => {
Expand Down
37 changes: 31 additions & 6 deletions src/matrixrtc/CallMembership.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ export interface CallMembershipData {
scope: CallScope;
device_id: string;
created_ts?: number;
expires: number;
expires?: number;
expires_ts?: number;
foci_active?: Focus[];
membershipID: string;
}
Expand All @@ -41,7 +42,20 @@ export class CallMembership {
private parentEvent: MatrixEvent,
private data: CallMembershipData,
) {
if (typeof data.expires !== "number") throw new Error("Malformed membership: expires must be numeric");
if (!(data.expires || data.expires_ts)) {
throw new Error("Malformed membership: expires_ts or expires must be present");
}
if (data.expires) {
if (typeof data.expires !== "number") {
throw new Error("Malformed membership: expires must be numeric");
}
}
if (data.expires_ts) {
if (typeof data.expires_ts !== "number") {
throw new Error("Malformed membership: expires_ts must be numeric");
}
}

if (typeof data.device_id !== "string") throw new Error("Malformed membership event: device_id must be string");
if (typeof data.call_id !== "string") throw new Error("Malformed membership event: call_id must be string");
if (typeof data.scope !== "string") throw new Error("Malformed membership event: scope must be string");
Expand Down Expand Up @@ -77,16 +91,27 @@ export class CallMembership {
}

public getAbsoluteExpiry(): number {
return this.createdTs() + this.data.expires;
if (this.data.expires) {
return this.createdTs() + this.data.expires;
} else {
// We know it exists because we checked for this in the constructor.
return this.data.expires_ts!;
}
}

// gets the expiry time of the event, converted into the device's local time
public getLocalExpiry(): number {
const relativeCreationTime = this.parentEvent.getTs() - this.createdTs();
if (this.data.expires_ts) {
// With expires_ts we cannot convert to local time.
// TODO: Check the server timestamp and compute a diff to local time.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we already have a follow-up ticket for that TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I refactored this to use its own class for tracking the server/client diff with unimplmeneted clock sync diffs.
(It actally computes the diff once we get an event with an age property and from that point on its using this offset)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactor is now on a different branch so that we can merge this "low risk"/"lightweight" version.

We will do the server-client-time sync properly here: #4038

return this.data.expires_ts;
} else {
const relativeCreationTime = this.parentEvent.getTs() - this.createdTs();

const localCreationTs = this.parentEvent.localTimestamp - relativeCreationTime;
const localCreationTs = this.parentEvent.localTimestamp - relativeCreationTime;

return localCreationTs + this.data.expires;
return localCreationTs + this.data.expires!;
}
}

public getMsUntilExpiry(): number {
Expand Down
3 changes: 3 additions & 0 deletions src/matrixrtc/MatrixRTCSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,9 @@ export class MatrixRTCSession extends TypedEventEmitter<MatrixRTCSessionEvent, M
};

if (prevMembership) m.created_ts = prevMembership.createdTs();
if (m.created_ts) m.expires_ts = m.created_ts + (m.expires ?? 0);
// TODO: Date.now() should be the origin_server_ts (now).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have the follow-up ticket for that TODO already?

else m.expires_ts = Date.now() + (m.expires ?? 0);

return m;
}
Expand Down
Loading