From b71d922a72b94a5c58f81a41410f818d7a63daa6 Mon Sep 17 00:00:00 2001 From: krombel Date: Wed, 22 Aug 2018 14:40:25 +0200 Subject: [PATCH] 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 --- .../dendrite/clientapi/httputil/parse.go | 9 +- .../dendrite/clientapi/jsonerror/jsonerror.go | 2 +- .../dendrite/clientapi/routing/createroom.go | 8 +- .../dendrite/clientapi/routing/joinroom.go | 20 +++- .../dendrite/clientapi/routing/membership.go | 9 +- .../dendrite/clientapi/routing/profile.go | 20 +++- .../dendrite/clientapi/routing/sendevent.go | 106 +++++++++++------- 7 files changed, 118 insertions(+), 56 deletions(-) diff --git a/src/github.com/matrix-org/dendrite/clientapi/httputil/parse.go b/src/github.com/matrix-org/dendrite/clientapi/httputil/parse.go index 754fa2427..ee6033416 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/httputil/parse.go +++ b/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 } diff --git a/src/github.com/matrix-org/dendrite/clientapi/jsonerror/jsonerror.go b/src/github.com/matrix-org/dendrite/clientapi/jsonerror/jsonerror.go index 87b0b8ac2..fa15d9d8e 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/jsonerror/jsonerror.go +++ b/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) } diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/createroom.go b/src/github.com/matrix-org/dendrite/clientapi/routing/createroom.go index b531a7bd0..a7187c495 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/createroom.go +++ b/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! diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/joinroom.go b/src/github.com/matrix-org/dendrite/clientapi/routing/joinroom.go index be4647aa1..98c7cd6a7 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/joinroom.go +++ b/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) diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/membership.go b/src/github.com/matrix-org/dendrite/clientapi/routing/membership.go index d00369aa9..b308de79a 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/membership.go +++ b/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, diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/profile.go b/src/github.com/matrix-org/dendrite/clientapi/routing/profile.go index 1469da6f8..e57d16fbf 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/profile.go +++ b/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) diff --git a/src/github.com/matrix-org/dendrite/clientapi/routing/sendevent.go b/src/github.com/matrix-org/dendrite/clientapi/routing/sendevent.go index 9176f4c87..e916e451e 100644 --- a/src/github.com/matrix-org/dendrite/clientapi/routing/sendevent.go +++ b/src/github.com/matrix-org/dendrite/clientapi/routing/sendevent.go @@ -55,52 +55,11 @@ func SendEvent( } } - // parse the incoming http request - userID := device.UserID - var r map[string]interface{} // must be a JSON object - resErr := httputil.UnmarshalJSONRequest(req, &r) + e, resErr := generateSendEvent(req, device, roomID, eventType, stateKey, cfg, queryAPI) 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 +}