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

@@ -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 {

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)
+func SendBan(
-// 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(
 	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}
+	body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI)
-	verRes := api.QueryRoomVersionForRoomResponse{}
+	if reqErr != nil {
-	if err := rsAPI.QueryRoomVersionForRoom(req.Context(), &verReq, &verRes); err != 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
+	var queryRes roomserverAPI.QueryMembershipForUserResponse
-	if reqErr := httputil.UnmarshalJSONRequest(req, &body); reqErr != nil {
+	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,
+		req, device, body, cfg, rsAPI, accountDB, roomID, evTime,
-		membership, 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{}{}
+	perr := roomserverAPI.SendInvite(
-
+		req.Context(), rsAPI,
-	switch membership {
+		event.Headered(roomVer),
-	case gomatrixserverlib.Invite:
+		nil, // ask the roomserver to draw up invite room state for us
-		// Invites need to be handled specially
+		cfg.Matrix.ServerName,
-		perr := roomserverAPI.SendInvite(
+		nil,
-			req.Context(), rsAPI,
+	)
-			event.Headered(verRes.RoomVersion),
+	if perr != nil {
-			nil, // ask the roomserver to draw up invite room state for us
+		util.GetLogger(req.Context()).WithError(perr).Error("producer.SendInvite failed")
-			cfg.Matrix.ServerName,
+		return perr.JSONResponse()
-			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()
-		}
 	}
-
 	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)
+	profile, err := loadProfile(ctx, targetUserID, cfg, accountDB, asAPI)
-	if err != nil {
-		return nil, err
-	}
-
-	profile, err := loadProfile(ctx, stateKey, 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,
+		StateKey: &targetUserID,
-	}
-
-	// "unban" or "kick" isn't a valid membership value, change it to "leave"
-	if membership == "unban" || membership == "kick" {
-		membership = gomatrixserverlib.Leave
 	}
 
 	content := gomatrixserverlib.MemberContent{
@@ -218,29 +285,33 @@ func loadProfile(
 	return profile, err
 }
 
-// getMembershipStateKey extracts the target user ID of a membership change.
+func extractRequestData(req *http.Request, roomID string, rsAPI api.RoomserverInternalAPI) (
-// For "join" and "leave" this will be the ID of the user making the change.
+	body *threepid.MembershipRequest, evTime time.Time, roomVer gomatrixserverlib.RoomVersion, resErr *util.JSONResponse,
-// 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,
+	verReq := api.QueryRoomVersionForRoomRequest{RoomID: roomID}
-// returns a JSONResponse with a corresponding error code and message.
+	verRes := api.QueryRoomVersionForRoomResponse{}
-func getMembershipStateKey(
+	if err := rsAPI.QueryRoomVersionForRoom(req.Context(), &verReq, &verRes); err != nil {
-	body threepid.MembershipRequest, device *userapi.Device, membership string,
+		resErr = &util.JSONResponse{
-) (stateKey string, reason string, err error) {
+			Code: http.StatusBadRequest,
-	if membership == gomatrixserverlib.Ban || membership == "unban" || membership == "kick" || membership == gomatrixserverlib.Invite {
+			JSON: jsonerror.UnsupportedRoomVersion(err.Error()),
-		// 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
 		}
+		return
+	}
+	roomVer = verRes.RoomVersion
 
-		stateKey = body.UserID
+	if reqErr := httputil.UnmarshalJSONRequest(req, &body); reqErr != nil {
-		reason = body.Reason
+		resErr = reqErr
-	} else {
+		return
-		stateKey = device.UserID
 	}
 
+	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{

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}",

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

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

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

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]
+	response.EventID = evs[0].EventID()
-	return nil
+	response.Membership, err = evs[0].Membership()
+	return err
 }
 
 // QueryMembershipsForRoom implements api.RoomserverInternalAPI

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