From 56e980782b0c3adad0eeef0cf12f4b32e41c899b Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 4 May 2020 12:52:20 +0100 Subject: [PATCH] Review comments, make FS API take list of servernames, dedupe them, break out of loop properly on success --- federationsender/api/perform.go | 9 +- federationsender/internal/perform.go | 189 +++++++++++++++------------ federationsender/types/types.go | 6 + roomserver/internal/perform_join.go | 37 ++---- 4 files changed, 127 insertions(+), 114 deletions(-) diff --git a/federationsender/api/perform.go b/federationsender/api/perform.go index 4b90aa6f9..a7b12adc7 100644 --- a/federationsender/api/perform.go +++ b/federationsender/api/perform.go @@ -4,6 +4,7 @@ import ( "context" commonHTTP "github.com/matrix-org/dendrite/common/http" + "github.com/matrix-org/dendrite/federationsender/types" "github.com/matrix-org/gomatrixserverlib" "github.com/opentracing/opentracing-go" ) @@ -43,10 +44,10 @@ func (h *httpFederationSenderInternalAPI) PerformDirectoryLookup( } type PerformJoinRequest struct { - RoomID string `json:"room_id"` - UserID string `json:"user_id"` - ServerName gomatrixserverlib.ServerName `json:"server_name"` - Content map[string]interface{} `json:"content"` + RoomID string `json:"room_id"` + UserID string `json:"user_id"` + ServerNames types.ServerNames `json:"server_names"` + Content map[string]interface{} `json:"content"` } type PerformJoinResponse struct { diff --git a/federationsender/internal/perform.go b/federationsender/internal/perform.go index d77aca66c..62a50291e 100644 --- a/federationsender/internal/perform.go +++ b/federationsender/internal/perform.go @@ -9,6 +9,8 @@ import ( "github.com/matrix-org/dendrite/federationsender/internal/perform" "github.com/matrix-org/dendrite/roomserver/version" "github.com/matrix-org/gomatrixserverlib" + "github.com/matrix-org/util" + "github.com/sirupsen/logrus" ) // PerformLeaveRequest implements api.FederationSenderInternalAPI @@ -42,91 +44,114 @@ func (r *FederationSenderInternalAPI) PerformJoin( supportedVersions = append(supportedVersions, version) } - // Try to perform a make_join using the information supplied in the - // request. - respMakeJoin, err := r.federation.MakeJoin( - ctx, - request.ServerName, - request.RoomID, - request.UserID, - supportedVersions, - ) - if err != nil { - // TODO: Check if the user was not allowed to join the room. - return fmt.Errorf("r.federation.MakeJoin: %w", err) + // Deduplicate the server names we were provided. + util.Unique(request.ServerNames) + + // Try each server that we were provided until we land on one that + // successfully completes the make-join send-join dance. + succeeded := false + for _, serverName := range request.ServerNames { + // Try to perform a make_join using the information supplied in the + // request. + respMakeJoin, err := r.federation.MakeJoin( + ctx, + serverName, + request.RoomID, + request.UserID, + supportedVersions, + ) + if err != nil { + // TODO: Check if the user was not allowed to join the room. + return fmt.Errorf("r.federation.MakeJoin: %w", err) + } + + // Set all the fields to be what they should be, this should be a no-op + // but it's possible that the remote server returned us something "odd" + respMakeJoin.JoinEvent.Type = gomatrixserverlib.MRoomMember + respMakeJoin.JoinEvent.Sender = request.UserID + respMakeJoin.JoinEvent.StateKey = &request.UserID + respMakeJoin.JoinEvent.RoomID = request.RoomID + respMakeJoin.JoinEvent.Redacts = "" + if request.Content == nil { + request.Content = map[string]interface{}{} + } + request.Content["membership"] = "join" + if err = respMakeJoin.JoinEvent.SetContent(request.Content); err != nil { + return fmt.Errorf("respMakeJoin.JoinEvent.SetContent: %w", err) + } + if err = respMakeJoin.JoinEvent.SetUnsigned(struct{}{}); err != nil { + return fmt.Errorf("respMakeJoin.JoinEvent.SetUnsigned: %w", err) + } + + // Work out if we support the room version that has been supplied in + // the make_join response. + if respMakeJoin.RoomVersion == "" { + respMakeJoin.RoomVersion = gomatrixserverlib.RoomVersionV1 + } + if _, err = respMakeJoin.RoomVersion.EventFormat(); err != nil { + return fmt.Errorf("respMakeJoin.RoomVersion.EventFormat: %w", err) + } + + // Build the join event. + event, err := respMakeJoin.JoinEvent.Build( + time.Now(), + r.cfg.Matrix.ServerName, + r.cfg.Matrix.KeyID, + r.cfg.Matrix.PrivateKey, + respMakeJoin.RoomVersion, + ) + if err != nil { + return fmt.Errorf("respMakeJoin.JoinEvent.Build: %w", err) + } + + // Try to perform a send_join using the newly built event. + respSendJoin, err := r.federation.SendJoin( + ctx, + serverName, + event, + respMakeJoin.RoomVersion, + ) + if err != nil { + logrus.WithError(err).Warnf("r.federation.SendJoin failed") + continue + } + + // Check that the send_join response was valid. + joinCtx := perform.JoinContext(r.federation, r.keyRing) + if err = joinCtx.CheckSendJoinResponse( + ctx, event, serverName, respMakeJoin, respSendJoin, + ); err != nil { + logrus.WithError(err).Warnf("joinCtx.CheckSendJoinResponse failed") + continue + } + + // If we successfully performed a send_join above then the other + // server now thinks we're a part of the room. Send the newly + // returned state to the roomserver to update our local view. + if err = r.producer.SendEventWithState( + ctx, + respSendJoin.ToRespState(), + event.Headered(respMakeJoin.RoomVersion), + ); err != nil { + logrus.WithError(err).Warnf("r.producer.SendEventWithState failed") + continue + } + + // We're all good. + succeeded = true + break } - // Set all the fields to be what they should be, this should be a no-op - // but it's possible that the remote server returned us something "odd" - respMakeJoin.JoinEvent.Type = gomatrixserverlib.MRoomMember - respMakeJoin.JoinEvent.Sender = request.UserID - respMakeJoin.JoinEvent.StateKey = &request.UserID - respMakeJoin.JoinEvent.RoomID = request.RoomID - respMakeJoin.JoinEvent.Redacts = "" - if request.Content == nil { - request.Content = map[string]interface{}{} + if succeeded { + // Everything went to plan. + return nil + } else { + // We didn't complete a join for some reason. + return fmt.Errorf( + "failed to join user %q to room %q through %d server(s)", + request.UserID, request.RoomID, len(request.ServerNames), + ) } - request.Content["membership"] = "join" - if err = respMakeJoin.JoinEvent.SetContent(request.Content); err != nil { - return fmt.Errorf("respMakeJoin.JoinEvent.SetContent: %w", err) - } - if err = respMakeJoin.JoinEvent.SetUnsigned(struct{}{}); err != nil { - return fmt.Errorf("respMakeJoin.JoinEvent.SetUnsigned: %w", err) - } - - // Work out if we support the room version that has been supplied in - // the make_join response. - if respMakeJoin.RoomVersion == "" { - respMakeJoin.RoomVersion = gomatrixserverlib.RoomVersionV1 - } - if _, err = respMakeJoin.RoomVersion.EventFormat(); err != nil { - return fmt.Errorf("respMakeJoin.RoomVersion.EventFormat: %w", err) - } - - // Build the join event. - event, err := respMakeJoin.JoinEvent.Build( - time.Now(), - r.cfg.Matrix.ServerName, - r.cfg.Matrix.KeyID, - r.cfg.Matrix.PrivateKey, - respMakeJoin.RoomVersion, - ) - if err != nil { - return fmt.Errorf("respMakeJoin.JoinEvent.Build: %w", err) - } - - // Try to perform a send_join using the newly built event. - respSendJoin, err := r.federation.SendJoin( - ctx, - request.ServerName, - event, - respMakeJoin.RoomVersion, - ) - if err != nil { - return fmt.Errorf("r.federation.SendJoin: %w", err) - } - - // Check that the send_join response was valid. - joinCtx := perform.JoinContext(r.federation, r.keyRing) - if err = joinCtx.CheckSendJoinResponse( - ctx, event, request.ServerName, respMakeJoin, respSendJoin, - ); err != nil { - return fmt.Errorf("perform.JoinRequest.CheckSendJoinResponse: %w", err) - } - - // If we successfully performed a send_join above then the other - // server now thinks we're a part of the room. Send the newly - // returned state to the roomserver to update our local view. - if err = r.producer.SendEventWithState( - ctx, - respSendJoin.ToRespState(), - event.Headered(respMakeJoin.RoomVersion), - ); err != nil { - return fmt.Errorf("r.producer.SendEventWithState: %w", err) - } - - // Everything went to plan. - return nil } // PerformLeaveRequest implements api.FederationSenderInternalAPI diff --git a/federationsender/types/types.go b/federationsender/types/types.go index 05ba92f77..398d32677 100644 --- a/federationsender/types/types.go +++ b/federationsender/types/types.go @@ -28,6 +28,12 @@ type JoinedHost struct { ServerName gomatrixserverlib.ServerName } +type ServerNames []gomatrixserverlib.ServerName + +func (s ServerNames) Len() int { return len(s) } +func (s ServerNames) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +func (s ServerNames) Less(i, j int) bool { return s[i] < s[j] } + // A EventIDMismatchError indicates that we have got out of sync with the // room server. type EventIDMismatchError struct { diff --git a/roomserver/internal/perform_join.go b/roomserver/internal/perform_join.go index 4b75ddca4..3dfa118fd 100644 --- a/roomserver/internal/perform_join.go +++ b/roomserver/internal/perform_join.go @@ -179,35 +179,16 @@ func (r *RoomserverInternalAPI) performJoinRoomByID( } // Try joining by all of the supplied server names. - // TODO: Update the FS API so that it accepts a list of server names and - // does this bit by itself. - joined := false - for _, serverName := range req.ServerNames { - // Otherwise, if we've reached this point, the room isn't a local room - // and we should ask the federation sender to try and join for us. - fedReq := fsAPI.PerformJoinRequest{ - RoomID: req.RoomIDOrAlias, // the room ID to try and join - UserID: req.UserID, // the user ID joining the room - ServerName: serverName, // the server to try joining with - Content: req.Content, // the membership event content - } - fedRes := fsAPI.PerformJoinResponse{} - err = r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes) - if err != nil { - logrus.WithError(err).Errorf("error joining federated room %q", req.RoomIDOrAlias) - continue - } - joined = true - break + fedReq := fsAPI.PerformJoinRequest{ + RoomID: req.RoomIDOrAlias, // the room ID to try and join + UserID: req.UserID, // the user ID joining the room + ServerNames: req.ServerNames, // the server to try joining with + Content: req.Content, // the membership event content } - - // If we didn't successfully join the room using any of the supplied - // servers then return an error saying such. - if !joined { - return fmt.Errorf( - "Failed to join %q using %d server(s)", - req.RoomIDOrAlias, len(req.ServerNames), - ) + fedRes := fsAPI.PerformJoinResponse{} + err = r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes) + if err != nil { + return fmt.Errorf("Error joining federated room: %q", err) } default: