Return remote errors from FS.PerformJoin (#1164)

* Return remote errors from FS.PerformJoin

Follows the same pattern as PerformJoin on roomserver (no error return).

Also return the right format for incompatible room version errors.

Makes a bunch of tests pass!

* Handle network errors better when returning remote HTTP errors

* Linting

* Fix tests

* Update whitelist, pass network errors through in API=1 mode

clientapi/jsonerror/jsonerror.go

@@ -125,10 +125,20 @@ func GuestAccessForbidden(msg string) *MatrixError {
 	return &MatrixError{"M_GUEST_ACCESS_FORBIDDEN", msg}
 }
 
+type IncompatibleRoomVersionError struct {
+	RoomVersion string `json:"room_version"`
+	Error       string `json:"error"`
+	Code        string `json:"errcode"`
+}
+
 // IncompatibleRoomVersion is an error which is returned when the client
 // requests a room with a version that is unsupported.
-func IncompatibleRoomVersion(roomVersion gomatrixserverlib.RoomVersion) *MatrixError {
+func IncompatibleRoomVersion(roomVersion gomatrixserverlib.RoomVersion) *IncompatibleRoomVersionError {
-	return &MatrixError{"M_INCOMPATIBLE_ROOM_VERSION", string(roomVersion)}
+	return &IncompatibleRoomVersionError{
+		Code:        "M_INCOMPATIBLE_ROOM_VERSION",
+		RoomVersion: string(roomVersion),
+		Error:       "Your homeserver does not support the features required to join this room",
+	}
 }
 
 // UnsupportedRoomVersion is an error which is returned when the client

federationsender/api/api.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/matrix-org/dendrite/federationsender/types"
+	"github.com/matrix-org/gomatrix"
 	"github.com/matrix-org/gomatrixserverlib"
 )
 
@@ -28,7 +29,7 @@ type FederationSenderInternalAPI interface {
 		ctx context.Context,
 		request *PerformJoinRequest,
 		response *PerformJoinResponse,
-	) error
+	)
 	// Handle an instruction to make_leave & send_leave with a remote server.
 	PerformLeave(
 		ctx context.Context,
@@ -62,6 +63,7 @@ type PerformJoinRequest struct {
 }
 
 type PerformJoinResponse struct {
+	LastError *gomatrix.HTTPError
 }
 
 type PerformLeaveRequest struct {

federationsender/internal/perform.go

@@ -2,6 +2,7 @@ package internal
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -9,6 +10,7 @@ import (
 	"github.com/matrix-org/dendrite/federationsender/internal/perform"
 	roomserverAPI "github.com/matrix-org/dendrite/roomserver/api"
 	"github.com/matrix-org/dendrite/roomserver/version"
+	"github.com/matrix-org/gomatrix"
 	"github.com/matrix-org/gomatrixserverlib"
 	"github.com/matrix-org/util"
 	"github.com/sirupsen/logrus"
@@ -40,7 +42,7 @@ func (r *FederationSenderInternalAPI) PerformJoin(
 	ctx context.Context,
 	request *api.PerformJoinRequest,
 	response *api.PerformJoinResponse,
-) (err error) {
+) {
 	// Look up the supported room versions.
 	var supportedVersions []gomatrixserverlib.RoomVersion
 	for version := range version.SupportedRoomVersions() {
@@ -63,6 +65,7 @@ func (r *FederationSenderInternalAPI) PerformJoin(
 
 	// Try each server that we were provided until we land on one that
 	// successfully completes the make-join send-join dance.
+	var lastErr error
 	for _, serverName := range request.ServerNames {
 		if err := r.performJoinUsingServer(
 			ctx,
@@ -76,17 +79,32 @@ func (r *FederationSenderInternalAPI) PerformJoin(
 				"server_name": serverName,
 				"room_id":     request.RoomID,
 			}).Warnf("Failed to join room through server")
+			lastErr = err
 			continue
 		}
 
 		// We're all good.
-		return nil
+		return
 	}
 
 	// If we reach here then we didn't complete a join for some reason.
-	return fmt.Errorf(
+	var httpErr gomatrix.HTTPError
-		"failed to join user %q to room %q through %d server(s)",
+	if ok := errors.As(lastErr, &httpErr); ok {
-		request.UserID, request.RoomID, len(request.ServerNames),
+		httpErr.Message = string(httpErr.Contents)
+		// Clear the wrapped error, else serialising to JSON (in polylith mode) will fail
+		httpErr.WrappedError = nil
+		response.LastError = &httpErr
+	} else {
+		response.LastError = &gomatrix.HTTPError{
+			Code:         0,
+			WrappedError: nil,
+			Message:      lastErr.Error(),
+		}
+	}
+
+	logrus.Errorf(
+		"failed to join user %q to room %q through %d server(s): last error %s",
+		request.UserID, request.RoomID, len(request.ServerNames), lastErr,
 	)
 }
 

federationsender/inthttp/client.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/matrix-org/dendrite/federationsender/api"
 	"github.com/matrix-org/dendrite/internal/httputil"
+	"github.com/matrix-org/gomatrix"
 	"github.com/opentracing/opentracing-go"
 )
 
@@ -77,12 +78,19 @@ func (h *httpFederationSenderInternalAPI) PerformJoin(
 	ctx context.Context,
 	request *api.PerformJoinRequest,
 	response *api.PerformJoinResponse,
-) error {
+) {
 	span, ctx := opentracing.StartSpanFromContext(ctx, "PerformJoinRequest")
 	defer span.Finish()
 
 	apiURL := h.federationSenderURL + FederationSenderPerformJoinRequestPath
-	return httputil.PostJSON(ctx, span, h.httpClient, apiURL, request, response)
+	err := httputil.PostJSON(ctx, span, h.httpClient, apiURL, request, response)
+	if err != nil {
+		response.LastError = &gomatrix.HTTPError{
+			Message:      err.Error(),
+			Code:         0,
+			WrappedError: err,
+		}
+	}
 }
 
 // Handle an instruction to make_join & send_join with a remote server.

federationsender/inthttp/server.go

@@ -33,9 +33,7 @@ func AddRoutes(intAPI api.FederationSenderInternalAPI, internalAPIMux *mux.Route
 			if err := json.NewDecoder(req.Body).Decode(&request); err != nil {
 				return util.MessageResponse(http.StatusBadRequest, err.Error())
 			}
-			if err := intAPI.PerformJoin(req.Context(), &request, &response); err != nil {
+			intAPI.PerformJoin(req.Context(), &request, &response)
-				return util.ErrorResponse(err)
-			}
 			return util.JSONResponse{Code: http.StatusOK, JSON: &response}
 		}),
 	)

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/matrix-org/go-http-js-libp2p v0.0.0-20200518170932-783164aeeda4
 	github.com/matrix-org/go-sqlite3-js v0.0.0-20200522092705-bc8506ccbcf3
 	github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26
-	github.com/matrix-org/gomatrixserverlib v0.0.0-20200623103809-13ff8109e137
+	github.com/matrix-org/gomatrixserverlib v0.0.0-20200625121044-e5d892cd30c1
 	github.com/matrix-org/naffka v0.0.0-20200422140631-181f1ee7401f
 	github.com/matrix-org/util v0.0.0-20190711121626-527ce5ddefc7
 	github.com/mattn/go-sqlite3 v2.0.2+incompatible

go.sum

@@ -371,8 +371,8 @@ github.com/matrix-org/go-sqlite3-js v0.0.0-20200522092705-bc8506ccbcf3 h1:Yb+Wlf
 github.com/matrix-org/go-sqlite3-js v0.0.0-20200522092705-bc8506ccbcf3/go.mod h1:e+cg2q7C7yE5QnAXgzo512tgFh1RbQLC0+jozuegKgo=
 github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26 h1:Hr3zjRsq2bhrnp3Ky1qgx/fzCtCALOoGYylh2tpS9K4=
 github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26/go.mod h1:3fxX6gUjWyI/2Bt7J1OLhpCzOfO/bB3AiX0cJtEKud0=
-github.com/matrix-org/gomatrixserverlib v0.0.0-20200623103809-13ff8109e137 h1:+eBh4L04+08IslvFM071TNrQTggU317GsQKzZ1SGEVo=
+github.com/matrix-org/gomatrixserverlib v0.0.0-20200625121044-e5d892cd30c1 h1:3yS6hw01X72jpJuAPGVOY+QFD9cpAETR/6Hq2WYKbpU=
-github.com/matrix-org/gomatrixserverlib v0.0.0-20200623103809-13ff8109e137/go.mod h1:JsAzE1Ll3+gDWS9JSUHPJiiyAksvOOnGWF2nXdg4ZzU=
+github.com/matrix-org/gomatrixserverlib v0.0.0-20200625121044-e5d892cd30c1/go.mod h1:JsAzE1Ll3+gDWS9JSUHPJiiyAksvOOnGWF2nXdg4ZzU=
 github.com/matrix-org/naffka v0.0.0-20200422140631-181f1ee7401f h1:pRz4VTiRCO4zPlEMc3ESdUOcW4PXHH4Kj+YDz1XyE+Y=
 github.com/matrix-org/naffka v0.0.0-20200422140631-181f1ee7401f/go.mod h1:y0oDTjZDv5SM9a2rp3bl+CU+bvTRINQsdb7YlDql5Go=
 github.com/matrix-org/util v0.0.0-20190711121626-527ce5ddefc7 h1:ntrLa/8xVzeSs8vHFHK25k0C+NV74sYMJnNSg5NoSRo=

roomserver/api/perform.go

@@ -1,6 +1,7 @@
 package api
 
 import (
+	"encoding/json"
 	"fmt"
 	"net/http"
 
@@ -13,6 +14,7 @@ type PerformErrorCode int
 
 type PerformError struct {
 	Msg        string
+	RemoteCode int // remote HTTP status code, for PerformErrRemote
 	Code       PerformErrorCode
 }
 
@@ -43,6 +45,17 @@ func (p *PerformError) JSONResponse() util.JSONResponse {
 			Code: http.StatusForbidden,
 			JSON: jsonerror.Forbidden(p.Msg),
 		}
+	case PerformErrRemote:
+		// if the code is 0 then something bad happened and it isn't
+		// a remote HTTP error being encapsulated, e.g network error to remote.
+		if p.RemoteCode == 0 {
+			return util.ErrorResponse(fmt.Errorf("%s", p.Msg))
+		}
+		return util.JSONResponse{
+			Code: p.RemoteCode,
+			// TODO: Should we assert this is in fact JSON? E.g gjson parse?
+			JSON: json.RawMessage(p.Msg),
+		}
 	default:
 		return util.ErrorResponse(p)
 	}
@@ -57,6 +70,8 @@ const (
 	PerformErrorNoRoom PerformErrorCode = 3
 	// PerformErrorNoOperation means that the request resulted in nothing happening e.g invite->invite or leave->leave.
 	PerformErrorNoOperation PerformErrorCode = 4
+	// PerformErrRemote means that the request failed and the PerformError.Msg is the raw remote JSON error response
+	PerformErrRemote PerformErrorCode = 5
 )
 
 type PerformJoinRequest struct {

roomserver/internal/perform_join.go

@@ -270,9 +270,13 @@ func (r *RoomserverInternalAPI) performFederatedJoinRoomByID(
 		Content:     req.Content,       // the membership event content
 	}
 	fedRes := fsAPI.PerformJoinResponse{}
-	if err := r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes); err != nil {
+	r.fsAPI.PerformJoin(ctx, &fedReq, &fedRes)
-		return fmt.Errorf("Error joining federated room: %q", err)
+	if fedRes.LastError != nil {
+		return &api.PerformError{
+			Code:       api.PerformErrRemote,
+			Msg:        fedRes.LastError.Message,
+			RemoteCode: fedRes.LastError.Code,
+		}
 	}
-
 	return nil
 }

sytest-whitelist

@@ -366,3 +366,10 @@ Typing notification sent to local room members
 Typing notifications also sent to remote room members
 Typing can be explicitly stopped
 Banned user is kicked and may not rejoin until unbanned
+Inbound federation rejects attempts to join v1 rooms from servers without v1 support
+Inbound federation rejects attempts to join v2 rooms from servers lacking version support
+Inbound federation rejects attempts to join v2 rooms from servers only supporting v1
+Outbound federation passes make_join failures through to the client
+Outbound federation correctly handles unsupported room versions
+Remote users may not join unfederated rooms
+Guest users denied access over federation if guest access prohibited