Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a handler for federation transactions #47

Merged
merged 9 commits into from
Dec 8, 2020

Conversation

anoadragon453
Copy link
Member

Add a handler which can accept a federation transaction, validate the PDUs and return errors in the case of failing to process a given PDU.

EDUs are accepted but we currently don't do anything with them.

This grew out of trying to fix TestOutboundFederationIgnoresMissingEventWithBadJSONForRoomVersion6 for Synapse. The test would fail because as soon as Synapse joined the room, it would start trying to send presence EDUs to the new server. Rather than disabling presence, I thought it would be better to write a proper handler for it.

I'll use this handler for that test (and possibly others) in a separate PR.

Add a handler which can accept a federation transaction, validate the PDUs and return errors
in the case of failing to process a given PDU.

EDUs are accepted but we currently don't do anything with them.
@anoadragon453
Copy link
Member Author

I've just realised that a proto version of this exists here:

// TODO: Have a nicer api shape than just http.Handler
srv.Mux().Handle("/_matrix/federation/v1/send/{txnID}", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer waiter.Finish()
var body gomatrixserverlib.Transaction
if err := json.NewDecoder(req.Body).Decode(&body); err != nil {
t.Errorf("failed to decode /send body: %s", err)
w.WriteHeader(500)
return
}
if len(body.PDUs) != 1 {
t.Fatalf("got %d pdus, want %d", len(body.PDUs), 1)
}
ev, err := gomatrixserverlib.NewEventFromUntrustedJSON(body.PDUs[0], ver)
if err != nil {
t.Fatalf("PDU failed NewEventFromUntrustedJSON checks: %s", err)
}
if ev.Type() != wantEventType {
t.Errorf("Wrong event type, got %s want %s", ev.Type(), wantEventType)
}
w.WriteHeader(200)
})).Methods("PUT")
, but perhaps this change is better still?

kegsay
kegsay previously requested changes Dec 4, 2020
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Nil check is important.

internal/federation/handle.go Show resolved Hide resolved
internal/federation/handle.go Outdated Show resolved Hide resolved
internal/federation/handle.go Outdated Show resolved Hide resolved
@@ -277,3 +279,101 @@ func HandleMediaRequests(mediaIds map[string]func(w http.ResponseWriter)) func(*
mediamux.Handle("/r0/download/{origin}/{mediaId}", downloadFn).Methods("GET")
}
}

// HandleTransactionRequests is an option which will process GET /_matrix/federation/v1/send/{transactionID} requests universally when requested.
func HandleTransactionRequests() func(*Server) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be cute to have a callback function here which is passed with valid gmsl.Event structs, since it's fairly common to want to process stuff on /send. E.g:

func HandleTransactionRequests(cb func(gmsl.Event)) func(*Server) {
    // when ready..
    cb(event)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what I was thinking, yeah. Then we could perhaps augment TestOutboundFederationSend to use this instead, while slimming down the test at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shall I add another callback method for EDUs? Edit: have done so.

req.Body has already been read from via gmsl.VerifyHTTPRequest thus trying to read
from it again produces an empty body.

I also added some more error logging.
@anoadragon453
Copy link
Member Author

Realised there was a bug in that we were reading from req.Body which had already been read from during gmsl.VerifyHTTPRequest. So now we read from fedReq.Content() instead.

Additionally I added nil checks for the pdu and edu handler functions.

kegsay
kegsay previously requested changes Dec 7, 2020
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Logging needs to be more explicit, and the panic on error needs to be fixed, otherwise LGTM

@anoadragon453 anoadragon453 dismissed kegsay’s stale review December 7, 2020 17:12

Review items addressed.

internal/federation/handle.go Outdated Show resolved Hide resolved
internal/federation/handle.go Outdated Show resolved Hide resolved
@kegsay kegsay merged commit 0c7cefe into master Dec 8, 2020
anoadragon453 added a commit that referenced this pull request Dec 11, 2020
Updates the following tests to use the new transaction handler introduced in #47:

* `TestOutboundFederationSend` - This test failed with Synapse with the inline transaction handler. Not entirely sure why, the handler function was just never called.
* `TestOutboundFederationIgnoresMissingEventWithBadJSONForRoomVersion6` - Synapse will attempt to send presence to the remote homeserver. The test would fail if not expecting this.andler.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants