Propagate error with wrong ?ts= param back to client (#576)

* make MatrixError implement error interface

* let ParseTSParam return error when no int transmitted

* fix to high cyclo for SendEvent

* Move generateSendEvent below SendEvent

* Drop ParseIntParam() as it is used only in one place

* Parse ts param at the beginning of JoinRoom

to be able to abort right in the beginning and
to not parse the MatrixError to get an error response

* make ParseTSParam() return error instead of JSONResponse

src/github.com/matrix-org/dendrite/clientapi/httputil/parse.go

@@ -13,6 +13,7 @@
 package httputil
 
 import (
+	"fmt"
 	"net/http"
 	"strconv"
 	"time"
@@ -21,18 +22,18 @@ import (
 // ParseTSParam takes a req (typically from an application service) and parses a Time object
 // from the req if it exists in the query parameters. If it doesn't exist, the
 // current time is returned.
-func ParseTSParam(req *http.Request) time.Time {
+func ParseTSParam(req *http.Request) (time.Time, error) {
 	// Use the ts parameter's value for event time if present
 	tsStr := req.URL.Query().Get("ts")
 	if tsStr == "" {
-		return time.Now()
+		return time.Now(), nil
 	}
 
 	// The parameter exists, parse into a Time object
 	ts, err := strconv.ParseInt(tsStr, 10, 64)
 	if err != nil {
-		return time.Unix(ts/1000, 0)
+		return time.Time{}, fmt.Errorf("Param 'ts' is no valid int (%s)", err.Error())
 	}
 
-	return time.Unix(ts/1000, 0)
+	return time.Unix(ts/1000, 0), nil
 }

src/github.com/matrix-org/dendrite/clientapi/jsonerror/jsonerror.go

@@ -28,7 +28,7 @@ type MatrixError struct {
 	Err     string `json:"error"`
 }
 
-func (e *MatrixError) Error() string {
+func (e MatrixError) Error() string {
 	return fmt.Sprintf("%s: %s", e.ErrCode, e.Err)
 }
 

src/github.com/matrix-org/dendrite/clientapi/routing/createroom.go

@@ -147,7 +147,13 @@ func createRoom(
 		return *resErr
 	}
 
-	evTime := httputil.ParseTSParam(req)
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
 	// TODO: visibility/presets/raw initial state/creation content
 	// TODO: Create room alias association
 	// Make sure this doesn't fall into an application service's namespace though!

src/github.com/matrix-org/dendrite/clientapi/routing/joinroom.go

@@ -18,6 +18,7 @@ import (
 	"fmt"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/matrix-org/dendrite/clientapi/auth/authtypes"
 	"github.com/matrix-org/dendrite/clientapi/auth/storage/accounts"
@@ -51,6 +52,14 @@ func JoinRoomByIDOrAlias(
 		return *resErr
 	}
 
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
+
 	localpart, _, err := gomatrixserverlib.SplitID('@', device.UserID)
 	if err != nil {
 		return httputil.LogThenError(req, err)
@@ -65,7 +74,9 @@ func JoinRoomByIDOrAlias(
 	content["displayname"] = profile.DisplayName
 	content["avatar_url"] = profile.AvatarURL
 
-	r := joinRoomReq{req, content, device.UserID, cfg, federation, producer, queryAPI, aliasAPI, keyRing}
+	r := joinRoomReq{
+		req, evTime, content, device.UserID, cfg, federation, producer, queryAPI, aliasAPI, keyRing,
+	}
 
 	if strings.HasPrefix(roomIDOrAlias, "!") {
 		return r.joinRoomByID(roomIDOrAlias)
@@ -81,6 +92,7 @@ func JoinRoomByIDOrAlias(
 
 type joinRoomReq struct {
 	req        *http.Request
+	evTime     time.Time
 	content    map[string]interface{}
 	userID     string
 	cfg        config.Dendrite
@@ -213,9 +225,8 @@ func (r joinRoomReq) joinRoomUsingServers(
 		return httputil.LogThenError(r.req, err)
 	}
 
-	evTime := httputil.ParseTSParam(r.req)
 	var queryRes roomserverAPI.QueryLatestEventsAndStateResponse
-	event, err := common.BuildEvent(r.req.Context(), &eb, r.cfg, evTime, r.queryAPI, &queryRes)
+	event, err := common.BuildEvent(r.req.Context(), &eb, r.cfg, r.evTime, r.queryAPI, &queryRes)
 	if err == nil {
 		if _, err = r.producer.SendEvents(r.req.Context(), []gomatrixserverlib.Event{*event}, r.cfg.Matrix.ServerName, nil); err != nil {
 			return httputil.LogThenError(r.req, err)
@@ -285,10 +296,9 @@ func (r joinRoomReq) joinRoomUsingServer(roomID string, server gomatrixserverlib
 		return nil, err
 	}
 
-	evTime := httputil.ParseTSParam(r.req)
 	eventID := fmt.Sprintf("$%s:%s", util.RandomString(16), r.cfg.Matrix.ServerName)
 	event, err := respMakeJoin.JoinEvent.Build(
-		eventID, evTime, r.cfg.Matrix.ServerName, r.cfg.Matrix.KeyID, r.cfg.Matrix.PrivateKey,
+		eventID, r.evTime, r.cfg.Matrix.ServerName, r.cfg.Matrix.KeyID, r.cfg.Matrix.PrivateKey,
 	)
 	if err != nil {
 		res := httputil.LogThenError(r.req, err)

src/github.com/matrix-org/dendrite/clientapi/routing/membership.go

@@ -50,7 +50,14 @@ func SendMembership(
 		return *reqErr
 	}
 
-	evTime := httputil.ParseTSParam(req)
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
+
 	inviteStored, err := threepid.CheckAndProcessInvite(
 		req.Context(), device, &body, cfg, queryAPI, accountDB, producer,
 		membership, roomID, evTime,

src/github.com/matrix-org/dendrite/clientapi/routing/profile.go

@@ -107,6 +107,14 @@ func SetAvatarURL(
 		return httputil.LogThenError(req, err)
 	}
 
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
+
 	oldProfile, err := accountDB.GetProfileByLocalpart(req.Context(), localpart)
 	if err != nil {
 		return httputil.LogThenError(req, err)
@@ -128,7 +136,7 @@ func SetAvatarURL(
 	}
 
 	events, err := buildMembershipEvents(
-		req.Context(), memberships, newProfile, userID, cfg, httputil.ParseTSParam(req), queryAPI,
+		req.Context(), memberships, newProfile, userID, cfg, evTime, queryAPI,
 	)
 	if err != nil {
 		return httputil.LogThenError(req, err)
@@ -196,6 +204,14 @@ func SetDisplayName(
 		return httputil.LogThenError(req, err)
 	}
 
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
+
 	oldProfile, err := accountDB.GetProfileByLocalpart(req.Context(), localpart)
 	if err != nil {
 		return httputil.LogThenError(req, err)
@@ -217,7 +233,7 @@ func SetDisplayName(
 	}
 
 	events, err := buildMembershipEvents(
-		req.Context(), memberships, newProfile, userID, cfg, httputil.ParseTSParam(req), queryAPI,
+		req.Context(), memberships, newProfile, userID, cfg, evTime, queryAPI,
 	)
 	if err != nil {
 		return httputil.LogThenError(req, err)

src/github.com/matrix-org/dendrite/clientapi/routing/sendevent.go

@@ -55,52 +55,11 @@ func SendEvent(
 		}
 	}
 
-	// parse the incoming http request
+	e, resErr := generateSendEvent(req, device, roomID, eventType, stateKey, cfg, queryAPI)
-	userID := device.UserID
-	var r map[string]interface{} // must be a JSON object
-	resErr := httputil.UnmarshalJSONRequest(req, &r)
 	if resErr != nil {
 		return *resErr
 	}
 
-	evTime := httputil.ParseTSParam(req)
-
-	// create the new event and set all the fields we can
-	builder := gomatrixserverlib.EventBuilder{
-		Sender:   userID,
-		RoomID:   roomID,
-		Type:     eventType,
-		StateKey: stateKey,
-	}
-	err := builder.SetContent(r)
-	if err != nil {
-		return httputil.LogThenError(req, err)
-	}
-
-	var queryRes api.QueryLatestEventsAndStateResponse
-	e, err := common.BuildEvent(req.Context(), &builder, cfg, evTime, queryAPI, &queryRes)
-	if err == common.ErrRoomNoExists {
-		return util.JSONResponse{
-			Code: http.StatusNotFound,
-			JSON: jsonerror.NotFound("Room does not exist"),
-		}
-	} else if err != nil {
-		return httputil.LogThenError(req, err)
-	}
-
-	// check to see if this user can perform this operation
-	stateEvents := make([]*gomatrixserverlib.Event, len(queryRes.StateEvents))
-	for i := range queryRes.StateEvents {
-		stateEvents[i] = &queryRes.StateEvents[i]
-	}
-	provider := gomatrixserverlib.NewAuthEvents(stateEvents)
-	if err = gomatrixserverlib.Allowed(*e, &provider); err != nil {
-		return util.JSONResponse{
-			Code: http.StatusForbidden,
-			JSON: jsonerror.Forbidden(err.Error()), // TODO: Is this error string comprehensible to the client?
-		}
-	}
-
 	var txnAndDeviceID *api.TransactionID
 	if txnID != nil {
 		txnAndDeviceID = &api.TransactionID{
@@ -129,3 +88,66 @@ func SendEvent(
 
 	return res
 }
+
+func generateSendEvent(
+	req *http.Request,
+	device *authtypes.Device,
+	roomID, eventType string, stateKey *string,
+	cfg config.Dendrite,
+	queryAPI api.RoomserverQueryAPI,
+) (*gomatrixserverlib.Event, *util.JSONResponse) {
+	// parse the incoming http request
+	userID := device.UserID
+	var r map[string]interface{} // must be a JSON object
+	resErr := httputil.UnmarshalJSONRequest(req, &r)
+	if resErr != nil {
+		return nil, resErr
+	}
+
+	evTime, err := httputil.ParseTSParam(req)
+	if err != nil {
+		return nil, &util.JSONResponse{
+			Code: http.StatusBadRequest,
+			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+		}
+	}
+
+	// create the new event and set all the fields we can
+	builder := gomatrixserverlib.EventBuilder{
+		Sender:   userID,
+		RoomID:   roomID,
+		Type:     eventType,
+		StateKey: stateKey,
+	}
+	err = builder.SetContent(r)
+	if err != nil {
+		resErr := httputil.LogThenError(req, err)
+		return nil, &resErr
+	}
+
+	var queryRes api.QueryLatestEventsAndStateResponse
+	e, err := common.BuildEvent(req.Context(), &builder, cfg, evTime, queryAPI, &queryRes)
+	if err == common.ErrRoomNoExists {
+		return nil, &util.JSONResponse{
+			Code: http.StatusNotFound,
+			JSON: jsonerror.NotFound("Room does not exist"),
+		}
+	} else if err != nil {
+		resErr := httputil.LogThenError(req, err)
+		return nil, &resErr
+	}
+
+	// check to see if this user can perform this operation
+	stateEvents := make([]*gomatrixserverlib.Event, len(queryRes.StateEvents))
+	for i := range queryRes.StateEvents {
+		stateEvents[i] = &queryRes.StateEvents[i]
+	}
+	provider := gomatrixserverlib.NewAuthEvents(stateEvents)
+	if err = gomatrixserverlib.Allowed(*e, &provider); err != nil {
+		return nil, &util.JSONResponse{
+			Code: http.StatusForbidden,
+			JSON: jsonerror.Forbidden(err.Error()), // TODO: Is this error string comprehensible to the client?
+		}
+	}
+	return e, nil
+}