From 600fbae31f1b0b6919bb9045958b932071de6737 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 22 Feb 2022 13:35:06 +0000 Subject: [PATCH 1/5] Only emit key change notifications from federation when changes are made (#2217) * Only emit key changes when poked over federation * Remove logging * Fix unit test possibly --- keyserver/internal/device_list_update.go | 27 ++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/keyserver/internal/device_list_update.go b/keyserver/internal/device_list_update.go index c5a5d40c7..b208f0ce5 100644 --- a/keyserver/internal/device_list_update.go +++ b/keyserver/internal/device_list_update.go @@ -241,14 +241,33 @@ func (u *DeviceListUpdater) update(ctx context.Context, event gomatrixserverlib. StreamID: event.StreamID, }, } + + // DeviceKeysJSON will side-effect modify this, so it needs + // to be a copy, not sharing any pointers with the above. + deviceKeysCopy := *keys[0].DeviceKeys + deviceKeysCopy.KeyJSON = nil + existingKeys := []api.DeviceMessage{ + { + Type: keys[0].Type, + DeviceKeys: &deviceKeysCopy, + StreamID: keys[0].StreamID, + }, + } + + // fetch what keys we had already and only emit changes + if err = u.db.DeviceKeysJSON(ctx, existingKeys); err != nil { + // non-fatal, log and continue + util.GetLogger(ctx).WithError(err).WithField("user_id", event.UserID).Errorf( + "failed to query device keys json for calculating diffs", + ) + } + err = u.db.StoreRemoteDeviceKeys(ctx, keys, nil) if err != nil { return false, fmt.Errorf("failed to store remote device keys for %s (%s): %w", event.UserID, event.DeviceID, err) } - // ALWAYS emit key changes when we've been poked over federation even if there's no change - // just in case this poke is important for something. - err = u.producer.ProduceKeyChanges(keys) - if err != nil { + + if err = emitDeviceKeyChanges(u.producer, existingKeys, keys); err != nil { return false, fmt.Errorf("failed to produce device key changes for %s (%s): %w", event.UserID, event.DeviceID, err) } return false, nil From e9545dc12fc699b237f51c1aa67dbbef898b27ce Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 22 Feb 2022 13:40:08 +0000 Subject: [PATCH 2/5] Remove error when state keys are missing for user NIDs (#2213) * Remove error when state keys are missing for user NIDs There is still an actual bug here somewhere in the membership updater, but this check does more harm than good, since it means that the key consumers don't actually distribute updates to *anyone*. It's better just to deal with this silently for now. To find these broken rows: ``` SELECT * FROM roomserver_membership AS m WHERE NOT EXISTS ( SELECT event_state_key_nid FROM roomserver_event_state_keys AS s WHERE m.sender_nid = s.event_state_key_nid ); ``` * Logging --- roomserver/storage/shared/storage.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/roomserver/storage/shared/storage.go b/roomserver/storage/shared/storage.go index b255cfb3f..e270e121c 100644 --- a/roomserver/storage/shared/storage.go +++ b/roomserver/storage/shared/storage.go @@ -13,6 +13,7 @@ import ( "github.com/matrix-org/dendrite/roomserver/types" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" + "github.com/sirupsen/logrus" "github.com/tidwall/gjson" ) @@ -1101,7 +1102,7 @@ func (d *Database) JoinedUsersSetInRooms(ctx context.Context, roomIDs []string) return nil, err } if len(nidToUserID) != len(userNIDToCount) { - return nil, fmt.Errorf("found %d users but only have state key nids for %d of them", len(userNIDToCount), len(nidToUserID)) + logrus.Warnf("SelectJoinedUsersSetForRooms found %d users but BulkSelectEventStateKey only returned state key NIDs for %d of them", len(userNIDToCount), len(nidToUserID)) } result := make(map[string]int, len(userNIDToCount)) for nid, count := range userNIDToCount { From 34116178e8ef75569a55655e3001a51f77dfb789 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 22 Feb 2022 13:47:14 +0000 Subject: [PATCH 3/5] Remove logging line in `PerformInvite` --- federationapi/internal/perform.go | 1 - 1 file changed, 1 deletion(-) diff --git a/federationapi/internal/perform.go b/federationapi/internal/perform.go index c51ecf146..b888b3654 100644 --- a/federationapi/internal/perform.go +++ b/federationapi/internal/perform.go @@ -554,7 +554,6 @@ func (r *FederationInternalAPI) PerformInvite( if err != nil { return fmt.Errorf("r.federation.SendInviteV2: failed to send invite: %w", err) } - logrus.Infof("GOT INVITE RESPONSE %s", string(inviteRes.Event)) inviteEvent, err := inviteRes.Event.UntrustedEvent(request.RoomVersion) if err != nil { From c7811e9d71041527128f4a1782e2b92453f0f57c Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Tue, 22 Feb 2022 15:43:17 +0000 Subject: [PATCH 4/5] Add `DeviceKeysEqual` (#2219) * Add `DeviceKeysEqual` * Update check order * Fix check * Tweak conditions again * One more time * Single return value --- keyserver/api/api.go | 21 +++++++++++++++++++++ keyserver/internal/internal.go | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/keyserver/api/api.go b/keyserver/api/api.go index 3933961c1..54eb04f8a 100644 --- a/keyserver/api/api.go +++ b/keyserver/api/api.go @@ -15,6 +15,7 @@ package api import ( + "bytes" "context" "encoding/json" "strings" @@ -73,6 +74,26 @@ type DeviceMessage struct { DeviceChangeID int64 } +// DeviceKeysEqual returns true if the device keys updates contain the +// same display name and key JSON. This will return false if either of +// the updates is not a device keys update, or if the user ID/device ID +// differ between the two. +func (m1 *DeviceMessage) DeviceKeysEqual(m2 *DeviceMessage) bool { + if m1.DeviceKeys == nil || m2.DeviceKeys == nil { + return false + } + if m1.UserID != m2.UserID || m1.DeviceID != m2.DeviceID { + return false + } + if m1.DisplayName != m2.DisplayName { + return false // different display names + } + if len(m1.KeyJSON) == 0 || len(m2.KeyJSON) == 0 { + return false // either is empty + } + return bytes.Equal(m1.KeyJSON, m2.KeyJSON) +} + // DeviceKeys represents a set of device keys for a single device // https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-keys-upload type DeviceKeys struct { diff --git a/keyserver/internal/internal.go b/keyserver/internal/internal.go index 0c264b718..dc3c404bd 100644 --- a/keyserver/internal/internal.go +++ b/keyserver/internal/internal.go @@ -718,7 +718,7 @@ func emitDeviceKeyChanges(producer KeyChangeProducer, existing, new []api.Device for _, existingKey := range existing { // Do not treat the absence of keys as equal, or else we will not emit key changes // when users delete devices which never had a key to begin with as both KeyJSONs are nil. - if bytes.Equal(existingKey.KeyJSON, newKey.KeyJSON) && len(existingKey.KeyJSON) > 0 { + if existingKey.DeviceKeysEqual(&newKey) { exists = true break } From cfff1b0aaafc68efbac27f366360667983f7696a Mon Sep 17 00:00:00 2001 From: S7evinK <2353100+S7evinK@users.noreply.github.com> Date: Tue, 22 Feb 2022 16:50:04 +0100 Subject: [PATCH 5/5] Remote banned user is kicked and may not rejoin until unbanned (#2216) * Remote banned user is kicked and may not rejoin until unbanned * Use gmsl constant --- clientapi/routing/state.go | 2 +- sytest-whitelist | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/clientapi/routing/state.go b/clientapi/routing/state.go index 088e412c6..d25ee8237 100644 --- a/clientapi/routing/state.go +++ b/clientapi/routing/state.go @@ -235,7 +235,7 @@ func OnIncomingStateTypeRequest( } // If the user has never been in the room then stop at this point. // We won't tell the user about a room they have never joined. - if !membershipRes.HasBeenInRoom { + if !membershipRes.HasBeenInRoom || membershipRes.Membership == gomatrixserverlib.Ban { return util.JSONResponse{ Code: http.StatusForbidden, JSON: jsonerror.Forbidden(fmt.Sprintf("Unknown room %q or user %q has never joined this room", roomID, device.UserID)), diff --git a/sytest-whitelist b/sytest-whitelist index 187a0f475..87904938c 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -596,4 +596,5 @@ Device list doesn't change if remote server is down /context/ on joined room works /context/ on non world readable room does not work /context/ returns correct number of events -/context/ with lazy_load_members filter works \ No newline at end of file +/context/ with lazy_load_members filter works +Remote banned user is kicked and may not rejoin until unbanned