Skip to content

Commit

Permalink
Merge pull request #60 from exul/refactor-admin-room-validation
Browse files Browse the repository at this point in the history 
Do not use the inviter of a join event to validate and admin room
  • Loading branch information
@exul
exul committed Feb 19, 2018
2 parents a8c5cbd + 4ae4d6d commit aaea73a
Show file tree
Hide file tree
Showing 4 changed files with 14 additions and 136 deletions.
2 changes: 0 additions & 2 deletions assets/translations.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,8 @@ en:
connect_without_rocketchat_server_id: "You have to provide an id to connect to a Rocket.Chat server. It can contain any alphanumeric character and `_`. For example `connect https://rocketchat.example.com my_token rocketchat_example`"
connect_with_invalid_rocketchat_server_id: "The provided Rocket.Chat server ID `${rocketchat_server_id}` is not valid, it can only contain lowercase alphanumeric characters. The maximum length is ${max_rocketchat_server_id_length} characters."
internal: "An internal error occurred"
inviter_unknown: "Could not determine if the admin room is valid, because the inviter is unknown. Possibly because the bot user was invited into an existing room. Please start a direct chat with the bot user ${bot_user_id}."
no_rocketchat_server: "No Rocket.Chat server found when querying ${rocketchat_url} (version information is missing from the response)"
other_user_joined: "Another user join the admin room, leaving, please create a new admin room."
only_room_creator_can_invite_bot_user: "Only the room creator can invite the Rocket.Chat bot user, please create a new room and invite the Rocket.Chat user to create an admin room."
rocketchat_channel_already_bridged: "The channel ${channel_name} is already bridged."
rocketchat_channel_not_found: "No channel with the name ${channel_name} found."
rocketchat_token_missing: "A token is needed to connect new Rocket.Chat servers"
Expand Down
14 changes: 1 addition & 13 deletions src/matrix-rocketchat/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use diesel_migrations::RunMigrationsError;
use iron::{IronError, Response};
use iron::modifier::Modifier;
use iron::status::Status;
use ruma_identifiers::{RoomId, UserId};
use ruma_identifiers::RoomId;
use serde_json;

use i18n::*;
Expand Down Expand Up @@ -311,18 +311,6 @@ error_chain!{
display("Room {} has more then two members and cannot be used as admin room", room_id)
}

InviterUnknown(room_id: RoomId, bot_user_id: UserId) {
description("Inviter for join event was not found")
display("Could not determine if the admin room {} is valid, because the inviter is unknown. \
Please invite the bot user {} to a direct chat.", room_id, bot_user_id)
}

OnlyRoomCreatorCanInviteBotUser(inviter_id: UserId, room_id: RoomId, creator_id: UserId) {
description("Only the room creator can invite the bot user")
display("The bot user was invited by the user {} but the room {} was created by {}, \
bot user is leaving", inviter_id, room_id, creator_id)
}

ConnectionPoolExtractionError {
description("Error when getting the connection pool from the request")
display("Could not get connection pool from iron request")
Expand Down
53 changes: 12 additions & 41 deletions src/matrix-rocketchat/handlers/matrix/membership_handler.rs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
use std::collections::HashMap;
use std::convert::TryFrom;

use diesel::sqlite::SqliteConnection;
use iron::url::Host;
use ruma_events::room::member::{MemberEvent, MembershipState};
use ruma_identifiers::UserId;
use slog::Logger;
use serde_json::{self, Value};

use api::MatrixApi;
use config::Config;
Expand Down Expand Up @@ -59,19 +57,7 @@ impl<'a> MembershipHandler<'a> {
MembershipState::Join if addressed_to_matrix_bot => {
debug!(self.logger, "Received join event for bot user {} and room {}", matrix_bot_user_id, self.room.id);

//TODO: This doesn't seem to work anymore, because the inviter is not part of the unsigned json object anymore
let unsigned: HashMap<String, Value> =
serde_json::from_value(event.unsigned.clone().unwrap_or_default()).unwrap_or_default();
let inviter_id = match unsigned.get("prev_sender") {
Some(prev_sender) => {
let raw_id: String = serde_json::from_value(prev_sender.clone()).unwrap_or_else(|_| "".to_string());
let inviter_id = UserId::try_from(&raw_id).chain_err(|| ErrorKind::InvalidUserId(raw_id))?;
Some(inviter_id)
}
None => None,
};

self.handle_bot_join(matrix_bot_user_id, inviter_id)?;
self.handle_bot_join(matrix_bot_user_id)?;
}
MembershipState::Join => {
debug!(self.logger, "Received join event for user {} and room {}", &state_key, &event.room_id);
Expand Down Expand Up @@ -111,7 +97,7 @@ impl<'a> MembershipHandler<'a> {
Ok(())
}

fn handle_bot_join(&self, matrix_bot_user_id: UserId, inviter_id: Option<UserId>) -> Result<()> {
fn handle_bot_join(&self, matrix_bot_user_id: UserId) -> Result<()> {
let is_admin_room = match self.room.is_admin_room() {
Ok(is_admin_room) => is_admin_room,
Err(err) => {
Expand All @@ -125,7 +111,7 @@ impl<'a> MembershipHandler<'a> {
};

if is_admin_room {
self.setup_admin_room(matrix_bot_user_id.clone(), inviter_id)?;
self.setup_admin_room(matrix_bot_user_id.clone())?;
return Ok(());
}

Expand All @@ -137,30 +123,17 @@ impl<'a> MembershipHandler<'a> {
Ok(())
}

fn setup_admin_room(&self, matrix_bot_user_id: UserId, inviter_id: Option<UserId>) -> Result<()> {
fn setup_admin_room(&self, matrix_bot_user_id: UserId) -> Result<()> {
debug!(self.logger, "Setting up a new admin room with id {}", self.room.id);

let inviter_id = match inviter_id {
Some(inviter_id) => inviter_id,
None => {
info!(self.logger, "Inviter is unknown, bot will leave and forget the room {}", self.room.id);
let bot_user_id = self.config.matrix_bot_user_id()?;
let err = user_error!(
ErrorKind::InviterUnknown(self.room.id.clone(), bot_user_id.clone()),
t!(["errors", "inviter_unknown"]).with_vars(vec![("bot_user_id", bot_user_id.to_string())])
);
self.handle_admin_room_setup_error(&err, matrix_bot_user_id);
return Ok(());
}
};

if let Err(err) = self.is_admin_room_valid(&inviter_id) {
let room_creator_id = self.matrix_api.get_room_creator(self.room.id.clone())?;
if let Err(err) = self.is_admin_room_valid() {
info!(self.logger, "Admin room {} is not valid, bot will leave and forget the room", self.room.id);
self.handle_admin_room_setup_error(&err, matrix_bot_user_id);
return Ok(());
}

match CommandHandler::build_help_message(self.conn, self.room, self.config.as_url.clone(), &inviter_id) {
match CommandHandler::build_help_message(self.conn, self.room, self.config.as_url.clone(), &room_creator_id) {
Ok(body) => {
self.matrix_api.send_text_message_event(self.room.id.clone(), matrix_bot_user_id, body)?;
}
Expand Down Expand Up @@ -203,13 +176,8 @@ impl<'a> MembershipHandler<'a> {
Ok(self.room.id.hostname().ne(&hs_hostname))
}

fn is_admin_room_valid(&self, inviter_id: &UserId) -> Result<()> {
fn is_admin_room_valid(&self) -> Result<()> {
debug!(self.logger, "Validating admin room");
let room_creator_id = self.matrix_api.get_room_creator(self.room.id.clone())?;
if inviter_id != &room_creator_id {
let err = ErrorKind::OnlyRoomCreatorCanInviteBotUser(inviter_id.clone(), self.room.id.clone(), room_creator_id);
bail_error!(err, t!(["errors", "only_room_creator_can_invite_bot_user"]));
}

if !self.is_private_room()? {
bail_error!(ErrorKind::TooManyUsersInAdminRoom(self.room.id.clone()), t!(["errors", "too_many_members_in_room"]));
Expand All @@ -219,7 +187,10 @@ impl<'a> MembershipHandler<'a> {
}

fn is_private_room(&self) -> Result<bool> {
Ok(self.room.user_ids(None)?.len() <= 2)
let room_members_events = self.matrix_api.get_room_members(self.room.id.clone(), None)?;
let mut user_ids: Vec<&UserId> = room_members_events.iter().map(|m| &m.user_id).collect();
user_ids.dedup();
Ok(user_ids.len() <= 2)
}

fn handle_admin_room_setup_error(&self, err: &Error, matrix_bot_user_id: UserId) {
Expand Down
81 changes: 1 addition & 80 deletions tests/admin_room.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -658,48 +658,6 @@ fn accept_invites_from_rooms_on_other_homeservers_if_accept_remote_invites_is_se
assert!(message_received_by_matrix.contains("Hi, I'm the Rocket.Chat application service"));
}

#[test]
fn reject_invites_when_the_inviting_user_is_not_the_room_creator() {
let test = Test::new();
let (message_forwarder, receiver) = MessageForwarder::new();
let mut matrix_router = test.default_matrix_routes();
matrix_router.put(SendMessageEventEndpoint::router_path(), message_forwarder, "send_message_event");
let test = test.with_matrix_routes(matrix_router).run();

helpers::create_room(
&test.config,
"admin_room",
UserId::try_from("@other_user:loalhost").unwrap(),
UserId::try_from("@spec_user:localhost").unwrap(),
);

helpers::join(
&test.config,
RoomId::try_from("!admin_room_id:localhost").unwrap(),
UserId::try_from("@spec_user:localhost").unwrap(),
);

helpers::leave_room(
&test.config,
RoomId::try_from("!admin_room_id:localhost").unwrap(),
UserId::try_from("@other_user:localhost").unwrap(),
);

helpers::invite(
&test.config,
RoomId::try_from("!admin_room_id:localhost").unwrap(),
UserId::try_from("@rocketchat:localhost").unwrap(),
UserId::try_from("@spec_user:localhost").unwrap(),
);

let message_received_by_matrix = receiver.recv_timeout(default_timeout()).unwrap();
assert!(message_received_by_matrix.contains(
"Only the room creator can invite the Rocket.Chat bot user, \
please create a new room and invite the Rocket.Chat user to \
create an admin room.",
));
}

#[test]
fn the_user_gets_a_message_when_getting_the_room_creator_fails() {
let test = Test::new();
Expand Down Expand Up @@ -817,41 +775,4 @@ fn join_events_for_rooms_that_are_not_accessible_by_the_bot_user_are_ignored() {
);

assert!(receiver.recv_timeout(default_timeout()).is_err());
}

#[test]
fn the_bot_user_leaves_the_admin_room_the_inviter_is_unknown() {
let test = Test::new();
let mut matrix_router = test.default_matrix_routes();
let (message_forwarder, receiver) = MessageForwarder::new();
matrix_router.put(SendMessageEventEndpoint::router_path(), message_forwarder, "send_message_event");
let (leave_room, leave_room_receiver) = handlers::MatrixLeaveRoom::with_forwarder(test.config.as_url.clone());
matrix_router.post(LeaveRoomEndpoint::router_path(), leave_room, "leave_room");
let (forget_forwarder, forget_receiver) = MessageForwarder::new();
matrix_router.post(ForgetRoomEndpoint::router_path(), forget_forwarder, "forget");
matrix_router.post(JoinEndpoint::router_path(), handlers::EmptyJson {}, "join_room");
let join_room_handler = handlers::MatrixJoinRoom {
as_url: test.config.as_url.clone(),
send_inviter: false,
};
matrix_router.post(JoinEndpoint::router_path(), join_room_handler, "join_room");

let test = test.with_matrix_routes(matrix_router).run();

helpers::create_room(
&test.config,
"admin_room",
UserId::try_from("@spec_user:localhost").unwrap(),
UserId::try_from("@rocketchat:localhost").unwrap(),
);

let message_received_by_matrix = receiver.recv_timeout(default_timeout()).unwrap();
assert!(message_received_by_matrix.contains(
"Could not determine if the admin room is valid, because the inviter is unknown. \
Possibly because the bot user was invited into an existing room. \
Please start a direct chat with the bot user @rocketchat:localhost",
));

assert!(leave_room_receiver.recv_timeout(default_timeout()).is_ok());
assert!(forget_receiver.recv_timeout(default_timeout()).is_ok());
}
}

0 comments on commit aaea73a

Please sign in to comment.