Commit f5a67f39 authored by Grisha Kruglov's avatar Grisha Kruglov
Browse files

FxA cleanup: remove authErrorRegistry

The sole reason for authErrorRegistry was to expose an instance of FxaAccountManager
to internal components which don't have direct access to it. The registry acted
an internal singleton, but with a bunch of overhead and conceptual complexity around it.

This patch simplifies this: it adds an actual singleton instead of the registry, with a
simple API for components to call into if they encounter authentication errors.

Behaviour of `handleFxaExceptions` also changed slightly, to reduce cognitive overhead:
- instead of calling into an Async function on the observer, and ignoring the result,
this API is now simply `suspend`, which allows us to reason about error handling within
the FxA state machine terms of structured concurrency.

Other cleanup involves marking an expensive OAuthAccount method as async, as well as some
simplification of error handling in FirefoxAccount.
parent 993d8c1d
Loading
Loading
Loading
Loading
+3 −3
Original line number Diff line number Diff line
@@ -86,14 +86,14 @@ interface OAuthAccount : AutoCloseable {
     * @param state the state token string
     * @param accessType the accessType method to be used by the returned code, determines whether
     * the code can be exchanged for a refresh token to be used offline or not
     * @return the authorized auth code string
     * @return Deferred authorized auth code string, or `null` in case of failure.
     */
    fun authorizeOAuthCode(
    fun authorizeOAuthCodeAsync(
        clientId: String,
        scopes: Array<String>,
        state: String,
        accessType: AccessType = AccessType.ONLINE
    ): String?
    ): Deferred<String?>

    /**
     * Fetches the profile object for the current client either from the existing cached state
+2 −2
Original line number Diff line number Diff line
@@ -16,7 +16,7 @@ sealed class SyncStatus {
    /**
     * Sync completed with an error.
     */
    data class Error(val exception: Throwable) : SyncStatus()
    data class Error(val exception: Exception) : SyncStatus()
}

/**
@@ -53,7 +53,7 @@ interface LockableStore : SyncableStore {
     * to key the store.
     * @param block A lambda to execute while the store is unlocked.
     */
    suspend fun <T> unlocked(encryptionKey: String, block: (store: LockableStore) -> T): T
    suspend fun <T> unlocked(encryptionKey: String, block: suspend (store: LockableStore) -> T): T
}

/**
+60 −67
Original line number Diff line number Diff line
@@ -8,16 +8,13 @@ import android.net.Uri
import kotlinx.coroutines.async
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Deferred
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.plus
import mozilla.appservices.fxaclient.FirefoxAccount as InternalFxAcct
import mozilla.components.concept.sync.AccessTokenInfo
import mozilla.components.concept.sync.AccessType
import mozilla.components.concept.sync.AuthFlowUrl
import mozilla.components.concept.sync.DeviceConstellation
import mozilla.components.concept.sync.OAuthAccount
import mozilla.components.concept.sync.Profile
import mozilla.components.concept.sync.StatePersistenceCallback
import mozilla.components.support.base.log.logger.Logger

@@ -100,64 +97,68 @@ class FirefoxAccount internal constructor(
        persistCallback.setCallback(callback)
    }

    override fun beginOAuthFlowAsync(scopes: Set<String>): Deferred<AuthFlowUrl?> {
        return scope.async {
    override fun beginOAuthFlowAsync(scopes: Set<String>) = scope.async {
        handleFxaExceptions(logger, "begin oauth flow", { null }) {
            val url = inner.beginOAuthFlow(scopes.toTypedArray())
            val state = Uri.parse(url).getQueryParameter("state")!!
            AuthFlowUrl(state, url)
        }
    }
    }

    override fun beginPairingFlowAsync(pairingUrl: String, scopes: Set<String>): Deferred<AuthFlowUrl?> {
        return scope.async {
    override fun beginPairingFlowAsync(pairingUrl: String, scopes: Set<String>) = scope.async {
        handleFxaExceptions(logger, "begin oauth pairing flow", { null }) {
            val url = inner.beginPairingFlow(pairingUrl, scopes.toTypedArray())
            val state = Uri.parse(url).getQueryParameter("state")!!
            AuthFlowUrl(state, url)
        }
    }
    }

    override fun getProfileAsync(ignoreCache: Boolean): Deferred<Profile?> {
        return scope.async {
    override fun getProfileAsync(ignoreCache: Boolean) = scope.async {
        handleFxaExceptions(logger, "getProfile", { null }) {
            inner.getProfile(ignoreCache).into()
        }
    }
    }

    override fun getCurrentDeviceId(): String? {
        return handleFxaExceptions(logger, "getCurrentDeviceId", { null }) {
        // This is awkward, yes. Underlying method simply reads some data from in-memory state, and yet it throws
        // in case that data isn't there. See https://github.com/mozilla/application-services/issues/2202.
        return try {
            inner.getCurrentDeviceId()
        } catch (e: FxaPanicException) {
            throw e
        } catch (e: FxaException) {
            null
        }
    }

    override fun authorizeOAuthCode(
    override fun authorizeOAuthCodeAsync(
        clientId: String,
        scopes: Array<String>,
        state: String,
        accessType: AccessType
    ): String? {
        return handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
    ) = scope.async {
        handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
            inner.authorizeOAuthCode(clientId, scopes, state, accessType.msg)
        }
    }

    override fun getSessionToken(): String? {
        return handleFxaExceptions(logger, "getSessionToken", { null }) {
        // This is awkward, yes. Underlying method simply reads some data from in-memory state, and yet it throws
        // in case that data isn't there. See https://github.com/mozilla/application-services/issues/2202.
        return try {
            inner.getSessionToken()
        } catch (e: FxaPanicException) {
            throw e
        } catch (e: FxaException) {
            null
        }
    }

    override fun migrateFromSessionTokenAsync(sessionToken: String, kSync: String, kXCS: String): Deferred<Boolean> {
        return scope.async {
    override fun migrateFromSessionTokenAsync(sessionToken: String, kSync: String, kXCS: String) = scope.async {
        handleFxaExceptions(logger, "migrateFromSessionToken") {
            inner.migrateFromSessionToken(sessionToken, kSync, kXCS)
        }
    }
    }

    override fun getTokenServerEndpointURL(): String {
        return inner.getTokenServerEndpointURL()
@@ -170,23 +171,19 @@ class FirefoxAccount internal constructor(
        return inner.getConnectionSuccessURL()
    }

    override fun completeOAuthFlowAsync(code: String, state: String): Deferred<Boolean> {
        return scope.async {
    override fun completeOAuthFlowAsync(code: String, state: String) = scope.async {
        handleFxaExceptions(logger, "complete oauth flow") {
            inner.completeOAuthFlow(code, state)
        }
    }
    }

    override fun getAccessTokenAsync(singleScope: String): Deferred<AccessTokenInfo?> {
        return scope.async {
    override fun getAccessTokenAsync(singleScope: String) = scope.async {
        handleFxaExceptions(logger, "get access token", { null }) {
            inner.getAccessToken(singleScope).into()
        }
    }
    }

    override fun checkAuthorizationStatusAsync(singleScope: String): Deferred<Boolean?> {
    override fun checkAuthorizationStatusAsync(singleScope: String) = scope.async {
        // fxalib maintains some internal token caches that need to be cleared whenever we
        // hit an auth problem. Call below makes that clean-up happen.
        inner.clearAccessTokenCache()
@@ -195,7 +192,6 @@ class FirefoxAccount internal constructor(
        // Do so by requesting a new access token using an internally-stored "refresh token".
        // Success here means that we're still able to connect - our cached access token simply expired.
        // Failure indicates that we need to re-authenticate.
        return scope.async {
        try {
            inner.getAccessToken(singleScope)
            // We were able to obtain a token, so we're in a good authorization state.
@@ -213,17 +209,14 @@ class FirefoxAccount internal constructor(
        }
        // Re-throw all other exceptions.
    }
    }

    override fun disconnectAsync(): Deferred<Boolean> {
        return scope.async {
    override fun disconnectAsync() = scope.async {
        // TODO can this ever throw FxaUnauthorizedException? would that even make sense? or is that a bug?
        handleFxaExceptions(logger, "disconnect", { false }) {
            inner.disconnect()
            true
        }
    }
    }

    override fun deviceConstellation(): DeviceConstellation {
        return deviceConstellation
+11 −8
Original line number Diff line number Diff line
@@ -4,9 +4,7 @@

package mozilla.components.service.fxa

import mozilla.components.concept.sync.AuthException
import mozilla.components.concept.sync.AuthExceptionType
import mozilla.components.service.fxa.manager.authErrorRegistry
import mozilla.components.service.fxa.manager.GlobalAccountManager
import mozilla.components.support.base.log.logger.Logger

/**
@@ -18,10 +16,10 @@ import mozilla.components.support.base.log.logger.Logger
 * @param handleErrorBlock A lambda to execute if [block] fails with a non-panic, non-auth [FxaException].
 * @return object of type T, as defined by [block].
 */
fun <T> handleFxaExceptions(
suspend fun <T> handleFxaExceptions(
    logger: Logger,
    operation: String,
    block: () -> T,
    block: suspend () -> T,
    postHandleAuthErrorBlock: (e: FxaUnauthorizedException) -> T,
    handleErrorBlock: (e: FxaException) -> T
): T {
@@ -38,7 +36,7 @@ fun <T> handleFxaExceptions(
        when (e) {
            is FxaUnauthorizedException -> {
                logger.warn("Auth error while running: $operation")
                authErrorRegistry.notifyObservers { onAuthErrorAsync(AuthException(AuthExceptionType.UNAUTHORIZED, e)) }
                GlobalAccountManager.authError(e)
                postHandleAuthErrorBlock(e)
            }
            else -> {
@@ -53,14 +51,19 @@ fun <T> handleFxaExceptions(
 * Helper method that handles [FxaException] and allows specifying a lazy default value via [default]
 * block for use in case of errors. Execution is wrapped in log statements.
 */
fun <T> handleFxaExceptions(logger: Logger, operation: String, default: (error: FxaException) -> T, block: () -> T): T {
suspend fun <T> handleFxaExceptions(
    logger: Logger,
    operation: String,
    default: (error: FxaException) -> T,
    block: suspend () -> T
): T {
    return handleFxaExceptions(logger, operation, block, { default(it) }, { default(it) })
}

/**
 * Helper method that handles [FxaException] and returns a [Boolean] success flag as a result.
 */
fun handleFxaExceptions(logger: Logger, operation: String, block: () -> Unit): Boolean {
suspend fun handleFxaExceptions(logger: Logger, operation: String, block: () -> Unit): Boolean {
    return handleFxaExceptions(logger, operation, { false }, {
        block()
        true
+24 −17
Original line number Diff line number Diff line
@@ -16,9 +16,11 @@ import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.async
import kotlinx.coroutines.launch
import kotlinx.coroutines.cancel
import kotlinx.coroutines.withContext
import mozilla.appservices.syncmanager.DeviceSettings
import mozilla.components.concept.sync.AccountObserver
import mozilla.components.concept.sync.AuthException
import mozilla.components.concept.sync.AuthExceptionType
import mozilla.components.concept.sync.DeviceCapability
import mozilla.components.concept.sync.AuthFlowUrl
import mozilla.components.concept.sync.AuthType
@@ -59,17 +61,24 @@ import java.util.concurrent.Executors
import kotlin.coroutines.CoroutineContext

/**
 * A global registry for propagating [AuthException] errors. Components such as [SyncManager] may
 * encounter authentication problems during their normal operation, and
 * this registry is how they inform [FxaAccountManager] that these errors happened.
 *
 * [FxaAccountManager] monitors this registry, adjusts internal state accordingly, and notifies
 * registered [AccountObserver] that account needs re-authentication.
 * Exposes an instance of [FxaAccountManager] for internal consumption; populated during initialization of
 * [FxaAccountManager]. This exists to allow various internal parts without a direct reference to an instance of
 * [FxaAccountManager] to notify it of encountered auth errors.
 */
val authErrorRegistry = ObserverRegistry<AuthErrorObserver>()
internal object GlobalAccountManager {
    private var instance: WeakReference<FxaAccountManager>? = null

    internal fun setInstance(am: FxaAccountManager) {
        instance = WeakReference(am)
    }

interface AuthErrorObserver {
    fun onAuthErrorAsync(e: AuthException): Deferred<Unit>
    internal fun close() {
        instance = null
    }

    internal suspend fun authError(cause: Exception? = null) {
        instance?.get()?.encounteredAuthError(cause)
    }
}

/**
@@ -147,15 +156,8 @@ open class FxaAccountManager(
        oauthObservers.register(fxaOAuthObserver)
    }

    private class FxaAuthErrorObserver(val manager: FxaAccountManager) : AuthErrorObserver {
        override fun onAuthErrorAsync(e: AuthException): Deferred<Unit> {
            return manager.processQueueAsync(Event.AuthenticationError(e))
        }
    }
    private val fxaAuthErrorObserver = FxaAuthErrorObserver(this)

    init {
        authErrorRegistry.register(fxaAuthErrorObserver)
        GlobalAccountManager.setInstance(this)
    }

    private class FxaStatePersistenceCallback(
@@ -507,10 +509,15 @@ open class FxaAccountManager(
    }

    override fun close() {
        GlobalAccountManager.close()
        coroutineContext.cancel()
        account.close()
    }

    internal suspend fun encounteredAuthError(cause: Exception? = null) = withContext(coroutineContext) {
        processQueueAsync(Event.AuthenticationError(AuthException(AuthExceptionType.UNAUTHORIZED, cause))).await()
    }

    /**
     * Pumps the state machine until all events are processed and their side-effects resolve.
     */
Loading