Merge pull request #4367 from RaphaelFakhri/RaphaelFakhri/shared-key-storage-key

Read and write the room shared key under its single-prefixed storage key

docker-compose-playwright.yml

@@ -19,7 +19,7 @@ services:
       - ./backend/playwright_homeserver-othersite.yaml:/data/cfg/homeserver.yaml:Z
   element-web:
     # Pin to a SHA so that upstream cannot break our tests. Renovate handles regular updates.
-    image: ghcr.io/element-hq/element-web:develop@sha256:79e47d8d5a93848403cb83042a405d8ed233a7727c499fa53b4b39a80f090be4
+    image: ghcr.io/element-hq/element-web:develop@sha256:ee8ce55012dda2c097147859aece32802ce7a9174e40c3711c56f75ccfe1b169
   element-web-1:
     # Pin to a SHA so that upstream cannot break our tests. Renovate handles regular updates.
-    image: ghcr.io/element-hq/element-web:develop@sha256:79e47d8d5a93848403cb83042a405d8ed233a7727c499fa53b4b39a80f090be4
+    image: ghcr.io/element-hq/element-web:develop@sha256:ee8ce55012dda2c097147859aece32802ce7a9174e40c3711c56f75ccfe1b169

src/e2ee/sharedKeyManagement.test.ts

@@ -5,11 +5,24 @@ SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial
 Please see LICENSE in the repository root for full details.
 */
 
-import { afterEach, describe, expect, test } from "vitest";
+import { afterEach, describe, expect, test, vi } from "vitest";
+import { act, renderHook } from "@testing-library/react";
+import { E2eeType } from "@element-hq/matrixrtc-sdk";
 
-import { getKeyForRoom, saveKeyForRoom } from "./sharedKeyManagement";
+import {
+  getKeyForRoom,
+  saveKeyForRoom,
+  useRoomEncryptionSystem,
+} from "./sharedKeyManagement";
+
+vi.mock("../ClientContext", () => {
+  const room = { hasEncryptionStateEvent: (): boolean => false };
+  const client = { getRoom: (): typeof room => room };
+  return { useClient: (): { client: typeof client } => ({ client }) };
+});
 
 const roomId = "!room:example.org";
+const otherRoomId = "!other:example.org";
 
 describe("getKeyForRoom", () => {
   afterEach(() => {
@@ -33,4 +46,58 @@ describe("getKeyForRoom", () => {
   test("has no key to offer for a room it has never seen", () => {
     expect(getKeyForRoom(roomId)).toBeNull();
   });
+
+  test("prefers the stored key when the parameters are for another room", () => {
+    saveKeyForRoom(roomId, "stored");
+    window.location.hash = `#?roomId=${encodeURIComponent(otherRoomId)}&password=other-room`;
+
+    expect(getKeyForRoom(roomId)).toBe("stored");
+  });
+
+  test("ignores the password in the parameters for another room", () => {
+    window.location.hash = `#?roomId=${encodeURIComponent(otherRoomId)}&password=other-room`;
+
+    expect(getKeyForRoom(roomId)).toBeNull();
+  });
+});
+
+describe("useRoomEncryptionSystem", () => {
+  afterEach(() => {
+    window.location.hash = "#";
+    localStorage.clear();
+  });
+
+  test("picks up a key saved for the room after it rendered", () => {
+    const { result } = renderHook(() => useRoomEncryptionSystem(roomId));
+    expect(result.current).toEqual({ kind: E2eeType.NONE });
+
+    act(() => saveKeyForRoom(roomId, "stored"));
+
+    expect(result.current).toEqual({
+      kind: E2eeType.SHARED_KEY,
+      secret: "stored",
+    });
+  });
+
+  test("stores a key from the parameters under the room's key only", () => {
+    window.location.hash = `#?roomId=${encodeURIComponent(roomId)}&password=from-the-link`;
+
+    renderHook(() => useRoomEncryptionSystem(roomId));
+
+    expect(Object.keys(localStorage)).toEqual([`room-shared-key-${roomId}`]);
+    expect(getKeyForRoom(roomId)).toBe("from-the-link");
+  });
+
+  test("keeps the stored key when the parameters are for another room", () => {
+    saveKeyForRoom(roomId, "stored");
+    window.location.hash = `#?roomId=${encodeURIComponent(otherRoomId)}&password=other-room`;
+
+    const { result } = renderHook(() => useRoomEncryptionSystem(roomId));
+
+    expect(result.current).toEqual({
+      kind: E2eeType.SHARED_KEY,
+      secret: "stored",
+    });
+    expect(localStorage.getItem(`room-shared-key-${roomId}`)).toBe("stored");
+  });
 });

src/e2ee/sharedKeyManagement.ts

@@ -59,22 +59,27 @@ const useRoomSharedKey = (
 
 /**
  * The shared key for a room, preferring one supplied in the parameters Element
- * Call was started with over whatever is in local storage.
+ * Call was started with over whatever is in local storage. If the parameters
+ * are for another room, only the stored key is used, because the password in
+ * the parameters belongs to that other room.
  */
 function keyForRoom(
   roomId: string,
   paramsRoomId: string | null,
   password: string | null,
 ): string | null {
-  if (roomId !== paramsRoomId)
+  const storedKey = localStorage.getItem(
+    getRoomSharedKeyLocalStorageKey(roomId),
+  );
+  if (roomId !== paramsRoomId) {
     logger.warn(
       "requested key for a roomId which is not the current call room id (from the URL)",
       roomId,
       paramsRoomId,
     );
-  return (
-    password ?? localStorage.getItem(getRoomSharedKeyLocalStorageKey(roomId))
-  );
+    return storedKey;
+  }
+  return password ?? storedKey;
 }
 
 export function getKeyForRoom(roomId: string): string | null {
@@ -89,11 +94,7 @@ export function useRoomEncryptionSystem(roomId: string): EncryptionSystem {
   const { roomId: paramsRoomId, password } = useUrlParams();
 
   const [storedPassword] = useRoomSharedKey(
-    // TODO: this passes an already-prefixed key where a room ID is expected, so
-    // the local storage key ends up prefixed twice and never matches what
-    // saveKeyForRoom writes. Preserved as-is here to keep this commit a pure
-    // refactor; the reactive read is effectively dead until it is fixed.
-    getRoomSharedKeyLocalStorageKey(roomId),
+    roomId,
     keyForRoom(roomId, paramsRoomId, password) ?? undefined,
   );
 

src/state/media/MediaViewModel.test.ts

@@ -235,22 +235,35 @@ test("audio and video follow the tracks' mute state", () => {
 
 test("user media polls stream stats only while the setting is on", () => {
   onTestFinished(() => showConnectionStats.setValue(false));
+
   withTestScheduler(({ cold, expectObservable, schedule }) => {
     const stats = { type: "inbound-rtp" } as RTCInboundRtpStreamStats;
     const vm = mockRemoteMedia(rtcMembership, {}, [
-      mockAudioTrack({
-        stats$: () => cold("-s", { s: stats }),
-      }),
+      mockAudioTrack({ stats$: () => cold("-s", { s: { ...stats } }) }),
     ]);
-    schedule("-a-b", {
-      a() {
-        showConnectionStats.setValue(true);
-      },
-      b() {
-        showConnectionStats.setValue(false);
-      },
+    schedule("-t-f-t", {
+      t: () => showConnectionStats.setValue(true),
+      f: () => showConnectionStats.setValue(false),
     });
-    expectObservable(vm.audioStreamStats$).toBe("u-su", {
+
+    expectObservable(vm.audioStreamStats$).toBe("u-s---s", {
+      u: undefined,
+      s: stats,
+    });
+  });
+});
+
+test("stream stats can be enabled before creating media", () => {
+  showConnectionStats.setValue(true);
+  onTestFinished(() => showConnectionStats.setValue(false));
+
+  withTestScheduler(({ cold, expectObservable }) => {
+    const stats = { type: "inbound-rtp" } as RTCInboundRtpStreamStats;
+    const vm = mockRemoteMedia(rtcMembership, {}, [
+      mockAudioTrack({ stats$: () => cold("-s", { s: stats }) }),
+    ]);
+
+    expectObservable(vm.audioStreamStats$).toBe("us", {
       u: undefined,
       s: stats,
     });

src/state/media/UserMediaViewModel.ts

@@ -16,7 +16,9 @@ import {
   BehaviorSubject,
   combineLatest,
   map,
+  NEVER,
   of,
+  startWith,
   Subject,
   switchMap,
 } from "rxjs";
@@ -105,10 +107,9 @@ export function createBaseUserMedia(
         showConnectionStats.value$,
       ]).pipe(
         switchMap(([track, show]) =>
-          track !== undefined && show
-            ? track.stats$(statsIntervalMs)
-            : of(undefined),
+          track !== undefined && show ? track.stats$(statsIntervalMs) : NEVER,
         ),
+        startWith<MediaStreamStats>(undefined),
       ),
     );