From fd3a555c83c7ccedd44f1c9e571c10e8387c977d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 29 Nov 2021 16:38:19 +0000 Subject: [PATCH 01/14] Change SyncUntil* functions to return next_batch --- internal/client/client.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/client/client.go b/internal/client/client.go index 04556088..9c8cdcb8 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -133,22 +133,28 @@ func (c *CSAPI) SendEventSynced(t *testing.T, roomID string, e b.Event) string { // - we see an event in the room for which the `check` function returns True // If the `check` function fails the test, the failing event will be automatically logged. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntilTimelineHas(t *testing.T, roomID string, check func(gjson.Result) bool) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntilTimelineHas(t *testing.T, roomID string, check func(gjson.Result) bool) string { t.Helper() - c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check) + return c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check) } // SyncUntilInvitedTo is a wrapper around SyncUntil. // It blocks and continually calls `/sync` until we've been invited to the given room. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) string { t.Helper() check := func(event gjson.Result) bool { return event.Get("type").Str == "m.room.member" && event.Get("content.membership").Str == "invite" && event.Get("state_key").Str == c.UserID } - c.SyncUntil(t, "", "", "rooms.invite."+GjsonEscape(roomID)+".invite_state.events", check) + return c.SyncUntil(t, "", "", "rooms.invite."+GjsonEscape(roomID)+".invite_state.events", check) } // SyncUntil blocks and continually calls /sync until @@ -157,7 +163,10 @@ func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) { // - some element in that array makes the `check` function return true. // If the `check` function fails the test, the failing event will be automatically logged. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gjson.Result) bool) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gjson.Result) bool) string { t.Helper() start := time.Now() checkCounter := 0 @@ -199,7 +208,7 @@ func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gj for i, ev := range events { lastEvent = &events[i] if check(ev) { - return + return since } wasFailed = t.Failed() checkCounter++ From 301f2e90bc99207e5f3fa6c00b0acc5d0c2f9db7 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Dec 2021 18:45:45 +0000 Subject: [PATCH 02/14] Add a matcher which checks that a key is missing --- internal/match/json.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/match/json.go b/internal/match/json.go index b61190fa..ff615d14 100644 --- a/internal/match/json.go +++ b/internal/match/json.go @@ -41,6 +41,18 @@ func JSONKeyPresent(wantKey string) JSON { } } +// JSONKeyMissing returns a matcher which will check that `forbiddenKey` is not present in the JSON object. +// `forbiddenKey` can be nested, see https://godoc.org/github.com/tidwall/gjson#Get for details. +func JSONKeyMissing(forbiddenKey string) JSON { + return func(body []byte) error { + res := gjson.GetBytes(body, forbiddenKey) + if res.Exists() { + return fmt.Errorf("key '%s' present", forbiddenKey) + } + return nil + } +} + // JSONKeyTypeEqual returns a matcher which will check that `wantKey` is present and its value is of the type `wantType`. // `wantKey` can be nested, see https://godoc.org/github.com/tidwall/gjson#Get for details. func JSONKeyTypeEqual(wantKey string, wantType gjson.Type) JSON { From 910528e5887c079a29c4277dab12344655e080b2 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Dec 2021 18:50:17 +0000 Subject: [PATCH 03/14] Reproduce matrix-org/synapse#11506 --- tests/csapi/ignored_users_test.go | 92 +++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 tests/csapi/ignored_users_test.go diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go new file mode 100644 index 00000000..8547fcbd --- /dev/null +++ b/tests/csapi/ignored_users_test.go @@ -0,0 +1,92 @@ +package csapi_tests + +import ( + "net/url" + "testing" + + "github.com/matrix-org/complement/internal/b" + "github.com/matrix-org/complement/internal/client" + "github.com/matrix-org/complement/internal/match" + "github.com/matrix-org/complement/internal/must" + "github.com/tidwall/gjson" +) + +// The Spec says here +// https://spec.matrix.org/v1.1/client-server-api/#server-behaviour-13 +// that +// > Servers must not send room invites from ignored users to clients. +// +// Synapse does not have this property, as detailed in +// https://github.com/matrix-org/synapse/issues/11506. +// This reproduces that bug. +func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { + deployment := Deploy(t, b.BlueprintCleanHS) + defer deployment.Destroy(t) + alice := deployment.RegisterUser(t, "hs1", "alice", "sufficiently_long_password_alice") + bob := deployment.RegisterUser(t, "hs1", "bob", "sufficiently_long_password_bob") + chris := deployment.RegisterUser(t, "hs1", "chris", "sufficiently_long_password_chris") + + // Alice creates a room for herself. + public_room := alice.CreateRoom(t, map[string]interface{}{ + "preset": "public_chat", + }) + + // Alice ignores Bob. + alice.MustDoFunc( + t, + "PUT", + []string{"_matrix", "client", "v3", "user", alice.UserID, "account_data", "m.ignored_user_list"}, + client.WithJSONBody(t, map[string]interface{}{ + "content": map[string]interface{}{ + "ignored_users": map[string]interface{}{ + bob.UserID: map[string]interface{}{}, + }, + }, + }), + ) + + start := alice.SyncUntilTimelineHas( + t, public_room, func(ev gjson.Result) bool { + return ev.Get("type").Str == "m.room.member" && + ev.Get("state_key").Str == alice.UserID && + ev.Get("content.membership").Str == "join" + }, + ) + + // Bob invites Alice to a private room. + bobRoom := bob.CreateRoom(t, map[string]interface{}{ + "preset": "private_chat", + "invite": []string{alice.UserID}, + }) + + // So does Chris. + chrisRoom := chris.CreateRoom(t, map[string]interface{}{ + "preset": "private_chat", + "invite": []string{alice.UserID}, + }) + + // Alice waits until she's seen Chris's invite. + alice.SyncUntilInvitedTo(t, chrisRoom) + + // We re-request the sync with a `since` token. We should see Chris's invite, but not Bob's. + queryParams := url.Values{ + "since": {start}, + "timeout": {"0"}, + } + // Note: SyncUntil only runs its callback on array elements. I want to investigate an object. + // So let's make the HTTP request more directly. + response := alice.MustDoFunc( + t, + "GET", + []string{"_matrix", "client", "v3", "sync"}, + client.WithQueries(queryParams), + ) + bobRoomPath := "rooms.invite." + client.GjsonEscape(bobRoom) + chrisRoomPath := "rooms.invite." + client.GjsonEscape(chrisRoom) + must.MatchResponse(t, response, match.HTTPResponse{ + JSON: []match.JSON{ + match.JSONKeyMissing(bobRoomPath), + match.JSONKeyPresent(chrisRoomPath), + }, + }) +} From 5963201350a962b1b78f3d59a69eb97476090269 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Dec 2021 18:52:57 +0000 Subject: [PATCH 04/14] goimports --- tests/csapi/ignored_users_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 8547fcbd..83768303 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -4,11 +4,12 @@ import ( "net/url" "testing" + "github.com/tidwall/gjson" + "github.com/matrix-org/complement/internal/b" "github.com/matrix-org/complement/internal/client" "github.com/matrix-org/complement/internal/match" "github.com/matrix-org/complement/internal/must" - "github.com/tidwall/gjson" ) // The Spec says here From 908f4d1ceec82cf89a5ac3dc5863096f4dff8ede Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Dec 2021 13:26:08 +0000 Subject: [PATCH 05/14] SyncUntilGlobalAccountDataHas Maybe this doesn't need a helper though? --- internal/client/client.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/client/client.go b/internal/client/client.go index 9c8cdcb8..13c332d5 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -141,6 +141,18 @@ func (c *CSAPI) SyncUntilTimelineHas(t *testing.T, roomID string, check func(gjs return c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check) } +// SyncUntilGlobalAccountDataHas is a wrapper around `SyncUntil`. +// It blocks and continually calls `/sync` until +// - we an event in the global account data for which the `check` function returns True +// If the `check` function fails the test, the failing event will be automatically logged. +// Will time out after CSAPI.SyncUntilTimeout. +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntilGlobalAccountDataHas(t *testing.T, check func(gjson.Result) bool) string { + t.Helper() + return c.SyncUntil(t, "", "", "account_data.events", check) +} // SyncUntilInvitedTo is a wrapper around SyncUntil. // It blocks and continually calls `/sync` until we've been invited to the given room. // Will time out after CSAPI.SyncUntilTimeout. From 3373df135db21051925e37c479d92e47418100eb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Dec 2021 13:29:37 +0000 Subject: [PATCH 06/14] Explicitly wait for the ignore to be synced --- tests/csapi/ignored_users_test.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 83768303..4c76d1eb 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -32,6 +32,15 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { "preset": "public_chat", }) + // Alice waits to see the join event. + alice.SyncUntilTimelineHas( + t, public_room, func(ev gjson.Result) bool { + return ev.Get("type").Str == "m.room.member" && + ev.Get("state_key").Str == alice.UserID && + ev.Get("content.membership").Str == "join" + }, + ) + // Alice ignores Bob. alice.MustDoFunc( t, @@ -46,11 +55,13 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { }), ) - start := alice.SyncUntilTimelineHas( - t, public_room, func(ev gjson.Result) bool { - return ev.Get("type").Str == "m.room.member" && - ev.Get("state_key").Str == alice.UserID && - ev.Get("content.membership").Str == "join" + // Alice waits to see that the ignore was successful. + sinceJoinedAndIgnored := alice.SyncUntilGlobalAccountDataHas( + t, + func(ev gjson.Result) bool { + t.Logf(ev.Raw + "\n") + return ev.Get("type").Str == "m.ignored_user_list" && + ev.Get("content.ignored_users."+client.GjsonEscape(bob.UserID)).Exists() }, ) @@ -71,7 +82,7 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { // We re-request the sync with a `since` token. We should see Chris's invite, but not Bob's. queryParams := url.Values{ - "since": {start}, + "since": {sinceJoinedAndIgnored}, "timeout": {"0"}, } // Note: SyncUntil only runs its callback on array elements. I want to investigate an object. From a347f1e2a17294f6782b67e871b126a400b883af Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Dec 2021 13:29:53 +0000 Subject: [PATCH 07/14] Fix format of the ignore request --- tests/csapi/ignored_users_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 4c76d1eb..59530501 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -47,10 +47,8 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { "PUT", []string{"_matrix", "client", "v3", "user", alice.UserID, "account_data", "m.ignored_user_list"}, client.WithJSONBody(t, map[string]interface{}{ - "content": map[string]interface{}{ - "ignored_users": map[string]interface{}{ - bob.UserID: map[string]interface{}{}, - }, + "ignored_users": map[string]interface{}{ + bob.UserID: map[string]interface{}{}, }, }), ) From f38ae642a9fe5c26c949d108ce65445461d60ba1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Dec 2021 13:35:06 +0000 Subject: [PATCH 08/14] Goimports --- internal/client/client.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/client/client.go b/internal/client/client.go index 13c332d5..3e96e7a9 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -153,6 +153,7 @@ func (c *CSAPI) SyncUntilGlobalAccountDataHas(t *testing.T, check func(gjson.Res t.Helper() return c.SyncUntil(t, "", "", "account_data.events", check) } + // SyncUntilInvitedTo is a wrapper around SyncUntil. // It blocks and continually calls `/sync` until we've been invited to the given room. // Will time out after CSAPI.SyncUntilTimeout. From 0b453d965a052c8ed146c825ddfe6bdd54f8da91 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Dec 2021 13:43:36 +0000 Subject: [PATCH 09/14] Blocklist this test on Dendrite --- tests/csapi/ignored_users_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 59530501..c6984606 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -1,3 +1,6 @@ +// +build !dendrite_blacklist + +// Rationale for being included in Dendrite's blacklist: https://github.com/matrix-org/dendrite/issues/600 package csapi_tests import ( From dfc81b739a085842915d9c3c2c1ad39464f4067d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 9 Dec 2021 14:35:13 +0000 Subject: [PATCH 10/14] Use lowerCamelCase for variables --- tests/csapi/ignored_users_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index c6984606..607149b7 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -31,13 +31,13 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { chris := deployment.RegisterUser(t, "hs1", "chris", "sufficiently_long_password_chris") // Alice creates a room for herself. - public_room := alice.CreateRoom(t, map[string]interface{}{ + publicRoom := alice.CreateRoom(t, map[string]interface{}{ "preset": "public_chat", }) // Alice waits to see the join event. alice.SyncUntilTimelineHas( - t, public_room, func(ev gjson.Result) bool { + t, publicRoom, func(ev gjson.Result) bool { return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == alice.UserID && ev.Get("content.membership").Str == "join" From 512c226c6671789fcb79845ff0acf047458c6a76 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 9 Dec 2021 14:36:20 +0000 Subject: [PATCH 11/14] Use r0 instead of v3 --- tests/csapi/ignored_users_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 607149b7..bdc6a6f9 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -48,7 +48,7 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { alice.MustDoFunc( t, "PUT", - []string{"_matrix", "client", "v3", "user", alice.UserID, "account_data", "m.ignored_user_list"}, + []string{"_matrix", "client", "r0", "user", alice.UserID, "account_data", "m.ignored_user_list"}, client.WithJSONBody(t, map[string]interface{}{ "ignored_users": map[string]interface{}{ bob.UserID: map[string]interface{}{}, @@ -91,7 +91,7 @@ func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { response := alice.MustDoFunc( t, "GET", - []string{"_matrix", "client", "v3", "sync"}, + []string{"_matrix", "client", "r0", "sync"}, client.WithQueries(queryParams), ) bobRoomPath := "rooms.invite." + client.GjsonEscape(bobRoom) From 1086814dd1f6b014a8fbc23fcd7467ecf700d508 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 9 Dec 2021 14:36:50 +0000 Subject: [PATCH 12/14] Try without dendrite blacklist maybe dendrite does support account data under r0? --- tests/csapi/ignored_users_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index bdc6a6f9..60c50ca9 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -1,6 +1,3 @@ -// +build !dendrite_blacklist - -// Rationale for being included in Dendrite's blacklist: https://github.com/matrix-org/dendrite/issues/600 package csapi_tests import ( From 7f6ec2575e0271e9623743751789a7148b9e72df Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 9 Dec 2021 15:01:06 +0000 Subject: [PATCH 13/14] Revert "Try without dendrite blacklist" This reverts commit 1086814dd1f6b014a8fbc23fcd7467ecf700d508. --- tests/csapi/ignored_users_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index 60c50ca9..bdc6a6f9 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -1,3 +1,6 @@ +// +build !dendrite_blacklist + +// Rationale for being included in Dendrite's blacklist: https://github.com/matrix-org/dendrite/issues/600 package csapi_tests import ( From 8b434d7bf821b84bc9b37215893c8026c57150d0 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 9 Dec 2021 15:08:38 +0000 Subject: [PATCH 14/14] Describe this as a regression test --- tests/csapi/ignored_users_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/csapi/ignored_users_test.go b/tests/csapi/ignored_users_test.go index bdc6a6f9..f3b42d37 100644 --- a/tests/csapi/ignored_users_test.go +++ b/tests/csapi/ignored_users_test.go @@ -20,9 +20,9 @@ import ( // that // > Servers must not send room invites from ignored users to clients. // -// Synapse does not have this property, as detailed in -// https://github.com/matrix-org/synapse/issues/11506. -// This reproduces that bug. +// This is a regression test for +// https://github.com/matrix-org/synapse/issues/11506 +// to ensure that Synapse complies with this part of the spec. func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) { deployment := Deploy(t, b.BlueprintCleanHS) defer deployment.Destroy(t)