More fixes from review

- Define constants for magic values
- Go back to the old network retry logic
- Removed the require_scoped_key param.  Scoped keys are always required
  for the `oldsync` scope and never for other scopes.
- Added back the `StateV2.device_capabilities` field.  This tracks
  something different than `server_device_info`.  Instead of tracking
  what the server thinks about our device, this tracks the capabilities
  that we know we have.  It's a subtle difference, but
  `device_capabilities` is what we want to use for
  `reregister_capabilities`.
- Added optional metrics parameter for BeginOAuthFlow and
  BeginPairingFlow

components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt

@@ -14,6 +14,18 @@ import kotlin.time.Duration
 import kotlin.time.Duration.Companion.milliseconds
 import kotlin.time.Duration.Companion.seconds
 
+// How many items should our channel hold?  This should be big enough that it will never be filled
+// under normal circumstances, but small enough that it will get filled if the `processChannel()`
+// task fails (and therefore we will see exceptions when we call `trySend` rather than failing
+// silently).
+private const val CHANNEL_SIZE = 100
+
+// How many times should we retry in the face of network errors?
+private const val NETWORK_RETRY_MAX = 3
+
+// How often should we try to recover from auth issues?
+private val AUTH_RECOVERY_PERIOD = 60.seconds
+
 /**
  * Processes actions sent to [FxaClient.queueAction] and sends events to the [FxaEventHandler]
  *
@@ -32,7 +44,7 @@ internal class FxaActionProcessor(
 ) {
     // Set a high bound on the channel so that if our the processChannel() task dies, or is never
     // started, we should eventually see error reports.
-    private val channel = Channel<FxaAction>(100)
+    private val channel = Channel<FxaAction>(CHANNEL_SIZE)
     internal val retryLogic = RetryLogic()
     internal var currentState = initialState
 
@@ -41,8 +53,6 @@ internal class FxaActionProcessor(
 
     // Queue a new action for processing
     fun queue(action: FxaAction) {
-        // trySend allows this function to be non-suspend and will never fail since the channel size
-        // is UNLIMITED
         channel.trySend(action)
     }
 
@@ -66,6 +76,7 @@ internal class FxaActionProcessor(
 
     @Suppress("ComplexMethod")
     internal suspend fun processAction(action: FxaAction) {
+        retryLogic.newActionStarted()
         val isValid = when (action) {
             // Auth flow actions are valid if you're disconnected and also if you're already
             // authenticating.  If a consumer accidentally starts multiple flows we should not
@@ -180,13 +191,13 @@ internal class FxaActionProcessor(
 
     private suspend fun handleBeginOAuthFlow(action: FxaAction.BeginOAuthFlow) {
         handleBeginEitherOAuthFlow(action.result) {
-            inner.beginOauthFlow(action.scopes.toList(), action.entrypoint, MetricsParams(mapOf()))
+            inner.beginOauthFlow(action.scopes.toList(), action.entrypoint, action.metrics ?: MetricsParams(mapOf()))
         }
     }
 
     private suspend fun handleBeginPairingFlow(action: FxaAction.BeginPairingFlow) {
         handleBeginEitherOAuthFlow(action.result) {
-            inner.beginPairingFlow(action.pairingUrl, action.scopes.toList(), action.entrypoint, MetricsParams(mapOf()))
+            inner.beginPairingFlow(action.pairingUrl, action.scopes.toList(), action.entrypoint, action.metrics ?: MetricsParams(mapOf()))
         }
     }
 
@@ -327,22 +338,25 @@ internal class FxaActionProcessor(
 }
 
 internal class RetryLogic {
-    private var lastNetworkRetry: Long = 0
+    private var networkRetryCount = 0
     private var lastAuthCheck: Long = 0
 
     fun shouldRetryAfterNetworkError(): Boolean {
-        val elasped = (System.currentTimeMillis() - lastNetworkRetry).milliseconds
-        if (elasped > 30.seconds) {
-            lastNetworkRetry = System.currentTimeMillis()
+        if (networkRetryCount < NETWORK_RETRY_MAX) {
+            networkRetryCount += 1
             return true
         } else {
             return false
         }
     }
 
+    fun newActionStarted() {
+        networkRetryCount = 0
+    }
+
     fun shouldRecheckAuthStatus(): Boolean {
         val elasped = (System.currentTimeMillis() - lastAuthCheck).milliseconds
-        if (elasped > 60.seconds) {
+        if (elasped > AUTH_RECOVERY_PERIOD) {
             lastAuthCheck = System.currentTimeMillis()
             return true
         } else {
@@ -352,7 +366,6 @@ internal class RetryLogic {
 
     // For testing
     fun fastForward(amount: Duration) {
-        lastNetworkRetry -= amount.inWholeMilliseconds
         lastAuthCheck -= amount.inWholeMilliseconds
     }
 }

components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt

@@ -223,9 +223,8 @@ class FxaClient private constructor(
     suspend fun getAccessToken(
         scope: String,
         ttl: Long? = null,
-        requireScopedKey: Boolean = false,
     ): AccessTokenInfo = wrapMethodCallAndPersist("getAccessToken") {
-        inner.getAccessToken(scope, ttl, requireScopedKey)
+        inner.getAccessToken(scope, ttl)
     }
 
     /**

components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/StateTypes.kt

@@ -39,11 +39,13 @@ sealed class FxaAction {
      * @param scopes OAuth scopes to request
      * @param entrypoint OAuth entrypoint
      * @param result If present, will be completed with the OAuth URL to navigate users too
+     * @param metrics If present, extra metrics trackingquery parameters in the resulting URL
      */
     data class BeginOAuthFlow(
         val scopes: Array<String>,
         val entrypoint: String,
         val result: CompletableDeferred<String?>? = null,
+        val metrics: MetricsParams? = null,
     ) : FxaAction()
 
     /**
@@ -58,12 +60,14 @@ sealed class FxaAction {
      * @param scopes OAuth scopes to request
      * @param entrypoint OAuth entrypoint
      * @param result If present, will be completed with the OAuth URL to navigate users too
+     * @param metrics If present, extra metrics trackingquery parameters in the resulting URL
      */
     data class BeginPairingFlow(
         val pairingUrl: String,
         val scopes: Array<String>,
         val entrypoint: String,
         val result: CompletableDeferred<String?>? = null,
+        val metrics: MetricsParams? = null,
     ) : FxaAction()
 
     /**
@@ -247,6 +251,7 @@ enum class FxaAuthEventKind {
     AUTH_CHECK_STARTED,
     AUTH_CHECK_FAILED,
     AUTH_CHECK_SUCCESS,
+
     // This is sent back when the consumer sends the `LogoutFromAuthIssues` action
     LOGOUT_FROM_AUTH_ISSUES,
 }

components/fxa-client/android/src/test/java/mozilla/appservices/fxaclient/FxaActionProcessorTest.kt

@@ -747,13 +747,15 @@ class FxaActionProcessorTest {
 
 class FxaRetryTest {
     @Test
-    fun `FxaActionProcessor retries after network errors`() = runTest {
+    fun `FxaActionProcessor retries 3 times after network errors`() = runTest {
         val mocks = Mocks.create(FxaAuthState.CONNECTED)
-        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException andThen testLocalDevice
+        every { mocks.firefoxAccount.setDeviceName(any()) } throwsMany listOf(networkException, networkException, networkException) andThen testLocalDevice
 
         mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
             coVerifySequence {
-                // This throws FxaException.Network, we should retry
+                // These throws FxaException.Network, we should retry
+                inner.setDeviceName("My Phone")
+                inner.setDeviceName("My Phone")
                 inner.setDeviceName("My Phone")
                 // This time it work
                 inner.setDeviceName("My Phone")
@@ -763,90 +765,41 @@ class FxaRetryTest {
             }
             FxaAuthState.CONNECTED
         }
-    }
 
-    @Test
-    fun `FxaActionProcessor fails after 2 network errors`() = runTest {
-        val mocks = Mocks.create(FxaAuthState.CONNECTED)
-        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException
+        // Each action gets a fresh retry count.  Test out another action that fails 3 times then
+        // succeeds.
+        every { mocks.firefoxAccount.setDeviceName(any()) } throwsMany listOf(networkException, networkException, networkException) andThen testLocalDevice
 
         mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
             coVerifySequence {
-                // This throws FxaException.Network, we should retry
+                // These throws FxaException.Network, we should retry
                 inner.setDeviceName("My Phone")
-                // This throws again, so the operation fails
                 inner.setDeviceName("My Phone")
-                eventHandler.onFxaEvent(FxaEvent.DeviceOperationFailed(FxaDeviceOperation.SET_DEVICE_NAME))
-            }
-            FxaAuthState.CONNECTED
-        }
-    }
-
-    @Test
-    fun `FxaActionProcessor fails after multiple network errors in a short time`() = runTest {
-        val mocks = Mocks.create(FxaAuthState.CONNECTED)
-        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException andThen testLocalDevice
-
-        mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
-            coVerifySequence {
-                // This fails with FxaException.Network, we should retry
                 inner.setDeviceName("My Phone")
-                // This time it works
+                // This time it work
                 inner.setDeviceName("My Phone")
                 eventHandler.onFxaEvent(
                     FxaEvent.DeviceOperationComplete(FxaDeviceOperation.SET_DEVICE_NAME, testLocalDevice),
                 )
             }
             FxaAuthState.CONNECTED
         }
-
-        mocks.actionProcessor.retryLogic.fastForward(29.seconds)
-        every {
-            mocks.firefoxAccount.setDeviceName(any())
-        } throws networkException andThen testLocalDevice
-
-        mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
-            coVerifySequence {
-                // This throws again and the timeout period is still active, we should fail
-                inner.setDeviceName("My Phone")
-                eventHandler.onFxaEvent(
-                    FxaEvent.DeviceOperationFailed(FxaDeviceOperation.SET_DEVICE_NAME),
-                )
-            }
-            FxaAuthState.CONNECTED
-        }
     }
 
     @Test
-    fun `FxaActionProcessor retrys network errors again after a timeout period`() = runTest {
+    fun `FxaActionProcessor fails after 4 network errors in a row`() = runTest {
         val mocks = Mocks.create(FxaAuthState.CONNECTED)
-        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException andThen testLocalDevice
+        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException
 
         mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
             coVerifySequence {
-                // This fails with FxaException.Network, we should retry
+                // These throws FxaException.Network and we should retry
                 inner.setDeviceName("My Phone")
-                // This time it works
                 inner.setDeviceName("My Phone")
-                eventHandler.onFxaEvent(
-                    FxaEvent.DeviceOperationComplete(FxaDeviceOperation.SET_DEVICE_NAME, testLocalDevice),
-                )
-            }
-            FxaAuthState.CONNECTED
-        }
-
-        mocks.actionProcessor.retryLogic.fastForward(31.seconds)
-        every { mocks.firefoxAccount.setDeviceName(any()) } throws networkException andThen testLocalDevice
-
-        mocks.verifyAction(setDeviceNameAction) { _, inner, eventHandler, _ ->
-            coVerifySequence {
-                // Timeout period over, we should retry this time
                 inner.setDeviceName("My Phone")
-                // This time it works
+                // On the 4th error, we give up
                 inner.setDeviceName("My Phone")
-                eventHandler.onFxaEvent(
-                    FxaEvent.DeviceOperationComplete(FxaDeviceOperation.SET_DEVICE_NAME, testLocalDevice),
-                )
+                eventHandler.onFxaEvent(FxaEvent.DeviceOperationFailed(FxaDeviceOperation.SET_DEVICE_NAME))
             }
             FxaAuthState.CONNECTED
         }
@@ -1158,3 +1111,25 @@ class FxaRetryTest {
         }
     }
 }
+
+class MetricsParamsTest {
+    @Test
+    fun `FxaActionProcessor handles BeginOAuthFlow metrics`() = runTest {
+        val mocks = Mocks.create(FxaAuthState.DISCONNECTED)
+        val testMetrics = MetricsParams(
+            parameters = mapOf("foo" to "bar"),
+        )
+        mocks.actionProcessor.processAction(beginOAuthFlowAction.copy(metrics = testMetrics))
+        coVerify { mocks.firefoxAccount.beginOauthFlow(any(), any(), testMetrics) }
+    }
+
+    @Test
+    fun `FxaActionProcessor handles BeginPairingFlow metrics`() = runTest {
+        val mocks = Mocks.create(FxaAuthState.DISCONNECTED)
+        val testMetrics = MetricsParams(
+            parameters = mapOf("foo" to "bar"),
+        )
+        mocks.actionProcessor.processAction(beginPairingFlowAction.copy(metrics = testMetrics))
+        coVerify { mocks.firefoxAccount.beginPairingFlow(any(), any(), any(), testMetrics) }
+    }
+}

components/fxa-client/src/error.rs

@@ -39,8 +39,8 @@ pub enum FxaError {
     #[error("the requested authentication flow was not active")]
     WrongAuthFlow,
     /// A required scoped key was missing in the server response
-    #[error("A required scoped key was missing")]
-    ScopedKeyMissing,
+    #[error("The sync scoped key was missing")]
+    SyncScopedKeyMissing,
     /// Thrown if there is a panic in the underlying Rust code.
     ///
     /// **Note:** This error is currently only thrown in the Kotlin language bindings.
@@ -106,8 +106,8 @@ pub enum Error {
     #[error("Remote key and local key mismatch")]
     MismatchedKeys,
 
-    #[error("A required scoped key was missing")]
-    ScopedKeyMissing,
+    #[error("The sync scoped key was missing")]
+    SyncScopedKeyMissing,
 
     #[error("Client: {0} is not allowed to request scope: {1}")]
     ScopeNotAllowed(String, String),
@@ -199,8 +199,10 @@ impl GetErrorHandling for Error {
             Error::RequestError(_) => {
                 ErrorHandling::convert(crate::FxaError::Network).log_warning()
             }
-            Error::ScopedKeyMissing => ErrorHandling::convert(crate::FxaError::ScopedKeyMissing)
-                .report_error("fxa-client-scoped-key-missing"),
+            Error::SyncScopedKeyMissing => {
+                ErrorHandling::convert(crate::FxaError::SyncScopedKeyMissing)
+                    .report_error("fxa-client-scoped-key-missing")
+            }
             _ => ErrorHandling::convert(crate::FxaError::Other)
                 .report_error("fxa-client-other-error"),
         }

components/fxa-client/src/fxa_client.udl

@@ -75,8 +75,8 @@ enum FxaError {
   // **Note:** This error is currently only thrown in the Swift language bindings.
   "WrongAuthFlow",
 
-  // A required scoped key was missing in the server response
-  "ScopedKeyMissing",
+  // The sync scoped key was missing in the server response
+  "SyncScopedKeyMissing",
 
   // Thrown if there is a panic in the underlying Rust code.
   //
@@ -583,8 +583,6 @@ interface FirefoxAccount {
   //        - This must be one of the scopes requested during the signin flow.
   //        - Only a single scope is supported; for multiple scopes request multiple tokens.
   //    - `ttl` - optionally, the time for which the token should be valid, in seconds.
-  //    - `require_scoped_key` - optionally, throw [FxaError.ScopedKeyMissing] if the scoped_key
-  //        is not present in the server response.
   //
   // # Notes
   //
@@ -593,7 +591,7 @@ interface FirefoxAccount {
   //      before requesting a fresh token.
   //
   [Throws=FxaError]
-  AccessTokenInfo get_access_token([ByRef] string scope,  optional i64? ttl = null, optional boolean require_scoped_key = false);
+  AccessTokenInfo get_access_token([ByRef] string scope,  optional i64? ttl = null);
 
   // Get the session token for the user's account, if one is available.
   //

components/fxa-client/src/internal/device.rs

@@ -97,6 +97,8 @@ impl FirefoxAccount {
         device_type: DeviceType,
         capabilities: &[DeviceCapability],
     ) -> Result<LocalDevice> {
+        self.state
+            .set_device_capabilities(capabilities.iter().cloned());
         let commands = self.register_capabilities(capabilities)?;
         let update = DeviceUpdateRequestBuilder::new()
             .display_name(name)
@@ -117,6 +119,8 @@ impl FirefoxAccount {
         &mut self,
         capabilities: &[DeviceCapability],
     ) -> Result<LocalDevice> {
+        self.state
+            .set_device_capabilities(capabilities.iter().cloned());
         // Don't re-register if we already have exactly those capabilities.
         if let Some(local_device) = self.state.server_local_device_info() {
             if capabilities == local_device.capabilities {
@@ -132,13 +136,12 @@ impl FirefoxAccount {
 
     /// Re-register the device capabilities, this should only be used internally.
     pub(crate) fn reregister_current_capabilities(&mut self) -> Result<()> {
-        if let Some(local_device) = self.state.server_local_device_info().cloned() {
-            let commands = self.register_capabilities(&local_device.capabilities)?;
-            let update = DeviceUpdateRequestBuilder::new()
-                .available_commands(&commands)
-                .build();
-            self.update_device(update)?;
-        }
+        let capabilities: Vec<_> = self.state.device_capabilities().iter().cloned().collect();
+        let commands = self.register_capabilities(&capabilities)?;
+        let update = DeviceUpdateRequestBuilder::new()
+            .available_commands(&commands)
+            .build();
+        self.update_device(update)?;
         Ok(())
     }