Skip to content

Commit

Permalink
Removing potential_dupes_ignoring_username()
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@bendk
bendk committed Oct 22, 2021
1 parent da937fb commit e625c03
Show file tree
Hide file tree
Showing 4 changed files with 78 additions and 91 deletions.
138 changes: 67 additions & 71 deletions components/logins/src/db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,51 +237,20 @@ impl LoginDb {
look: LoginEntry,
encdec: &EncryptorDecryptor,
) -> Result<Option<Login>> {
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::<Result<Vec<Login>>>()?;
// 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
Expand Down Expand Up @@ -634,58 +603,85 @@ impl LoginDb {
entry: &LoginEntry,
encdec: &EncryptorDecryptor,
) -> Result<Option<Guid>> {
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<Vec<EncryptedLogin>> {
// 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<Vec<EncryptedLogin>> {
// 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<bool> {
Expand Down
4 changes: 0 additions & 4 deletions components/logins/src/logins.udl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<EncryptedLogin> potential_dupes_ignoring_username([ByRef]string id, LoginEntry login);

[Throws=LoginsStorageError]
EncryptedLogin? get([ByRef] string id);

Expand Down
15 changes: 11 additions & 4 deletions components/logins/src/migrate_sqlcipher_db.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down
12 changes: 0 additions & 12 deletions components/logins/src/store.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down Expand Up @@ -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<Vec<EncryptedLogin>> {
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)
}
Expand Down

0 comments on commit e625c03

Please sign in to comment.