From 755c6551fe1770968b38945b7bf5539e184ad82a Mon Sep 17 00:00:00 2001 From: Tessa Heidkamp Date: Thu, 11 Jun 2026 11:03:05 +0200 Subject: [PATCH 1/2] Bug 2049263 - Add LoginsBridgedEngine to expose logins sync to Desktop via UniFFI Co-Authored-By: Johannes Salas Schmidt Co-Authored-By: Krist Baliev --- CHANGELOG.md | 6 + components/logins/src/error.rs | 11 ++ components/logins/src/lib.rs | 2 +- components/logins/src/logins.udl | 48 +++++ components/logins/src/sync/bridge.rs | 275 +++++++++++++++++++++++++++ components/logins/src/sync/engine.rs | 23 ++- components/logins/src/sync/mod.rs | 2 + 7 files changed, 357 insertions(+), 10 deletions(-) create mode 100644 components/logins/src/sync/bridge.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 400e2642b3..7f9882ca1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,15 @@ [Full Changelog](In progress) +## ✨ What's New ✨ + ### Glean - Updated to v68.0.0 ([#7438](https://github.com/mozilla/application-services/issues/7438)) +### Logins + +- Add `LoginStore.bridgedEngine()`, which exposes the logins sync engine to Desktop's Sync. ([bug 2049263](https://bugzilla.mozilla.org/show_bug.cgi?id=2049263)) + # v153.0 (_2026-06-15_) ## ⚠️ Breaking Changes ⚠️ diff --git a/components/logins/src/error.rs b/components/logins/src/error.rs index 6198bd07a5..bc183effee 100644 --- a/components/logins/src/error.rs +++ b/components/logins/src/error.rs @@ -195,6 +195,17 @@ impl GetErrorHandling for Error { } } +// The bridged sync engine (`sync::bridge`) deals in `anyhow::Result`, as that's +// what the `sync15` BridgedEngine traits use. This lets UniFFI map those errors +// onto our public error type when the bridge methods are exposed via the UDL. +impl From for LoginsApiError { + fn from(value: anyhow::Error) -> Self { + LoginsApiError::UnexpectedLoginsApiError { + reason: value.to_string(), + } + } +} + impl From for LoginsApiError { fn from(error: uniffi::UnexpectedUniFFICallbackError) -> Self { LoginsApiError::UnexpectedLoginsApiError { diff --git a/components/logins/src/lib.rs b/components/logins/src/lib.rs index 47707ca92a..cd430b4e13 100644 --- a/components/logins/src/lib.rs +++ b/components/logins/src/lib.rs @@ -29,7 +29,7 @@ use crate::encryption::{check_canary, create_canary, create_key}; pub use crate::error::*; pub use crate::login::*; pub use crate::store::*; -pub use crate::sync::LoginsSyncEngine; +pub use crate::sync::{LoginsBridgedEngine, LoginsSyncEngine}; use std::sync::Arc; // Utility function to create a StaticKeyManager to be used for the time being until support lands diff --git a/components/logins/src/logins.udl b/components/logins/src/logins.udl index 8d306b8886..3feea9642d 100644 --- a/components/logins/src/logins.udl +++ b/components/logins/src/logins.udl @@ -301,10 +301,58 @@ interface LoginStore { [Self=ByArc] void register_with_sync_manager(); + /// Returns a bridged sync engine for Desktop's Sync framework. + /// Without this UDL entry the engine is invisible to JS: UniFFI generates + /// the XPCOM glue that lets JS call `rustStore.bridgedEngine()`. + [Throws=LoginsApiError, Self=ByArc] + LoginsBridgedEngine bridged_engine(); + [Self=ByArc] void shutdown(); }; +/// The Desktop-facing bridged sync engine. The canonical docs are in +/// https://searchfox.org/mozilla-central/source/services/interfaces/mozIBridgedSyncEngine.idl +/// It's only actually used on Desktop, but it's fine to expose this everywhere. +/// NOTE: all timestamps here are milliseconds. +interface LoginsBridgedEngine { + [Throws=LoginsApiError] + i64 last_sync(); + + [Throws=LoginsApiError] + void set_last_sync(i64 last_sync); + + [Throws=LoginsApiError] + string? sync_id(); + + [Throws=LoginsApiError] + string reset_sync_id(); + + [Throws=LoginsApiError] + string ensure_current_sync_id([ByRef]string new_sync_id); + + [Throws=LoginsApiError] + void sync_started(); + + [Throws=LoginsApiError] + void store_incoming(sequence incoming_envelopes_as_json); + + [Throws=LoginsApiError] + sequence apply(); + + [Throws=LoginsApiError] + void set_uploaded(i64 new_timestamp, sequence uploaded_ids); + + [Throws=LoginsApiError] + void sync_finished(); + + [Throws=LoginsApiError] + void reset(); + + [Throws=LoginsApiError] + void wipe(); +}; + dictionary RunMaintenanceOptions { // Wipe un-decryptable logins. These will hopefully come back on the next sync. boolean delete_undecryptable_records_for_remote_replacement=true; diff --git a/components/logins/src/sync/bridge.rs b/components/logins/src/sync/bridge.rs new file mode 100644 index 0000000000..ff45c53cfc --- /dev/null +++ b/components/logins/src/sync/bridge.rs @@ -0,0 +1,275 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +use crate::sync::engine::LoginsSyncEngine; +use crate::LoginStore; +use anyhow::Result; +use std::sync::Arc; +use sync15::bso::{IncomingBso, OutgoingBso}; +use sync15::engine::{BridgedEngine, BridgedEngineAdaptor}; +use sync15::ServerTimestamp; +use sync_guid::Guid as SyncGuid; + +impl LoginStore { + /// Returns a bridged sync engine for Desktop for this store. + /// + /// Unlike Tabs, constructing a `LoginsSyncEngine` locks the DB and can + /// fail, so this is fallible (and exposed as `[Throws]` in the UDL). The + /// internal error is surfaced via `anyhow`, which UniFFI maps onto + /// `LoginsApiError` through `From`. + pub fn bridged_engine(self: Arc) -> Result> { + let engine = LoginsSyncEngine::new(self)?; + let bridged_engine = LoginsBridgedEngineAdaptor { engine }; + Ok(Arc::new(LoginsBridgedEngine::new(Box::new(bridged_engine)))) + } +} + +/// `LoginsSyncEngine` only implements the internal `sync15::SyncEngine` trait, +/// which is what the mobile (Android/iOS) sync manager drives. Desktop's Sync +/// framework instead speaks the `mozIBridgedSyncEngine` interface, whose Rust +/// shape is `sync15::BridgedEngine`. This adaptor wraps our `SyncEngine` and, +/// via the blanket `impl BridgedEngine for A`, gives +/// us a `BridgedEngine` for free. The adaptor exists only because these two +/// sync-engine traits still live side by side; it can go away if they're ever +/// unified. +struct LoginsBridgedEngineAdaptor { + engine: LoginsSyncEngine, +} + +/// see sync15/src/engine/bridged_engine.rs for required functions for the trait +impl BridgedEngineAdaptor for LoginsBridgedEngineAdaptor { + fn last_sync(&self) -> Result { + // `get_last_sync` takes the `&LoginDb` to avoid deadlocking when called + // mid-sync (while the lock is already held). The bridge methods are + // always called outside a sync transaction, so we can lock here. + let db = self.engine.store.lock_db()?; + Ok(self + .engine + .get_last_sync(&db)? + .unwrap_or_default() + .as_millis()) + } + + fn set_last_sync(&self, last_sync_millis: i64) -> Result<()> { + let db = self.engine.store.lock_db()?; + self.engine + .set_last_sync(&db, ServerTimestamp::from_millis(last_sync_millis))?; + Ok(()) + } + + fn engine(&self) -> &dyn sync15::engine::SyncEngine { + &self.engine + } +} + +// This is what UniFFI exposes; it does nothing other than delegate back to the +// `BridgedEngine` trait object (and handle the JSON (de)serialization of BSOs +// that crosses the FFI boundary). +/// see services/interfaces/mozIBridgedSyncEngine.idl for contract +pub struct LoginsBridgedEngine { + bridge_impl: Box, +} + +impl LoginsBridgedEngine { + pub fn new(bridge_impl: Box) -> Self { + Self { bridge_impl } + } + + pub fn last_sync(&self) -> Result { + self.bridge_impl.last_sync() + } + + pub fn set_last_sync(&self, last_sync: i64) -> Result<()> { + self.bridge_impl.set_last_sync(last_sync) + } + + pub fn sync_id(&self) -> Result> { + self.bridge_impl.sync_id() + } + + pub fn reset_sync_id(&self) -> Result { + self.bridge_impl.reset_sync_id() + } + + pub fn ensure_current_sync_id(&self, sync_id: &str) -> Result { + self.bridge_impl.ensure_current_sync_id(sync_id) + } + + pub fn sync_started(&self) -> Result<()> { + self.bridge_impl.sync_started() + } + + // Decode the JSON-encoded IncomingBso's that UniFFI passes to us + fn convert_incoming_bsos(&self, incoming: Vec) -> Result> { + let mut bsos = Vec::with_capacity(incoming.len()); + for inc in incoming { + bsos.push(serde_json::from_str::(&inc)?); + } + Ok(bsos) + } + + // Encode OutgoingBso's into JSON for UniFFI + fn convert_outgoing_bsos(&self, outgoing: Vec) -> Result> { + let mut bsos = Vec::with_capacity(outgoing.len()); + for e in outgoing { + bsos.push(serde_json::to_string(&e)?); + } + Ok(bsos) + } + + pub fn store_incoming(&self, incoming: Vec) -> Result<()> { + self.bridge_impl + .store_incoming(self.convert_incoming_bsos(incoming)?) + } + + pub fn apply(&self) -> Result> { + let apply_results = self.bridge_impl.apply()?; + self.convert_outgoing_bsos(apply_results.records) + } + + pub fn set_uploaded(&self, server_modified_millis: i64, guids: Vec) -> Result<()> { + // UniFFI hands us plain strings; the bridge works in terms of `Guid`. + let guids: Vec = guids.into_iter().map(SyncGuid::from).collect(); + self.bridge_impl + .set_uploaded(server_modified_millis, &guids) + } + + pub fn sync_finished(&self) -> Result<()> { + self.bridge_impl.sync_finished() + } + + pub fn reset(&self) -> Result<()> { + self.bridge_impl.reset() + } + + pub fn wipe(&self) -> Result<()> { + self.bridge_impl.wipe() + } +} + +#[cfg(not(feature = "keydb"))] +#[cfg(test)] +mod tests { + use super::*; + use crate::db::test_utils::insert_login; + use nss_as::ensure_initialized; + use std::collections::HashMap; + + // Exercises the sync-metadata plumbing (last_sync / sync_id / reset) that + // Desktop's Sync framework drives through the bridge, mirroring the Tabs + // `test_sync_meta` test. + #[test] + fn test_sync_meta() { + ensure_initialized(); + error_support::init_for_tests(); + + let store = Arc::new(LoginStore::new_in_memory()); + let bridge = store.bridged_engine().expect("should create bridge"); + + // Fresh DB: never synced. + assert_eq!(bridge.last_sync().unwrap(), 0); + bridge.set_last_sync(3).unwrap(); + assert_eq!(bridge.last_sync().unwrap(), 3); + + assert!(bridge.sync_id().unwrap().is_none()); + + bridge.ensure_current_sync_id("some_guid").unwrap(); + assert_eq!(bridge.sync_id().unwrap(), Some("some_guid".to_string())); + // changing the sync ID should reset the timestamp + assert_eq!(bridge.last_sync().unwrap(), 0); + bridge.set_last_sync(3).unwrap(); + + bridge.reset_sync_id().unwrap(); + // should now be a random guid. + assert_ne!(bridge.sync_id().unwrap(), Some("some_guid".to_string())); + // should have reset the last sync timestamp. + assert_eq!(bridge.last_sync().unwrap(), 0); + bridge.set_last_sync(3).unwrap(); + + // `reset` clears the guid and the timestamp + bridge.reset().unwrap(); + assert_eq!(bridge.last_sync().unwrap(), 0); + assert!(bridge.sync_id().unwrap().is_none()); + } + + // A roundtrip through the bridge's data path: stage an incoming remote + // login, apply it, and confirm the local-only login comes back out for + // upload. Unlike `test_sync_meta`, this exercises the JSON (de)serialization + // of BSOs and the staged-incoming `Mutex`. Mirrors the Tabs + // `test_sync_via_bridge` test. + #[test] + fn test_sync_via_bridge() { + ensure_initialized(); + error_support::init_for_tests(); + + let store = Arc::new(LoginStore::new_in_memory()); + + // A local-only login: nothing on the server knows about it yet, so it + // should be uploaded. + insert_login( + &store.lock_db().unwrap(), + "local-only-aaaa", + Some("local-password"), + None, + ); + + let bridge = store + .clone() + .bridged_engine() + .expect("should create bridge"); + + bridge.sync_started().unwrap(); + + // An incoming remote login that isn't known locally. We build the + // envelope as raw JSON, exactly as the JS bridge hands it to us. + let incoming = vec![serde_json::json!({ + "id": "remote-only-bbbb", + "modified": 0, + "payload": serde_json::json!({ + "id": "remote-only-bbbb", + "hostname": "https://remote.example.com", + "formSubmitURL": "https://remote.example.com", + "username": "remote-user", + "password": "remote-password", + }) + .to_string(), + }) + .to_string()]; + bridge + .store_incoming(incoming) + .expect("should store incoming"); + + // Applying stores the remote record locally and returns the local-only + // login for upload. + let outgoing = bridge.apply().expect("should apply"); + let changes: HashMap = outgoing + .into_iter() + .map(|s| { + let bso: serde_json::Value = serde_json::from_str(&s).unwrap(); + let payload: serde_json::Value = + serde_json::from_str(bso["payload"].as_str().unwrap()).unwrap(); + (payload["id"].as_str().unwrap().to_string(), payload) + }) + .collect(); + + // Only the local login is outgoing; the just-applied remote one is not + // re-uploaded. + assert_eq!(changes.len(), 1); + assert_eq!(changes["local-only-aaaa"]["password"], "local-password"); + + // The incoming remote login was actually persisted. + let stored = store + .get("remote-only-bbbb") + .unwrap() + .expect("remote login should have been stored"); + assert_eq!(stored.password, "remote-password"); + + // Acknowledging the upload advances last_sync. + bridge + .set_uploaded(1234, vec!["local-only-aaaa".to_string()]) + .unwrap(); + bridge.sync_finished().unwrap(); + assert_eq!(bridge.last_sync().unwrap(), 1234); + } +} diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 6e816ac719..8f0149b523 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -16,9 +16,8 @@ use crate::LoginStore; use interrupt_support::SqlInterruptScope; use rusqlite::named_params; use sql_support::ConnExt; -use std::cell::RefCell; use std::collections::HashSet; -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::time::{Duration, UNIX_EPOCH}; use sync15::bso::{IncomingBso, OutgoingBso, OutgoingEnvelope}; use sync15::engine::{CollSyncIds, CollectionRequest, EngineSyncAssociation, SyncEngine}; @@ -30,7 +29,9 @@ pub struct LoginsSyncEngine { pub store: Arc, pub scope: SqlInterruptScope, pub encdec: Arc, - pub staged: RefCell>, + // `Mutex` (rather than `RefCell`) so the engine is `Sync`, which the + // Desktop `BridgedEngineAdaptor` requires. Only ever locked briefly. + pub staged: Mutex>, } impl LoginsSyncEngine { @@ -43,7 +44,7 @@ impl LoginsSyncEngine { store, encdec, scope, - staged: RefCell::new(vec![]), + staged: Mutex::new(vec![]), }) } @@ -292,9 +293,13 @@ impl LoginsSyncEngine { db.put_meta(schema::LAST_SYNC_META_KEY, &last_sync_millis) } - fn get_last_sync(&self, db: &LoginDb) -> Result> { - let millis = db.get_meta::(schema::LAST_SYNC_META_KEY)?.unwrap(); - Ok(Some(ServerTimestamp(millis))) + // Public so the bridged engine (`sync::bridge`) can read the last-sync + // timestamp without needing access to the private internals here. Returns + // `None` when we've never synced, rather than panicking on a fresh DB. + pub fn get_last_sync(&self, db: &LoginDb) -> Result> { + Ok(db + .get_meta::(schema::LAST_SYNC_META_KEY)? + .map(ServerTimestamp)) } fn mark_as_synchronized(&self, guids: &[&str], ts: ServerTimestamp) -> Result<()> { @@ -424,7 +429,7 @@ impl SyncEngine for LoginsSyncEngine { ) -> anyhow::Result<()> { // We don't have cross-item dependencies like bookmarks does, so we can // just apply now instead of "staging" - self.staged.borrow_mut().append(&mut inbound); + self.staged.lock().unwrap().append(&mut inbound); Ok(()) } @@ -433,7 +438,7 @@ impl SyncEngine for LoginsSyncEngine { timestamp: ServerTimestamp, telem: &mut telemetry::Engine, ) -> anyhow::Result> { - let inbound = (*self.staged.borrow_mut()).drain(..).collect(); + let inbound = self.staged.lock().unwrap().drain(..).collect(); Ok(self.do_apply_incoming(inbound, timestamp, telem)?) } diff --git a/components/logins/src/sync/mod.rs b/components/logins/src/sync/mod.rs index 7e8ac45542..0ea4c679ae 100644 --- a/components/logins/src/sync/mod.rs +++ b/components/logins/src/sync/mod.rs @@ -2,11 +2,13 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +mod bridge; mod engine; pub(crate) mod merge; mod payload; mod update_plan; +pub use bridge::LoginsBridgedEngine; pub use engine::LoginsSyncEngine; use payload::{IncomingLogin, LoginPayload}; From d3cbf66b6395f3a88819d83c605056e6536e6b4d Mon Sep 17 00:00:00 2001 From: Johannes Salas Schmidt Date: Mon, 22 Jun 2026 11:19:29 +0200 Subject: [PATCH 2/2] Bug 2049263 - Exclude FxA credentials login from logins sync The Rust logins sync engine now skips the chrome://FirefoxAccounts FxA-login when collecting outgoing records, matching the exclusion the JS PasswordEngine does on Desktop. Without this, the FxA session credentials would be uploaded once Desktop syncs logins through the bridged engine. --- components/logins/src/sync/engine.rs | 86 ++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 18 deletions(-) diff --git a/components/logins/src/sync/engine.rs b/components/logins/src/sync/engine.rs index 8f0149b523..c1e1a538d2 100644 --- a/components/logins/src/sync/engine.rs +++ b/components/logins/src/sync/engine.rs @@ -24,6 +24,13 @@ use sync15::engine::{CollSyncIds, CollectionRequest, EngineSyncAssociation, Sync use sync15::{telemetry, ServerTimestamp}; use sync_guid::Guid; +// The Desktop FxA session-credentials pseudo-login. Firefox stores its account +// credentials as a login under this origin; it must never be synced. This +// mirrors the exclusion the JS `PasswordEngine` does via +// `Utils.getSyncCredentialsHosts()`. Only relevant on Desktop (mobile never has +// such a login), but it's harmless to filter everywhere. +const FXA_CREDENTIALS_ORIGIN: &str = "chrome://FirefoxAccounts"; + // The sync engine. pub struct LoginsSyncEngine { pub store: Arc, @@ -246,26 +253,31 @@ impl LoginsSyncEngine { let mut stmt = db.prepare_cached(&format!( "SELECT L.*, M.enc_unknown_fields FROM loginsL L LEFT JOIN loginsM M ON L.guid = M.guid - WHERE sync_status IS NOT {synced}", + WHERE sync_status IS NOT {synced} + -- Never sync Desktop's FxA session-credentials pseudo-login. + AND L.origin IS NOT :fxa_origin", synced = SyncStatus::Synced as u8 ))?; - let bsos = stmt.query_and_then([], |row| { - self.scope.err_if_interrupted()?; - Ok(if row.get::<_, bool>("is_deleted")? { - let envelope = OutgoingEnvelope { - id: row.get::<_, String>("guid")?.into(), - sortindex: Some(TOMBSTONE_SORTINDEX), - ..Default::default() - }; - OutgoingBso::new_tombstone(envelope) - } else { - let unknown = row.get::<_, Option>("enc_unknown_fields")?; - let mut bso = - EncryptedLogin::from_row(row)?.into_bso(self.encdec.as_ref(), unknown)?; - bso.envelope.sortindex = Some(DEFAULT_SORTINDEX); - bso - }) - })?; + let bsos = stmt.query_and_then( + named_params! { ":fxa_origin": FXA_CREDENTIALS_ORIGIN }, + |row| { + self.scope.err_if_interrupted()?; + Ok(if row.get::<_, bool>("is_deleted")? { + let envelope = OutgoingEnvelope { + id: row.get::<_, String>("guid")?.into(), + sortindex: Some(TOMBSTONE_SORTINDEX), + ..Default::default() + }; + OutgoingBso::new_tombstone(envelope) + } else { + let unknown = row.get::<_, Option>("enc_unknown_fields")?; + let mut bso = + EncryptedLogin::from_row(row)?.into_bso(self.encdec.as_ref(), unknown)?; + bso.envelope.sortindex = Some(DEFAULT_SORTINDEX); + bso + }) + }, + )?; bsos.collect::>() } @@ -808,6 +820,44 @@ mod tests { assert!(changes["changed"].get("deleted").is_none()); } + #[test] + fn test_fetch_outgoing_excludes_fxa_credentials() { + ensure_initialized(); + let store = LoginStore::new_in_memory(); + + // A normal local login that should be uploaded. + insert_login(&store.lock_db().unwrap(), "normal", Some("password"), None); + + // Desktop's FxA session-credentials pseudo-login must never be synced. + store + .add(LoginEntry { + origin: FXA_CREDENTIALS_ORIGIN.to_string(), + http_realm: Some("Firefox Accounts credentials".to_string()), + username: "uid".to_string(), + password: "sync-token".to_string(), + ..Default::default() + }) + .unwrap(); + + let changeset = run_fetch_outgoing(store); + let changes: HashMap = changeset + .into_iter() + .map(|b| { + ( + b.envelope.id.to_string(), + serde_json::from_str(&b.payload).unwrap(), + ) + }) + .collect(); + + // The normal login still uploads; nothing pointing at the FxA origin + // is outgoing. + assert!(changes.contains_key("normal")); + assert!(changes + .values() + .all(|payload| payload["hostname"] != FXA_CREDENTIALS_ORIGIN)); + } + #[test] fn test_bad_record() { ensure_initialized();