Refactor SendMembership - make ban test pass (#1160)

* Refactor SendMembership - make ban test pass

* Only check invite auth events for local invites

4 files changed, 201 insertions, 97 deletions

diff --git a/clientapi/routing/createroom.go b/clientapi/routing/createroom.go
index 8682b03a..42e1895c 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 aff1730c..1f316384 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()
 	}
 
-	var body threepid.MembershipRequest
-	if reqErr := httputil.UnmarshalJSONRequest(req, &body); reqErr != nil {
+	_, 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"),
+		}
+	}
 
-	evTime, err := httputil.ParseTSParam(req)
+	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: http.StatusBadRequest,
-			JSON: jsonerror.InvalidArgumentValue(err.Error()),
+			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
+	}
 
 	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 825ac50f..eadcfd1a 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 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 SendMembership(req, accountDB, device, vars["roomID"], vars["membership"], cfg, rsAPI, asAPI)
+			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 c308cb1f..89bc8606 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