Return HTTP errors when trying to kick invalid users (#1221)

Room integrity was never compromised as GMSL does auth checks,
but we would incorrectly 200 OK the request instead of 403ing.

clientapi/routing/membership.go

@@ -96,6 +96,7 @@ func SendKick(
 	req *http.Request, accountDB accounts.Database, device *userapi.Device,
 	roomID string, cfg *config.Dendrite,
 	rsAPI roomserverAPI.RoomserverInternalAPI, asAPI appserviceAPI.AppServiceQueryAPI,
+	stateAPI currentstateAPI.CurrentStateInternalAPI,
 ) util.JSONResponse {
 	body, evTime, roomVer, reqErr := extractRequestData(req, roomID, rsAPI)
 	if reqErr != nil {
@@ -108,6 +109,11 @@ func SendKick(
 		}
 	}
 
+	errRes := checkMemberInRoom(req.Context(), stateAPI, device.UserID, roomID)
+	if errRes != nil {
+		return *errRes
+	}
+
 	var queryRes roomserverAPI.QueryMembershipForUserResponse
 	err := rsAPI.QueryMembershipForUser(req.Context(), &roomserverAPI.QueryMembershipForUserRequest{
 		RoomID: roomID,
@@ -116,11 +122,11 @@ func SendKick(
 	if err != nil {
 		return util.ErrorResponse(err)
 	}
-	// kick is only valid if the user is not currently banned
-	if queryRes.Membership == "ban" {
+	// kick is only valid if the user is not currently banned or left (that is, they are joined or invited)
+	if queryRes.Membership != "join" && queryRes.Membership != "invite" {
 		return util.JSONResponse{
 			Code: 403,
-			JSON: jsonerror.Unknown("cannot /kick banned users"),
+			JSON: jsonerror.Unknown("cannot /kick banned or left users"),
 		}
 	}
 	// TODO: should we be using SendLeave instead?

clientapi/routing/routing.go

@@ -155,7 +155,7 @@ func Setup(
 			if err != nil {
 				return util.ErrorResponse(err)
 			}
-			return SendKick(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI)
+			return SendKick(req, accountDB, device, vars["roomID"], cfg, rsAPI, asAPI, stateAPI)
 		}),
 	).Methods(http.MethodPost, http.MethodOptions)
 	r0mux.Handle("/rooms/{roomID}/unban",

sytest-whitelist

@@ -413,3 +413,5 @@ A full_state incremental update returns only recent timeline
 A prev_batch token can be used in the v1 messages API
 We don't send redundant membership state across incremental syncs by default
 Typing notifications don't leak
+Users cannot kick users from a room they are not in
+Users cannot kick users who have already left a room