Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Appservice virt member test #747

Merged
merged 2 commits into from
Jun 14, 2022
Merged

Appservice virt member test #747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
688f7e3
fix(appservice): Virtual client non-membership
agraven Jun 9, 2022
9bd9b46
test(appservice): Virtual client membership
agraven Jun 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions crates/matrix-sdk-appservice/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ impl<'a> VirtualUserBuilder<'a> {
}

let user_id = UserId::parse_with_server_name(self.localpart, &self.appservice.server_name)?;
if !(self.appservice.user_id_is_in_namespace(&user_id)?
|| self.localpart == self.appservice.registration.sender_localpart)
{
warn!("Virtual client id '{user_id}' is not in the namespace")
}

let mut builder = self.client_builder;

Expand Down Expand Up @@ -685,11 +690,12 @@ impl AppService {
let client = client.clone();
let virt_client = virt_client.clone();
let transaction = transaction.clone();
let appserv_uid = self.registration.sender_localpart.clone();

let task = tokio::spawn(async move {
let user_id = match virt_client.user_id() {
Some(user_id) => user_id.localpart(),
// Unauthenticated client. (should that be possible?)
// The client is not logged in, skipping
None => return Ok(()),
};
let mut response = sync_events::v3::Response::new(transaction.txn_id.to_string());
Expand All @@ -709,25 +715,28 @@ impl AppService {
let key = &[USER_MEMBER, room_id.as_bytes(), b".", user_id.as_bytes()].concat();
let membership = match client.store().get_custom_value(key).await? {
Some(value) => String::from_utf8(value).ok().map(MembershipState::from),
// Assume the appservice is in every known room
None if user_id == appserv_uid => Some(MembershipState::Join),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like a safe assumption that if the appservice receives events for a given room, the main client is already a member of it and we just missed it or haven't processed the relevant membership event yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems like a save assumption. A proper fix would be to force a full state sync if we see events for a room the first time, but that's out of scope for this PR.

None => None,
};

match membership.unwrap_or(MembershipState::Join) {
MembershipState::Join => {
match membership {
Some(MembershipState::Join) => {
let room = response.rooms.join.entry(room_id).or_default();
room.timeline.events.push(raw_event.clone().cast())
}
MembershipState::Leave | MembershipState::Ban => {
Some(MembershipState::Leave | MembershipState::Ban) => {
let room = response.rooms.leave.entry(room_id).or_default();
room.timeline.events.push(raw_event.clone().cast())
}
MembershipState::Knock => {
Some(MembershipState::Knock) => {
response.rooms.knock.entry(room_id).or_default();
}
MembershipState::Invite => {
Some(MembershipState::Invite) => {
response.rooms.invite.entry(room_id).or_default();
}
_ => debug!("Membership type we don't know how to handle"),
Some(unknown) => debug!("Unknown membership type: {unknown}"),
None => debug!("Assuming {user_id} is not in {room_id}"),
}
}
virt_client.receive_transaction(&transaction.txn_id, response).await?;
Expand Down
106 changes: 105 additions & 1 deletion crates/matrix-sdk-appservice/tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ use matrix_sdk::{
};
use matrix_sdk_appservice::*;
use matrix_sdk_test::{appservice::TransactionBuilder, async_test, EventsJson};
use ruma::{api::MatrixVersion, room_id};
use ruma::{
api::{appservice::event::push_events, MatrixVersion},
events::AnyRoomEvent,
room_id,
serde::Raw,
};
use serde_json::json;
use warp::{Filter, Reply};

Expand Down Expand Up @@ -299,6 +304,105 @@ async fn test_appservice_on_sub_path() -> Result<()> {
Ok(())
}

#[async_test]
async fn test_receive_transaction() -> Result<()> {
tracing_subscriber::fmt().try_init().ok();
let json = vec![
Raw::new(&json!({
"content": {
"avatar_url": null,
"displayname": "Appservice",
"membership": "join"
},
"event_id": "$151800140479rdvjg:localhost",
"membership": "join",
"origin_server_ts": 151800140,
"sender": "@_appservice:localhost",
"state_key": "@_appservice:localhost",
"type": "m.room.member",
"room_id": "!coolplace:localhost",
"unsigned": {
"age": 2970366
}
}))?
.cast::<AnyRoomEvent>(),
Raw::new(&json!({
"content": {
"avatar_url": null,
"displayname": "Appservice",
"membership": "join"
},
"event_id": "$151800140491rfbja:localhost",
"membership": "join",
"origin_server_ts": 151800140,
"sender": "@_appservice:localhost",
"state_key": "@_appservice:localhost",
"type": "m.room.member",
"room_id": "!boringplace:localhost",
"unsigned": {
"age": 2970366
}
}))?
.cast::<AnyRoomEvent>(),
Raw::new(&json!({
"content": {
"avatar_url": null,
"displayname": "Alice",
"membership": "join"
},
"event_id": "$151800140517rfvjc:localhost",
"membership": "join",
"origin_server_ts": 151800140,
"sender": "@_appservice_alice:localhost",
"state_key": "@_appservice_alice:localhost",
"type": "m.room.member",
"room_id": "!coolplace:localhost",
"unsigned": {
"age": 2970366
}
}))?
.cast::<AnyRoomEvent>(),
Raw::new(&json!({
"content": {
"avatar_url": null,
"displayname": "Bob",
"membership": "invite"
},
"event_id": "$151800140594rfvjc:localhost",
"membership": "invite",
"origin_server_ts": 151800174,
"sender": "@_appservice_bob:localhost",
"state_key": "@_appservice_bob:localhost",
"type": "m.room.member",
"room_id": "!boringplace:localhost",
"unsigned": {
"age": 2970366
}
}))?
.cast::<AnyRoomEvent>(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure where to put the test json, as the needed Vec<Raw> form was slightly different than usual.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe into https://github.com/matrix-org/matrix-rust-sdk/blob/main/crates/matrix-sdk-test/src/appservice.rs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most cases I prefer to put the JSON close to the test, even at the cost of some duplication. Otherwise it makes it harder to keep in mind which JSON we're using and what assumptions it carries with it.

];
let appservice = appservice(None).await?;

let alice = appservice.virtual_user_client("_appservice_alice").await?;
let bob = appservice.virtual_user_client("_appservice_bob").await?;
appservice
.receive_transaction(push_events::v1::IncomingRequest::new("dontcare".into(), json))
.await?;
let coolplace = room_id!("!coolplace:localhost");
let boringplace = room_id!("!boringplace:localhost");
assert!(
alice.get_joined_room(coolplace).is_some(),
"Alice's membership in coolplace should be join"
);
assert!(
bob.get_invited_room(boringplace).is_some(),
"Bob's membership in boringplace should be invite"
);
assert!(alice.get_room(boringplace).is_none(), "Alice should not know about boringplace");
assert!(bob.get_room(coolplace).is_none(), "Bob should not know about coolplace");
Ok(())
}

mod registration {
use super::*;

Expand Down