diff --git a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt index be917a833c..789793b493 100644 --- a/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt +++ b/components/logins/android/src/test/java/mozilla/appservices/logins/DatabaseLoginsStorageTest.kt @@ -46,33 +46,25 @@ class DatabaseLoginsStorageTest { store.add( LoginEntry( - fields = LoginFields( - origin = "https://www.example.com", - httpRealm = "Something", - usernameField = "users_name", - passwordField = "users_password", - formActionOrigin = null, - ), - secFields = SecureLoginFields( - username = "Foobar2000", - password = "hunter2", - ), + origin = "https://www.example.com", + httpRealm = "Something", + usernameField = "users_name", + passwordField = "users_password", + formActionOrigin = null, + username = "Foobar2000", + password = "hunter2", ), ) store.add( LoginEntry( - fields = LoginFields( - origin = "https://www.example.org", - httpRealm = "", - formActionOrigin = "https://www.example.org/login", - usernameField = "users_name", - passwordField = "users_password", - ), - secFields = SecureLoginFields( - password = "MyVeryCoolPassword", - username = "Foobar2000", - ), + origin = "https://www.example.org", + httpRealm = "", + formActionOrigin = "https://www.example.org/login", + usernameField = "users_name", + passwordField = "users_password", + password = "MyVeryCoolPassword", + username = "Foobar2000", ), ) @@ -93,17 +85,13 @@ class DatabaseLoginsStorageTest { val login = store.add( LoginEntry( - fields = LoginFields( - origin = "https://www.example.com", - httpRealm = "Something", - usernameField = "users_name", - passwordField = "users_password", - formActionOrigin = null, - ), - secFields = SecureLoginFields( - username = "Foobar2000", - password = "hunter2", - ), + origin = "https://www.example.com", + httpRealm = "Something", + usernameField = "users_name", + passwordField = "users_password", + formActionOrigin = null, + username = "Foobar2000", + password = "hunter2", ), ) @@ -112,17 +100,13 @@ class DatabaseLoginsStorageTest { // N.B. this is invalid due to `formActionOrigin` being an invalid url. val invalid = LoginEntry( - fields = LoginFields( - origin = "https://test.example.com", - formActionOrigin = "not a url", - httpRealm = "", - usernameField = "users_name", - passwordField = "users_password", - ), - secFields = SecureLoginFields( - username = "Foobar2000", - password = "hunter2", - ), + origin = "https://test.example.com", + formActionOrigin = "not a url", + httpRealm = "", + usernameField = "users_name", + passwordField = "users_password", + username = "Foobar2000", + password = "hunter2", ) try { @@ -138,8 +122,8 @@ class DatabaseLoginsStorageTest { assertNull(LoginsStoreMetrics.readQueryCount.testGetValue()) assertNull(LoginsStoreMetrics.readQueryErrorCount["storage_error"].testGetValue()) - val record = store.get(login.record.id)!! - assertEquals(record.fields.origin, "https://www.example.com") + val record = store.get(login.id)!! + assertEquals(record.origin, "https://www.example.com") assertEquals(LoginsStoreMetrics.readQueryCount.testGetValue(), 1) assertNull(LoginsStoreMetrics.readQueryErrorCount["storage_error"].testGetValue()) @@ -153,13 +137,13 @@ class DatabaseLoginsStorageTest { val login = store.list()[0] // Wait 100ms so that touch is certain to change timeLastUsed. Thread.sleep(100) - store.touch(login.record.id) + store.touch(login.id) - val updatedLogin = store.get(login.record.id) + val updatedLogin = store.get(login.id) assertNotNull(updatedLogin) - assertEquals(login.record.timesUsed + 1, updatedLogin!!.record.timesUsed) - assert(updatedLogin.record.timeLastUsed > login.record.timeLastUsed) + assertEquals(login.timesUsed + 1, updatedLogin!!.timesUsed) + assert(updatedLogin.timeLastUsed > login.timeLastUsed) assertThrows(LoginsApiException.NoSuchRecord::class.java) { store.touch("abcdabcdabcd") } @@ -171,11 +155,11 @@ class DatabaseLoginsStorageTest { val store = getTestStore() val login = store.list()[0] - assertNotNull(store.get(login.record.id)) - assertTrue(store.delete(login.record.id)) - assertNull(store.get(login.record.id)) - assertFalse(store.delete(login.record.id)) - assertNull(store.get(login.record.id)) + assertNotNull(store.get(login.id)) + assertTrue(store.delete(login.id)) + assertNull(store.get(login.id)) + assertFalse(store.delete(login.id)) + assertNull(store.get(login.id)) finishAndClose(store) } @@ -189,8 +173,8 @@ class DatabaseLoginsStorageTest { test.wipeLocal() assertEquals(0, test.list().size) - assertNull(test.get(logins[0].record.id)) - assertNull(test.get(logins[1].record.id)) + assertNull(test.get(logins[0].id)) + assertNull(test.get(logins[1].id)) finishAndClose(test) } diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 2bf1ffcfdc..b1e86744ec 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -223,13 +223,9 @@ impl LoginDb { Ok(logins // First, try to match the username .iter() - .find(|login| login.sec_fields.username == look.sec_fields.username) + .find(|login| login.username == look.username) // Fall back on a blank username - .or_else(|| { - logins - .iter() - .find(|login| login.sec_fields.username.is_empty()) - }) + .or_else(|| logins.iter().find(|login| login.username.is_empty())) // Clone the login to avoid ref issues when returning across the FFI .cloned()) } @@ -364,6 +360,10 @@ impl LoginDb { let now_ms = util::system_time_ms_i64(SystemTime::now()); let new_entry = self.fixup_and_check_for_dupes(&guid, entry, encdec)?; + let sec_fields = SecureLoginFields { + username: new_entry.username, + password: new_entry.password, + }; let result = EncryptedLogin { record: RecordFields { id: guid.to_string(), @@ -372,8 +372,14 @@ impl LoginDb { time_last_used: now_ms, times_used: 1, }, - fields: new_entry.fields, - sec_fields: new_entry.sec_fields.encrypt(encdec)?, + fields: LoginFields { + origin: new_entry.origin, + form_action_origin: new_entry.form_action_origin, + http_realm: new_entry.http_realm, + username_field: new_entry.username_field, + password_field: new_entry.password_field, + }, + sec_fields: sec_fields.encrypt(encdec)?, }; let tx = self.unchecked_transaction()?; self.insert_new_login(&result)?; @@ -402,8 +408,8 @@ impl LoginDb { // Try to detect if sync is enabled by checking if there are any mirror logins let has_mirror_row: bool = self.db.query_one("SELECT EXISTS (SELECT 1 FROM loginsM)")?; - let has_http_realm = entry.fields.http_realm.is_some(); - let has_form_action_origin = entry.fields.form_action_origin.is_some(); + let has_http_realm = entry.http_realm.is_some(); + let has_form_action_origin = entry.form_action_origin.is_some(); report_error!( "logins-duplicate-in-update", "(mirror: {has_mirror_row}, realm: {has_http_realm}, form_origin: {has_form_action_origin})"); @@ -415,27 +421,36 @@ impl LoginDb { // We must read the existing record so we can correctly manage timePasswordChanged. let existing = match self.get_by_id(sguid)? { - Some(e) => e, + Some(e) => e.decrypt(encdec)?, None => return Err(Error::NoSuchRecord(sguid.to_owned())), }; - let time_password_changed = - if existing.decrypt_fields(encdec)?.password == entry.sec_fields.password { - existing.record.time_password_changed - } else { - now_ms - }; + let time_password_changed = if existing.password == entry.password { + existing.time_password_changed + } else { + now_ms + }; // Make the final object here - every column will be updated. + let sec_fields = SecureLoginFields { + username: entry.username, + password: entry.password, + }; let result = EncryptedLogin { record: RecordFields { - id: existing.record.id, - time_created: existing.record.time_created, + id: existing.id, + time_created: existing.time_created, time_password_changed, time_last_used: now_ms, - times_used: existing.record.times_used + 1, + times_used: existing.times_used + 1, + }, + fields: LoginFields { + origin: entry.origin, + form_action_origin: entry.form_action_origin, + http_realm: entry.http_realm, + username_field: entry.username_field, + password_field: entry.password_field, }, - fields: entry.fields, - sec_fields: entry.sec_fields.encrypt(encdec)?, + sec_fields: sec_fields.encrypt(encdec)?, }; self.update_existing_login(&result)?; @@ -451,7 +466,7 @@ impl LoginDb { // Make sure to fixup the entry first, in case that changes the username let entry = entry.fixup()?; match self.find_login_to_update(entry.clone(), encdec)? { - Some(login) => self.update(&login.record.id, entry, encdec), + Some(login) => self.update(&login.id, entry, encdec), None => self.add(entry, encdec), } } @@ -497,7 +512,7 @@ impl LoginDb { for possible in self.get_by_entry_target(entry)? { if possible.guid() != *guid { let pos_sec_fields = possible.decrypt_fields(encdec)?; - if pos_sec_fields.username == entry.sec_fields.username { + if pos_sec_fields.username == entry.username { return Ok(Some(possible.guid())); } } @@ -546,13 +561,10 @@ impl LoginDb { common_cols = schema::COMMON_COLS ); } - match ( - entry.fields.form_action_origin.as_ref(), - entry.fields.http_realm.as_ref(), - ) { + match (entry.form_action_origin.as_ref(), entry.http_realm.as_ref()) { (Some(form_action_origin), None) => { let params = named_params! { - ":origin": &entry.fields.origin, + ":origin": &entry.origin, ":form_action_origin": form_action_origin, }; self.db @@ -562,7 +574,7 @@ impl LoginDb { } (None, Some(http_realm)) => { let params = named_params! { - ":origin": &entry.fields.origin, + ":origin": &entry.origin, ":http_realm": http_realm, }; self.db @@ -885,21 +897,16 @@ mod tests { use super::*; use crate::encryption::test_utils::TEST_ENCDEC; use crate::sync::merge::LocalLogin; - use crate::SecureLoginFields; use std::{thread, time}; #[test] fn test_username_dupe_semantics() { let mut login = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..LoginFields::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "sekret".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "sekret".into(), + ..LoginEntry::default() }; let db = LoginDb::open_in_memory().unwrap(); @@ -916,7 +923,7 @@ mod tests { ); // Add one with an empty username - not a dupe. - login.sec_fields.username = "".to_string(); + login.username = "".to_string(); db.add(login.clone(), &*TEST_ENCDEC) .expect("empty login isn't a dupe"); @@ -935,17 +942,13 @@ mod tests { let added = db .add( LoginEntry { - fields: LoginFields { - form_action_origin: Some("http://😍.com".into()), - origin: "http://😍.com".into(), - http_realm: None, - username_field: "😍".into(), - password_field: "😍".into(), - }, - sec_fields: SecureLoginFields { - username: "😍".into(), - password: "😍".into(), - }, + form_action_origin: Some("http://😍.com".into()), + origin: "http://😍.com".into(), + http_realm: None, + username_field: "😍".into(), + password_field: "😍".into(), + username: "😍".into(), + password: "😍".into(), }, &*TEST_ENCDEC, ) @@ -973,16 +976,12 @@ mod tests { let added = db .add( LoginEntry { - fields: LoginFields { - form_action_origin: None, - origin: "http://😍.com".into(), - http_realm: Some("😍😍".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "😍".into(), - password: "😍".into(), - }, + form_action_origin: None, + origin: "http://😍.com".into(), + http_realm: Some("😍😍".into()), + username: "😍".into(), + password: "😍".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1019,15 +1018,10 @@ mod tests { for h in good.iter().chain(bad.iter()) { db.add( LoginEntry { - fields: LoginFields { - origin: (*h).into(), - http_realm: Some((*h).into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - password: "test".into(), - ..Default::default() - }, + origin: (*h).into(), + http_realm: Some((*h).into()), + password: "test".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1108,15 +1102,11 @@ mod tests { fn test_add() { let db = LoginDb::open_in_memory().unwrap(); let to_add = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test_user".into(), - password: "test_password".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() }; let login = db.add(to_add, &*TEST_ENCDEC).unwrap(); let login2 = db.get_by_id(&login.record.id).unwrap().unwrap(); @@ -1132,15 +1122,11 @@ mod tests { let login = db .add( LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "user1".into(), - password: "password1".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1148,15 +1134,11 @@ mod tests { db.update( &login.record.id, LoginEntry { - fields: LoginFields { - origin: "https://www.example2.com".into(), - http_realm: Some("https://www.example2.com".into()), - ..login.fields - }, - sec_fields: SecureLoginFields { - username: "user2".into(), - password: "password2".into(), - }, + origin: "https://www.example2.com".into(), + http_realm: Some("https://www.example2.com".into()), + username: "user2".into(), + password: "password2".into(), + ..Default::default() // TODO: check and fix if needed }, &*TEST_ENCDEC, ) @@ -1180,15 +1162,11 @@ mod tests { let login = db .add( LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "user1".into(), - password: "password1".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "user1".into(), + password: "password1".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1207,15 +1185,11 @@ mod tests { let login = db .add( LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test_user".into(), - password: "test_password".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test_user".into(), + password: "test_password".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1241,15 +1215,11 @@ mod tests { fn make_entry(username: &str, password: &str) -> LoginEntry { LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("the website".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: username.into(), - password: password.into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("the website".into()), + username: username.into(), + password: password.into(), + ..Default::default() } } @@ -1279,15 +1249,11 @@ mod tests { // Non-match because the http_realm is different db.add( LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("the other website".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "user".into(), - password: "pass".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("the other website".into()), + username: "user".into(), + password: "pass".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1295,15 +1261,11 @@ mod tests { // Non-match because it uses form_action_origin instead of http_realm db.add( LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - form_action_origin: Some("https://www.example.com/".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "user".into(), - password: "pass".into(), - }, + origin: "https://www.example.com".into(), + form_action_origin: Some("https://www.example.com/".into()), + username: "user".into(), + password: "pass".into(), + ..Default::default() }, &*TEST_ENCDEC, ) @@ -1344,11 +1306,8 @@ mod tests { assert!(db .find_login_to_update( LoginEntry { - fields: LoginFields { - http_realm: None, - form_action_origin: None, - ..LoginFields::default() - }, + http_realm: None, + form_action_origin: None, ..LoginEntry::default() }, &*TEST_ENCDEC @@ -1367,11 +1326,11 @@ mod tests { db.insert_new_login(&dupe).unwrap(); let mut entry = login.entry(); - entry.sec_fields.password = "pass2".to_string(); - db.update(&login.record.id, entry, &*TEST_ENCDEC).unwrap(); + entry.password = "pass2".to_string(); + db.update(&login.id, entry, &*TEST_ENCDEC).unwrap(); let mut entry = login.entry(); - entry.sec_fields.password = "pass3".to_string(); + entry.password = "pass3".to_string(); db.add_or_update(entry, &*TEST_ENCDEC).unwrap(); } } diff --git a/components/logins/src/login.rs b/components/logins/src/login.rs index a1057bda2c..e822f94937 100644 --- a/components/logins/src/login.rs +++ b/components/logins/src/login.rs @@ -294,7 +294,82 @@ pub struct LoginFields { pub password_field: String, } -impl LoginFields { +/// LoginEntry fields that are stored encrypted +#[derive(Debug, Clone, Hash, PartialEq, Eq, Serialize, Deserialize, Default)] +pub struct SecureLoginFields { + // - Username cannot be null, use the empty string instead + // - Password can't be empty or null (enforced in the ValidateAndFixup code) + // + // This matches the desktop behavior: + // https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/toolkit/components/passwordmgr/LoginManager.jsm#260-267 + + // Because we store the json version of this in the DB, and that's the only place the json + // is used, we rename the fields to short names, just to reduce the overhead in the DB. + #[serde(rename = "u")] + pub username: String, + #[serde(rename = "p")] + pub password: String, +} + +impl SecureLoginFields { + pub fn encrypt(&self, encdec: &dyn EncryptorDecryptor) -> Result { + let string = serde_json::to_string(&self)?; + let cipherbytes = encdec + .encrypt(string.as_bytes().into()) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + let ciphertext = std::str::from_utf8(&cipherbytes) + .map_err(|e| Error::EncryptionFailed(e.to_string()))?; + Ok(ciphertext.to_owned()) + } + + pub fn decrypt(ciphertext: &str, encdec: &dyn EncryptorDecryptor) -> Result { + let jsonbytes = encdec + .decrypt(ciphertext.as_bytes().into()) + .map_err(|e| Error::DecryptionFailed(e.to_string()))?; + let json = + std::str::from_utf8(&jsonbytes).map_err(|e| Error::DecryptionFailed(e.to_string()))?; + Ok(serde_json::from_str(json)?) + } +} + +/// Login data specific to database records +#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] +pub struct RecordFields { + pub id: String, + pub time_created: i64, + pub time_password_changed: i64, + pub time_last_used: i64, + pub times_used: i64, +} + +/// A login handed over to the store API; ie a login not yet persisted +#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] +pub struct LoginEntry { + // login fields + pub origin: String, + pub form_action_origin: Option, + pub http_realm: Option, + pub username_field: String, + pub password_field: String, + + // secure fields + pub username: String, + pub password: String, +} + +impl LoginEntry { + pub fn new(fields: LoginFields, sec_fields: SecureLoginFields) -> Self { + Self { + origin: fields.origin, + form_action_origin: fields.form_action_origin, + http_realm: fields.http_realm, + username_field: fields.username_field, + password_field: fields.password_field, + + username: sec_fields.username, + password: sec_fields.password, + } + } /// Internal helper for validation and fixups of an "origin" stored as /// a string. fn validate_and_fixup_origin(origin: &str) -> Result> { @@ -348,87 +423,88 @@ impl LoginFields { } } -/// LoginEntry fields that are stored encrypted -#[derive(Debug, Clone, Hash, PartialEq, Eq, Serialize, Deserialize, Default)] -pub struct SecureLoginFields { - // - Username cannot be null, use the empty string instead - // - Password can't be empty or null (enforced in the ValidateAndFixup code) - // - // This matches the desktop behavior: - // https://searchfox.org/mozilla-central/rev/d3683dbb252506400c71256ef3994cdbdfb71ada/toolkit/components/passwordmgr/LoginManager.jsm#260-267 - - // Because we store the json version of this in the DB, and that's the only place the json - // is used, we rename the fields to short names, just to reduce the overhead in the DB. - #[serde(rename = "u")] - pub username: String, - #[serde(rename = "p")] - pub password: String, -} - -impl SecureLoginFields { - pub fn encrypt(&self, encdec: &dyn EncryptorDecryptor) -> Result { - let string = serde_json::to_string(&self)?; - let cipherbytes = encdec - .encrypt(string.as_bytes().into()) - .map_err(|e| Error::EncryptionFailed(e.to_string()))?; - let ciphertext = std::str::from_utf8(&cipherbytes) - .map_err(|e| Error::EncryptionFailed(e.to_string()))?; - Ok(ciphertext.to_owned()) - } - - pub fn decrypt(ciphertext: &str, encdec: &dyn EncryptorDecryptor) -> Result { - let jsonbytes = encdec - .decrypt(ciphertext.as_bytes().into()) - .map_err(|e| Error::DecryptionFailed(e.to_string()))?; - let json = - std::str::from_utf8(&jsonbytes).map_err(|e| Error::DecryptionFailed(e.to_string()))?; - Ok(serde_json::from_str(json)?) - } -} - -/// Login data specific to database records +/// A login handed over from the store API, which has been persisted and contains persistence +/// information such as id and time stamps #[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] -pub struct RecordFields { +pub struct Login { + // record fields pub id: String, pub time_created: i64, pub time_password_changed: i64, pub time_last_used: i64, pub times_used: i64, -} -/// A login entered by the user -#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] -pub struct LoginEntry { - pub fields: LoginFields, - pub sec_fields: SecureLoginFields, -} + // login fields + pub origin: String, + pub form_action_origin: Option, + pub http_realm: Option, + pub username_field: String, + pub password_field: String, -/// A login stored in the database -#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] -pub struct Login { - pub record: RecordFields, - pub fields: LoginFields, - pub sec_fields: SecureLoginFields, + // secure fields + pub username: String, + pub password: String, } impl Login { + pub fn new(record: RecordFields, fields: LoginFields, sec_fields: SecureLoginFields) -> Self { + Self { + id: record.id, + time_created: record.time_created, + time_password_changed: record.time_password_changed, + time_last_used: record.time_last_used, + times_used: record.times_used, + + origin: fields.origin, + form_action_origin: fields.form_action_origin, + http_realm: fields.http_realm, + username_field: fields.username_field, + password_field: fields.password_field, + + username: sec_fields.username, + password: sec_fields.password, + } + } + #[inline] pub fn guid(&self) -> Guid { - Guid::from_string(self.record.id.clone()) + Guid::from_string(self.id.clone()) } pub fn entry(&self) -> LoginEntry { LoginEntry { - fields: self.fields.clone(), - sec_fields: self.sec_fields.clone(), + origin: self.origin.clone(), + form_action_origin: self.form_action_origin.clone(), + http_realm: self.http_realm.clone(), + username_field: self.username_field.clone(), + password_field: self.password_field.clone(), + + username: self.username.clone(), + password: self.password.clone(), } } pub fn encrypt(self, encdec: &dyn EncryptorDecryptor) -> Result { + let sec_fields = SecureLoginFields { + username: self.username, + password: self.password, + }; Ok(EncryptedLogin { - record: self.record, - fields: self.fields, - sec_fields: self.sec_fields.encrypt(encdec)?, + record: RecordFields { + id: self.id, + time_created: self.time_created, + time_password_changed: self.time_password_changed, + time_last_used: self.time_last_used, + times_used: self.times_used, + }, + fields: LoginFields { + origin: self.origin, + form_action_origin: self.form_action_origin, + http_realm: self.http_realm, + username_field: self.username_field, + password_field: self.password_field, + }, + sec_fields: sec_fields.encrypt(encdec)?, }) } } @@ -454,11 +530,11 @@ impl EncryptedLogin { } pub fn decrypt(self, encdec: &dyn EncryptorDecryptor) -> Result { - Ok(Login { - record: self.record, - fields: self.fields, - sec_fields: SecureLoginFields::decrypt(&self.sec_fields, encdec)?, - }) + Ok(Login::new( + self.record, + self.fields, + SecureLoginFields::decrypt(&self.sec_fields, encdec)?, + )) } pub fn decrypt_fields(&self, encdec: &dyn EncryptorDecryptor) -> Result { @@ -536,8 +612,7 @@ pub trait ValidateAndFixup { Self: Sized; } -impl ValidateAndFixup for LoginFields { - /// Internal helper for doing validation and fixups. +impl ValidateAndFixup for LoginEntry { fn validate_and_fixup(&self, fixup: bool) -> Result> { // XXX TODO: we've definitely got more validation and fixups to add here! @@ -662,13 +737,8 @@ impl ValidateAndFixup for LoginFields { } } - Ok(maybe_fixed) - } -} - -impl ValidateAndFixup for SecureLoginFields { - /// We don't actually have fixups. - fn validate_and_fixup(&self, _fixup: bool) -> Result> { + // secure fields + // // \r\n chars are valid in desktop for some reason, so we allow them here too. if self.username.contains('\0') { return Err(InvalidLogin::IllegalFieldValue { @@ -685,26 +755,8 @@ impl ValidateAndFixup for SecureLoginFields { } .into()); } - Ok(None) - } -} -impl ValidateAndFixup for LoginEntry { - fn validate_and_fixup(&self, fixup: bool) -> Result> { - let new_fields = self.fields.validate_and_fixup(fixup)?; - let new_sec_fields = self.sec_fields.validate_and_fixup(fixup)?; - Ok(match (new_fields, new_sec_fields) { - (Some(fields), Some(sec_fields)) => Some(Self { fields, sec_fields }), - (Some(fields), None) => Some(Self { - fields, - sec_fields: self.sec_fields.clone(), - }), - (None, Some(sec_fields)) => Some(Self { - fields: self.fields.clone(), - sec_fields, - }), - (None, None) => None, - }) + Ok(maybe_fixed) } } @@ -754,7 +806,7 @@ mod tests { "file://", "https://[::1]", ] { - assert_eq!(LoginFields::validate_and_fixup_origin(input)?, None); + assert_eq!(LoginEntry::validate_and_fixup_origin(input)?, None); } // And URLs which get normalized. @@ -786,7 +838,7 @@ mod tests { ), ] { assert_eq!( - LoginFields::validate_and_fixup_origin(input)?, + LoginEntry::validate_and_fixup_origin(input)?, Some((*output).into()) ); } @@ -803,257 +855,173 @@ mod tests { } let valid_login = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_empty_origin = LoginEntry { - fields: LoginFields { - origin: "".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_empty_password = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "".into(), + ..Default::default() }; let login_with_form_submit_and_http_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - form_action_origin: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + form_action_origin: Some("https://www.example.com".into()), + username: "".into(), + password: "test".into(), + ..Default::default() }; let login_without_form_submit_or_http_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + username: "".into(), + password: "test".into(), + ..Default::default() }; let login_with_legacy_form_submit_and_http_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - form_action_origin: Some("".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + form_action_origin: Some("".into()), + username: "".into(), + password: "test".into(), + ..Default::default() }; let login_with_null_http_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.\0com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.\0com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_null_username = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "\0".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "\0".into(), + password: "test".into(), + ..Default::default() }; let login_with_null_password = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "username".into(), - password: "test\0".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "username".into(), + password: "test\0".into(), + ..Default::default() }; let login_with_newline_origin = LoginEntry { - fields: LoginFields { - origin: "\rhttps://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "\rhttps://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_newline_username_field = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username_field: "\n".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username_field: "\n".into(), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_newline_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("foo\nbar".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("foo\nbar".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_newline_password = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test\n".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test\n".into(), + ..Default::default() }; let login_with_period_username_field = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - username_field: ".".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + http_realm: Some("https://www.example.com".into()), + username_field: ".".into(), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_period_form_action_origin = LoginEntry { - fields: LoginFields { - form_action_origin: Some(".".into()), - origin: "https://www.example.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + form_action_origin: Some(".".into()), + origin: "https://www.example.com".into(), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_javascript_form_action_origin = LoginEntry { - fields: LoginFields { - form_action_origin: Some("javascript:".into()), - origin: "https://www.example.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + form_action_origin: Some("javascript:".into()), + origin: "https://www.example.com".into(), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_malformed_origin_parens = LoginEntry { - fields: LoginFields { - origin: " (".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: " (".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_host_unicode = LoginEntry { - fields: LoginFields { - origin: "http://💖.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "http://💖.com".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_origin_trailing_slash = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com/".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example.com/".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_origin_expanded_ipv6 = LoginEntry { - fields: LoginFields { - origin: "https://[0:0:0:0:0:0:1:1]".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://[0:0:0:0:0:0:1:1]".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_unknown_protocol = LoginEntry { - fields: LoginFields { - origin: "moz-proxy://127.0.0.1:8888".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "moz-proxy://127.0.0.1:8888".into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let test_cases = [ @@ -1201,67 +1169,47 @@ mod tests { // Note that most URL fixups are tested above, but we have one or 2 here. let login_with_full_url = LoginEntry { - fields: LoginFields { - origin: "http://example.com/foo?query=wtf#bar".into(), - form_action_origin: Some("http://example.com/foo?query=wtf#bar".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "http://example.com/foo?query=wtf#bar".into(), + form_action_origin: Some("http://example.com/foo?query=wtf#bar".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_host_unicode = LoginEntry { - fields: LoginFields { - origin: "http://😍.com".into(), - form_action_origin: Some("http://😍.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "http://😍.com".into(), + form_action_origin: Some("http://😍.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_period_fsu = LoginEntry { - fields: LoginFields { - origin: "https://example.com".into(), - form_action_origin: Some(".".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://example.com".into(), + form_action_origin: Some(".".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_empty_fsu = LoginEntry { - fields: LoginFields { - origin: "https://example.com".into(), - form_action_origin: Some("".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://example.com".into(), + form_action_origin: Some("".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }; let login_with_form_submit_and_http_realm = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - form_action_origin: Some("https://www.example.com".into()), - // If both http_realm and form_action_origin are specified, we drop - // the former when fixing up. So for this test we must have an - // invalid value in http_realm to ensure we don't validate a value - // we end up dropping. - http_realm: Some("\n".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "".into(), - password: "test".into(), - }, + origin: "https://www.example.com".into(), + form_action_origin: Some("https://www.example.com".into()), + // If both http_realm and form_action_origin are specified, we drop + // the former when fixing up. So for this test we must have an + // invalid value in http_realm to ensure we don't validate a value + // we end up dropping. + http_realm: Some("\n".into()), + username: "".into(), + password: "test".into(), + ..Default::default() }; let test_cases = [ @@ -1296,14 +1244,10 @@ mod tests { for tc in &test_cases { let login = tc.login.clone().fixup().expect("should work"); if let Some(expected) = tc.fixedup_host { - assert_eq!( - login.fields.origin, expected, - "origin not fixed in {:#?}", - tc - ); + assert_eq!(login.origin, expected, "origin not fixed in {:#?}", tc); } assert_eq!( - login.fields.form_action_origin, tc.fixedup_form_action_origin, + login.form_action_origin, tc.fixedup_form_action_origin, "form_action_origin not fixed in {:#?}", tc, ); diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index 22b0674e09..2617742e3b 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -30,42 +30,40 @@ namespace logins { EncryptorDecryptor create_managed_encdec(KeyManager key_manager); }; -/// The fields you can add or update. -dictionary LoginFields { +/// A login entry from the user, not linked to any database record. +/// The add/update APIs input these. +dictionary LoginEntry { + // login fields string origin; string? http_realm; string? form_action_origin; string username_field; string password_field; -}; -/// Fields which are encrypted at rest. -dictionary SecureLoginFields { + // secure login fields string password; string username; }; -/// Fields specific to database records -dictionary RecordFields { +/// A login stored in the database +dictionary Login { + // record fields string id; i64 times_used; i64 time_created; i64 time_last_used; i64 time_password_changed; -}; -/// A login entry from the user, not linked to any database record. -/// The add/update APIs input these. -dictionary LoginEntry { - LoginFields fields; - SecureLoginFields sec_fields; -}; + // login fields + string origin; + string? http_realm; + string? form_action_origin; + string username_field; + string password_field; -/// A login stored in the database -dictionary Login { - RecordFields record; - LoginFields fields; - SecureLoginFields sec_fields; + // secure login fields + string password; + string username; }; /// These are the errors returned by our public API. diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index bd55f72bb6..94200e5947 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -197,15 +197,18 @@ mod test { use super::*; use crate::encryption::test_utils::TEST_ENCDEC; use crate::util; - use crate::{LoginFields, SecureLoginFields}; use more_asserts::*; use std::cmp::Reverse; use std::time::SystemTime; fn assert_logins_equiv(a: &LoginEntry, b: &Login) { - assert_eq!(a.fields, b.fields); - assert_eq!(b.sec_fields.username, a.sec_fields.username); - assert_eq!(b.sec_fields.password, a.sec_fields.password); + assert_eq!(a.origin, b.origin); + assert_eq!(a.form_action_origin, b.form_action_origin); + assert_eq!(a.http_realm, b.http_realm); + assert_eq!(a.username_field, b.username_field); + assert_eq!(a.password_field, b.password_field); + assert_eq!(b.username, a.username); + assert_eq!(b.password, a.password); } #[test] @@ -216,32 +219,24 @@ mod test { let start_us = util::system_time_ms_i64(SystemTime::now()); let a = LoginEntry { - fields: LoginFields { - origin: "https://www.example.com".into(), - form_action_origin: Some("https://www.example.com".into()), - username_field: "user_input".into(), - password_field: "pass_input".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "coolperson21".into(), - password: "p4ssw0rd".into(), - }, + origin: "https://www.example.com".into(), + form_action_origin: Some("https://www.example.com".into()), + username_field: "user_input".into(), + password_field: "pass_input".into(), + username: "coolperson21".into(), + password: "p4ssw0rd".into(), + ..Default::default() }; let b = LoginEntry { - fields: LoginFields { - origin: "https://www.example2.com".into(), - http_realm: Some("Some String Here".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "asdf".into(), - password: "fdsa".into(), - }, + origin: "https://www.example2.com".into(), + http_realm: Some("Some String Here".into()), + username: "asdf".into(), + password: "fdsa".into(), + ..Default::default() }; - let a_id = store.add(a.clone()).expect("added a").record.id; - let b_id = store.add(b.clone()).expect("added b").record.id; + let a_id = store.add(a.clone()).expect("added a").id; + let b_id = store.add(b.clone()).expect("added b").id; let a_from_db = store .get(&a_id) @@ -249,10 +244,10 @@ mod test { .expect("a to exist"); assert_logins_equiv(&a, &a_from_db); - assert_ge!(a_from_db.record.time_created, start_us); - assert_ge!(a_from_db.record.time_password_changed, start_us); - assert_ge!(a_from_db.record.time_last_used, start_us); - assert_eq!(a_from_db.record.times_used, 1); + assert_ge!(a_from_db.time_created, start_us); + assert_ge!(a_from_db.time_password_changed, start_us); + assert_ge!(a_from_db.time_last_used, start_us); + assert_eq!(a_from_db.times_used, 1); let b_from_db = store .get(&b_id) @@ -260,10 +255,10 @@ mod test { .expect("b to exist"); assert_logins_equiv(&LoginEntry { ..b.clone() }, &b_from_db); - assert_ge!(b_from_db.record.time_created, start_us); - assert_ge!(b_from_db.record.time_password_changed, start_us); - assert_ge!(b_from_db.record.time_last_used, start_us); - assert_eq!(b_from_db.record.times_used, 1); + assert_ge!(b_from_db.time_created, start_us); + assert_ge!(b_from_db.time_password_changed, start_us); + assert_ge!(b_from_db.time_last_used, start_us); + assert_eq!(b_from_db.times_used, 1); let mut list = store.list().expect("Grabbing list to work"); assert_eq!(list.len(), 2); @@ -307,10 +302,8 @@ mod test { let now_us = util::system_time_ms_i64(SystemTime::now()); let b2 = LoginEntry { - sec_fields: SecureLoginFields { - username: b.sec_fields.username.to_owned(), - password: "newpass".into(), - }, + username: b.username.to_owned(), + password: "newpass".into(), ..b }; @@ -324,12 +317,12 @@ mod test { .expect("b to exist"); assert_logins_equiv(&b2, &b_after_update); - assert_ge!(b_after_update.record.time_created, start_us); - assert_le!(b_after_update.record.time_created, now_us); - assert_ge!(b_after_update.record.time_password_changed, now_us); - assert_ge!(b_after_update.record.time_last_used, now_us); + assert_ge!(b_after_update.time_created, start_us); + assert_le!(b_after_update.time_created, now_us); + assert_ge!(b_after_update.time_password_changed, now_us); + assert_ge!(b_after_update.time_last_used, now_us); // Should be two even though we updated twice - assert_eq!(b_after_update.record.times_used, 2); + assert_eq!(b_after_update.times_used, 2); } #[test] diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 3e1fbe9b93..158850f479 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -719,43 +719,31 @@ mod tests { let store = LoginStore::new_in_memory(TEST_ENCDEC.clone()).unwrap(); let to_add = LoginEntry { - fields: LoginFields { - form_action_origin: Some("https://www.example.com".into()), - origin: "http://not-relevant-here.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + form_action_origin: Some("https://www.example.com".into()), + origin: "http://not-relevant-here.com".into(), + username: "test".into(), + password: "test".into(), + ..Default::default() }; - let first_id = store.add(to_add).expect("should insert first").record.id; + let first_id = store.add(to_add).expect("should insert first").id; let to_add = LoginEntry { - fields: LoginFields { - form_action_origin: Some("https://www.example1.com".into()), - origin: "http://not-relevant-here.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test1".into(), - password: "test1".into(), - }, + form_action_origin: Some("https://www.example1.com".into()), + origin: "http://not-relevant-here.com".into(), + username: "test1".into(), + password: "test1".into(), + ..Default::default() }; - let second_id = store.add(to_add).expect("should insert second").record.id; + let second_id = store.add(to_add).expect("should insert second").id; let to_add = LoginEntry { - fields: LoginFields { - http_realm: Some("http://some-realm.com".into()), - origin: "http://not-relevant-here.com".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test1".into(), - password: "test1".into(), - }, + http_realm: Some("http://some-realm.com".into()), + origin: "http://not-relevant-here.com".into(), + username: "test1".into(), + password: "test1".into(), + ..Default::default() }; - let no_form_origin_id = store.add(to_add).expect("should insert second").record.id; + let no_form_origin_id = store.add(to_add).expect("should insert second").id; let engine = LoginsSyncEngine::new(Arc::new(store)).unwrap(); @@ -840,15 +828,11 @@ mod tests { .update( "dummy_000001", LoginEntry { - fields: LoginFields { - origin: "https://www.example2.com".into(), - http_realm: Some("https://www.example2.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, + origin: "https://www.example2.com".into(), + http_realm: Some("https://www.example2.com".into()), + username: "test".into(), + password: "test".into(), + ..Default::default() }, ) .unwrap(); diff --git a/components/logins/src/sync/payload.rs b/components/logins/src/sync/payload.rs index 5e3ca3fef2..6c8e7b9d4d 100644 --- a/components/logins/src/sync/payload.rs +++ b/components/logins/src/sync/payload.rs @@ -11,7 +11,7 @@ use crate::encryption::EncryptorDecryptor; use crate::error::*; use crate::login::ValidateAndFixup; use crate::SecureLoginFields; -use crate::{EncryptedLogin, LoginFields, RecordFields}; +use crate::{EncryptedLogin, LoginEntry, LoginFields, RecordFields}; use serde_derive::*; use sync15::bso::OutgoingBso; use sync_guid::Guid; @@ -64,17 +64,34 @@ impl IncomingLogin { p: LoginPayload, encdec: &dyn EncryptorDecryptor, ) -> Result { - let fields = LoginFields { + let original_fields = LoginFields { origin: p.hostname, form_action_origin: p.form_submit_url, http_realm: p.http_realm, username_field: p.username_field, password_field: p.password_field, }; - let sec_fields = SecureLoginFields { + let original_sec_fields = SecureLoginFields { username: p.username, password: p.password, }; + // we do a bit of a dance here to maybe_fixup() the fields via LoginEntry + let original_login_entry = LoginEntry::new(original_fields, original_sec_fields); + let login_entry = original_login_entry + .maybe_fixup()? + .unwrap_or(original_login_entry); + let fields = LoginFields { + origin: login_entry.origin, + form_action_origin: login_entry.form_action_origin, + http_realm: login_entry.http_realm, + username_field: login_entry.username_field, + password_field: login_entry.password_field, + }; + let sec_fields = SecureLoginFields { + username: login_entry.username, + password: login_entry.password, + }; + // We handle NULL in the DB for migrated databases and it's wasteful // to encrypt the common case of an empty map, so... let unknown = if p.unknown_fields.is_empty() { @@ -93,11 +110,8 @@ impl IncomingLogin { time_last_used: p.time_last_used, times_used: p.times_used, }, - fields: fields.maybe_fixup()?.unwrap_or(fields), - sec_fields: sec_fields - .maybe_fixup()? - .unwrap_or(sec_fields) - .encrypt(encdec)?, + fields, + sec_fields: sec_fields.encrypt(encdec)?, }, unknown, }) diff --git a/examples/sync-pass/src/sync-pass.rs b/examples/sync-pass/src/sync-pass.rs index b9ca657f1b..2ef0e0e625 100644 --- a/examples/sync-pass/src/sync-pass.rs +++ b/examples/sync-pass/src/sync-pass.rs @@ -11,10 +11,7 @@ use cli_support::fxa_creds::{ }; use cli_support::prompt::{prompt_char, prompt_string, prompt_usize}; use logins::encryption::{create_key, ManagedEncryptorDecryptor, StaticKeyManager}; -use logins::{ - Login, LoginEntry, LoginFields, LoginStore, LoginsSyncEngine, SecureLoginFields, - ValidateAndFixup, -}; +use logins::{Login, LoginEntry, LoginStore, LoginsSyncEngine, ValidateAndFixup}; use prettytable::{row, Cell, Row, Table}; use std::fs; @@ -56,14 +53,13 @@ fn read_form_based_login() -> LoginEntry { let username_field = prompt_string("username_field").unwrap_or_default(); let password_field = prompt_string("password_field").unwrap_or_default(); LoginEntry { - fields: LoginFields { - username_field, - password_field, - form_action_origin, - http_realm: None, - origin, - }, - sec_fields: SecureLoginFields { username, password }, + username_field, + password_field, + form_action_origin, + http_realm: None, + origin, + username, + password, } } @@ -75,14 +71,13 @@ fn read_auth_based_login() -> LoginEntry { let username_field = prompt_string("username_field").unwrap_or_default(); let password_field = prompt_string("password_field").unwrap_or_default(); LoginEntry { - fields: LoginFields { - username_field, - password_field, - form_action_origin: None, - http_realm, - origin, - }, - sec_fields: SecureLoginFields { username, password }, + username_field, + password_field, + form_action_origin: None, + http_realm, + origin, + username, + password, } } @@ -96,12 +91,12 @@ fn update_string(field_name: &str, field: &mut String, extra: &str) -> bool { } } -fn update_encrypted_fields(fields: &mut SecureLoginFields, extra: &str) { - if let Some(v) = prompt_string(format!("new username [now {}{}]", fields.username, extra)) { - fields.username = v; +fn update_username_and_password(entry: &mut LoginEntry, extra: &str) { + if let Some(v) = prompt_string(format!("new username [now {}{}]", entry.username, extra)) { + entry.username = v; }; - if let Some(v) = prompt_string(format!("new password [now {}{}]", fields.password, extra)) { - fields.password = v; + if let Some(v) = prompt_string(format!("new password [now {}{}]", entry.password, extra)) { + entry.password = v; }; } @@ -114,47 +109,44 @@ fn string_opt_or<'a>(o: &'a Option, or: &'a str) -> &'a str { } fn update_login(login: Login) -> LoginEntry { - let mut record = LoginEntry { - sec_fields: login.sec_fields, - fields: login.fields, - }; - update_encrypted_fields(&mut record.sec_fields, ", leave blank to keep"); - update_string("origin", &mut record.fields.origin, ", leave blank to keep"); + let mut entry = login.entry(); + update_username_and_password(&mut entry, ", leave blank to keep"); + update_string("origin", &mut entry.origin, ", leave blank to keep"); update_string( "username_field", - &mut record.fields.username_field, + &mut entry.username_field, ", leave blank to keep", ); update_string( "password_field", - &mut record.fields.password_field, + &mut entry.password_field, ", leave blank to keep", ); if prompt_bool(&format!( "edit form_action_origin? (now {}) [yN]", - string_opt_or(&record.fields.form_action_origin, "(none)") + string_opt_or(&entry.form_action_origin, "(none)") )) .unwrap_or(false) { - record.fields.form_action_origin = prompt_string("form_action_origin"); + entry.form_action_origin = prompt_string("form_action_origin"); } if prompt_bool(&format!( "edit http_realm? (now {}) [yN]", - string_opt_or(&record.fields.http_realm, "(none)") + string_opt_or(&entry.http_realm, "(none)") )) .unwrap_or(false) { - record.fields.http_realm = prompt_string("http_realm"); + entry.http_realm = prompt_string("http_realm"); } - if let Err(e) = record.check_valid() { + if let Err(e) = entry.check_valid() { log::warn!("Warning: produced invalid record: {}", e); // but we return it anyway! } - record + entry } fn prompt_bool(msg: &str) -> Option { @@ -242,23 +234,23 @@ fn show_all(store: &LoginStore) -> Result> { table.add_row(row![ r->v.len(), Fr->&login.guid(), - &login.sec_fields.username, - &login.sec_fields.password, - &login.fields.origin, + &login.username, + &login.password, + &login.origin, - string_opt_or(&login.fields.form_action_origin, ""), - string_opt_or(&login.fields.http_realm, ""), + string_opt_or(&login.form_action_origin, ""), + string_opt_or(&login.http_realm, ""), - &login.fields.username_field, - &login.fields.password_field, + &login.username_field, + &login.password_field, - login.record.times_used, - timestamp_to_string(login.record.time_created), - timestamp_to_string(login.record.time_password_changed), - if login.record.time_last_used == 0 { + login.times_used, + timestamp_to_string(login.time_created), + timestamp_to_string(login.time_password_changed), + if login.time_last_used == 0 { "Never".to_owned() } else { - timestamp_to_string(login.record.time_last_used) + timestamp_to_string(login.time_last_used) } ]); v.push(login.guid().to_string()); diff --git a/testing/sync-test/src/auth.rs b/testing/sync-test/src/auth.rs index 510c373344..9981e988cd 100644 --- a/testing/sync-test/src/auth.rs +++ b/testing/sync-test/src/auth.rs @@ -59,7 +59,9 @@ impl TestClient { }; let key = create_key().unwrap(); - let encdec = Arc::new(ManagedEncryptorDecryptor::new(Arc::new(StaticKeyManager::new(key.clone())))); + let encdec = Arc::new(ManagedEncryptorDecryptor::new(Arc::new( + StaticKeyManager::new(key.clone()), + ))); Ok(Self { cli, diff --git a/testing/sync-test/src/logins.rs b/testing/sync-test/src/logins.rs index 1de55c83fe..dfb82d25b4 100644 --- a/testing/sync-test/src/logins.rs +++ b/testing/sync-test/src/logins.rs @@ -4,24 +4,26 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ use crate::auth::TestClient; use crate::testing::TestGroup; use anyhow::Result; -use logins::{ - ApiResult as LoginResult, Login, LoginEntry, LoginFields, LoginStore, SecureLoginFields, -}; +use logins::{ApiResult as LoginResult, Login, LoginEntry, LoginStore}; use std::collections::HashMap; // helpers... // Doesn't check metadata fields pub fn assert_logins_equiv(a: &Login, b: &Login) { assert_eq!(b.guid(), a.guid(), "id mismatch"); - assert_eq!(b.fields, a.fields); - assert_eq!(b.sec_fields, a.sec_fields); + assert_eq!(b.origin, a.origin); + assert_eq!(b.form_action_origin, a.form_action_origin); + assert_eq!(b.http_realm, a.http_realm); + assert_eq!(b.username_field, a.username_field); + assert_eq!(b.password_field, a.password_field); + assert_eq!(b.username, a.username); + assert_eq!(b.password, a.password); } pub fn times_used_for_id(s: &LoginStore, id: &str) -> i64 { s.get(id) .expect("get() failed") .expect("Login doesn't exist") - .record .times_used } @@ -55,10 +57,7 @@ pub fn update_login( ) -> LoginResult { let mut login = s.get(id)?.expect("No such login!"); callback(&mut login); - let to_update = LoginEntry { - fields: login.fields, - sec_fields: login.sec_fields, - }; + let to_update = login.entry(); s.update(id, to_update)?; Ok(s.get(id)?.expect("Just updated this")) } @@ -83,37 +82,29 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { let l0id = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "http://www.example.com".into(), - form_action_origin: Some("http://login.example.com".into()), - username_field: "uname".into(), - password_field: "pword".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username".into(), - password: "hunter2".into(), - }, + origin: "http://www.example.com".into(), + form_action_origin: Some("http://login.example.com".into()), + username_field: "uname".into(), + password_field: "pword".into(), + username: "cool_username".into(), + password: "hunter2".into(), + ..Default::default() }, ) .expect("add l0") .guid(); let login0_c0 = touch_login(&c0.logins_store, &l0id, 2).expect("touch0 c0"); - assert_eq!(login0_c0.record.times_used, 3); + assert_eq!(login0_c0.times_used, 3); let login1_c0 = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "http://www.example.com".into(), - http_realm: Some("Login".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username".into(), - password: "sekret".into(), - }, + origin: "http://www.example.com".into(), + http_realm: Some("Login".into()), + username: "cool_username".into(), + password: "sekret".into(), + ..Default::default() }, ) .expect("add l1"); @@ -144,18 +135,18 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { // Change login0 on both update_login(&c1.logins_store, &l0id, |l| { - l.sec_fields.password = "testtesttest".into(); + l.password = "testtesttest".into(); }) .unwrap(); let login0_c0 = update_login(&c0.logins_store, &l0id, |l| { - l.fields.username_field = "users_name".into(); + l.username_field = "users_name".into(); }) .unwrap(); // and login1 on remote. let login1_c1 = update_login(&c1.logins_store, &l1id, |l| { - l.sec_fields.username = "less_cool_username".into(); + l.username = "less_cool_username".into(); }) .unwrap(); @@ -173,15 +164,9 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { verify_login( &c0.logins_store, &Login { - fields: LoginFields { - username_field: "users_name".into(), - ..login0_c0.fields - }, - sec_fields: SecureLoginFields { - username: login0_c0.sec_fields.username, - password: "testtesttest".into(), - }, - record: login0_c0.record, + username_field: "users_name".into(), + password: "testtesttest".into(), + ..login0_c0 }, ); @@ -190,7 +175,6 @@ fn test_login_general(c0: &mut TestClient, c1: &mut TestClient) { .get(&l0id) .unwrap() .unwrap() - .record .times_used, 5, // initially 1, touched twice, updated twice (on two accounts! // doing this right requires 3WM) @@ -204,17 +188,13 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { let login0 = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "http://www.example.com".into(), - form_action_origin: Some("http://login.example.com".into()), - username_field: "uname".into(), - password_field: "pword".into(), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username".into(), - password: "hunter2".into(), - }, + origin: "http://www.example.com".into(), + form_action_origin: Some("http://login.example.com".into()), + username_field: "uname".into(), + password_field: "pword".into(), + username: "cool_username".into(), + password: "hunter2".into(), + ..Default::default() }, ) .expect("add l0"); @@ -223,15 +203,11 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { let login1 = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "http://www.example.com".into(), - http_realm: Some("Login".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username".into(), - password: "sekret".into(), - }, + origin: "http://www.example.com".into(), + http_realm: Some("Login".into()), + username: "cool_username".into(), + password: "sekret".into(), + ..Default::default() }, ) .expect("add l1"); @@ -240,15 +216,11 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { let login2 = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "https://www.example.org".into(), - http_realm: Some("Test".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username100".into(), - password: "123454321".into(), - }, + origin: "https://www.example.org".into(), + http_realm: Some("Test".into()), + username: "cool_username100".into(), + password: "123454321".into(), + ..Default::default() }, ) .expect("add l2"); @@ -257,15 +229,11 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { let login3 = add_login( &c0.logins_store, LoginEntry { - fields: LoginFields { - origin: "https://www.example.net".into(), - http_realm: Some("Http Realm".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "cool_username99".into(), - password: "aaaaa".into(), - }, + origin: "https://www.example.net".into(), + http_realm: Some("Http Realm".into()), + username: "cool_username99".into(), + password: "aaaaa".into(), + ..Default::default() }, ) .expect("add l3"); @@ -311,7 +279,7 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { // case 3a. c0 modifies record (c1 will delete it after c0 syncs so the timestamps line up) log::info!("Updating {} on c0", l2id); let login2_new = update_login(&c0.logins_store, &l2id, |l| { - l.sec_fields.username = "foobar".into(); + l.username = "foobar".into(); }) .unwrap(); @@ -331,7 +299,7 @@ fn test_login_deletes(c0: &mut TestClient, c1: &mut TestClient) { log::info!("Update {} on c0", l3id); // 4b update_login(&c0.logins_store, &l3id, |l| { - l.sec_fields.password = "quux".into(); + l.password = "quux".into(); }) .unwrap();