matrix/authentication-service
1
0
Fork 0
mirror of https://github.com/matrix-org/matrix-authentication-service.git synced 2025-08-06 06:02:40 +03:00
Code Activity

Improve errors when MAS contacts the Synapse homeserver (#2794)

Browse Source 
* Add some drive-by docstrings

* Change text rendering of catch_http_codes::HttpError

Using `#[source]` is unnatural here because it makes it look like
two distinct errors (one being a cause of the other),
when in reality it is just one error, with 2 parts.

Using `Display` formatting for that leads to a more natural error.

* Add constraints to `catch_http_code{,s}` methods

Not strictly required, but does two things:

- documents what kind of function is expected
- provides a small extra amount of type enforcement at the call site,
  rather than later on when you find the result doesn't implement Service

* Add a `catch_http_errors` shorthand

Nothing major, just a quality of life improvement so you don't have to
repetitively write out what a HTTP error is

* Unexpected error page: remove leading whitespace from preformatted 'details' section

The extra whitespace was probably unintentional and makes the error harder to read,
particularly when it wraps onto a new line unnecessarily

* Capture and log Matrix errors received from Synapse

* Drive-by clippy fix: use clamp instead of min().max()

* Convert `err(Display)` to `err(Debug)` for `anyhow::Error`s in matrix-synapse support module
This commit is contained in:
reivilibre
2024-06-07 12:14:04 +01:00
committed by GitHub
parent d76b54b13f
commit 49e8fe57f4
9 changed files with 236 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
Cargo.lock generated
View File

@@ -3384,6 +3384,7 @@ dependencies = [
"mas-http",
"mas-matrix",
"serde",
"serde_json",
"tower",
"tracing",
"url",

2
crates/axum-utils/src/language_detection.rs
View File

@@ -109,7 +109,7 @@ impl Header for AcceptLanguage {
let quality = std::str::from_utf8(quality).map_err(|_e| Error::invalid())?;
let quality = quality.parse::<f64>().map_err(|_e| Error::invalid())?;
// Bound the quality between 0 and 1
let quality = quality.min(1_f64).max(0_f64);
let quality = quality.clamp(0_f64, 1_f64);
// Make sure the iterator is empty
if it.next().is_some() {

33
crates/http/src/ext.rs
View File

@@ -14,7 +14,7 @@
use std::{ops::RangeBounds, sync::OnceLock};
use http::{header::HeaderName, Request, StatusCode};
use http::{header::HeaderName, Request, Response, StatusCode};
use tower::Service;
use tower_http::cors::CorsLayer;
@@ -70,6 +70,9 @@ pub trait ServiceExt<Body>: Sized {
BytesToBodyRequest::new(self)
}
/// Adds a layer which collects all the response body into a contiguous
/// byte buffer.
/// This makes the response type `Response<Bytes>`.
fn response_body_to_bytes(self) -> BodyToBytesResponse<Self> {
BodyToBytesResponse::new(self)
}
@@ -86,20 +89,40 @@ pub trait ServiceExt<Body>: Sized {
FormUrlencodedRequest::new(self)
}
fn catch_http_code<M>(self, status_code: StatusCode, mapper: M) -> CatchHttpCodes<Self, M>
/// Catches responses with the given status code and then maps those
/// responses to an error type using the provided `mapper` function.
fn catch_http_code<M, ResBody, E>(
self,
status_code: StatusCode,
mapper: M,
) -> CatchHttpCodes<Self, M>
where
M: Clone,
M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
{
self.catch_http_codes(status_code..=status_code, mapper)
}
fn catch_http_codes<B, M>(self, bounds: B, mapper: M) -> CatchHttpCodes<Self, M>
/// Catches responses with the given status codes and then maps those
/// responses to an error type using the provided `mapper` function.
fn catch_http_codes<B, M, ResBody, E>(self, bounds: B, mapper: M) -> CatchHttpCodes<Self, M>
where
B: RangeBounds<StatusCode>,
M: Clone,
M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
{
CatchHttpCodes::new(self, bounds, mapper)
}
/// Shorthand for [`Self::catch_http_codes`] which catches all client errors
/// (4xx) and server errors (5xx).
fn catch_http_errors<M, ResBody, E>(self, mapper: M) -> CatchHttpCodes<Self, M>
where
M: Fn(Response<ResBody>) -> E + Send + Clone + 'static,
{
self.catch_http_codes(
StatusCode::from_u16(400).unwrap()..StatusCode::from_u16(600).unwrap(),
mapper,
)
}
}
impl<S, B> ServiceExt<B> for S where S: Service<Request<B>> {}

14
crates/http/src/layers/catch_http_codes.rs
View File

@@ -24,12 +24,8 @@ pub enum Error<S, E> {
#[error(transparent)]
Service { inner: S },
#[error("request failed with status {status_code}")]
HttpError {
status_code: StatusCode,
#[source]
inner: E,
},
#[error("request failed with status {status_code}: {inner}")]
HttpError { status_code: StatusCode, inner: E },
}
impl<S, E> Error<S, E> {
@@ -45,10 +41,16 @@ impl<S, E> Error<S, E> {
}
}
/// A layer that catches responses with the HTTP status codes lying within
/// `bounds` and then maps the requests into a custom error type using `mapper`.
#[derive(Clone)]
pub struct CatchHttpCodes<S, M> {
/// The inner service
inner: S,
/// Which HTTP status codes to catch
bounds: (Bound<StatusCode>, Bound<StatusCode>),
/// The function used to convert errors, which must be
/// `Fn(Response<ResBody>) -> E + Send + Clone + 'static`.
mapper: M,
}

1
crates/http/src/layers/json_response.rs
View File

@@ -23,6 +23,7 @@ use tower::{Layer, Service};
#[derive(Debug, Error)]
pub enum Error<Service> {
/// An error from the inner service.
#[error(transparent)]
Service { inner: Service },

1
crates/matrix-synapse/Cargo.toml
View File

@@ -16,6 +16,7 @@ anyhow.workspace = true
async-trait.workspace = true
http.workspace = true
serde.workspace = true
serde_json.workspace = true
tower.workspace = true
tracing.workspace = true
url.workspace = true

64
crates/matrix-synapse/src/error.rs Normal file
View File

@@ -0,0 +1,64 @@
use std::{error::Error, fmt::Display};
use http::Response;
use mas_axum_utils::axum::body::Bytes;
use serde::Deserialize;
use tracing::debug;
/// Represents a Matrix error
/// Ref: <https://spec.matrix.org/v1.10/client-server-api/#standard-error-response>
#[derive(Debug, Deserialize)]
struct MatrixError {
errcode: String,
error: String,
}
/// Represents an error received from the homeserver.
/// Where possible, we capture the Matrix error from the JSON response body.
///
/// Note that the `CatchHttpCodes` layer already captures the `StatusCode` for
/// us; we don't need to do that twice.
#[derive(Debug)]
pub(crate) struct HomeserverError {
matrix_error: Option<MatrixError>,
}
impl Display for HomeserverError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
if let Some(matrix_error) = &self.matrix_error {
write!(f, "{matrix_error}")
} else {
write!(f, "(no specific error)")
}
}
}
impl Error for HomeserverError {}
impl HomeserverError {
/// Return the error code (`errcode`)
pub fn errcode(&self) -> Option<&str> {
self.matrix_error.as_ref().map(|me| me.errcode.as_str())
}
}
/// Parses a JSON-encoded Matrix error from the response body
/// Spec reference: <https://spec.matrix.org/v1.10/client-server-api/#standard-error-response>
#[allow(clippy::needless_pass_by_value)]
pub(crate) fn catch_homeserver_error(response: Response<Bytes>) -> HomeserverError {
let matrix_error: Option<MatrixError> = match serde_json::from_slice(response.body().as_ref()) {
Ok(body) => Some(body),
Err(err) => {
debug!("failed to deserialise expected homeserver error: {err:?}");
None
}
};
HomeserverError { matrix_error }
}
impl Display for MatrixError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let MatrixError { errcode, error } = &self;
write!(f, "{errcode}: {error}")
}
}

161
crates/matrix-synapse/src/lib.rs
View File

@@ -14,16 +14,29 @@
#![allow(clippy::blocks_in_conditions)]
use anyhow::{bail, Context};
use http::{header::AUTHORIZATION, request::Builder, Method, Request, StatusCode};
use mas_axum_utils::http_client_factory::HttpClientFactory;
use mas_http::{EmptyBody, HttpServiceExt};
use mas_http::{catch_http_codes, json_response, EmptyBody, HttpServiceExt};
use mas_matrix::{HomeserverConnection, MatrixUser, ProvisionRequest};
use serde::{Deserialize, Serialize};
use tower::{Service, ServiceExt};
use tracing::debug;
use url::Url;
use self::error::catch_homeserver_error;
static SYNAPSE_AUTH_PROVIDER: &str = "oauth-delegated";
/// Encountered when trying to register a user ID which has been taken.
/// — <https://spec.matrix.org/v1.10/client-server-api/#other-error-codes>
const M_USER_IN_USE: &str = "M_USER_IN_USE";
/// Encountered when trying to register a user ID which is not valid.
/// — <https://spec.matrix.org/v1.10/client-server-api/#other-error-codes>
const M_INVALID_USERNAME: &str = "M_INVALID_USERNAME";
mod error;
#[derive(Clone)]
pub struct SynapseConnection {
homeserver: String,
@@ -136,6 +149,13 @@ struct SynapseDeactivateUserRequest {
#[derive(Serialize)]
struct SynapseAllowCrossSigningResetRequest {}
/// Response body of
/// `/_synapse/admin/v1/username_available?username={localpart}`
#[derive(Deserialize)]
struct UsernameAvailableResponse {
available: bool,
}
#[async_trait::async_trait]
impl HomeserverConnection for SynapseConnection {
type Error = anyhow::Error;
@@ -151,7 +171,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.homeserver = self.homeserver,
matrix.mxid = mxid,
),
err(Display),
err(Debug),
)]
async fn query_user(&self, mxid: &str) -> Result<MatrixUser, Self::Error> {
let mxid = urlencoding::encode(mxid);
@@ -159,13 +179,19 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.query_user")
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error)
.json_response();
let request = self
.get(&format!("_synapse/admin/v2/users/{mxid}"))
.body(EmptyBody::new())?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to query user from Synapse")?;
if response.status() != StatusCode::OK {
return Err(anyhow::anyhow!("Failed to query user from Synapse"));
@@ -186,13 +212,16 @@ impl HomeserverConnection for SynapseConnection {
matrix.homeserver = self.homeserver,
matrix.localpart = localpart,
),
err(Display),
err(Debug),
)]
async fn is_localpart_available(&self, localpart: &str) -> Result<bool, Self::Error> {
let localpart = urlencoding::encode(localpart);
let mut client = self
.http_client_factory
.client("homeserver.is_localpart_available");
.client("homeserver.is_localpart_available")
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error)
.json_response::<UsernameAvailableResponse>();
let request = self
.get(&format!(
@@ -200,14 +229,39 @@ impl HomeserverConnection for SynapseConnection {
))
.body(EmptyBody::new())?;
let response = client.ready().await?.call(request).await?;
let response = client.ready().await?.call(request).await;
match response.status() {
StatusCode::OK => Ok(true),
StatusCode::BAD_REQUEST => Ok(false),
_ => Err(anyhow::anyhow!(
"Failed to query localpart availability from Synapse"
)),
match response {
Ok(resp) => {
if !resp.status().is_success() {
// We should have already handled 4xx and 5xx errors by this point
// so anything not 2xx is fairly weird
bail!(
"unexpected response from /username_available: {}",
resp.status()
);
}
Ok(resp.into_body().available)
}
Err(err) => match err {
// Convoluted as... but we want to handle some of the 400 Bad Request responses
// ourselves
json_response::Error::Service {
inner:
catch_http_codes::Error::HttpError {
status_code: StatusCode::BAD_REQUEST,
inner: homeserver_error,
},
} if homeserver_error.errcode() == Some(M_INVALID_USERNAME)
|| homeserver_error.errcode() == Some(M_USER_IN_USE) =>
{
debug!("Username not available: {homeserver_error}");
Ok(false)
}
other_err => Err(anyhow::Error::new(other_err)
.context("Failed to query localpart availability from Synapse")),
},
}
}
@@ -219,7 +273,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.mxid = request.mxid(),
user.id = request.sub(),
),
err(Display),
err(Debug),
)]
async fn provision_user(&self, request: &ProvisionRequest) -> Result<bool, Self::Error> {
let mut body = SynapseUser {
@@ -254,14 +308,21 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.provision_user")
.request_bytes_to_body()
.json_request();
.json_request()
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let mxid = urlencoding::encode(request.mxid());
let request = self
.put(&format!("_synapse/admin/v2/users/{mxid}"))
.body(body)?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to provision user in Synapse")?;
match response.status() {
StatusCode::CREATED => Ok(true),
@@ -281,7 +342,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.mxid = mxid,
matrix.device_id = device_id,
),
err(Display),
err(Debug),
)]
async fn create_device(&self, mxid: &str, device_id: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
@@ -289,13 +350,20 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.create_device")
.request_bytes_to_body()
.json_request();
.json_request()
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let request = self
.post(&format!("_synapse/admin/v2/users/{mxid}/devices"))
.body(SynapseDevice { device_id })?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to create device in Synapse")?;
if response.status() != StatusCode::CREATED {
return Err(anyhow::anyhow!("Failed to create device in Synapse"));
@@ -312,12 +380,16 @@ impl HomeserverConnection for SynapseConnection {
matrix.mxid = mxid,
matrix.device_id = device_id,
),
err(Display),
err(Debug),
)]
async fn delete_device(&self, mxid: &str, device_id: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
let device_id = urlencoding::encode(device_id);
let mut client = self.http_client_factory.client("homeserver.delete_device");
let mut client = self
.http_client_factory
.client("homeserver.delete_device")
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let request = self
.delete(&format!(
@@ -325,7 +397,12 @@ impl HomeserverConnection for SynapseConnection {
))
.body(EmptyBody::new())?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to delete device in Synapse")?;
if response.status() != StatusCode::OK {
return Err(anyhow::anyhow!("Failed to delete device in Synapse"));
@@ -342,7 +419,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.mxid = mxid,
erase = erase,
),
err(Display),
err(Debug),
)]
async fn delete_user(&self, mxid: &str, erase: bool) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
@@ -350,13 +427,20 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.delete_user")
.request_bytes_to_body()
.json_request();
.json_request()
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let request = self
.post(&format!("_synapse/admin/v1/deactivate/{mxid}"))
.body(SynapseDeactivateUserRequest { erase })?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to delete user in Synapse")?;
if response.status() != StatusCode::OK {
return Err(anyhow::anyhow!("Failed to delete user in Synapse"));
@@ -373,7 +457,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.mxid = mxid,
matrix.displayname = displayname,
),
err(Display),
err(Debug),
)]
async fn set_displayname(&self, mxid: &str, displayname: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
@@ -381,13 +465,20 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.set_displayname")
.request_bytes_to_body()
.json_request();
.json_request()
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let request = self
.put(&format!("_matrix/client/v3/profile/{mxid}/displayname"))
.body(SetDisplayNameRequest { displayname })?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to set displayname in Synapse")?;
if response.status() != StatusCode::OK {
return Err(anyhow::anyhow!("Failed to set displayname in Synapse"));
@@ -416,7 +507,7 @@ impl HomeserverConnection for SynapseConnection {
matrix.homeserver = self.homeserver,
matrix.mxid = mxid,
),
err(Display),
err(Debug),
)]
async fn allow_cross_signing_reset(&self, mxid: &str) -> Result<(), Self::Error> {
let mxid = urlencoding::encode(mxid);
@@ -424,7 +515,9 @@ impl HomeserverConnection for SynapseConnection {
.http_client_factory
.client("homeserver.allow_cross_signing_reset")
.request_bytes_to_body()
.json_request();
.json_request()
.response_body_to_bytes()
.catch_http_errors(catch_homeserver_error);
let request = self
.post(&format!(
@@ -432,11 +525,17 @@ impl HomeserverConnection for SynapseConnection {
))
.body(SynapseAllowCrossSigningResetRequest {})?;
let response = client.ready().await?.call(request).await?;
let response = client
.ready()
.await?
.call(request)
.await
.context("Failed to allow cross-signing reset in Synapse")?;
if response.status() != StatusCode::OK {
return Err(anyhow::anyhow!(
"Failed to allow cross signing reset in Synapse"
"Failed to allow cross signing reset in Synapse: {}",
response.status()
));
}

5
templates/pages/error.html
View File

@@ -43,9 +43,8 @@ limitations under the License.
{% if details %}
<hr />
<pre>
<code class="font-mono whitespace-pre-wrap break-all">{{ details }}</code>
</pre>
{# caution: do not introduce whitespace between <pre> and <code> #}
<pre><code class="font-mono whitespace-pre-wrap break-all">{{ details }}</code></pre>
{% endif %}
</main>
{% endblock %}