From e560619f76d3c54c018ed8117c20346ab79007b0 Mon Sep 17 00:00:00 2001 From: Kegsay Date: Wed, 24 Jun 2020 18:19:54 +0100 Subject: [PATCH] Refactor SendMembership - make ban test pass (#1160) * Refactor SendMembership - make ban test pass * Only check invite auth events for local invites --- clientapi/routing/createroom.go | 7 +- clientapi/routing/membership.go | 255 ++++++++++++++++---------- clientapi/routing/routing.go | 42 ++++- clientapi/threepid/invites.go | 4 +- roomserver/api/query.go | 2 + roomserver/internal/perform_invite.go | 20 ++ roomserver/internal/query.go | 11 +- sytest-whitelist | 1 + 8 files changed, 237 insertions(+), 105 deletions(-) diff --git a/clientapi/routing/createroom.go b/clientapi/routing/createroom.go index 8682b03a4..42e1895ce 100644 --- a/clientapi/routing/createroom.go +++ b/clientapi/routing/createroom.go @@ -28,7 +28,6 @@ import ( "github.com/matrix-org/dendrite/clientapi/httputil" "github.com/matrix-org/dendrite/clientapi/jsonerror" - "github.com/matrix-org/dendrite/clientapi/threepid" "github.com/matrix-org/dendrite/internal/config" "github.com/matrix-org/dendrite/internal/eventutil" "github.com/matrix-org/dendrite/userapi/storage/accounts" @@ -373,13 +372,9 @@ func createRoom( // If this is a direct message then we should invite the participants. for _, invitee := range r.Invite { - // Build the membership request. - body := threepid.MembershipRequest{ - UserID: invitee, - } // Build the invite event. inviteEvent, err := buildMembershipEvent( - req.Context(), body, accountDB, device, gomatrixserverlib.Invite, + req.Context(), invitee, "", accountDB, device, gomatrixserverlib.Invite, roomID, true, cfg, evTime, rsAPI, asAPI, ) if err != nil { diff --git a/clientapi/routing/membership.go b/clientapi/routing/membership.go index aff1730c5..1f316384b 100644 --- a/clientapi/routing/membership.go +++ b/clientapi/routing/membership.go @@ -38,40 +38,141 @@ import ( var errMissingUserID = errors.New("'user_id' must be supplied") -// SendMembership implements PUT /rooms/{roomID}/(join|kick|ban|unban|leave|invite) -// by building a m.room.member event then sending it to the room server -// TODO: Can we improve the cyclo count here? Separate code paths for invites? -// nolint:gocyclo -func SendMembership( +func SendBan( req *http.Request, accountDB accounts.Database, device *userapi.Device, - roomID string, membership string, cfg *config.Dendrite, + roomID string, cfg *config.Dendrite, rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI, ) util.JSONResponse { - verReq := api.QueryRoomVersionForRoomRequest{RoomID: roomID} - verRes := api.QueryRoomVersionForRoomResponse{} - if err := rsAPI.QueryRoomVersionForRoom(req.Context(), &verReq, &verRes); err != nil { + body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI) + if reqErr != nil { + return *reqErr + } + return sendMembership(req.Context(), accountDB, device, roomID, "ban", body.Reason, cfg, body.UserID, evTime, roomVer, rsAPI, asAPI) +} + +func sendMembership(ctx context.Context, accountDB accounts.Database, device *userapi.Device, + roomID, membership, reason string, cfg *config.Dendrite, targetUserID string, evTime time.Time, + roomVer gomatrixserverlib.RoomVersion, + rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI) util.JSONResponse { + + event, err := buildMembershipEvent( + ctx, targetUserID, reason, accountDB, device, membership, + roomID, false, cfg, evTime, rsAPI, asAPI, + ) + if err == errMissingUserID { return util.JSONResponse{ Code: http.StatusBadRequest, - JSON: jsonerror.UnsupportedRoomVersion(err.Error()), + JSON: jsonerror.BadJSON(err.Error()), + } + } else if err == eventutil.ErrRoomNoExists { + return util.JSONResponse{ + Code: http.StatusNotFound, + JSON: jsonerror.NotFound(err.Error()), + } + } else if err != nil { + util.GetLogger(ctx).WithError(err).Error("buildMembershipEvent failed") + return jsonerror.InternalServerError() + } + + _, err = roomserverAPI.SendEvents( + ctx, rsAPI, + []gomatrixserverlib.HeaderedEvent{event.Headered(roomVer)}, + cfg.Matrix.ServerName, + nil, + ) + if err != nil { + util.GetLogger(ctx).WithError(err).Error("SendEvents failed") + return jsonerror.InternalServerError() + } + + return util.JSONResponse{ + Code: http.StatusOK, + JSON: struct{}{}, + } +} + +func SendKick( + req *http.Request, accountDB accounts.Database, device *userapi.Device, + roomID string, cfg *config.Dendrite, + rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI, +) util.JSONResponse { + body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI) + if reqErr != nil { + return *reqErr + } + if body.UserID == "" { + return util.JSONResponse{ + Code: 400, + JSON: jsonerror.BadJSON("missing user_id"), } } - var body threepid.MembershipRequest - if reqErr := httputil.UnmarshalJSONRequest(req, &body); reqErr != nil { + var queryRes roomserverAPI.QueryMembershipForUserResponse + err := rsAPI.QueryMembershipForUser(req.Context(), &roomserverAPI.QueryMembershipForUserRequest{ + RoomID: roomID, + UserID: body.UserID, + }, &queryRes) + if err != nil { + return util.ErrorResponse(err) + } + // kick is only valid if the user is not currently banned + if queryRes.Membership == "ban" { + return util.JSONResponse{ + Code: 403, + JSON: jsonerror.Unknown("cannot /kick banned users"), + } + } + // TODO: should we be using SendLeave instead? + return sendMembership(req.Context(), accountDB, device, roomID, "leave", body.Reason, cfg, body.UserID, evTime, roomVer, rsAPI, asAPI) +} + +func SendUnban( + req *http.Request, accountDB accounts.Database, device *userapi.Device, + roomID string, cfg *config.Dendrite, + rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI, +) util.JSONResponse { + body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI) + if reqErr != nil { + return *reqErr + } + if body.UserID == "" { + return util.JSONResponse{ + Code: 400, + JSON: jsonerror.BadJSON("missing user_id"), + } + } + + var queryRes roomserverAPI.QueryMembershipForUserResponse + err := rsAPI.QueryMembershipForUser(req.Context(), &roomserverAPI.QueryMembershipForUserRequest{ + RoomID: roomID, + UserID: body.UserID, + }, &queryRes) + if err != nil { + return util.ErrorResponse(err) + } + // unban is only valid if the user is currently banned + if queryRes.Membership != "ban" { + return util.JSONResponse{ + Code: 400, + JSON: jsonerror.Unknown("can only /unban users that are banned"), + } + } + // TODO: should we be using SendLeave instead? + return sendMembership(req.Context(), accountDB, device, roomID, "leave", body.Reason, cfg, body.UserID, evTime, roomVer, rsAPI, asAPI) +} + +func SendInvite( + req *http.Request, accountDB accounts.Database, device *userapi.Device, + roomID string, cfg *config.Dendrite, + rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI, +) util.JSONResponse { + body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI) + if reqErr != nil { return *reqErr } - evTime, err := httputil.ParseTSParam(req) - if err != nil { - return util.JSONResponse{ - Code: http.StatusBadRequest, - JSON: jsonerror.InvalidArgumentValue(err.Error()), - } - } - inviteStored, jsonErrResp := checkAndProcessThreepid( - req, device, &body, cfg, rsAPI, accountDB, - membership, roomID, evTime, + req, device, body, cfg, rsAPI, accountDB, roomID, evTime, ) if jsonErrResp != nil { return *jsonErrResp @@ -88,7 +189,7 @@ func SendMembership( } event, err := buildMembershipEvent( - req.Context(), body, accountDB, device, membership, + req.Context(), body.UserID, body.Reason, accountDB, device, "invite", roomID, false, cfg, evTime, rsAPI, asAPI, ) if err == errMissingUserID { @@ -106,61 +207,32 @@ func SendMembership( return jsonerror.InternalServerError() } - var returnData interface{} = struct{}{} - - switch membership { - case gomatrixserverlib.Invite: - // Invites need to be handled specially - perr := roomserverAPI.SendInvite( - req.Context(), rsAPI, - event.Headered(verRes.RoomVersion), - nil, // ask the roomserver to draw up invite room state for us - cfg.Matrix.ServerName, - nil, - ) - if perr != nil { - util.GetLogger(req.Context()).WithError(perr).Error("producer.SendInvite failed") - return perr.JSONResponse() - } - case gomatrixserverlib.Join: - // The join membership requires the room id to be sent in the response - returnData = struct { - RoomID string `json:"room_id"` - }{roomID} - fallthrough - default: - _, err = roomserverAPI.SendEvents( - req.Context(), rsAPI, - []gomatrixserverlib.HeaderedEvent{event.Headered(verRes.RoomVersion)}, - cfg.Matrix.ServerName, - nil, - ) - if err != nil { - util.GetLogger(req.Context()).WithError(err).Error("SendEvents failed") - return jsonerror.InternalServerError() - } + perr := roomserverAPI.SendInvite( + req.Context(), rsAPI, + event.Headered(roomVer), + nil, // ask the roomserver to draw up invite room state for us + cfg.Matrix.ServerName, + nil, + ) + if perr != nil { + util.GetLogger(req.Context()).WithError(perr).Error("producer.SendInvite failed") + return perr.JSONResponse() } - return util.JSONResponse{ Code: http.StatusOK, - JSON: returnData, + JSON: struct{}{}, } } func buildMembershipEvent( ctx context.Context, - body threepid.MembershipRequest, accountDB accounts.Database, + targetUserID, reason string, accountDB accounts.Database, device *userapi.Device, membership, roomID string, isDirect bool, cfg *config.Dendrite, evTime time.Time, rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI, ) (*gomatrixserverlib.Event, error) { - stateKey, reason, err := getMembershipStateKey(body, device, membership) - if err != nil { - return nil, err - } - - profile, err := loadProfile(ctx, stateKey, cfg, accountDB, asAPI) + profile, err := loadProfile(ctx, targetUserID, cfg, accountDB, asAPI) if err != nil { return nil, err } @@ -169,12 +241,7 @@ func buildMembershipEvent( Sender: device.UserID, RoomID: roomID, Type: "m.room.member", - StateKey: &stateKey, - } - - // "unban" or "kick" isn't a valid membership value, change it to "leave" - if membership == "unban" || membership == "kick" { - membership = gomatrixserverlib.Leave + StateKey: &targetUserID, } content := gomatrixserverlib.MemberContent{ @@ -218,29 +285,33 @@ func loadProfile( return profile, err } -// getMembershipStateKey extracts the target user ID of a membership change. -// For "join" and "leave" this will be the ID of the user making the change. -// For "ban", "unban", "kick" and "invite" the target user ID will be in the JSON request body. -// In the latter case, if there was an issue retrieving the user ID from the request body, -// returns a JSONResponse with a corresponding error code and message. -func getMembershipStateKey( - body threepid.MembershipRequest, device *userapi.Device, membership string, -) (stateKey string, reason string, err error) { - if membership == gomatrixserverlib.Ban || membership == "unban" || membership == "kick" || membership == gomatrixserverlib.Invite { - // If we're in this case, the state key is contained in the request body, - // possibly along with a reason (for "kick" and "ban") so we need to parse - // it - if body.UserID == "" { - err = errMissingUserID - return +func extractRequestData(req *http.Request, roomID string, rsAPI api.RoomserverInternalAPI) ( + body *threepid.MembershipRequest, evTime time.Time, roomVer gomatrixserverlib.RoomVersion, resErr *util.JSONResponse, +) { + verReq := api.QueryRoomVersionForRoomRequest{RoomID: roomID} + verRes := api.QueryRoomVersionForRoomResponse{} + if err := rsAPI.QueryRoomVersionForRoom(req.Context(), &verReq, &verRes); err != nil { + resErr = &util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.UnsupportedRoomVersion(err.Error()), } + return + } + roomVer = verRes.RoomVersion - stateKey = body.UserID - reason = body.Reason - } else { - stateKey = device.UserID + if reqErr := httputil.UnmarshalJSONRequest(req, &body); reqErr != nil { + resErr = reqErr + return } + evTime, err := httputil.ParseTSParam(req) + if err != nil { + resErr = &util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.InvalidArgumentValue(err.Error()), + } + return + } return } @@ -251,13 +322,13 @@ func checkAndProcessThreepid( cfg *config.Dendrite, rsAPI roomserverAPI.RoomserverInternalAPI, accountDB accounts.Database, - membership, roomID string, + roomID string, evTime time.Time, ) (inviteStored bool, errRes *util.JSONResponse) { inviteStored, err := threepid.CheckAndProcessInvite( req.Context(), device, body, cfg, rsAPI, accountDB, - membership, roomID, evTime, + roomID, evTime, ) if err == threepid.ErrMissingParameter { return inviteStored, &util.JSONResponse{ diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index 825ac50f2..eadcfd1ab 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -101,6 +101,17 @@ func Setup( return GetJoinedRooms(req, device, accountDB) }), ).Methods(http.MethodGet, http.MethodOptions) + r0mux.Handle("/rooms/{roomID}/join", + httputil.MakeAuthAPI(gomatrixserverlib.Join, userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { + vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) + if err != nil { + return util.ErrorResponse(err) + } + return JoinRoomByIDOrAlias( + req, device, rsAPI, accountDB, vars["roomID"], + ) + }), + ).Methods(http.MethodPost, http.MethodOptions) r0mux.Handle("/rooms/{roomID}/leave", httputil.MakeAuthAPI("membership", userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) @@ -112,13 +123,40 @@ func Setup( ) }), ).Methods(http.MethodPost, http.MethodOptions) - r0mux.Handle("/rooms/{roomID}/{membership:(?:join|kick|ban|unban|invite)}", + r0mux.Handle("/rooms/{roomID}/ban", httputil.MakeAuthAPI("membership", userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) if err != nil { return util.ErrorResponse(err) } - return SendMembership(req, accountDB, device, vars["roomID"], vars["membership"], cfg, rsAPI, asAPI) + return SendBan(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI) + }), + ).Methods(http.MethodPost, http.MethodOptions) + r0mux.Handle("/rooms/{roomID}/invite", + httputil.MakeAuthAPI("membership", userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { + vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) + if err != nil { + return util.ErrorResponse(err) + } + return SendInvite(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI) + }), + ).Methods(http.MethodPost, http.MethodOptions) + r0mux.Handle("/rooms/{roomID}/kick", + httputil.MakeAuthAPI("membership", userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { + vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) + if err != nil { + return util.ErrorResponse(err) + } + return SendKick(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI) + }), + ).Methods(http.MethodPost, http.MethodOptions) + r0mux.Handle("/rooms/{roomID}/unban", + httputil.MakeAuthAPI("membership", userAPI, func(req *http.Request, device *api.Device) util.JSONResponse { + vars, err := httputil.URLDecodeMapValues(mux.Vars(req)) + if err != nil { + return util.ErrorResponse(err) + } + return SendUnban(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI) }), ).Methods(http.MethodPost, http.MethodOptions) r0mux.Handle("/rooms/{roomID}/send/{eventType}", diff --git a/clientapi/threepid/invites.go b/clientapi/threepid/invites.go index c308cb1f4..89bc86064 100644 --- a/clientapi/threepid/invites.go +++ b/clientapi/threepid/invites.go @@ -88,10 +88,10 @@ func CheckAndProcessInvite( ctx context.Context, device *userapi.Device, body *MembershipRequest, cfg *config.Dendrite, rsAPI api.RoomserverInternalAPI, db accounts.Database, - membership string, roomID string, + roomID string, evTime time.Time, ) (inviteStoredOnIDServer bool, err error) { - if membership != gomatrixserverlib.Invite || (body.Address == "" && body.IDServer == "" && body.Medium == "") { + if body.Address == "" && body.IDServer == "" && body.Medium == "" { // If none of the 3PID-specific fields are supplied, it's a standard invite // so return nil for it to be processed as such return diff --git a/roomserver/api/query.go b/roomserver/api/query.go index 6586b1af3..f0cb9374b 100644 --- a/roomserver/api/query.go +++ b/roomserver/api/query.go @@ -112,6 +112,8 @@ type QueryMembershipForUserResponse struct { HasBeenInRoom bool `json:"has_been_in_room"` // True if the user is in room. IsInRoom bool `json:"is_in_room"` + // The current membership + Membership string } // QueryMembershipsForRoomRequest is a request to QueryMembershipsForRoom diff --git a/roomserver/internal/perform_invite.go b/roomserver/internal/perform_invite.go index c65c87f91..4600bec0b 100644 --- a/roomserver/internal/perform_invite.go +++ b/roomserver/internal/perform_invite.go @@ -55,6 +55,7 @@ func (r *RoomserverInternalAPI) performInvite(ctx context.Context, return nil } +// nolint:gocyclo func (r *RoomserverInternalAPI) processInviteEvent( ctx context.Context, ow *RoomserverInternalAPI, @@ -135,6 +136,25 @@ func (r *RoomserverInternalAPI) processInviteEvent( } event := input.Event.Unwrap() + + // check that the user is allowed to do this. We can only do this check if it is + // a local invite as we have the auth events, else we have to take it on trust. + if loopback != nil { + _, err = checkAuthEvents(ctx, r.DB, input.Event, input.Event.AuthEventIDs()) + if err != nil { + log.WithError(err).WithField("event_id", event.EventID()).WithField("auth_event_ids", event.AuthEventIDs()).Error( + "processInviteEvent.checkAuthEvents failed for event", + ) + if _, ok := err.(*gomatrixserverlib.NotAllowed); ok { + return nil, &api.PerformError{ + Msg: err.Error(), + Code: api.PerformErrorNotAllowed, + } + } + return nil, err + } + } + if len(input.InviteRoomState) > 0 { // If we were supplied with some invite room state already (which is // most likely to be if the event came in over federation) then use diff --git a/roomserver/internal/query.go b/roomserver/internal/query.go index 4fc8e4c25..19236bfbd 100644 --- a/roomserver/internal/query.go +++ b/roomserver/internal/query.go @@ -225,13 +225,18 @@ func (r *RoomserverInternalAPI) QueryMembershipForUser( } response.IsInRoom = stillInRoom - eventIDMap, err := r.DB.EventIDs(ctx, []types.EventNID{membershipEventNID}) + + evs, err := r.DB.Events(ctx, []types.EventNID{membershipEventNID}) if err != nil { return err } + if len(evs) != 1 { + return fmt.Errorf("failed to load membership event for event NID %d", membershipEventNID) + } - response.EventID = eventIDMap[membershipEventNID] - return nil + response.EventID = evs[0].EventID() + response.Membership, err = evs[0].Membership() + return err } // QueryMembershipsForRoom implements api.RoomserverInternalAPI diff --git a/sytest-whitelist b/sytest-whitelist index 18bb7ca43..ce97e8af7 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -365,3 +365,4 @@ Invited user can reject local invite after originator leaves 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