Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix external guest access url for unencrypted rooms #12345

Merged
merged 8 commits into from
Mar 28, 2024
Merged

Fix external guest access url for unencrypted rooms #12345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a21032a
use unencrypted calls in unencrypted rooms (make external call links …
toger5 Mar 15, 2024
a8b5576
use same logic in Call.ts
toger5 Mar 15, 2024
a6754c3
Merge branch 'develop' into toger5/fix-link-share-encryption
toger5 Mar 26, 2024
b14e8ea
fix tests
toger5 Mar 26, 2024
21fcab4
fix test
toger5 Mar 26, 2024
0f70ec2
Merge branch 'develop' into toger5/fix-link-share-encryption
toger5 Mar 26, 2024
fba9055
Merge branch 'develop' into toger5/fix-link-share-encryption
toger5 Mar 27, 2024
42ce14a
Merge branch 'develop' into toger5/fix-link-share-encryption
toger5 Mar 27, 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
2 changes: 1 addition & 1 deletion src/hooks/room/useRoomCall.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ export const useRoomCall = (
url.pathname = "/room/";
// Set params for the sharable url
url.searchParams.set("roomId", room.roomId);
url.searchParams.set("perParticipantE2EE", "true");
if (room.hasEncryptionStateEvent()) url.searchParams.set("perParticipantE2EE", "true");
Copy link
Member

Choose a reason for hiding this comment

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

This is different from the logic used for the widget URL: https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/models/Call.ts#L784 - the other one seems more correct. Shouldn't they just share a function to decide whether the call is encrypted, and/or some common code to generate the common parts of the URL?

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 other one says it's deprecated. Maybe updating both makes sense though.

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, but that says to use CryptoApi.isEncryptionEnabledInRoom instead. I think the problem with just checking for the presence of a state event is that it can be redacted, potentially by a nasty homeserver, but the clients should still treat the room as encrypted. This might let the HS force calls to be unencrypted when they ought to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an sync fn however. that would trickle down quiet far. I would need to make Call.get async (because that needs the widget, which needs the widget url parameters that contain on the encryption flag. Is this what we want?
Its also super important, that both clients have the same understanding of the current encryption state of the room. Could CryptoApi remembering that state one one client but another client does not have this information bring this out of sync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am doing the "use isEncryptionEnabledInRoom" change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i found a fix for it: 6c03bb4

Copy link
Member

Choose a reason for hiding this comment

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

Eugh, sorry - I didn't notice this was async, and yes, having Call.get be async is not ideal. Chatting to the crypto team suggests that actually the presence of the state event might be fine after all, so I think whichever you think do whatever you think is best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated back to not be async and use hasEncryptionStateEvent

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, sorry for the run-around

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no worries. I think it was worth investigating if it can be done using the async fn.

for (const server of calculateRoomVia(room)) {
url.searchParams.set("viaServers", server);
}
Expand Down
5 changes: 4 additions & 1 deletion src/models/Call.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,7 +778,10 @@ export class ElementCall extends Call {
overwriteData: IWidgetData,
): IWidgetData {
let perParticipantE2EE = false;
if (client.isRoomEncrypted(roomId) && !SettingsStore.getValue("feature_disable_call_per_sender_encryption"))
if (
client.getRoom(roomId)?.hasEncryptionStateEvent() &&
!SettingsStore.getValue("feature_disable_call_per_sender_encryption")
)
perParticipantE2EE = true;
return {
...currentData,
Expand Down
17 changes: 16 additions & 1 deletion test/components/views/rooms/RoomHeader-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,9 +563,10 @@ describe("RoomHeader", () => {
const { container } = render(<RoomHeader room={room} />, getWrapper());
expect(getByLabelText(container, _t("voip|get_call_link"))).toBeInTheDocument();
});
it("opens the share dialog with the correct share link", () => {
it("opens the share dialog with the correct share link in an encrypted room", () => {
jest.spyOn(room, "getJoinRule").mockReturnValue(JoinRule.Public);
jest.spyOn(SdkContextClass.instance.roomViewStore, "isViewingCall").mockReturnValue(true);
jest.spyOn(room, "hasEncryptionStateEvent").mockReturnValue(true);

const { container } = render(<RoomHeader room={room} />, getWrapper());
const modalSpy = jest.spyOn(Modal, "createDialog");
Expand All @@ -583,6 +584,20 @@ describe("RoomHeader", () => {
});
expect(arg1.target.toString()).toEqual(target);
});

it("share dialog has correct link in an unencrypted room", () => {
jest.spyOn(room, "getJoinRule").mockReturnValue(JoinRule.Public);
jest.spyOn(room, "hasEncryptionStateEvent").mockReturnValue(false);
jest.spyOn(SdkContextClass.instance.roomViewStore, "isViewingCall").mockReturnValue(true);

const { container } = render(<RoomHeader room={room} />, getWrapper());
const modalSpy = jest.spyOn(Modal, "createDialog");
fireEvent.click(getByLabelText(container, _t("voip|get_call_link")));
const target =
"https://guest_spa_url.com/room/#/!1:example.org?roomId=%211%3Aexample.org&viaServers=example.org";
const arg1 = modalSpy.mock.calls[0][1] as any;
expect(arg1.target.toString()).toEqual(target);
});
});

describe("public room", () => {
Expand Down
5 changes: 2 additions & 3 deletions test/models/Call-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,7 +1039,7 @@ describe("ElementCall", () => {
call.destroy();
const addWidgetSpy = jest.spyOn(WidgetStore.instance, "addVirtualWidget");
// If a room is not encrypted we will never add the perParticipantE2EE flag.
client.isRoomEncrypted.mockReturnValue(true);
const roomSpy = jest.spyOn(room, "hasEncryptionStateEvent").mockReturnValue(true);

// should create call with perParticipantE2EE flag
ElementCall.create(room);
Expand All @@ -1049,8 +1049,7 @@ describe("ElementCall", () => {
enabledSettings.add("feature_disable_call_per_sender_encryption");
expect(Call.get(room)?.widget?.data?.perParticipantE2EE).toBe(false);
enabledSettings.delete("feature_disable_call_per_sender_encryption");

client.isRoomEncrypted.mockClear();
roomSpy.mockRestore();
addWidgetSpy.mockRestore();
});

Expand Down
1 change: 1 addition & 0 deletions test/test-utils/test-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,7 @@ export function mkStubRoom(
isElementVideoRoom: jest.fn().mockReturnValue(false),
isSpaceRoom: jest.fn().mockReturnValue(false),
isCallRoom: jest.fn().mockReturnValue(false),
hasEncryptionStateEvent: jest.fn().mockReturnValue(false),
loadMembersIfNeeded: jest.fn(),
maySendMessage: jest.fn().mockReturnValue(true),
myUserId: client?.getUserId(),
Expand Down
Loading