[pseudoIDs] More pseudo ID fixes - Part 2 (#3181)

Fixes include:
- Translating state keys that contain user IDs to their respective room
keys for both querying and sending state events
- **NOTE**: there may be design discussion needed on what should happen
when sender keys cannot be found for users
- A simple fix for kicking guests from rooms properly
- Logic for boundary history visibilities was slightly off (I'm
surprised this only manifested in pseudo ID room versions)

Signed-off-by: `Sam Wedgwood <sam@wedgwood.dev>`

4 files changed, 274 insertions, 24 deletions

diff --git a/syncapi/internal/history_visibility.go b/syncapi/internal/history_visibility.go
index 91a2d63c..7aae9fd3 100644
--- a/syncapi/internal/history_visibility.go
+++ b/syncapi/internal/history_visibility.go
@@ -163,17 +163,23 @@ func ApplyHistoryVisibilityFilter(
 		// by setting the effective evVis to the least restrictive
 		// of the old vs new.
 		// https://spec.matrix.org/v1.3/client-server-api/#server-behaviour-5
-		if hisVis, err := ev.HistoryVisibility(); err == nil {
-			prevHisVis := gjson.GetBytes(ev.Unsigned(), "prev_content.history_visibility").String()
-			oldPrio, ok := historyVisibilityPriority[gomatrixserverlib.HistoryVisibility(prevHisVis)]
-			// if we can't get the previous history visibility, default to shared.
-			if !ok {
-				oldPrio = historyVisibilityPriority[gomatrixserverlib.HistoryVisibilityShared]
-			}
-			// no OK check, since this should have been validated when setting the value
-			newPrio := historyVisibilityPriority[hisVis]
-			if oldPrio < newPrio {
-				evVis.visibility = gomatrixserverlib.HistoryVisibility(prevHisVis)
+		if ev.Type() == spec.MRoomHistoryVisibility {
+			hisVis, err := ev.HistoryVisibility()
+
+			if err == nil && hisVis != "" {
+				prevHisVis := gjson.GetBytes(ev.Unsigned(), "prev_content.history_visibility").String()
+				oldPrio, ok := historyVisibilityPriority[gomatrixserverlib.HistoryVisibility(prevHisVis)]
+				// if we can't get the previous history visibility, default to shared.
+				if !ok {
+					oldPrio = historyVisibilityPriority[gomatrixserverlib.HistoryVisibilityShared]
+				}
+				// no OK check, since this should have been validated when setting the value
+				newPrio := historyVisibilityPriority[hisVis]
+				if oldPrio < newPrio {
+					evVis.visibility = gomatrixserverlib.HistoryVisibility(prevHisVis)
+				} else {
+					evVis.visibility = hisVis
+				}
 			}
 		}
 		// do the actual check
diff --git a/syncapi/internal/history_visibility_test.go b/syncapi/internal/history_visibility_test.go
new file mode 100644
index 00000000..984f90ed
--- /dev/null
+++ b/syncapi/internal/history_visibility_test.go
@@ -0,0 +1,214 @@
+package internal
+
+import (
+	"context"
+	"fmt"
+	"math"
+	"testing"
+
+	rsapi "github.com/matrix-org/dendrite/roomserver/api"
+	"github.com/matrix-org/dendrite/roomserver/types"
+	"github.com/matrix-org/dendrite/syncapi/storage"
+	"github.com/matrix-org/gomatrixserverlib"
+	"github.com/matrix-org/gomatrixserverlib/spec"
+	"gotest.tools/v3/assert"
+)
+
+type mockHisVisRoomserverAPI struct {
+	rsapi.RoomserverInternalAPI
+	events []*types.HeaderedEvent
+	roomID string
+}
+
+func (s *mockHisVisRoomserverAPI) QueryMembershipAtEvent(ctx context.Context, roomID spec.RoomID, eventIDs []string, senderID spec.SenderID) (map[string]*types.HeaderedEvent, error) {
+	if roomID.String() == s.roomID {
+		membershipMap := map[string]*types.HeaderedEvent{}
+
+		for _, queriedEventID := range eventIDs {
+			for _, event := range s.events {
+				if event.EventID() == queriedEventID {
+					membershipMap[queriedEventID] = event
+				}
+			}
+		}
+
+		return membershipMap, nil
+	} else {
+		return nil, fmt.Errorf("room not found: \"%v\"", roomID)
+	}
+}
+
+func (s *mockHisVisRoomserverAPI) QuerySenderIDForUser(ctx context.Context, roomID spec.RoomID, userID spec.UserID) (*spec.SenderID, error) {
+	senderID := spec.SenderIDFromUserID(userID)
+	return &senderID, nil
+}
+
+func (s *mockHisVisRoomserverAPI) QueryUserIDForSender(ctx context.Context, roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
+	userID := senderID.ToUserID()
+	if userID == nil {
+		return nil, fmt.Errorf("sender ID not user ID")
+	}
+	return userID, nil
+}
+
+type mockDB struct {
+	storage.DatabaseTransaction
+	// user ID -> membership (i.e. 'join', 'leave', etc.)
+	currentMembership map[string]string
+	roomID            string
+}
+
+func (s *mockDB) SelectMembershipForUser(ctx context.Context, roomID string, userID string, pos int64) (string, int, error) {
+	if roomID == s.roomID {
+		membership, ok := s.currentMembership[userID]
+		if !ok {
+			return spec.Leave, math.MaxInt64, nil
+		}
+		return membership, math.MaxInt64, nil
+	}
+
+	return "", 0, fmt.Errorf("room not found: \"%v\"", roomID)
+}
+
+// Tests logic around history visibility boundaries
+//
+// Specifically that if a room's history visibility before or after a particular history visibility event
+// allows them to see events (a boundary), then the history visibility event itself should be shown
+// ( spec: https://spec.matrix.org/v1.8/client-server-api/#server-behaviour-5 )
+//
+// This also aims to emulate "Only see history_visibility changes on bounadries" in sytest/tests/30rooms/30history-visibility.pl
+func Test_ApplyHistoryVisbility_Boundaries(t *testing.T) {
+	ctx := context.Background()
+
+	roomID := "!roomid:domain"
+
+	creatorUserID := spec.NewUserIDOrPanic("@creator:domain", false)
+	otherUserID := spec.NewUserIDOrPanic("@other:domain", false)
+	roomVersion := gomatrixserverlib.RoomVersionV10
+	roomVerImpl := gomatrixserverlib.MustGetRoomVersion(roomVersion)
+
+	eventsJSON := []struct {
+		id   string
+		json string
+	}{
+		{id: "$create-event", json: fmt.Sprintf(`{
+			"type": "m.room.create", "state_key": "",
+			"room_id": "%v", "sender": "%v",
+			"content": {"creator": "%v", "room_version": "%v"}
+		}`, roomID, creatorUserID.String(), creatorUserID.String(), roomVersion)},
+		{id: "$creator-joined", json: fmt.Sprintf(`{
+			"type": "m.room.member", "state_key": "%v",
+			"room_id": "%v", "sender": "%v",
+			"content": {"membership": "join"}
+		}`, creatorUserID.String(), roomID, creatorUserID.String())},
+		{id: "$hisvis-1", json: fmt.Sprintf(`{
+			"type": "m.room.history_visibility", "state_key": "",
+			"room_id": "%v", "sender": "%v",
+			"content": {"history_visibility": "shared"}
+		}`, roomID, creatorUserID.String())},
+		{id: "$msg-1", json: fmt.Sprintf(`{
+			"type": "m.room.message",
+			"room_id": "%v", "sender": "%v",
+			"content": {"body": "1"}
+		}`, roomID, creatorUserID.String())},
+		{id: "$hisvis-2", json: fmt.Sprintf(`{
+			"type": "m.room.history_visibility", "state_key": "",
+			"room_id": "%v", "sender": "%v",
+			"content": {"history_visibility": "joined"},
+			"unsigned": {"prev_content": {"history_visibility": "shared"}}
+		}`, roomID, creatorUserID.String())},
+		{id: "$msg-2", json: fmt.Sprintf(`{
+			"type": "m.room.message",
+			"room_id": "%v", "sender": "%v",
+			"content": {"body": "1"}
+		}`, roomID, creatorUserID.String())},
+		{id: "$hisvis-3", json: fmt.Sprintf(`{
+			"type": "m.room.history_visibility", "state_key": "",
+			"room_id": "%v", "sender": "%v",
+			"content": {"history_visibility": "invited"},
+			"unsigned": {"prev_content": {"history_visibility": "joined"}}
+		}`, roomID, creatorUserID.String())},
+		{id: "$msg-3", json: fmt.Sprintf(`{
+			"type": "m.room.message",
+			"room_id": "%v", "sender": "%v",
+			"content": {"body": "2"}
+		}`, roomID, creatorUserID.String())},
+		{id: "$hisvis-4", json: fmt.Sprintf(`{
+			"type": "m.room.history_visibility", "state_key": "",
+			"room_id": "%v", "sender": "%v",
+			"content": {"history_visibility": "shared"},
+			"unsigned": {"prev_content": {"history_visibility": "invited"}}
+		}`, roomID, creatorUserID.String())},
+		{id: "$msg-4", json: fmt.Sprintf(`{
+			"type": "m.room.message",
+			"room_id": "%v", "sender": "%v",
+			"content": {"body": "3"}
+		}`, roomID, creatorUserID.String())},
+		{id: "$other-joined", json: fmt.Sprintf(`{
+			"type": "m.room.member", "state_key": "%v",
+			"room_id": "%v", "sender": "%v",
+			"content": {"membership": "join"}
+		}`, otherUserID.String(), roomID, otherUserID.String())},
+	}
+
+	events := make([]*types.HeaderedEvent, len(eventsJSON))
+
+	hisVis := gomatrixserverlib.HistoryVisibilityShared
+
+	for i, eventJSON := range eventsJSON {
+		pdu, err := roomVerImpl.NewEventFromTrustedJSONWithEventID(eventJSON.id, []byte(eventJSON.json), false)
+		if err != nil {
+			t.Fatalf("failed to prepare event %s for test: %s", eventJSON.id, err.Error())
+		}
+		events[i] = &types.HeaderedEvent{PDU: pdu}
+
+		// 'Visibility' should be the visibility of the room just before this event was sent
+		// (according to processRoomEvent in roomserver/internal/input/input_events.go)
+		events[i].Visibility = hisVis
+		if pdu.Type() == spec.MRoomHistoryVisibility {
+			newHisVis, err := pdu.HistoryVisibility()
+			if err != nil {
+				t.Fatalf("failed to prepare history visibility event: %s", err.Error())
+			}
+			hisVis = newHisVis
+		}
+	}
+
+	rsAPI := &mockHisVisRoomserverAPI{
+		events: events,
+		roomID: roomID,
+	}
+	syncDB := &mockDB{
+		roomID: roomID,
+		currentMembership: map[string]string{
+			creatorUserID.String(): spec.Join,
+			otherUserID.String():   spec.Join,
+		},
+	}
+
+	filteredEvents, err := ApplyHistoryVisibilityFilter(ctx, syncDB, rsAPI, events, nil, otherUserID, "hisVisTest")
+	if err != nil {
+		t.Fatalf("ApplyHistoryVisibility returned non-nil error: %s", err.Error())
+	}
+
+	filteredEventIDs := make([]string, len(filteredEvents))
+	for i, event := range filteredEvents {
+		filteredEventIDs[i] = event.EventID()
+	}
+
+	assert.DeepEqual(t,
+		[]string{
+			"$create-event",   // Always see m.room.create
+			"$creator-joined", // Always see membership
+			"$hisvis-1",       // Sets room to shared (technically the room is already shared since shared is default)
+			"$msg-1",          // Room currently 'shared'
+			"$hisvis-2",       // Room changed from 'shared' to 'joined', so boundary event and should be shared
+			// Other events hidden, as other is not joined yet
+			// hisvis-3 is also hidden, as it changes from joined to invited, neither of which is visible to other
+			"$hisvis-4",     // Changes from 'invited' to 'shared', so is a boundary event and visible
+			"$msg-4",        // Room is 'shared', so visible
+			"$other-joined", // other's membership
+		},
+		filteredEventIDs,
+	)
+}
diff --git a/syncapi/internal/keychange_test.go b/syncapi/internal/keychange_test.go
index 81b82bf6..56954cfa 100644
--- a/syncapi/internal/keychange_test.go
+++ b/syncapi/internal/keychange_test.go
@@ -59,22 +59,22 @@ func (k *mockKeyAPI) QueryDeviceMessages(ctx context.Context, req *userapi.Query
 func (k *mockKeyAPI) QuerySignatures(ctx context.Context, req *userapi.QuerySignaturesRequest, res *userapi.QuerySignaturesResponse) {
 }
 
-type mockRoomserverAPI struct {
+type keyChangeMockRoomserverAPI struct {
 	api.RoomserverInternalAPI
 	roomIDToJoinedMembers map[string][]string
 }
 
-func (s *mockRoomserverAPI) QueryUserIDForSender(ctx context.Context, roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
+func (s *keyChangeMockRoomserverAPI) QueryUserIDForSender(ctx context.Context, roomID spec.RoomID, senderID spec.SenderID) (*spec.UserID, error) {
 	return spec.NewUserID(string(senderID), true)
 }
 
 // QueryRoomsForUser retrieves a list of room IDs matching the given query.
-func (s *mockRoomserverAPI) QueryRoomsForUser(ctx context.Context, userID spec.UserID, desiredMembership string) ([]spec.RoomID, error) {
+func (s *keyChangeMockRoomserverAPI) QueryRoomsForUser(ctx context.Context, userID spec.UserID, desiredMembership string) ([]spec.RoomID, error) {
 	return nil, nil
 }
 
 // QueryBulkStateContent does a bulk query for state event content in the given rooms.
-func (s *mockRoomserverAPI) QueryBulkStateContent(ctx context.Context, req *api.QueryBulkStateContentRequest, res *api.QueryBulkStateContentResponse) error {
+func (s *keyChangeMockRoomserverAPI) QueryBulkStateContent(ctx context.Context, req *api.QueryBulkStateContentRequest, res *api.QueryBulkStateContentResponse) error {
 	res.Rooms = make(map[string]map[gomatrixserverlib.StateKeyTuple]string)
 	if req.AllowWildcards && len(req.StateTuples) == 1 && req.StateTuples[0].EventType == spec.MRoomMember && req.StateTuples[0].StateKey == "*" {
 		for _, roomID := range req.RoomIDs {
@@ -91,7 +91,7 @@ func (s *mockRoomserverAPI) QueryBulkStateContent(ctx context.Context, req *api.
 }
 
 // QuerySharedUsers returns a list of users who share at least 1 room in common with the given user.
-func (s *mockRoomserverAPI) QuerySharedUsers(ctx context.Context, req *api.QuerySharedUsersRequest, res *api.QuerySharedUsersResponse) error {
+func (s *keyChangeMockRoomserverAPI) QuerySharedUsers(ctx context.Context, req *api.QuerySharedUsersRequest, res *api.QuerySharedUsersResponse) error {
 	roomsToQuery := req.IncludeRoomIDs
 	for roomID, members := range s.roomIDToJoinedMembers {
 		exclude := false
@@ -123,7 +123,7 @@ func (s *mockRoomserverAPI) QuerySharedUsers(ctx context.Context, req *api.Query
 
 // This is actually a database function, but seeing as we track the state inside the
 // *mockRoomserverAPI, we'll just comply with the interface here instead.
-func (s *mockRoomserverAPI) SharedUsers(ctx context.Context, userID string, otherUserIDs []string) ([]string, error) {
+func (s *keyChangeMockRoomserverAPI) SharedUsers(ctx context.Context, userID string, otherUserIDs []string) ([]string, error) {
 	commonUsers := []string{}
 	for _, members := range s.roomIDToJoinedMembers {
 		for _, member := range members {
@@ -211,7 +211,7 @@ func TestKeyChangeCatchupOnJoinShareNewUser(t *testing.T) {
 	syncResponse := types.NewResponse()
 	syncResponse = joinResponseWithRooms(syncResponse, syncingUser, []string{newlyJoinedRoom})
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			newlyJoinedRoom: {syncingUser, newShareUser},
 			"!another:room": {syncingUser},
@@ -234,7 +234,7 @@ func TestKeyChangeCatchupOnLeaveShareLeftUser(t *testing.T) {
 	syncResponse := types.NewResponse()
 	syncResponse = leaveResponseWithRooms(syncResponse, syncingUser, []string{newlyLeftRoom})
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			newlyLeftRoom:   {removeUser},
 			"!another:room": {syncingUser},
@@ -257,7 +257,7 @@ func TestKeyChangeCatchupOnJoinShareNoNewUsers(t *testing.T) {
 	syncResponse := types.NewResponse()
 	syncResponse = joinResponseWithRooms(syncResponse, syncingUser, []string{newlyJoinedRoom})
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			newlyJoinedRoom: {syncingUser, existingUser},
 			"!another:room": {syncingUser, existingUser},
@@ -279,7 +279,7 @@ func TestKeyChangeCatchupOnLeaveShareNoUsers(t *testing.T) {
 	syncResponse := types.NewResponse()
 	syncResponse = leaveResponseWithRooms(syncResponse, syncingUser, []string{newlyLeftRoom})
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			newlyLeftRoom:   {existingUser},
 			"!another:room": {syncingUser, existingUser},
@@ -343,7 +343,7 @@ func TestKeyChangeCatchupNoNewJoinsButMessages(t *testing.T) {
 	jr.Timeline = &types.Timeline{Events: roomTimelineEvents}
 	syncResponse.Rooms.Join[roomID] = jr
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			roomID: {syncingUser, existingUser},
 		},
@@ -369,7 +369,7 @@ func TestKeyChangeCatchupChangeAndLeft(t *testing.T) {
 	syncResponse = joinResponseWithRooms(syncResponse, syncingUser, []string{newlyJoinedRoom})
 	syncResponse = leaveResponseWithRooms(syncResponse, syncingUser, []string{newlyLeftRoom})
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			newlyJoinedRoom: {syncingUser, newShareUser, newShareUser2},
 			newlyLeftRoom:   {newlyLeftUser, newlyLeftUser2},
@@ -459,7 +459,7 @@ func TestKeyChangeCatchupChangeAndLeftSameRoom(t *testing.T) {
 	lr.Timeline = &types.Timeline{Events: roomEvents}
 	syncResponse.Rooms.Leave[roomID] = lr
 
-	rsAPI := &mockRoomserverAPI{
+	rsAPI := &keyChangeMockRoomserverAPI{
 		roomIDToJoinedMembers: map[string][]string{
 			roomID:          {newShareUser, newShareUser2},
 			"!another:room": {syncingUser},
diff --git a/syncapi/synctypes/clientevent.go b/syncapi/synctypes/clientevent.go
index 6f03d9ff..a78aea1c 100644
--- a/syncapi/synctypes/clientevent.go
+++ b/syncapi/synctypes/clientevent.go
@@ -16,6 +16,8 @@
 package synctypes
 
 import (
+	"fmt"
+
 	"github.com/matrix-org/gomatrixserverlib"
 	"github.com/matrix-org/gomatrixserverlib/spec"
 )
@@ -118,3 +120,31 @@ func ToClientEventDefault(userIDQuery spec.UserIDForSender, event gomatrixserver
 	}
 	return ToClientEvent(event, FormatAll, sender, sk)
 }
+
+// If provided state key is a user ID (state keys beginning with @ are reserved for this purpose)
+// fetch it's associated sender ID and use that instead. Otherwise returns the same state key back.
+//
+// # This function either returns the state key that should be used, or an error
+//
+// TODO: handle failure cases better (e.g. no sender ID)
+func FromClientStateKey(roomID spec.RoomID, stateKey string, senderIDQuery spec.SenderIDForUser) (*string, error) {
+	if len(stateKey) >= 1 && stateKey[0] == '@' {
+		parsedStateKey, err := spec.NewUserID(stateKey, true)
+		if err != nil {
+			// If invalid user ID, then there is no associated state event.
+			return nil, fmt.Errorf("Provided state key begins with @ but is not a valid user ID: %s", err.Error())
+		}
+		senderID, err := senderIDQuery(roomID, *parsedStateKey)
+		if err != nil {
+			return nil, fmt.Errorf("Failed to query sender ID: %s", err.Error())
+		}
+		if senderID == nil {
+			// If no sender ID, then there is no associated state event.
+			return nil, fmt.Errorf("No associated sender ID found.")
+		}
+		newStateKey := string(*senderID)
+		return &newStateKey, nil
+	} else {
+		return &stateKey, nil
+	}
+}