Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FC] Handle process kills after returning from browsers in AuthSessions #6853

Conversation

carlosmuvi-stripe
Copy link
Collaborator

@carlosmuvi-stripe carlosmuvi-stripe commented Jun 9, 2023

Problem

We're not handling process kills while authenticating in the browser:

  • FinancialConnectionsNativeActivity is in the foreground and it launches the browser.
  • The browser takes the foreground, Activity goes to the background.
  • If the system needs resources, it might kill the background Activity.
  • The browser finishes its flow and triggers a redirect back to the Activity.

Solution

  • Save and restore state across process kills using @PersistState (that uses onSaveInstanceState and onRestoreInstanceState internally)

Testing

  • Test the AuthFlow usual cases
  • Enable don't keep activities and test the same cases. Settings -> Developer options -> Don't keep activities
  • Added tests
  • Modified tests
  • Manually verified

Motivation

📔  [Android] Handle process kills after returning from browsers in AuthSessions
🌐  BANKCON-7116

@carlosmuvi-stripe carlosmuvi-stripe added the work-cli Added to pull requests created with #work-cli for usage tracking. label Jun 9, 2023
@carlosmuvi-stripe carlosmuvi-stripe self-assigned this Jun 9, 2023
Comment on lines 370 to 390
sealed class WebAuthFlowState : Parcelable {
@Parcelize
object Uninitialized : WebAuthFlowState()

@Parcelize
object InProgress : WebAuthFlowState()

@Parcelize
data class Success(
val url: String
) : WebAuthFlowState()

@Parcelize
object Canceled : WebAuthFlowState()

@Parcelize
data class Failed(
val message: String,
val reason: String?
) : WebAuthFlowState()
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a Parcelable state that can be saved and restored on process kills via @PersistState and replaced the Async object by this. It should also be simpler (there's no need for an Async object here, a plain sealed class is enough).

That way we can recover the web state pre-process kill and act accordingly.

Before this, we were losing the Web AuthFlow state on process kills.

@carlosmuvi-stripe carlosmuvi-stripe marked this pull request as ready for review June 10, 2023 00:28
@carlosmuvi-stripe carlosmuvi-stripe requested review from a team as code owners June 10, 2023 00:28
@carlosmuvi-stripe carlosmuvi-stripe requested review from tillh-stripe and jaynewstrom-stripe and removed request for awush-stripe June 11, 2023 19:53
Comment on lines +47 to +51
override fun onResume() {
super.onResume()
// prevent playground configuration from leaking to example apps.
connectionsDebugSharedPrefs.edit { clear() }
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change on the Playground in the example app to make it work across process kills -> we override a deeplink based on a shared preference for local testing. We were clearing them on Activity's onDestroy to prevent it from leaking to the official examples just in case.

Process kills kill the activity triggering the preference clear. Moved the clear to the launcher activity instead.

Comment on lines 61 to 70
withState {
if (it.firstInit) {
setState { copy(firstInit = false) }
launchBrowserIfNonOauth()
createAuthSession()
} else {
restoreAuthSession()
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to create a new session if coming from a process kill. We'll restore the session from the manifest instead.

tillh-stripe
tillh-stripe previously approved these changes Jun 12, 2023
import com.airbnb.mvrx.Success
import com.airbnb.mvrx.Uninitialized
import com.stripe.android.financialconnections.model.DataAccessNotice
import com.stripe.android.financialconnections.model.FinancialConnectionsAuthorizationSession
import com.stripe.android.financialconnections.model.FinancialConnectionsInstitution

internal data class PartnerAuthState(
@PersistState
val firstInit: Boolean = true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of persisting firstInit, would it make sense to persist a nullable session ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that better actually! Even if we can't "fetch the auth session by id" I find it more readable as well - c9b74e9

…7116/fc-handle-process-kills-after-returning-from-brows

# Conflicts:
#	financial-connections/src/main/java/com/stripe/android/financialconnections/presentation/FinancialConnectionsSheetNativeViewModel.kt
@carlosmuvi-stripe carlosmuvi-stripe merged commit bc03b7a into master Jun 12, 2023
7 checks passed
@carlosmuvi-stripe carlosmuvi-stripe deleted the carlosmuvi/BANKCON-7116/fc-handle-process-kills-after-returning-from-brows branch June 12, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-cli Added to pull requests created with #work-cli for usage tracking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants