Skip to content

Commit

Permalink
Updated the Fxa states
Browse files Browse the repository at this point in the history 
Rust:
  - Made AuthIssues it's own state and not just a "sub-state" of Disconnected.
  - Split up the [StateV2.start_over] into [StateManager.disconnect] and
    [StateManager.logout_from_auth_issues].
Kotlin:
  - Flattened out FxaAuthState so that each possible state is just an
    object with no fields
  - Renamed `AuthStateChanged` to `AuthEvent`.  I think this is a better
    name because the auth state doesn't always change.
  - Added support for the AUTH_ISSUES state and logoutFromAuthIssues
  - Updated the unit tests to test all possible actions from all
    possible states.
  - Handled some corner cases better in FxaActionProcessor, for example
    if one OAuth flow is in-progress and a second BeginOAuthFlow action
    fails, we now stay in the AUTHENTICATING state rather than go back
    to DISCONNECTED.
  • Loading branch information
@bendk
bendk committed Oct 5, 2023
1 parent b034bdb commit 816a9cd
Show file tree
Hide file tree
Showing 10 changed files with 562 additions and 527 deletions.
140 changes: 85 additions & 55 deletions ...ents/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaActionProcessor.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,29 +66,46 @@ internal class FxaActionProcessor(

@Suppress("ComplexMethod")
internal suspend fun processAction(action: FxaAction) {
val currentState = currentState
// Match on the current state since many actions are only valid from a particular state
when (currentState) {
is FxaAuthState.Connected -> when (action) {
is FxaAction.Disconnect -> handleDisconnect(action)
FxaAction.CheckAuthorization -> handleCheckAuthorization(currentState)
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
// create extra issues for them.
is FxaAction.BeginOAuthFlow,
is FxaAction.BeginPairingFlow,
is FxaAction.CompleteOAuthFlow,
is FxaAction.CancelOAuthFlow,
-> currentState in listOf(FxaAuthState.DISCONNECTED, FxaAuthState.AUTHENTICATING)
// These actions require the user to be connected
FxaAction.CheckAuthorization,
is FxaAction.InitializeDevice,
is FxaAction.EnsureCapabilities,
is FxaAction.SetDeviceName,
is FxaAction.SetDevicePushSubscription,
is FxaAction.SendSingleTab,
-> currentState.isConnected()
// These are always valid, although they're no-op if you're already in the
// DISCONNECTED/AUTH_ISSUES state
FxaAction.Disconnect,
FxaAction.LogoutFromAuthIssues,
-> true
}
if (isValid) {
when (action) {
is FxaAction.BeginOAuthFlow -> handleBeginOAuthFlow(action)
is FxaAction.BeginPairingFlow -> handleBeginPairingFlow(action)
is FxaAction.CompleteOAuthFlow -> handleCompleteFlow(action)
is FxaAction.CancelOAuthFlow -> handleCancelFlow()
is FxaAction.InitializeDevice -> handleInitializeDevice(action)
is FxaAction.EnsureCapabilities -> handleEnsureCapabilities(action)
is FxaAction.SetDeviceName -> handleSetDeviceName(action)
is FxaAction.SetDevicePushSubscription -> handleSetDevicePushSubscription(action)
is FxaAction.SendSingleTab -> handleSendSingleTab(action)
else -> Log.e(LOG_TAG, "Invalid $action (state: $currentState)")
}
is FxaAuthState.Disconnected -> when (action) {
is FxaAction.BeginOAuthFlow -> handleBeginOAuthFlow(currentState, action)
is FxaAction.BeginPairingFlow -> handleBeginPairingFlow(currentState, action)
is FxaAction.CompleteOAuthFlow -> handleCompleteFlow(currentState, action)
is FxaAction.CancelOAuthFlow -> handleCancelFlow(currentState)
// If we see Disconnect or CheckAuthorization from the Disconnected state, just ignore it
FxaAction.CheckAuthorization -> Unit
is FxaAction.Disconnect -> Unit
else -> Log.e(LOG_TAG, "Invalid $action (state: $currentState)")
FxaAction.CheckAuthorization -> handleCheckAuthorization()
FxaAction.Disconnect -> handleDisconnect()
FxaAction.LogoutFromAuthIssues -> handleLogoutFromAuthIssues()
}
} else {
Log.e(LOG_TAG, "Invalid $action (state: $currentState)")
}
}

Expand All @@ -102,10 +119,12 @@ internal class FxaActionProcessor(
}
}

private suspend fun changeState(newState: FxaAuthState, transition: FxaAuthStateTransition) {
Log.d(LOG_TAG, "Changing state from $currentState to $newState")
currentState = newState
sendEvent(FxaEvent.AuthStateChanged(newState, transition))
private suspend fun sendAuthEvent(kind: FxaAuthEventKind, newState: FxaAuthState?) {
if (newState != null && newState != currentState) {
Log.d(LOG_TAG, "Changing state from $currentState to $newState")
currentState = newState
}
sendEvent(FxaEvent.AuthEvent(kind, currentState))
}

// Perform an operation, retrying after network errors
Expand Down Expand Up @@ -136,23 +155,21 @@ internal class FxaActionProcessor(
try {
return withNetworkRetry(operation)
} catch (e: FxaException.Authentication) {
val currentState = currentState

if (currentState !is FxaAuthState.Connected) {
if (!currentState.isConnected()) {
throw e
}

if (retryLogic.shouldRecheckAuthStatus()) {
Log.d(LOG_TAG, "Auth error: re-checking")
handleCheckAuthorization(currentState)
handleCheckAuthorization()
} else {
Log.d(LOG_TAG, "Auth error: disconnecting")
inner.disconnectFromAuthIssues()
inner.logoutFromAuthIssues()
persistState()
changeState(FxaAuthState.Disconnected(true), FxaAuthStateTransition.AUTH_CHECK_FAILED)
sendAuthEvent(FxaAuthEventKind.AUTH_CHECK_FAILED, FxaAuthState.AUTH_ISSUES)
}

if (this.currentState is FxaAuthState.Connected) {
if (currentState.isConnected()) {
continue
} else {
throw e
Expand All @@ -161,49 +178,53 @@ internal class FxaActionProcessor(
}
}

private suspend fun handleBeginOAuthFlow(currentState: FxaAuthState.Disconnected, action: FxaAction.BeginOAuthFlow) {
handleBeginEitherOAuthFlow(currentState, action.result) {
private suspend fun handleBeginOAuthFlow(action: FxaAction.BeginOAuthFlow) {
handleBeginEitherOAuthFlow(action.result) {
inner.beginOauthFlow(action.scopes.toList(), action.entrypoint, MetricsParams(mapOf()))
}
}

private suspend fun handleBeginPairingFlow(currentState: FxaAuthState.Disconnected, action: FxaAction.BeginPairingFlow) {
handleBeginEitherOAuthFlow(currentState, action.result) {
private suspend fun handleBeginPairingFlow(action: FxaAction.BeginPairingFlow) {
handleBeginEitherOAuthFlow(action.result) {
inner.beginPairingFlow(action.pairingUrl, action.scopes.toList(), action.entrypoint, MetricsParams(mapOf()))
}
}

private suspend fun handleBeginEitherOAuthFlow(currentState: FxaAuthState.Disconnected, result: CompletableDeferred<String?>?, operation: () -> String) {
private suspend fun handleBeginEitherOAuthFlow(result: CompletableDeferred<String?>?, operation: () -> String) {
try {
val url = withRetry { operation() }
persistState()
changeState(currentState.copy(connecting = true), FxaAuthStateTransition.OAUTH_STARTED)
sendAuthEvent(FxaAuthEventKind.OAUTH_STARTED, FxaAuthState.AUTHENTICATING)
sendEvent(FxaEvent.BeginOAuthFlow(url))
result?.complete(url)
} catch (e: FxaException) {
Log.e(LOG_TAG, "Exception when handling BeginOAuthFlow", e)
persistState()
changeState(currentState.copy(connecting = false), FxaAuthStateTransition.OAUTH_FAILED_TO_BEGIN)
// Stay in the AUTHENTICATING if we were in that state , since there may be another
// oauth flow in progress. We only switch to DISCONNECTED if we see CancelOAuthFlow.
sendAuthEvent(FxaAuthEventKind.OAUTH_FAILED_TO_BEGIN, null)
result?.complete(null)
}
}

private suspend fun handleCompleteFlow(currentState: FxaAuthState.Disconnected, action: FxaAction.CompleteOAuthFlow) {
private suspend fun handleCompleteFlow(action: FxaAction.CompleteOAuthFlow) {
try {
withRetry { inner.completeOauthFlow(action.code, action.state) }
persistState()
changeState(FxaAuthState.Connected(), FxaAuthStateTransition.OAUTH_COMPLETE)
sendAuthEvent(FxaAuthEventKind.OAUTH_COMPLETE, FxaAuthState.CONNECTED)
} catch (e: FxaException) {
persistState()
Log.e(LOG_TAG, "Exception when handling CompleteOAuthFlow", e)
changeState(currentState, FxaAuthStateTransition.OAUTH_FAILED_TO_COMPLETE)
// Stay in the AUTHENTICATING, since there may be another oauth flow in progress. We
// only switch to DISCONNECTED if we see CancelOAuthFlow.
sendAuthEvent(FxaAuthEventKind.OAUTH_FAILED_TO_COMPLETE, null)
}
}

private suspend fun handleCancelFlow(currentState: FxaAuthState.Disconnected) {
private suspend fun handleCancelFlow() {
// No need to call an inner method or persist the state, since the connecting flag is
// handled soley in this layer
changeState(currentState.copy(connecting = false), FxaAuthStateTransition.OAUTH_CANCELLED)
// handled in this layer only.
sendAuthEvent(FxaAuthEventKind.OAUTH_CANCELLED, FxaAuthState.DISCONNECTED)
}

private suspend fun handleInitializeDevice(action: FxaAction.InitializeDevice) {
Expand Down Expand Up @@ -257,20 +278,29 @@ internal class FxaActionProcessor(
}
}

private suspend fun handleDisconnect(action: FxaAction.Disconnect) {
if (action.fromAuthIssues) {
inner.disconnectFromAuthIssues()
persistState()
changeState(FxaAuthState.Disconnected(fromAuthIssues = true), FxaAuthStateTransition.AUTH_CHECK_FAILED)
} else {
inner.disconnect()
persistState()
changeState(FxaAuthState.Disconnected(), FxaAuthStateTransition.DISCONNECTED)
private suspend fun handleDisconnect() {
if (currentState == FxaAuthState.DISCONNECTED) {
return
}
inner.disconnect()
persistState()
sendAuthEvent(FxaAuthEventKind.DISCONNECTED, FxaAuthState.DISCONNECTED)
}

private suspend fun handleLogoutFromAuthIssues() {
if (currentState in listOf(FxaAuthState.AUTH_ISSUES, FxaAuthState.DISCONNECTED)) {
return
}
inner.logoutFromAuthIssues()
persistState()
sendAuthEvent(FxaAuthEventKind.LOGOUT_FROM_AUTH_ISSUES, FxaAuthState.AUTH_ISSUES)
}

private suspend fun handleCheckAuthorization(currentState: FxaAuthState.Connected) {
changeState(currentState.copy(authCheckInProgress = true), FxaAuthStateTransition.AUTH_CHECK_STARTED)
private suspend fun handleCheckAuthorization() {
if (currentState in listOf(FxaAuthState.DISCONNECTED, FxaAuthState.AUTH_ISSUES)) {
return
}
sendAuthEvent(FxaAuthEventKind.AUTH_CHECK_STARTED, FxaAuthState.CHECKING_AUTH)
val success = try {
val status = withNetworkRetry { inner.checkAuthorizationStatus() }
status.active
Expand All @@ -287,11 +317,11 @@ internal class FxaActionProcessor(
}
if (success) {
persistState()
changeState(currentState.copy(authCheckInProgress = false), FxaAuthStateTransition.AUTH_CHECK_SUCCESS)
sendAuthEvent(FxaAuthEventKind.AUTH_CHECK_SUCCESS, FxaAuthState.CONNECTED)
} else {
inner.disconnectFromAuthIssues()
inner.logoutFromAuthIssues()
persistState()
changeState(FxaAuthState.Disconnected(true), FxaAuthStateTransition.AUTH_CHECK_FAILED)
sendAuthEvent(FxaAuthEventKind.AUTH_CHECK_FAILED, FxaAuthState.AUTH_ISSUES)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/FxaClient.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ class FxaClient private constructor(
/**
* Saves the current account's authentication state as a JSON string, for persistence in
* the Android KeyStore/shared preferences. The authentication state can be restored using
* [FirefoxAccount.fromJSONString].
* [FirefoxAccount.fromJson].
*
* FIXME: https://github.com/mozilla/application-services/issues/5819
*
Expand Down
75 changes: 41 additions & 34 deletions components/fxa-client/android/src/main/java/mozilla/appservices/fxaclient/StateTypes.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ sealed class FxaAction {
/**
* Begin an OAuth flow
*
* BeginOAuthFlow can be sent in either the DISCONNECTED or AUTHENTICATING state. If the
* operation fails the state will not change. AuthEvent(OAUTH_FAILED_TO_BEGIN) will be sent to
* the event handler, which can respond by sending CancelOAuthFlow if the application wants to
* move back to the DISCONNECTED state.
*
* @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
Expand All @@ -44,6 +49,11 @@ sealed class FxaAction {
/**
* Begin an OAuth flow using a paring code URL
*
* BeginPairingFlow can be sent in either the DISCONNECTED or AUTHENTICATING state. If the
* operation fails the state will not change. AuthEvent(OAUTH_FAILED_TO_BEGIN) will be sent to
* the event handler, which can respond by sending CancelOAuthFlow if the application wants to
* move back to the DISCONNECTED state.
*
* @param pairingUrl the url to initialize the paring flow with
* @param scopes OAuth scopes to request
* @param entrypoint OAuth entrypoint
Expand Down Expand Up @@ -143,12 +153,16 @@ sealed class FxaAction {

/**
* Disconnect from the FxA server and destroy our device record.
*/
object Disconnect : FxaAction()

/**
* Logout because of authentication / authorization issues
*
* @param fromAuthIssues: are we disconnecting because of auth issues? Setting this flag
* changes `FxaEvent.AuthStateChanged` so that the `fromAuthIssues` flag is will set and the
* transition is `AUTH_CHECK_FAILED`
* Send this action if the user has gotten into a unathorized state without logging out, for
* example because of a password reset. The sure will need to re-authenticate to login.
*/
data class Disconnect(val fromAuthIssues: Boolean = false) : FxaAction()
object LogoutFromAuthIssues : FxaAction()

/**
* Check the FxA authorization status.
Expand All @@ -164,11 +178,15 @@ sealed class FxaAction {
*/
sealed class FxaEvent {
/**
* Called when the auth state changes. Applications should use this to update their UI.
* Sent on login, logout, auth checks, etc. See [FxaAuthEventKind] for a list of all auth events.
* `state` is the current auth state of the client. All auth state transitions are accompanied
* by an AuthEvent, although some AuthEvents don't correspond to a state transition.
*
* Applications should use this to update their UI.
*/
data class AuthStateChanged(
val newState: FxaAuthState,
val transition: FxaAuthStateTransition,
data class AuthEvent(
val kind: FxaAuthEventKind,
val state: FxaAuthState,
) : FxaEvent()

/**
Expand Down Expand Up @@ -196,43 +214,30 @@ sealed class FxaEvent {
/**
* Kotlin authorization state class
*
* This is [FxaRustAuthState] with added data that Rust doesn't track yet.
* This is [FxaRustAuthState] with added states that Rust doesn't track yet.
*/
sealed class FxaAuthState {
/**
* Client has disconnected
*
* @property fromAuthIssues client was disconnected because of invalid auth tokens, for
* example because of a password reset on another device
* @property connecting is there an OAuth flow in progress?
*/
data class Disconnected(
val fromAuthIssues: Boolean = false,
val connecting: Boolean = false,
) : FxaAuthState()
enum class FxaAuthState {
DISCONNECTED,
AUTHENTICATING,
CONNECTED,
CHECKING_AUTH,
AUTH_ISSUES,
;

/**
* Client is currently connected
*
* @property authCheckInProgress Client is checking the auth tokens and may disconnect soon
*/
data class Connected(
val authCheckInProgress: Boolean = false,
) : FxaAuthState()
fun isConnected() = (this == CONNECTED)

companion object {
fun fromRust(authState: FxaRustAuthState): FxaAuthState {
return when (authState) {
is FxaRustAuthState.Connected -> FxaAuthState.Connected()
is FxaRustAuthState.Disconnected -> {
FxaAuthState.Disconnected(authState.fromAuthIssues)
}
FxaRustAuthState.CONNECTED -> FxaAuthState.CONNECTED
FxaRustAuthState.DISCONNECTED -> FxaAuthState.DISCONNECTED
FxaRustAuthState.AUTH_ISSUES -> FxaAuthState.AUTH_ISSUES
}
}
}
}

enum class FxaAuthStateTransition {
enum class FxaAuthEventKind {
OAUTH_STARTED,
OAUTH_COMPLETE,
OAUTH_CANCELLED,
Expand All @@ -242,6 +247,8 @@ enum class FxaAuthStateTransition {
AUTH_CHECK_STARTED,
AUTH_CHECK_FAILED,
AUTH_CHECK_SUCCESS,
// This is sent back when the consumer sends the `LogoutFromAuthIssues` action
LOGOUT_FROM_AUTH_ISSUES,
}

enum class FxaDeviceOperation {
Expand Down
Loading

0 comments on commit 816a9cd

Please sign in to comment.