From 033d8de5c1e961488ab99d5272134dac71604a4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20J=C3=B6rg=20Schmidt?= Date: Tue, 17 Dec 2024 19:20:28 +0100 Subject: [PATCH] flatten Login struct The current Login structure is nested: ``` Login { RecordFields record; LoginFields fields; SecureLoginFields sec_fields; } ``` and thus exposes internal data structuring to the consumer. Since we make the encryption transparent for the consumer (Android, iOS, desktop) here, such a separation no longer makes sense here and above can be simplified to ``` Login { // record fields string id; i64 times_used; i64 time_created; i64 time_last_used; i64 time_password_changed; // login fields string origin; string? http_realm; string? form_action_origin; string username_field; string password_field; // secure login fields string password; string username; } ``` The advantage of eliminating this separation lies, on the one hand, in the simplification of the API and the resulting easier use of the component, and on the other hand, in the easier changeability of the internal data structure. If, for example, we decide later to encrypt additional fields, such a change is possible without having to adapt the consumers. --- .../logins/DatabaseLoginsStorageTest.kt | 98 ++- components/logins/src/db.rs | 261 +++----- components/logins/src/login.rs | 628 ++++++++---------- components/logins/src/logins.udl | 36 +- components/logins/src/store.rs | 79 +-- components/logins/src/sync/engine.rs | 62 +- components/logins/src/sync/payload.rs | 30 +- examples/sync-pass/src/sync-pass.rs | 94 ++- testing/sync-test/src/auth.rs | 4 +- testing/sync-test/src/logins.rs | 136 ++-- 10 files changed, 633 insertions(+), 795 deletions(-) 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();