Merge #4360

4360: Closes #4350: Allow for a missing `action` param in FxA auth flows r=jonalmeida a=grigoryk

It's possible that `action` parameter may be missing. This patch adds handling for that case,
and tests for the interceptor (which were missing entirely) that cover all combinations.




Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>

components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt

@@ -80,9 +80,9 @@ sealed class AuthType {
     object Pairing : AuthType()
 
     /**
-     * Account was created for an unknown external reason, identified by [action].
+     * Account was created for an unknown external reason, hopefully identified by [action].
      */
-    data class OtherExternal(val action: String) : AuthType()
+    data class OtherExternal(val action: String?) : AuthType()
 
     /**
      * Account created via a shared account state from another app.

components/feature/accounts/src/main/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeature.kt

@@ -68,7 +68,7 @@ class FirefoxAccountsAuthFeature(
                 val code = parsedUri.getQueryParameter("code")
 
                 if (code != null) {
-                    val authType = parsedUri.getQueryParameter("action")!!.toAuthType()
+                    val authType = parsedUri.getQueryParameter("action").toAuthType()
                     val state = parsedUri.getQueryParameter("state") as String
 
                     // Notify the state machine about our success.

components/feature/accounts/src/test/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeatureTest.kt

@@ -21,9 +21,15 @@ import org.mockito.ArgumentMatchers.anyBoolean
 import org.mockito.ArgumentMatchers.anyString
 import org.mockito.Mockito.`when`
 import androidx.test.ext.junit.runners.AndroidJUnit4
+import mozilla.components.concept.engine.request.RequestInterceptor
 import mozilla.components.concept.sync.AuthFlowUrl
+import mozilla.components.concept.sync.AuthType
 import mozilla.components.service.fxa.DeviceConfig
+import mozilla.components.service.fxa.FxaAuthData
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.mockito.Mockito.never
+import org.mockito.Mockito.verify
 
 // Same as the actual account manager, except we get to control how FirefoxAccountShaped instances
 // are created. This is necessary because due to some build issues (native dependencies not available
@@ -125,6 +131,78 @@ class FirefoxAccountsAuthFeatureTest {
         assertEquals("https://accounts.firefox.com/signin", authLambda.url)
     }
 
+    @Test
+    fun `auth interceptor`() {
+        val manager = mock<FxaAccountManager>()
+        val redirectUrl = "https://accounts.firefox.com/oauth/success/123"
+        val feature = FirefoxAccountsAuthFeature(
+            manager,
+            redirectUrl,
+            mock()
+        ) { _, _ -> }
+
+        // Non-final FxA url.
+        assertNull(feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/not/the/right/url"))
+        verify(manager, never()).finishAuthenticationAsync(any())
+
+        // Non-FxA url.
+        assertNull(feature.interceptor.onLoadRequest(mock(), "https://www.wikipedia.org"))
+        verify(manager, never()).finishAuthenticationAsync(any())
+
+        // Redirect url, without code/state.
+        assertNull(feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/"))
+        verify(manager, never()).finishAuthenticationAsync(any())
+
+        // Redirect url, without code/state.
+        assertNull(feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/test"))
+        verify(manager, never()).finishAuthenticationAsync(any())
+
+        // Code+state, no action.
+        assertEquals(
+            RequestInterceptor.InterceptionResponse.Url(redirectUrl),
+            feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/?code=testCode1&state=testState1")
+        )
+        verify(manager).finishAuthenticationAsync(
+            FxaAuthData(authType = AuthType.OtherExternal(null), code = "testCode1", state = "testState1")
+        )
+
+        // Code+state, action=signin.
+        assertEquals(
+            RequestInterceptor.InterceptionResponse.Url(redirectUrl),
+            feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/?code=testCode2&state=testState2&action=signin")
+        )
+        verify(manager).finishAuthenticationAsync(
+            FxaAuthData(authType = AuthType.Signin, code = "testCode2", state = "testState2")
+        )
+
+        // Code+state, action=signup.
+        assertEquals(
+            RequestInterceptor.InterceptionResponse.Url(redirectUrl),
+            feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/?code=testCode3&state=testState3&action=signup")
+        )
+        verify(manager).finishAuthenticationAsync(
+            FxaAuthData(authType = AuthType.Signup, code = "testCode3", state = "testState3")
+        )
+
+        // Code+state, action=pairing.
+        assertEquals(
+            RequestInterceptor.InterceptionResponse.Url(redirectUrl),
+            feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/?code=testCode4&state=testState4&action=pairing")
+        )
+        verify(manager).finishAuthenticationAsync(
+            FxaAuthData(authType = AuthType.Pairing, code = "testCode4", state = "testState4")
+        )
+
+        // Code+state, action is an unknown value.
+        assertEquals(
+            RequestInterceptor.InterceptionResponse.Url(redirectUrl),
+            feature.interceptor.onLoadRequest(mock(), "https://accounts.firefox.com/oauth/success/123/?code=testCode5&state=testState5&action=someNewActionType")
+        )
+        verify(manager).finishAuthenticationAsync(
+            FxaAuthData(authType = AuthType.OtherExternal("someNewActionType"), code = "testCode5", state = "testState5")
+        )
+    }
+
     private fun prepareAccountManagerForSuccessfulAuthentication(): TestableFxaAccountManager {
         val mockAccount: OAuthAccount = mock()
         val profile = Profile(uid = "testUID", avatar = null, email = "test@example.com", displayName = "test profile")

components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt

@@ -23,12 +23,13 @@ import mozilla.components.concept.sync.SyncAuthInfo
  * Converts a raw 'action' string into an [AuthType] instance.
  * Actions come to us from FxA during an OAuth login, either over the WebChannel or via the redirect URL.
  */
-fun String.toAuthType(): AuthType {
+fun String?.toAuthType(): AuthType {
     return when (this) {
         "signin" -> AuthType.Signin
         "signup" -> AuthType.Signup
         "pairing" -> AuthType.Pairing
-        // We want to gracefully handle 'actions' we don't know about.
+        // We want to gracefully handle an 'action' we don't know about.
+        // This also covers the `null` case.
         else -> AuthType.OtherExternal(this)
     }
 }

docs/changelog.md

@@ -75,6 +75,9 @@ permalink: /changelog/
 * **support-utils**
   * `Intent.asForegroundServicePendingIntent(Context)` extension method to create pending intent for the service that will play nicely with background execution limitations introduced in Android O (e.g. foreground service).
 
+* **concept-sync**
+  * ⚠️ **This is a breaking change**: `action` param of `AuthType.OtherExternal` is now optional. Missing `action` indicates that we really don't know what external authType we've hit.
+
 # 11.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v10.0.0...v11.0.0)