Fix flawed membership checks and align membership checking patterns

client/sync.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"net/url"
 	"reflect"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -269,95 +270,138 @@ func SyncPresenceHas(fromUser string, expectedPresence *string, checks ...func(g
 	}
 }
 
-// Checks that `userID` gets invited to `roomID`.
+// syncMembershipIn checks that `userID` has `membership` in `roomID`, with optional
+// extra checks on the found membership event.
 //
-// This checks different parts of the /sync response depending on the client making the request.
-// If the client is also the person being invited to the room then the 'invite' block will be inspected.
-// If the client is different to the person being invited then the 'join' block will be inspected.
-func SyncInvitedTo(userID, roomID string) SyncCheckOpt {
-	return func(clientUserID string, topLevelSyncJSON gjson.Result) error {
-		// two forms which depend on what the client user is:
-		// - passively viewing an invite for a room you're joined to (timeline events)
-		// - actively being invited to a room.
-		if clientUserID == userID {
-			// active
-			err := checkArrayElements(
-				topLevelSyncJSON, "rooms.invite."+GjsonEscape(roomID)+".invite_state.events",
-				func(ev gjson.Result) bool {
-					return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == userID && ev.Get("content.membership").Str == "invite"
-				},
-			)
-			if err != nil {
-				return fmt.Errorf("SyncInvitedTo(%s): %s", roomID, err)
-			}
-			return nil
-		}
-		// passive
-		return SyncTimelineHas(roomID, func(ev gjson.Result) bool {
-			return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == userID && ev.Get("content.membership").Str == "invite"
-		})(clientUserID, topLevelSyncJSON)
-	}
-}
-
-// Check that `userID` gets joined to `roomID` by inspecting the join timeline for a membership event.
+// This can be also used to passively observe another user's membership changes in a
+// room although we assume that the observing client is joined to the room.
 //
-// Additional checks can be passed to narrow down the check, all must pass.
-func SyncJoinedTo(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
-	checkJoined := func(ev gjson.Result) bool {
-		if ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == userID && ev.Get("content.membership").Str == "join" {
+// Note: This will not work properly with leave/ban membership for initial syncs, see
+// https://github.com/matrix-org/matrix-doc/issues/3537
+func syncMembershipIn(userID, roomID, membership string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	checkMembership := func(ev gjson.Result) bool {
+		if ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == userID && ev.Get("content.membership").Str == membership {
 			for _, check := range checks {
 				if !check(ev) {
 					// short-circuit, bail early
 					return false
 				}
 			}
-			// passed both basic join check and all other checks
+			// passed both basic membership check and all other checks
 			return true
 		}
 		return false
 	}
 	return func(clientUserID string, topLevelSyncJSON gjson.Result) error {
-		// Check both the timeline and the state events for the join event
-		// since on initial sync, the state events may only be in
-		// <room>.state.events.
+		// Check both the timeline and the state events for the membership event since on
+		// initial sync, the state events may only be in state. Additionally, state only
+		// covers the "updates for the room up to the start of the timeline."
+
+		// We assume the passively observing client user is joined to the room
+		roomTypeKey := "join"
+		// Otherwise, if the client is the user whose membership we are checking, we need to
+		// pick the correct room type JSON key based on the membership being checked.
+		if clientUserID == userID {
+			if membership == "join" {
+				roomTypeKey = "join"
+			} else if membership == "leave" || membership == "ban" {
+				roomTypeKey = "leave"
+			} else if membership == "invite" {
+				roomTypeKey = "invite"
+			} else if membership == "knock" {
+				roomTypeKey = "knock"
+			} else {
+				return fmt.Errorf("syncMembershipIn(%s, %s): unknown membership: %s", roomID, membership, membership)
+			}
+		}
+
+		// We assume the passively observing client user is joined to the room (`rooms.join.<roomID>.state`)
+		stateKey := "state"
+		// Otherwise, if the client is the user whose membership we are checking,
+		// we need to pick the correct JSON key based on the membership being checked.
+		if clientUserID == userID {
+			if membership == "join" || membership == "leave" || membership == "ban" {
+				stateKey = "state"
+			} else if membership == "invite" {
+				stateKey = "invite_state"
+			} else if membership == "knock" {
+				stateKey = "knock_state"
+			} else {
+				return fmt.Errorf("syncMembershipIn(%s, %s): unknown membership: %s", roomID, membership, membership)
+			}
+		}
+
+		// Check the state first as it's a better source of truth than the `timeline`.
+		//
+		// FIXME: Ideally, we'd use something like `state_after` to get the actual current
+		// state in the room instead of us assuming that no state resets/conflicts happen
+		// when we apply state from the `timeline` on top of the `state`. But `state_after`
+		// is gated behind a sync request parameter which we can't control here.
 		firstErr := checkArrayElements(
-			topLevelSyncJSON, "rooms.join."+GjsonEscape(roomID)+".timeline.events", checkJoined,
+			topLevelSyncJSON, "rooms."+roomTypeKey+"."+GjsonEscape(roomID)+"."+stateKey+".events", checkMembership,
 		)
 		if firstErr == nil {
 			return nil
 		}
 
-		secondErr := checkArrayElements(
-			topLevelSyncJSON, "rooms.join."+GjsonEscape(roomID)+".state.events", checkJoined,
-		)
-		if secondErr == nil {
-			return nil
+		// Check the timeline
+		//
+		// This is also important to differentiate between leave/ban because those both
+		// appear in the `leave` `roomTypeKey` and we need to specifically check the
+		// timeline for the membership event to differentiate them.
+		var secondErr error
+		// The `timeline` is only available for join/leave/ban memberships.
+		if slices.Contains([]string{"join", "leave", "ban"}, membership) ||
+			// We assume the passively observing client user is joined to the room (therefore
+			// has `timeline`).
+			clientUserID != userID {
+			secondErr = checkArrayElements(
+				topLevelSyncJSON, "rooms."+roomTypeKey+"."+GjsonEscape(roomID)+".timeline.events", checkMembership,
+			)
+			if secondErr == nil {
+				return nil
+			}
 		}
-		return fmt.Errorf("SyncJoinedTo(%s): %s & %s", roomID, firstErr, secondErr)
+
+		return fmt.Errorf("syncMembershipIn(%s, %s): %s & %s - %s", roomID, membership, firstErr, secondErr, topLevelSyncJSON)
 	}
 }
 
-// Check that `userID` is leaving `roomID` by inspecting the timeline for a membership event, or witnessing `roomID` in `rooms.leave`
+// Checks that `userID` gets invited to `roomID`
+//
+// Additional checks can be passed to narrow down the check, all must pass.
+func SyncInvitedTo(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	return syncMembershipIn(userID, roomID, "invite", checks...)
+}
+
+// Checks that `userID` has knocked on `roomID`
+//
+// Additional checks can be passed to narrow down the check, all must pass.
+func SyncKnockedOn(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	return syncMembershipIn(userID, roomID, "knock", checks...)
+}
+
+// Check that `userID` gets joined to `roomID`
+//
+// Additional checks can be passed to narrow down the check, all must pass.
+func SyncJoinedTo(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	return syncMembershipIn(userID, roomID, "join", checks...)
+}
+
+// Check that `userID` has left the `roomID`
 // Note: This will not work properly with initial syncs, see https://github.com/matrix-org/matrix-doc/issues/3537
-func SyncLeftFrom(userID, roomID string) SyncCheckOpt {
-	return func(clientUserID string, topLevelSyncJSON gjson.Result) error {
-		// two forms which depend on what the client user is:
-		// - passively viewing a membership for a room you're joined in
-		// - actively leaving the room
-		if clientUserID == userID {
-			// active
-			events := topLevelSyncJSON.Get("rooms.leave." + GjsonEscape(roomID))
-			if !events.Exists() {
-				return fmt.Errorf("no leave section for room %s", roomID)
-			} else {
-				return nil
-			}
-		}
-		// passive
-		return SyncTimelineHas(roomID, func(ev gjson.Result) bool {
-			return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == userID && ev.Get("content.membership").Str == "leave"
-		})(clientUserID, topLevelSyncJSON)
-	}
+//
+// Additional checks can be passed to narrow down the check, all must pass.
+func SyncLeftFrom(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	return syncMembershipIn(userID, roomID, "leave", checks...)
+}
+
+// Check that `userID` is banned from the `roomID`
+// Note: This will not work properly with initial syncs, see https://github.com/matrix-org/matrix-doc/issues/3537
+//
+// Additional checks can be passed to narrow down the check, all must pass.
+func SyncBannedFrom(userID, roomID string, checks ...func(gjson.Result) bool) SyncCheckOpt {
+	return syncMembershipIn(userID, roomID, "ban", checks...)
 }
 
 // Calls the `check` function for each global account data event, and returns with success if the

tests/csapi/apidoc_room_members_test.go

@@ -79,16 +79,7 @@ func TestRoomMembers(t *testing.T) {
 				},
 			})
 
-			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-				roomID,
-				func(ev gjson.Result) bool {
-					if ev.Get("type").Str != "m.room.member" || ev.Get("state_key").Str != bob.UserID {
-						return false
-					}
-					must.Equal(t, ev.Get("content").Get("membership").Str, "join", "Bob failed to join the room")
-					return true
-				},
-			))
+			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID))
 		})
 		// sytest: Test that we can be reinvited to a room we created
 		t.Run("Test that we can be reinvited to a room we created", func(t *testing.T) {
@@ -122,14 +113,7 @@ func TestRoomMembers(t *testing.T) {
 			alice.MustLeaveRoom(t, roomID)
 
 			// Wait until alice has left the room
-			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(
-				roomID,
-				func(ev gjson.Result) bool {
-					return ev.Get("type").Str == "m.room.member" &&
-						ev.Get("content.membership").Str == "leave" &&
-						ev.Get("state_key").Str == alice.UserID
-				},
-			))
+			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncLeftFrom(alice.UserID, roomID))
 
 			bob.MustInviteRoom(t, roomID, alice.UserID)
 			since := alice.MustSyncUntil(t, client.SyncReq{}, client.SyncInvitedTo(alice.UserID, roomID))
@@ -203,12 +187,7 @@ func TestRoomMembers(t *testing.T) {
 			})
 			res := alice.Do(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "ban"}, banBody)
 			must.MatchResponse(t, res, match.HTTPResponse{StatusCode: 200})
-			alice.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(roomID, func(ev gjson.Result) bool {
-				if ev.Get("type").Str != "m.room.member" || ev.Get("state_key").Str != bob.UserID {
-					return false
-				}
-				return ev.Get("content.membership").Str == "ban"
-			}))
+			alice.MustSyncUntil(t, client.SyncReq{}, client.SyncBannedFrom(bob.UserID, roomID))
 			// verify bob is banned
 			content := alice.MustGetStateEventContent(t, roomID, "m.room.member", bob.UserID)
 			must.MatchGJSON(t, content, match.JSONKeyEqual("membership", "ban"))

tests/csapi/rooms_state_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/matrix-org/complement/b"
 	"github.com/matrix-org/complement/client"
 	"github.com/matrix-org/complement/helpers"
+	"github.com/matrix-org/complement/match"
 	"github.com/matrix-org/complement/must"
 )
 
@@ -46,13 +47,10 @@ func TestRoomCreationReportsEventsToMyself(t *testing.T) {
 		t.Run("Room creation reports m.room.member to myself", func(t *testing.T) {
 			t.Parallel()
 
-			alice.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas(roomID, func(ev gjson.Result) bool {
-				if ev.Get("type").Str != "m.room.member" {
-					return false
-				}
-				must.Equal(t, ev.Get("sender").Str, alice.UserID, "wrong sender")
-				must.Equal(t, ev.Get("state_key").Str, alice.UserID, "wrong state_key")
-				must.Equal(t, ev.Get("content").Get("membership").Str, "join", "wrong content.membership")
+			alice.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(alice.UserID, roomID, func(ev gjson.Result) bool {
+				must.MatchGJSON(t, ev,
+					match.JSONKeyEqual("sender", alice.UserID),
+				)
 				return true
 			}))
 		})

tests/federation_room_ban_test.go

@@ -30,7 +30,7 @@ func TestUnbanViaInvite(t *testing.T) {
 	bob.MustDo(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "ban"}, client.WithJSONBody(t, map[string]interface{}{
 		"user_id": alice.UserID,
 	}))
-	alice.MustSyncUntil(t, client.SyncReq{}, client.SyncLeftFrom(alice.UserID, roomID))
+	alice.MustSyncUntil(t, client.SyncReq{}, client.SyncBannedFrom(alice.UserID, roomID))
 
 	// Unban Alice
 	bob.MustDo(t, "POST", []string{"_matrix", "client", "v3", "rooms", roomID, "unban"}, client.WithJSONBody(t, map[string]interface{}{

tests/federation_rooms_invite_test.go

@@ -227,22 +227,15 @@ func TestFederationRoomsInvite(t *testing.T) {
 				"is_direct": true,
 			})
 			bob.MustJoinRoom(t, roomID, []spec.ServerName{})
-			bob.MustSyncUntil(t, client.SyncReq{},
-				client.SyncTimelineHas(roomID, func(result gjson.Result) bool {
-					// We expect a membership event ..
-					if result.Get("type").Str != spec.MRoomMember {
-						return false
-					}
-					// .. for Bob
-					if result.Get("state_key").Str != bob.UserID {
-						return false
-					}
-					// Check that we've got tbe expected is_idrect flag
-					return result.Get("unsigned.prev_content.membership").Str == "invite" &&
-						result.Get("unsigned.prev_content.is_direct").Bool() == true &&
-						result.Get("unsigned.prev_sender").Str == alice.UserID
-				}),
-			)
+
+			bob.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(bob.UserID, roomID, func(ev gjson.Result) bool {
+				must.MatchGJSON(t, ev,
+					match.JSONKeyEqual("unsigned.prev_content.membership", "invite"),
+					match.JSONKeyEqual("unsigned.prev_content.is_direct", true),
+					match.JSONKeyEqual("unsigned.prev_sender", alice.UserID),
+				)
+				return true
+			}))
 		})
 	})
 }