From d6ca3a27bbda34320daa94f1a7139336dd382bc7 Mon Sep 17 00:00:00 2001 From: Johannes Becker Date: Mon, 7 Jun 2021 16:52:21 +0200 Subject: [PATCH] appservice: Properly scope webserver configuration --- matrix_sdk_appservice/src/error.rs | 11 +++ matrix_sdk_appservice/src/lib.rs | 38 ++++++---- .../src/{ => webserver}/actix.rs | 17 ++--- matrix_sdk_appservice/src/webserver/mod.rs | 4 ++ .../src/{ => webserver}/warp.rs | 25 ++++--- matrix_sdk_appservice/tests/tests.rs | 72 ++++++++++++++----- 6 files changed, 116 insertions(+), 51 deletions(-) rename matrix_sdk_appservice/src/{ => webserver}/actix.rs (90%) create mode 100644 matrix_sdk_appservice/src/webserver/mod.rs rename matrix_sdk_appservice/src/{ => webserver}/warp.rs (91%) diff --git a/matrix_sdk_appservice/src/error.rs b/matrix_sdk_appservice/src/error.rs index 73a6bc7b..1e08c192 100644 --- a/matrix_sdk_appservice/src/error.rs +++ b/matrix_sdk_appservice/src/error.rs @@ -67,6 +67,10 @@ pub enum Error { #[error(transparent)] SerdeJson(#[from] serde_json::Error), + #[cfg(feature = "warp")] + #[error("warp rejection: {0}")] + WarpRejection(String), + #[cfg(feature = "actix")] #[error(transparent)] Actix(#[from] actix_web::Error), @@ -81,3 +85,10 @@ impl actix_web::error::ResponseError for Error {} #[cfg(feature = "warp")] impl warp::reject::Reject for Error {} + +#[cfg(feature = "warp")] +impl From for Error { + fn from(rejection: warp::Rejection) -> Self { + Self::WarpRejection(format!("{:?}", rejection)) + } +} diff --git a/matrix_sdk_appservice/src/lib.rs b/matrix_sdk_appservice/src/lib.rs index 51221222..40c429f5 100644 --- a/matrix_sdk_appservice/src/lib.rs +++ b/matrix_sdk_appservice/src/lib.rs @@ -105,11 +105,8 @@ use ruma::{ }; use tracing::{info, warn}; -#[cfg(feature = "actix")] -mod actix; mod error; -#[cfg(feature = "warp")] -mod warp; +mod webserver; pub type Result = std::result::Result; pub type Host = String; @@ -427,18 +424,35 @@ impl Appservice { Ok(false) } - /// [`actix_web::Scope`] to be used with [`actix_web::App::service()`] + /// Returns a closure to be used with [`actix_web::App::configure()`] + /// + /// Note that if you handle any of the [application-service-specific + /// routes], including the legacy routes, you will break the appservice + /// functionality. + /// + /// [application-service-specific routes]: https://spec.matrix.org/unstable/application-service-api/#legacy-routes #[cfg(feature = "actix")] #[cfg_attr(docs, doc(cfg(feature = "actix")))] - pub fn actix_service(&self) -> actix::Scope { - actix::get_scope().data(self.clone()) + pub fn actix_configure(&self) -> impl FnOnce(&mut actix_web::web::ServiceConfig) { + let appservice = self.clone(); + + move |config| { + config.data(appservice); + webserver::actix::configure(config); + } } - /// [`::warp::Filter`] to be used as warp serve route + /// Returns a [`warp::Filter`] to be used as [`warp::serve()`] route + /// + /// Note that if you handle any of the [application-service-specific + /// routes], including the legacy routes, you will break the appservice + /// functionality. + /// + /// [application-service-specific routes]: https://spec.matrix.org/unstable/application-service-api/#legacy-routes #[cfg(feature = "warp")] #[cfg_attr(docs, doc(cfg(feature = "warp")))] - pub fn warp_filter(&self) -> ::warp::filters::BoxedFilter<(impl ::warp::Reply,)> { - crate::warp::warp_filter(self.clone()) + pub fn warp_filter(&self) -> warp::filters::BoxedFilter<(impl warp::Reply,)> { + webserver::warp::warp_filter(self.clone()) } /// Convenience method that runs an http server depending on the selected @@ -453,13 +467,13 @@ impl Appservice { #[cfg(feature = "actix")] { - actix::run_server(self.clone(), host, port).await?; + webserver::actix::run_server(self.clone(), host, port).await?; Ok(()) } #[cfg(feature = "warp")] { - warp::run_server(self.clone(), host, port).await?; + webserver::warp::run_server(self.clone(), host, port).await?; Ok(()) } diff --git a/matrix_sdk_appservice/src/actix.rs b/matrix_sdk_appservice/src/webserver/actix.rs similarity index 90% rename from matrix_sdk_appservice/src/actix.rs rename to matrix_sdk_appservice/src/webserver/actix.rs index 9228dcf0..b983f92e 100644 --- a/matrix_sdk_appservice/src/actix.rs +++ b/matrix_sdk_appservice/src/webserver/actix.rs @@ -33,7 +33,7 @@ pub async fn run_server( host: impl Into, port: impl Into, ) -> Result<(), Error> { - HttpServer::new(move || App::new().service(appservice.actix_service())) + HttpServer::new(move || App::new().configure(appservice.actix_configure())) .bind((host.into(), port.into()))? .run() .await?; @@ -41,13 +41,14 @@ pub async fn run_server( Ok(()) } -pub fn get_scope() -> Scope { - gen_scope("/"). // handle legacy routes - service(gen_scope("/_matrix/app/v1")) -} - -fn gen_scope(scope: &str) -> Scope { - web::scope(scope).service(push_transactions).service(query_user_id).service(query_room_alias) +pub fn configure(config: &mut actix_web::web::ServiceConfig) { + // also handles legacy routes + config.service(push_transactions).service(query_user_id).service(query_room_alias).service( + web::scope("/_matrix/app/v1") + .service(push_transactions) + .service(query_user_id) + .service(query_room_alias), + ); } #[tracing::instrument] diff --git a/matrix_sdk_appservice/src/webserver/mod.rs b/matrix_sdk_appservice/src/webserver/mod.rs new file mode 100644 index 00000000..a0880ec6 --- /dev/null +++ b/matrix_sdk_appservice/src/webserver/mod.rs @@ -0,0 +1,4 @@ +#[cfg(feature = "actix")] +pub mod actix; +#[cfg(feature = "warp")] +pub mod warp; diff --git a/matrix_sdk_appservice/src/warp.rs b/matrix_sdk_appservice/src/webserver/warp.rs similarity index 91% rename from matrix_sdk_appservice/src/warp.rs rename to matrix_sdk_appservice/src/webserver/warp.rs index ff9cc7fc..d0e1c7cb 100644 --- a/matrix_sdk_appservice/src/warp.rs +++ b/matrix_sdk_appservice/src/webserver/warp.rs @@ -38,8 +38,9 @@ pub async fn run_server( } pub fn warp_filter(appservice: Appservice) -> BoxedFilter<(impl Reply,)> { + // TODO: try to use a struct instead of cloning appservice before `warp::path!` + // matching warp::any() - .and(filters::valid_access_token(appservice.registration().hs_token.clone())) .and(filters::transactions(appservice)) .or(filters::users()) .or(filters::rooms()) @@ -82,6 +83,7 @@ mod filters { .or(warp::path!("transactions" / String)) .unify(), ) + .and(filters::valid_access_token(appservice.registration().hs_token.clone())) .and(with_appservice(appservice)) .and(http_request().and_then(|request| async move { let request = crate::transform_legacy_route(request).map_err(Error::from)?; @@ -181,18 +183,15 @@ struct ErrorMessage { message: String, } -pub async fn handle_rejection( - err: Rejection, -) -> std::result::Result { - let mut code = http::StatusCode::INTERNAL_SERVER_ERROR; - let mut message = "INTERNAL_SERVER_ERROR"; - +pub async fn handle_rejection(err: Rejection) -> std::result::Result { if err.find::().is_some() || err.find::().is_some() { - code = http::StatusCode::UNAUTHORIZED; - message = "UNAUTHORIZED"; + let code = http::StatusCode::UNAUTHORIZED; + let message = "UNAUTHORIZED"; + + let json = + warp::reply::json(&ErrorMessage { code: code.as_u16(), message: message.into() }); + Ok(warp::reply::with_status(json, code)) + } else { + Err(err) } - - let json = warp::reply::json(&ErrorMessage { code: code.as_u16(), message: message.into() }); - - Ok(warp::reply::with_status(json, code)) } diff --git a/matrix_sdk_appservice/tests/tests.rs b/matrix_sdk_appservice/tests/tests.rs index 25635aff..371bc729 100644 --- a/matrix_sdk_appservice/tests/tests.rs +++ b/matrix_sdk_appservice/tests/tests.rs @@ -1,10 +1,7 @@ -use std::{ - env, - sync::{Arc, Mutex}, -}; +use std::sync::{Arc, Mutex}; #[cfg(feature = "actix")] -use actix_web::{test as actix_test, App as ActixApp}; +use actix_web::{test as actix_test, App as ActixApp, HttpResponse}; use matrix_sdk::{ api_appservice::Registration, async_trait, @@ -17,17 +14,17 @@ use matrix_sdk_test::{appservice::TransactionBuilder, async_test, EventsJson}; use sdk::{ClientConfig, RequestConfig}; use serde_json::json; #[cfg(feature = "warp")] -use warp::Reply; +use warp::{Filter, Reply}; fn registration_string() -> String { include_str!("../tests/registration.yaml").to_owned() } async fn appservice(registration: Option) -> Result { - env::set_var( - "RUST_LOG", - "mockito=debug,matrix_sdk=debug,ruma=debug,actix_web=debug,warp=debug", - ); + // env::set_var( + // "RUST_LOG", + // "mockito=debug,matrix_sdk=debug,ruma=debug,actix_web=debug,warp=debug", + // ); let _ = tracing_subscriber::fmt::try_init(); let registration = match registration { @@ -104,7 +101,7 @@ async fn test_put_transaction() -> Result<()> { #[cfg(feature = "actix")] let status = { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::put().uri(uri).set_json(&transaction).to_request(); @@ -135,7 +132,7 @@ async fn test_get_user() -> Result<()> { #[cfg(feature = "actix")] let status = { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::get().uri(uri).to_request(); @@ -166,7 +163,7 @@ async fn test_get_room() -> Result<()> { #[cfg(feature = "actix")] let status = { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::get().uri(uri).to_request(); @@ -202,7 +199,7 @@ async fn test_invalid_access_token() -> Result<()> { #[cfg(feature = "actix")] let status = { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::put().uri(uri).set_json(&transaction).to_request(); @@ -242,7 +239,7 @@ async fn test_no_access_token() -> Result<()> { #[cfg(feature = "actix")] { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::put().uri(uri).set_json(&transaction).to_request(); @@ -267,7 +264,7 @@ async fn test_event_handler() -> Result<()> { impl Example { pub fn new() -> Self { - #[allow(clippy::mutex::mutex_atomic)] + #[allow(clippy::mutex_atomic)] Self { on_state_member: Arc::new(Mutex::new(false)) } } } @@ -299,9 +296,9 @@ async fn test_event_handler() -> Result<()> { .unwrap(); #[cfg(feature = "actix")] - let status = { + { let app = - actix_test::init_service(ActixApp::new().service(appservice.actix_service())).await; + actix_test::init_service(ActixApp::new().configure(appservice.actix_configure())).await; let req = actix_test::TestRequest::put().uri(uri).set_json(&transaction).to_request(); @@ -314,6 +311,45 @@ async fn test_event_handler() -> Result<()> { Ok(()) } +#[async_test] +async fn test_unrelated_path() -> Result<()> { + let appservice = appservice(None).await?; + + #[cfg(feature = "warp")] + let status = { + let consumer_filter = warp::any() + .and(appservice.warp_filter()) + .or(warp::get().and(warp::path("unrelated").map(|| warp::reply()))); + + let response = warp::test::request() + .method("GET") + .path("/unrelated") + .filter(&consumer_filter) + .await? + .into_response(); + + response.status() + }; + + #[cfg(feature = "actix")] + let status = { + let app = actix_test::init_service( + ActixApp::new() + .configure(appservice.actix_configure()) + .route("/unrelated", actix_web::web::get().to(HttpResponse::Ok)), + ) + .await; + + let req = actix_test::TestRequest::get().uri("/unrelated").to_request(); + + actix_test::call_service(&app, req).await.status() + }; + + assert_eq!(status, 200); + + Ok(()) +} + mod registration { use super::*;