aws-amplify / amplify-ui-android

Amplify UI for Android is a collection of accessible, themeable, performant Android components that can connect directly to the cloud.
https://ui.docs.amplify.aws
Apache License 2.0
13 stars 6 forks source link

[Authenticator] Improve documentation regarding navigation #140

Open pmellaaho opened 3 months ago

pmellaaho commented 3 months ago

Before creating a new issue, please confirm:

Which UI component?

Authenticator

Gradle script dependencies

// Put output below this line
authenticatorVersion = "1.1.0"

Environment information

# Put output below this line
Gradle 8.5

Please include any relevant guides or documentation you're referencing

https://ui.docs.amplify.aws/android/connected-components/authenticator https://ui.docs.amplify.aws/android/connected-components/authenticator/customization#theming https://github.com/aws-amplify/amplify-ui-android/blob/main/samples/authenticator/app/src/main/java/com/amplifyframework/ui/sample/authenticator/MainActivity.kt https://developer.android.com/develop/ui/compose/navigation

Describe the bug

What is the correct way to add Authenticator if you working on a Compose Android App that uses Jetpack Compose Navigation component?

It seems to me that there is conflicting and misleading documentation and code samples in the referred sources. There are basically two approaches that one can take:

  1. Follow the sample code in the Theming documentation In this case the whole application would be wrapped inside Authenticator and the MainActivity code would look something like this: MyApplicationTheme { Authenticator( signInContent = { signInState -> SignInScreen(signInState) } ) { state -> val navController: NavHostController = rememberNavController() MyAppNavHost( authenticatorState = state, navController = navController ) } }

  2. Follow the Authenticator sample code in GitHub In this case we would use the mechanisms available in Compose navigation component and define e.g. AuthenticatorScreen() as a startDestination (= Authenticator.route) in NavHost. AuthenticatorScreen() would look something like this: `@Composable fun AuthenticatorScreen( onShowSignedInContent: () -> Unit ) { Scaffold { padding -> Column( modifier = Modifier .fillMaxSize() .padding(padding) ) { Authenticator( modifier = Modifier.fillMaxWidth(), signInContent = { signInState -> SignInScreen(signInState) } ) { state -> val activity = (LocalContext.current as? Activity) (activity as MainActivity).signedInState = state onShowSignedInContent() } } } }

And passed onShowSignedInContent lambda would do the navigating: onShowSignedInContent = { navController.navigate(Main.route) }

In this case we have store signedInState somewhere because it can’t be passed as navigation argument. Also, the screen containing the SignOut -action would receive the following lambda as parameter: onSignOutClick = { (activity as MainActivity).signedInState?.signOut() navController.navigateSingleTopTo(Authenticator.route) }`

I started experimenting with the 1) but noticed that there is a problem because Authenticator is not placed inside e.g. Scaffold that applies the correct colours from theme and I can’t really do that when using Compose navigation i.e. it’s included in Screen level composables. I then moved to 2) approach which seems to work but are there caveats in the way that you’re aware of? Also, there should be an API (e.g. Amplify.Auth.fetchCurrentState()) to fetch the current state to avoid need to store it in way shown above code.

So, the question is, which approach is the recommended one?

Reproduction steps (if applicable)

No response

Code Snippet

// Put your code below this line.

Log output

// Put your logs below this line

amplifyconfiguration.json

No response

Additional information and screenshots

No response

mattcreaser commented 3 months ago

Hey @pmellaaho thanks for filing your question!

Authenticator is not overly opinionated about how it is integrated into your application, so there is no single recommended approach, as it will be different for different use cases.

Option 1 is certainly the easiest way. I'm not sure I fully understand your issue (colors should be read from the theme without issue) but if there are wrappers that you need that are only applied at screen level then I would agree this isn't quite what you want to do.

One thing to note is that you don't really need to pass around the SignedInState. That object holds the AuthUser and provides a convenience method for signing out. You can get these where you need them without requiring the state object by talking directly to Amplify. The specific calls are Amplify.Auth.getCurrentUser() and Amplify.Auth.signOut(), respectively.

If you do want to access the SignedInState from outside the Authenticator's content composable then I would recommend hoisting the AuthenticatorState and reading it from there. This is a more conventional Compose way to get the current state elsewhere in your hierarchy. Here's a simple example:

@Composable
fun MyApplication() {
    val authenticatorState = rememberAuthenticatorState()
    val stepState = authenticatorState.stepState

    if (stepState is SignedInState) {
        MyApplicationContent(stepState)
    } else {
        AuthenticatorScreen(authenticatorState)
    }
}

@Composable
fun AuthenticatorScreen(authenticatorState: AuthenticatorState) {
    Authenticator(state = authenticatorState) {
        // no need to show content here, Authenticator only visible when stepState != SignedInState
    }
}

Hope that helps!

pmellaaho commented 3 months ago

Thank you for very fast response.

The issue on option 1 is that it's not enough to just place the Authenticator under application theme and expect the Authenticator to use it. So, something like this is needed to make it work: MyApplicationTheme { Surface(modifier = Modifier.fillMaxSize()) { Authenticator( signInContent = { signInState -> SignInScreen(signInState) } ) { state -> val navController: NavHostController = rememberNavController() MyAppNavHost( authenticatorState = state, navController = navController ) } } }

This is because Surface uses default values (colour, content colour) from theme to apply correct colours. This is just a small detail but maybe you could fix it in Theming chapter I was referring to.

Option 2 might be a bit more appealing because we would be using the standard Jetpack Compose Navigation way of doing things and even use transitions. I however have doubts because I think that Authenticator might not be able to do it's magic if it's only present in the one navigation target? For example, is it able to renew the access token automatically or take user to SignIn if tokens have expired. Maybe I would indeed need to observe AuthenticatorState as shown in your code above and do manual navigation to AuthenticatorScreen when needed.

mattcreaser commented 2 months ago

Thank you for the clarification, I understand better now. Authenticator does not assume how to best fit into your theme, this is intentional. I'll make a note to tweak the documentation appropriately.

For your second point, the Authenticator composable itself is strictly a UI layer, and does not handle any behavioural logic vis-a-vis the user's session. Token refreshes are handled by the underlying Amplify Auth library, which works whether or not Authenticator is in the composition hierarchy.

Furthermore, as long as AuthenticatorState remains in the hierarchy, it will respond to the user's signed in status - again, independently of whether the Authenticator UI is present or not. This is why you can have Authenticator under a navigation target as long as the AuthenticatorState is hoisted up higher.

Alternatively, you can even instead choose to listen directly to Amplify Auth Events yourself instead of using AuthenticatorState, as this is what that class is doing internally.

pmellaaho commented 2 months ago

Thank you for the advice, I think it's great idea to listen to Auth events directly!

I continued experimenting with the both approaches discussed above but I think they both suffer from same issues. Let's imagine the following user story:

  1. User does signIn and puts the App on background and device on in flight mode.
  2. User brings the App to foreground after the Access token has expired. Amplify is not able to use the Refresh token to renew the Access token so user should be taken to SignIn view immediately but that happens only if App is stopped and restarted.
  3. After App is restarted and SignIn view is shown, the App is put on background again.
  4. Device is set back to normal mode and App is brought to foreground. Amplify should be now able to refresh the Access token and let user in directly as the Refresh token is still valid. However, this happens only when App is stopped and restarted.

Do you agree that my findings indicate that there is a problem and I should file another issue for it?

mattcreaser commented 2 months ago

Hey @pmellaaho. I think in the scenario you described the actual issue is in step 3, you should not be shown the SignIn view if there is still a valid refresh token. The user should still be considered "signed in" in this situation because, once connectivity is restored, they will be able to refresh their access token and continue with their session.

This appears to be a bug in the Amplify Auth library, and there is already an outstanding GitHub issue open for it: https://github.com/aws-amplify/amplify-android/issues/2783. I would recommend following that issue for updates, and I'll ping the team to see if we can make some progress there.

pmellaaho commented 2 months ago

Ok, this is good to know and thanks for letting them know that I'm also interested in getting this resolved!