From c2e655c61cb813cace71d6d36e9d7141816097d5 Mon Sep 17 00:00:00 2001 From: Kegan Dougal <7190048+kegsay@users.noreply.github.com> Date: Fri, 26 Jan 2024 14:36:31 +0000 Subject: [PATCH] Refactor how client creation/login is done --- go.mod | 2 +- go.sum | 2 ++ internal/api/client.go | 5 +-- internal/api/js/js.go | 6 ++++ internal/api/rust/rust.go | 6 ++++ tests/federation_connectivity_test.go | 12 +++---- tests/key_backup_test.go | 28 ++++++---------- tests/main_test.go | 47 ++++++++++++++++++++------- tests/membership_acls_test.go | 6 ++-- tests/one_time_keys_test.go | 10 +++--- tests/room_keys_test.go | 3 +- tests/state_synchronisation_test.go | 16 ++++----- 12 files changed, 87 insertions(+), 56 deletions(-) diff --git a/go.mod b/go.mod index c8e715b..01ba008 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/chromedp/cdproto v0.0.0-20231025043423-5615e204d422 github.com/chromedp/chromedp v0.9.3 github.com/docker/go-connections v0.4.0 - github.com/matrix-org/complement v0.0.0-20240117130049-f24331b29b64 + github.com/matrix-org/complement v0.0.0-20240126134841-458bfba5f7f3 github.com/testcontainers/testcontainers-go v0.26.0 github.com/tidwall/gjson v1.16.0 golang.org/x/exp v0.0.0-20230905200255-921286631fa9 diff --git a/go.sum b/go.sum index bf073c0..fdcd1af 100644 --- a/go.sum +++ b/go.sum @@ -89,6 +89,8 @@ github.com/matrix-org/complement v0.0.0-20240117124104-5ecb086412c1 h1:YryEz+vrd github.com/matrix-org/complement v0.0.0-20240117124104-5ecb086412c1/go.mod h1:GMCwbgMOudedB86u1c5+nfQS1L31sFHZ9/YzTYqWyjU= github.com/matrix-org/complement v0.0.0-20240117130049-f24331b29b64 h1:dBvjM8idvlfkgyCeimfSxZTXdE85wdfJfGIEPq4GAu8= github.com/matrix-org/complement v0.0.0-20240117130049-f24331b29b64/go.mod h1:GMCwbgMOudedB86u1c5+nfQS1L31sFHZ9/YzTYqWyjU= +github.com/matrix-org/complement v0.0.0-20240126134841-458bfba5f7f3 h1:M8k58YcnDn/0wSVLYyuei21VnJOd9CitzUuMo0xxIVY= +github.com/matrix-org/complement v0.0.0-20240126134841-458bfba5f7f3/go.mod h1:GMCwbgMOudedB86u1c5+nfQS1L31sFHZ9/YzTYqWyjU= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530 h1:kHKxCOLcHH8r4Fzarl4+Y3K5hjothkVW5z7T1dUM11U= github.com/matrix-org/gomatrix v0.0.0-20220926102614-ceba4d9f7530/go.mod h1:/gBX06Kw0exX1HrwmoBibFA98yBk/jxKpGVeyQbff+s= github.com/matrix-org/gomatrixserverlib v0.0.0-20230921171121-0466775328c7 h1:NhPNNFLHwdDb/upeicBh1GkxX/sFinEp5HF1WBqPtiY= diff --git a/internal/api/client.go b/internal/api/client.go index 0e96a98..72e4959 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -67,6 +67,7 @@ type Client interface { // The user for this client UserID() string Type() ClientTypeLang + Opts() ClientCreationOpts } type LoggedClient struct { @@ -184,11 +185,11 @@ type ClientCreationOpts struct { DeviceID string } -func FromComplementClient(c *client.CSAPI, password string) ClientCreationOpts { +func NewClientCreationOpts(c *client.CSAPI) ClientCreationOpts { return ClientCreationOpts{ BaseURL: c.BaseURL, UserID: c.UserID, - Password: password, + Password: c.Password, DeviceID: c.DeviceID, } } diff --git a/internal/api/js/js.go b/internal/api/js/js.go index 31ee403..c41f8e9 100644 --- a/internal/api/js/js.go +++ b/internal/api/js/js.go @@ -58,6 +58,7 @@ type JSClient struct { listenerID atomic.Int32 listenersMu *sync.RWMutex userID string + opts api.ClientCreationOpts } func NewJSClient(t ct.TestLike, opts api.ClientCreationOpts) (api.Client, error) { @@ -65,6 +66,7 @@ func NewJSClient(t ct.TestLike, opts api.ClientCreationOpts) (api.Client, error) listeners: make(map[int32]func(roomID string, ev api.Event)), userID: opts.UserID, listenersMu: &sync.RWMutex{}, + opts: opts, } portKey := opts.UserID + opts.DeviceID browser, err := chrome.RunHeadless(func(s string) { @@ -247,6 +249,10 @@ func (c *JSClient) UserID() string { return c.userID } +func (c *JSClient) Opts() api.ClientCreationOpts { + return c.opts +} + func (c *JSClient) MustGetEvent(t ct.TestLike, roomID, eventID string) api.Event { t.Helper() // serialised output (if encrypted): diff --git a/internal/api/rust/rust.go b/internal/api/rust/rust.go index 5772733..c0b4eeb 100644 --- a/internal/api/rust/rust.go +++ b/internal/api/rust/rust.go @@ -43,6 +43,7 @@ type RustClient struct { roomsMu *sync.RWMutex userID string persistentStoragePath string + opts api.ClientCreationOpts } func NewRustClient(t ct.TestLike, opts api.ClientCreationOpts, ssURL string) (api.Client, error) { @@ -65,6 +66,7 @@ func NewRustClient(t ct.TestLike, opts api.ClientCreationOpts, ssURL string) (ap rooms: make(map[string]*RustRoomInfo), listeners: make(map[int32]func(roomID string)), roomsMu: &sync.RWMutex{}, + opts: opts, } if opts.PersistentStorage { c.persistentStoragePath = "./rust_storage/" + username @@ -73,6 +75,10 @@ func NewRustClient(t ct.TestLike, opts api.ClientCreationOpts, ssURL string) (ap return &api.LoggedClient{Client: c}, nil } +func (c *RustClient) Opts() api.ClientCreationOpts { + return c.opts +} + func (c *RustClient) Login(t ct.TestLike, opts api.ClientCreationOpts) error { var deviceID *string if opts.DeviceID != "" { diff --git a/tests/federation_connectivity_test.go b/tests/federation_connectivity_test.go index e285977..e913c89 100644 --- a/tests/federation_connectivity_test.go +++ b/tests/federation_connectivity_test.go @@ -49,11 +49,11 @@ func TestNewUserCannotGetKeysForOfflineServer(t *testing.T) { ss := deployment.SlidingSyncURL(t) alice := MustCreateClient(t, api.ClientType{HS: "hs1", Lang: api.ClientTypeRust}, - api.FromComplementClient(csapiAlice, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiAlice), ss, WithDoLogin(t)) defer alice.Close(t) bob := MustCreateClient(t, api.ClientType{HS: "hs2", Lang: api.ClientTypeJS}, - api.FromComplementClient(csapiBob, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiBob), ss, WithDoLogin(t)) defer bob.Close(t) aliceStopSyncing := alice.MustStartSyncing(t) defer aliceStopSyncing() @@ -77,7 +77,7 @@ func TestNewUserCannotGetKeysForOfflineServer(t *testing.T) { csapiAlice.MustInviteRoom(t, roomID, csapiCharlie.UserID) charlie := MustCreateClient(t, api.ClientType{HS: "hs1", Lang: api.ClientTypeRust}, - api.FromComplementClient(csapiCharlie, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiCharlie), ss, WithDoLogin(t)) defer charlie.Close(t) charlieStopSyncing := charlie.MustStartSyncing(t) defer charlieStopSyncing() @@ -171,15 +171,15 @@ func TestExistingSessionCannotGetKeysForOfflineServer(t *testing.T) { ss := deployment.SlidingSyncURL(t) alice := MustCreateClient(t, api.ClientType{HS: "hs1", Lang: api.ClientTypeRust}, - api.FromComplementClient(csapiAlice, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiAlice), ss, WithDoLogin(t)) defer alice.Close(t) bob := MustCreateClient(t, api.ClientType{HS: "hs2", Lang: api.ClientTypeJS}, - api.FromComplementClient(csapiBob, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiBob), ss, WithDoLogin(t)) defer bob.Close(t) charlie := MustCreateClient(t, api.ClientType{HS: "hs1", Lang: api.ClientTypeRust}, - api.FromComplementClient(csapiCharlie, "complement-crypto-password"), ss, WithDoLogin(t)) + api.NewClientCreationOpts(csapiCharlie), ss, WithDoLogin(t)) defer charlie.Close(t) aliceStopSyncing := alice.MustStartSyncing(t) defer aliceStopSyncing() diff --git a/tests/key_backup_test.go b/tests/key_backup_test.go index 218d7be..22142ee 100644 --- a/tests/key_backup_test.go +++ b/tests/key_backup_test.go @@ -18,12 +18,8 @@ func TestCanBackupKeys(t *testing.T) { return } t.Logf("backup creator = %s backup restorer = %s", clientTypeA.Lang, clientTypeB.Lang) - deployment := Deploy(t) - csapiAlice := deployment.Register(t, clientTypeA.HS, helpers.RegistrationOpts{ - LocalpartSuffix: "alice", - Password: "complement-crypto-password", - }) - roomID := csapiAlice.MustCreateRoom(t, map[string]interface{}{ + tc := CreateTestContext(t, clientTypeA) + roomID := tc.Alice.MustCreateRoom(t, map[string]interface{}{ "name": t.Name(), "preset": "public_chat", // shared history visibility "invite": []string{}, @@ -41,7 +37,7 @@ func TestCanBackupKeys(t *testing.T) { // SDK testing below // ----------------- - backupCreator := LoginClientFromComplementClient(t, deployment, csapiAlice, clientTypeA) + backupCreator := tc.MustLoginClient(t, tc.Alice, clientTypeA) defer backupCreator.Close(t) stopSyncing := backupCreator.MustStartSyncing(t) defer stopSyncing() @@ -57,11 +53,11 @@ func TestCanBackupKeys(t *testing.T) { t.Logf("recovery key -> %s", recoveryKey) // Now login on a new device - csapiAlice2 := deployment.Login(t, clientTypeB.HS, csapiAlice, helpers.LoginOpts{ + csapiAlice2 := tc.Deployment.Login(t, clientTypeB.HS, tc.Alice, helpers.LoginOpts{ DeviceID: "BACKUP_RESTORER", Password: "complement-crypto-password", }) - backupRestorer := LoginClientFromComplementClient(t, deployment, csapiAlice2, clientTypeB) + backupRestorer := tc.MustLoginClient(t, csapiAlice2, clientTypeB) defer backupRestorer.Close(t) // load the key backup using the recovery key @@ -86,12 +82,8 @@ func TestBackupWrongRecoveryKeyFails(t *testing.T) { return } t.Logf("backup creator = %s backup restorer = %s", clientTypeA.Lang, clientTypeB.Lang) - deployment := Deploy(t) - csapiAlice := deployment.Register(t, clientTypeA.HS, helpers.RegistrationOpts{ - LocalpartSuffix: "alice", - Password: "complement-crypto-password", - }) - roomID := csapiAlice.MustCreateRoom(t, map[string]interface{}{ + tc := CreateTestContext(t, clientTypeA) + roomID := tc.Alice.MustCreateRoom(t, map[string]interface{}{ "name": t.Name(), "preset": "public_chat", // shared history visibility "invite": []string{}, @@ -109,7 +101,7 @@ func TestBackupWrongRecoveryKeyFails(t *testing.T) { // SDK testing below // ----------------- - backupCreator := LoginClientFromComplementClient(t, deployment, csapiAlice, clientTypeA) + backupCreator := tc.MustLoginClient(t, tc.Alice, clientTypeA) defer backupCreator.Close(t) stopSyncing := backupCreator.MustStartSyncing(t) defer stopSyncing() @@ -125,11 +117,11 @@ func TestBackupWrongRecoveryKeyFails(t *testing.T) { t.Logf("recovery key -> %s", recoveryKey) // Now login on a new device - csapiAlice2 := deployment.Login(t, clientTypeB.HS, csapiAlice, helpers.LoginOpts{ + csapiAlice2 := tc.Deployment.Login(t, clientTypeB.HS, tc.Alice, helpers.LoginOpts{ DeviceID: "BACKUP_RESTORER", Password: "complement-crypto-password", }) - backupRestorer := LoginClientFromComplementClient(t, deployment, csapiAlice2, clientTypeB) + backupRestorer := tc.MustLoginClient(t, csapiAlice2, clientTypeB) defer backupRestorer.Close(t) // load the key backup using a valid but wrong recovery key diff --git a/tests/main_test.go b/tests/main_test.go index 4d0d6c7..774a75f 100644 --- a/tests/main_test.go +++ b/tests/main_test.go @@ -141,23 +141,46 @@ func (c *TestContext) CreateNewEncryptedRoom(t *testing.T, creator *client.CSAPI }) } -func (c *TestContext) MustLoginDevice(t *testing.T, existing *client.CSAPI, clientType api.ClientType, deviceID string) (*client.CSAPI, api.Client) { - newClient := c.Deployment.Login(t, clientType.HS, existing, helpers.LoginOpts{ - DeviceID: deviceID, - Password: "complement-crypto-password", +func (c *TestContext) OptsFromClient(t *testing.T, existing *client.CSAPI, options ...func(*api.ClientCreationOpts)) api.ClientCreationOpts { + o := &api.ClientCreationOpts{ + BaseURL: existing.BaseURL, + UserID: existing.UserID, + DeviceID: existing.DeviceID, + Password: existing.Password, + } + for _, opt := range options { + opt(o) + } + return *o +} + +func WithPersistentStorage() func(*api.ClientCreationOpts) { + return func(o *api.ClientCreationOpts) { + o.PersistentStorage = true + } +} + +func (c *TestContext) MustRegisterNewDevice(t *testing.T, cli *client.CSAPI, hsName, newDeviceID string) *client.CSAPI { + return c.Deployment.Login(t, hsName, cli, helpers.LoginOpts{ + DeviceID: newDeviceID, + Password: cli.Password, }) - return newClient, c.MustLoginClient(t, newClient, clientType) } -func (c *TestContext) MustLoginClient(t *testing.T, cli *client.CSAPI, clientType api.ClientType) api.Client { - return LoginClientFromComplementClient(t, c.Deployment, cli, clientType) +func (c *TestContext) MustCreateClient(t *testing.T, cli *client.CSAPI, clientType api.ClientType, options ...func(*api.ClientCreationOpts)) api.Client { + t.Helper() + cfg := api.NewClientCreationOpts(cli) + cfg.BaseURL = c.Deployment.ReverseProxyURLForHS(clientType.HS) + for _, opt := range options { + opt(&cfg) + } + client := MustCreateClient(t, clientType, cfg, c.Deployment.SlidingSyncURL(t)) + return client } -func LoginClientFromComplementClient(t *testing.T, dep *deploy.SlidingSyncDeployment, cli *client.CSAPI, clientType api.ClientType) api.Client { +func (c *TestContext) MustLoginClient(t *testing.T, cli *client.CSAPI, clientType api.ClientType, options ...func(*api.ClientCreationOpts)) api.Client { t.Helper() - cfg := api.FromComplementClient(cli, "complement-crypto-password") - cfg.BaseURL = dep.ReverseProxyURLForHS(clientType.HS) - client := MustCreateClient(t, clientType, cfg, dep.SlidingSyncURL(t)) - must.NotError(t, "failed to login client", client.Login(t, cfg)) + client := c.MustCreateClient(t, cli, clientType, options...) + must.NotError(t, "failed to login client", client.Login(t, client.Opts())) return client } diff --git a/tests/membership_acls_test.go b/tests/membership_acls_test.go index be7ae05..552b0b2 100644 --- a/tests/membership_acls_test.go +++ b/tests/membership_acls_test.go @@ -277,7 +277,8 @@ func TestOnNewDeviceBobCanSeeButNotDecryptHistoryInPublicRoom(t *testing.T) { waiter.Wait(t, 5*time.Second) // now bob logs in on a new device. He should NOT be able to decrypt this event (though can see it due to history visibility) - csapiBob2, bob2 := tc.MustLoginDevice(t, tc.Bob, clientTypeB, "NEW_DEVICE") + csapiBob2 := tc.MustRegisterNewDevice(t, tc.Bob, clientTypeB.HS, "NEW_DEVICE") + bob2 := tc.MustLoginClient(t, csapiBob2, clientTypeB) bob2StopSyncing := bob2.MustStartSyncing(t) bob2StoppedSyncing := false defer func() { @@ -353,7 +354,8 @@ func TestChangingDeviceAfterInviteReEncrypts(t *testing.T) { evID := alice.SendMessage(t, roomID, body) // now Bob logs in on a different device and accepts the invite. The different device should be able to decrypt the message. - _, bob2 := tc.MustLoginDevice(t, tc.Bob, clientTypeB, "NEW_DEVICE") + csapiBob2 := tc.MustRegisterNewDevice(t, tc.Bob, clientTypeB.HS, "NEW_DEVICE") + bob2 := tc.MustLoginClient(t, csapiBob2, clientTypeB) bob2StopSyncing := bob2.MustStartSyncing(t) defer bob2StopSyncing() diff --git a/tests/one_time_keys_test.go b/tests/one_time_keys_test.go index 88a5e1c..278ca8d 100644 --- a/tests/one_time_keys_test.go +++ b/tests/one_time_keys_test.go @@ -95,7 +95,7 @@ func TestFallbackKeyIsUsedIfOneTimeKeysRunOut(t *testing.T) { // ================= // Upload OTKs and a fallback - alice := LoginClientFromComplementClient(t, tc.Deployment, tc.Alice, clientTypeA) + alice := tc.MustLoginClient(t, tc.Alice, clientTypeA) defer alice.Close(t) aliceStopSyncing := alice.MustStartSyncing(t) defer aliceStopSyncing() @@ -106,7 +106,7 @@ func TestFallbackKeyIsUsedIfOneTimeKeysRunOut(t *testing.T) { tc.Alice.MustCreateRoom(t, map[string]interface{}{}) // also let bob upload OTKs before we block the upload endpoint! - bob := LoginClientFromComplementClient(t, tc.Deployment, tc.Bob, clientTypeB) + bob := tc.MustLoginClient(t, tc.Bob, clientTypeB) defer bob.Close(t) bobStopSyncing := bob.MustStartSyncing(t) defer bobStopSyncing() @@ -200,7 +200,7 @@ func TestFailedOneTimeKeyUploadRetries(t *testing.T) { "filter": "~u .*\\/keys\\/upload.* ~m POST", }, }, func() { - alice := LoginClientFromComplementClient(t, tc.Deployment, tc.Alice, clientType) + alice := tc.MustLoginClient(t, tc.Alice, clientType) defer alice.Close(t) aliceStopSyncing := alice.MustStartSyncing(t) defer aliceStopSyncing() @@ -248,11 +248,11 @@ func TestFailedKeysClaimRetries(t *testing.T) { ForEachClientType(t, func(t *testing.T, clientType api.ClientType) { tc := CreateTestContext(t, clientType, clientType) // both clients start syncing to upload OTKs - alice := LoginClientFromComplementClient(t, tc.Deployment, tc.Alice, clientType) + alice := tc.MustLoginClient(t, tc.Alice, clientType) defer alice.Close(t) aliceStopSyncing := alice.MustStartSyncing(t) defer aliceStopSyncing() - bob := LoginClientFromComplementClient(t, tc.Deployment, tc.Bob, clientType) + bob := tc.MustLoginClient(t, tc.Bob, clientType) defer bob.Close(t) bobStopSyncing := bob.MustStartSyncing(t) defer bobStopSyncing() diff --git a/tests/room_keys_test.go b/tests/room_keys_test.go index 4a8680e..061c3a5 100644 --- a/tests/room_keys_test.go +++ b/tests/room_keys_test.go @@ -20,7 +20,8 @@ func TestRoomKeyIsCycledOnDeviceLeaving(t *testing.T) { // Alice, Alice2 and Bob are in a room. alice := tc.MustLoginClient(t, tc.Alice, clientTypeA) defer alice.Close(t) - csapiAlice2, alice2 := tc.MustLoginDevice(t, tc.Alice, clientTypeA, "OTHER_DEVICE") + csapiAlice2 := tc.MustRegisterNewDevice(t, tc.Alice, clientTypeA.HS, "OTHER_DEVICE") + alice2 := tc.MustLoginClient(t, csapiAlice2, clientTypeA) defer alice2.Close(t) bob := tc.MustLoginClient(t, tc.Bob, clientTypeB) defer bob.Close(t) diff --git a/tests/state_synchronisation_test.go b/tests/state_synchronisation_test.go index bbc6134..1bb1a33 100644 --- a/tests/state_synchronisation_test.go +++ b/tests/state_synchronisation_test.go @@ -61,7 +61,7 @@ func testSigkillBeforeKeysUploadResponseRust(t *testing.T, clientType api.Client "filter": "~u .*\\/keys\\/upload.*", }, }, func() { - cfg := api.FromComplementClient(tc.Alice, "complement-crypto-password") + cfg := api.NewClientCreationOpts(tc.Alice) cfg.BaseURL = tc.Deployment.ReverseProxyURLForHS(clientType.HS) cfg.PersistentStorage = true // run some code in a separate process so we can kill it later @@ -140,10 +140,7 @@ func testSigkillBeforeKeysUploadResponseJS(t *testing.T, clientType api.ClientTy "filter": "~u .*\\/keys\\/upload.*", }, }, func() { - cfg := api.FromComplementClient(tc.Alice, "complement-crypto-password") - cfg.BaseURL = tc.Deployment.ReverseProxyURLForHS(clientType.HS) - cfg.PersistentStorage = true - clientWhichWillBeKilled := MustCreateClient(t, clientType, cfg, tc.Deployment.SlidingSyncURL(t)) + clientWhichWillBeKilled := tc.MustCreateClient(t, tc.Alice, clientType, WithPersistentStorage()) // attempt to login, this should cause OTKs to be uploaded waiter := helpers.NewWaiter() terminateClient = func() { @@ -154,7 +151,7 @@ func testSigkillBeforeKeysUploadResponseJS(t *testing.T, clientType api.ClientTy waiter.Finish() } go func() { - must.NotError(t, "failed to login", clientWhichWillBeKilled.Login(t, cfg)) + must.NotError(t, "failed to login", clientWhichWillBeKilled.Login(t, clientWhichWillBeKilled.Opts())) // need to start syncing to make JS do /keys/upload // we don't need to stopSyncing because we'll SIGKILL this. clientWhichWillBeKilled.StartSyncing(t) @@ -163,9 +160,10 @@ func testSigkillBeforeKeysUploadResponseJS(t *testing.T, clientType api.ClientTy waiter.Wait(t, 5*time.Second) // wait for /keys/upload and subsequent SIGKILL t.Logf("terminated browser, making new client") // now make the same client - recreatedClient := MustCreateClient(t, clientType, cfg, tc.Deployment.SlidingSyncURL(t)) - recreatedClient.Login(t, cfg) // login should work - recreatedClient.StartSyncing(t) // ignore errors, we just need to kick it to /keys/upload + recreatedClient := tc.MustCreateClient(t, tc.Alice, clientType, WithPersistentStorage()) + recreatedClient.Login(t, recreatedClient.Opts()) // login should work + recreatedClient.StartSyncing(t) // ignore errors, we just need to kick it to /keys/upload + recreatedClient.DeletePersistentStorage(t) recreatedClient.Close(t) // ensure we see the 2nd keys/upload