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

[Authenticator] Improve documentation regarding navigation #140

Open
2 tasks done
pmellaaho opened this issue Apr 17, 2024 · 11 comments
Open
2 tasks done

[Authenticator] Improve documentation regarding navigation #140

pmellaaho opened this issue Apr 17, 2024 · 11 comments
Labels
authenticator This issue relates to the Authenticator component question Further information is requested

Comments

@pmellaaho
Copy link

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

@github-actions github-actions bot added the pending-triage Issue is pending triage label Apr 17, 2024
@mattcreaser mattcreaser added question Further information is requested authenticator This issue relates to the Authenticator component labels Apr 17, 2024
@github-actions github-actions bot removed the pending-triage Issue is pending triage label Apr 17, 2024
@mattcreaser
Copy link
Member

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
Copy link
Author

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
Copy link
Member

mattcreaser commented Apr 29, 2024

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
Copy link
Author

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
Copy link
Member

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: aws-amplify/amplify-android#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
Copy link
Author

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

@JOikarinen
Copy link

I've tested scenarios similar to those mentioned by @pmellaaho using AmplifyUIAuthenticator v1.2.2 and AmplifyCore v2.21.0.

Preconditions:

  • Configured a user pool client with an access and ID token expiry of 5 minutes and a refresh token expiry of 1 hour.

Scenario 1:

  1. Sign in to the app.
  2. Enable Airplane Mode (i.e. disconnecting from Wi-Fi and cellular network).
  3. Force close the app.
  4. Wait for 5 minutes.
  5. Reopen the app.

Observed behavior in scenario 1:

  • AmplifyUI forces the user to the sign-in screen.

Scenario 2:

  1. Sign in to the app.
  2. Enable Airplane Mode (i.e. disconnecting from Wi-Fi and cellular network).
  3. Wait for 5 minutes.
  4. Call the isSigned function for the session:
 val session = Amplify.Auth.fetchAuthSession() as AWSCognitoAuthSession
 if(session.isSignedIn) {
      val accessToken =  session.accessToken
       ... do something useful.. 
 }

Observed behavior in scenario 2:

  • The isSignedIn function returns false.

Questions:

  1. Is the behavior observed in Scenario 1 expected for Amplify UI?
  2. Is the behavior observed in Scenario 2 expected for Amplify Core? I ask because the fix in 2783 suggests that isSignedIn might return true in Scenario 2.
  3. If the behaviors in both Scenario 1 and 2 are expected, should we manually log out the user in Scenario 2, given that the session indicates the user is no longer signed in calling:
Amplify.Auth.signOut()

@mattcreaser
Copy link
Member

Hi @JOikarinen. I would say that the scenarios you describe are not expected behaviour. Both of the scenarios are related to the information returned from Amplify, as the logic of whether to show the Sign In screen is driven off the isSignedIn flag from Amplify. I would expect that to have been fixed in Amplify 2.19.1 as per the linked issue aws-amplify/amplify-android#2783.

Just to confirm, when you say you were using "AmplifyCore" 2.21.0, you explicitly added a dependency like the following? The fix was in the Cognito plugin so you'd need this specific dependency.

implementation("com.amplifyframework:aws-auth-cognito:2.21.0")

@JOikarinen
Copy link

Thanks @mattcreaser for the quick response. In my experiment I didn't explicitly add the aws-auth-cognito dependency as you suggested. So, I conducted the experiment using the following approach:

implementation("com.amplifyframework.ui:authenticator:1.2.2") {
        exclude(group = "com.amplifyframework", module = "aws-auth-cognito")
 }
implementation("com.amplifyframework:aws-auth-cognito:2.21.0")
implementation("com.amplifyframework:core-kotlin:2.21.0")

With the dependency setup above, Scenario 2 now returns isSignedIn = true, which I believe is the desired behavior. However, Scenario 1 fails at step 5, where Amplify UI displays a "Something went wrong" screen.

I guess that things might start working correctly once a version of ui:authenticator is released with a dependency on aws-auth-cognito:2.21.0. Do you have any estimate on when this release might be available?

@JOikarinen
Copy link

@mattcreaser ui:authenticator:1.2.3 is dependent on aws-auth-cognito:2.18.0. The mentioned scenarios 1 and 2 above are still valid bugs in ui:authenticator:1.2.3. Do you have an estimate of when a fix will be available? Should I file scenarios 1 and 2 as separate bugs for more clarity?

@JOikarinen
Copy link

I've retested scenarios mentioned above (i.e. scenario 1 and scenario 2) using AmplifyUIAuthenticator v1.3.0 and AmplifyCore v2.21.1.

I can confirm both scenarios now work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
authenticator This issue relates to the Authenticator component question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants