appservice: Properly scope webserver configuration

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<warp::Rejection> for Error {
+    fn from(rejection: warp::Rejection) -> Self {
+        Self::WarpRejection(format!("{:?}", rejection))
+    }
+}

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 webserver;
-mod warp;
 
 pub type Result<T> = std::result::Result<T, Error>;
 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 {
+    pub fn actix_configure(&self) -> impl FnOnce(&mut actix_web::web::ServiceConfig) {
-        actix::get_scope().data(self.clone())
+        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,)> {
+    pub fn warp_filter(&self) -> warp::filters::BoxedFilter<(impl warp::Reply,)> {
-        crate::warp::warp_filter(self.clone())
+        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(())
         }
 

matrix_sdk_appservice/src/webserver/actix.rs

@@ -33,7 +33,7 @@ pub async fn run_server(
     host: impl Into<String>,
     port: impl Into<u16>,
 ) -> 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 {
+pub fn configure(config: &mut actix_web::web::ServiceConfig) {
-    gen_scope("/"). // handle legacy routes
+    // also handles legacy routes
-    service(gen_scope("/_matrix/app/v1"))
+    config.service(push_transactions).service(query_user_id).service(query_room_alias).service(
-}
+        web::scope("/_matrix/app/v1")
-
+            .service(push_transactions)
-fn gen_scope(scope: &str) -> Scope {
+            .service(query_user_id)
-    web::scope(scope).service(push_transactions).service(query_user_id).service(query_room_alias)
+            .service(query_room_alias),
+    );
 }
 
 #[tracing::instrument]

matrix_sdk_appservice/src/webserver/mod.rs

@@ -0,0 +1,4 @@
+#[cfg(feature = "actix")]
+pub mod actix;
+#[cfg(feature = "warp")]
+pub mod warp;

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(
+pub async fn handle_rejection(err: Rejection) -> std::result::Result<impl Reply, Rejection> {
-    err: Rejection,
-) -> std::result::Result<impl Reply, std::convert::Infallible> {
-    let mut code = http::StatusCode::INTERNAL_SERVER_ERROR;
-    let mut message = "INTERNAL_SERVER_ERROR";
-
     if err.find::<Unauthorized>().is_some() || err.find::<warp::reject::InvalidQuery>().is_some() {
-        code = http::StatusCode::UNAUTHORIZED;
+        let code = http::StatusCode::UNAUTHORIZED;
-        message = "UNAUTHORIZED";
+        let message = "UNAUTHORIZED";
-    }
-
-    let json = warp::reply::json(&ErrorMessage { code: code.as_u16(), message: message.into() });
 
+        let json =
+            warp::reply::json(&ErrorMessage { code: code.as_u16(), message: message.into() });
         Ok(warp::reply::with_status(json, code))
+    } else {
+        Err(err)
+    }
 }

matrix_sdk_appservice/tests/tests.rs

@@ -1,10 +1,7 @@
-use std::{
+use std::sync::{Arc, Mutex};
-    env,
-    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<Registration>) -> Result<Appservice> {
-    env::set_var(
+    // env::set_var(
-        "RUST_LOG",
+    //     "RUST_LOG",
-        "mockito=debug,matrix_sdk=debug,ruma=debug,actix_web=debug,warp=debug",
+    //     "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::*;