diff --git a/components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt b/components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt index 8c0de27661d..ef7036655f2 100644 --- a/components/concept/sync/src/main/java/mozilla/components/concept/sync/OAuthAccount.kt +++ b/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. diff --git a/components/feature/accounts/src/main/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeature.kt b/components/feature/accounts/src/main/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeature.kt index b2d6db1610e..3694698fe34 100644 --- a/components/feature/accounts/src/main/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeature.kt +++ b/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. diff --git a/components/feature/accounts/src/test/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeatureTest.kt b/components/feature/accounts/src/test/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeatureTest.kt index 1fb1d592207..b082ab94bfd 100644 --- a/components/feature/accounts/src/test/java/mozilla/components/feature/accounts/FirefoxAccountsAuthFeatureTest.kt +++ b/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() + 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") diff --git a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt b/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt index ecf428647a2..f04e12cb513 100644 --- a/components/service/firefox-accounts/src/main/java/mozilla/components/service/fxa/Types.kt +++ b/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) } } diff --git a/docs/changelog.md b/docs/changelog.md index 0f14a5fbb8e..7eb3dc30160 100644 --- a/docs/changelog.md +++ b/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)