Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

Fix failing federation tests #1579

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions clientapi/routing/sendevent.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package routing

import (
"context"
"net/http"
"sync"

Expand Down Expand Up @@ -80,6 +81,12 @@ func SendEvent(
return *resErr
}

if e.Type() == gomatrixserverlib.MRoomPowerLevels {
if ok, plErr := checkPowerLevels(req.Context(), roomID, rsAPI, e); !ok {
return *plErr
}
}

var txnAndSessionID *api.TransactionID
if txnID != nil {
txnAndSessionID = &api.TransactionID{
Expand Down Expand Up @@ -120,6 +127,46 @@ func SendEvent(
return res
}

func checkPowerLevels(
ctx context.Context,
roomID string,
rsAPI api.RoomserverInternalAPI,
event *gomatrixserverlib.Event,
) (ok bool, response *util.JSONResponse) {
req := api.QueryLatestEventsAndStateRequest{RoomID: roomID}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably be setting StateToFetch in req so that we aren't pulling out lots of state events that we don't need - ultimately we only care about the create event and the existing power level event.

res := api.QueryLatestEventsAndStateResponse{}
if err := rsAPI.QueryLatestEventsAndState(ctx, &req, &res); err != nil {
return false, &util.JSONResponse{
Code: http.StatusInternalServerError,
JSON: err.Error(),
}
}

var creator string
for _, v := range res.StateEvents {
pl, _ := v.PowerLevels() // we only care about the existence of power levels, so ignore any errors
if pl != nil {
return true, nil
}
if v.Type() == gomatrixserverlib.MRoomCreate {
creator = v.Sender()
}
}

if creator != "" && event.Sender() != creator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we need a check in the client API for this? The roomserver should reject an event that it isn't happy with, and the client API won't be involved in remote servers pushing state to us.

logrus.WithFields(logrus.Fields{
"creator": creator,
"sender": event.Sender(),
}).Warnf("rejecting request to set initial powerlevel, no powerlevels defined and sender != creator")
return false, &util.JSONResponse{
Code: http.StatusForbidden,
JSON: jsonerror.Forbidden("non room creator is not allowed to set initial powerlevels"),
}
}

return true, nil
}

func generateSendEvent(
req *http.Request,
device *userapi.Device,
Expand Down
39 changes: 38 additions & 1 deletion roomserver/internal/input/input_events.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ func (r *Inputer) processRoomEvent(
isRejected = true
}

var isPLRejected bool
var isPLRejectedErr error
if event.Type() == gomatrixserverlib.MRoomPowerLevels {
isPLRejected, isPLRejectedErr = r.checkPowerLevels(ctx, event)
}

var softfail bool
if input.Kind == api.KindNew {
// Check that the event passes authentication checks based on the
Expand Down Expand Up @@ -149,7 +155,10 @@ func (r *Inputer) processRoomEvent(
}

// We stop here if the event is rejected: We've stored it but won't update forward extremities or notify anyone about it.
if isRejected || softfail {
if isRejected || isPLRejected || softfail {
if rejectionErr == nil {
rejectionErr = isPLRejectedErr
}
logrus.WithFields(logrus.Fields{
"event_id": event.EventID(),
"type": event.Type(),
Expand Down Expand Up @@ -210,6 +219,34 @@ func (r *Inputer) processRoomEvent(
return event.EventID(), nil
}

func (r *Inputer) checkPowerLevels(
ctx context.Context, event gomatrixserverlib.Event,
) (isRejected bool, err error) {
req := &api.QueryLatestEventsAndStateRequest{RoomID: event.RoomID()}
resp := &api.QueryLatestEventsAndStateResponse{}
if err := helpers.QueryLatestEventsAndState(ctx, r.DB, req, resp); err != nil {
logrus.WithError(err).Error("helpers.QueryLatestEventsAndState failed to get latest events and state")
return true, err
}
var creator string
for _, v := range resp.StateEvents {
if v.Type() == gomatrixserverlib.MRoomCreate {
creator = v.Sender()
break
}
}
if event.Sender() != creator && r.ServerName != event.Origin() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check is doing the right thing - ultimately what we want to do is check if the event is sent by the same creator only if there isn't a power level event already in the room.

If there is an existing power level event then we don't need to do this, as the soft-fail check will capture it if the current state events don't allow the new event.

logrus.WithFields(logrus.Fields{
"event_id": event.EventID(),
"origin": event.Origin(),
"servername": r.ServerName,
}).Error("remote server is not allowed to set powerlevels, rejecting event")

return true, &gomatrixserverlib.NotAllowed{Message: "remote server is not allowed to set powerlevels"}
}
return false, nil
}

func (r *Inputer) calculateAndSetState(
ctx context.Context,
input *api.InputRoomEvent,
Expand Down
2 changes: 2 additions & 0 deletions sytest-whitelist
Original file line number Diff line number Diff line change
Expand Up @@ -496,3 +496,5 @@ Forgotten room messages cannot be paginated
Forgetting room does not show up in v2 /sync
Can forget room you've been kicked from
Can re-join room if re-invited
Remote servers should reject attempts by non-creators to set the power levels
Remote servers cannot set power levels in rooms without existing powerlevels