Update error codes for delegation endpoint (#229)

Author: Johennes

integration-tests/src/fake_homeserver.rs

@@ -74,9 +74,9 @@ struct HsState {
     /// The recorded /delayed_events requests.
     delayed_event_requests: Vec<DelayedEventRequest>,
 
-    /// The delay (in ms) `GET /delayed_events/{delay_id}` reports, keyed by
-    /// delay ID.
-    delays: HashMap<String, i64>,
+    /// The delay in ms and room_id that `GET /delayed_events/{delay_id}` reports,
+    /// keyed by delay ID.
+    delays: HashMap<String, (i64, String)>,
 
     /// The HTTP status to return on delayed-event look-ups, overriding the
     /// scripted delays.
@@ -237,14 +237,14 @@ impl FakeHomeserver {
         self.state.lock().unwrap().delayed_event_requests.clone()
     }
 
-    /// Makes `GET /delayed_events/{delay_id}` report the given delay for
-    /// `delay_id`.
-    pub fn set_delay(&self, delay_id: &str, delay_ms: i64) {
+    /// Makes `GET /delayed_events/{delay_id}` report the given delay and
+    /// room ID for `delay_id`.
+    pub fn set_delay(&self, delay_id: &str, delay_ms: i64, room_id: &str) {
         self.state
             .lock()
             .unwrap()
             .delays
-            .insert(delay_id.to_owned(), delay_ms);
+            .insert(delay_id.to_owned(), (delay_ms, room_id.to_owned()));
     }
 
     /// Sets the HTTP status delayed-event look-ups fail with, regardless of the
@@ -375,11 +375,11 @@ async fn handle_delayed_event_look_up(
     }
 
     match state.delays.get(&delay_id) {
-        Some(delay_ms) => (
+        Some((delay_ms, room_id)) => (
             StatusCode::OK,
             Json(json!({
                 "delay_id": delay_id,
-                "room_id": "!room:example.com",
+                "room_id": room_id,
                 "type": "m.room.member",
                 "delay_ms": delay_ms,
                 "content": {},

integration-tests/tests/delegate_delayed_leave_cs.rs

@@ -330,7 +330,7 @@ async fn restart_and_send_use_identity_assertion() {
 async fn delay_timeout_looked_up_when_absent() {
     let hs = FakeHomeserver::new().await;
     let user = hs.new_user("alice");
-    hs.set_delay("syd_cs_integration_1", 8000);
+    hs.set_delay("syd_cs_integration_1", 8000, "!room:example.com");
 
     let redis = FakeRedis::new().await;
 
@@ -401,7 +401,7 @@ async fn delay_timeout_not_looked_up_when_given() {
 async fn looked_up_delay_drives_the_job() {
     let hs = FakeHomeserver::new().await;
     let user = hs.new_user("alice");
-    hs.set_delay("syd_cs_integration_1", 300);
+    hs.set_delay("syd_cs_integration_1", 300, "!room:example.com");
 
     let svc = Service::start(ServiceConfig {
         full_access_homeservers: vec!["*".to_owned()],
@@ -420,8 +420,8 @@ async fn looked_up_delay_drives_the_job() {
         .await;
 }
 
-/// An unknown delay ID is rejected with 404 M_NOT_FOUND, and no job is
-/// scheduled for it.
+/// An unknown delay ID is rejected with 400 M_INVALID_PARAM per MSC4195, and
+/// no job is scheduled for it.
 #[tokio::test]
 async fn unknown_delay_id_rejected() {
     let hs = FakeHomeserver::new().await;
@@ -440,7 +440,31 @@ async fn unknown_delay_id_rejected() {
     request.as_object_mut().unwrap().remove("delay_timeout");
     let (status, body) = post_delegate_cs(&svc, request.to_string(), Some(&user.user_id)).await;
 
-    expect_matrix_error(status, &body, 404, "M_NOT_FOUND");
+    expect_matrix_error(status, &body, 400, "M_INVALID_PARAM");
+    expect_no_delayed_event_requests(&hs);
+}
+
+/// A delay ID that exists but was scheduled for a different room than the
+/// one being delegated for is rejected.
+#[tokio::test]
+async fn delay_id_for_another_room_rejected() {
+    let hs = FakeHomeserver::new().await;
+    let user = hs.new_user("alice");
+    hs.set_delay("syd_cs_integration_1", 300, "!other-room:example.com");
+
+    let svc = Service::start(ServiceConfig {
+        full_access_homeservers: vec!["*".to_owned()],
+        cs_api_url_overrides: hs.cs_api_url_override(),
+        extra_env: app_service_env_with_hs_server_name(hs.server_name()),
+        ..Default::default()
+    })
+    .await;
+
+    let mut request = delegate_request();
+    request.as_object_mut().unwrap().remove("delay_timeout");
+    let (status, body) = post_delegate_cs(&svc, request.to_string(), Some(&user.user_id)).await;
+
+    expect_matrix_error(status, &body, 400, "M_INVALID_PARAM");
     expect_no_delayed_event_requests(&hs);
 }
 
@@ -450,7 +474,7 @@ async fn unknown_delay_id_rejected() {
 async fn delay_lookup_failure_rejected() {
     let hs = FakeHomeserver::new().await;
     let user = hs.new_user("alice");
-    hs.set_delay("syd_cs_integration_1", 8000);
+    hs.set_delay("syd_cs_integration_1", 8000, "!room:example.com");
     hs.set_delay_lookup_status(500);
 
     let svc = Service::start(ServiceConfig {

src/handler.rs

@@ -718,17 +718,20 @@ impl Handler {
     }
 
     /// Looks up the delay of `delay_id` on the homeserver, for delegation
-    /// requests that do not carry one.
+    /// requests that do not carry one. Also verifies that the delayed event
+    /// was scheduled in `room_id`.
     async fn look_up_delay_timeout(
         &self,
         cs_api_url: &CsApiUrl,
         delay_id: &str,
+        room_id: &str,
         owner_user_id: &str,
     ) -> Result<Duration, MatrixErrorResponse> {
         self.deps
             .get_delayed_event_delay(
                 cs_api_url,
                 delay_id,
+                room_id,
                 AppServiceIdentity {
                     as_token: &self.app_service_config.as_token,
                     user_id: owner_user_id,
@@ -740,8 +743,8 @@ impl Handler {
                     "Handler: could not look up the delay of the delayed event");
                 if err.is_delayed_event_not_found() {
                     MatrixErrorResponse {
-                        status: 404,
-                        errcode: "M_NOT_FOUND".into(),
+                        status: 400,
+                        errcode: "M_INVALID_PARAM".into(),
                         err: "Unknown `delay_id`".into(),
                     }
                 } else {
@@ -1240,7 +1243,7 @@ impl Handler {
         let delay_timeout = match req.delay_timeout {
             Some(timeout) => Duration::from_millis(timeout.max(0) as u64),
             None => {
-                self.look_up_delay_timeout(&cs_api_url, &req.delay_id, mxid_header)
+                self.look_up_delay_timeout(&cs_api_url, &req.delay_id, &req.room_id, mxid_header)
                     .await?
             }
         };

src/handler_tests.rs

@@ -39,7 +39,7 @@ type ExecuteDelayedEventActionFn = Box<
     dyn Fn(&CsApiUrl, &str, DelayEventAction, &str, &str) -> Result<u16, ActionError> + Send + Sync,
 >;
 type GetDelayedEventDelayFn =
-    Box<dyn Fn(&CsApiUrl, &str, &str, &str) -> Result<Duration, ActionError> + Send + Sync>;
+    Box<dyn Fn(&CsApiUrl, &str, &str, &str, &str) -> Result<Duration, ActionError> + Send + Sync>;
 type IsJoinedFn = Box<dyn Fn(&CsApiUrl, &str, &str) -> Result<bool, String> + Send + Sync>;
 type RequestGetTokenViaFederationFn = Box<
     dyn Fn(&CsApiUrl, &str, &GetTokenSsRequest) -> Result<GetTokenSsResponse, FederationTokenError>
@@ -143,10 +143,17 @@ impl Deps for HandlerTestDeps {
         &self,
         cs_api_url: &CsApiUrl,
         delay_id: &str,
+        room_id: &str,
         identity: AppServiceIdentity<'_>,
     ) -> Result<Duration, ActionError> {
         match &self.get_delayed_event_delay_fn {
-            Some(f) => f(cs_api_url, delay_id, identity.as_token, identity.user_id),
+            Some(f) => f(
+                cs_api_url,
+                delay_id,
+                room_id,
+                identity.as_token,
+                identity.user_id,
+            ),
             None => panic!("get_delayed_event_delay not mocked in HandlerTestDeps"),
         }
     }
@@ -4182,11 +4189,12 @@ async fn test_process_delegate_delayed_leave_cs_restart_uses_identity_assertion(
 }
 
 /// A request without a delay timeout makes the service look the delay up on
-/// the homeserver, asserting the caller's identity as it does so.
+/// the homeserver, asserting the caller's identity and the request's
+/// `room_id` as it does so.
 #[tokio::test]
 async fn test_process_delegate_delayed_leave_cs_looks_up_missing_delay_timeout() {
-    /// (CS API URL, delay ID, as_token, user_id) of the recorded lookup.
-    type Lookup = (String, String, String, String);
+    /// (CS API URL, delay ID, room ID, as_token, user_id) of the recorded lookup.
+    type Lookup = (String, String, String, String, String);
     let looked_up: Arc<Mutex<Option<Lookup>>> = Arc::new(Mutex::new(None));
     let looked_up_clone = looked_up.clone();
     let deps = HandlerTestDeps {
@@ -4195,10 +4203,11 @@ async fn test_process_delegate_delayed_leave_cs_looks_up_missing_delay_timeout()
         })),
         participant_exists_fn: participant_exists_block_until_cancelled(),
         get_delayed_event_delay_fn: Some(Box::new(
-            move |cs_api_url, delay_id, as_token, user_id| {
+            move |cs_api_url, delay_id, room_id, as_token, user_id| {
                 *looked_up_clone.lock().unwrap() = Some((
                     cs_api_url.0.clone(),
                     delay_id.to_owned(),
+                    room_id.to_owned(),
                     as_token.to_owned(),
                     user_id.to_owned(),
                 ));
@@ -4216,13 +4225,14 @@ async fn test_process_delegate_delayed_leave_cs_looks_up_missing_delay_timeout()
         .await
         .expect("unexpected error");
 
-    let (cs_api_url, delay_id, as_token, user_id) = looked_up
+    let (cs_api_url, delay_id, room_id, as_token, user_id) = looked_up
         .lock()
         .unwrap()
         .clone()
         .expect("expected the delay to be looked up");
     assert_eq!(cs_api_url, "https://matrix-client.example.com");
     assert_eq!(delay_id, req.delay_id, "expected the requested delay_id");
+    assert_eq!(room_id, req.room_id, "expected the requested room_id");
     assert_eq!(as_token, "as_token", "expected the configured as_token");
     assert_eq!(
         user_id, DELEGATE_DELAYED_LEAVE_CS_MXID,
@@ -4245,7 +4255,7 @@ async fn test_process_delegate_delayed_leave_cs_looked_up_delay_drives_the_job()
         // Confirmed absent, so the job stays in WaitingForInitialConnect until
         // its timeout — which is the looked-up delay — elapses.
         participant_exists_fn: Some(Box::new(|_, _| Box::pin(async { Ok(false) }))),
-        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _| Ok(Duration::from_millis(200)))),
+        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _, _| Ok(Duration::from_millis(200)))),
         execute_delayed_event_action_fn: Some(Box::new(move |_, _, action, _, _| {
             if action == DelayEventAction::Send {
                 *sent_clone.lock().unwrap() = true;
@@ -4282,7 +4292,7 @@ async fn test_process_delegate_delayed_leave_cs_skips_lookup_when_delay_timeout_
             Ok(CsApiUrl("https://matrix-client.example.com".into()))
         })),
         participant_exists_fn: participant_exists_block_until_cancelled(),
-        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _| {
+        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _, _| {
             panic!("the delay must not be looked up when the request carries one")
         })),
         ..Default::default()
@@ -4298,14 +4308,14 @@ async fn test_process_delegate_delayed_leave_cs_skips_lookup_when_delay_timeout_
     handler.close().await;
 }
 
-/// An unknown delay_id surfaces as 404 M_NOT_FOUND.
+/// An unknown delay_id is rejected as HTTP 400 / M_INVALID_PARAM.
 #[tokio::test]
 async fn test_process_delegate_delayed_leave_cs_delay_lookup_not_found() {
     let deps = HandlerTestDeps {
         resolve_cs_api_url_fn: Some(Box::new(|_| {
             Ok(CsApiUrl("https://matrix-client.example.com".into()))
         })),
-        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _| {
+        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _, _| {
             Err(ActionError::DelayedEventNotFound { status: 404 })
         })),
         ..Default::default()
@@ -4318,8 +4328,8 @@ async fn test_process_delegate_delayed_leave_cs_delay_lookup_not_found() {
         .process_delegate_delayed_leave_cs(&req, DELEGATE_DELAYED_LEAVE_CS_MXID)
         .await
         .expect_err("expected MatrixErrorResponse");
-    assert_eq!(err.status, 404, "expected 404");
-    assert_eq!(err.errcode, "M_NOT_FOUND", "expected M_NOT_FOUND");
+    assert_eq!(err.status, 400, "expected 400");
+    assert_eq!(err.errcode, "M_INVALID_PARAM", "expected M_INVALID_PARAM");
     handler.close().await;
 }
 
@@ -4331,7 +4341,7 @@ async fn test_process_delegate_delayed_leave_cs_delay_lookup_unavailable() {
         resolve_cs_api_url_fn: Some(Box::new(|_| {
             Ok(CsApiUrl("https://matrix-client.example.com".into()))
         })),
-        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _| {
+        get_delayed_event_delay_fn: Some(Box::new(|_, _, _, _, _| {
             Err(ActionError::Transient {
                 status: 500,
                 msg: "boom".into(),