From e7e1c0b4bacf142c7c60dae085d784322f94f35f Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Fri, 26 Aug 2022 21:15:28 +0100 Subject: [PATCH 1/5] Add more thorough tests for device list tracking Test what happens when local and remote users join and leave rooms. The previous test only covered local users already in the same room. Signed-off-by: Sean Quah --- tests/csapi/device_lists_test.go | 350 ++++++++++++++++++++++++++----- 1 file changed, 302 insertions(+), 48 deletions(-) diff --git a/tests/csapi/device_lists_test.go b/tests/csapi/device_lists_test.go index 91fdb546..37b0228e 100644 --- a/tests/csapi/device_lists_test.go +++ b/tests/csapi/device_lists_test.go @@ -6,61 +6,315 @@ import ( "github.com/matrix-org/complement/internal/b" "github.com/matrix-org/complement/internal/client" + "github.com/matrix-org/complement/internal/docker" + "github.com/matrix-org/complement/internal/match" + "github.com/matrix-org/complement/internal/must" + "github.com/matrix-org/complement/runtime" + "maunium.net/go/mautrix/crypto/olm" "github.com/tidwall/gjson" ) -// TestLocalUsersReceiveDeviceListUpdates tests that users on the same -// homeserver receive device list updates from other local users, as -// long as they share a room. -func TestLocalUsersReceiveDeviceListUpdates(t *testing.T) { - // Create a homeserver with two users that share a room - deployment := Deploy(t, b.BlueprintOneToOneRoom) - defer deployment.Destroy(t) +// TestDeviceListUpdates tests various flows and checks that: +// 1. `/sync`'s `device_lists.changed/left` contain the correct user IDs. +// 2. `/keys/query` returns the correct information after device list updates. +func TestDeviceListUpdates(t *testing.T) { + localpartIndex := 0 + // generateLocalpart generates a unique localpart based on the given name. + generateLocalpart := func(localpart string) string { + localpartIndex++ + return fmt.Sprintf("%s%d", localpart, localpartIndex) + } - // Get a reference to the already logged-in CS API clients for each user - alice := deployment.Client(t, "hs1", "@alice:hs1") - bob := deployment.Client(t, "hs1", "@bob:hs1") - - // Deduce alice's device ID - resp := alice.MustDoFunc( - t, - "GET", - []string{"_matrix", "client", "v3", "account", "whoami"}, - ) - responseBodyBytes := client.ParseJSON(t, resp) - aliceDeviceID := gjson.GetBytes(responseBodyBytes, "device_id").Str - - // Bob performs an initial sync - _, bobNextBatch := bob.MustSync(t, client.SyncReq{}) - - // Alice then updates their device list by renaming their current device - alice.MustDoFunc( - t, - "PUT", - []string{"_matrix", "client", "v3", "devices", aliceDeviceID}, - client.WithJSONBody( - t, - map[string]interface{}{ - "display_name": "A New Device Name", - }, - ), - ) - - // Check that Bob received a device list update from Alice - bob.MustSyncUntil( - t, - client.SyncReq{ - Since: bobNextBatch, - }, func(clientUserID string, topLevelSyncJSON gjson.Result) error { - // Ensure that Bob sees that Alice has updated their device list - usersWithChangedDeviceListsArray := topLevelSyncJSON.Get("device_lists.changed").Array() + // uploadNewKeys uploads a new set of keys for a given client. + // Returns a check function that can be passed to mustQueryKeys. + uploadNewKeys := func(t *testing.T, user *client.CSAPI) []match.JSON { + account := olm.NewAccount() + ed25519Key, curve25519Key := account.IdentityKeys() + + ed25519KeyID := fmt.Sprintf("ed25519:%s", user.DeviceID) + curve25519KeyID := fmt.Sprintf("curve25519:%s", user.DeviceID) + + user.MustDoFunc(t, "POST", []string{"_matrix", "client", "v3", "keys", "upload"}, + client.WithJSONBody(t, map[string]interface{}{ + "device_keys": map[string]interface{}{ + "user_id": user.UserID, + "device_id": user.DeviceID, + "algorithms": []interface{}{"m.olm.v1.curve25519-aes-sha2", "m.megolm.v1.aes-sha2"}, + "keys": map[string]interface{}{ + ed25519KeyID: ed25519Key.String(), + curve25519KeyID: curve25519Key.String(), + }, + }, + }), + ) + + algorithmsPath := fmt.Sprintf("device_keys.%s.%s.algorithms", user.UserID, user.DeviceID) + ed25519Path := fmt.Sprintf("device_keys.%s.%s.keys.%s", user.UserID, user.DeviceID, ed25519KeyID) + curve25519Path := fmt.Sprintf("device_keys.%s.%s.keys.%s", user.UserID, user.DeviceID, curve25519KeyID) + return []match.JSON{ + match.JSONKeyEqual(algorithmsPath, []interface{}{"m.olm.v1.curve25519-aes-sha2", "m.megolm.v1.aes-sha2"}), + match.JSONKeyEqual(ed25519Path, ed25519Key.String()), + match.JSONKeyEqual(curve25519Path, curve25519Key.String()), + } + } + + // mustQueryKeys checks that /keys/query returns the correct device keys. + // Accepts a check function produced by a prior call to uploadNewKeys. + mustQueryKeys := func(t *testing.T, user *client.CSAPI, userID string, check []match.JSON) { + t.Helper() + + res := user.DoFunc(t, "POST", []string{"_matrix", "client", "v3", "keys", "query"}, + client.WithJSONBody(t, map[string]interface{}{ + "device_keys": map[string]interface{}{ + userID: []string{}, + }, + }), + ) + must.MatchResponse(t, res, match.HTTPResponse{ + StatusCode: 200, + JSON: check, + }) + } + + // syncDeviceListsHas checks that `device_lists.changed` or `device_lists.left` contains a given + // user ID. + syncDeviceListsHas := func(section string, expectedUserID string) client.SyncCheckOpt { + jsonPath := fmt.Sprintf("device_lists.%s", section) + return func(clientUserID string, topLevelSyncJSON gjson.Result) error { + usersWithChangedDeviceListsArray := topLevelSyncJSON.Get(jsonPath).Array() for _, userID := range usersWithChangedDeviceListsArray { - if userID.Str == alice.UserID { + if userID.Str == expectedUserID { return nil } } - return fmt.Errorf("missing device list update for Alice") - }, - ) + return fmt.Errorf( + "syncDeviceListsHas: %s not found in %s", + expectedUserID, + jsonPath, + ) + } + } + + // In all of these test scenarios, there are two users: Alice and Bob. + // We only care about what Alice sees. + + // testOtherUserJoin tests another user joining a room Alice is already in. + testOtherUserJoin := func(t *testing.T, deployment *docker.Deployment, hsName string, otherHSName string) { + alice := deployment.RegisterUser(t, hsName, generateLocalpart("alice"), "password", false) + bob := deployment.RegisterUser(t, otherHSName, generateLocalpart("bob"), "password", false) + checkBobKeys := uploadNewKeys(t, bob) + + roomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + + // Alice performs an initial sync + _, aliceNextBatch := alice.MustSync(t, client.SyncReq{}) + + // Bob joins the room + bob.JoinRoom(t, roomID, []string{hsName}) + bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID)) + + // Check that Alice receives a device list update from Bob + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("changed", bob.UserID), + ) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + // Some homeservers (Synapse) may emit another `changed` update after querying keys. + _, aliceNextBatch = alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) + + // Both homeservers think Bob has joined now + // Bob then updates their device list + checkBobKeys = uploadNewKeys(t, bob) + + // Check that Alice receives a device list update from Bob + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("changed", bob.UserID), + ) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + } + + // testJoin tests Alice joining a room another user is already in. + testJoin := func( + t *testing.T, deployment *docker.Deployment, hsName string, otherHSName string, + ) { + alice := deployment.RegisterUser(t, hsName, generateLocalpart("alice"), "password", false) + bob := deployment.RegisterUser(t, otherHSName, generateLocalpart("bob"), "password", false) + checkBobKeys := uploadNewKeys(t, bob) + + roomID := bob.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + + // Alice performs an initial sync + _, aliceNextBatch := alice.MustSync(t, client.SyncReq{}) + + // Alice joins the room + alice.JoinRoom(t, roomID, []string{otherHSName}) + bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID)) + + // Check that Alice receives a device list update from Bob + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("changed", bob.UserID), + ) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + // Some homeservers (Synapse) may emit another `changed` update after querying keys. + _, aliceNextBatch = alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) + + // Both homeservers think Alice has joined now + // Bob then updates their device list + checkBobKeys = uploadNewKeys(t, bob) + + // Check that Alice receives a device list update from Bob + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("changed", bob.UserID), + ) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + } + + // testOtherUserLeave tests another user leaving a room Alice is in. + testOtherUserLeave := func(t *testing.T, deployment *docker.Deployment, hsName string, otherHSName string) { + alice := deployment.RegisterUser(t, hsName, generateLocalpart("alice"), "password", false) + bob := deployment.RegisterUser(t, otherHSName, generateLocalpart("bob"), "password", false) + checkBobKeys := uploadNewKeys(t, bob) + + roomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + + // Bob joins the room + bob.JoinRoom(t, roomID, []string{hsName}) + bobNextBatch := bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID)) + + // Alice performs an initial sync + alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID)) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + // Some homeservers (Synapse) may emit another `changed` update after querying keys. + _, aliceNextBatch := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) + + // Bob leaves the room + bob.LeaveRoom(t, roomID) + bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(bob.UserID, roomID)) + + // Check that Alice is notified + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("left", bob.UserID), + ) + + // Both homeservers think Bob has left now + // Bob then updates their device list + // Alice's homeserver is not expected to get the device list update and must not return a + // cached device list for Bob. + checkBobKeys = uploadNewKeys(t, bob) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + } + + // testLeave tests Alice leaving a room another user is in. + testLeave := func(t *testing.T, deployment *docker.Deployment, hsName string, otherHSName string) { + alice := deployment.RegisterUser(t, hsName, generateLocalpart("alice"), "password", false) + bob := deployment.RegisterUser(t, otherHSName, generateLocalpart("bob"), "password", false) + checkBobKeys := uploadNewKeys(t, bob) + + roomID := bob.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + + // Alice joins the room + alice.JoinRoom(t, roomID, []string{otherHSName}) + bobNextBatch := bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID)) + + // Alice performs an initial sync + alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID)) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + // Some homeservers (Synapse) may emit another `changed` update after querying keys. + _, aliceNextBatch := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) + + // Alice leaves the room + alice.LeaveRoom(t, roomID) + bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(alice.UserID, roomID)) + + // Check that Alice is notified + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("left", bob.UserID), + ) + + // Both homeservers think Alice has left now + // Bob then updates their device list + // Alice's homeserver is not expected to get the device list update and must not return a + // cached device list for Bob. + checkBobKeys = uploadNewKeys(t, bob) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + } + + // testOtherUserRejoin tests another user leaving and rejoining a room Alice is in. + testOtherUserRejoin := func(t *testing.T, deployment *docker.Deployment, hsName string, otherHSName string) { + alice := deployment.RegisterUser(t, hsName, generateLocalpart("alice"), "password", false) + bob := deployment.RegisterUser(t, otherHSName, generateLocalpart("bob"), "password", false) + checkBobKeys := uploadNewKeys(t, bob) + + roomID := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + + // Bob joins the room + bob.JoinRoom(t, roomID, []string{hsName}) + bobNextBatch := bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID)) + + // Alice performs an initial sync + alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID)) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + // Some homeservers (Synapse) may emit another `changed` update after querying keys. + _, aliceNextBatch := alice.MustSync(t, client.SyncReq{TimeoutMillis: "0"}) + + // Both homeservers think Bob has joined now + // Bob leaves the room + bob.LeaveRoom(t, roomID) + bobNextBatch = bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(bob.UserID, roomID)) + + // Check that Alice is notified + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("left", bob.UserID), + ) + + // Both homeservers think Bob has left now + // Bob then updates their device list before rejoining the room + // Alice's homeserver is not expected to get the device list update. + checkBobKeys = uploadNewKeys(t, bob) + + // Bob rejoins the room + bob.JoinRoom(t, roomID, []string{hsName}) + bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncJoinedTo(bob.UserID, roomID)) + + // Check that Alice is notified + // Alice's homeserver must not return a cached device list for Bob. + alice.MustSyncUntil( + t, + client.SyncReq{Since: aliceNextBatch}, + syncDeviceListsHas("changed", bob.UserID), + ) + mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + } + + // Create two homeservers + deployment := Deploy(t, b.BlueprintFederationOneToOneRoom) + defer deployment.Destroy(t) + + t.Run("when local user joins a room", func(t *testing.T) { testOtherUserJoin(t, deployment, "hs1", "hs1") }) + t.Run("when remote user joins a room", func(t *testing.T) { testOtherUserJoin(t, deployment, "hs1", "hs2") }) + t.Run("when joining a room with a local user", func(t *testing.T) { testJoin(t, deployment, "hs1", "hs1") }) + t.Run("when joining a room with a remote user", func(t *testing.T) { testJoin(t, deployment, "hs1", "hs2") }) + t.Run("when local user leaves a room", func(t *testing.T) { testOtherUserLeave(t, deployment, "hs1", "hs1") }) + t.Run("when remote user leaves a room", func(t *testing.T) { testOtherUserLeave(t, deployment, "hs1", "hs2") }) + t.Run("when leaving a room with a local user", func(t *testing.T) { testLeave(t, deployment, "hs1", "hs1") }) + t.Run("when leaving a room with a remote user", func(t *testing.T) { + runtime.SkipIf(t, runtime.Synapse) // https://github.com/matrix-org/synapse/issues/13650 + testLeave(t, deployment, "hs1", "hs2") + }) + t.Run("when local user rejoins a room", func(t *testing.T) { testOtherUserRejoin(t, deployment, "hs1", "hs1") }) + t.Run("when remote user rejoins a room", func(t *testing.T) { testOtherUserRejoin(t, deployment, "hs1", "hs2") }) } From b55f8165a86461ddb01a665e297d792321f3d640 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 12 Sep 2022 18:45:13 +0100 Subject: [PATCH 2/5] fixup: add missing t.Helper() --- tests/csapi/device_lists_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/csapi/device_lists_test.go b/tests/csapi/device_lists_test.go index 37b0228e..5aa61805 100644 --- a/tests/csapi/device_lists_test.go +++ b/tests/csapi/device_lists_test.go @@ -29,6 +29,8 @@ func TestDeviceListUpdates(t *testing.T) { // uploadNewKeys uploads a new set of keys for a given client. // Returns a check function that can be passed to mustQueryKeys. uploadNewKeys := func(t *testing.T, user *client.CSAPI) []match.JSON { + t.Helper() + account := olm.NewAccount() ed25519Key, curve25519Key := account.IdentityKeys() From ece3547d15831c95a257f4b7e401e2fc34407aad Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 12 Sep 2022 18:46:22 +0100 Subject: [PATCH 3/5] fixup: expand on "Check that Alice is notified" comments --- tests/csapi/device_lists_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/csapi/device_lists_test.go b/tests/csapi/device_lists_test.go index 5aa61805..48825073 100644 --- a/tests/csapi/device_lists_test.go +++ b/tests/csapi/device_lists_test.go @@ -201,7 +201,7 @@ func TestDeviceListUpdates(t *testing.T) { bob.LeaveRoom(t, roomID) bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(bob.UserID, roomID)) - // Check that Alice is notified + // Check that Alice is notified that she will no longer receive updates about Bob's devices alice.MustSyncUntil( t, client.SyncReq{Since: aliceNextBatch}, @@ -238,7 +238,7 @@ func TestDeviceListUpdates(t *testing.T) { alice.LeaveRoom(t, roomID) bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(alice.UserID, roomID)) - // Check that Alice is notified + // Check that Alice is notified that she will no longer receive updates about Bob's devices alice.MustSyncUntil( t, client.SyncReq{Since: aliceNextBatch}, @@ -276,7 +276,7 @@ func TestDeviceListUpdates(t *testing.T) { bob.LeaveRoom(t, roomID) bobNextBatch = bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(bob.UserID, roomID)) - // Check that Alice is notified + // Check that Alice is notified that she will no longer receive updates about Bob's devices alice.MustSyncUntil( t, client.SyncReq{Since: aliceNextBatch}, @@ -292,7 +292,7 @@ func TestDeviceListUpdates(t *testing.T) { bob.JoinRoom(t, roomID, []string{hsName}) bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncJoinedTo(bob.UserID, roomID)) - // Check that Alice is notified + // Check that Alice is notified that Bob's devices have a change // Alice's homeserver must not return a cached device list for Bob. alice.MustSyncUntil( t, From de5410687d6309d43896d18f1a3f2edb7b21a08b Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 12 Sep 2022 18:46:52 +0100 Subject: [PATCH 4/5] fixup: add note about blueprint users not being used --- tests/csapi/device_lists_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/csapi/device_lists_test.go b/tests/csapi/device_lists_test.go index 48825073..87577e42 100644 --- a/tests/csapi/device_lists_test.go +++ b/tests/csapi/device_lists_test.go @@ -303,6 +303,8 @@ func TestDeviceListUpdates(t *testing.T) { } // Create two homeservers + // The users and rooms in the blueprint won't be used. + // Each test creates their own Alice and Bob users. deployment := Deploy(t, b.BlueprintFederationOneToOneRoom) defer deployment.Destroy(t) From 03d77ba83920d1ef9e5bdc3208d137bbdd5f8d34 Mon Sep 17 00:00:00 2001 From: Sean Quah Date: Mon, 12 Sep 2022 18:47:11 +0100 Subject: [PATCH 5/5] fixup: check for lack of `device_lists` updates in room leave tests --- tests/csapi/device_lists_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/csapi/device_lists_test.go b/tests/csapi/device_lists_test.go index 87577e42..d8d4eedc 100644 --- a/tests/csapi/device_lists_test.go +++ b/tests/csapi/device_lists_test.go @@ -202,7 +202,7 @@ func TestDeviceListUpdates(t *testing.T) { bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(bob.UserID, roomID)) // Check that Alice is notified that she will no longer receive updates about Bob's devices - alice.MustSyncUntil( + aliceNextBatch = alice.MustSyncUntil( t, client.SyncReq{Since: aliceNextBatch}, syncDeviceListsHas("left", bob.UserID), @@ -214,6 +214,12 @@ func TestDeviceListUpdates(t *testing.T) { // cached device list for Bob. checkBobKeys = uploadNewKeys(t, bob) mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + + // Check that Alice is not notified about Bob's device update + syncResult, _ := alice.MustSync(t, client.SyncReq{Since: aliceNextBatch}) + if syncDeviceListsHas("changed", bob.UserID)(alice.UserID, syncResult) == nil { + t.Fatalf("Alice was unexpectedly notified about Bob's device update even though they share no rooms") + } } // testLeave tests Alice leaving a room another user is in. @@ -239,7 +245,7 @@ func TestDeviceListUpdates(t *testing.T) { bob.MustSyncUntil(t, client.SyncReq{Since: bobNextBatch}, client.SyncLeftFrom(alice.UserID, roomID)) // Check that Alice is notified that she will no longer receive updates about Bob's devices - alice.MustSyncUntil( + aliceNextBatch = alice.MustSyncUntil( t, client.SyncReq{Since: aliceNextBatch}, syncDeviceListsHas("left", bob.UserID), @@ -251,6 +257,12 @@ func TestDeviceListUpdates(t *testing.T) { // cached device list for Bob. checkBobKeys = uploadNewKeys(t, bob) mustQueryKeys(t, alice, bob.UserID, checkBobKeys) + + // Check that Alice is not notified about Bob's device update + syncResult, _ := alice.MustSync(t, client.SyncReq{Since: aliceNextBatch}) + if syncDeviceListsHas("changed", bob.UserID)(alice.UserID, syncResult) == nil { + t.Fatalf("Alice was unexpectedly notified about Bob's device update even though they share no rooms") + } } // testOtherUserRejoin tests another user leaving and rejoining a room Alice is in.