-
Notifications
You must be signed in to change notification settings - Fork 85
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
Changes from 4 commits
f795642
4210907
c1c6067
5c8b16e
8281c4a
0d202e5
c23fc62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Handle federation request failures in `/request` explicitly, to reduce Sentry noise. |
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", | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Very possibly. I'm just trying to replicate what we had before (and also try to be a good citizen and provide error context).
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", | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.