From bb24f6ad90513326582a908672543749571a2054 Mon Sep 17 00:00:00 2001 From: Devin Ragotzy Date: Sun, 15 Nov 2020 16:48:43 -0500 Subject: [PATCH] Address some review issues fmt, errors, comments --- src/client_server/backup.rs | 2 +- src/client_server/media.rs | 2 +- src/client_server/membership.rs | 2 -- src/database/rooms.rs | 15 +++++++++++- src/database/sending.rs | 6 ++--- src/ruma_wrapper.rs | 41 +++++++++++++++++---------------- src/server_server.rs | 21 ++++++++--------- 7 files changed, 50 insertions(+), 39 deletions(-) diff --git a/src/client_server/backup.rs b/src/client_server/backup.rs index 607fa39e..676b5a3f 100644 --- a/src/client_server/backup.rs +++ b/src/client_server/backup.rs @@ -256,7 +256,7 @@ pub async fn get_backup_key_session_route( let key_data = db .key_backups .get_session(&sender_user, &body.version, &body.room_id, &body.session_id)? - .ok_or_else(|| Error::BadDatabase("Backup key not found for this user's session"))?; + .ok_or_else(|| Error::bad_database("Backup key not found for this user's session."))?; Ok(get_backup_key_session::Response { key_data }.into()) } diff --git a/src/client_server/media.rs b/src/client_server/media.rs index 6d72107e..0c234884 100644 --- a/src/client_server/media.rs +++ b/src/client_server/media.rs @@ -39,7 +39,7 @@ pub async fn create_content_route( db.media.create( mxc.clone(), &body.filename.as_deref(), - &body.content_type.as_deref(), // TODO this is now optional handle + &body.content_type.as_deref(), &body.file, )?; diff --git a/src/client_server/membership.rs b/src/client_server/membership.rs index 39d69cdd..849fb7e2 100644 --- a/src/client_server/membership.rs +++ b/src/client_server/membership.rs @@ -519,7 +519,6 @@ async fn join_room_by_id_helper( canon_json_stub.remove("event_id"); // In order to create a compatible ref hash (EventID) the `hashes` field needs to be present - // who the hell knew... ruma::signatures::hash_and_sign_event( db.globals.server_name().as_str(), db.globals.keypair(), @@ -602,7 +601,6 @@ async fn join_room_by_id_helper( )))) // Add join event we just created .map(|r| { let (event_id, value) = r?; - // TODO remove .clone when I remove debug logging state_res::StateEvent::from_id_value(event_id.clone(), value.clone()) .map(|ev| (event_id, Arc::new(ev))) .map_err(|e| { diff --git a/src/database/rooms.rs b/src/database/rooms.rs index d8e61316..3d5b890b 100644 --- a/src/database/rooms.rs +++ b/src/database/rooms.rs @@ -35,6 +35,11 @@ use super::admin::AdminCommand; /// hashing the entire state. pub type StateHashId = IVec; +/// An enum that represents the two valid states when searching +/// for an events "parent". +/// +/// An events parent is any event we are aware of that is part of +/// the events `prev_events` array. pub enum ClosestParent { Append, Insert(u64), @@ -80,7 +85,7 @@ impl StateStore for Rooms { .map_err(StateError::custom)? .ok_or_else(|| { StateError::NotFound(format!( - "PDU via room_id and event_id not found in the db.\n{}", + "PDU via room_id and event_id not found in the db: {}", event_id.as_str() )) })?; @@ -258,6 +263,8 @@ impl Rooms { } /// Force the creation of a new StateHash and insert it into the db. + /// + /// Whatever `state` is supplied to `force_state` __is__ the current room state snapshot. pub fn force_state( &self, room_id: &RoomId, @@ -403,6 +410,12 @@ impl Rooms { } } + /// Recursively search for a PDU from our DB that is also in the + /// `prev_events` field of the incoming PDU. + /// + /// First we check if the last PDU inserted to the given room is a parent + /// if not we recursively check older `prev_events` to insert the incoming + /// event after. pub fn get_closest_parent( &self, room: &RoomId, diff --git a/src/database/sending.rs b/src/database/sending.rs index 14558e3c..6b9e0fe7 100644 --- a/src/database/sending.rs +++ b/src/database/sending.rs @@ -2,7 +2,7 @@ use std::{collections::HashMap, convert::TryFrom, time::SystemTime}; use crate::{server_server, utils, Error, PduEvent, Result}; use federation::transactions::send_transaction_message; -use log::debug; +use log::{debug, error}; use rocket::futures::stream::{FuturesUnordered, StreamExt}; use ruma::{api::federation, ServerName}; use sled::IVec; @@ -115,8 +115,8 @@ impl Sending { // servercurrentpdus with the prefix should be empty now } } - Err((_server, _e)) => { - log::error!("server: {}\nerror: {}", _server, _e) + Err((server, e)) => { + error!("server: {}\nerror: {}", server, e) // TODO: exponential backoff } }; diff --git a/src/ruma_wrapper.rs b/src/ruma_wrapper.rs index 1c5529a3..a68b09d0 100644 --- a/src/ruma_wrapper.rs +++ b/src/ruma_wrapper.rs @@ -67,27 +67,28 @@ where let (sender_user, sender_device) = // TODO: Do we need to matches! anything else here? ServerSignatures - if matches!(T::METADATA.authentication, AuthScheme::AccessToken | AuthScheme::QueryOnlyAccessToken) { - // Get token from header or query value - let token = match request - .headers() - .get_one("Authorization") - .map(|s| s[7..].to_owned()) // Split off "Bearer " - .or_else(|| request.get_query_value("access_token").and_then(|r| r.ok())) - { - // TODO: M_MISSING_TOKEN - None => return Failure((Status::Unauthorized, ())), - Some(token) => token, - }; - - // Check if token is valid - match db.users.find_from_token(&token).unwrap() { - // TODO: M_UNKNOWN_TOKEN - None => return Failure((Status::Unauthorized, ())), - Some((user_id, device_id)) => (Some(user_id), Some(device_id.into())), + match T::METADATA.authentication { + AuthScheme::AccessToken | AuthScheme::QueryOnlyAccessToken => { + // Get token from header or query value + let token = match request + .headers() + .get_one("Authorization") + .map(|s| s[7..].to_owned()) // Split off "Bearer " + .or_else(|| request.get_query_value("access_token").and_then(|r| r.ok())) + { + // TODO: M_MISSING_TOKEN + None => return Failure((Status::Unauthorized, ())), + Some(token) => token, + }; + + // Check if token is valid + match db.users.find_from_token(&token).unwrap() { + // TODO: M_UNKNOWN_TOKEN + None => return Failure((Status::Unauthorized, ())), + Some((user_id, device_id)) => (Some(user_id), Some(device_id.into())), + } } - } else { - (None, None) + _ => (None, None) }; let mut http_request = http::Request::builder() diff --git a/src/server_server.rs b/src/server_server.rs index b9d26fd9..89d8eb16 100644 --- a/src/server_server.rs +++ b/src/server_server.rs @@ -4,7 +4,7 @@ use crate::{ }; use get_profile_information::v1::ProfileField; use http::header::{HeaderValue, AUTHORIZATION, HOST}; -use log::warn; +use log::{error, warn}; use rocket::{get, post, put, response::content::Json, State}; use ruma::{ api::{ @@ -26,7 +26,6 @@ use std::{ collections::BTreeMap, convert::{TryFrom, TryInto}, fmt::Debug, - sync::Arc, time::{Duration, SystemTime}, }; use trust_dns_resolver::AsyncResolver; @@ -99,7 +98,7 @@ where let mut http_request = request .try_into_http_request(&actual_destination, Some("")) .map_err(|e| { - warn!("failed to find destination {}: {}", actual_destination, e); + warn!("Failed to find destination {}: {}", actual_destination, e); Error::BadServerResponse("Invalid destination") })?; @@ -264,12 +263,14 @@ pub fn get_server_keys(db: State<'_, Database>) -> Json { .body(), ) .unwrap(); + ruma::signatures::sign_json( db.globals.server_name().as_str(), db.globals.keypair(), &mut response, ) .unwrap(); + Json(ruma::serde::to_canonical_json_string(&response).expect("JSON is canonical")) } @@ -413,8 +414,8 @@ pub async fn send_transaction_message_route<'a>( "m.receipt" => {} _ => {} }, - Err(_err) => { - log::error!("{}", _err); + Err(err) => { + error!("{}", err); continue; } } @@ -434,11 +435,9 @@ pub async fn send_transaction_message_route<'a>( .expect("all ruma pdus are conduit pdus"); let room_id = &pdu.room_id; - // If we have no idea about this room - // TODO: Does a server only send us events that we should know about or - // when everyone on this server leaves a room can we ignore those events? + // If we have no idea about this room skip the PDU if !db.rooms.exists(&pdu.room_id)? { - log::error!("Room does not exist on this server"); + error!("Room does not exist on this server."); resolved_map.insert(event_id, Err("Room is unknown to this server".into())); continue; } @@ -460,7 +459,7 @@ pub async fn send_transaction_message_route<'a>( // As an example a possible error // {"errcode":"M_FORBIDDEN","error":"Host not in room."} Err(err) => { - log::error!("Request failed: {}", err); + error!("Request failed: {}", err); resolved_map.insert(event_id, Err(err.to_string())); continue; } @@ -487,7 +486,7 @@ pub async fn send_transaction_message_route<'a>( if value.get("state_key").is_none() { if !db.rooms.is_joined(&pdu.sender, &pdu.room_id)? { - log::error!("Sender is not joined {}", pdu.kind); + error!("Sender is not joined {}", pdu.kind); resolved_map.insert(event_id, Err("User is not in this room".into())); continue; }