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

Catch failures to contact remote homeserver for /register #456

Merged
merged 7 commits into from
Nov 8, 2021
Merged
Changes from 4 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
2 changes: 1 addition & 1 deletion .github/workflows/pipeline.yml
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ jobs:
- uses: actions/setup-python@v2
with:
python-version: ${{ matrix.python-version }}
- run: python -m pip install -e .
- run: python -m pip install -e .[dev]
- run: trial tests

run-matrix-is-tests:
1 change: 1 addition & 0 deletions changelog.d/456.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Handle federation request failures in `/request` explicitly, to reduce Sentry noise.
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
@@ -56,6 +56,7 @@ def read(fname):
],
extras_require={
"dev": [
"parameterized==0.8.1",
"flake8==3.9.2",
"flake8-pyi==20.10.0",
"black==21.6b0",
2 changes: 2 additions & 0 deletions stubs/twisted/internet/error.pyi
Original file line number Diff line number Diff line change
@@ -2,3 +2,5 @@ from typing import Any, Optional

class ConnectError(Exception):
def __init__(self, osError: Optional[Any] = ..., string: str = ...): ...

class DNSLookupError(IOError): ...
8 changes: 7 additions & 1 deletion stubs/twisted/web/client.pyi
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
from typing import BinaryIO, Optional, Type, TypeVar
from typing import BinaryIO, Optional, Sequence, Type, TypeVar
Copy link
Member

Choose a reason for hiding this comment

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

We should probably include license headers in the stubs files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm---possibly? They feel more like metadata derived from twisted rather than original creation of ourselves. Not sure what the right thing to do from a licensing perspective is there.


from twisted.internet.defer import Deferred
from twisted.internet.interfaces import IConsumer, IProtocol
from twisted.internet.task import Cooperator
from twisted.python.failure import Failure
from twisted.web.http_headers import Headers
from twisted.web.iweb import (
IAgent,
@@ -15,6 +16,11 @@ from zope.interface import implementer

_C = TypeVar("_C")

class ResponseFailed(Exception):
def __init__(
self, reasons: Sequence[Failure], response: Optional[Response] = ...
): ...

class HTTPConnectionPool:
persistent: bool
maxPersistentPerHost: int
41 changes: 28 additions & 13 deletions sydent/http/servlets/registerservlet.py
Original file line number Diff line number Diff line change
@@ -14,8 +14,11 @@

import logging
import urllib
from http import HTTPStatus
from typing import TYPE_CHECKING

from twisted.internet.error import ConnectError, DNSLookupError
from twisted.web.client import ResponseFailed
from twisted.web.resource import Resource
from twisted.web.server import Request

@@ -56,22 +59,34 @@ async def render_POST(self, request: Request) -> JsonDict:
"error": "matrix_server_name must be a valid Matrix server name (IP address or hostname)",
}

result = await self.client.get_json(
"matrix://%s/_matrix/federation/v1/openid/userinfo?access_token=%s"
% (
matrix_server,
urllib.parse.quote(args["access_token"]),
),
1024 * 5,
)
try:
result = await self.client.get_json(
"matrix://%s/_matrix/federation/v1/openid/userinfo?access_token=%s"
% (
matrix_server,
urllib.parse.quote(args["access_token"]),
),
1024 * 5,
)
except (DNSLookupError, ConnectError, ResponseFailed) as e:
logger.warning("Unable to contact %s: %s", matrix_server, e)
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
"error": f"Unable to contact the Matrix homeserver ({type(e).__name__})",
}

if "sub" not in result:
raise Exception("Invalid response from homeserver")
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
"error": "The Matrix homeserver did not include 'sub' in its response",
}
Copy link
Member

Choose a reason for hiding this comment

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

I think clients don't usually expect JSON bodies for 500 errors so something feels off to me?

I guess this is similar to what we had before but now we'll not error to sentry?

Do we want to log a warning or anything in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think clients don't usually expect JSON bodies for 500 errors so something feels off to me?

Very possibly. I'm just trying to replicate what we had before (and also try to be a good citizen and provide error context).

I guess this is similar to what we had before but now we'll not error to sentry?

That's right. I didn't think it merited a sentry warning since it's not a problem in our application---it's duff data from the far end.

I think a warning would be prudent so that we'll have something in the logs to diagnose this. I added something for this on line 72. I'll add something for this and the cases below.


user_id = result["sub"]

if not isinstance(user_id, str):
request.setResponseCode(500)
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
"error": "The Matrix homeserver returned a malformed reply",
@@ -81,7 +96,7 @@ async def render_POST(self, request: Request) -> JsonDict:

# Ensure there's a localpart and domain in the returned user ID.
if len(user_id_components) != 2:
request.setResponseCode(500)
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
"error": "The Matrix homeserver returned an invalid MXID",
@@ -90,14 +105,14 @@ async def render_POST(self, request: Request) -> JsonDict:
user_id_server = user_id_components[1]

if not is_valid_matrix_server_name(user_id_server):
request.setResponseCode(500)
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
"error": "The Matrix homeserver returned an invalid MXID",
}

if user_id_server != matrix_server:
request.setResponseCode(500)
request.setResponseCode(HTTPStatus.INTERNAL_SERVER_ERROR)
return {
"errcode": "M_UNKNOWN",
"error": "The Matrix homeserver returned a MXID belonging to another homeserver",
43 changes: 41 additions & 2 deletions tests/test_register.py
Original file line number Diff line number Diff line change
@@ -11,7 +11,12 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from http import HTTPStatus
from unittest.mock import patch

import twisted.internet.error
import twisted.web.client
from parameterized import parameterized
from twisted.trial import unittest

from tests.utils import make_request, make_sydent
@@ -20,11 +25,11 @@
class RegisterTestCase(unittest.TestCase):
"""Tests Sydent's register servlet"""

def setUp(self):
def setUp(self) -> None:
# Create a new sydent
self.sydent = make_sydent()

def test_sydent_rejects_invalid_hostname(self):
def test_sydent_rejects_invalid_hostname(self) -> None:
"""Tests that the /register endpoint rejects an invalid hostname passed as matrix_server_name"""
self.sydent.run()

@@ -40,3 +45,37 @@ def test_sydent_rejects_invalid_hostname(self):
request.render(self.sydent.servlets.registerServlet)

self.assertEqual(channel.code, 400)

@parameterized.expand(
[
(twisted.internet.error.DNSLookupError(),),
(twisted.internet.error.TimeoutError(),),
(twisted.internet.error.ConnectionRefusedError(),),
# Naughty: strictly we're supposed to initialise a ResponseNeverReceived
# with a list of 1 or more failures.
(twisted.web.client.ResponseNeverReceived([]),),
]
)
def test_connection_failure(self, exc: Exception) -> None:
self.sydent.run()
request, channel = make_request(
self.sydent.reactor,
"POST",
"/_matrix/identity/v2/account/register",
content={
"matrix_server_name": "matrix.alice.com",
"access_token": "back_in_wonderland",
},
)
servlet = self.sydent.servlets.registerServlet

def mock_get_json(*args: object, **kwargs: object) -> None:
raise exc

with patch.object(servlet.client, "get_json", mock_get_json):
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
request.render(servlet)
self.assertEqual(channel.code, HTTPStatus.INTERNAL_SERVER_ERROR)
self.assertEqual(channel.json_body["errcode"], "M_UNKNOWN")
# Check that we haven't just returned the generic error message in asyncjsonwrap
self.assertNotEqual(channel.json_body["error"], "Internal Server Error")
self.assertIn("contact", channel.json_body["error"])