Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test that invites from ignored users are omitted from incremental syncs #267

Merged
merged 14 commits into from
Dec 9, 2021
34 changes: 28 additions & 6 deletions internal/client/client.go
Expand Up @@ -133,22 +133,41 @@ 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()
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()
c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check)
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.
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
Expand All @@ -157,7 +176,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
Expand Down Expand Up @@ -199,7 +221,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++
Expand Down
12 changes: 12 additions & 0 deletions internal/match/json.go
Expand Up @@ -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 {
Expand Down
105 changes: 105 additions & 0 deletions tests/csapi/ignored_users_test.go
@@ -0,0 +1,105 @@
// +build !dendrite_blacklist

// Rationale for being included in Dendrite's blacklist: https://github.com/matrix-org/dendrite/issues/600
package csapi_tests

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"
)

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe?

Suggested change
// Synapse does not have this property, as detailed in
// https://github.com/matrix-org/synapse/issues/11506.
// This reproduces that bug.
// Synapse does not handle this properly, as detailed in
// https://github.com/matrix-org/synapse/issues/11506.
// This reproduces that bug.

I don't think this is true though since this is now fixed, it might be better to say:

This is a regression test for link to issue.

func TestInviteFromIgnoredUsersDoesNotAppearInSync(t *testing.T) {
deployment := Deploy(t, b.BlueprintCleanHS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be much simpler, I think, by starting with BlueprintOneToOneRoom? I'm not sure the third user is very helpful here.

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 third user was an attempt to stop this test from being racey. We want to be convinced that the server has processed Bob's invite and chosen to ignore it. I could assert that there's no invite from Bob in Alice's next sync, but... what if that's only true because Alice synced really quickly?

In #237 (comment) suggested sending and awaiting a dummy event. The third user was my attempt at doing something similar here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could do that by sending a dummy user into the room, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, would Alice expect to see state updates for a room she's invited to, but hasn't joined yet? If so that would be much clearer.

Copy link
Contributor

@clokep clokep Dec 7, 2021

Choose a reason for hiding this comment

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

Oh, would Alice expect to see state updates for a room she's invited to, but hasn't joined yet? If so that would be much clearer.

No, they would not see those. I think I'm confused though -- isn't this just checking that some sort of events got processed by any room? (I.e. sync is working at all?)

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{}{
Copy link
Member

Choose a reason for hiding this comment

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

Please do not use underscores for variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, old habits. Is there some way to configure gofmt/goimports or VSCode to flag this up?

"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,
"PUT",
[]string{"_matrix", "client", "v3", "user", alice.UserID, "account_data", "m.ignored_user_list"},
Copy link
Member

Choose a reason for hiding this comment

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

Dendrite doesn't implement any /v3/ endpoint. Is there some spec documentation on the changes between r0 and v3?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dendrite doesn't implement any /v3/ endpoint. Is there some spec documentation on the changes between r0 and v3?

All r0 endpoints became v3 with Matrix 1.1 -- @DMRobertson Maybe we should just use r0 for now here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to use r0; I was just blindly going by what the latest spec said.

client.WithJSONBody(t, map[string]interface{}{
"ignored_users": map[string]interface{}{
bob.UserID: map[string]interface{}{},
},
}),
)

// 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()
},
)

// 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": {sinceJoinedAndIgnored},
"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),
},
})
}