From 84a75c871bfb063d838d3db2eb1f07d93e2b3be8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa?= Date: Thu, 15 Feb 2018 19:05:57 +0100 Subject: [PATCH] Improved error messagees, implemented delete ciphers, attachments and account, implemented two factor recovery. Known missing: - import ciphers, create ciphers types other than login and card, update ciphers - clear and put device_tokens - Equivalent domains - Organizations --- README.md | 2 +- docker-compose.yml | 10 --- src/api/core/accounts.rs | 20 ++++-- src/api/core/ciphers.rs | 43 ++++++++++--- src/api/core/mod.rs | 4 +- src/api/core/two_factor.rs | 37 ++++++++++- src/api/icons.rs | 2 +- src/api/identity.rs | 118 +++++++++++------------------------- src/auth.rs | 26 ++------ src/db/models/attachment.rs | 4 ++ src/db/models/device.rs | 6 ++ src/db/models/user.rs | 23 +++++-- src/main.rs | 37 +---------- src/tests.rs | 14 +---- src/util.rs | 27 +++++++-- 15 files changed, 181 insertions(+), 192 deletions(-) delete mode 100644 docker-compose.yml diff --git a/README.md b/README.md index c8201c01..e139a7e3 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ on other systems use their respective package managers. Then run: ``` -cargo run +cargo run --bin bitwarden_rs # or cargo build ``` diff --git a/docker-compose.yml b/docker-compose.yml deleted file mode 100644 index 64051dea..00000000 --- a/docker-compose.yml +++ /dev/null @@ -1,10 +0,0 @@ -## Docker Compose file, experimental and untested -# Run 'docker compose up' to start the service -version: '3' -services: - web: - build: . - ports: - - "8000:80" - volumes: - - ./data:/data diff --git a/src/api/core/accounts.rs b/src/api/core/accounts.rs index 66797f28..42c06350 100644 --- a/src/api/core/accounts.rs +++ b/src/api/core/accounts.rs @@ -144,11 +144,23 @@ fn delete_account(data: Json, headers: Headers, conn: DbConn) -> Result<( err!("Invalid password") } - // Delete all ciphers by user_uuid - // Delete all devices by user_uuid - // Delete user + // Delete ciphers and their attachments + for cipher in Cipher::find_by_user(&user.uuid, &conn) { + for a in Attachment::find_by_cipher(&cipher.uuid, &conn) { a.delete(&conn); } - err!("Not implemented") + cipher.delete(&conn); + } + + // Delete folders + for f in Folder::find_by_user(&user.uuid, &conn) { f.delete(&conn); } + + // Delete devices + for d in Device::find_by_user(&user.uuid, &conn) { d.delete(&conn); } + + // Delete user + user.delete(&conn); + + Ok(()) } #[get("/accounts/revision-date")] diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 3a1e5b77..a3ea7548 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -258,11 +258,7 @@ fn delete_attachment(uuid: String, attachment_id: String, headers: Headers, conn err!("Cipher is not owned by user") } - // Delete file - let file = attachment.get_file_path(); - util::delete_file(&file); - - // Delete entry in cipher + // Delete attachment attachment.delete(&conn); Ok(()) @@ -274,13 +270,32 @@ fn post_cipher(uuid: String, headers: Headers, conn: DbConn) -> Result")] -fn put_cipher(uuid: String, headers: Headers, conn: DbConn) -> Result> { err!("Not implemented") } +fn put_cipher(uuid: String, headers: Headers, conn: DbConn) -> Result> { + err!("Not implemented") +} #[delete("/ciphers/")] -fn delete_cipher(uuid: String, headers: Headers, conn: DbConn) -> Result> { err!("Not implemented") } +fn delete_cipher(uuid: String, headers: Headers, conn: DbConn) -> Result<(), BadRequest> { + let cipher = match Cipher::find_by_uuid(&uuid, &conn) { + Some(cipher) => cipher, + None => err!("Cipher doesn't exist") + }; + + if cipher.user_uuid != headers.user.uuid { + err!("Cipher is not owned by user") + } + + // Delete attachments + for a in Attachment::find_by_cipher(&cipher.uuid, &conn) { a.delete(&conn); } + + // Delete cipher + cipher.delete(&conn); + + Ok(()) +} #[post("/ciphers/delete", data = "")] -fn delete_all(data: Json, headers: Headers, conn: DbConn) -> Result> { +fn delete_all(data: Json, headers: Headers, conn: DbConn) -> Result<(), BadRequest> { let password_hash = data["masterPasswordHash"].as_str().unwrap(); let user = headers.user; @@ -289,7 +304,15 @@ fn delete_all(data: Json, headers: Headers, conn: DbConn) -> Result Vec { get_twofactor, get_recover, + recover, generate_authenticator, activate_authenticator, disable_authenticator, @@ -107,8 +108,7 @@ fn post_eq_domains(data: Json, headers: Headers, conn: DbConn) let user = headers.user; - - //BODY. "{\"ExcludedGlobalEquivalentDomains\":[2],\"EquivalentDomains\":[[\"uoc.edu\",\"uoc.es\"]]}" + //BODY. "{\"ExcludedGlobalEquivalentDomains\":[2],\"EquivalentDomains\":[[\"example.org\",\"example.net\"]]}" err!("Not implemented") } diff --git a/src/api/core/two_factor.rs b/src/api/core/two_factor.rs index 54f7e240..e413e569 100644 --- a/src/api/core/two_factor.rs +++ b/src/api/core/two_factor.rs @@ -44,6 +44,39 @@ fn get_recover(data: Json, headers: Headers) -> Result, conn: DbConn) -> Result> { + println!("{:#?}", data); + + use db::models::User; + + // Get the user + let username = data["email"].as_str().unwrap(); + let mut user = match User::find_by_mail(username, &conn) { + Some(user) => user, + None => err!("Username or password is incorrect. Try again.") + }; + + // Check password + let password = data["masterPasswordHash"].as_str().unwrap(); + if !user.check_valid_password(password) { + err!("Username or password is incorrect. Try again.") + } + + // Check if recovery code is correct + let recovery_code = data["recoveryCode"].as_str().unwrap(); + + if !user.check_valid_recovery_code(recovery_code) { + err!("Recovery code is incorrect. Try again.") + } + + user.totp_secret = None; + user.totp_recover = None; + user.save(&conn); + + Ok(Json(json!({}))) +} + #[post("/two-factor/get-authenticator", data = "")] fn generate_authenticator(data: Json, headers: Headers) -> Result> { let password_hash = data["masterPasswordHash"].as_str().unwrap(); @@ -71,8 +104,8 @@ fn activate_authenticator(data: Json, headers: Headers, conn: DbConn) -> if !headers.user.check_valid_password(password_hash) { err!("Invalid password"); } - let token = data["token"].as_str(); // 123456 - let key = data["key"].as_str().unwrap(); // YI4SKBIXG32LOA6VFKH2NI25VU3E4QML + let token = data["token"].as_str(); + let key = data["key"].as_str().unwrap(); // Validate key as base32 and 20 bytes length let decoded_key: Vec = match BASE32.decode(key.as_bytes()) { diff --git a/src/api/icons.rs b/src/api/icons.rs index 0a8016b5..7331402e 100644 --- a/src/api/icons.rs +++ b/src/api/icons.rs @@ -71,7 +71,7 @@ fn get_icon_cached(key: &str, url: &str) -> io::Result> { // Save the currently downloaded icon match File::create(path) { - Ok(mut f) => { f.write_all(&icon); } + Ok(mut f) => { f.write_all(&icon).expect("Error writing icon file"); } Err(_) => { /* Continue */ } }; diff --git a/src/api/identity.rs b/src/api/identity.rs index e1623fae..5a57b7c8 100644 --- a/src/api/identity.rs +++ b/src/api/identity.rs @@ -41,61 +41,44 @@ fn login(connect_data: Form, conn: DbConn) -> Result user, - None => err!("Invalid username or password") + None => err!("Username or password is incorrect. Try again.") }; // Check password let password = data.get("password").unwrap(); if !user.check_valid_password(password) { - err!("Invalid username or password") + err!("Username or password is incorrect. Try again.") } - /* - //TODO: When invalid username or password, return this with a 400 BadRequest: - { - "error": "invalid_grant", - "error_description": "invalid_username_or_password", - "ErrorModel": { - "Message": "Username or password is incorrect. Try again.", - "ValidationErrors": null, - "ExceptionMessage": null, - "ExceptionStackTrace": null, - "InnerExceptionMessage": null, - "Object": "error" - } - } - */ - // Check if totp code is required and the value is correct - let totp_code = util::parse_option_string(data.get("twoFactorToken").map(String::as_ref)); + let totp_code = util::parse_option_string(data.get("twoFactorToken")); if !user.check_totp_code(totp_code) { // Return error 400 err_json!(json!({ - "error" : "invalid_grant", - "error_description" : "Two factor required.", - "TwoFactorProviders" : [ 0 ], - "TwoFactorProviders2" : { "0" : null } - })) + "error" : "invalid_grant", + "error_description" : "Two factor required.", + "TwoFactorProviders" : [ 0 ], + "TwoFactorProviders2" : { "0" : null } + })) } // Let's only use the header and ignore the 'devicetype' parameter // TODO Get header Device-Type let device_type_num = 0;// headers.device_type; - let (device_id, device_name) = match data.get("client_id").unwrap().as_ref() { - "web" => { (format!("web-{}", user.uuid), String::from("web")) } - "browser" | "mobile" => { + let (device_id, device_name) = match data.is_device { + false => { (format!("web-{}", user.uuid), String::from("web")) } + true => { ( data.get("deviceidentifier").unwrap().clone(), data.get("devicename").unwrap().clone(), ) } - _ => err!("Invalid client id") }; // Find device or create new - let device = match Device::find_by_uuid(&device_id, &conn) { + match Device::find_by_uuid(&device_id, &conn) { Some(device) => { // Check if valid device if device.user_uuid != user.uuid { @@ -109,10 +92,7 @@ fn login(connect_data: Form, conn: DbConn) -> Result, conn: DbConn) -> Result, conn: DbConn) -> Result, } +#[derive(Debug, Copy, Clone)] +enum GrantType { RefreshToken, Password } + impl ConnectData { fn get(&self, key: &str) -> Option<&String> { self.data.get(&key.to_lowercase()) } } -#[derive(Debug, Copy, Clone)] -enum GrantType { RefreshToken, Password } - - const VALUES_REFRESH: [&str; 1] = ["refresh_token"]; - -const VALUES_PASSWORD: [&str; 5] = ["client_id", - "grant_type", "password", "scope", "username"]; - -const VALUES_DEVICE: [&str; 3] = ["deviceidentifier", - "devicename", "devicetype"]; - +const VALUES_PASSWORD: [&str; 5] = ["client_id", "grant_type", "password", "scope", "username"]; +const VALUES_DEVICE: [&str; 3] = ["deviceidentifier", "devicename", "devicetype"]; impl<'f> FromForm<'f> for ConnectData { type Error = String; @@ -164,62 +138,40 @@ impl<'f> FromForm<'f> for ConnectData { // Insert data into map for (key, value) in items { - let decoded_key: String = match key.url_decode() { - Ok(decoded) => decoded, - Err(_) => return Err(format!("Error decoding key: {}", value)), + match (key.url_decode(), value.url_decode()) { + (Ok(key), Ok(value)) => data.insert(key.to_lowercase(), value), + _ => return Err(format!("Error decoding key or value")), }; - - let decoded_value: String = match value.url_decode() { - Ok(decoded) => decoded, - Err(_) => return Err(format!("Error decoding value: {}", value)), - }; - - data.insert(decoded_key.to_lowercase(), decoded_value); } // Validate needed values - let grant_type = - match data.get("grant_type").map(|s| &s[..]) { + let (grant_type, is_device) = + match data.get("grant_type").map(String::as_ref) { Some("refresh_token") => { - // Check if refresh token is proviced - if let Err(msg) = check_values(&data, &VALUES_REFRESH) { - return Err(msg); - } - - GrantType::RefreshToken + check_values(&data, &VALUES_REFRESH)?; + (GrantType::RefreshToken, false) // Device doesn't matter here } Some("password") => { - // Check if basic values are provided - if let Err(msg) = check_values(&data, &VALUES_PASSWORD) { - return Err(msg); - } + check_values(&data, &VALUES_PASSWORD)?; - // Check that device values are present on device - match data.get("client_id").unwrap().as_ref() { - "browser" | "mobile" => { - if let Err(msg) = check_values(&data, &VALUES_DEVICE) { - return Err(msg); - } - } - _ => {} - } - - GrantType::Password + let is_device = match data.get("client_id").unwrap().as_ref() { + "browser" | "mobile" => check_values(&data, &VALUES_DEVICE)?, + _ => false + }; + (GrantType::Password, is_device) } - _ => return Err(format!("Grant type not supported")) }; - Ok(ConnectData { grant_type, data }) + Ok(ConnectData { grant_type, is_device, data }) } } -fn check_values(map: &HashMap, values: &[&str]) -> Result<(), String> { +fn check_values(map: &HashMap, values: &[&str]) -> Result { for value in values { if !map.contains_key(*value) { return Err(format!("{} cannot be blank", value)); } } - - Ok(()) + Ok(true) } \ No newline at end of file diff --git a/src/auth.rs b/src/auth.rs index 6e69ee7e..a1607273 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -93,7 +93,6 @@ use db::DbConn; use db::models::{User, Device}; pub struct Headers { - pub device_type: Option, pub host: String, pub device: Device, pub user: User, @@ -105,29 +104,19 @@ impl<'a, 'r> FromRequest<'a, 'r> for Headers { fn from_request(request: &'a Request<'r>) -> request::Outcome { let headers = request.headers(); - // Get device type - let device_type = match headers.get_one("Device-Type") - .map(|s| s.parse::()) { - Some(Ok(dt)) => Some(dt),// dt, - _ => None // return err_handler!("Device-Type is invalid or missing") - }; - // Get host let host = match headers.get_one("Host") { Some(host) => format!("http://{}", host), // TODO: Check if HTTPS - _ => String::new() // return err_handler!("Host is invalid or missing") + _ => String::new() }; // Get access_token let access_token: &str = match request.headers().get_one("Authorization") { Some(a) => { - let split: Option<&str> = a.rsplit("Bearer ").next(); - - if split.is_none() { - err_handler!("No access token provided") + match a.rsplit("Bearer ").next() { + Some(split) => split, + None => err_handler!("No access token provided") } - - split.unwrap() } None => err_handler!("No access token provided") }; @@ -135,10 +124,7 @@ impl<'a, 'r> FromRequest<'a, 'r> for Headers { // Check JWT token is valid and get device and user from it let claims: JWTClaims = match decode_jwt(access_token) { Ok(claims) => claims, - Err(msg) => { - println!("Invalid claim: {}", msg); - err_handler!("Invalid claim") - } + Err(msg) => err_handler!("Invalid claim") }; let device_uuid = claims.device; @@ -163,6 +149,6 @@ impl<'a, 'r> FromRequest<'a, 'r> for Headers { err_handler!("Invalid security stamp") } - Outcome::Success(Headers { device_type, host, device, user }) + Outcome::Success(Headers { host, device, user }) } } \ No newline at end of file diff --git a/src/db/models/attachment.rs b/src/db/models/attachment.rs index 7d09a77f..0ae88ffb 100644 --- a/src/db/models/attachment.rs +++ b/src/db/models/attachment.rs @@ -63,6 +63,10 @@ impl Attachment { } pub fn delete(self, conn: &DbConn) -> bool { + use util; + + util::delete_file(&self.get_file_path()); + match diesel::delete(attachments::table.filter( attachments::id.eq(self.id))) .execute(&**conn) { diff --git a/src/db/models/device.rs b/src/db/models/device.rs index 346b5d2c..9539a65e 100644 --- a/src/db/models/device.rs +++ b/src/db/models/device.rs @@ -113,4 +113,10 @@ impl Device { .filter(devices::refresh_token.eq(refresh_token)) .first::(&**conn).ok() } + + pub fn find_by_user(user_uuid: &str, conn: &DbConn) -> Vec { + devices::table + .filter(devices::user_uuid.eq(user_uuid)) + .load::(&**conn).expect("Error loading devices") + } } diff --git a/src/db/models/user.rs b/src/db/models/user.rs index f1aecd68..dca0e1ed 100644 --- a/src/db/models/user.rs +++ b/src/db/models/user.rs @@ -3,6 +3,7 @@ use serde_json::Value as JsonValue; use uuid::Uuid; +use crypto; use CONFIG; #[derive(Debug, Identifiable, Queryable, Insertable)] @@ -38,8 +39,6 @@ impl User { let now = Utc::now().naive_utc(); let email = mail.to_lowercase(); - use crypto; - let iterations = CONFIG.password_iterations; let salt = crypto::get_random_64(); let password_hash = crypto::hash_password(password.as_bytes(), &salt, iterations as u32); @@ -70,16 +69,21 @@ impl User { } pub fn check_valid_password(&self, password: &str) -> bool { - use crypto; - crypto::verify_password_hash(password.as_bytes(), &self.salt, &self.password_hash, self.password_iterations as u32) } + pub fn check_valid_recovery_code(&self, recovery_code: &str) -> bool { + if let Some(ref totp_recover) = self.totp_recover { + recovery_code == totp_recover.to_lowercase() + } else { + false + } + } + pub fn set_password(&mut self, password: &str) { - use crypto; self.password_hash = crypto::hash_password(password.as_bytes(), &self.salt, self.password_iterations as u32); @@ -149,6 +153,15 @@ impl User { } } + pub fn delete(self, conn: &DbConn) -> bool { + match diesel::delete(users::table.filter( + users::uuid.eq(self.uuid))) + .execute(&**conn) { + Ok(1) => true, // One row deleted + _ => false, + } + } + pub fn find_by_mail(mail: &str, conn: &DbConn) -> Option { let lower_mail = mail.to_lowercase(); users::table diff --git a/src/main.rs b/src/main.rs index bdb2f1a7..3fbb29ca 100644 --- a/src/main.rs +++ b/src/main.rs @@ -53,7 +53,6 @@ fn init_rocket() -> Rocket { .mount("/identity", api::identity_routes()) .mount("/icons", api::icons_routes()) .manage(db::init_pool()) - .attach(DebugFairing) } // Embed the migrations from the migrations folder into the application @@ -66,7 +65,7 @@ fn main() { // Make sure the database is up to date (create if it doesn't exist, or run the migrations) let connection = db::get_connection().expect("Can't conect to DB"); - embedded_migrations::run_with_output(&connection, &mut io::stdout()); + embedded_migrations::run_with_output(&connection, &mut io::stdout()).expect("Can't run migrations"); // Validate location of rsa keys if !util::file_exists(&CONFIG.private_rsa_key) { @@ -114,37 +113,3 @@ impl Config { } } } - -struct DebugFairing; - -impl Fairing for DebugFairing { - fn info(&self) -> Info { - Info { - name: "Request Debugger", - kind: Kind::Request, - } - } - - fn on_request(&self, req: &mut Request, data: &Data) { - let uri_string = req.uri().to_string(); - - // Ignore web requests - if !uri_string.starts_with("/api") && - !uri_string.starts_with("/identity") { - return; - } - - /* - for header in req.headers().iter() { - println!("DEBUG- {:#?} {:#?}", header.name(), header.value()); - } - */ - - /*let body_data = data.peek(); - - if body_data.len() > 0 { - println!("DEBUG- Body Complete: {}", data.peek_complete()); - println!("DEBUG- {}", String::from_utf8_lossy(body_data)); - }*/ - } -} diff --git a/src/tests.rs b/src/tests.rs index 90a7f841..f6a411f3 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -2,19 +2,9 @@ use super::init_rocket; use rocket::local::Client; use rocket::http::Status; -#[test] -fn hello_world() { - let client = Client::new(init_rocket()).expect("valid rocket instance"); - let mut response = client.get("/alive").dispatch(); - assert_eq!(response.status(), Status::Ok); - // assert_eq!(response.body_string(), Some("Hello, world!".into())); -} - // TODO: For testing, we can use either a test_transaction, or an in-memory database - -// TODO: test_transaction http://docs.diesel.rs/diesel/connection/trait.Connection.html#method.begin_test_transaction - -// TODO: in-memory database https://github.com/diesel-rs/diesel/issues/419 (basically use ":memory:" as the connection string +// test_transaction: http://docs.diesel.rs/diesel/connection/trait.Connection.html#method.begin_test_transaction +// in-memory database: https://github.com/diesel-rs/diesel/issues/419 (basically use ":memory:" as the connection string describe! route_tests { before_each { diff --git a/src/util.rs b/src/util.rs index 66b036c3..f656382c 100644 --- a/src/util.rs +++ b/src/util.rs @@ -3,9 +3,18 @@ /// #[macro_export] macro_rules! err { - ($expr:expr) => {{ - err_json!(json!($expr)); - }} + ($err:expr, $err_desc:expr, $msg:expr) => { + err_json!(json!({ + "error": $err, + "error_description": $err_desc, + "ErrorModel": { + "Message": $msg, + "ValidationErrors": null, + "Object": "error" + } + })) + }; + ($msg:expr) => { err!("default_error", "default_error_description", $msg) } } #[macro_export] @@ -49,7 +58,13 @@ pub fn read_file(path: &str) -> Result, String> { } pub fn delete_file(path: &str) -> bool { - fs::remove_file(path).is_ok() + let res = fs::remove_file(path).is_ok(); + + if let Some(parent) = Path::new(path).parent() { + fs::remove_dir(parent); // Only removes if the directory is empty + } + + res } @@ -88,8 +103,8 @@ pub fn upcase_first(s: &str) -> String { } } -pub fn parse_option_string(string: Option) -> Option where S: Into, T: FromStr { - if let Some(Ok(value)) = string.map(|s| s.into().parse::()) { +pub fn parse_option_string(string: Option) -> Option where S: AsRef, T: FromStr { + if let Some(Ok(value)) = string.map(|s| s.as_ref().parse::()) { Some(value) } else { None