From 3c1474f68f2ac5e564d2bd4bd15f01d2a73f2845 Mon Sep 17 00:00:00 2001 From: Till <2353100+S7evinK@users.noreply.github.com> Date: Tue, 11 Oct 2022 16:04:02 +0200 Subject: [PATCH] Fix `/get_missing_events` for rooms with `joined`/`invited` history_visibility (#2787) Sytest was using a wrong `history_visibility` for `invited` (https://github.com/matrix-org/sytest/pull/1303), so `invited` was passing for the wrong reason (-> defaulted to `shared`, as `invite` wasn't understood). This change now handles missing events like Synapse, if a server isn't allowed to see the event, it gets a redacted version of it, making the `get_missing_events` tests pass. --- are-we-synapse-yet.list | 2 +- roomserver/auth/auth.go | 30 +++++++------------ roomserver/internal/helpers/helpers.go | 15 +++++----- .../internal/perform/perform_backfill.go | 5 +++- roomserver/internal/query/query.go | 6 ++-- .../storage/postgres/state_snapshot_table.go | 4 ++- sytest-whitelist | 5 ++-- 7 files changed, 33 insertions(+), 34 deletions(-) diff --git a/are-we-synapse-yet.list b/are-we-synapse-yet.list index c776a7400..81c0f8049 100644 --- a/are-we-synapse-yet.list +++ b/are-we-synapse-yet.list @@ -643,7 +643,7 @@ fed Inbound federation redacts events from erased users fme Outbound federation can request missing events fme Inbound federation can return missing events for world_readable visibility fme Inbound federation can return missing events for shared visibility -fme Inbound federation can return missing events for invite visibility +fme Inbound federation can return missing events for invited visibility fme Inbound federation can return missing events for joined visibility fme outliers whose auth_events are in a different room are correctly rejected fbk Outbound federation can backfill events diff --git a/roomserver/auth/auth.go b/roomserver/auth/auth.go index aa1d5bc25..31a856e8e 100644 --- a/roomserver/auth/auth.go +++ b/roomserver/auth/auth.go @@ -13,8 +13,6 @@ package auth import ( - "encoding/json" - "github.com/matrix-org/gomatrixserverlib" ) @@ -30,7 +28,7 @@ func IsServerAllowed( historyVisibility := HistoryVisibilityForRoom(authEvents) // 1. If the history_visibility was set to world_readable, allow. - if historyVisibility == "world_readable" { + if historyVisibility == gomatrixserverlib.HistoryVisibilityWorldReadable { return true } // 2. If the user's membership was join, allow. @@ -39,12 +37,12 @@ func IsServerAllowed( return true } // 3. If history_visibility was set to shared, and the user joined the room at any point after the event was sent, allow. - if historyVisibility == "shared" && serverCurrentlyInRoom { + if historyVisibility == gomatrixserverlib.HistoryVisibilityShared && serverCurrentlyInRoom { return true } // 4. If the user's membership was invite, and the history_visibility was set to invited, allow. invitedUserExists := IsAnyUserOnServerWithMembership(serverName, authEvents, gomatrixserverlib.Invite) - if invitedUserExists && historyVisibility == "invited" { + if invitedUserExists && historyVisibility == gomatrixserverlib.HistoryVisibilityInvited { return true } @@ -52,27 +50,16 @@ func IsServerAllowed( return false } -func HistoryVisibilityForRoom(authEvents []*gomatrixserverlib.Event) string { +func HistoryVisibilityForRoom(authEvents []*gomatrixserverlib.Event) gomatrixserverlib.HistoryVisibility { // https://matrix.org/docs/spec/client_server/r0.6.0#id87 // By default if no history_visibility is set, or if the value is not understood, the visibility is assumed to be shared. - visibility := "shared" - knownStates := []string{"invited", "joined", "shared", "world_readable"} + visibility := gomatrixserverlib.HistoryVisibilityShared for _, ev := range authEvents { if ev.Type() != gomatrixserverlib.MRoomHistoryVisibility { continue } - // TODO: This should be HistoryVisibilityContent to match things like 'MemberContent'. Do this when moving to GMSL - content := struct { - HistoryVisibility string `json:"history_visibility"` - }{} - if err := json.Unmarshal(ev.Content(), &content); err != nil { - break // value is not understood - } - for _, s := range knownStates { - if s == content.HistoryVisibility { - visibility = s - break - } + if vis, err := ev.HistoryVisibility(); err == nil { + visibility = vis } } return visibility @@ -80,6 +67,9 @@ func HistoryVisibilityForRoom(authEvents []*gomatrixserverlib.Event) string { func IsAnyUserOnServerWithMembership(serverName gomatrixserverlib.ServerName, authEvents []*gomatrixserverlib.Event, wantMembership string) bool { for _, ev := range authEvents { + if ev.Type() != gomatrixserverlib.MRoomMember { + continue + } membership, err := ev.Membership() if err != nil || membership != wantMembership { continue diff --git a/roomserver/internal/helpers/helpers.go b/roomserver/internal/helpers/helpers.go index 3b83a0a6d..a6de8ac84 100644 --- a/roomserver/internal/helpers/helpers.go +++ b/roomserver/internal/helpers/helpers.go @@ -324,7 +324,7 @@ func slowGetHistoryVisibilityState( func ScanEventTree( ctx context.Context, db storage.Database, info *types.RoomInfo, front []string, visited map[string]bool, limit int, serverName gomatrixserverlib.ServerName, -) ([]types.EventNID, error) { +) ([]types.EventNID, map[string]struct{}, error) { var resultNIDs []types.EventNID var err error var allowed bool @@ -345,6 +345,7 @@ func ScanEventTree( var checkedServerInRoom bool var isServerInRoom bool + redactEventIDs := make(map[string]struct{}) // Loop through the event IDs to retrieve the requested events and go // through the whole tree (up to the provided limit) using the events' @@ -358,7 +359,7 @@ BFSLoop: // Retrieve the events to process from the database. events, err = db.EventsFromIDs(ctx, front) if err != nil { - return resultNIDs, err + return resultNIDs, redactEventIDs, err } if !checkedServerInRoom && len(events) > 0 { @@ -395,16 +396,16 @@ BFSLoop: ) // drop the error, as we will often error at the DB level if we don't have the prev_event itself. Let's // just return what we have. - return resultNIDs, nil + return resultNIDs, redactEventIDs, nil } // If the event hasn't been seen before and the HS // requesting to retrieve it is allowed to do so, add it to // the list of events to retrieve. - if allowed { - next = append(next, pre) - } else { + next = append(next, pre) + if !allowed { util.GetLogger(ctx).WithField("server", serverName).WithField("event_id", pre).Info("Not allowed to see event") + redactEventIDs[pre] = struct{}{} } } } @@ -413,7 +414,7 @@ BFSLoop: front = next } - return resultNIDs, err + return resultNIDs, redactEventIDs, err } func QueryLatestEventsAndState( diff --git a/roomserver/internal/perform/perform_backfill.go b/roomserver/internal/perform/perform_backfill.go index 69a075733..57e121ea2 100644 --- a/roomserver/internal/perform/perform_backfill.go +++ b/roomserver/internal/perform/perform_backfill.go @@ -78,7 +78,7 @@ func (r *Backfiller) PerformBackfill( } // Scan the event tree for events to send back. - resultNIDs, err := helpers.ScanEventTree(ctx, r.DB, info, front, visited, request.Limit, request.ServerName) + resultNIDs, redactEventIDs, err := helpers.ScanEventTree(ctx, r.DB, info, front, visited, request.Limit, request.ServerName) if err != nil { return err } @@ -95,6 +95,9 @@ func (r *Backfiller) PerformBackfill( } for _, event := range loadedEvents { + if _, ok := redactEventIDs[event.EventID()]; ok { + event.Redact() + } response.Events = append(response.Events, event.Headered(info.RoomVersion)) } diff --git a/roomserver/internal/query/query.go b/roomserver/internal/query/query.go index 7a424a334..f41132403 100644 --- a/roomserver/internal/query/query.go +++ b/roomserver/internal/query/query.go @@ -453,7 +453,7 @@ func (r *Queryer) QueryMissingEvents( return fmt.Errorf("missing RoomInfo for room %s", events[0].RoomID()) } - resultNIDs, err := helpers.ScanEventTree(ctx, r.DB, info, front, visited, request.Limit, request.ServerName) + resultNIDs, redactEventIDs, err := helpers.ScanEventTree(ctx, r.DB, info, front, visited, request.Limit, request.ServerName) if err != nil { return err } @@ -470,7 +470,9 @@ func (r *Queryer) QueryMissingEvents( if verr != nil { return verr } - + if _, ok := redactEventIDs[event.EventID()]; ok { + event.Redact() + } response.Events = append(response.Events, event.Headered(roomVersion)) } } diff --git a/roomserver/storage/postgres/state_snapshot_table.go b/roomserver/storage/postgres/state_snapshot_table.go index 99c76befe..a00c026f4 100644 --- a/roomserver/storage/postgres/state_snapshot_table.go +++ b/roomserver/storage/postgres/state_snapshot_table.go @@ -21,10 +21,11 @@ import ( "fmt" "github.com/lib/pq" + "github.com/matrix-org/util" + "github.com/matrix-org/dendrite/internal/sqlutil" "github.com/matrix-org/dendrite/roomserver/storage/tables" "github.com/matrix-org/dendrite/roomserver/types" - "github.com/matrix-org/util" ) const stateSnapshotSchema = ` @@ -91,6 +92,7 @@ const bulkSelectStateForHistoryVisibilitySQL = ` WHERE state_snapshot_nid = $1 ) ) + ORDER BY depth ASC ) AS roomserver_events INNER JOIN roomserver_event_state_keys ON roomserver_events.event_state_key_nid = roomserver_event_state_keys.event_state_key_nid diff --git a/sytest-whitelist b/sytest-whitelist index 9ba9df75d..a3218ed70 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -306,7 +306,7 @@ Alternative server names do not cause a routing loop Events whose auth_events are in the wrong room do not mess up the room state Inbound federation can return events Inbound federation can return missing events for world_readable visibility -Inbound federation can return missing events for invite visibility +Inbound federation can return missing events for invited visibility Inbound federation can get public room list PUT /rooms/:room_id/redact/:event_id/:txn_id as power user redacts message PUT /rooms/:room_id/redact/:event_id/:txn_id as original message sender redacts message @@ -742,4 +742,5 @@ User in private room doesn't appear in user directory User joining then leaving public room appears and dissappears from directory User in remote room doesn't appear in user directory after server left room User in shared private room does appear in user directory until leave -Existing members see new member's presence \ No newline at end of file +Existing members see new member's presence +Inbound federation can return missing events for joined visibility \ No newline at end of file