Use MatrixFederationAgent for binds to Synapse #157
Conversation
sydent/threepid/bind.py
Outdated
| @@ -101,29 +102,27 @@ def _notify(self, assoc, attempt): | |||
| mxid = assoc["mxid"] | |||
| domain = mxid.split(":")[-1] | |||
| server = yield self._pickServer(domain) | |||
| callbackUrl = "https://%s/_matrix/federation/v1/3pid/onbind" % ( | |||
|
|
|||
| post_url = "https://%s/_matrix/federation/v1/3pid/onbind" % ( | |||
| server, | |||
| ) | |||
|
|
|||
| logger.info("Making bind callback to: %s", callbackUrl) | |||
There was a problem hiding this comment.
callbackUrl doesn't seem to be defined here any more
| @@ -101,29 +102,27 @@ def _notify(self, assoc, attempt): | |||
| mxid = assoc["mxid"] | |||
| domain = mxid.split(":")[-1] | |||
There was a problem hiding this comment.
this is the wrong way to extract a domain from an mxid, ftr... but that's probably a problem for another day.
sydent/threepid/bind.py
Outdated
| # Make a POST to the chosen Synapse server | ||
| http_client = FederationHttpClient() | ||
| response = yield http_client.post_json_get_nothing(post_url, assoc, { | ||
| 'headers': { | ||
| "Content-Type": ["application/json"], |
There was a problem hiding this comment.
this is set by post_json_get_nothing
sydent/threepid/bind.py
Outdated
| # Make a POST to the chosen Synapse server | ||
| http_client = FederationHttpClient() | ||
| response = yield http_client.post_json_get_nothing(post_url, assoc, { | ||
| 'headers': { | ||
| "Content-Type": ["application/json"], | ||
| "User-Agent": ["Sydent"], |
There was a problem hiding this comment.
and if this is worth doing, I think it should be done by the http client. Relatedly: I don't think it's worth doing.
sydent/threepid/bind.py
Outdated
| ) | ||
| # If the request failed, try again with exponential backoff | ||
| if response.code != 200: | ||
| self._notifyErrback(assoc, attempt, err) |
sydent/threepid/bind.py
Outdated
| Headers({ | ||
|
|
||
| # Make a POST to the chosen Synapse server | ||
| http_client = FederationHttpClient() |
There was a problem hiding this comment.
FederationHttpClient takes a parameter.
sydent/threepid/bind.py
Outdated
|
|
||
| # Make a POST to the chosen Synapse server | ||
| http_client = FederationHttpClient() | ||
| response = yield http_client.post_json_get_nothing(post_url, assoc, { |
There was a problem hiding this comment.
the docs on Agent (which also apply to FederationAgent, since they share an interface) have this to say:
An important thing to keep in mind is that the body will only be read from the connection after Response.deliverBody is called. This also means that the connection will remain open until this is done (and the body read). So, in general, any response with a body must have that body read using deliverBody . If the application is not interested in the body, it should issue a HEAD request or use a protocol which immediately calls stopProducing on its transport.
In other words, we're going to leak HTTP connections because we fail to read the response.
If you look at things like post_json_get_json in the synapse you'll notice that it is careful to always call readBody (which in turn calls deliverBody). I'd suggest you cargo-cult that function into the sydent codebase.
| else: | ||
| logger.info("Successfully notified on bind for %s" % (mxid,)) | ||
| except Exception as e: | ||
| self._notifyErrback(assoc, attempt, e) |
There was a problem hiding this comment.
this will get called if the first _notifyErrback raises, which is admittedly unlikely, but it would be nice to make it right. How about:
try:
response = yield http_client.post_json_get_nothing(post_url, assoc, {})
except Exception as e:
self._notifyErrback(assoc, attempt, e)
return
# now we can process response
|
(feel free to fix and merge) |
Stop using the
InsecureInterceptableContextFactoryand instead use MatrixFederationAgent which can handle .well-known server files.