diff --git a/CHANGELOG.md b/CHANGELOG.md index bba0478286..77849c51a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ [Full Changelog](In progress) +## ✨ What's Changed ✨ + +### Logins + +- Add `LoginStore::list_candidates()` and `LoginStore::get_many()`, a pair of read APIs for consumers which filter logins on their unencrypted fields. `list_candidates()` returns a `LoginCandidate` per stored login - everything `Login` has except the secure fields (`username`/`password`), so searching by `origin`, `httpRealm` or `formActionOrigin` no longer forces a primary password prompt. `get_many()` then decrypts just the logins which matched. `list()` is unchanged, for callers who really do want every login in cleartext. + # v155.0 (_2026-08-13_) [Full Changelog](https://github.com/mozilla/application-services/compare/v154.0...v155.0) diff --git a/components/logins/src/db.rs b/components/logins/src/db.rs index 5315d92bac..89ccec39c0 100644 --- a/components/logins/src/db.rs +++ b/components/logins/src/db.rs @@ -194,6 +194,27 @@ impl LoginDb { rows.collect::>() } + /// Like `get_all()`, but only the logins with the given guids. Guids we don't have a login + /// for are simply absent from the result, so this can return fewer rows than it was given + /// ids. As with `get_all()` the order of the rows is whatever the query gives us - in + /// particular it is not the order of `ids`. + pub fn get_many(&self, ids: &[String]) -> Result> { + let mut logins = Vec::with_capacity(ids.len()); + sql_support::each_chunk(ids, |chunk, _| -> Result<()> { + logins.extend(self.db.query_rows_and_then( + &format!( + "SELECT * FROM ({}) WHERE guid IN ({})", + &*GET_ALL_SQL, + sql_support::repeat_sql_values(chunk.len()) + ), + rusqlite::params_from_iter(chunk), + EncryptedLogin::from_row, + )?); + Ok(()) + })?; + Ok(logins) + } + pub fn get_by_base_domain(&self, base_domain: &str) -> Result> { // We first parse the input string as a host so it is normalized. let base_host = match Host::parse(base_domain) { @@ -1387,6 +1408,51 @@ mod tests { assert_eq!(db.get_all().unwrap().len(), 2); } + #[test] + fn test_get_many() { + ensure_initialized(); + + let db = LoginDb::open_in_memory(); + let mut added = Vec::new(); + for origin in ["https://a.example.com", "https://b.example.com"] { + added.push( + db.add(LoginEntry { + origin: origin.into(), + http_realm: Some("https://www.example.com".into()), + username: "test".into(), + password: "sekret".into(), + ..LoginEntry::default() + }) + .expect("should be able to add login"), + ); + } + let ids = added.iter().map(|l| l.meta.id.clone()).collect::>(); + + // Neither `get_many()` nor `get_all()` promises an order, so compare them sorted. + let by_origin = |logins: Vec| { + let mut logins = logins; + logins.sort_by(|l, r| l.fields.origin.cmp(&r.fields.origin)); + logins + }; + + // Asking for every id gives us exactly what `get_all()` does. + assert_eq!( + by_origin(db.get_many(&ids).unwrap()), + by_origin(db.get_all().unwrap()) + ); + + // A subset gives us just that subset... + assert_eq!(db.get_many(&ids[1..]).unwrap(), added[1..]); + + // ...and ids we don't have a login for are absent rather than an error. + assert_eq!( + db.get_many(&[ids[0].clone(), "no-such-guid".to_string()]) + .unwrap(), + added[..1] + ); + assert_eq!(db.get_many(&[]).unwrap(), Vec::new()); + } + #[test] fn test_add_many() { ensure_initialized(); diff --git a/components/logins/src/login.rs b/components/logins/src/login.rs index 22bfdfe209..f7e20eff80 100644 --- a/components/logins/src/login.rs +++ b/components/logins/src/login.rs @@ -19,6 +19,8 @@ //! * [`Login`] - A [`LoginEntry`] plus DB record information. This includes the GUID and metadata //! like time_last_used. //! * [`EncryptedLogin`] -- A Login above with the username/password data encrypted. +//! * [`LoginCandidate`] -- A [`Login`] without the username/password, for callers who want to +//! filter on the cleartext fields before asking for the encryption key. //! * [`LoginFields`], [`SecureLoginFields`], [`LoginMeta`] -- These group the common fields in the //! structs above. //! @@ -718,6 +720,52 @@ impl EncryptedLogin { } } +/// A login stored in the database, minus the encrypted fields. +/// +/// Getting one of these never needs the encryption key, so callers which only match on the +/// cleartext fields (eg, `origin`) can do so without forcing the user to authenticate. Once +/// they know which logins they want, `LoginStore::get_many()` decrypts just those. +#[derive(Debug, Clone, Hash, PartialEq, Eq, Default)] +pub struct LoginCandidate { + // meta fields + pub id: String, + pub time_created: i64, + pub time_password_changed: i64, + pub time_last_used: i64, + pub times_used: i64, + + // breach alerts + pub time_last_breach_alert_dismissed: Option, + + // login fields + pub origin: String, + pub form_action_origin: Option, + pub http_realm: Option, + pub username_field: String, + pub password_field: String, +} + +impl From for LoginCandidate { + fn from(login: EncryptedLogin) -> Self { + // Note the `sec_fields` are simply dropped - we never look at the key. + let EncryptedLogin { meta, fields, .. } = login; + Self { + id: meta.id, + time_created: meta.time_created, + time_password_changed: meta.time_password_changed, + time_last_used: meta.time_last_used, + times_used: meta.times_used, + time_last_breach_alert_dismissed: meta.time_last_breach_alert_dismissed, + + 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, + } + } +} + fn string_or_default(row: &Row<'_>, col: &str) -> Result { Ok(row.get::<_, Option>(col)?.unwrap_or_default()) } diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index bc9cafc625..8f0492d945 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -98,6 +98,28 @@ dictionary Login { string username; }; +/// A login stored in the database, minus the encrypted fields. +/// +/// Reading these never needs the encryption key, so consumers which filter on the cleartext +/// fields can do so without forcing the user to authenticate. See `list_candidates()`. +dictionary LoginCandidate { + // meta fields + string id; + i64 times_used; + i64 time_created; + i64 time_last_used; + i64 time_password_changed; + // breach fields + i64? time_last_breach_alert_dismissed; + + // login fields + string origin; + string? http_realm; + string? form_action_origin; + string username_field; + string password_field; +}; + /// Metrics tracking deletion of logins that cannot be decrypted, see `delete_undecryptable_records_for_remote_replacement` /// for more details dictionary LoginsDeletionMetrics { @@ -292,6 +314,11 @@ interface LoginStore { [Throws=LoginsApiError] sequence list(); + /// Like `list()`, but without the encrypted fields - and so without needing the encryption + /// key. Resolve the ids you're interested in with `get_many()`. + [Throws=LoginsApiError] + sequence list_candidates(); + [Throws=LoginsApiError] sequence get_by_base_domain([ByRef] string base_domain); @@ -304,6 +331,11 @@ interface LoginStore { [Throws=LoginsApiError] Login? get([ByRef] string id); + /// Get and decrypt the logins with the given ids. Ids which don't exist are skipped, as are + /// logins which fail to decrypt. + [Throws=LoginsApiError] + sequence get_many(sequence ids); + /// Run maintenance on the DB /// /// This is intended to be run during idle time and will take steps / to clean up / shrink the diff --git a/components/logins/src/store.rs b/components/logins/src/store.rs index b347376a06..0854c12e58 100644 --- a/components/logins/src/store.rs +++ b/components/logins/src/store.rs @@ -4,7 +4,9 @@ use crate::db::{LoginDb, LoginsDeletionMetrics}; use crate::encryption::EncryptorDecryptor; use crate::error::*; -use crate::login::{BulkResultEntry, EncryptedLogin, Login, LoginEntry, LoginEntryWithMeta}; +use crate::login::{ + BulkResultEntry, EncryptedLogin, Login, LoginCandidate, LoginEntry, LoginEntryWithMeta, +}; use crate::LoginsSyncEngine; use parking_lot::Mutex; use sql_support::run_maintenance; @@ -120,6 +122,21 @@ impl LoginStore { }) } + /// List all logins without decrypting them. + /// + /// Unlike `list()` this never touches the encryption key, so consumers which only need the + /// cleartext fields to decide which logins they care about can filter without forcing the + /// user to authenticate. Feed the ids of the matches to `get_many()`. + #[handle_error(Error)] + pub fn list_candidates(&self) -> ApiResult> { + Ok(self + .lock_db()? + .get_all()? + .into_iter() + .map(LoginCandidate::from) + .collect()) + } + #[handle_error(Error)] pub fn count(&self) -> ApiResult { self.lock_db()?.count_all() @@ -148,6 +165,20 @@ impl LoginStore { } } + /// Get the logins with the given ids, decrypting them. + /// + /// This is the other half of `list_candidates()`: having filtered on the cleartext fields, + /// only pay for decrypting the logins which actually matched. Ids we don't have a login for + /// are skipped; a login we can't decrypt fails the call, as it does for `list()`. + #[handle_error(Error)] + pub fn get_many(&self, ids: Vec) -> ApiResult> { + let db = self.lock_db()?; + db.get_many(&ids)? + .into_iter() + .map(|login| login.decrypt(db.encdec.as_ref())) + .collect() + } + #[handle_error(Error)] pub fn get_by_base_domain(&self, base_domain: &str) -> ApiResult> { let db = self.lock_db()?; @@ -388,9 +419,11 @@ impl Default for RunMaintenanceOptions { #[cfg(test)] mod tests { use super::*; + use crate::encryption::{create_key, KeyManager, ManagedEncryptorDecryptor}; use crate::util; use nss_as::ensure_initialized; use std::cmp::Reverse; + use std::sync::atomic::{AtomicUsize, Ordering}; use std::time::SystemTime; fn assert_logins_equiv(a: &LoginEntry, b: &Login) { @@ -417,8 +450,8 @@ mod tests { 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(), + username: "user".into(), + password: "password".into(), ..Default::default() }; @@ -573,6 +606,136 @@ mod tests { assert!(store.db.lock().is_none()); } + /// A `KeyManager` which counts how often it was asked for the key, so we can prove + /// `list_candidates()` never asks. + struct CountingKeyManager { + key: String, + calls: AtomicUsize, + } + + impl KeyManager for CountingKeyManager { + fn get_key(&self) -> ApiResult> { + self.calls.fetch_add(1, Ordering::SeqCst); + Ok(self.key.as_bytes().into()) + } + } + + fn store_with_encdec(encdec: Arc) -> LoginStore { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + LoginStore::new_from_db(LoginDb::with_connection(conn, encdec).unwrap()) + } + + fn test_entry(origin: &str, username: &str) -> LoginEntry { + LoginEntry { + origin: origin.into(), + http_realm: Some("Some Realm".into()), + username: username.into(), + password: "p4ssw0rd".into(), + ..Default::default() + } + } + + #[test] + fn test_list_candidates_does_not_need_the_key() { + ensure_initialized(); + + let key_manager = Arc::new(CountingKeyManager { + key: create_key().unwrap(), + calls: AtomicUsize::new(0), + }); + let store = store_with_encdec(Arc::new(ManagedEncryptorDecryptor::new( + key_manager.clone(), + ))); + + let a = store + .add(test_entry("https://www.a.com", "a-user")) + .unwrap(); + let b = store + .add(test_entry("https://www.b.com", "b-user")) + .unwrap(); + + // Adding needed the key; listing the candidates must not. + assert!(key_manager.calls.load(Ordering::SeqCst) > 0); + key_manager.calls.store(0, Ordering::SeqCst); + + let mut candidates = store.list_candidates().unwrap(); + assert_eq!(key_manager.calls.load(Ordering::SeqCst), 0); + + candidates.sort_by(|l, r| l.origin.cmp(&r.origin)); + assert_eq!(candidates.len(), 2); + for (candidate, login) in candidates.iter().zip([&a, &b]) { + assert_eq!(candidate.id, login.id); + assert_eq!(candidate.origin, login.origin); + assert_eq!(candidate.http_realm, login.http_realm); + assert_eq!(candidate.form_action_origin, login.form_action_origin); + assert_eq!(candidate.username_field, login.username_field); + assert_eq!(candidate.password_field, login.password_field); + assert_eq!(candidate.times_used, login.times_used); + assert_eq!(candidate.time_created, login.time_created); + assert_eq!(candidate.time_last_used, login.time_last_used); + assert_eq!(candidate.time_password_changed, login.time_password_changed); + assert_eq!( + candidate.time_last_breach_alert_dismissed, + login.time_last_breach_alert_dismissed + ); + } + } + + #[test] + fn test_get_many() { + ensure_initialized(); + + let store = LoginStore::new_in_memory(); + let a = store + .add(test_entry("https://www.a.com", "a-user")) + .unwrap(); + let b = store + .add(test_entry("https://www.b.com", "b-user")) + .unwrap(); + store + .add(test_entry("https://www.c.com", "c-user")) + .unwrap(); + + assert_eq!(store.get_many(vec![]).unwrap(), vec![]); + + // Ids we don't know about are skipped rather than being an error. Note the results come + // back in the db's order, not the order of the ids we asked for. + let mut got = store + .get_many(vec![b.id.clone(), "no-such-guid".to_string(), a.id.clone()]) + .unwrap(); + got.sort_by(|l, r| l.origin.cmp(&r.origin)); + assert_eq!(got, vec![a, b]); + } + + #[test] + fn test_get_many_with_an_undecryptable_login() { + ensure_initialized(); + + let store = LoginStore::new_in_memory(); + let a = store + .add(test_entry("https://www.a.com", "a-user")) + .unwrap(); + let b = store + .add(test_entry("https://www.b.com", "b-user")) + .unwrap(); + + store + .lock_db() + .unwrap() + .db + .execute( + "UPDATE loginsL SET secFields = 'not-a-ciphertext' WHERE guid = ?", + [&b.id], + ) + .unwrap(); + + // As with `list()`, one login we can't read fails the whole call. + assert!(matches!( + store.get_many(vec![a.id.clone(), b.id]), + Err(LoginsApiError::UnexpectedLoginsApiError { .. }) + )); + } + #[test] fn test_delete_undecryptable_records_for_remote_replacement() { ensure_initialized();