From da937fb4c56edd71f986aaf5b47ae5b12b3d00a1 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 15 Oct 2021 11:47:32 -0400 Subject: [PATCH 1/2] Removing `check_valid_with_no_dupes()` It's a deprecated function and not used in either the iOS or Android wrappers. I think we can just ditch it. --- components/logins/src/db.rs | 118 ------------------------------- components/logins/src/logins.udl | 4 -- components/logins/src/store.rs | 13 ---- 3 files changed, 135 deletions(-) diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 5a0c324b3d..311773a32e 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -596,16 +596,6 @@ impl LoginDb { } } - pub fn check_valid_with_no_dupes( - &self, - guid: &Guid, - entry: &LoginEntry, - encdec: &EncryptorDecryptor, - ) -> Result<()> { - entry.check_valid()?; - self.check_for_dupes(guid, entry, encdec) - } - pub fn fixup_and_check_for_dupes( &self, guid: &Guid, @@ -1037,114 +1027,6 @@ mod tests { use crate::sync::LocalLogin; use crate::SecureLoginFields; - #[test] - fn test_check_valid_with_no_dupes() { - let db = LoginDb::open_in_memory().unwrap(); - let added = db - .add( - LoginEntry { - fields: LoginFields { - form_action_origin: Some("https://www.example.com".into()), - origin: "https://www.example.com".into(), - http_realm: None, - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test".into(), - }, - }, - &TEST_ENCRYPTOR, - ) - .unwrap(); - - let unique_login = LoginEntry { - fields: LoginFields { - form_action_origin: None, - 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(), - }, - }; - - let duplicate_login = LoginEntry { - fields: LoginFields { - form_action_origin: Some("https://www.example.com".into()), - origin: "https://www.example.com".into(), - http_realm: None, - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test2".into(), - }, - }; - - let updated_login = LoginEntry { - fields: LoginFields { - form_action_origin: None, - origin: "https://www.example.com".into(), - http_realm: Some("https://www.example.com".into()), - ..Default::default() - }, - sec_fields: SecureLoginFields { - username: "test".into(), - password: "test4".into(), - }, - }; - - struct TestCase { - guid: Guid, - entry: LoginEntry, - should_err: bool, - expected_err: &'static str, - } - - let test_cases = [ - TestCase { - guid: "unique_value".into(), - // unique_login should not error because it does not share the same origin, - // username, and formActionOrigin or httpRealm with the pre-existing login - // (login with guid `added.id`). - entry: unique_login, - should_err: false, - expected_err: "", - }, - TestCase { - guid: "unique_value".into(), - // duplicate_login has the same origin, username, and formActionOrigin as a pre-existing - // login (guid `added.id`) and duplicate_login has no guid value, i.e. its guid - // doesn't match with that of a pre-existing record so it can't be considered update, - // so it should error. - entry: duplicate_login, - should_err: true, - expected_err: "Invalid login: Login already exists", - }, - TestCase { - // updated_login is an update to the existing record (has the same guid) so it is not a dupe - // and should not error. - guid: added.record.id.into(), - entry: updated_login, - should_err: false, - expected_err: "", - }, - ]; - - for tc in &test_cases { - let login_check = db.check_valid_with_no_dupes(&tc.guid, &tc.entry, &TEST_ENCRYPTOR); - if tc.should_err { - assert!(&login_check.is_err()); - assert_eq!(&login_check.unwrap_err().to_string(), tc.expected_err) - } else { - assert!(&login_check.is_ok()) - } - } - } - #[test] fn test_username_dupe_semantics() { let mut login = LoginEntry { diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index 9cb6078e2b..442694aaf7 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -106,10 +106,6 @@ interface LoginStore { [Throws=LoginsStorageError] constructor(string path); - // XXX - Can we kill this, and just fix the semantics of add/update? - [Throws=LoginsStorageError] - void check_valid_with_no_dupes([ByRef] string id, [ByRef] LoginEntry login, [ByRef] string encryption_key); - [Throws=LoginsStorageError] EncryptedLogin add(LoginEntry login, [ByRef]string encryption_key); diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index 09655f370e..c13b38bd49 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -131,19 +131,6 @@ impl LoginStore { Ok(serde_json::to_string(&metrics)?) } - pub fn check_valid_with_no_dupes( - &self, - id: &str, - entry: &LoginEntry, - enc_key: &str, - ) -> Result<()> { - let encdec = crate::encryption::EncryptorDecryptor::new(enc_key)?; - self.db - .lock() - .unwrap() - .check_valid_with_no_dupes(&Guid::new(id), entry, &encdec) - } - /// A convenience wrapper around sync_multiple. // Unfortunately, iOS still uses this until they use the sync manager // This can almost die later - consumers should never call it (they should From e625c03bfff5c17476b2a33c2704cebcf9b6b164 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 15 Oct 2021 13:18:23 -0400 Subject: [PATCH 2/2] Removing `potential_dupes_ignoring_username()` This is another deprecated function, although it was still in use by the dupe checking code. So removing it required more effort. There was a lot of overlap between `potential_dupes_ignoring_username()` and `find_login_to_update()`. I factored out the common parts into the new `get_by_entry_target()` function. Naming this one was hard, but the idea is to search using `origin` plus either `form_action_origin` or `http_realm`. The new function has one difference from the old ones. It checks which of `form_action_origin` or `http_realm` is set and uses that as the query param, rather than throwing both in there. This means: - For valid login entries where exactly 1 is set, it works exactly the same, since NULL comparisons are always false. - If neither or both are set, it returns an error rather than running the query. I think in most cases that query would return 0 rows. This affected one thing, which was the dupe-checking in the migration function tests. I think this was maybe a test-only issue, since we currently don't clear `form_action_origin` and `http_realm` when deleting the login (#4573). I'm not sure what desktop does though. In any case, I think not running the dupe check for deleted logins makes the migration logic slightly safer. --- components/logins/src/db.rs | 138 +++++++++--------- components/logins/src/logins.udl | 4 - components/logins/src/migrate_sqlcipher_db.rs | 15 +- components/logins/src/store.rs | 12 -- 4 files changed, 78 insertions(+), 91 deletions(-) diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 311773a32e..38ac09cb53 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -237,51 +237,20 @@ impl LoginDb { look: LoginEntry, encdec: &EncryptorDecryptor, ) -> Result> { - lazy_static::lazy_static! { - static ref LOGINS_TO_SEARCH_SQL: String = format!( - "SELECT {common_cols} FROM loginsL - WHERE is_deleted = 0 - AND origin = :origin - AND ( - formActionOrigin = :form_action_origin - OR - httpRealm = :http_realm - ) - - UNION ALL - - SELECT {common_cols} FROM loginsM - WHERE is_overridden = 0 - AND origin = :origin - AND ( - formActionOrigin = :form_action_origin - OR - httpRealm = :http_realm - ) - ", - common_cols = schema::COMMON_COLS - ); - } - - // Fetch all potentially matching rows let look = look.fixup()?; - let mut stmt = self.db.prepare_cached(&LOGINS_TO_SEARCH_SQL)?; - let params = named_params! { - ":origin": look.fields.origin, - ":http_realm": look.fields.http_realm, - ":form_action_origin": look.fields.form_action_origin, - }; - let rows = stmt - .query_and_then_named(params, |row| EncryptedLogin::from_row(row)?.decrypt(encdec))? + let logins = self + .get_by_entry_target(&look)? + .into_iter() + .map(|enc_login| enc_login.decrypt(encdec)) .collect::>>()?; - // Search through the results and try to pick a login - Ok(rows + Ok(logins // First, try to match the username .iter() .find(|login| login.sec_fields.username == look.sec_fields.username) // Fall back on a blank username .or_else(|| { - rows.iter() + logins + .iter() .find(|login| login.sec_fields.username.is_empty()) }) // Clone the login to avoid ref issues when returning across the FFI @@ -634,58 +603,85 @@ impl LoginDb { entry: &LoginEntry, encdec: &EncryptorDecryptor, ) -> Result> { - for possible in self.potential_dupes_ignoring_username(guid, &entry.fields)? { - let pos_sec_fields = possible.decrypt_fields(encdec)?; - if pos_sec_fields.username == entry.sec_fields.username { - return Ok(Some(possible.guid())); + 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 { + return Ok(Some(possible.guid())); + } } } Ok(None) } - pub fn potential_dupes_ignoring_username( - &self, - guid: &Guid, - fields: &LoginFields, - ) -> Result> { + // Find saved logins that match the target for a `LoginEntry` + // + // This means that: + // - `origin` matches + // - Either `form_action_origin` or `http_realm` matches, depending on which one is non-null + // + // This is used for dupe-checking and `find_login_to_update()` + fn get_by_entry_target(&self, entry: &LoginEntry) -> Result> { // Could be lazy_static-ed... lazy_static::lazy_static! { - static ref DUPES_IGNORING_USERNAME_SQL: String = format!( + static ref GET_BY_FORM_ACTION_ORIGIN: String = format!( + "SELECT {common_cols} FROM loginsL + WHERE is_deleted = 0 + AND origin = :origin + AND formActionOrigin = :form_action_origin + + UNION ALL + + SELECT {common_cols} FROM loginsM + WHERE is_overridden = 0 + AND origin = :origin + AND formActionOrigin = :form_action_origin + ", + common_cols = schema::COMMON_COLS + ); + static ref GET_BY_HTTP_REALM: String = format!( "SELECT {common_cols} FROM loginsL WHERE is_deleted = 0 - AND guid <> :guid AND origin = :origin - AND ( - formActionOrigin = :form_submit - OR - httpRealm = :http_realm - ) + AND httpRealm = :http_realm UNION ALL SELECT {common_cols} FROM loginsM WHERE is_overridden = 0 - AND guid <> :guid - AND origin = :origin - AND ( - formActionOrigin = :form_submit - OR - httpRealm = :http_realm - ) + AND origin = :origin + AND httpRealm = :http_realm ", common_cols = schema::COMMON_COLS ); } - let mut stmt = self.db.prepare_cached(&DUPES_IGNORING_USERNAME_SQL)?; - let params = named_params! { - ":guid": guid, - ":origin": &fields.origin, - ":http_realm": fields.http_realm.as_ref(), - ":form_submit": fields.form_action_origin.as_ref(), - }; - // Needs to be two lines for borrow checker - let rows = stmt.query_and_then_named(params, EncryptedLogin::from_row)?; - rows.collect() + match ( + entry.fields.form_action_origin.as_ref(), + entry.fields.http_realm.as_ref(), + ) { + (Some(form_action_origin), None) => { + let params = named_params! { + ":origin": &entry.fields.origin, + ":form_action_origin": form_action_origin, + }; + self.db + .prepare_cached(&GET_BY_FORM_ACTION_ORIGIN)? + .query_and_then_named(params, EncryptedLogin::from_row)? + .collect() + } + (None, Some(http_realm)) => { + let params = named_params! { + ":origin": &entry.fields.origin, + ":http_realm": http_realm, + }; + self.db + .prepare_cached(&GET_BY_HTTP_REALM)? + .query_and_then_named(params, EncryptedLogin::from_row)? + .collect() + } + (Some(_), Some(_)) => Err(InvalidLogin::BothTargets.into()), + (None, None) => Err(InvalidLogin::NoTarget.into()), + } } pub fn exists(&self, id: &str) -> Result { diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index 442694aaf7..02b22d37f3 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -139,10 +139,6 @@ interface LoginStore { [Throws=LoginsStorageError] Login? find_login_to_update(LoginEntry look, [ByRef]string encryption_key); - // XXX - Can we kill this, and just fix the semantics of add/update? - [Throws=LoginsStorageError] - sequence potential_dupes_ignoring_username([ByRef]string id, LoginEntry login); - [Throws=LoginsStorageError] EncryptedLogin? get([ByRef] string id); diff --git a/components/logins/src/migrate_sqlcipher_db.rs b/components/logins/src/migrate_sqlcipher_db.rs index 0443a8f5f8..d2afc0bca8 100644 --- a/components/logins/src/migrate_sqlcipher_db.rs +++ b/components/logins/src/migrate_sqlcipher_db.rs @@ -566,10 +566,17 @@ fn insert_local_login( let login = &local_login.login; let dec_login = &local_login.login.clone().decrypt(encdec)?; - if let Err(e) = new_db.check_for_dupes(&login.guid(), &dec_login.entry(), encdec) { - log::warn!("Duplicate {} ({}).", login.record.id, e); - return Ok(()); - }; + // Check for existing duplicate logins + // + // Skip the check for deleted logins, which have issues with the dupe checking logic, since + // http_realm and form_action_origin may be unset. This doesn't currently happen on mobile, + // but it might on desktop (see #4573) + if !local_login.is_deleted { + if let Err(e) = new_db.check_for_dupes(&login.guid(), &dec_login.entry(), encdec) { + log::warn!("Duplicate {} ({}).", login.record.id, e); + return Ok(()); + }; + } match conn.execute_named_cached( sql, named_params! { diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index c13b38bd49..1a13dc65d2 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -9,7 +9,6 @@ use crate::LoginsSyncEngine; use std::path::Path; use std::sync::{Arc, Mutex, Weak}; use sync15::{sync_multiple, EngineSyncAssociation, MemoryCachedState, SyncEngine}; -use sync_guid::Guid; // Our "sync manager" will use whatever is stashed here. lazy_static::lazy_static! { @@ -65,17 +64,6 @@ impl LoginStore { self.db.lock().unwrap().find_login_to_update(entry, &encdec) } - pub fn potential_dupes_ignoring_username( - &self, - id: &str, - entry: LoginEntry, - ) -> Result> { - self.db - .lock() - .unwrap() - .potential_dupes_ignoring_username(&Guid::new(id), &entry.fields) - } - pub fn touch(&self, id: &str) -> Result<()> { self.db.lock().unwrap().touch(id) }