From 23f6c42b1fc3618029fbf0ae5dd0662c6bde6d08 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 6 Oct 2026 21:18:59 +1100 Subject: [PATCH] Add EngineChoicesModel, replacing enabled_changes, for per-device sync engine choices SyncParams.enabled_changes becomes SyncParams.engine_choices_model: EngineChoicesModel::Account { enabled_changes } keeps today's behaviour, and EngineChoicesModel::Device selects per-device engine choices. DeviceSettings gains enabled_engines, the engines this device syncs, which is passed through to the clients engine's Settings so it can later be published for other devices on the account. --- CHANGELOG.md | 5 +++ components/sync15/src/client/sync_multiple.rs | 16 +++---- .../sync15/src/clients_engine/engine.rs | 4 ++ components/sync15/src/clients_engine/mod.rs | 2 + components/sync15/src/engine_choices.rs | 43 +++++++++++++++++++ components/sync15/src/lib.rs | 3 ++ components/sync_manager/src/manager.rs | 15 +++---- components/sync_manager/src/types.rs | 12 +++--- testing/sync-test/src/auth.rs | 8 ++-- 9 files changed, 83 insertions(+), 25 deletions(-) create mode 100644 components/sync15/src/engine_choices.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6123c01e51a..344dd3b6c72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,11 @@ - **BREAKING**: The key-based `encrypt_string(key, ...)` / `decrypt_string(key, ...)` namespace functions are removed - with transparent card numbers nothing needs them. `create_autofill_key()` stays; for canaries use `db_crypto.create_canary` / `check_canary` (format-compatible). - **BREAKING**: Credit card numbers are transparent in the API: `UpdatableCreditCardFields` takes `cc_number` in cleartext (the store encrypts it and derives `cc_number_last_4` itself), and `CreditCard.cc_number` comes back decrypted - empty for a scrubbed card, while a value the key cannot read fails the read. An empty `cc_number` is stored as an empty `cc_number_enc` rather than as a ciphertext of "", so a scrubbed card stays recognizable as such. `cc_number_enc` is gone from both dictionaries; consumers never handle ciphertext. ([bug 2075563](https://bugzilla.mozilla.org/show_bug.cgi?id=2075563)) +### Sync Manager + +- `SyncParams.enabled_changes` is replaced by `engine_choices_model`. Pass `EngineChoicesModel.Account(enabledChanges)` to keep the current behaviour (in Kotlin, `EngineChoicesModel` is in `mozilla.appservices.sync15`). +- `DeviceSettings` has a new optional `enabled_engines` field, not yet used. + ## ✨ What's New ✨ ### Error support diff --git a/components/sync15/src/client/sync_multiple.rs b/components/sync15/src/client/sync_multiple.rs index e24d0d44104..5814cea3fa2 100644 --- a/components/sync15/src/client/sync_multiple.rs +++ b/components/sync15/src/client/sync_multiple.rs @@ -8,11 +8,11 @@ use super::state::{EngineChangesNeeded, GlobalState, PersistedGlobalState, SetupStateMachine}; use super::status::{ServiceStatus, SyncResult}; use super::storage_client::{BackoffListener, Sync15StorageClient, Sync15StorageClientInit}; -use crate::KeyBundle; use crate::clients_engine::{self, CLIENTS_TTL_REFRESH, CommandProcessor}; use crate::engine::{EngineSyncAssociation, SyncEngine}; use crate::error::{Error, debug, info, trace, warn}; use crate::telemetry; +use crate::{EngineChoicesModel, KeyBundle}; use interrupt_support::Interruptee; use std::collections::HashMap; use std::result; @@ -97,7 +97,7 @@ pub fn sync_multiple( storage_init: &Sync15StorageClientInit, root_sync_key: &KeyBundle, interruptee: &dyn Interruptee, - req_info: Option>, + req_info: Option, ) -> SyncResult { sync_multiple_with_command_processor( None, @@ -123,7 +123,7 @@ pub fn sync_multiple_with_command_processor( storage_init: &Sync15StorageClientInit, root_sync_key: &KeyBundle, interruptee: &dyn Interruptee, - req_info: Option>, + req_info: Option, ) -> SyncResult { info!("Syncing {} engines", engines.len()); let mut sync_result = SyncResult { @@ -141,7 +141,7 @@ pub fn sync_multiple_with_command_processor( engines, storage_init, interruptee, - engines_to_state_change: req_info.engines_to_state_change, + engine_choices_model: req_info.engine_choices_model, backoff: backoff.clone(), root_sync_key, result: &mut sync_result, @@ -177,8 +177,8 @@ pub fn sync_multiple_with_command_processor( /// otherwise we won't. It should probably be rethought if it gains many more /// fields. #[derive(Debug, Default)] -pub struct SyncRequestInfo<'a> { - pub engines_to_state_change: Option<&'a HashMap>, +pub struct SyncRequestInfo { + pub engine_choices_model: EngineChoicesModel, pub is_user_action: bool, } @@ -190,7 +190,7 @@ struct SyncMultipleDriver<'info, 'res, 'pgs, 'mcs> { root_sync_key: &'info KeyBundle, interruptee: &'info dyn Interruptee, backoff: BackoffListener, - engines_to_state_change: Option<&'info HashMap>, + engine_choices_model: EngineChoicesModel, result: &'res mut SyncResult, persisted_global_state: &'pgs mut Option, mem_cached_state: &'mcs mut MemoryCachedState, @@ -359,7 +359,7 @@ impl SyncMultipleDriver<'_, '_, '_, '_> { &client_info.client, self.root_sync_key, pgs, - self.engines_to_state_change, + self.engine_choices_model.enabled_changes(), self.interruptee, ); diff --git a/components/sync15/src/clients_engine/engine.rs b/components/sync15/src/clients_engine/engine.rs index dd43b8c28f3..3eaf13df415 100644 --- a/components/sync15/src/clients_engine/engine.rs +++ b/components/sync15/src/clients_engine/engine.rs @@ -410,6 +410,7 @@ mod tests { fxa_device_id: "deviceAAAAAA".into(), device_name: "Laptop".into(), device_type: DeviceType::Desktop, + enabled_engines: vec![], }, outgoing_commands: [ Command::Wipe("bookmarks".into()), @@ -564,6 +565,7 @@ mod tests { fxa_device_id: "deviceAAAAAA".into(), device_name: "Laptop".into(), device_type: DeviceType::Desktop, + enabled_engines: vec![], }, outgoing_commands: [].iter().cloned().collect(), }; @@ -630,6 +632,7 @@ mod tests { fxa_device_id: "deviceAAAAAA".into(), device_name: "Laptop".into(), device_type: DeviceType::Desktop, + enabled_engines: vec![], }, outgoing_commands: [].iter().cloned().collect(), }; @@ -699,6 +702,7 @@ mod tests { fxa_device_id: "deviceAAAAAA".into(), device_name: "Laptop".into(), device_type: DeviceType::Desktop, + enabled_engines: vec![], }, outgoing_commands: HashSet::new(), }; diff --git a/components/sync15/src/clients_engine/mod.rs b/components/sync15/src/clients_engine/mod.rs index 7346712dc77..fc373ee38f5 100644 --- a/components/sync15/src/clients_engine/mod.rs +++ b/components/sync15/src/clients_engine/mod.rs @@ -80,6 +80,8 @@ pub struct Settings { pub device_name: String, /// The type of this client: mobile, tablet, desktop, or other. pub device_type: DeviceType, + /// The engines this client syncs. Not yet published. + pub enabled_engines: Vec, } #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] diff --git a/components/sync15/src/engine_choices.rs b/components/sync15/src/engine_choices.rs new file mode 100644 index 00000000000..18081766670 --- /dev/null +++ b/components/sync15/src/engine_choices.rs @@ -0,0 +1,43 @@ +/* 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 std::collections::HashMap; + +/// How the user's choice of engines to sync is modelled. +#[derive(Debug, Clone, PartialEq, Eq, uniffi::Enum)] +pub enum EngineChoicesModel { + /// The legacy model: engine choices are shared by every device on the + /// account, via meta/global's declined engines. + Account { + /// Which engine states should be *changed* in the "account global" list (for + /// example, if the UI was used to change an engine's state since the last + /// sync). + /// Normally empty - only used for selection changes, does not reflect current actual choices. + enabled_changes: HashMap, + }, + /// Engine choices are specific to each device. Not yet ready for use + /// outside of testing. + Device, +} + +/// The legacy model, with no changes. +impl Default for EngineChoicesModel { + fn default() -> Self { + Self::Account { + enabled_changes: HashMap::new(), + } + } +} + +impl EngineChoicesModel { + /// The changes to make to the account-wide engine choices, if there are any. + pub fn enabled_changes(&self) -> Option<&HashMap> { + match self { + Self::Account { enabled_changes } if !enabled_changes.is_empty() => { + Some(enabled_changes) + } + _ => None, + } + } +} diff --git a/components/sync15/src/lib.rs b/components/sync15/src/lib.rs index a57f9f973bf..79549bf6f83 100644 --- a/components/sync15/src/lib.rs +++ b/components/sync15/src/lib.rs @@ -10,6 +10,8 @@ pub mod bso; pub mod client; // Type to describe device types mod device_type; +// How the user's engine choices are modelled +mod engine_choices; // Types to describe client records mod client_types; // Note that `clients_engine` should probably be in `sync_client`, but let's not make @@ -30,6 +32,7 @@ pub mod telemetry; pub use crate::client_types::{ClientData, RemoteClient}; pub use crate::device_type::DeviceType; +pub use crate::engine_choices::EngineChoicesModel; pub use crate::error::{Error, Result}; #[cfg(feature = "crypto")] pub use enc_payload::EncryptedPayload; diff --git a/components/sync_manager/src/manager.rs b/components/sync_manager/src/manager.rs index c1822997893..40383a78b0a 100644 --- a/components/sync_manager/src/manager.rs +++ b/components/sync_manager/src/manager.rs @@ -154,16 +154,11 @@ impl SyncManager { access_token: params.auth_info.fxa_access_token.clone(), tokenserver_url, }; - let engines_to_change = if params.enabled_changes.is_empty() { - None - } else { - Some(¶ms.enabled_changes) - }; - let settings = Settings { fxa_device_id: params.device_settings.fxa_device_id, device_name: params.device_settings.name, device_type: params.device_settings.kind, + enabled_engines: params.device_settings.enabled_engines, }; let c = SyncClient::new(settings); let result = sync_multiple_with_command_processor( @@ -175,7 +170,7 @@ impl SyncManager { &key_bundle, &interruptee, Some(SyncRequestInfo { - engines_to_state_change: engines_to_change, + engine_choices_model: params.engine_choices_model, is_user_action: matches!(params.reason, SyncReason::User), }), ); @@ -259,7 +254,7 @@ fn backoff_in_effect(next_sync_after: Option, p: &SyncParams) -> boo p.reason ); false - } else if !p.enabled_changes.is_empty() { + } else if p.engine_choices_model.enabled_changes().is_some() { info!("Still under backoff, but syncing because we have enabled state changes."); false } else { @@ -324,6 +319,7 @@ impl CommandProcessor for SyncClient { mod test { use super::*; use crate::types::{DeviceSettings, SyncAuthInfo}; + use sync15::EngineChoicesModel; #[test] fn test_engine_id_sanity() { @@ -336,7 +332,7 @@ mod test { SyncParams { reason: SyncReason::Scheduled, engines: SyncEngineSelection::All, - enabled_changes: HashMap::new(), + engine_choices_model: EngineChoicesModel::default(), local_encryption_keys: HashMap::new(), auth_info: SyncAuthInfo { kid: "kid".to_string(), @@ -349,6 +345,7 @@ mod test { fxa_device_id: "device-id".to_string(), name: "Test Device".to_string(), kind: sync15::DeviceType::Mobile, + enabled_engines: vec![], }, } } diff --git a/components/sync_manager/src/types.rs b/components/sync_manager/src/types.rs index a1e3692e66c..6950d0d935f 100644 --- a/components/sync_manager/src/types.rs +++ b/components/sync_manager/src/types.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use std::time::SystemTime; -use sync15::DeviceType; +use sync15::{DeviceType, EngineChoicesModel}; #[derive(Debug, uniffi::Record)] pub struct SyncParams { @@ -12,10 +12,8 @@ pub struct SyncParams { pub reason: SyncReason, /// Which engines should we sync? pub engines: SyncEngineSelection, - /// Which engines should be enabled in the "account global" list (for - /// example, if the UI was used to change an engine's state since the last - /// sync). - pub enabled_changes: HashMap, + /// How the user's choice of engines to sync is modelled. + pub engine_choices_model: EngineChoicesModel, /// Keys to encrypt/decrypt data from local database files. These are /// separate from the key we use to encrypt the sync payload as a whole. pub local_encryption_keys: HashMap, @@ -61,6 +59,10 @@ pub struct DeviceSettings { pub fxa_device_id: String, pub name: String, pub kind: DeviceType, + /// The engines this device syncs, to be published for other devices on the + /// account to see. Not yet published. + #[uniffi(default)] + pub enabled_engines: Vec, } #[derive(Debug, uniffi::Record)] diff --git a/testing/sync-test/src/auth.rs b/testing/sync-test/src/auth.rs index 1e7eadcb28a..765f487a28c 100644 --- a/testing/sync-test/src/auth.rs +++ b/testing/sync-test/src/auth.rs @@ -11,7 +11,7 @@ use std::collections::{hash_map::RandomState, HashMap}; use std::sync::Arc; use sync15::{ client::{SetupStorageClient, Sync15StorageClient}, - DeviceType, + DeviceType, EngineChoicesModel, }; use sync_manager::{ manager::SyncManager, DeviceSettings, SyncEngineSelection, SyncParams, SyncReason, @@ -102,7 +102,7 @@ impl TestClient { engines: SyncEngineSelection::Some { engines: engines.to_vec(), }, - enabled_changes: HashMap::new(), + engine_choices_model: EngineChoicesModel::default(), local_encryption_keys, auth_info: sync_info.auth_info, persisted_state: self.persisted_state.take(), @@ -110,6 +110,7 @@ impl TestClient { fxa_device_id: self.device.id.clone(), name: self.device.display_name.clone(), kind: self.device.device_type, + enabled_engines: vec![], }, }; let result = self.sync_manager.sync(params)?; @@ -144,7 +145,7 @@ impl TestClient { engines: SyncEngineSelection::Some { engines: engines.to_vec(), }, - enabled_changes: HashMap::new(), + engine_choices_model: EngineChoicesModel::default(), local_encryption_keys, auth_info: sync_info.auth_info, persisted_state: self.persisted_state.take(), @@ -152,6 +153,7 @@ impl TestClient { fxa_device_id: self.device.id.clone(), name: self.device.display_name.clone(), kind: self.device.device_type, + enabled_engines: vec![], }, }; let result = self.sync_manager.sync(params)?;