feat: appservice #204

johannescpk · 2021-04-15T08:05:48Z

Follow-up tracking issue: #228

Todo

The general idea is that incoming appservice transactions from the homeserver are translated into an api::client::r0::sync::sync_events::Response and then passed into receive_sync_response.

The user associated with the client Session is the "main appservice user" (sender_localpart in the registration). This allows for the state store to properly propagated and an API surface similar to a regular client while still handling most events coming in over transactions:

let config = AppserviceConfig::new(homeserver_url, server_name, appservice_registration)?;
let appservice = Appservice::new(config).await?;

struct ExampleHandler {}
impl ExampleHandler {
    pub fn new() -> Self {
        Self {}
    }
}

#[async_trait]
impl EventHandler for ExampleHandler {
    async fn on_state_member(
        &self,
        room: Room,
        event: &SyncStateEvent<MemberEventContent>,
    ) {
        dbg!(room, event);
    }
}

appservice
    .client()
    .set_event_handler(Box::new(Example::new()))
    .await;

appservice.receive_transaction(incoming_transaction).await;

This draft only handles AnyStateEvent::RoomMember with MembershipState::Join for now. If the general direction sounds like something you'd consider supporting I'd keep completing the approach.

There's also support in ruma now to assign user_ids to OutgoingRequests, so another API surface that I'd like to add is something like appservice.client_with_user_id() which returns a client that attaches the given user_id to all requests.

Another topic is whether the SDK wants to expose the actual webserver that receives HTTP requests from the homeserver. Currently there's only an actix example included, but it could be considered to expose it behind a feature flag like appservice-actix or something.

And finally, this isn't E2EE-ready. I guess it would need a crypto store instance for every virtual appservice user.

poljar

Generally this looks really neat. I know this is an initial version but I pointed out that we should be a bit better with the docs anyways.

I need to double check if it's ok to have a crate in the repo that I'm technically not maintaining myself, and after we fix those couple small nits we can merge this.

poljar · 2021-05-04T12:16:18Z

matrix_sdk/src/http_client.rs

+            // this should be unreachable since assert_identity on request_config can only be set
+            // with the appservice feature active
+            #[cfg(not(feature = "appservice"))]
+            return Err(HttpError::NeedsAppserviceFeature);


Hmm it feels a bit wrong to have an error that never gets returned. Can't we restructure that somehow so we don't need this error?

Changed to cfg-based feature-switch, also allows restricting all assert_identity things to the appservice feature. The catch here is that because of crate-interdependence currently the appservice feature seems to be always enabled when running commands in the crate root. So cargo test in the root will run with the appservice feature in matrix-sdk active. Something we could do is running cargo test --workspace --exclude matrix-sdk-appservice in CI and the appservice tests with an additional command? I'll open a PR to rework the test-features CI pipeline and try how that would look.

I've integrated the CI changes here directly, with #226 being a possible cleanup to reduce CI time

matrix_sdk/src/client.rs