Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Commit

Permalink
For fenix#16232 - Make sure we cancel any in-progress auth
Browse files Browse the repository at this point in the history
Prior to #7856 we only had a NotAuthenticated
state, which user would remain in until the very end of the authentication process (receiving a 'finished...' api call).

After the refactor, we introduced an in-progress states - specifically, BeginningAuthentication. If the user
cancels the auth flow (e.g. closes a custom tab), they will remain in this state. Subsequent login attempts
would then fail.

This patch fixes this by introducing a Cancel event which we trigger whenever handling 'beginAuthentication' calls.
In normal scenarios, this event is ignored. Otherwise, it "resets" the state machine into NotAuthenticated
state before emitting any Begin* events.
  • Loading branch information
Grisha Kruglov authored and st3fan committed Nov 14, 2020
1 parent 0a2993d commit e239163
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,12 @@ open class FxaAccountManager(
* @return An authentication url which is to be presented to the user.
*/
suspend fun beginAuthentication(pairingUrl: String? = null): String? {
// It's possible that at this point authentication is considered to be "in-progress".
// For example, if user started authentication flow, but cancelled it (closing a custom tab)
// without finishing.
// In a clean scenario (no prior auth attempts), this event will be ignored by the state machine.
processQueue(Event.Progress.CancelAuth)

val event = if (pairingUrl != null) {
Event.Account.BeginPairingFlow(pairingUrl)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ internal sealed class Event {
object FailedToCompleteAuthRestore : Progress()
object FailedToCompleteAuth : Progress()

object CancelAuth : Progress()

object FailedToRecoverFromAuthenticationProblem : Progress()
object RecoveredFromAuthenticationProblem : Progress()

Expand Down Expand Up @@ -110,6 +112,7 @@ internal fun State.next(event: Event): State? = when (this) {
ProgressState.BeginningAuthentication -> when (event) {
is Event.Progress.AuthData -> State.Active(ProgressState.CompletingAuthentication)
Event.Progress.FailedToBeginAuth -> State.Idle(AccountState.NotAuthenticated)
Event.Progress.CancelAuth -> State.Idle(AccountState.NotAuthenticated)
else -> null
}
ProgressState.CompletingAuthentication -> when (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,49 @@ class FxaAccountManagerTest {
assertEquals(profile, manager.accountProfile())
}

@Test
fun `repeated unfinished authentication attempts succeed`() = runBlocking {
val mockAccount: OAuthAccount = mock()
val constellation: DeviceConstellation = mock()
`when`(mockAccount.deviceConstellation()).thenReturn(constellation)
val profile = Profile(uid = "testUID", avatar = null, email = "test@example.com", displayName = "test profile")
val accountStorage = mock<AccountStorage>()
val accountObserver: AccountObserver = mock()
val manager = prepareHappyAuthenticationFlow(mockAccount, profile, accountStorage, accountObserver, this.coroutineContext)

// We start off as logged-out, but the event won't be called (initial default state is assumed).
verify(accountObserver, never()).onLoggedOut()
verify(accountObserver, never()).onAuthenticated(any(), any())

// Begin auth for the first time.
reset(accountObserver)
assertEquals("auth://url", manager.beginAuthentication(pairingUrl = "auth://pairing"))
assertNull(manager.authenticatedAccount())
assertNull(manager.accountProfile())

// Now, try to begin again before finishing the first one.
assertEquals("auth://url", manager.beginAuthentication(pairingUrl = "auth://pairing"))
assertNull(manager.authenticatedAccount())
assertNull(manager.accountProfile())

// The rest should "just work".
`when`(mockAccount.getCurrentDeviceId()).thenReturn("testDeviceId")
`when`(mockAccount.deviceConstellation()).thenReturn(constellation)
`when`(constellation.finalizeDevice(any(), any())).thenReturn(ServiceResult.Ok)

assertTrue(manager.finishAuthentication(FxaAuthData(AuthType.Signin, "dummyCode", EXPECTED_AUTH_STATE)))

verify(accountStorage, times(1)).read()
verify(accountStorage, never()).clear()

verify(accountObserver, times(1)).onAuthenticated(mockAccount, AuthType.Signin)
verify(accountObserver, times(1)).onProfileUpdated(profile)
verify(accountObserver, never()).onLoggedOut()

assertEquals(mockAccount, manager.authenticatedAccount())
assertEquals(profile, manager.accountProfile())
}

@Test
fun `unhappy authentication flow`() = runBlocking {
val accountStorage = mock<AccountStorage>()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class StateKtTest {
}
ProgressState.BeginningAuthentication -> when (event) {
Event.Progress.FailedToBeginAuth -> State.Idle(AccountState.NotAuthenticated)
Event.Progress.CancelAuth -> State.Idle(AccountState.NotAuthenticated)
is Event.Progress.AuthData -> State.Active(ProgressState.CompletingAuthentication)
else -> null
}
Expand Down Expand Up @@ -79,6 +80,7 @@ class StateKtTest {
"Start" -> Event.Account.Start
"BeginPairingFlow" -> Event.Account.BeginPairingFlow("http://some.pairing.url.com")
"BeginEmailFlow" -> Event.Account.BeginEmailFlow
"CancelAuth" -> Event.Progress.CancelAuth
"AuthenticationError" -> Event.Account.AuthenticationError("fxa op")
"MigrateFromAccount" -> Event.Account.MigrateFromAccount(mock(), true)
"RetryMigration" -> Event.Account.RetryMigration
Expand Down

0 comments on commit e239163

Please sign in to comment.