Add EngineChoicesModel, replacing enabled_changes, for per-device sync engine choices

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

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<SyncRequestInfo<'_>>,
+    req_info: Option<SyncRequestInfo>,
 ) -> 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<SyncRequestInfo<'_>>,
+    req_info: Option<SyncRequestInfo>,
 ) -> 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<String, bool>>,
+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<String, bool>>,
+    engine_choices_model: EngineChoicesModel,
     result: &'res mut SyncResult,
     persisted_global_state: &'pgs mut Option<String>,
     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,
         );
 

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(),
         };

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<String>,
 }
 
 #[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]

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<String, bool>,
+    },
+    /// 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<String, bool>> {
+        match self {
+            Self::Account { enabled_changes } if !enabled_changes.is_empty() => {
+                Some(enabled_changes)
+            }
+            _ => None,
+        }
+    }
+}

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;

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(&params.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<SystemTime>, 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![],
             },
         }
     }

components/sync_manager/src/types.rs

@@ -4,18 +4,16 @@
 
 use std::collections::HashMap;
 use std::time::SystemTime;
-use sync15::DeviceType;
+use sync15::{DeviceType, EngineChoicesModel};
 
 #[derive(Debug, uniffi::Record)]
 pub struct SyncParams {
     /// Why are we performing this sync?
     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<String, bool>,
+    /// 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<String, String>,
@@ -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<String>,
 }
 
 #[derive(Debug, uniffi::Record)]

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,14 +102,15 @@ 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(),
             device_settings: DeviceSettings {
                 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,14 +145,15 @@ 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(),
             device_settings: DeviceSettings {
                 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)?;